fix(ENG-14445): Fixed the package analytics endpoint generation by mcp, included v2 in it

[agupta-cruiser]

Description

The Cloudsmith MCP server was returning 404 for every v2 backed tool, even when authentication was valid and the workspace slug was correct. This first surfaced with a customer using analytics_logs_package_list, but it affected all v2 endpoints.

The root cause is in how the server builds request URLs. It generates tools dynamically from Cloudsmith's OpenAPI specs and builds each request URL by joining the API host with the path taken from the spec. The version prefix (/v1 or /v2) is not part of the path keys though. It is declared separately: the v2 spec puts it in the servers section (https://api.cloudsmith.io/v2/) and the v1 spec puts it in basePath. The server was ignoring that, so a v2 tool was sent to https://api.cloudsmith.io/analytics/logs/package/{workspace}/ instead of https://api.cloudsmith.io/v2/analytics/logs/package/{workspace}/, which 404s.

v1 tools were never affected because the v1 API also answers at the un-versioned root, so dropping the prefix was harmless for v1. The v2 API only answers under /v2/, so every v2 tool failed. This is a client side URL construction bug, not a permissions or plan issue.

This PR adds a helper that reads the version prefix from the right place for whichever spec is being loaded (the servers URL for v2, or basePath for v1) and prepends it to the configured host. Tool generation now uses that version aware base URL. The fix keeps the configured API host and only borrows the version path from the spec, so a custom API host still works, and v1 behaviour is unchanged.

Type of Change

Bug fix

Additional Notes

Added some tests also.

[Copilot]

Pull request overview

Fixes MCP tool URL construction so dynamically generated tools for Cloudsmith API v2 include the required /v2 prefix (derived from the OpenAPI spec’s servers[].url for v2 or basePath for v1), preventing authenticated v2 calls from incorrectly hitting unversioned endpoints and returning 404s.

Changes:

• Added DynamicMCPServer._spec_base_url() to compute a version-aware base URL from the loaded spec while preserving the configured API host.
• Updated tool generation to accept and use a per-spec base_url instead of always using self.api_base_url.
• Added unit tests validating base URL derivation and that a v2 tool is generated under the /v2/... path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
cloudsmith_cli/core/mcp/server.py	Builds a version-aware base URL from spec metadata and uses it when generating MCP tools so v2 endpoints resolve correctly.
cloudsmith_cli/cli/tests/commands/test_mcp.py	Adds coverage to ensure v2 uses servers[].url path prefix, v1 falls back to basePath, and generated tool URLs include /v2.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jtaylor-cs]

Nice fix. This one was a real gap. I ran a quick live check against a real repo to confirm the premise before digging into the code:

GET https://api.cloudsmith.io/v2/analytics/logs/package/jtaylor-test/   -> 200
GET https://api.cloudsmith.io/analytics/logs/package/jtaylor-test/      -> 404 (today's behavior)

The root cause and the fix's target URL both check out. A few things worth addressing or at least considering:

1. v1's basePath branch is inert, this is worth fixing the comment.
   The url=api_url (with /v1/) passed into get_schema_view has its path component explicitly discarded (generators.py logs "path component of api base URL %s is ignored; use FORCE_SCRIPT_NAME instead" and never uses it). basePath is computed independently by determine_path_prefix() off the version-relative endpoint patterns, which resolves to "/" for our single-segment paths (/repos/, packages, etc.) This matches the checked-in openapi.json ("basePath": "/"). So this branch will never actually contribute a /v1 prefix, in production or anywhere else. Harmless today since v1 already answers unprefixed, but the docstring's claim ("Swagger 2 (v1) uses basePath") may give false confidence if v1 ever needed prefixing later. Consider rewording the comment to say it's a no-op fallback.

2. Consider a test which drives the real load_openapi_spec() wiring.
   Your new tests call _spec_base_url() / _generate_tools_from_spec(base_url) directly with hand-built spec dicts but nothing exercises load_openapi_spec() itself, so a future refactor that drops the self._spec_base_url() argument at the call site wouldn't be caught by CI.

3. Generate a CHANGELOG entry. These appear to be manually produced in this repository.
   I had AI generate a suggestion for the entry:

- MCP tools backed by the v2 API (e.g. `analytics_logs_package_list`) no longer 404. Dynamically-generated tool URLs now include the `/v2` version prefix, which the OpenAPI spec declares separately from its path keys. Previously every v2-backed MCP tool call failed regardless of authentication or plan.

4. servers[0] is taken unconditionally.
   This may be fine today since our backend only ever emits one entry, but worth a defensive comment/guard for a future multi-server spec.

5. Minor (not a blocker: The cross-version tool-name overwrite (self.tools[name] = tool) is pre-existing, but now more consequential. Before this fix both versions shared the same (uniformly wrong) base URL so a same-name collision was harmless either way, but now v1/v2 tools can have genuinely different, both-correct base URLs, so whichever wins actually matters. This was an item that an AI review uncovered and here is how it explained it.

The code has no protection against a future collision, and the fix changes what a collision would mean if one ever occurred. Here's the mechanism, worked through with a made-up example so it's concrete:

load_openapi_spec() loops {"v1": ..., "v2": ...} in that order, fetching and processing v1 first, v2 second.

Each tool gets stored as self.tools[tool.name] = tool - there is no check for "does this name already exist."

Suppose both specs defined an operation called packages_list. v1's packages_list tool gets stored first. Then v2's packages_list tool gets processed and overwrites it in the dict; silently, no warning, no error.

Before this PR: both the v1 and v2 packages_list tools would have carried the exact same base_url (self.api_base_url, unprefixed) - because that's what the pre-fix code always used regardless of spec. So even though v2 silently won the overwrite, the "wrong" one that got discarded (v1) would have produced the identical URL anyway. The collision existed, but it was invisible.

After this PR: v1's tool would resolve to https://api.cloudsmith.io/packages/... and v2's to https://api.cloudsmith.io/v2/packages/... If a collision like this ever happened, the fix means whichever version's tool the dict ends up holding is the only one an LLM using this MCP server could ever call under that name. The other version's endpoint becomes permanently unreachable through that name, silently.

So the risk isn't "this will break something today" (0 real overlap right now), it's "the fix removes an accidental safety net without adding a replacement one (name-collision detection)."

6. Nit: is draft_ENG-14445 in the title a placeholder that needs resolving before merge?

I hope this is helpful as an initial pre-review. If you feed this output into AI, ask the agent to "dispatch an adversarial sub-agent" to challenge this review. You might be impressed by the outcome.

[cloudsmith-iduffy]

I rebased this branch on master and pushed a smaller fix in 0dfefad.

load_openapi_spec already loops over the API version and fetches each spec from {host}/{version}/. The fix passes that same prefix to _generate_tools_from_spec. Any host that serves the spec also serves the API under that prefix. Thus we do not need to read servers or basePath from the spec. I removed _spec_base_url and the optional base_url fallback.

v1 tools now use /v1/... URLs. I checked that api.cloudsmith.io answers at both / and /v1/.

I replaced the five new tests with one test that goes through load_openapi_spec. The test fails on master and passes with the fix. I also made the CHANGELOG entry shorter.