fix(hooks): resolve quoted plugin-root refs and warn on the ones that survive

[Mohammed Alkindi (MohammedAlkindi)]

fix(hooks): resolve quoted plugin-root refs and warn on the ones that survive

TL;DR

_rewrite_command_for_target recognised a plugin-root reference only when a
path separator followed the closing brace immediately. A hook command that
closes the quote first, python3 "${CLAUDE_PLUGIN_ROOT}"/hooks/probe.py,
matched nothing, so no script was copied, the command was never rewritten, and
no warning was emitted.

This implements both halves of the triage verdict:

1. A pre-pass normalises "${VAR}"/rest to "${VAR}/rest" before the
   finditer call, turning the unsupported spelling into the one that is
   already matched, already rewritten, and already covered by tests.
2. A post-loop pass scans the rewritten command and warns on any plugin-root
   reference that survived, so the diagnostic no longer depends on the matcher
   succeeding.

Closes #2639.

Problem (WHY)

• The matcher required [\\/] directly after \}. With a quote in that
  position re.finditer yields nothing.
• The Hook script not found warning lived inside the finditer loop
  body. Zero matches means zero iterations means zero warnings, so the failure
  was completely silent: apm install exits 0 and reports success.
• The raw ${CLAUDE_PLUGIN_ROOT} then lands in the deployed config, where
  the target rejects it on every matching tool call. That is the symptom [BUG] apm install --target claude writes ${CLAUDE_PLUGIN_ROOT} into ~/.claude/settings.json where Claude Code refuses to expand it #1310
  reported; [BUG] apm install --target claude writes ${CLAUDE_PLUGIN_ROOT} into ~/.claude/settings.json where Claude Code refuses to expand it #1310's fix covers the unquoted spelling, and this is the residual
  case it does not reach.
• "${VAR}"/path is valid, idiomatic shell. It quotes the segment that can
  contain spaces, so plugin authors write it and get a silently dead hook.

Approach

Normalisation, not a wider group. Widening the matcher to accept an optional
quote is the trap the issue calls out: the matched span would then run past the
closing quote, so substituting the resolved path leaves the opening quote
dangling and emits an unbalanced command. Moving the closing quote first yields
the fully-quoted spelling the existing code already handles, including its
quote-detection for project-scoped rewrites. The matcher, the rewrite and the
warning branch are untouched.

Residual scan. The loop now records the references it handled, and a pass
after it reports any plugin-root reference still present in the rewritten
command that the loop never saw. A matched-but-missing script therefore still
warns exactly once, through the existing Hook script not found path, rather
than twice.

One judgement call worth your eyes: a reference with no path, such as
echo "${CLAUDE_PLUGIN_ROOT}", is left unrewritten by design and is now also
reported by the residual scan. I read the verdict's "catches all unresolved
references regardless of quoting" as covering it, and an unexpanded plugin-root
variable in a deployed settings.json is rejected by the target at runtime
either way. If you would rather that shape stayed quiet, it is a one-line change
to require a path segment.

Validation evidence

Fail-before and pass-after were both executed, per half.

Normalisation, tests present and hook_integrator.py at main:

4 failed, 169 deselected in 1.54s

The warning test fails as assert 'Hook script not found' in '', which is the
silent skip this issue is about.

Residual scan, tests present and only the normalisation applied:

2 failed, 2 passed, 173 deselected in 0.88s
FAILED ...::test_unparseable_plugin_root_reference_warns
FAILED ...::test_mismatched_quotes_around_plugin_root_warns

Both fail as CaptureResult(out='', err=''). The two "must stay quiet" guard
tests pass in both arms, as they should.

All eight new tests with both halves applied:

8 passed, 169 deselected in 0.83s

Behaviour, same package, target claude:

command	before	after
python3 ${CLAUDE_PLUGIN_ROOT}/hooks/probe.py	rewritten	rewritten
python3 "${CLAUDE_PLUGIN_ROOT}/hooks/probe.py"	rewritten	rewritten
python3 "${CLAUDE_PLUGIN_ROOT}"/hooks/probe.py	leaked, 0 scripts, silent	rewritten
python3 '${CLAUDE_PLUGIN_ROOT}'/hooks/probe.py	leaked, 0 scripts, silent	rewritten
echo "${CLAUDE_PLUGIN_ROOT}"	silent	unchanged, warns
python3 "${CLAUDE_PLUGIN_ROOT}'/hooks/probe.py (mismatched)	silent	unchanged, warns
${CLAUDE_PLUGIN_ROOT}/hooks/absent.py	warns once	warns once
npx prettier --check .	silent	silent

