fix(mcp): rewrite skill-shipped MCP command paths for the Claude target#1465
Conversation
Skill packages that ship a self-defined MCP launcher (e.g. juspay/nix-chrome-devtools-mcp) declare their `command:` using the cross-tool converged path `.agents/skills/<name>/...`. Every target except `claude` deploys skills to `.agents/skills/`, so the upstream path is correct as-declared. Claude is the documented outlier -- `install/skill_path_migration.py:55` explicitly excludes `.claude/skills/` from the convergence -- and consumers with `target: claude` end up with files at `.claude/skills/<name>/...` while `.mcp.json` still points at the non-existent `.agents/skills/<name>/...`. Override `_format_server_config` in `ClaudeClientAdapter` to rewrite the `.agents/skills/` prefix to `.claude/skills/` before the entry is written. The rewrite is scoped to the prefix only (bare binaries like `npx`, absolute paths, and unrelated relative paths pass through) and runs once per format, so drift detection (which compares upstream `dep.to_dict()` against the stored baseline, not on-disk paths) is unaffected and reinstalls remain idempotent. Repro: `apm install` in a project with `target: claude` that depends on `juspay/nix-chrome-devtools-mcp` writes a `.mcp.json` whose `command` points at `.agents/skills/nix-chrome-devtools-mcp/bin/serve` -- a path that does not exist, so Claude Code fails to launch the server.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Claude client adapter to rewrite self-defined MCP skill launcher paths from the cross-tool .agents/skills/ convention to Claude’s .claude/skills/ location so generated .mcp.json entries point to paths that exist after deployment.
Changes:
- Add command-prefix rewrite logic in
ClaudeClientAdapterand apply it when formatting server config. - Add unit tests verifying rewrite behavior for skill-path commands and no-op behavior for other command shapes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/test_claude_mcp.py | Adds regression tests covering .agents/skills/ → .claude/skills/ rewriting and ensuring non-skill commands are unchanged. |
| src/apm_cli/adapters/client/claude.py | Implements the command-prefix rewrite and applies it to formatted server configs for Claude. |
| @classmethod | ||
| def _rewrite_self_defined_skill_command(cls, command: str) -> str: | ||
| if isinstance(command, str) and command.startswith(cls._CONVERGED_SKILL_PREFIX): | ||
| return cls._CLAUDE_SKILL_PREFIX + command[len(cls._CONVERGED_SKILL_PREFIX) :] |
There was a problem hiding this comment.
This is ruff format's style (Black-style, E203) for slices whose index is a complex expression — and the project's configured formatter, per pyproject.toml. Both ruff check src/apm_cli/adapters/client/claude.py and ruff format --check pass; removing the space would in fact be re-added by ruff format.
It's also the dominant convention in src/apm_cli/ — a quick grep:
src/apm_cli/bundle/plugin_exporter.py:72: normalized = normalized[len("skills/") :].lstrip("/")
src/apm_cli/utils/normalization.py:41: return content[len(_BOM) :]
src/apm_cli/core/conflict_detector.py:115: server_name = raw_key[len("mcp_servers.") :]
src/apm_cli/commands/policy.py:54: return source[len(prefix) :]
src/apm_cli/bundle/lockfile_enrichment.py:153: mapped = dst_prefix + f[len(src_prefix) :]
src/apm_cli/core/command_logger.py:693: reason = reason[len(prefix) :]
src/apm_cli/bundle/local_bundle.py:281: return value[len("sha256:") :].strip().lower()
src/apm_cli/commands/marketplace/check.py:60: tag_name = tag_name[len("refs/tags/") :]
src/apm_cli/core/token_manager.py:242: token = line[len("password=") :]
Leaving as-is.
## Summary `.mcp.json` shipped at HEAD points the `chrome-devtools` server at `.agents/skills/nix-chrome-devtools-mcp/bin/serve` — a path that **does not exist** on disk for this project. The launcher actually lives at `.claude/skills/nix-chrome-devtools-mcp/bin/serve` because our `apm.yml` declares `target: claude`, and Claude is the only apm target that does not converge to `.agents/skills/`. Result: Claude Code silently fails to start the chrome-devtools MCP server. ## What changed - **`.mcp.json`**: rewrite the `command` prefix from `.agents/skills/` to `.claude/skills/`. - **`apm.yml`**: add a comment explaining the bug and linking the upstream fix. ## Upstream fix [microsoft/apm#1465](microsoft/apm#1465) — `ClaudeClientAdapter` learns to rewrite the prefix when emitting `.mcp.json`. Once that ships in an apm release, `apm install` regenerates `.mcp.json` identically to the version committed here, so this patch becomes a no-op. ## Test plan - [x] Verified `./.claude/skills/nix-chrome-devtools-mcp/bin/serve` exists and starts cleanly - [x] `apm install` with the patched apm-cli produces the same `.mcp.json` as this PR (idempotent) - [ ] In Claude Code: `chrome-devtools` MCP server connects and exposes its tools
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Minimal, well-scoped fix: Template Method override + isolated classmethod rewriter; only args-scope omission warrants a doc comment. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output changes; silent path rewrite is correct -- existing success/error surface is unchanged and appropriate. |
| DevX UX Expert | 0 | 1 | 0 | Fix is correct and invisible to users on the happy path; the only UX gap worth addressing is surfacing the broken-command-path failure at install time rather than silently deferring it to Claude runtime. |
| Supply Chain Security Expert | 0 | 0 | 1 | No supply-chain security regressions; the string-prefix rewrite is type-guarded, scope-limited, and does not amplify any pre-existing path or credential risk. |
| OSS Growth Hacker | 0 | 1 | 0 | Solid silent-failure fix for Claude users; needs a CHANGELOG entry so it converts to trust signal rather than invisible maintenance. |
| Doc Writer | 0 | 1 | 0 | Add a CHANGELOG [Unreleased] ### Fixed entry -- this bug fix affects Claude users and the section already exists but has no entry for this PR. |
| Test Coverage Expert | 0 | 1 | 0 | Unit tests are well-targeted and cover the rewrite logic in isolation; no integration-tier test verifies the on-disk .mcp.json command path after a real Claude skill install -- a recommended gap against the install-pipeline floor. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [OSS Growth Hacker + Doc Writer] Add CHANGELOG [Unreleased] ### Fixed entry for the Claude skill path rewrite. -- Dual-panelist convergence is the highest signal in this review. The fix is invisible to users without a changelog entry; with one, it becomes evidence of responsive maintenance and harness-aware quality. Suggested line:
Fix Claude skill MCP launcher path: rewrite .agents/skills/ command prefix to .claude/skills/ in ClaudeClientAdapter so self-defined skill packages resolve correctly for Claude users. (#1465) - [Test Coverage Expert] Add integration-with-fixtures test: verify .claude/skills/ path in written .mcp.json after a real Claude skill install. -- Unit tests cover the rewrite logic but the install-pipeline surface has no integration-tier assertion. If prefix constants or the super-call drift, no automated check catches it before a user ships a broken .mcp.json. Track as a follow-up issue; suggested file:
tests/integration/test_claude_mcp_schema_fidelity.py. - [DevX UX Expert] Add install-time existence check: emit _rich_warning if the rewritten command path does not exist on disk. -- Currently the broken-path failure is deferred to Claude runtime. A stat check after rewrite would surface the error at apm install time, aligning with the pragmatic-as-npm principle.
- [Python Architect] Add a one-line docstring to _rewrite_self_defined_skill_command clarifying that only command is rewritten, not args. -- The intentional scope (command only) is not self-evident; without a comment a future contributor may file a follow-up bug or incorrectly extend the rewrite to args.
- [Supply Chain Security Expert] Track path validation at the call-site as a future hardening task. -- The prefix rewrite preserves any traversal sequences already present in the command. Not a regression, but a resolved-path validation would close the theoretical risk surface.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<ABC>>
+configure_mcp_server(name) bool
+_format_server_config(info, env, vars) dict
}
class CopilotClientAdapter {
<<ConcreteBase>>
+_format_server_config(info, env, vars) dict
}
class ClaudeClientAdapter {
<<ConcreteStrategy>>
+_CONVERGED_SKILL_PREFIX str
+_CLAUDE_SKILL_PREFIX str
+_rewrite_self_defined_skill_command(cmd) str
+_format_server_config(info, env, vars) dict
}
class CursorClientAdapter {
<<ConcreteStrategy>>
+_format_server_config(info, env, vars) dict
}
class GeminiClientAdapter {
<<ConcreteStrategy>>
}
class WindsurfClientAdapter {
<<ConcreteStrategy>>
}
MCPClientAdapter <|-- CopilotClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
CopilotClientAdapter <|-- CursorClientAdapter
CopilotClientAdapter <|-- GeminiClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
note for ClaudeClientAdapter "Template Method: super()._format_server_config then rewrites .agents/skills/ -> .claude/skills/"
class ClaudeClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["configure_mcp_server (claude.py:249)"] --> B["_format_server_config (claude.py:67)"]
B --> C["super()._format_server_config (copilot.py:408)"]
C --> D["reads _raw_stdio.command (copilot.py:433)"]
D --> E{"command starts with .agents/skills/?"}
E -- yes --> F["_rewrite_self_defined_skill_command (claude.py:62) returns .claude/skills/... path"]
E -- no --> G["command unchanged"]
F --> H["config dict with rewritten command"]
G --> H
H --> I["FS write .mcp.json"]
Recommendation
Ship after adding the CHANGELOG entry (highest-convergence follow-up, one line, pre-merge preferred). All other follow-ups are post-merge: integration fixture test, install-time warning, docstring, and path validation. The fix is correct, the unit evidence is solid, and the only pre-merge gap is the missing changelog line that converts this invisible maintenance into a community trust signal.
Full per-persona findings
Python Architect
- [nit] args not rewritten -- acceptable but worth a comment at
src/apm_cli/adapters/client/claude.py:62
Onlycommandis rewritten;argsentries that might contain.agents/skills/paths are left as-is. For the current skill launcher convention (command holds the binary path, args are flags) this is correct. A one-line docstring on_rewrite_self_defined_skill_commandstating the intentional scope (commandonly, notargs) would prevent a future contributor from filing a follow-up bug.
Suggested: Add docstring: "Rewrites the command field only; args are intentionally left unchanged as skill launchers place the binary path in command, not args."
CLI Logging Expert
No findings.
DevX UX Expert
- [recommended] Silent runtime failure deferred from install to Claude launch at
src/apm_cli/adapters/client/claude.py (_format_server_config)
The fix corrects a silent runtime failure -- broken command path in .mcp.json only surfaces when Claude tries (and fails) to start the MCP server, not atapm installtime. Consider adding an install-time existence check (or at least a warning) when the resolved command path does not exist on disk.
Suggested: After rewriting the command, stat the resolved path and emit a_rich_warningif it does not exist. This turns a silent runtime break into a recoverable install-time prompt.
Supply Chain Security Expert
- [nit] Path traversal sequences in command are preserved by the prefix rewrite at
src/apm_cli/adapters/client/claude.py
The prefix rewrite preserves any path traversal sequences already present in the command (e.g..agents/skills/../../../evil->.claude/skills/../../../evil). This is not a regression introduced by this PR -- the risk existed before and is equal in both forms -- but it is a signal that the call-site should eventually validate the resolved path. No action required for this PR.
OSS Growth Hacker
- [recommended] No CHANGELOG entry for a user-facing Claude bug fix at
CHANGELOG.md
The [Unreleased] section has no entry for this fix. Claude users who hit the silent MCP failure would never know it was resolved -- and prospective Claude users evaluating APM won't see evidence that Claude first-run quality is actively maintained.
Suggested: Add a line under [Unreleased] ### Fixed:Fix: rewrite .agents/skills/ command prefix to .claude/skills/ in ClaudeClientAdapter so skill MCP launchers resolve correctly for Claude users. (#1465)
Doc Writer
- [recommended] Missing CHANGELOG [Unreleased] entry for the Claude skill path rewrite fix at
CHANGELOG.md
This PR fixes a user-facing bug: Claude users installing skill packages with self-defined MCP launchers received broken command paths (.agents/skills/ instead of .claude/skills/). The CHANGELOG [Unreleased] section has a ### Fixed block (PR fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters #1461) but no entry for this fix. Per project conventions, bug fixes that affect user-observable behavior belong in CHANGELOG [Unreleased] ### Fixed. The 0.14.0 release notes explicitly document the .claude/skills/ routing contract, so readers who encounter this breakage and search the changelog will find no record of the fix.
Suggested: Add under ## [Unreleased] ### Fixed:Fix Claude skill MCP launcher path: rewrite .agents/skills/ command prefix to .claude/skills/ in ClaudeClientAdapter so self-defined skill packages resolve correctly for Claude users. (#1465)
Test Coverage Expert
- [recommended] Install-pipeline surface: no integration-tier test verifying .claude/skills/ path in written .mcp.json
Unit tests added are well-targeted but stay atunittier. The tier-floor matrix requiresintegration-with-fixturesfor the install-pipeline surface. No integration test intests/integration/covers the.agents/skills/->.claude/skills/rewrite via a real fixture package. If_format_server_configsuper-call or prefix constants drift, unit tests catch the rewrite logic in isolation, but no integration-tier assertion would fail before a user ships a broken.mcp.json.
Proof (passed at unit):tests/unit/test_claude_mcp.py::test_self_defined_skill_command_rewritten_to_claude_skills-- proves: When a skill MCP launcher declares .agents/skills/... as its command, configure_mcp_server writes .claude/skills/... to .mcp.json [devx, multi-harness-support]
self.assertEqual(data['mcpServers']['chrome-devtools']['command'], '.claude/skills/nix-chrome-devtools-mcp/bin/serve')
Proof (passed at unit):tests/unit/test_claude_mcp.py::test_self_defined_non_skill_command_left_unchanged-- proves: Bare binaries, absolute paths, and unrelated relative paths are not rewritten [devx]
self.assertEqual(self.adapter._rewrite_self_defined_skill_command(raw_command), raw_command)
Proof (missing at integration-with-fixtures):tests/integration/test_claude_mcp_schema_fidelity.py::test_skill_mcp_launcher_command_rewritten_to_claude_skills_prefix-- proves: After apm install of a Claude-targeted skill with an MCP launcher, the on-disk .mcp.json has a .claude/skills/ command path [devx, multi-harness-support, portability-by-manifest]
Auth Expert -- inactive
PR only rewrites a filesystem command path prefix in ClaudeClientAdapter._format_server_config; no auth flows, token resolution, credential handling, or AuthResolver touched.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 4 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1465
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - fix(mcp): rewrite skill-shipped MCP command paths for the Claude target #1465
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - 3a7389d
list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1465
search_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1465 · ● 2.6M · ◷
* chore: cut 0.15.0 Move Unreleased -> [0.15.0] - 2026-05-27 and bump pyproject + uv.lock. Audit applied: every PR merged since v0.14.2 has exactly one changelog entry; each entry leads with the user-visible impact. Fixes during audit: - Add missing entries for #1367, #1403, #1465, #1487, #1492, #1462, #1477, #1439, #1484, and the 131679f follow-up commit. - Collapse the two #1473 lines into one. - Merge the #1476 Security/GitCache-hardening entry into its Added entry (same PR, one logical change). - Replace bogus #1243 PR ref with the actual merge PR #1308 for the persisted transport-flag config. - Relocate the #1324-delivered marketplace CLI entries (apm pack --marketplace / --marketplace-path / --json, outputs map form) out of Unreleased and into [0.14.2], where they actually shipped. They were mis-attributed to #1317 and orphaned across the 0.14.2 cut. Verified locally: ruff check + ruff format --check both clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Self-defined MCP launchers shipped inside a skill package (e.g.
juspay/nix-chrome-devtools-mcp) declare theircommand:using the cross-tool converged skill path.agents/skills/<name>/.... Every other target deploys skills to.agents/skills/, so the upstream value is correct as-declared — but Claude is the documented outlier (seeinstall/skill_path_migration.py:55: ".claude/skills/ is excluded (Claude is not in the convergence set)"). Consumers whoseapm.ymlhastarget: claudeend up with files at.claude/skills/<name>/...while.mcp.jsonstill points at the non-existent.agents/skills/<name>/..., so Claude Code fails to launch the server.Repro
Fix
Override
_format_server_configinClaudeClientAdapterto rewrite the.agents/skills/prefix to.claude/skills/on the way out. Scoped to the prefix only — bare binaries (npx,node), absolute paths, and unrelated relative paths pass through unchanged.Drift detection (
_detect_mcp_config_drift) compares the upstreamdep.to_dict()against the lockfile baseline (both upstream-shaped), not against on-disk paths, so the rewrite does not perturb idempotence. Verified: a secondapm installreports "already configured" and the on-disk path remains rewritten.Why this layer
_build_self_defined_infoinmcp_integrator.pybuilds onesynthetic_infoper dep that is reused across alltarget_runtimes— rewriting there would either need per-runtime copies or runtime-specific knowledge in the integrator._normalize_mcp_entry_for_claude_code), and Claude is the only target that needs this rewrite, so a single override stays local.Test plan
.agents/skills/<name>/...→.claude/skills/<name>/...end-to-end throughconfigure_mcp_server→.mcp.jsonnpx, absolute paths,./scripts/...,.agents/other/...tests/unit/test_claude_mcp.pysuite: 28 passed (+4 subtests)tests/unit/adapters,tests/unit/test_copilot_adapter.py,tests/integration/test_claude_mcp_schema_fidelity.py: 116 passedtests/unit/integration,tests/integration/test_agent_skills_target.py: 1444 passedapm installin atarget: claudeproject consumingjuspay/nix-chrome-devtools-mcp;.mcp.jsonnow points at the deployed launcher and./.claude/skills/nix-chrome-devtools-mcp/bin/serveboots successfully