Grade [features].hooks when a Codex registration exists, not when config.toml does by topij · Pull Request #708 · topij/agentic-dev-kit

topij · 2026-09-07T19:13:30Z

Fixes the blind spot recorded on #698, scoped by the approved live observation.

What was wrong

kit_doctor graded [features].hooks only when .codex/config.toml existed, because codex_documents holds only surfaces that exist as files. The adopter who never wrote that file — the population likeliest to have left the switch unset — got no line about it at all, so a fully verified .codex/hooks.json rendered as an unqualified green report.

Of the three states an operator needs told apart, only explicitly disabled was caught. Enabled and never configured were identical in the output.

This repository is itself that population: it ships .codex/hooks.json and no .codex/config.toml.

What changed

[features].hooks is graded whenever a kit registration exists, rather than whenever .codex/config.toml does. The occurrence_names half of the old condition is kept deliberately — with no kit registration anywhere there is no hook for the switch to gate, and advising an adopter to set it would be advice about nothing.

A new unset registration state reports at · and does not reach the exit code. That is the calibration decision, and the observation above is its basis: a Codex client discovered these registrations with the switch unset, so failing the run would assert an outcome that probe did not establish. An explicit false remains misconfigured and still exits 1.

Deliberately left alone: a .codex/config.toml that exists but does not parse. It already carries an unreadable line, and adding unset beside it would state the switch's value from a document the run never read.

Live behaviour

uv run scripts/kit_doctor.py in /Users/topi/Coding/agentic-dev-kit at 7d0e122 on 2026-09-07 printed:

  · .codex/config.toml [codex]: [features].hooks is not set — ./init.sh's registration block asks for it
    (registrations are hand-written — ./init.sh prints both blocks; `/hooks` in a session is the authority on what loaded)

and exited 0. The same run's --json carries {"runtime": "codex", "surface": ".codex/config.toml", "state": "unset", "detail": "[features].hooks"}.

Scope

This does not establish hook execution after trust, the explicitly-disabled case in a live client, or a default shared by every Codex client. Those remain for a batched Codex session, where /hooks is the authority; static doctor output is not.

Verification

Both new branches were proven by mutation rather than by a passing run:

Neutralising the absent-config.toml branch failed test_an_absent_codex_config_still_reports_the_unset_feature_switch and test_the_unset_switch_does_not_reach_the_exit_code.
Neutralising feature_unset = not feature_keys failed test_a_features_table_carrying_neither_spelling_reports_unset.

test_an_ungranted_line_carries_its_own_hand_written_footer gains the switch in its fixture, for the reason its own comment already gives about absent: unset is a third state that pulls in the shared footer, so without it that test's negative assertion would be vacuous.

The full make test stamp follows in a comment once the run completes.

Refs #698 — the issue stays open until the batched Codex session closes what this cannot.

https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

…fig.toml does `kit_doctor` graded the switch only when `.codex/config.toml` was present, because `codex_documents` holds only surfaces that exist as files. The adopter who never wrote the file — the population likeliest to have left the switch unset — got no line about it at all, so a fully verified `.codex/hooks.json` rendered as an unqualified green report. Of the three states an operator needs told apart, only explicitly disabled was caught; enabled and never configured were identical in the output. The condition keeps `occurrence_names`, which is the half of it that was right: with no kit registration anywhere there is no hook for the switch to gate, and advising an adopter to set it would be advice about nothing. The new `unset` state reports at `·` and does not reach the exit code. The live observation on #698 recorded a Codex client discovering these registrations with the switch unset, so failing the run would assert an outcome that probe did not establish; an explicit `false` stays `misconfigured` and still exits 1. It shares the hand-written-registration footer rather than taking its own, because both of that footer's claims are true of it. A `.codex/config.toml` that exists but does not parse is left alone — it already carries an `unreadable` line, and adding `unset` beside it would state the switch's value from a document the run never read. `test_an_ungranted_line_carries_its_own_hand_written_footer` gains the switch in its fixture for the reason its own comment already gives about `absent`: `unset` is a third state that pulls in the shared footer, and without it that test's negative assertion would be vacuous. Refs #698 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

coderabbitai · 2026-09-07T19:13:37Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 6417428e-d919-4aa0-aafc-f32ff522ab6a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

`kit-manifest.json` does not track `CHANGELOG.md`, so this commit needs no manifest regeneration. Refs #698 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

