Skip to content

fix(install-claude-hooks): sweep the other revision's spelling of a skill hook, not double it - #6165

Merged
qingyun-wu merged 1 commit into
mainfrom
fix/hook-sweep-skill-spellings
Nov 2, 2026
Merged

qingyun-wu merged 1 commit into
mainfrom
fix/hook-sweep-skill-spellings

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

What

On this Mac every agent-activity row, phase record and summary was written twice: .claude/settings.json carried the activity hook twice per event (PreToolUse, PostToolUse, Stop). The two entries are two spellings of the same hook by two revisions of the installer:

The stale-variant sweep matches an entry only when the text after the marker equals ours exactly (so an operator's customisation after the marker survives), so each installer leaves the other's entry in place.

Fix

A skill hook opens with the fail-open guard on its own path, and no operator writes that guard. The sweep now also treats an entry that opens with [ -f <our exact hook path> ] || exit 0; and carries the marker as ours, whatever its tail (GUARD_SHAPE, beside SHAPE and LEGACY_SHAPE). An unguarded operator hook that runs the same script is still left alone.

Cross-branch note

#3997 and #4309 must keep this sweep (or carry it), or whichever merges second collides with main's spelling again and the rows double once more. Each gets a comment pointing here.

Tests

Regression test: tests/install-claude-hooks.test.sh
Before fix: FAIL (2 of 56 checks: "other spelling: PreToolUse has exactly one guarded entry for the hook", "…Stop…")
After fix: PASS (56 checks)

Measured on this Mac before the fix: 71 of the newest 200 sidecar rows were identical pairs at +0.0 s. Found by the visibility gap audit (2026-11-02).

🤖 Generated with Claude Code

…kill hook, not double it

A skill hook starts with the fail-open guard on its own path. Another revision of the
installer (#3997, #4309) writes the same guard and then resolves the interpreter at event
time, so its tail after the marker differs and the exact-tail sweep kept both: every
activity row was written twice. An entry that opens with our guard for our path and
carries the marker is ours whatever follows; an operator's unguarded hook is still not.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent-assisted review at 6e5c325ba04a1d3a9d27125421cdf2921e70ca8c: changes needed for the stated “whatever its tail” invariant (COMMENT). The concrete alternate-interpreter spelling is fixed, and the unguarded customized operator entry survives, but the new ownership predicate is narrower than the described rule.

[P2] Do not require the marker a second time after the exact guard — src/install-claude-hooks.sh:284. GUARD_SHAPE is ^<escaped guard>.*<marker>. The guard already contains the own hook path and its marker; this pattern nevertheless demands another occurrence later in the command. The separate contains(marker) predicate also already checks presence. An entry opening with the exact fail-open guard and a wrapper-only tail is therefore not swept, despite satisfying the requested ownership rule.

Independent reproduction with the real installer in a throwaway repo/settings/home: seed [ -f <exact demo/hooks/g.py> ] || exit 0; exec my-hook-wrapper plus the unguarded python3 <same path> --mine; run the installer. Exit 0, guarded variant retained=True, operator retained=True, guarded count=2. The installed current command coexists with the owned guard variant. Match the exact opening guard without requiring a second marker in its tail (retaining the existing marker/ownership checks), and add a control whose tail contains no further marker. That also avoids the dot wildcard imposing an unintended single-line-tail restriction.

Validation: full install-claude-hooks suite passes 56 checks at this head. Holding its tests fixed and restoring the parent installer fails exactly two checks (alternate spelling remains duplicated for PreToolUse and Stop). Sources restored, worktree clean, git diff --check passes. Repository scan reports hardcoded-paths/root-artifacts clean and PARTIAL because prose-cap has no Python files in scope. CI still has pending jobs. Worst-case remaining impact is duplicate execution of a guarded hook variant. No live settings or runtime were changed, no push or merge.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 6e5c325. This fixes the doubled activity rows at the cause. The two installer revisions spell the same skill hook differently after the shared [ -f <path> ] || exit 0; guard, so each one's exact-tail sweep left the other's entry in place. GUARD_SHAPE now also counts an entry as ours when it opens with our exact guard for our exact hook path and carries the marker, whatever follows. The guard is taken from our own CMD and regex-escaped, so it can't match another hook's path. The tests set both spellings in settings.json plus an operator's unguarded python3 <hook> --mine. After install there is exactly one guarded entry per event, it is this installer's, and the operator's hook survives.

Non-blocking: the match depends on the other revision quoting the path the same way (shlex.quote on the resolved path). If a branch ever quotes it differently, the two entries would coexist again silently. The cross-branch comments on #3997 and #4309 cover this for now.

(agent-assisted, Blue for Chi's seat)

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

@qingyun-wu
qingyun-wu merged commit f6d99ff into main Nov 2, 2026
45 checks passed
@qingyun-wu
qingyun-wu deleted the fix/hook-sweep-skill-spellings branch November 2, 2026 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants