fix(hooks): JSON-aware claude settings upgrade preserves customizations#2072
Merged
sjarmak merged 7 commits intoMay 14, 2026
Merged
Conversation
Adds a discriminator that exercises pipex-city's exact shape: an extra managed PreCompact command stuck on the bare `gc handoff "context cycle"` form (legacy from before gastownhall#1825 introduced `--auto`), combined with a user-customized Stop hook event that the embedded template never ships. The byte-enumerated claudeFileNeedsUpgrade enumerates 16 transforms of the canonical template; any custom addition defeats every variant match. This left cities like pipex-city silently stuck on the bare-handoff form for weeks after 7b3b913 landed, because their non-canonical Stop hook prevented the upgrade gate from firing. The test fails on parent (PreCompact stays bare) and is paired with a JSON-aware upgrader in the follow-up commit that handles the customized shape correctly.
Replaces the byte-enumerated claudeFileNeedsUpgrade with a JSON-aware
walker (upgradeClaudeFile) that parses settings.json, iterates hook
events, and applies event-scoped command/matcher upgrades to managed
gascity commands while leaving any custom hook events or custom
commands verbatim.
desiredClaudeSettings now calls upgradeClaudeFile on the user override
before merging with the embedded base, so customizations survive while
managed commands get upgraded. The previous "use base instead" gate in
readClaudeSettingsOverride is removed: cities whose settings.json had
stale managed-hook commands AND user customizations no longer fall
through to the base, which silently discarded customizations.
Currently-known upgrades, all event-scoped:
- PreCompact: `gc prime --hook` (pre-handoff legacy) →
`gc handoff --auto "context cycle"`
- PreCompact: bare `gc handoff "context cycle"` →
`gc handoff --auto "context cycle"` (the gastownhall#1825 / 7b3b913 patch
that previously never propagated to customized cities)
- SessionStart: bare `gc prime --hook` →
`GC_MANAGED_SESSION_HOOK=1 GC_HOOK_EVENT_NAME=SessionStart gc prime --hook`
- SessionStart: empty matcher → "startup"
Custom hook events (Stop, custom UserPromptSubmit entries, etc.) and
any non-managed commands within a hook entry are untouched.
The companion test in a4418c18 (TestInstallClaudeUpgradesPreCompact
PreservingCustomHookEvent) exercises pipex-city's exact shape (bare
PreCompact + extra Stop hook): FAIL on parent, PASS here.
Observed locally: pipex-city had been stuck on bare-handoff for weeks
after 7b3b913 landed, every PreCompact event killing the session,
because the city's extra Stop hook defeated the byte-enumerate match.
…configs PR gastownhall#2072's initial JSON-aware upgrade used bare strings.Contains on 'gc prime --hook' and similar legacy command tokens. The adversarial multi-model review surfaced two real regressions: 1. isStaleHookFile (via claudeFileNeedsUpgrade) would treat any user-owned hooks/claude.json containing 'gc prime --hook' as a stale GC-managed file and overwrite it on Install, violating the 'remains user-owned unless gc can prove it is safe to update a stale generated copy' contract. 2. upgradeClaudeHookEntry would normalize ANY SessionStart entry with matcher:"" to matcher:"startup" and rewrite ANY command body containing the legacy substring, silently mutating user-authored wrappers like 'my-wrapper gc prime --hook --foo'. Fix: - Introduce startsWithLegacyCommandToken (HasPrefix + shell-token boundary check) so 'gc prime --hookable' and 'my-wrapper gc prime --hook --foo' no longer match. - Introduce commandBodyAfterCanonicalPrefix to strip the canonical PATH-export prefix so gc's own managed forms still upgrade while user-arbitrary prefixes do not. - Introduce isLegacyGCManagedCommand and gate matcher normalization on at least one hook command in the entry matching a known legacy form; user-authored entries with matcher:"" survive untouched. - Log unexpected upgrade failures (previously silent) so a MarshalCanonicalJSON regression would be discoverable. Tests added: - TestInstallClaudeDoesNotClobberUserWrappedCommand — regression for Codex finding 1. - TestInstallClaudeDoesNotNormalizeUserAuthoredEmptyMatcher — regression for Codex finding 2. - TestInstallClaudeIdempotent — code-reviewer finding (Codex path had a byte-stability test; Claude path did not). All gates green: go test -race ./internal/hooks/, go vet, make build, make lint (0 issues).
The pass-1 fixup blocked wrapper prefixes ("my-wrapper gc prime --hook")
via token-anchored prefix matching, but the token boundary accepted any
whitespace-bounded suffix — so user-authored commands like
"gc prime --hook --my-flag" or "gc prime --hook && echo extra" still
matched as managed and were silently rewritten on every gc run, with
matcher:"" normalized to "startup" alongside.
Tighten the legacy-form check to require exact-body match after the
canonical PATH-prefix strip. gc has only ever emitted these tokens as
the complete command body, never with trailing user content, so any
deviation is user authorship and must be left alone. Also anchor the
SessionStart env-var-prefix check on the full current-form preamble
rather than the bare "GC_MANAGED_SESSION_HOOK=" token.
Add two regression tests covering the suffix-append shapes
("gc prime --hook --my-flag" on SessionStart, "gc prime --hook && echo"
on PreCompact) — both fail against the prior token-prefix check and
pass against the exact-body check.
…cognition The iter-2 change tightened the legacy-form recognition to exact-body match but left the SessionStart current-form path using strings.HasPrefix(body, sessionStartCurrentFormBody) on the rationale of "tolerate future trailing args gc may add." That carve-out re-opens the same suffix-append class the prior commit closed: a user-authored command beginning with the canonical env-var preamble plus extra args (e.g. "GC_MANAGED_SESSION_HOOK=1 GC_HOOK_EVENT_NAME= SessionStart gc prime --hook --my-flag") still classified as managed, driving matcher:"" → "startup" normalization on a user-authored entry. Switch the current-form recognition to equalsLegacyCommandBody. If gc ever extends the current-form emission with additional arguments, update sessionStartCurrentFormBody alongside the emission site rather than relaxing the recognition gate. The function-level doc on isLegacyGCManagedCommand is updated so the exact-body invariant applies uniformly across all recognition paths. Add a regression test exercising the current-form suffix-append shape that fails against the iter-2 HasPrefix variant and passes under exact-body match.
github-actions
Bot
added
the
status/needs-triage
There was a problem hiding this comment.
Pull request overview
This PR makes Claude hook/settings upgrades JSON-aware so managed Gas City hook commands (and SessionStart matcher) can be upgraded in-place while preserving user customizations, fixing cases where byte-exact template matching prevented upstream hook fixes from propagating to customized cities.
Changes:
- Replace Claude settings “needs upgrade” detection with a JSON-walking upgrader (
upgradeClaudeFile) that applies event-scoped command/matcher upgrades without touching non-managed entries. - Apply the upgrader to the user override before merging with embedded defaults; remove the previous
!claudeFileNeedsUpgrade(...)gating that caused customized files to fall back to base. - Add regression tests covering preservation of custom hook events and preventing accidental rewrites of user-authored wrapper/suffix/chained commands; add an idempotence test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/hooks/hooks.go | Adds JSON-aware Claude settings upgrade logic; adjusts override selection/merge behavior to preserve customizations while upgrading managed hook commands. |
| internal/hooks/hooks_test.go | Adds focused regression coverage for customization-preserving upgrades and non-clobbering heuristics, plus an idempotence check. |
Comments suppressed due to low confidence (1)
internal/hooks/hooks.go:687
- upgradeClaudeFile’s doc comment says custom hook events/commands are preserved “verbatim”, but the function re-emits canonical JSON when it makes any change, so whitespace/key ordering won’t be preserved. Suggest rewording to avoid implying byte-level preservation.
// upgradeClaudeFile parses the existing Claude settings.json and patches
// known legacy forms of managed gascity hook commands and matchers to their
// current shape. Walks the hook events so upgrades can be event-aware
// (e.g. SessionStart matcher upgrade, PreCompact command upgrade); custom
// hook events and custom commands are preserved verbatim.
//
// Returns the (possibly re-marshaled) JSON bytes and whether any patch
// was applied.
Comment on lines
+327
to
+332
| // Upgrade failure (e.g., malformed JSON) — fall back to original | ||
| // override; better to keep the user file as-is than fail install. | ||
| // Log so unexpected upgrade failures are discoverable rather than | ||
| // silent: a malformed user file is benign here, but a | ||
| // MarshalCanonicalJSON failure would indicate a gascity bug. | ||
| log.Printf("hooks: claude settings upgrade failed, using original override: %v", upgradeErr) |
Comment on lines
+322
to
+324
| // preserved verbatim. The previous "use base instead" path discarded | ||
| // user customizations along with stale managed-hook bytes; this path | ||
| // patches the managed bytes while keeping customizations intact. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Take-the-good of #2021 by @realies. The 2 commits from #2021 cherry-pick cleanly against current main — the hooks.go conflict the original review anticipated has been absorbed by intervening main commits. One follow-up commit on top fixes two misspell lint findings (marshalled/marshalling → marshaled/marshaling, doc/error-string only). All gates green.
Co-authored-by: realies 5107843+realies@users.noreply.github.com
Closes #2021 via supersession.