…is skipped Panel round 1, both lenses at 2010d3a. **The medium finding was that `render()` was pinned by nothing.** Every test added with the fix exercised `inspect_registrations`, so the correctness lens replaced the `·` mark and the message with different wording, and separately dropped `unset` from the footer's trigger tuple, and both mutants survived the whole file. The CHANGELOG promises the adopter a new advisory line; nothing held that promise to the text actually printed. The three tests added here are built from a hand-made `Report` rather than a repo fixture, deliberately: a fixture also emits `unregistered` and `absent` lines, which trigger the same footer and satisfy the same substring — so a fixture-based test would have passed with the tuple mutated back. That is the vacuity the ungranted-footer test already documents, one state over. Both surviving mutants are now killed and the file restored byte-identical after each. **The comment above the skip claimed more than the branch knows.** It said a `.codex/config.toml` absent from `codex_documents` "did not parse"; the correctness lens built one that parses and then exhausts `_hook_commands` on a degenerately nested `hooks` value, reaching the same `continue` with a `RecursionError` detail. What the routes share is that the read did not complete, not the stage it stopped at. Two coverage gaps the adversarial lens found, both behaviours already correct: a `features` value that is not a table, and a registration carried solely by `.codex/config.toml` rather than `.codex/hooks.json`. Refs #698 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij · 2026-09-07T19:53:16Z

Fallback review panel — disposition

CodeRabbit auto-reviews are disabled on this repository, so the panel ran rather than the bot. Receipt recorded at 2010d3af62d985b443f53fc8bd92a70f67cc2e06 before any fix landed.

Round 1 — `2010d3a`: both lenses, one medium finding

Both lenses independently confirmed the reviewed sha against git ls-remote origin main, reported non-empty diffstats matching assembly time, and excluded the driftcheck marker with a confirmed deselected count before believing any kill.

correctness (medium, the one that mattered): render() was pinned by nothing. Every test shipped with the fix exercised inspect_registrations, so replacing the · mark and the message with different wording, and separately dropping unset from the footer's trigger tuple, left both mutants alive through the whole file. The CHANGELOG promises the adopter a new advisory line; nothing held that promise to the text actually printed.
correctness (low, imprecision): the comment above the skip said a .codex/config.toml absent from codex_documents "did not parse". The lens built one that parses and then exhausts _hook_commands on a degenerately nested hooks value, reaching the same continue with a RecursionError detail. Behaviour correct, causal claim overstated.
adversarial (low, coverage): a features value that is not a table, and a registration carried solely by .codex/config.toml rather than .codex/hooks.json, were both correct and both unexercised.
adversarial: no functional defects, fail-open paths or bypasses in the guard itself. Its three mutations — the empty-table branch, the absent-file branch, and widening the exit-code set — were each killed by behavioural assertions.

Fixed in 180b549. The three new render tests are built from a hand-made Report rather than a repo fixture, deliberately: a fixture also emits unregistered and absent, which trigger the same footer and satisfy the same substring, so a fixture-based test would have passed with the tuple mutated back. Both previously-surviving mutants are now killed, with the file restored byte-identical after each.

Not attributable to this diff

Both lenses independently hit a full-suite failure in a file this PR does not touch, and neither could make it deterministic:

make test at 4b56d3eec285781ac382e0b897d5d1da9c7fe40e (base) → 1 failed, 2445 passed, 1 skipped in 379.54s, failing test_pr_followup_hook.py::test_a_payload_too_deep_for_json_load_still_exits_zero.
The adversarial lens, running base-content files under concurrent load, got a different single failure — test_reconcile_sessions.py::test_portable_bounded_runner_reaps_on_startup_interrupt.
The correctness lens re-ran at head and got 2456 passed, with the failure not reproducing.

It passes in isolation every time. Local uv run resolves Python 3.14.7 while .github/workflows/test.yml pins 3.12, and #393 records that json's RecursionError behaviour changes at 3.14 — but the non-reproducibility above means the interpreter split is a candidate contributor, not an established mechanism. Recorded here rather than filed; #393 stays open.

Process finding against the panel itself

The adversarial lens built its mutation scratch with cp -a of the linked worktree it was handed. That copies .git as a pointer file aliasing the same per-worktree admin directory, so its git commands wrote into the shared index — file contents were never altered. It detected and repaired this with an index-only git reset; the cockpit independently verified afterwards that the tree is clean, the index empty, HEAD still 2010d3a, and both changed files hash-match the manifest. Worth a note in the panel's No writes in the tree you were given item: cp -a of a linked worktree is not isolation.

https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

Panel round 2, both lenses at 180b549. Correctness found nothing. Adversarial found that round 1's comment was true and pinned by nothing. The comment claims a `.codex/config.toml` that PARSES and then exhausts `_hook_commands` reaches the same silent skip as one that never parsed, and so gets no `unset` line beside its `unreadable` one. Both lenses confirmed the behaviour by execution. The gap was that every test reaching that skip did so through a TOML syntax error, so gating `codex_documents[surface] = document` into the `_RegistrationTooDeep` handler reintroduced the double report — the report stating the switch's value from a document the run declined to read — and survived the whole file. That mutation now fails the new test. No production change this round: the claim was already correct, and what was missing was the thing holding it. Refs #698 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij · 2026-09-07T20:15:06Z