Output quoting stays balanced in both the double- and single-quote cases.
Mismatched quotes are deliberately not normalised: the (?P=quote)
backreference requires the pair to match.

Suite, pytest tests/unit/integration/ -q -p no:randomly:

• main: 6 failed, 1935 passed, 11 skipped, 4 errors
• this branch: 6 failed, 1943 passed, 11 skipped, 4 errors

Same failure set, +8 passed being the new tests. Those 10 pre-existing
failures and errors are all symlink tests and are an artifact of my environment,
Windows 11 Home without Developer Mode, where os.symlink raises
OSError: [WinError 1314]. They reproduce with identical test names on
unmodified main, so they are unrelated to this change.

I did not run the full tests/unit suite to completion locally; it exceeded my
ten-minute cap. ruff check and ruff format --check pass on both changed
files, and hook_integrator.py is 2139 lines, under the 2450 guardrail.

How to test

pytest tests/unit/integration/test_hook_integrator.py -k "plugin_root or quote_before_separator" -q

Or against a real install, using the two-plugin reproduction in #2639: the
probe-broken plugin now deploys its script and its command is rewritten,
matching probe-fixed.

---

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

Spec conformance (OpenAPM v0.1)

• Adds consumer requirement req-tg-012 for split-quoted plugin-root
  resolution and default-visible unresolved-reference diagnostics.
• Updates the requirements manifest, Appendix C, consumer enumerations,
  revision history, and generated CONFORMANCE.md / CONFORMANCE.json.
• Adds a functional conformance test bound to req-tg-012.

Scenario Evidence

#	Scenario	Principle(s)	Test(s) proving it	Type
1	Double- and single-quoted split roots	Secure by default; DevX	tests/unit/integration/test_hook_integrator.py::TestScriptPathRewriting::test_rewrite_plugin_root_with_quote_before_separator; tests/spec_conformance/test_manifest_reqs.py::test_plugin_root_hook_resolution_preserves_quoting_and_warns	Unit; integration-with-fixtures
2	Escaped spaces and adjacent shell operators	Secure by default; DevX	tests/unit/integration/test_hook_integrator.py::TestScriptPathRewriting::test_quote_before_separator_preserves_escaped_space; ::test_quote_before_separator_stops_before_shell_operator	Unit
3	Source-install lifecycle	Multi-harness support; DevX	tests/integration/test_hook_event_native_lifecycle.py::test_claude_install_handles_quoted_and_malformed_plugin_roots	E2E
4	Malformed, missing, traversal, and terminal-control inputs	Secure by default	tests/unit/integration/test_hook_integrator.py::TestScriptPathRewriting warning regressions	Unit
5	Canonical placeholder vocabulary	Portability by manifest; OSS	tests/integration/test_architecture_contract_guards.py::test_plugin_root_hook_command_vocabulary_has_one_owner; AC15d	Integration-with-fixtures; static

Maintainer validation after rebasing on current main: 186 focused hook/lifecycle tests
passed; 51 exact-head owner/conformance tests passed; the spec orphan check aligned
all 114 requirements. Ruff check and format check pass. The local full conformance
run passed 161 tests with one unrelated environment-only failure because the system
Python lacks the optional toml package; CI runs with the locked dev environment.

---

I used an AI coding assistant while preparing this change. I wrote the
reproduction, ran the fail-before and pass-after for each half myself on the
numbers quoted above, and verified the baseline against unmodified main before
attributing any failure to this change. The repository states no AI policy, so I
am disclosing rather than assuming it does not matter.

[Mohammed Alkindi (MohammedAlkindi)]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Fixes a gap in hook command rewriting where plugin-root variables were not detected when a closing quote appeared between the variable and the path separator (e.g. "${CLAUDE_PLUGIN_ROOT}"/hooks/...), and ensures unresolved plugin-root references always produce a warning.

Changes:

• Normalize "${...}"/path and '${...}'/path into the already-supported fully-quoted spelling before plugin-root matching/rewrite.
• Add a post-rewrite residual scan that warns on any remaining ${*_PLUGIN_ROOT...} references not handled by the main matcher.
• Add focused unit tests covering quoted-split spellings, residual warnings, and non-duplication of warnings.

Reviewed changes

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

