refactor: twenty-one module deepenings across the hook library, guardrails and the check scripts - #4059
Merged
Merged
Conversation
…y for writer and reader The pair-cooccurrence reader hard-coded the repo-scope store path and read neither skill_usage_scope nor skill_usage_dir, so under the user or data-dir scope the hook wrote where the reader never looked. The reader now sources claude-ops-paths.sh from its own plugin root and calls the same claude_ops::resolve_skill_usage_dir the writer uses, with --scope, --dir, --data-root and --print-store; --store stays as the explicit override, and a missing store names the scope. The data-dir scope requires --data-root and never falls back to an inherited CLAUDE_PLUGIN_DATA. Deepening candidate 12 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…atus aggregate After the six-job collapse the lanes live as steps, and the lane-coverage gate still proved reachability for jobs only, so a gate step whose id was left out of the aggregator feed turned nothing red. The gate now asserts set equality at both levels from one awk pass: jobs against ci-status.needs, and per job the steps carrying continue-on-error with an id against the ids the feed reads, minus the opt-outs declared in scripts/lane-coverage-step-opt-outs.txt (the resolver's own detect and match steps). Both directions fail: an unfed gate, an unreadable gate with no id, and a dangling feed row. The opt-out list is read through scripts/lib/read-list.sh. Deepening candidate 15 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
Seven of the nine audit hooks were the same fifteen-to-thirty-line shape, differing only in the event they listened to and the fields they projected, so every change to the shape was seven edits and the row table lived only in a test. One script, audit-event-emitter.sh, is now registered on those seven events and dispatches on hook_event_name to a per-row projection. Each row keeps its own <name>_enabled switch and emits the same hook name, event, status and data fields as before, so telemetry readers see byte-identical envelopes. hook-failure-audit.sh and skill-usage-audit.sh keep their own files: the first is earned behavior, the second sits on the PostToolUse hot path where the kill-switch hoist gate requires a single-switch predicate. The session_id extraction is done once. The seven per-hook suites are replaced by one table-driven suite at the emitter's interface. Deepening candidate 10 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…r and the guard The PreToolUse guard restated the engine's command grammar positionally (flag positions, the valueless and optional flag sets, the tier set, the subcommand tuple), so every CLI change was a lockstep edit in two files and the guard could drift stricter or looser than the parser. plugins/disk-hygiene/lib/engine_grammar.py now declares, per subcommand, the required flags in order, the valueless flags, the optional flags with their validators, and the tier set; build_parser derives its subparsers from it and classify_exact_engine_command validates a literal invocation against it through match_invocation. The guard never imports the engine, and the module adds about half a millisecond on the always-on path. An agreement suite proves a flag added to the grammar is accepted by the engine and admitted by the guard in one test. Deepening candidate 20 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…y from evidence arrays What a finding kind is had no module: forty-seven emit_finding call sites spelled confidence and disposition as literals, two hand-written kind lists decided which kinds produce an action, the reference doc carried the same rows as a table, and a CI gate recovered the kind list by grepping the collector. A FINDING_REGISTRY now keys every kind to its confidence, action class and disposition; emit_finding takes the kind and looks the rest up, with emit_finding_as as the escape for the four kinds whose tier or disposition is computed per finding; the action-kind predicates are lookups; and --print-finding-registry emits the table as data for the coverage gate and the doc-parity assertion. analyze_repo is now collect-then-classify. Collection owns every git and gh call and fills parallel evidence arrays, including the merge-base ancestry that used to be probed inside the classification loop; classify_worktrees and classify_branches read only those arrays and make no probe, so the suite drives them with array literals. Six grandfathered kinds now assert collector output and leave the coverage baseline. The per-worktree remote probe is resolved once per repository, so spawn counts are equal or lower. Deepening candidates 18 and 19 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…ence suite The suite builds git fixtures, so an inherited absolute GIT_DIR would write the fixture identity into the caller's clone; the fixture-isolation gate requires the environment cleared at the top of every such suite. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…f through changed-files.sh Six check scripts carried a verbatim copy of the list parser that scripts/lib/read-list.sh already owns, three of them with a bare read loop that silently dropped a final entry with no trailing newline, and thirteen scripts re-derived the stale-entry guard with eight diagnostic prefixes and five consumed-tracking shapes. They now call read_list::into, and the library gains read_list::mark_used, read_list::stale_to and read_list::report_stale so an exemption that outlives what it excuses is reported once, the same way, from one place. Four gates that still answered "what changed against this base" with their own git diff, in the shape that lets a failed diff pass as an empty change set, now go through changed_files::into, with a regression case proving a diff failure after ref validation exits non-zero. Deepening candidates 13 and 14 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
The record schema was implicit across four writers that shared no formatter (two routes in the telemetry sink, the session event log's string-concatenated line, and the SkillUse record), and the observability reader reconciled them with a normalization prelude on every query. session-log-lib.sh now owns slog_record_to and slog_event_record_to, printf-based and jq-free so the event log's hot path can call them, and every writer builds its line through them under one documented key set. The reader's prelude and hand-maintained key table are gone, the hook-telemetry convention states the schema, and each writer's suite carries a round-trip assertion against it. The repository's own sink copy under .claude/hooks is re-synced with its repo-relative source block. Deepening candidate 11 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…parse steps The git-invocation resolver was deep in implementation and shallow at the interface: six consumers each called resolve-index, copied two globals, called resolve-subcommand and copied two more, and two guards grew byte-identical private helpers on top. hook::git_invocation now performs that sequence in one call and returns the parsed invocation (index, resolved words, wrapper chdir record, subcommand and its index, config assignments with their kinds, and the expanded alias chain with its termination reason). The library absorbs hook::git_subcommand_ignores_alias, hook::git_effective_dir_to, hook::git_alias_reparse_to and hook::git_alias_admit, the last two in the fork-free form. The four guardrails guards and the two source-control worktree gates keep their predicates and lose the prologue, and a direct table test covers the sudo, env -S, env -u, shell -c, -c and --config-env, and shell-alias cases that previously ran only through spawned guards. Every carrying plugin's copy is regenerated. Deepening candidate 6 of the architecture review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…in the shared parse Two deepenings of the shared hook library. The nine file-edit hooks each re-assembled the same fifteen-step prologue in the same order, and the copies had drifted: nine hand-rolled tool_name jq parses bypassing hook::jq_fields, a FILE_REL_DEGRADED branch in three hooks and not the others, and a data payload built nine ways with actionlint's missing the changed key. hook::begin now performs that sequence in one call and exits on the early paths itself, hook::data_json_to builds every payload, and the duplicate extension-gate pair collapses to one declarative glob list. The nine hooks lose 291 code lines between them and the library holds the sixteen ordering facts once. hook::bash_parse_segments modelled a segment as argv words and discarded the redirect operator and its target, which is the half of the grammar block-hook-bypass exists to inspect, so that hook carried a private tokenizer and called the shared parser zero times. Segments now carry their ordered redirections with quoting provenance alongside argv, every existing consumer sees byte-identical argv, and the bypass guard becomes a predicate over parsed segments, dropping 426 lines and its own sentinel alphabet. Deepening candidates 1 and 7 of the architecture review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
Twenty-seven suites re-created the world they test in: a temp root, a scripts directory, a copy of the script under test, a plugins tree, often a git repo with a hand-written identity, and an inline unset naming three of the seven git variables that actually leak. The divergences were real: shorter unset lists than the shared harness uses, identities missing commit.gpgsign and core.autocrlf, and a 679-line gate that exists to police by hand an invariant no shared code held. scripts/lib/fixture-tree.sh now owns fixture_tree::build, which creates the root outside the checkout, copies the script under test with scripts/lib beside it, registers cleanup once, writes the full throwaway identity, and clears all seven git variables at source time. Forty-five suites build through it and all but four now count through the shared harness. The isolation gate credits it the way it credits any clearing harness, resolved by basename, so the invariant is held by construction rather than by inspection. Deepening candidate 16 of the architecture review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…t and dispatched The test helper stopped at JSON builders, so eight suites each declared their own near-identical run(), and each side of the composition was tested against a stand-in of the other: guard suites spawned the guard directly and never asked run-guards.sh for a verdict, while the dispatcher's own suite drove stub guards only. The dispatcher's stdin and field-cache overrides, its multi-document merge and its no-jq arbitration therefore had no coverage against a real guard. guard_invoke is now the one command-to-verdict entry point, with expect and expect_both layered on it, so thirty-six cases run identically both ways. The dispatched mode adds verdict coverage where there was none: the primed field cache serving a guard's probe, the cache-miss path on a NUL payload, the PowerShell classifier loaded once per event, two shipped guards merging into exactly one document, and the no-jq arbitration running on documents the guards actually wrote. The rc-1 and rc-3 stdin arms stay direct-only because the dispatcher answers them once per event before any guard is sourced; each is commented with the dispatcher-side assertion that covers the other half. Deepening candidate 9 of the architecture review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…tract Every scripts/check-*.sh is invoked the same way by CI and by affected-tests.sh, but each one restated its own half of the calling convention in its header and four members disagreed with it. A missing jq or PyYAML exited 1, the same code a real finding uses, so a caller could not tell a tree that violates a rule from a host that cannot check it. check-queue-front-matter.sh wrote its findings to stdout while the rest wrote them to stderr. State the contract once in README.md under "The check-script contract" and have each member's header point at it: 0 clean, 1 findings, 2 environment or usage, findings on stderr, a prerequisite failure spelled `|| exit 2` rather than left to `set -e`. Four members are brought into line and the rest verified already conforming. check-script-contract.test.sh holds the contract. It carries a registry row per family member, so an unregistered script and a stale row both fail; it runs every prerequisite-declaring member with the prerequisite removed and asserts exit 2; and it runs clean and seeded-violation fixtures for the seven members with a fixture recipe, asserting the exit code and the stream the findings land on. Registering by basename is also what routes a member's future changes back to this suite through affected-tests. Three exits stay 1 by design and are now tested as such: a hook declaration the exec-form reader cannot parse, an empty hook corpus, and a kill-switch pin mismatch are statements about the tree, not the host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…ed string The portability gate carried its whole scanner as a 1,576-line awk program inside a single-quoted shell word, so check-shell-portability.sh was 2,082 lines of which three quarters were another language. Nothing could parse the program: no editor mode, no syntax check, no way to run the scanner on one file without going through the gate's mode dispatch and reporting. Move the program to scripts/lib/shell-portability-scan.awk and give it a stated interface: two data operands in, one `LINE: token -> text` per unexcused hit on stdout, exit 2 with a stderr diagnostic on an empty pattern set or an unknown class. The gate keeps what is actually its own, mode dispatch, scannability, the skill-md baseline and the PORTABILITY reporting, and drops to 520 lines with a four-line scan_file. The program path is resolved and existence-checked once at startup, so a missing scanner fails closed there instead of as an opaque per-file awk status. The extraction is whitespace-only: every program line was four-space indented and none blank, so the dedent is reversible, and every executable statement is byte-identical to the embedded original. Comments that described the embedding now state the reason that still holds, keeping a quote or backslash out of the string literals that build the regexes. Both gate versions were run over the same 1,889-file tree at the same moment: identical stdout, identical 52-finding set. Three new assertions cover what was previously untestable: that the program compiles at all, that it can be driven directly on two operands, and that a missing program file is a fail-closed rc 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…nt walk with hook::walk_up_to Every file-edit hook ended each of its arms by hand: take the rewrite guard's disclosure, emit telemetry with the right status, emit exactly one channels document, exit 0. Eight hooks spelled that sequence between one and six times each, and the order was a convention nothing enforced. A hook that took the disclosure against the wrong file, or emitted a second document, or skipped telemetry on one arm, was wrong in a way only a reader comparing arms could see. bash-format took its disclosure against $FILE while the rewrite had targeted $_fmt_target. hook::finish is the bookend to hook::begin and owns that whole sequence. Behind the interface: settle data.changed, release the guard's snapshot, emit telemetry with the arm's verdict, emit one document, exit. It reads the plugin, event, start time, tool and file that begin already recorded, so an arm states its status and its findings and nothing else. Thirty-two hand-written exit arms across eight hooks become thirty-two hook::finish calls, and rewrite_take_disclosure now takes against the file begin was given, which removes the bash-format mismatch by construction. hook::walk_up_to replaces eight hand-rolled parent walks that searched for a config file or a tool binary. The walk takes a ceiling and a predicate answering accept, continue, or stop, in first-hit or topmost-hit mode. The ceiling is now required and an unresolved one fails closed, where four of the walks previously walked to the filesystem root when their ceiling variable was empty. That is the safe direction for gates that decide whether a repository's files get rewritten: a ceiling-less walk adopts configuration from directories the repository does not own. In practice the variable is empty only when REPO_ROOT is not a directory, which begin makes hard to reach, and no suite exercised it. ctx_flush is now ctx_take_to plus emit_channels, and emit_additional_context delegates to emit_channels, retiring its jq -n twin and the silent no-jq return behind it. Five always-on hooks lose a jq fork per emission; hook::finish adds only builtins. Arms that never took the disclosure now emit data.changed false rather than omitting the key, which is the guard's documented semantics. No suite asserted the key's absence. eol-normalizer keeps its own epilogue: its status is derived from the take, so folding it in would need a status-derivation flag no other caller wants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…aration run-guards.sh owns the per-event work, but each guard independently restated its own half of the contract: the jq fields it reads, whether it needs the PowerShell classifier, and how it spells its plugin root. Nothing checked the two halves agreed. PRIME_FILTERS was a hand-maintained union of every guard's jq filters, and the dispatcher read tool_name out of the primed values BY POSITION. Inserting a filter ahead of the tool-name filter silently made the dispatcher read a neighbouring value, and the guards' own re-sourcing of the classifier is the only reason correctness survived it. Four guards re-sourced a ~104 KB PowerShell classifier the dispatcher had already loaded, spelling the plugin root four different ways between them. Each guard now declares what it consumes in one place the dispatcher reads. Primed fields are bound to names, so the dispatcher asks for tool_name by name and a reordering cannot misread it. The guards share one spelling of the plugin root and one way to reach the classifier, so a guard states what it needs and the dispatcher satisfies it. A guard that adds a filter without registering it now fails loudly instead of reading its neighbour. Guards still work sourced directly as well as through the dispatcher; both are live paths and both arms stay covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…he format hooks Two deepenings that meet in the shared library and in the generated plugin copies of it, so they land together rather than leaving a commit whose vendored copies do not match their source. One calling convention. Seventeen library names were spelling variants of eleven behaviors, and choosing between them was a per-call-site decision the interface never stated. Each past fork-removal added a caller-writes-to-a- variable twin without retiring the value-printing form, so the interface grew every time. A print form whose whole body calls its twin is a pass-through: deleting it moves no complexity to the caller, it only stops charging one a subshell fork for a value the shell already has. Seven such forms are retired and every call site moved to the surviving spelling. Comments and test assertions that named a retired form are swept with it; two plugin README passages that record what a past version dropped keep the old names, because they describe the state at that version and renaming them would falsify the record. One engine behind the format hooks. Six hooks implemented the same five-outcome state machine over one file, with the same per-line accumulation, the same findings encode, and the same ten-line copied comment above it, differing only in the exit-code map and the message text. The engine now owns running the tool, accumulating its output, classifying the outcome, and encoding findings; an adapter states only what differs. The missing-binary notice, identical in four hooks, comes from one place. Typos, markdown and eol-normalizer stay out of the engine on purpose. They derive their disclosure from the tool's own report, and eol-normalizer has no binary to run, so folding them in would add a flag per exception rather than depth. This also fixes measured waste. The guard that skips the findings encode when no telemetry sink is listening was present in three hooks and absent in four, where two jq processes were spawned per diagnostic-producing edit to build a document nothing read. The encode now lives behind the engine's own guard, so no adapter can omit it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
Two conflicts, both where main's worktree-root rename met this branch's restructuring of the same files. worktree-add-containment-gate.sh: main replaced the inline last-wins read of the old config key with a call to the shared resolver. Main's side is taken whole; this branch had not changed that function. audit-fleet.sh: main renamed the worktree-root reader and its config key while this branch replaced hand-written finding tuples with a registry. Neither change touches the other's logic, so both are kept. The reader is main's; the finding calls stay in the registry-driven shape this branch introduced, which reads severity and disposition from the registry rather than repeating them at each call site; the config key is renamed throughout to match main. Three helper definitions that the merge re-added alongside their moved copies are dropped, keeping one definition of each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…changes The plugin version is the update cache key, so every plugin carrying a changed shared library or a changed file of its own bumps, or consumers never receive the change. Each changelog entry states what the plugin's reader will notice: the retired helper spellings, the single exit arm, the ceiling requirement on the parent walk, the shared formatter engine, the dispatcher declaration, and the two measured process savings. Three entries record a deliberate exclusion rather than a change, so the next reader does not try to finish the job: eol-normalizer keeps its own exit arm because its status is derived from the rewrite, and it and markdown-format and typos-format stay outside the formatter engine because they take their disclosure from the tool's own report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
The hygiene lane's exec-bit check failed. Every `.test.sh` in this repository carries the executable bit, and the two suites this branch adds were committed without it: they were the only two non-executable test suites in the tree. The other files this branch adds keep mode 644 and are correct there: a sourced library, an awk program and a Python module are never executed directly, which is how every one of their siblings is committed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
…mprovements-hqfc0i
kyle-sexton
marked this pull request as ready for review
September 11, 2026 05:24
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…mprovements-hqfc0i Three changelog conflicts, each one both sides adding entries under a version number they had picked independently from the same base. Main's entry keeps the number it released under; this branch's entries move up one patch: claude-config 0.41.1 -> 0.41.2, claude-ops 0.47.1 -> 0.47.2, guardrails 0.33.1 -> 0.33.2, with each manifest bumped to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a69b2ac3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… also produces Two assertions this branch adds read Windows-specific spellings back and compared them against POSIX ones, so the Windows lane failed on them alone. The repo-root case expected the fixture's own path, but `rev-parse --show-toplevel` answers in the drive-letter spelling on Git Bash while the mktemp fixture path is the MSYS one. It now expects what git reports for the same directory, which keeps the claim discriminating: a CWD-anchored resolution would still name a different repository. The two-line user-channel case read `jq -r` output containing an interior newline. The Windows jq writes stdout in text mode, so that newline arrives as CRLF; the single-line cases beside it lose theirs to the command substitution and never showed it. Stripped with the same substitution the suite already uses for jq output elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
kyle-sexton
marked this pull request as draft
September 11, 2026 06:01
kyle-sexton
marked this pull request as ready for review
September 11, 2026 06:01
…mprovements-hqfc0i Two changelogs and their manifests conflicted again: main minor-bumped claude-config to 0.42.0 and claude-ops to 0.48.0, past the patch numbers this branch had taken in the previous merge. Main's entries keep the numbers they released under and this branch's entries move to 0.42.1 and 0.48.1, above them in the newest-first order, with each manifest bumped to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
kyle-sexton
marked this pull request as draft
September 11, 2026 06:22
kyle-sexton
marked this pull request as ready for review
September 11, 2026 06:22
Contributor
|
Claude finished @kyle-sexton's task in 3m 17s —— View job Claude is reviewing this PR
|
Contributor
|
Claude finished @kyle-sexton's task in 8m 3s —— View job Security review in progress
|
Contributor
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Contributor
|
Last security-reviewed head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c94be90026
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ors, and free --print-store of jq Three findings from the review lanes, each verified against the code before being taken. `hook::begin`'s jq-free pre-filter matched globs against the payload's JSON string literal, still escaped. A Windows path arrives there as `C:\\repo\\.github\\workflows\\ci.yml`, and normalizing that turns each of the two backslashes into a slash, so `*/.github/workflows/*.yml` could not match and the hook exited before ever running the linter. The hook it replaced compensated with a separator-loose glob; the shared pre-filter inherited the doubling without the compensation. The escape is now collapsed before matching, so one glob list serves both the pre-filter and the authoritative re-check on the parsed path, and every caller writes the separator its path actually has. This only widens what the pre-filter admits, and the re-check is what decides. actionlint's list goes back to the exact spelling; a case covers the escaped form, and it fails without the collapse. `skill-pair-cooccurrence.sh --print-store` refused to run without `jq`. That arm resolves and prints a path and reads nothing, and it is the bridge to the Python auditor, whose host this skill promises needs only Python. The prerequisite moved below the arm, so the report still requires jq and the bridge no longer does. The skill body's documented resolver call omitted `--data-root`, which the `data-dir` scope requires and which is deliberately not read from the environment, so a reader on that scope got exit 2 from the one command the skill gives them. It is passed from the skill's own expansion now, as the sibling lanes skill already does; the other two scopes ignore it, so one command serves all three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
Contributor
The durable candidate artifact, preserved hereThis PR's description points at "the durable candidate artifact" that lists all twenty-one It is the Phase 1 scan output after Phase 1.5 verification: per candidate, the files, the The abstract: "Deepening candidates for the shared hook substrate, guardrails, claude-ops hooks, the CI check family and three large scripts; 14 Strong, all reproduced"Deepening candidates (Ousterhout lens)
Collapse the file-edit hook prologue into hook::begin
Own the exit-arm protocol with one hook::finish
One hook::walk_up for eight hand-rolled upward walks
Absorb run-and-classify into a format_one_file adapter seam
Collapse the _to and print dual spelling into one calling convention
Return a parsed git invocation, not five parse steps
Carry redirects in the shared parse and retire block-hook-bypass's second tokenizer
Make the dispatcher and guard contract one declaration instead of a convention
One command-to-verdict test driver, run through the dispatcher
Collapse eight thin audit hooks into one event-dispatching emitter
One writer for the JSONL record shape
One store-path resolver for the skill-usage writer and reader
Finish the read-list extraction and give the baseline family one stale-entry guard
Route the remaining base-ref and diff walkers through changed-files.sh
Make a gate's CI registration one fact instead of three hand-edited sites
One fixture-tree builder for the check-script suites
Give the check family one exit-code and stream contract
Make the finding kind a registry in audit-fleet, not 47 hand-written argument tuples
Split analyze_repo into evidence collection and a probe-free classifier
Give hygiene.py's invocation grammar one declared origin the guard consumes
Move the shell-portability scanner out of the quoted string into an awk file
top-recommendation: Return a parsed git invocation, not five parse steps Generated by Claude Code |
kyle-sexton
added a commit
that referenced
this pull request
Sep 11, 2026
… its check row (#4106) No related issue: this is the agent-side compensation for the CI defect filed as #4093, which it deliberately does not close. The lanes stay broken until that issue's acceptance criteria are met; this only stops the PR monitor from reading their green rows as evidence. ## Summary An AI-review lane reports on its **session**, not on its **output**. A session that ends without error concludes success even when it posted no review body and produced no finding, so the check row goes green and nothing downstream can tell "reviewed and found nothing" apart from "never reviewed". The PR monitor's per-iteration checklist classified such a lane PASS on its check row alone, and the readiness gate cleared on that. This reproduced on this pull request while it was open: both `claude[bot]` lanes concluded success and posted **zero reviews**, on every head. Every external finding here came from Codex. ## Fix **New step B2 in the per-iteration monitoring checklist** (`skills/pull-request/SKILL.md`), and the same invariant as a **Gate 5 item in `reference/readiness.md`**, which is the single source of truth both `monitor.md` (Phase 3.4) and `merge.md` (Phase 4.1) rerun, so a direct `merge` and the final re-verification in `full` enforce it too. A lane that concluded success having produced nothing **for the current round** reviewed nothing, whether it never started, could not load the skill it was told to invoke, or stopped early inside its own session. It is classified ABSENT rather than PASS, a local review stands in its place over the same diff, and both are named in the verdict. A lane whose posted body admits a manual fallback counts the same. Four details are load-bearing: - **The roster comes from `gh pr checks`, never from the authors who posted.** A lane that posted nothing is missing from every author-derived roster by construction, so an author-derived roster would apply the rule to an empty set, which is exactly the lane it exists for. - **Productivity is scoped to the head under review**, per surface, with the field each surface actually has: `original_commit_id` on inline review comments, `commit_id` on the reviews endpoint, and `created_at` against push time on issue-level comments, which carry no commit field and are where AI-review summaries land. - **A rerun of the current head is bounded by timestamp, not by commit field**, because its artifacts carry the same SHA and no commit field can separate them. - **Both readiness templates gained a `Review lanes:` line**, so an absent lane and its substitute are reported rather than dropped from a verdict that otherwise reads all-clear. The local fallback is `/review:fanout` for breadth, or the bundled `/code-review` against an explicit target for a correctness lane, each conditioned on resolving in the session. Not `/review:code-review`, whose own Routing section says it runs only where the workflow invokes it and depends on that wrapper for the repository, PR number, head SHA, and posting mechanics. Because the rule is now stated on two surfaces, it is registered as a contract clause (`B2-lane-productivity`) with `readiness.md` canonical and the monitor checklist tagged as a restatement, putting its four qualifiers under `check-contract-clause-coverage.py`. **Step E** reads `ALL checks terminal + every review lane productive or substituted per B2 + ALL comments addressed + 2-min cooldown`. **One gotcha** carries the reason, so the rule is not a bare instruction. ## Verification Branched fresh from `main`, then `main` moved and is merged in at `8d03719e`, conflict-free. Every gate re-run against that base on the final head: ``` check-contract-clause-coverage 6 canonical surfaces, 17 tagged restatements check-purged-em-dashes 383 declared paths, 1194 files, none found check-changelog-parity --check every versioned plugin has an entry check-changelog-parity --check-bump every changed plugin has its version entry check-changelog-parity --check-order 93 changelogs newest-first, no duplicates check-changelog-parity --check-preserved 250 existing version headings preserved markdownlint-cli2 0 issues in 4 files affected-tests --run fetch-annotations 11/11, contract-clause coverage suite, 24 pytest cases, all pass ``` Two review rounds, both of which found real defects. **Codex, on the first head:** three findings, all verified against the files before being taken, all real. The productivity check was existential over the whole PR rather than scoped to the round; the invariant was missing from the authoritative merge gate; and the local fallback named the CI lane skill. **A local review, on the current head:** run because both AI-review lanes produced nothing and this PR's own new rule classes that ABSENT. Six findings, five confirmed, and they are the four load-bearing details above plus the `Review lanes:` line. The sixth claimed `babysit-prs` merges through `readiness.md`; checked, and it does not, which made the changelog's "every merge path" the actual overclaim, now corrected to name the readiness gate and say the babysit tiers are unchanged. The rule earned its place on its own pull request: it caught that this PR's review lanes were absent, and the substitution it prescribes found a hole in the PR's own headline guarantee. This change is prose in a skill body and its references plus one registry entry; it alters what an agent following the checklist does, and no shipped code path. ## Related - Refs #4093, the CI defect this compensates for and does not close. Both lanes concluded success there with 23 and 8 permission denials, 20 and 34 turns of 75, and no review posted; that issue also reports that the `review-skill-evidence` guard passes a lane which posts nothing at all. This PR is a third reproduction. - Refs #3147, closed with all three acceptance criteria unchecked; same root cause, and the guard it produced is the one #4093 reports a gap in. - #4059, where the failure was first observed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
No related issue: an
/architecture:improvedeepening pass over the wholerepository. The candidates were produced by the scan itself rather than filed
in advance; the durable candidate artifact lists all twenty-one with their
verified shallow signals.
Summary
Twenty-one module-level deepenings, each one a place where the same decision
was spelled in several places and nothing checked the spellings agreed. Every
candidate's shallow signal was reproduced before any code was written, and the
two that turned out to be misread are recorded as corrections rather than
implemented.
The work is behavior-preserving at every interface except where a change is
called out below. Twenty-one commits carry the work, one per candidate or per
pair of candidates that meet in the same file, plus one fix each for the
executable bit, the Windows spellings, and the three review findings. Four
further commits merge the base branch, which moved four times while this was
open.
Three candidates carry a measured runtime saving rather than only a structural
one. The rest earn their place by concentrating a decision that was previously
duplicated.
Fix
Ordered by leverage. Each area below spans one or two commits.
Shared hook library.
hook::beginowns the file-edit prologue that eighthooks had each hand-written: read the payload once from the inherited
descriptor, parse the tool name and file path, capture the start time.
hook::finishowns the exit arm they had each hand-written thirty-two timesover: take the rewrite guard's disclosure, emit telemetry with the arm's
verdict, emit exactly one channels document, exit.
hook::walk_up_toreplaceseight hand-rolled parent walks.
hook::git_invocationreturns one parsed gitcommand where five separate walks over the same words used to run, and the
shared parse now carries redirect operators, targets and quoting, which let
block-hook-bypassdrop its own 400-line tokenizer and become a predicate overthe parse it already had.
One calling convention. Seventeen library names were spelling variants of
eleven behaviors. Seven value-printing forms whose whole body called their
caller-writes-to-a-variable twin are retired, along with the subshell fork each
call site paid for a value the shell already had.
One engine behind the format hooks. Six hooks implemented the same
five-outcome state machine with the same accumulation loop, the same findings
encode and the same ten-line copied comment, differing only in the exit-code
map and message text. An adapter now states only what differs. Typos, markdown
and eol-normalizer stay out by design and each changelog says so.
Guardrails. The dispatcher and each guard now share one declaration of the
contract between them. The dispatcher had been reading the tool name out of the
primed values by position, so inserting a filter ahead of it would have made it
read a neighbouring value. Guards also share one plugin-root spelling and one
route to the PowerShell classifier, which drops a redundant re-source of a
104 KB file. A new test driver runs thirty-six cases both directly and through
the dispatcher, so a guard that behaves differently under dispatch now fails.
claude-ops. Seven near-identical audit hooks collapse into one
event-dispatching emitter, with every envelope verified byte-identical per row.
Every JSONL record is built by one formatter. The skill-usage store path is
resolved by one policy for the writer and the reader, which had resolved it
separately.
Repository scripts. The check-script family gets one exit-code and stream
contract, stated once and held by a new suite with a registry row per member,
so an unregistered script and a stale row both fail. Forty-five suites build
their fixtures through one builder. Every list reads through one reader and
every diff through one walker. A 1,575-line awk program moves out of a quoted
shell string into its own file. A gate's CI registration becomes one fact
instead of three hand-edited sites.
Other plugins. The fleet audit's finding kinds become a registry instead of
forty hand-written argument tuples, and classification no longer probes.
Disk-hygiene's invocation grammar is declared once and consumed by both the
parser and the destructive guard, which had hand-written a second copy to
validate against.
Behavior changes, stated deliberately
ceiling variable was empty. The shared walk requires a ceiling and fails
closed. That is the safe direction for gates deciding whether a repository's
files get rewritten, since a ceiling-less walk adopts configuration from
directories the repository does not own. The ceiling is unresolvable only
when the repository root is not a directory, and no suite exercised it.
data.changedasfalse rather than omitting the key, which is the rewrite guard's documented
meaning. No suite asserted the key's absence.
current consumer reads only success or failure, so no lane behavior changes.
Measured savings
jqprocesses removed per diagnostic-producing edit on four always-on hooksjqfork removed per emission on five always-on hooksVerification
The base branch moved four times while this was open and is merged in each
time, most recently at
5cba274b. The first two merges were conflict-free. Thethird and fourth conflicted in the same changelogs, each one both sides adding
entries under a version number picked independently from the same base;
main'sentries keep the numbers they released under and this branch's move above them
(claude-config 0.42.1, claude-ops 0.48.1, guardrails 0.33.2), with each manifest
bumped to match. Every gate below was re-run against that current base, not
against the base this branch started from.
Repository-wide gates, all clean:
Also checked before this was put up for review, and clean: no conflict markers
anywhere in the diff, no scratch or memory-tier paths committed, no live call to
any of the seven retired helpers remaining in the tree, and all five new shared
functions present and synced to their vendored copies.
Twenty plugins bump their version, each with a changelog entry stating what its
reader will notice. Three entries record a deliberate exclusion rather than a
change, so the next reader does not try to fold the three exception hooks into
the shared engine.
The review lanes found three things, and all three were real
The jq-free pre-filter could not match a Windows path.
hook::beginmatchedits globs against the payload's JSON string literal, still escaped, so a Windows
path arrived as
C:\\repo\\.github\\workflows\\ci.ymland normalizing it turnedeach of the two backslashes into a slash. A glob naming an interior directory
then could not match, and
actionlint-checkexited before ever running thelinter. The hook it replaced compensated with a separator-loose glob; the shared
pre-filter inherited the doubling without the compensation. Reproduced, then
fixed in the library rather than in one hook's globs, since the trap was there
for any caller: the escape is collapsed before matching, which only widens what
the pre-filter admits, and the authoritative re-check on the parsed path is what
decides. One glob list now serves both gates. A new case drives an escaped
Windows payload through
hook::begin, and it fails with the collapse removed.--print-storerequiredjqfor work that reads nothing. That arm resolvesand prints a path, and it is the bridge to the Python auditor, whose host this
skill promises needs only Python. The prerequisite moved below the arm. Verified
on a PATH built from symlinks to the tools the script actually runs with jq
absent: the arm exits 0 with the path, the report arm still exits 2. Both are
regression cases, with an assertion that the fixture PATH really has no jq so the
pair cannot pass vacuously.
The documented resolver call omitted
--data-root. The prose named therequirement but the copyable block did not carry it, so a reader on the
data-dirscope got exit 2 from the one command the skill gives them. It ispassed from the skill body's own expansion now, the shape the sibling
lanesskill already uses; the other two scopes ignore it, so one command serves all
three.
Two earlier heads failed a lane, and both failures were this branch's
The hygiene lane's
exec-bitrow. Every.test.shin this repository carriesthe executable bit and the two suites this branch adds were committed without
it; they were the only two non-executable test suites in the tree. Fixed in its
own commit. The other files this branch adds keep mode 644 and are correct
there, since a sourced library, an awk program and a Python module are never
executed directly.
The
test-windowslane, on two assertions this branch adds and nothing else;all four Linux shards and the
hook-utilslane were green on the same tree.Both read a Windows-specific spelling back and compared it against a POSIX one.
The repo-root case expected the fixture's own path, but
rev-parse --show-toplevelanswers in the drive-letter spelling on Git Bash while themktemp fixture path is the MSYS one; it now expects what git reports for the
same directory, which keeps the claim discriminating since a CWD-anchored
resolution would still name a different repository. The two-line user-channel
case read
jq -routput containing an interior newline, and the Windows jqwrites stdout in text mode, so that newline arrives as CRLF; the single-line
cases beside it lose theirs to the command substitution and never showed it.
Stripped with the same substitution the suite already uses for jq output
elsewhere. Both are test-side fixes; no library behavior changed. The lane is
green on the current head.
One suite fails locally, and it fails the same way on
mainplugins/claude-ops/skills/plugins/scripts/cache-content-check.test.shfailstwo process-budget assertions in the container this work was done in:
Its trace probe cannot count process creations on that host. Reproduced from a
clean worktree of
origin/mainwith none of this branch's changes present, andit fails identically there. A host limitation, not a regression, and it is the
only suite-level failure in the local sweep. CI does not reproduce it.
Related
/architecture:improveitself. Its four reserved scope questions have sincebeen researched and resolved in a comment there. This PR does not close it;
the two are separate work.
.claude/rules/hook-budget.mdgoverns the always-on hook latency budget everyhook change here is measured against.
docs/adr/0019 (shared code is vendored byte-identical with a sync gate) and0028 (hook plugin classification) constrain how the shared library and the
hook plugins could be restructured.
docs/conventions/hook-observability/README.mdnames the composition pointfor hook output;
hook::finishis now the fleet's spelling for a file-edithook's exit arm and would be worth naming there in a follow-up.
🤖 Generated with Claude Code
https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP