Repository navigation
Judge the Installer's Hook Ownership by File Name, Not Substring - #1954
Conversation
install.py decided whether a PreToolUse or SessionEnd hook registration belonged to this kit with a substring test on the whole command string (GUARD_STEM/SWEEP_STEM). A maintainer's own hook whose command merely contained one of those stems, such as `my-gh-write-guard-audit.sh`, was silently removed on every install and counted by `--report` as a broken registration of the kit's own hook. names_hook() now judges ownership by the exact file name a command's own token names, the way kit_prefix() already judges CLAUDE_CODE_SHELL_PREFIX, so a wrapper whose name merely contains the stem is left alone in both the re-registration and registration_problems paths. Fixes #1905 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A local-strict-review pass on the previous commit found that names_hook's shlex-based tokenization disagreed with runs_hook on a command runs_hook already treats as running the deployed hook: one wrapped in `bash -c "..."`, in `(...)`, followed by `; true`, or carrying an unbalanced quote that sent it down the whitespace-split fallback. shlex.split (and the plain split fallback) folds a directly-attached boundary character such as `)` or `;` into the token, or flattens a whole quoted `bash -c "..."` argument into one token, so `os.path.basename` never landed on the bare deployed name in those shapes. A re-run then left the kit's own hook registered twice, and `--report` called it absent. names_hook now matches with a regex built on the same boundary characters runs_hook already uses, as a trailing path segment rather than by tokenizing, so it agrees with runs_hook on every shape it accepts while still refusing a maintainer's own hook whose name merely contains the stem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…y Class A second local-strict-review pass found that names_hook's bare-word boundary class still disagreed with runs_hook wherever the deployed path itself carries one of the boundary characters, such as a Windows account name with a space and parentheses, or a CLAUDE_HOME containing `&`, `;`, or `|`. runs_hook accepts the whole quoted path as one literal token; names_hook's prefix could not cross those characters to reach the deployed name, so a correct, quoted registration under such a path was reported unregistered and then duplicated on every re-run. names_hook now reads a double- or single-quoted path through to its own closing quote first, where every character inside is literal, and falls back to the boundary-class match only for a bare, unquoted word. Also drops the now-stale top-of-file comment on GUARD_STEM/SWEEP_STEM: it described the substring search this change replaces, and the mechanism it justified no longer exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A third local-strict-review pass found that deleting the whole top-of-file comment on GUARD_STEM/SWEEP_STEM (over the prior commit's stale claim about a substring search) also deleted a still-live reason: GUARD_NAME/SWEEP_NAME each serve both a writer (DEPLOYED_HOOKS, hook_dst/sweep_dst) and a reader (names_hook), so a rename made on one side and not the other reports the registration absent forever. That comment is restored, scoped to what is still true, and notes that STEM is read by the test suite alone now. test_the_guard_is_deployed_and_searched_for_under_one_name asserted GUARD_STEM in the registered command and GUARD_NAME.startswith(GUARD_STEM), neither of which is what the installer actually checks any more. Replaced with a check against names_hook itself, the real live contract, renamed to test_the_guard_is_deployed_and_registered_under_one_name. The same pass also found two narrower shapes where names_hook still disagrees with runs_hook: an unquoted path whose own directory holds a boundary character, and a few shell compositions (an escaped quote nested inside an outer quote, a no-space redirect, a trailing backtick) that runs_hook only accepts because it already holds the exact literal path rather than needing to discover one. Closing those means widening runs_hook's own matching, a design change beyond this fix's scope, so they are documented in names_hook's docstring and filed as issue 1953 rather than chased here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1954 +/- ##
==========================================
Coverage ? 55.40%
==========================================
Files ? 16
Lines ? 7223
Branches ? 0
==========================================
Hits ? 4002
Misses ? 3221
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is narrowly scoped, aligns ownership detection with the existing boundary model, and is backed by targeted regression tests covering the previously failing scenarios.
Review effort: Lite
Findings: None
What changed in this PR
This PR hardens host-setup/agent-safety/claude/install.py hook ownership detection by switching from substring-based matching to filename-based matching, preventing the installer from mistakenly stripping or misreporting maintainer-owned hooks whose commands merely contain the kit’s stems.
Changes:
- Added
names_hook(command, name)to identify registrations by an exact hook filename (with boundary rules aligned toruns_hook), replacing the previous substring checks. - Updated registration validation (
registration_problems) and re-registration strip/append logic to usenames_hookfor both PreToolUse and SessionEnd hooks. - Extended the test suite with regression coverage ensuring wrappers that merely contain stems are not treated as kit-owned, and that
names_hookagrees withruns_hookacross representative accepted command shapes.
| File | Description |
|---|---|
| host-setup/agent-safety/claude/install.py | Replaces substring-based hook ownership detection with names_hook and applies it consistently in reporting and re-registration. |
| host-setup/agent-safety/claude/test_install.py | Updates existing tests to the new matching semantics and adds new regression tests for wrapper-name false positives and names_hook/runs_hook agreement. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… and Setup Tooling (#2000) ## Summary Promotes develop to main, carrying the 16 pull requests merged to develop since #1943. - **Wait-loop guard (requirement 7):** [#1999](#1999) credits a comparison bound only inside a `[`, `[[`, or `test` invocation, and [#1996](#1996) reconciles the requirement's README count and diagram with its hook. - **Registry:** [#1994](#1994) and [#1976](#1976) record Vantage-Config's line endings and description. - **Scripts and tooling:** - [#1988](#1988) and [#1972](#1972) harden `ruleset_id()`. - [#1974](#1974) and [#1949](#1949) fix `pr_review.py` `reply --match` and `wait`. - [#1969](#1969), [#1964](#1964), [#1954](#1954) and [#1951](#1951) fix host-setup tool shadowing, shims, hook ownership and dpkg ownership checks. - **Gates and audit:** - [#1980](#1980) triages a path collision. - [#1967](#1967) and [#1962](#1962) tighten sha-pin and version-literal checks. - [#1956](#1956) keeps a folded `if:` visible to the interface audit. Closes #1636 Closes #1639 Closes #1948 Closes #1971 Closes #1718 Closes #1934 Closes #1877 Closes #1880 Closes #1865 Closes #1889 Closes #1966 Closes #1906 Closes #1935 Closes #1901 Closes #1905 Closes #1866 Closes #1897 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
install.pydecided whether a PreToolUse or SessionEnd hook registration belonged to this kit by testing whether a bare substring (GUARD_STEM/SWEEP_STEM) appeared anywhere in the hook'scommandstring. A maintainer's own hook whose command merely contained that substring, such as a wrapper namedmy-gh-write-guard-audit.sh, was silently stripped and re-added on every install, and misreported by--reportas a broken registration of the kit's own hook.names_hook(command, name), matched with the same boundary charactersruns_hookalready uses (a quoted path is read to its own closing quote; a bare word is bounded by whitespace and shell metacharacters), so it agrees withruns_hookon every shape it accepts while correctly rejecting a maintainer's own hook whose name merely contains the stem.registration_problemsand both strip-then-append sites inmain's re-registration now usenames_hookinstead of the old substring test.names_hookand the existingruns_hookon unusual but real command shapes (bash -c "..."wrapping, parenthesized subshells, a trailing; true, an unbalanced quote, and a quoted path whose own directory contains shell metacharacters like spaces or&/;/|/(/)); both were fixed and are covered by a new regression test,test_names_hook_agrees_with_runs_hook_on_every_shape_it_accepts.runs_hookonly accepts because it already holds the exact literal path). Closing that fully means wideningruns_hookitself, a design change beyond this fix's scope, so it is documented innames_hook's docstring and filed separately as names_hook Does Not Match Every Shape runs_hook Accepts #1953.Closes on promotion: #1905
Test plan
python3 -m unittest test_install— 87 tests pass (86 existing + 1 new agreement test)ruff check/ruff format --checkcleanmypy host-setup/agent-safety/claude/install.pyclean (repo's configured type-check gate)🤖 Generated with Claude Code