File	Description
src/apm_cli/integration/hook_integrator.py	Adds pre-normalization for quoted-split plugin-root references and a residual post-pass warning for unresolved plugin-root tokens.
tests/unit/integration/test_hook_integrator.py	Adds regression tests covering quoted-split rewrites, residual warnings, and “warn once” behavior.

---

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: needs_rework

Repair quoted Claude plugin-root rewriting and validate hook installation before this compatibility fix is ready to ship.

cc Mohammed Alkindi (@MohammedAlkindi) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

This initial pass found that the quoted-root rewrite belongs in HookIntegrator and has acceptable linear command-processing cost, but a single-quoted project-root reference would leave Claude's project variable unexpanded. The in-scope repair also needs lifecycle proof and visible diagnostics for malformed or traversal-shaped references retained in a command.

Aligned with: Secure by default: retained traversal-shaped references need a visible diagnostic. Multi-harness support: quoted roots must normalize consistently for Claude. Pragmatic as npm: actionable diagnostics and lifecycle proof make the compatibility repair dependable.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	0	Canonical owner is sound; lifecycle fixture proof is missing.
CLI Logging Expert	0	2	0	Add actionable diagnostics and lifecycle evidence.
DevX UX Expert	1	2	0	Single-quoted Claude roots do not expand; fix and prove install behavior.
Supply Chain Security Expert	0	1	0	Traversal-shaped references need residual visibility.
OSS Growth Hacker	0	1	0	Actionable warnings preserve user trust; no release work needed.
Test Coverage Expert	0	2	0	Unit tests are sub-tier for hook install; add lifecycle regression proof.
Performance Expert	0	0	0	New scans are linear in command length.

B = blocking-severity findings, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.

Top follow-ups

1. [DevX UX Expert] Validate single-quoted project-root expansion through hook installation -- Claude cannot expand a variable inside single quotes.
2. [Test Coverage Expert] Add an end-to-end lifecycle scenario for quoted and malformed roots -- unit coverage does not prove installed hook behavior.
3. [Supply Chain Security Expert] Warn when traversal-shaped references remain after containment handling -- users need visibility into retained commands.
4. [CLI Logging Expert] Make unsupported hook syntax warnings actionable -- users need a repair path.

Recommendation

The listed follow-ups are in scope and are being folded into this PR. Once the corrected rewrite and lifecycle proof are validated, no strategic, docs, auth, or performance expansion is needed.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[Daniel Meppiel (danielmeppiel)]

APM Spec Guardian: fold_and_ship

Scope: editorial-patch; initial spec diff = +20/-4 lines across 1 normative artifact. Shocked-meter avg: 7/10.

Four independent spec lenses found the new req-tg-012 mechanically well integrated. Their convergent asks were narrow: fix the single-quote/double-quote ambiguity, define the implementation-recognized placeholder set, and name both path separators. All were folded before the final push.

Convergence

Panel	Stance	Shocked	High-signal	Recommended	Nits
Swagger / OpenAPI editor	ship_with_followups	7/10	0	4	1
OCI distribution editor	ship_with_followups	7/10	0	3	1
Package-manager contract editor	ship_with_followups	7/10	0	4	1
Web architecture editor	ship_with_followups	7/10	0	2	1

Folded in this pass

1. req-tg-012 now requires one expansion-capable double-quoted span regardless of the source quote character.
2. The requirement consistently uses implementation-defined plugin-root placeholder and gives ${NAME} / ${PLUGIN_ROOT} syntax guidance.
3. Forward slash and backslash are named explicitly, with a worked split-to-normalized example.
4. The manifest note, conformance artifacts, indexes, counts, revision history, and functional conformance test remain aligned at 114 statements (109 MUST, 5 SHOULD).

Linter note

Checks 1-10 passed: printable ASCII, forbidden-token scan, schema and fixture parsing, anchor uniqueness, count consistency, links, fixture citations, and CHANGELOG binding. Check 11 observed runtime Python changes, so the general review panel ran in parallel; terminal CI Lint, Spec conformance, and Test Architecture Ratchets are green.

Not folded in this scope

The OCI lens suggested refusing install or dropping a hook entry when a plugin-root path escapes its package. That changes this issue's warn-only contract and needs a dedicated threat-model and migration design rather than a behavior expansion here.

_{This review is advisory. Re-apply the spec-review label for another pass.}

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_now

