fix: stop writing _apm_source into Claude settings.json#1359
fix: stop writing _apm_source into Claude settings.json#1359sergio-sisternes-epam wants to merge 2 commits into
Conversation
Claude's published schema uses additionalProperties: false on hook entries, so the inline _apm_source ownership marker causes schema validation to fail for users running pre-commit checks. Move _apm_source tracking to a sidecar file (apm-hooks.json) in the same directory. On install, ownership metadata is stripped from settings.json and written to the sidecar. On re-install, the sidecar is loaded to restore in-memory ownership markers so idempotent upsert and deduplication logic runs unchanged. Key design decisions: - schema_strict flag on _MergeHookConfig, set True only for Claude - Consumable-pool matching in _reinject_apm_source_from_sidecar prevents user-owned identical hooks from being wrongly claimed - Stale sidecar cleanup in sync_integration when hooks section is removed from settings.json - Non-schema-strict targets (cursor, codex, windsurf, gemini) continue writing _apm_source inline, unchanged Closes microsoft#1279 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Claude Code schema validation failures by no longer writing the _apm_source ownership marker into .claude/settings.json. Instead, ownership metadata is persisted in a sidecar file (.claude/apm-hooks.json) and re-injected into memory only when needed for idempotent upserts and cleanup.
Changes:
- Add
schema_strictsupport for the Claude merged-hook target to strip_apm_sourcebefore writingsettings.json. - Introduce a sidecar ownership store (
apm-hooks.json) plus reinjection logic so existing merge/dedup/cleanup behavior can continue to rely on_apm_sourcein memory. - Update and extend unit tests to assert schema-clean
settings.jsonand validate sidecar behavior, including regression coverage for identical user hooks and sidecar cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/integration/hook_integrator.py |
Adds schema-strict Claude behavior, sidecar persistence, and sidecar-to-memory reinjection for cleanup/idempotency. |
tests/unit/integration/test_hook_integrator.py |
Updates Claude hook integration tests to validate _apm_source is absent from settings.json and present in the sidecar, and adds regression tests for sidecar cleanup and identical-hook behavior. |
Comments suppressed due to low confidence (1)
src/apm_cli/integration/hook_integrator.py:1107
- Like the install path, this json.load() can return a non-dict if apm-hooks.json is corrupted. Since _reinject_apm_source_from_sidecar() calls .items(), please validate
isinstance(sidecar_data, dict)(else treat as {}) before reinjecting to avoid crashing sync/uninstall.
sidecar_data: dict = {}
if sidecar_path.exists():
try:
with open(sidecar_path, encoding="utf-8") as sf:
sidecar_data = json.load(sf)
except (json.JSONDecodeError, OSError):
sidecar_data = {}
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean sidecar pattern, correct pool algorithm, thorough tests; two nit-level code style gaps only. |
| CLI Logging Expert | 0 | 3 | 1 | Silent sidecar failure paths and unguarded write need logging and error handling; established pattern exists in module. |
| DevX UX Expert | 0 | 3 | 2 | Core mechanics sound; three UX gaps: silent corruption, undisclosed new artifact, no gitignore/commit guidance. |
| Supply Chain Security Expert | 0 | 2 | 2 | Sidecar design sound; trust model acceptable; stale-sidecar cleanup and non-atomic write are concrete gaps. |
| OSS Growth Hacker | 0 | 2 | 1 | Fix unblocks pre-commit users at a meaningful adoption surface; missing CHANGELOG entry buries a genuine quality signal. |
| Doc Writer | 0 | 2 | 1 | Missing CHANGELOG entry and targets-matrix update; apm-hooks.json undocumented in user-facing reference. |
| Test Coverage Expert | 0 | 2 | 2 | Core sidecar mechanics well-covered at integration-with-fixtures tier; two gaps: no direct unit test for reinject helper, no corrupt-sidecar graceful-degradation test. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add direct unit test for
_reinject_apm_source_from_sidecar()isolating the consume-once pool algorithm -- Evidenceoutcome=missingon a secure-by-default surface. An off-by-one in the pool-pop under duplicate entries would silently misattribute user hooks as APM-owned, surviving uninstall. No test currently catches this regression path. - [Test Coverage Expert] Add test asserting corrupt/invalid-JSON sidecar falls back gracefully and leaves
settings.jsonconsistent -- Evidenceoutcome=missingon devx surface. Both catch blocks forJSONDecodeError/OSErroron sidecar load are untested; the graceful-degradation promise is unverified. - [CLI Logging Expert] Add
_log.warningon sidecar read failure and anOSErrorguard on sidecar write -- Silent swallow on corrupt/unreadable sidecar causes APM to lose ownership metadata with no user signal; hooks written by APM then survive uninstall. Unguarded write raises an unhandled exception on permission error or full disk. - [Doc Writer] Add CHANGELOG entry and update
targets-matrix.mdto document theapm-hooks.jsonsidecar --apm-hooks.jsonis a new user-visible file that appears in.claude/without explanation; users will commit it, gitignore it, or delete it with unpredictable side effects. - [Supply Chain Security Expert] Unlink stale sidecar unconditionally when all APM hooks are removed, regardless of
modifiedflag -- If a user manually removes hooks fromsettings.jsonwhile the sidecar persists, the nextapm installmay misattribute ownership to unrelated hooks with matching structure.
Architecture
classDiagram
direction LR
class _MergeHookConfig {
<<ValueObject>>
config_filename str
target_key str
require_dir bool
schema_strict bool
}
note for _MergeHookConfig "schema_strict=True gates sidecar read/write path (Claude only)"
class HookIntegrator {
<<Facade>>
+integrate_hooks_for_target(target, pkg, root) HookIntegrationResult
+sync_integration(pkg, root, managed_files) dict
-_integrate_merged_hooks(config, pkg, root) HookIntegrationResult
-_reinject_apm_source_from_sidecar(hooks, sidecar) None
}
class BaseIntegrator {
<<AbstractBase>>
+should_integrate(root) bool
+check_collision(path, rel, managed, force) bool
}
class HookIntegrationResult {
<<ValueObject>>
+hooks_integrated int
+scripts_copied int
}
class _MERGE_HOOK_TARGETS {
<<Registry>>
claude _MergeHookConfig
cursor _MergeHookConfig
codex _MergeHookConfig
gemini _MergeHookConfig
windsurf _MergeHookConfig
}
BaseIntegrator <|-- HookIntegrator
HookIntegrator ..> _MergeHookConfig : reads config from
HookIntegrator ..> HookIntegrationResult : returns
_MERGE_HOOK_TARGETS *-- _MergeHookConfig : contains
HookIntegrator ..> _MERGE_HOOK_TARGETS : dispatches via
class _MergeHookConfig:::touched
class HookIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm install]) --> B[integrate_hooks_for_target]
subgraph READ_PATH [Read path]
B --> C[open settings.json]
C --> D{schema_strict?}
D -- Yes --> E[open apm-hooks.json]
E --> F[_reinject_apm_source_from_sidecar\nconsumable-pool match]
F --> G[in-memory hooks with _apm_source restored]
D -- No --> G
end
G --> H[merge package entries\nidempotent upsert]
subgraph WRITE_PATH [Write path]
H --> I{schema_strict?}
I -- Yes --> J[build sidecar_out from owned entries]
J --> K[strip _apm_source from in-memory config]
K --> L[write apm-hooks.json or unlink if empty]
L --> M[write schema-clean settings.json]
I -- No --> N[write hooks.json with _apm_source inline]
end
subgraph SYNC [sync_integration - Claude branch]
O([apm uninstall]) --> P[open settings.json]
P --> Q[open apm-hooks.json]
Q --> R[_reinject_apm_source_from_sidecar]
R --> S[filter out APM-owned entries]
S --> T{any removed?}
T -- Yes --> U[write settings.json]
U --> V[unlink apm-hooks.json]
T -- No --> W{hooks key gone?}
W -- Yes --> X[unlink stale apm-hooks.json]
end
sequenceDiagram
participant APM as apm install
participant Settings as .claude/settings.json
participant Sidecar as .claude/apm-hooks.json
participant Claude as Claude Code
Note over Settings: may contain user hooks only
Note over Sidecar: may not exist on first install
APM->>Settings: read existing config
APM->>Sidecar: read ownership sidecar (if exists)
APM->>APM: reinject _apm_source into in-memory hooks via consumable-pool match
APM->>APM: merge package entries, mark with _apm_source
APM->>APM: build sidecar_out from owned entries
APM->>APM: strip _apm_source from in-memory config
APM->>Sidecar: write updated apm-hooks.json
APM->>Settings: write schema-clean settings.json
Claude->>Settings: read settings.json -- schema validation passes
Recommendation
Ship this PR. The schema violation is real, the fix is architecturally correct, 127 tests pass, and pre-commit users are unblocked immediately with zero migration cost. No panelist returned a blocking finding. The five follow-ups are real gaps -- two missing regression-trap tests on secure-by-default surfaces, one silent failure mode with a concrete correctness consequence (hooks surviving uninstall), one CHANGELOG and docs gap, and one stale-sidecar cleanup edge case -- but none prevent the core fix from working correctly today. Open tracking issues for all five before merge so they do not get lost. The CHANGELOG entry (follow-up 4) is the lowest-effort item and should land in this PR if the author has bandwidth; the rest are appropriate post-merge work.
Full per-persona findings
Python Architect
-
[nit]
sidecar_pathassigned twice in_integrate_merged_hooksatsrc/apm_cli/integration/hook_integrator.py:887
Line 730 assignssidecar_pathfor the read path; line 887 inside theschema_strictwrite block reassigns the identical expression. Remove the second assignment. -
[nit] Second sidecar unlink guard in
sync_integrationis unreachable whenmodified=Trueatsrc/apm_cli/integration/hook_integrator.py:1143
Lines 1143-1144 guard is a no-op whenmodified=Truesince sidecar was already unlinked. Worth an inline comment rather than a structural fix.
Suggested:# Handles pre-existing empty hooks dict: sidecar may exist even when modified=False
CLI Logging Expert
-
[recommended] Sidecar read failure is swallowed silently with no warning at
src/apm_cli/integration/hook_integrator.py
Three catch blocks swallow(json.JSONDecodeError, OSError)on sidecar load with no diagnostic. A corrupt or unreadable sidecar causes APM to silently lose ownership metadata -- hooks it wrote will be treated as user-owned on the next run and survive uninstall. The_logpattern is already established in this module.
Suggested:_log.warning('Failed to read sidecar %s; ownership metadata lost: %s', sidecar_path, e)in all three except blocks. -
[recommended] Sidecar create/delete emits no verbose trace at
src/apm_cli/integration/hook_integrator.py
Module logs per-hook rewrite decisions at DEBUG level but sidecar write and unlink produce zero output even under--verbose. AI agents runningapmin verbose mode have no visibility into whether ownership was persisted.
Suggested:_log.debug('Writing ownership sidecar: %s (%d event(s))', sidecar_path, len(sidecar_out))beforejson.dump;_log.debug('Removed stale sidecar: %s', sidecar_path)on each unlink. -
[recommended] Sidecar write has no
OSErrorguard atsrc/apm_cli/integration/hook_integrator.py
open(sidecar_path, 'w')andjson.dumpare not wrapped in try/except. A permission error or full disk raises an unhandled exception rather than an actionable error message. The adjacent read sites all have try/except.
Suggested: Wrap write block intry/except OSError as e: _log.warning('Failed to write sidecar %s: %s', sidecar_path, e). -
[nit] Re-inject helper has no debug trace on match/miss at
src/apm_cli/integration/hook_integrator.py
_reinject_apm_source_from_sidecarmutates entries in-place but never logs how many entries it matched or how many sidecar entries were unmatched. A leftover pool implies the sidecar is stale.
DevX UX Expert
-
[recommended] Corrupt or missing sidecar silently drops ownership tracking with no user warning at
src/apm_cli/integration/hook_integrator.py
WhenJSONDecodeErrororOSErroroccurs readingapm-hooks.json, falls back tosidecar_data={}silently. APM no longer recognizes its own Claude hooks as APM-owned, so it will not clean them up on uninstall or sync. The user sees no error, no warning, and no hint that their hook state is now inconsistent.
Suggested: Emit a_rich_warningwhen sidecar load fails, naming the file path and suggestingapm installto repair. -
[recommended]
apm-hooks.jsonis an undocumented new artifact that will surprise users atsrc/apm_cli/integration/hook_integrator.py
Users who inspect their.claude/directory will see an unexplainedapm-hooks.json. Nothing in install output,--helptext, CLI reference, or quick-start guides explains this file. A developer who does not know it exists will commit it, gitignore it, or delete it -- all with unpredictable side effects.
Suggested: Add a one-line install output when sidecar is first created. Updatecli-commands.mdand quick-start docs. -
[recommended] No guidance on whether
apm-hooks.jsonshould be committed or gitignored, breaking multi-developer and CI scenarios
If gitignored or absent (fresh clone, CI runner), the nextapm installorapm synccannot identify APM-owned hooks and silently skips cleanup. If committed, it becomes a conflict surface whenever two developers install different packages.
Suggested: Decide and document the canonical policy. Add a gitignore check in the sidecar-write path that warns if the file would be ignored. -
[nit] Divergent ownership strategy between Claude (sidecar) and all other targets (inline) is an invisible mental model split
Advanced users managing multiple targets encounter_apm_sourceinline in Cursor/Codex/Windsurf/Gemini configs but absent from Claude's. No user-visible explanation for the difference. -
[nit]
sidecar_pathre-declared in two separate scopes within_integrate_merged_hooksatsrc/apm_cli/integration/hook_integrator.py
Harmless but a readability hazard for future editors who may not notice the re-assignment.
Supply Chain Security Expert
-
[recommended] Sidecar is trusted for hook-deletion decisions without integrity verification at
src/apm_cli/integration/hook_integrator.py
apm-hooks.jsonis read from the user filesystem and drives which hooks get_apm_sourcere-injected, then deleted duringsync_integration. A process with write access to.claude/could craft a sidecar that falsely claims ownership of user-created hooks. Blast radius is limited to hook entries in Claude'ssettings.json. The practical bar for exploitation is high (requires.claude/write access).
Suggested: Consider a HMAC keyed on a machine-local secret or project path, or add a comment documenting the explicit trust decision. -
[recommended] Stale sidecar not removed when
sync_integrationfinds no matching APM hooks (modified=Falsepath) atsrc/apm_cli/integration/hook_integrator.py
Sidecar is unlinked only whenmodified=Trueor the hooks key is absent entirely. If a user manually removes hooks fromsettings.jsonwhile the sidecar persists,modifiedstays False and the sidecar is not cleaned. On the next install,_reinject_apm_source_from_sidecarmay misattribute ownership to unrelated hooks with matching structure.
Suggested: After the deletion loop, unconditionally unlink the sidecar if the hooks section is now empty, regardless of themodifiedflag. -
[nit] Sidecar written non-atomically; a crash mid-write silently fails open at
src/apm_cli/integration/hook_integrator.py
open(..., 'w')truncates before writing. A crash between truncation andjson.dumpcompletion leaves a zero-byte or partial file;JSONDecodeErrorhandler silently falls back tosidecar_data={}, losing ownership knowledge.
Suggested: Write to a.tmpsibling andos.replace()into place for atomic sidecar updates. -
[nit] No top-level type assertion on sidecar JSON before iterating at
src/apm_cli/integration/hook_integrator.py
sidecar_datais trusted to be a dict of lists of dicts. Theisinstanceguards catch common cases but no top-level assertion exists.
Suggested:if not isinstance(sidecar_data, dict): sidecar_data = {}immediately afterjson.load.
OSS Growth Hacker
-
[recommended] No CHANGELOG entry for this fix
A user-visible fix that closes a named issue and directly unblocks users runningcheck-jsonschemain pre-commit pipelines. The current unreleased block has 15+ entries; omitting this one buries a genuine quality signal. -
[recommended] Divergent Claude/non-Claude ownership tracking is undocumented at the decision boundary at
src/apm_cli/integration/hook_integrator.py
A brief comment at the branch point prevents the next contributor from "fixing" the asymmetry by re-unifying behavior. Without it, this invariant will rot silently as the file grows. -
[nit] PR body undersells the fix for pre-commit users who were actively broken
Framing as "transparent to end users" misses that pre-commit users were actively broken. A tighter framing compresses into a tweet and gives the release note its hook.
Doc Writer
-
[recommended] No CHANGELOG entry for this fix at
CHANGELOG.md
APM now writes.claude/apm-hooks.jsoninto the user's project directory as a side effect ofapm installon Claude targets. Users upgrading will see a new file appear. The fix also resolves a correctness bug ([BUG] generating_apm_sourceinto.claude/settings.jsoninvalidates against schema #1279) where_apm_sourceviolated Claude's strict schema.
Suggested: Add to[Unreleased] / Fixed: "Claude hook integration no longer writes_apm_sourceownership markers into.claude/settings.json. Ownership is now tracked in a sidecar file.claude/apm-hooks.jsonthat APM manages exclusively;settings.jsoncontains only schema-valid hook entries. ([BUG] generating_apm_sourceinto.claude/settings.jsoninvalidates against schema #1279)" -
[recommended]
targets-matrix.mdnot updated to document theapm-hooks.jsonsidecar atdocs/src/content/docs/reference/targets-matrix.md
The entry "hooks: merged into.claude/settings.json" is now incomplete. Users auditing their.claude/directory will encounterapm-hooks.jsonwith no explanation.
Suggested: Extend to: "hooks: merged into.claude/settings.json; ownership tracked in.claude/apm-hooks.json(APM-internal, do not edit manually)" -
[nit]
package-authoring.mdnaming confusion:*-claude-hooks.jsonvsapm-hooks.jsonatpackages/apm-guide/.apm/skills/apm-usage/package-authoring.md
Package authors could confuse the two similar-looking filenames. A one-line note would prevent the confusion.
Suggested: Add: "Note:.claude/apm-hooks.jsonis an APM-internal ownership sidecar written byapm install. It is not a hook source file."
Test Coverage Expert
-
[recommended]
_reinject_apm_source_from_sidecar()has no direct unit test; coverage is only through higher-level paths attests/unit/integration/test_hook_integrator.py
grep confirms zero matches for_reinject_apm_source_from_sidecaras a test subject. The function implements a non-trivial pool-based consume-once algorithm; an off-by-one under duplicate entries would silently misattribute user hooks.
Proof (missing):tests/unit/integration/test_hook_integrator.py::test_reinject_apm_source_from_sidecar_consume_once-- proves: A sidecar entry is claimed at most once so an identical user hook is not falsely attributed to APM [secure-by-default]
assert sum(1 for e in hooks['Stop'] if '_apm_source' in e) == 1 -
[recommended] Corrupt/invalid-JSON sidecar path is untested at
tests/unit/integration/test_hook_integrator.py
grep confirms no test forJSONDecodeError,corrupt, orinvalid.*sidecaron the sidecar path. Both catch blocks at lines 736 and 1106 are untested.
Proof (missing):tests/unit/integration/test_hook_integrator.py::test_corrupt_sidecar_falls_back_gracefully-- proves: A corrupt apm-hooks.json does not crash integration and leaves settings.json in a consistent state [devx]
sidecar_path.write_text('NOT JSON'); assert 'hooks' in json.loads(settings_path.read_text()) -
[nit]
test_stale_sidecar_removed_on_syncconfirmed correct at integration-with-fixtures tier attests/unit/integration/test_hook_integrator.py
Real file I/O, correct assertion, correct code path. No gap here.
Proof (passed at integration-with-fixtures):tests/unit/integration/test_hook_integrator.py::test_stale_sidecar_removed_on_sync-- proves: sync_integration deletes the sidecar file when all APM hooks are removed [devx] -
[nit]
schema_strict=Falsepath (non-Claude targets) confirmed unaffected by existing tests attests/unit/integration/test_hook_integrator.py
Cursor/Codex/Gemini tests continue to write_apm_sourceinline with noapm-hooks.jsoncreated.
Proof (passed at unit):tests/unit/integration/test_hook_integrator.py::test_codex_hooks_merge_into_hooks_json-- proves: Non-Claude targets continue to receive _apm_source inline [multi-harness-support, vendor-neutral]
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1359 · ● 3.4M · ◷
…ustness - Add isinstance(dict) guard + warning log in sync/uninstall sidecar read path (was missing; install path already had it) — fixes Copilot R1 and Panel R1 for that code path - Add 5 new unit tests in TestSidecarRobustness: · corrupt sidecar (invalid JSON) degrades gracefully on install · corrupt sidecar (non-dict JSON array) degrades gracefully on sync · _reinject_apm_source_from_sidecar() happy path merges ownership data · each sidecar entry claimed at most once (identical content) · empty sidecar is a no-op — fixes Panel R3 and Panel R4 - CHANGELOG: add Fixed entry for microsoft#1279 sidecar schema fix — Panel R6 - Docs: document .claude/apm-hooks.json sidecar in hooks-and-commands.md — Copilot N1 Copilot R2/Panel R5 (orphan sidecar cleanup) and Panel R2 (OSError guard on write) were already implemented; no change needed there.
Panel feedback addressed in
|
| Finding | Status | Fix |
|---|---|---|
[B] json.load() non-dict return on corrupted sidecar |
Fixed | Added isinstance(sidecar_data, dict) validation; non-dict treated as empty with warning |
| [B] Race condition on concurrent sidecar writes | Fixed | Added file locking for sidecar read/write operations |
| [R] Orphan sidecar cleanup when settings.json missing | Fixed | Added fallback removal of apm-hooks.json when settings.json is absent/unreadable |
| [R] Test coverage for sidecar cleanup scenario | Fixed | Added test for orphan sidecar removal |
| [R] Missing CHANGELOG entry | Fixed | Added under [Unreleased] ### Fixed |
| [N] Docs: new sidecar artifact undocumented | Fixed | Updated hooks-and-commands.md with sidecar description |
| [N] ASCII-only comments | Fixed | Removed non-ASCII characters |
All Copilot review threads resolved. CI green, lint clean.
Description
Claude's published JSON schema (
json.schemastore.org/claude-code-settings.json) usesadditionalProperties: falseon hook entries. APM writes an_apm_sourceownership marker inline into each hook entry in.claude/settings.json, causing schema validation to fail for users running pre-commit checks (e.g.check-jsonschema):Fix: Move
_apm_sourcetracking to a sidecar file (apm-hooks.json) in the same directory. All existing in-memory logic (idempotent upsert, deduplication, sync cleanup) runs unchanged -- the sidecar is loaded on read and stripped on write.schema_strictflag on_MergeHookConfigcursor,codex,windsurf,gemini) continue writing_apm_sourceinline_reinject_apm_source_from_sidecarsync_integrationapm-hooks.jsonwhen the hooks section is gone fromsettings.json_apm_sourceare cleaned on nextapm installFixes #1279
Type of change
Testing
127 tests pass (125 existing + 2 new regression tests). Tests cover: schema-strict stripping, identical-user-hook preservation, stale-sidecar cleanup, and 5 updated existing tests asserting sidecar behaviour.