Panel round 2 — `180b549`, both lenses

correctness: no findings. It verified the round-1 comment's claim by execution rather than by reading — building a .codex/config.toml that parses and then exceeds the walk's depth cap, and confirming the file gets unreadable and no unset beside it. It also mutation-killed each of the five round-1 tests against the behaviour each names.

adversarial: one Medium, not a regression — a coverage gap. The same claim correctness verified was pinned by nothing. Gating codex_documents[surface] = document into the _RegistrationTooDeep/RecursionError handler reintroduces the exact double report the comment says is avoided — unreadable and unset on one file, the second stating the switch's value from a document the run declined to read — and survived the whole file. Every existing test reaching that skip did so through a TOML syntax error, never through walk exhaustion.

Fixed in 6fba745, which adds test_a_config_that_parses_but_blows_the_walk_is_not_also_reported_unset. The lens's exact mutation was reproduced and now fails it. No production change this round: the behaviour was already correct, and what was missing was the thing holding it.

The doctrine's second-class rule would have permitted filing this rather than fixing it (Medium, not a regression, and this change is a report field nothing acts on). It was fixed instead because a twelve-line test settles it permanently, where a filed note only helps if someone acts on it.

A process failure of mine, recorded rather than smoothed over

#666 requires the round's receipt to be recorded before that round's fixes. I recorded round 1's correctly at 2010d3a and then did not record round 2's before committing 6fba745. Attempting it afterwards, pr_watch.py --record-review --head 180b549… refused:

error: PR head changed during review (expected 180b549…, current 6fba745…);
review the new head before recording evidence

So round 2's evidence is not recordable, by the engine's own design — which is #666 working. The recovery is the re-run that step 6 already required: round 3 at 6fba745, with its receipt recorded before anything else changes.

Round classification, stated for dispute

This change is second class by the panel's blast-radius rule — a report line and a --json field, with the exit code deliberately untouched. It is not under safety-critical-changes.md. Round 3's delta is one test, which is executable code, so it takes the full panel rather than a delta pass.

https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij · 2026-09-07T20:35:47Z

Panel round 3 — `6fba745`, both lenses: no findings

That is step 6's termination condition met — a full pass finding nothing new — so this round closes the loop rather than opening another.

adversarial: four mutations, all killed by behaviour-asserting tests with driftcheck deselected and the deselected count confirmed each run — removing the absent-config.toml branch, adding unset to dead_registrations, flattening feature_unset = not feature_keys, and dropping unset from the footer tuple. It separately confirmed no other module in the repo consumes RegistrationStatus.state or inspect_registrations, so the new state value has no second consumer to break, and that kit-manifest.json's diff is exactly the two expected hash bumps with nothing smuggled alongside.

correctness: no findings. It checked each CHANGELOG claim against the code that implements it — including confirming init.sh really does instruct [features].hooks = true at both registration blocks, which is what the advisory line tells the reader — and recomputed both manifest hashes independently. Four mutations, all killed.

Verification, including what was not established

make test at 6fba745e87b8915f007ade498ce5fab4f7f30c43 on 2026-09-07, run by the correctness lens in its own worktree → 1 failed, 2460 passed, 1 skipped in 390.80s. The one failure is test_pr_followup_hook.py::test_a_payload_too_deep_for_json_load_still_exits_zero, in a file this diff does not touch.
The adversarial lens's full-suite run was cut short by its own kill at roughly 84–90%, after one failure whose node id it could not recover because its output was piped through tail. It reported that as an unestablished result rather than inferring the flake, which is the right call and is recorded here as-is.
CI is green at this head.

The one open item, replied to rather than fixed

The adversarial lens's round-1 Medium on #709 has a sibling here worth stating: kit_commit is a presence-only exemption, so anyone willing to add that single key to the file a guard reads defeats it. That is kit_doctor._was_written_by_record_install's existing convention rather than anything these PRs introduce, and narrowing it would be a new mechanism no finding asked for. Filed as an observation, not built.

Round summary

round	head	adversarial	correctness
1	`2010d3a`	2 low (coverage)	1 medium — `render()` unpinned
2	`180b549`	1 medium — walk-exhaustion route unpinned	none
3	`6fba745`	none	none

Both mediums were the same shape and neither was a behaviour defect: a property the code got right and no test held. That is the third and fourth instance this session of the pattern the 2026-08-22 friction entry parked for accumulation — every finding in a claim about the work rather than in the work.

https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij · 2026-09-07T20:36:17Z

Review disposition — `6fba745`

reviewed head: 6fba745e87b8915f007ade498ce5fab4f7f30c43
review source: fallback:panel
lenses: adversarial, correctness