Community-reported plugin-root hook parsing is now fixed with one canonical path parser, cause-specific warnings, and mutation-proven coverage.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

This PR converges cleanly. It centralizes command path parsing behind one owner, stops capture at shell operators while preserving escaped spaces, and replaces conflated diagnostics with sanitized, package-aware remediation for missing files, traversal, bare tokens, and malformed quotes. Architecture, CLI UX, docs, growth, security, and test findings were folded across four bounded iterations. Auth and performance were correctly inactive.

The one deferred idea is a deliberate behavior expansion: refusing install or dropping a hook entry on path traversal instead of warning. The spec and general panels agree that this needs its own threat-model and migration design rather than riding on this warn-only bug fix.

Aligned with: secure defaults through sanitized and tested diagnostics; governed behavior through OpenAPM req-tg-012; community trust through a root-cause fix; predictable package-manager behavior through one canonical parser.

Panel summary

Persona	B	R	N	Takeaway
Python architect	0	0	1	Canonical owner and dual guardrail are clean.
CLI logging expert	0	1	0	Cause-specific remediation was folded.
DevX UX expert	0	1	0	Bare-token recovery now matches the actual cause.
Supply-chain security expert	0	0	1	No new vulnerability; adversarial parser coverage added.
OSS growth hacker	0	0	2	CHANGELOG now shows concrete before/after syntax and anchored spec path.
Doc writer	0	1	1	Recovery cross-link and duplicate-prose cleanup were folded.
Test coverage expert	0	1	0	Exact warning wording and scenario evidence are now guarded.

Counts show the signals raised during the terminal pass; every in-scope item above was folded before this comment.

Top follow-up

1. [Security] Open a dedicated design issue for fail-or-drop handling of escaping plugin-root hook paths -- it changes the current warn-only contract and needs migration framing.

Architecture

classDiagram
    direction LR
    class hook_command_paths {
      <<CanonicalOwner>>
      +PLUGIN_ROOT_NAMES tuple
      +normalize_quoted_plugin_root(command) str
      +iter_plugin_root_paths(command) Iterator
      +plugin_root_relative_path(path) str
      +unresolved_plugin_root_references(command) tuple
      +iter_relative_script_paths(command) Iterator
    }
    class hook_command_warnings {
      <<DiagnosticPolicy>>
      +warn_unresolved_plugin_root(command, reference, package_name)
    }
    class HookIntegrator {
      <<Consumer>>
      -_rewrite_command_for_target(command, package_path, package_name, target) tuple
    }
    HookIntegrator ..> hook_command_paths : tokenizes paths
    HookIntegrator ..> hook_command_warnings : renders recovery
    hook_command_warnings ..> hook_command_paths : classifies residual shape

Loading

Folded in this run

• Canonicalized plugin-root and relative hook path parsing in hook_command_paths.py, with AC15d and an architecture test.
• Preserved escaped spaces/operators while stopping unescaped &&, ;, and | from becoming path text.
• Added sanitized, package-aware, deduplicated and cause-specific diagnostics.
• Added unit, source lifecycle, spec-conformance, owner, and long-adversarial-input coverage.
• Added and refined OpenAPM req-tg-012, generated conformance artifacts, author docs, install reference, usage resource, CHANGELOG, and PR Scenario Evidence.

Deferred (scope follow-up)

• Evaluate fail-or-drop traversal handling in a dedicated threat-model/design issue; changing warning behavior to install refusal crosses this PR's stated contract.

Regression-trap evidence

• Removing split-quote normalization failed its behavior and req-tg-012 tests.
• Removing AC15d failed the architecture authority test.
• Removing long-input normalization failed the adversarial-input test.
• Forcing either residual-warning cause failed its exact remediation test.

Lint contract

CI Lint job 97202244811 passed on exact head, including ruff, format, file length, duplication, YAML I/O, portable paths, and auth boundaries.

CI

All terminal checks are green at https://github.com/microsoft/apm/actions/runs/32642638389. Spec conformance, Test Architecture Ratchets, Lifecycle Smoke, Windows, CodeQL, docs build, NOTICE, CLA, and the aggregate gate also pass.

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2645	0030839373b5	ship_now	4	7	1	1	green	MERGEABLE	BLOCKED	GitHub review requirement remains.

Recommendation

Ship after maintainer review. All current-scope findings are folded, the exact-head owner gate is semantically verified, and terminal CI is green.

_{This panel is advisory. Re-apply the panel-review label for another pass.}