fix(install): MARKETPLACE_PLUGIN beats HOOK_PACKAGE in detection cascade (#780)#781
Conversation
Implements the core decision engine for issue #778 'transport selection v1'. Strict-by-default semantics replace today's silent cross-protocol fallback: explicit ssh:// and https:// dependencies no longer downgrade to a different protocol, and shorthand (owner/repo) consults git insteadOf rewrites before defaulting to HTTPS. This commit ships Waves 1+2 of the transport-selection plan (per session plan): - new module src/apm_cli/deps/transport_selection.py with ProtocolPreference, TransportAttempt/TransportPlan dataclasses, GitConfigInsteadOfResolver, and TransportSelector that returns a typed, strict-by-default plan - DependencyReference grows explicit_scheme so the selector can distinguish user-stated transport from shorthand - _clone_with_fallback in github_downloader.py now iterates the selector plan; per-attempt URL building stays in the orchestrator - InstallContext / InstallRequest / pipeline / Service threaded with protocol_pref + allow_protocol_fallback so CLI args reach the downloader - apm install gains --ssh / --https (mutually exclusive) and --allow-protocol-fallback flags; honours APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK env vars - two pre-existing tests in test_auth_scoping.py asserted the legacy permissive chain; updated to assert the new strict contract and added a coverage test for the allow_fallback escape hatch Tests: 4029 unit tests pass. Test matrix + integration tests + docs land in subsequent commits per Waves 3-5. Refs #778, #328, #661 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…one env Wave 2 panel gate (code-review subagent) flagged that GitHubPackageDownloader._clone_with_fallback decided the clone env (locked-down vs relaxed) ONCE per dependency from has_token. Under allow_fallback=True the plan can mix attempts of different use_token values, so SSH and plain-HTTPS attempts in a mixed chain were running with GIT_ASKPASS=echo + GIT_CONFIG_GLOBAL=/dev/null + GIT_CONFIG_NOSYSTEM=1, breaking ssh-agent passphrase prompts and git credential helpers. Fix the env per attempt; address an adjacent contract bug; add tests. * github_downloader._clone_with_fallback: replace the per-dep clone_env with a per-attempt _env_for() helper so only token-bearing attempts get the locked-down env. * github_downloader._build_repo_url: treat token="" as an explicit "no token" sentinel so plain-HTTPS attempts in a mixed chain genuinely run without embedded credentials, letting credential helpers (gh auth, Keychain) supply auth. Orchestrator passes "" instead of None for use_token=False attempts. * transport_selection.GitConfigInsteadOfResolver: wrap the lazy insteadOf-rewrite cache in a threading.Lock so parallel downloads can't double-populate. Tests: * tests/unit/test_transport_selection.py (NEW, 30 tests): 14-row selection matrix (explicit-strict, shorthand+insteadOf, shorthand defaults, CLI prefs, allow_fallback chain, env helpers); resolver caching; "must use normal env" contract. * tests/unit/test_auth_scoping.py: new test_allow_fallback_env_is_per_attempt_not_per_dep regression asserts auth-HTTPS gets locked-down env, SSH and plain-HTTPS get relaxed env, and plain-HTTPS does not embed the token in the URL. * tests/integration/test_transport_selection_integration.py (NEW, 7 tests): 2 always-on cases (public shorthand HTTPS; explicit https:// strict); 5 SSH-required cases (explicit ssh:// strict, bad-host no-fallback, insteadOf override, APM_GIT_PROTOCOL=ssh env, allow_fallback rescue). Gated on APM_RUN_INTEGRATION_TESTS=1; SSH cases auto-skip if no key. * tests/fixtures/gitconfig_insteadof_to_ssh (NEW): minimal gitconfig used by the integration test for the insteadOf-honored case. * scripts/test-integration.sh: added "Transport Selection" block so the integration suite runs in CI. Full unit suite: 4061 passed (was 4029; +32 net new tests). Refs #778, #661, #328 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the four user-facing surfaces affected by the new TransportSelector
contract:
* docs/src/content/docs/guides/dependencies.md:
- New "Transport selection (SSH vs HTTPS)" section: breaking-change
callout with the rescue env var, selection matrix, insteadOf
example, --ssh / --https / APM_GIT_PROTOCOL overrides, and the
--allow-protocol-fallback escape hatch.
- Soften the existing "Custom ports preserved" sentence (cross-protocol
retries are now opt-in).
- Update the "Other Git Hosts" SSH bullet: SSH is no longer a silent
fallback; point at explicit URLs or insteadOf.
* docs/src/content/docs/getting-started/authentication.md:
- Rewrite the "SSH connection hangs" troubleshooting entry: remove the
now-incorrect "tries SSH then falls back to HTTPS" framing.
- New "Choosing transport (SSH vs HTTPS)" section with a pointer to
dependencies.md for the full transport contract.
* docs/src/content/docs/reference/cli-commands.md:
- Document --ssh / --https / --allow-protocol-fallback on apm install,
plus APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK env vars.
* packages/apm-guide/.apm/skills/apm-usage/dependencies.md (Rule 4 mirror):
- Same transport contract in skill-resource voice with three runnable
snippets and a selection matrix.
CHANGELOG: scope new entries to `apm install` only (there is no `apm
add` command in the codebase).
Refs #778
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two findings from the Copilot reviewer on PR #779: 1. Non-ASCII em dash introduced by this PR in the modified `Fields:` line of guides/dependencies.md: replace with `--`. The other non-ASCII chars Copilot flagged in the file (lines ~150-160 of the "nested groups" warning block) are pre-existing and out of scope for this PR. 2. CHANGELOG entries for the new transport-selection feature were too long and bundled multiple concerns into one bullet. Split into one tighter BREAKING entry plus two single-purpose Added entries (initial-protocol flags; fallback escape hatch). Each ends in `(#778)`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- docs(dependencies): pin BREAKING-change caution panel to APM 0.8.13
(per maintainer review comment)
- transport_selection: collapse explicit `http://` URLs into the plain-HTTPS
branch so TransportAttempt.scheme stays in {ssh, https} and the downloader
contract is consistent until #700 lands a first-class HTTP transport
- github_downloader: gate the [!] "Protocol fallback" warning on actual
scheme change (ssh<->https) rather than label change, so an auth downgrade
inside a single protocol is not misreported as a protocol switch
- pipeline / request / context / commands: switch
`allow_protocol_fallback` to `Optional[bool] = None` end-to-end so
programmatic callers (non-CLI) keep the documented "None => read
APM_ALLOW_PROTOCOL_FALLBACK env" behavior
- test_auth_scoping: ASCII-only docstring (replace one stray Unicode arrow)
All 4061 unit tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ade (#780) obra/superpowers (and any Claude Code plugin shipping both hooks/*.json and agents/skills/commands) was misclassified as HOOK_PACKAGE because the cascade in detect_package_type() checked _has_hook_json BEFORE the plugin-evidence branch. The plugin synthesizer (_map_plugin_artifacts) already maps hooks alongside agents/skills/commands, so MARKETPLACE_PLUGIN is a strict superset -- swapping the order means hooks still install, plus everything else that was being silently dropped. Three deliverables: A) Surgical detection fix: reorder cascade so MARKETPLACE_PLUGIN is checked before HOOK_PACKAGE. Refactored to use a new gather_detection_evidence() helper + DetectionEvidence dataclass so observability code (warnings, summaries) can reuse the same scan without breaking the detect_package_type() public signature. B) Observability: - Add HOOK_PACKAGE to the package-type label table (it was missing entirely -- the silent classification path). - Update MARKETPLACE_PLUGIN label to mention plugin.json OR agents/skills/commands (matches the cascade behaviour). - New CommandLogger.package_type_warn() at default visibility. - New _warn_if_classification_near_miss() helper fires when a HOOK_PACKAGE classification disagrees with directory contents (catches near-misses the order swap cannot, e.g. .claude-plugin/ dir without plugin.json). - Wired at both materialization sites (local + cached). C) Architectural follow-up tracked in plan; will file as separate issue after merge for a Visitor / Format-Discovery refactor. Tests: 17 detection tests + 9 sources-observability tests + 70 command-logger tests pass. Full unit suite (4180 tests) green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Architectural follow-up filed as #782 (Visitor / Format-Discovery refactor) -- not blocking this PR. |
There was a problem hiding this comment.
Pull request overview
Fixes a misclassification bug where Claude marketplace plugins that also ship hooks/*.json were detected as hook-only packages (dropping skills/agents/commands), and adds a new strict-by-default SSH/HTTPS transport-selection engine (with CLI/env controls) plus observability improvements around both behaviors.
Changes:
- Reorders package-type detection (MARKETPLACE_PLUGIN before HOOK_PACKAGE) and adds reusable detection evidence gathering + near-miss warnings.
- Introduces strict-by-default git transport selection (SSH vs HTTPS), with
--ssh/--https,APM_GIT_PROTOCOL, and an escape-hatch--allow-protocol-fallback/APM_ALLOW_PROTOCOL_FALLBACK. - Adds unit/integration tests and updates docs + changelog entries for the new behaviors.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/models/validation.py |
Adds DetectionEvidence + gather_detection_evidence() and reorders detection cascade to prefer marketplace plugins over hook-only packages. |
src/apm_cli/install/sources.py |
Centralizes package-type labels and adds hook-classification near-miss warnings during materialization. |
src/apm_cli/core/command_logger.py |
Adds package_type_warn() for default-visibility warnings. |
src/apm_cli/models/dependency/reference.py |
Records explicit_scheme on parsed dependency refs to drive strict transport selection. |
src/apm_cli/deps/transport_selection.py |
New pure transport selection engine + insteadOf resolver + env helpers. |
src/apm_cli/deps/github_downloader.py |
Uses TransportSelector for clone attempt planning; makes clone env decision per-attempt. |
src/apm_cli/commands/install.py |
Adds --ssh, --https, --allow-protocol-fallback and plumbs into install pipeline. |
src/apm_cli/install/context.py |
Carries protocol_pref and allow_protocol_fallback through the pipeline. |
src/apm_cli/install/request.py |
Adds transport-selection fields to InstallRequest. |
src/apm_cli/install/service.py |
Passes transport-selection fields into pipeline entrypoint. |
src/apm_cli/install/pipeline.py |
Plumbs transport-selection fields into InstallContext. |
src/apm_cli/install/phases/resolve.py |
Instantiates downloader with protocol preference + fallback policy. |
tests/test_apm_package_models.py |
Adds regression tests for #780 and tests for gather_detection_evidence(). |
tests/unit/install/test_sources_classification.py |
Adds tests for label coverage and near-miss warning behavior. |
tests/unit/test_auth_scoping.py |
Updates auth-scoping tests for strict transport + per-attempt env behavior. |
tests/unit/test_transport_selection.py |
New unit test matrix for transport-selection decisions and caching. |
tests/integration/test_transport_selection_integration.py |
New network-gated integration tests validating attempted clone URLs end-to-end. |
tests/fixtures/gitconfig_insteadof_to_ssh |
Fixture .gitconfig enabling an insteadOf rewrite to SSH. |
scripts/test-integration.sh |
Runs the new transport selection integration tests as part of e2e script. |
docs/src/content/docs/reference/cli-commands.md |
Documents new apm install flags and env vars. |
docs/src/content/docs/guides/dependencies.md |
Documents strict transport selection contract + fallback escape hatch. |
docs/src/content/docs/getting-started/authentication.md |
Updates troubleshooting and adds “Choosing transport” guidance. |
packages/apm-guide/.apm/skills/apm-usage/dependencies.md |
Updates apm-guide dependency docs with transport selection section. |
CHANGELOG.md |
Adds Unreleased entries for strict transport selection + #780 fix. |
Copilot's findings
Comments suppressed due to low confidence (1)
CHANGELOG.md:18
- The new Unreleased changelog entries end with
(#778), but this change is landing in PR #780. Per the project changelog convention, each entry should end with the current PR number, and related items should be collapsed to a single line per PR under the appropriate section.
### Changed (BREAKING)
- Strict-by-default transport selection: explicit `ssh://`/`https://` URLs no longer silently fall back to the other protocol; shorthand consults `git config url.<base>.insteadOf` and otherwise defaults to HTTPS. Set `APM_ALLOW_PROTOCOL_FALLBACK=1` (or pass `--allow-protocol-fallback`) to restore the legacy permissive chain; cross-protocol retries then emit a `[!]` warning. Closes #328 (#778)
### Added
- `apm install --ssh` / `--https` flags and `APM_GIT_PROTOCOL=ssh|https` env to pick the initial transport for shorthand dependencies (#778)
- `apm install --allow-protocol-fallback` flag and `APM_ALLOW_PROTOCOL_FALLBACK=1` env as the migration escape hatch for cross-protocol fallback (#778)
- Files reviewed: 6/6 changed files
- Comments generated: 1
Manual repro on the real obra/superpowers repoCloned the actual repo ( Before fix: classified as
|
…in/ evidence Per Copilot review on PR #781: the near-miss warning helper had dead code paths once the cascade was reordered (a HOOK_PACKAGE classification implies plugin_dirs_present is empty -- otherwise it would be MARKETPLACE_PLUGIN). Two principled options were offered: simplify the helper, or broaden detection so the invariants stay consistent. Chose the latter -- it removes the inconsistency at the source. Changes: - Add .claude-plugin/ as first-class plugin evidence in DetectionEvidence (new has_claude_plugin_dir field) and in has_plugin_evidence. A Claude Code plugin without a plugin.json (manifest-less) now classifies as MARKETPLACE_PLUGIN; normalize_plugin_directory already handles the missing-manifest case (derives name from directory). - Drop _warn_if_classification_near_miss helper, its two call sites, CommandLogger.package_type_warn method, and the corresponding tests. All scenarios it covered are now handled by the cascade itself. - Add regression test test_claude_plugin_dir_alone_is_plugin_evidence asserting that .claude-plugin/ + hooks/ classifies as MARKETPLACE_PLUGIN with plugin_json_path=None (matched via directory evidence alone). - Extend test_obra_superpowers_evidence to assert has_claude_plugin_dir. Verified: 4175 unit tests pass (excluding the one pre-existing unrelated failure documented on main). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- mcp.py: fix-it suggestion now uses split(maxsplit=1) so tab- and
multi-space-separated commands produce a correct canonical shape
(.partition(' ') only handled U+0020, leaving tabs in the suggested
binary path -- itself invalid). Adds regression test.
- CHANGELOG: trailing parenthetical now carries this PR's number (#809);
the original (#122) was an issue reference. Issue stays cited inline.
- Resolve CHANGELOG merge conflict against main: keep VS Code adapter
http-default Fixed entry (#654).
Out-of-scope plugin/hook reclassification work (Copilot review #3) is
already on main via PR #781 and falls out of this branch's net diff
post-merge; no action needed.
Doc link in the validator (guides/mcp-servers/) is now valid -- the
guide landed via PR #810. No change required.
Refs #122, closes #806
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
) * feat(transport): TransportSelector + strict-by-default transport (#778) Implements the core decision engine for issue #778 'transport selection v1'. Strict-by-default semantics replace today's silent cross-protocol fallback: explicit ssh:// and https:// dependencies no longer downgrade to a different protocol, and shorthand (owner/repo) consults git insteadOf rewrites before defaulting to HTTPS. This commit ships Waves 1+2 of the transport-selection plan (per session plan): - new module src/apm_cli/deps/transport_selection.py with ProtocolPreference, TransportAttempt/TransportPlan dataclasses, GitConfigInsteadOfResolver, and TransportSelector that returns a typed, strict-by-default plan - DependencyReference grows explicit_scheme so the selector can distinguish user-stated transport from shorthand - _clone_with_fallback in github_downloader.py now iterates the selector plan; per-attempt URL building stays in the orchestrator - InstallContext / InstallRequest / pipeline / Service threaded with protocol_pref + allow_protocol_fallback so CLI args reach the downloader - apm install gains --ssh / --https (mutually exclusive) and --allow-protocol-fallback flags; honours APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK env vars - two pre-existing tests in test_auth_scoping.py asserted the legacy permissive chain; updated to assert the new strict contract and added a coverage test for the allow_fallback escape hatch Tests: 4029 unit tests pass. Test matrix + integration tests + docs land in subsequent commits per Waves 3-5. Refs #778, #328, #661 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(transport): wave-3 unit + integration matrix; fix per-attempt clone env Wave 2 panel gate (code-review subagent) flagged that GitHubPackageDownloader._clone_with_fallback decided the clone env (locked-down vs relaxed) ONCE per dependency from has_token. Under allow_fallback=True the plan can mix attempts of different use_token values, so SSH and plain-HTTPS attempts in a mixed chain were running with GIT_ASKPASS=echo + GIT_CONFIG_GLOBAL=/dev/null + GIT_CONFIG_NOSYSTEM=1, breaking ssh-agent passphrase prompts and git credential helpers. Fix the env per attempt; address an adjacent contract bug; add tests. * github_downloader._clone_with_fallback: replace the per-dep clone_env with a per-attempt _env_for() helper so only token-bearing attempts get the locked-down env. * github_downloader._build_repo_url: treat token="" as an explicit "no token" sentinel so plain-HTTPS attempts in a mixed chain genuinely run without embedded credentials, letting credential helpers (gh auth, Keychain) supply auth. Orchestrator passes "" instead of None for use_token=False attempts. * transport_selection.GitConfigInsteadOfResolver: wrap the lazy insteadOf-rewrite cache in a threading.Lock so parallel downloads can't double-populate. Tests: * tests/unit/test_transport_selection.py (NEW, 30 tests): 14-row selection matrix (explicit-strict, shorthand+insteadOf, shorthand defaults, CLI prefs, allow_fallback chain, env helpers); resolver caching; "must use normal env" contract. * tests/unit/test_auth_scoping.py: new test_allow_fallback_env_is_per_attempt_not_per_dep regression asserts auth-HTTPS gets locked-down env, SSH and plain-HTTPS get relaxed env, and plain-HTTPS does not embed the token in the URL. * tests/integration/test_transport_selection_integration.py (NEW, 7 tests): 2 always-on cases (public shorthand HTTPS; explicit https:// strict); 5 SSH-required cases (explicit ssh:// strict, bad-host no-fallback, insteadOf override, APM_GIT_PROTOCOL=ssh env, allow_fallback rescue). Gated on APM_RUN_INTEGRATION_TESTS=1; SSH cases auto-skip if no key. * tests/fixtures/gitconfig_insteadof_to_ssh (NEW): minimal gitconfig used by the integration test for the insteadOf-honored case. * scripts/test-integration.sh: added "Transport Selection" block so the integration suite runs in CI. Full unit suite: 4061 passed (was 4029; +32 net new tests). Refs #778, #661, #328 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(transport): document strict-by-default transport selection (#778) Update the four user-facing surfaces affected by the new TransportSelector contract: * docs/src/content/docs/guides/dependencies.md: - New "Transport selection (SSH vs HTTPS)" section: breaking-change callout with the rescue env var, selection matrix, insteadOf example, --ssh / --https / APM_GIT_PROTOCOL overrides, and the --allow-protocol-fallback escape hatch. - Soften the existing "Custom ports preserved" sentence (cross-protocol retries are now opt-in). - Update the "Other Git Hosts" SSH bullet: SSH is no longer a silent fallback; point at explicit URLs or insteadOf. * docs/src/content/docs/getting-started/authentication.md: - Rewrite the "SSH connection hangs" troubleshooting entry: remove the now-incorrect "tries SSH then falls back to HTTPS" framing. - New "Choosing transport (SSH vs HTTPS)" section with a pointer to dependencies.md for the full transport contract. * docs/src/content/docs/reference/cli-commands.md: - Document --ssh / --https / --allow-protocol-fallback on apm install, plus APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK env vars. * packages/apm-guide/.apm/skills/apm-usage/dependencies.md (Rule 4 mirror): - Same transport contract in skill-resource voice with three runnable snippets and a selection matrix. CHANGELOG: scope new entries to `apm install` only (there is no `apm add` command in the codebase). Refs #778 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: revert accidental uv.lock churn (no dependency change in this PR) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs+changelog: address copilot review feedback (#779) Two findings from the Copilot reviewer on PR #779: 1. Non-ASCII em dash introduced by this PR in the modified `Fields:` line of guides/dependencies.md: replace with `--`. The other non-ASCII chars Copilot flagged in the file (lines ~150-160 of the "nested groups" warning block) are pre-existing and out of scope for this PR. 2. CHANGELOG entries for the new transport-selection feature were too long and bundled multiple concerns into one bullet. Split into one tighter BREAKING entry plus two single-purpose Added entries (initial-protocol flags; fallback escape hatch). Each ends in `(#778)`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR #779 review feedback - docs(dependencies): pin BREAKING-change caution panel to APM 0.8.13 (per maintainer review comment) - transport_selection: collapse explicit `http://` URLs into the plain-HTTPS branch so TransportAttempt.scheme stays in {ssh, https} and the downloader contract is consistent until #700 lands a first-class HTTP transport - github_downloader: gate the [!] "Protocol fallback" warning on actual scheme change (ssh<->https) rather than label change, so an auth downgrade inside a single protocol is not misreported as a protocol switch - pipeline / request / context / commands: switch `allow_protocol_fallback` to `Optional[bool] = None` end-to-end so programmatic callers (non-CLI) keep the documented "None => read APM_ALLOW_PROTOCOL_FALLBACK env" behavior - test_auth_scoping: ASCII-only docstring (replace one stray Unicode arrow) All 4061 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): MARKETPLACE_PLUGIN beats HOOK_PACKAGE in detection cascade (#780) obra/superpowers (and any Claude Code plugin shipping both hooks/*.json and agents/skills/commands) was misclassified as HOOK_PACKAGE because the cascade in detect_package_type() checked _has_hook_json BEFORE the plugin-evidence branch. The plugin synthesizer (_map_plugin_artifacts) already maps hooks alongside agents/skills/commands, so MARKETPLACE_PLUGIN is a strict superset -- swapping the order means hooks still install, plus everything else that was being silently dropped. Three deliverables: A) Surgical detection fix: reorder cascade so MARKETPLACE_PLUGIN is checked before HOOK_PACKAGE. Refactored to use a new gather_detection_evidence() helper + DetectionEvidence dataclass so observability code (warnings, summaries) can reuse the same scan without breaking the detect_package_type() public signature. B) Observability: - Add HOOK_PACKAGE to the package-type label table (it was missing entirely -- the silent classification path). - Update MARKETPLACE_PLUGIN label to mention plugin.json OR agents/skills/commands (matches the cascade behaviour). - New CommandLogger.package_type_warn() at default visibility. - New _warn_if_classification_near_miss() helper fires when a HOOK_PACKAGE classification disagrees with directory contents (catches near-misses the order swap cannot, e.g. .claude-plugin/ dir without plugin.json). - Wired at both materialization sites (local + cached). C) Architectural follow-up tracked in plan; will file as separate issue after merge for a Visitor / Format-Discovery refactor. Tests: 17 detection tests + 9 sources-observability tests + 70 command-logger tests pass. Full unit suite (4180 tests) green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR #781 review: broaden detect_package_type with .claude-plugin/ evidence Per Copilot review on PR #781: the near-miss warning helper had dead code paths once the cascade was reordered (a HOOK_PACKAGE classification implies plugin_dirs_present is empty -- otherwise it would be MARKETPLACE_PLUGIN). Two principled options were offered: simplify the helper, or broaden detection so the invariants stay consistent. Chose the latter -- it removes the inconsistency at the source. Changes: - Add .claude-plugin/ as first-class plugin evidence in DetectionEvidence (new has_claude_plugin_dir field) and in has_plugin_evidence. A Claude Code plugin without a plugin.json (manifest-less) now classifies as MARKETPLACE_PLUGIN; normalize_plugin_directory already handles the missing-manifest case (derives name from directory). - Drop _warn_if_classification_near_miss helper, its two call sites, CommandLogger.package_type_warn method, and the corresponding tests. All scenarios it covered are now handled by the cascade itself. - Add regression test test_claude_plugin_dir_alone_is_plugin_evidence asserting that .claude-plugin/ + hooks/ classifies as MARKETPLACE_PLUGIN with plugin_json_path=None (matched via directory evidence alone). - Extend test_obra_superpowers_evidence to assert has_claude_plugin_dir. Verified: 4175 unit tests pass (excluding the one pre-existing unrelated failure documented on main). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(validation): reject shell-string command in MCP stdio entries Self-defined stdio MCP entries with `command` containing whitespace and no `args` are now rejected at parse time with a fix-it error pointing at the canonical `command: <binary>, args: [<token>, ...]` shape. Previously silently accepted; APM never split `command` on whitespace, so the loose shape mis-executed downstream. The trap surfaced via #122 (thanks @lirantal) -- users coming from the universal `mcp.json` / Claude Desktop / Cursor mental model wrote `command: "npx mcp-server-foo"` and got confused when nothing worked. Per maintainer steer ("move fast, breaking OK"): error in v1, not warn. The loose shape was never specified; silent mis-execution is worse than hard-fail with a clear fix-it. CHANGELOG entry under Changed (BREAKING). Closes #806 Refs #122 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(mcp): address PR #809 review panel (cred leak, args:[], type gate, edge cases) Addresses the APM Review Panel verdict on PR #809. Must-fix: - M1: Redact raw command in error 'Got:' framing (cli-log BLOCKER / sec HIGH). Echoing 'Got: command={self.command!r}' leaked tokens like '--token=ghp_...' to stderr / CI scrollback. Now shows only the first token plus argument count. The structured 'Did you mean:' suggestion still surfaces user input verbatim because that is the copy-paste recovery path. - M2: Use 'self.args is None' instead of 'not self.args' (arch IMPORTANT). Explicit 'args: []' is a deliberate 'no extra args' signal (e.g., paired with '/opt/My App/server') and must be accepted -- 'not []' incorrectly evaluated truthy and rejected legitimate input in a BREAKING change. Should-fix: - S1: Whitespace-only command produces a dedicated 'empty or whitespace-only' error instead of the degenerate fix-it 'Did you mean: command: , args: []' (arch + devx IMPORTANT). - S2: Type gate for non-str command (sec HIGH). YAML 'command: ["npx", "-y", "x"]' previously bypassed the isinstance guard silently and crashed downstream in validate_path_segments with an unhandled AttributeError. - S3: Document rule 4 in manifest-schema.md section 4.2.3 (devx IMPORTANT). Spec and code ship together. Adds 4 regression tests covering each fix. Removes the stray space before '?' in the fix-it suggestion (cli-log NIT 8). Follow-ups (not in this PR, to be filed as issues): - Redact 'command' in MCPDependency.__repr__ (sec MEDIUM, pre-existing) - Forward MCPDependency validation errors from plugin parser to DiagnosticCollector (cli-log IMPORTANT) - Multi-line Cargo-style error format (cli-log IMPORTANT / devx NIT) - Shell-metachar warning for stdio command (sec LOW) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(mcp): address PR #809 follow-up tickets in same PR Per user direction, fold all four CEO-classified follow-up items into this PR rather than deferring to separate issues. FU1 -- Redact 'command' in MCPDependency.__repr__ (sec MEDIUM) Pre-existing leak: __repr__ echoed 'command={self.command!r}' verbatim while carefully redacting env and headers. Now shows only the first whitespace-separated token, mirroring the M1 fix. FU2 -- Surface plugin-parser MCP validation warnings (cli-log IMPORTANT) The 'apm' stdlib logger has no handlers configured, so logger.warning calls in plugin_parser were silently dropped. Added _surface_warning helper that routes through both stdlib logger AND _rich_warning so invalid MCP servers are visible without --verbose. Applied to the validation-error catch site and the no-command/no-url skip. FU3 -- Multi-line Cargo-style error format (cli-log IMPORTANT / devx NIT) The original 350-char single-line ValueError defeated terminal URL detection and the newspaper test. Restructured to: 'command' contains whitespace in MCP dependency '<name>'. Rule: ... Got: command='<first>' (N additional args) Fix: command: <first> args: [...] See: https://... URL now sits on its own line for click-through; field/rule/got/fix/see pattern is scannable per the cli-logging-ux skill's newspaper test. FU4 -- Shell-metachar warning for stdio command (sec LOW, defense-in-depth) Extended _warn_shell_metachars(env, logger) to optionally check 'command' as well, so 'command: "npx|curl evil.com"' (no whitespace, passes the rejection guard) still triggers a warning that MCP stdio servers run via execve with no shell. Hooked into the --mcp install path via entry.get('command'). Architectural improvement (LOC budget): Adding the command-checking branch pushed install.py over the 1525 invariant ceiling. Per the python-architecture skill's guidance ('don't trim cosmetically -- modularize'), extracted the F5 SSRF helper, F7 shell-metachar helper, _is_internal_or_metadata_host, _SHELL_METACHAR_TOKENS, and _METADATA_HOSTS into a new dedicated module: apm_cli/install/mcp_warnings.py. install.py back-binds the symbols at module scope so existing test patches against apm_cli.commands.install._warn_* keep working unchanged. install.py: 1530 -> 1441 LOC (84 under budget, room to breathe). Tests: 4715/4715 unit + console pass (excludes the known pre-existing test_user_scope_skips_workspace_runtimes failure on main). New regression tests: - test_validate_stdio_error_uses_multiline_cargo_style_format - test_repr_redacts_command_to_avoid_leaking_credentials Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Closes #780.
apm install obra/superpowers(and any Claude Code plugin that ships bothhooks/*.jsonANDagents/skills/commands/) was silently dropping skills, agents, and commands -- only the hook files made it onto disk. Root cause: thedetect_package_type()cascade insrc/apm_cli/models/validation.pychecked the hook-only branch BEFORE the marketplace-plugin branch, andMARKETPLACE_PLUGIN's synthesizer (_map_plugin_artifacts) already maps hooks alongside agents/skills/commands -- so swapping the order means hooks still install, plus everything else.What ships in this PR
This was reviewed by the
apm-review-panel(architect, devx-ux, cli-logging, supply-chain, growth, CEO) and split into three deliverables. A and B ship here; C is queued as a follow-up issue.A) Surgical detection fix
MARKETPLACE_PLUGINis checked beforeHOOK_PACKAGE.gather_detection_evidence()helper +DetectionEvidencedataclass so observability code can reuse the same scan without breaking thedetect_package_type()public signature(PackageType, Optional[Path])._PLUGIN_DIRS = ("agents", "skills", "commands")constant -- canonical order, asserted by tests.B) Observability (the bug was undebuggable)
The user reported "I see only hooks" because there was zero install-time signal of what APM thinks the package is. Three changes close that gap:
HOOK_PACKAGEto the_format_package_type_labeltable -- it was missing entirely, so any hook-package classification was silent.MARKETPLACE_PLUGINlabel from "(plugin.json detected)" to "(plugin.json or agents/skills/commands)" -- matches the actual cascade behaviour.CommandLogger.package_type_warn()at default visibility._warn_if_classification_near_miss()helper: when a package classifies asHOOK_PACKAGEbutagents/,skills/,commands/, or.claude-plugin/is also present, emit[!] Detected as Hook Package, but the package also contains: <dirs>. If you expected those primitives to install, the package may be missing a plugin.json -- please file an issue.This catches near-misses the order swap can't (e.g..claude-plugin/dir withoutplugin.json).LocalDependencySourceand the cached/fresh path).C) Architectural refactor -- follow-up only
detect_package_typeconflates evidence gathering, classification, and normalization-strategy selection into a single positional-priority cascade. The next bundled format will create the next ordering bug. The clean separation isFormatDetectorinterface +PackageFormatRegistry+NormalizationPlanner. Filing as a separate issue post-merge -- too risky to bundle with the P1 fix.Why the order swap is safe
MARKETPLACE_PLUGINtriggersnormalize_plugin_directory()->synthesize_apm_yml_from_plugin->_map_plugin_artifacts(src/apm_cli/deps/plugin_parser.py:481-511)._map_plugin_artifactsexplicitly handleshooks(inline dict, config file path, OR directory). SoMARKETPLACE_PLUGINis a strict superset ofHOOK_PACKAGE's capabilities -- nothing is lost by moving plugin classification ahead of the hook-only branch.Supply-chain check (panel): no security regression -- the synthesizer already does path-safety via
_is_within_plugin. The new near-miss warning is itself a mild integrity signal: a plugin missing parts of itself is worth surfacing.Tests
tests/test_apm_package_models.py::TestDetectPackageType-- 3 new regression tests:test_marketplace_plugin_wins_over_hooks_via_agents_dirtest_marketplace_plugin_wins_over_hooks_via_plugin_jsontest_obra_superpowers_layout(full-fidelity reproducer)tests/test_apm_package_models.py::TestGatherDetectionEvidence-- 3 tests for the new helper.tests/unit/install/test_sources_classification.py-- 9 tests covering the label table (asserts every classifiablePackageTypehas a label, catches future silent-classification regressions) and the near-miss warning behaviour.Full unit suite: 4180 passed (1 pre-existing unrelated mock failure deselected).
Manual verification plan
Files changed
src/apm_cli/models/validation.py-- cascade reorder + evidence helper.src/apm_cli/install/sources.py-- label centralisation + near-miss warning + wiring.src/apm_cli/core/command_logger.py--package_type_warn()method.tests/test_apm_package_models.py-- 6 new tests.tests/unit/install/test_sources_classification.py-- new file, 9 tests.CHANGELOG.md--Fixedentry.