Round 3 at 6fba745: both lenses reported no findings, which is step 6's termination condition. Four mutations per lens, all killed by behaviour-asserting tests with driftcheck deselected and the deselected count confirmed. Rounds 1 and 2 each found one medium, both the same shape and neither a behaviour defect: a property the code got right and no test held — render() for the new state (round 1), and the walk-exhaustion route into the unreadable skip (round 2). Both are now pinned by tests that fail against the exact mutation the lens used. make test at 6fba745 on 2026-09-07 printed 1 failed, 2460 passed, 1 skipped in 390.80s; the one failure is test_pr_followup_hook.py::test_a_payload_too_deep_for_json_load_still_exits_zero, a pre-existing order-dependent flake in a file this diff does not touch. The round-3 adversarial lens self-interrupted its own full-suite run and reported that as an unestablished result rather than inferring the flake. Not fixed, replied-to: kit_commit is a presence-only exemption, which is kit_doctor._was_written_by_record_install's existing convention rather than anything this PR introduces. Process note: round 2's receipt was not recorded before its fix, and pr_watch refused it afterwards because the head had moved — #666 working as designed; round 3 supersedes it.

…ntries Panel round 2, both lenses at aefcdcf. Merges #708. **Both lenses independently found the same high finding.** `SKIP:adopter-baseline` fell into the shell's warn branch, so every push in every `--record-install` adopter printed a fail-open warning forever — telling them a check "could not" run when it had deliberately declined to, and pointing them at a kit-repo self-check that is not theirs. It contradicted this PR's own CHANGELOG entry and arrived through the very mechanism meant to stop a guard going unread. All three quiet outcomes are answers; only a failure to check warns. `test_an_adopter_install_baseline_is_left_alone` asserted the exit code and never stderr, so it stayed green throughout. "Left alone" has to mean quiet, and it now asserts that. **The second high is round 1's, one level down.** A `files` entry carrying no usable `sha256` was dropped from the checked set while the run still printed `CHECKED`, so a tracked, tampered file went unhashed with no signal. Unusable entries are now reported by path. They do not refuse the push: an unusable record is a broken manifest rather than a stale file, and this check cannot tell whether those bytes are current — so it says that instead of implying either. The stale list also lost its indent after the first path: `printf` with one newline-joined argument never reuses its format string. Each of the three fixes fails against `aefcdcf`'s hook and passes against this one, checked by running the new tests against both. Refs #706 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij added 2 commits September 7, 2026 22:19

Add the CHANGELOG entry for the unset registration state

2010d3a

`kit-manifest.json` does not track `CHANGELOG.md`, so this commit needs no manifest regeneration. Refs #698 Claude-Session: https://claude.ai/code/session_019VqvhqHbSiZaLJjUnoy3nY

topij merged commit ec75075 into main Sep 7, 2026
2 checks passed

topij deleted the chore/698-doctor-features-hooks branch September 7, 2026 20:52

This was referenced Sep 7, 2026

chore: update handoff — #698's doctor correction and #706's push gate merged #711

Merged

Lens isolation is scoped to the handed tree, but a lens can name any path — two occurrences reached outside it #712

Open

Navigation Menu

Grade [features].hooks when a Codex registration exists, not when config.toml does - #708

Grade [features].hooks when a Codex registration exists, not when config.toml does#708
topij merged 4 commits into
mainfrom
chore/698-doctor-features-hooks

topij commented Sep 7, 2026

Uh oh!

coderabbitai Bot commented Sep 7, 2026 •

edited

Loading

Review skipped

Uh oh!

topij commented Sep 7, 2026

Uh oh!

topij commented Sep 7, 2026

Uh oh!

topij commented Sep 7, 2026

Uh oh!

topij commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

topij commented Sep 7, 2026

What was wrong

What changed

Live behaviour

Scope

Verification

Uh oh!

coderabbitai Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

topij commented Sep 7, 2026

Fallback review panel — disposition

Round 1 — 2010d3a: both lenses, one medium finding

Not attributable to this diff

Process finding against the panel itself

Uh oh!

topij commented Sep 7, 2026

Panel round 2 — 180b549, both lenses

A process failure of mine, recorded rather than smoothed over

Round classification, stated for dispute

Uh oh!

topij commented Sep 7, 2026

Panel round 3 — 6fba745, both lenses: no findings

Verification, including what was not established

The one open item, replied to rather than fixed

Round summary

Uh oh!

topij commented Sep 7, 2026

Review disposition — 6fba745

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Sep 7, 2026 •

edited

Loading

Round 1 — `2010d3a`: both lenses, one medium finding

Panel round 2 — `180b549`, both lenses

Panel round 3 — `6fba745`, both lenses: no findings

Review disposition — `6fba745`