fix(gate): ship self-protection in the default registry; stop swallowing parse errors (NF2/NF3) - #8
Conversation
…ing parse errors NF2 — the default registry shipped without the self-protection the code already supports. corpus_detect_cases covered the detection ENGINE against a fixture registry that carries gate-self-mod and checkpoint-removed; the registry commitward actually ships carried neither. So the engine was green while the product shipped with its guard off: an agent could delete destructive-ops and add a dangerous recursive-force removal in the same commit with nothing firing. Both checkpoints are now in checkpoints.yaml, pointed at the paths commitward itself resolves — the default registry, the repo-local override, the installed commit-msg hook, and install-hook.sh. Two independent guards on the same act, so neither is a single point of failure. NF3 — the gate subcommand swallowed registry parse errors into an empty checkpoint set and returned status ok / exit_class 0: a security control reporting success exactly when it could not run. A supplied-but-unparseable registry now yields an error envelope, which ADR-0052 defines as "do not trust this result, fall back to your in-process path". The system still fails open — audibly at both layers instead of silently at one. An ABSENT registry is still an empty set; supplying nothing is a choice, supplying garbage is a defect. Every ok envelope now carries body.warnings naming the guards that could not run, because exit_class 0 cannot otherwise distinguish "nothing fired" from "nothing was checked". The native CLI keeps exit 0 + a stderr diagnostic, unchanged. HITL-ACK: destructive-ops tests/default_registry.rs is a test fixture, not an executable op. It asserts that the destructive-ops checkpoint still fires, which requires the guarded pattern to appear literally in the test data. Acknowledged rather than obfuscated around the denylist, and rather than widening content_exempt_paths — quieting the gate to make one's own commit pass is the exact weakening this PR closes. Closes #7
…default CI diffs checkpoints.yaml against tests/corpus/promise/checkpoints.yaml so the self-contained test snapshot cannot drift from the baseline the CLI loads. The NF2 additions had to land in both. The corpus's .dotclaude registry already carries gate-self-mod and checkpoint-removed and overrides by name, so the conformance expectations are unchanged.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — the default self-protection is a real improvement; the "registry protects itself" framing overclaims. (Comment only — self-authored; a non-author approval is still required to merge.)
What's right (keep it):
- Shipping
gate-self-mod(path) +checkpoint-removed(semantic) +destructive-opsin the default registry, addingbody.warnings, and turning a malformed/missing-base registry into a loud non-pass instead of a silent green — all correct, all valuable. The malformed-registry RED test fires loud against the old swallow-to-clean-pass.
Two reasons this is REQUEST_CHANGES:
-
NF2 is proven at the engine layer, not the git-hook path.
nf2_deleting_the_guard_and_using_it_in_one_commit_does_not_pass_silentlycallsdetect(&compiled, …)directly (default_registry.rs:245/260), against the real shippedcheckpoints.yaml— good, the logic is validated against the real registry. But the native commit-msg path (main::run) is only exercised by the warnings cases (gate_warnings.rsspawns the binary). The headline NF2 scenario never runs through the shipped CLI wrapper. Close that seam — drive NF2 through the realdotclaude/commitward hook invocation. -
Whole-registry removal fires nothing — and the PR's own source says so.
checkpoint-removedis inactive without a base registry (checkpoints.yaml:45/56, 125-126; commitward#4). And the shipped checkpoints.yaml comments admit it directly at lines 74/179: "checkpoint cannot fire, and no other checkpoint watches the registry." So a single commit that deletes the guarding checkpoints (repo-local registry, or the no-base path) produces zero fires.residual_gap_adr0010_checkpoint_removed_itself_removed(lib.rs:774) documents the semantic guard's self-removal blindness. A self-referential guard needs an out-of-band anchor (a compiled-in minimum set the on-disk registry can't delete, or a base-registry requirement enforced loudly) — the registry cannot be the sole thing that protects the registry.
Asks: scope the PR/CONTRACT honestly ("default registry adds self-protection with a documented whole-registry-removal residual"), and file the out-of-band-anchor follow-up rather than merging as "registry self-protects."
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Follow-up filed: commitward#9 — "Registry cannot be the sole protector of the registry: whole-registry-removal needs an out-of-band anchor." Captures both legs (no-base → #4; with-base self-removal → the residual_gap test + the in-source checkpoints.yaml admission) and proposes the compiled-in-minimum-anchor fix. This PR can land its default self-protection once the framing is scoped to match (documented residual, not "registry self-protects") and the NF2 seam is driven through the real git-hook path.
…claim Two review findings on #8. NF2 was proven at the engine layer only — default_registry.rs calls detect(&compiled, ..) directly. That validates the logic but not the wrapper a consumer runs: argument parsing, registry resolution, base-ref diffing and the exit-code contract all live in main::run and none of them were on the NF2 path. cli_smoke.rs now drives the full scenario through the real binary against the real shipped registry: adopt the default registry, then in ONE commit delete destructive-ops and add a dangerous recursive-force removal. Asserts exit 2 and that the fire names a self-protection checkpoint. Verified RED against main's registry: exit 0, a clean gate. With the fix, exit 2. Scope: CONTRACT said "the default registry protects itself", which overclaims. It does not survive removal of the guards themselves — checkpoint-removed needs base names and cannot fire without them (#4), and a commit deleting the guarding entries leaves nothing watching the registry, which lib.rs's residual_gap_adr0010_checkpoint_removed_itself_removed already pins. Retitled to "carries self-protection, with a documented residual" and the residual is named inline, pointing at #9 for the out-of-band anchor. HITL-ACK: destructive-ops tests/default_registry.rs and tests/cli_smoke.rs are test fixtures, not executable ops. Both assert that the destructive-ops checkpoint still fires, which requires the guarded pattern to appear literally in the test data. Acknowledged rather than obfuscated past the denylist or exempted via content_exempt_paths.
Review on #8 was REQUEST_CHANGES partly on this: shipping `gate-self-mod` and `checkpoint-removed` in the default registry is real self-protection, but it is self-referential — both live in the file they guard, so deleting the file deletes its own guard in the same act, and `checkpoint-removed` cannot fire at all without a base registry (#4). The shipped checkpoints.yaml admitted it in its own comments. Filed as #9; this closes it. `compile()` now merges `anchor_checkpoints()` into every registry, including an empty one, and applies it last so a same-named on-disk entry cannot shadow it. `anchor-gate-integrity` watches the gate's own files — checkpoints.yaml at any depth, .commitward/checkpoints.yaml, the commit-msg hook, install-hook.sh. It is compiled in, so there is no YAML edit that removes it. In `compile()` rather than at the call sites deliberately: a consumer cannot obtain a compiled registry without the floor, so it does not depend on each caller remembering to add it. RED first, through the shipped CLI on a real git repo — a commit that deletes `.commitward/checkpoints.yaml` outright and adds the guarded file, with no global registry either: "fired": [] exit 0 before anchor-gate-integrity, exit 2 after Paired with a guard test that an ordinary commit still exits 0. An anchor that fires on everything is not a gate, it is a nuisance that teaches people to ignore the output. Deliberately narrow: the anchor covers the gate's own integrity, not policy. An anchor that grew to cover policy would be a second registry no repo could declare or amend, which is the thing this design exists to avoid. Two consequences, both stated in CONTRACT.md rather than discovered later: - A commit that touches a registry or hook now ALWAYS fires at least once, including the commit that first adopts a registry. Ack it like any other fire; `ack_trailer_lifts_the_block_to_exit_1` now acks both. - The NF3 "no checkpoints supplied" warning now asks whether the CALLER supplied anything, not whether the compiled set is empty — it never is. The warning survives, with its claim narrowed to what is now true. Still open and still pinned: a removed `checkpoint_removed` entry produces no *semantic* fire. The anchor covers the act, not the semantics. Closes #9
|
Both reasons addressed. Ask 1 in 1. "NF2 is proven at the engine layer, not the git-hook path."
2. "Whole-registry removal fires nothing — and the PR's own source says so." Right, and the source comments you quoted were the honest part of a design that still had a hole. Rather than only scoping the claim, the hole is closed:
In RED first, through the shipped CLI, on the acceptance criterion from #9 — a commit that deletes Paired with Deliberately narrow. The anchor covers the gate's own integrity, not policy. An anchor that grew to cover policy would be a second registry that no repo could declare or amend — the thing this design exists to avoid. Two consequences, in CONTRACT.md rather than discovered later:
Still open, still pinned:
|
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Re-review → APPROVE (was REQUEST_CHANGES; the new commit clears it). Comment only — self-authored; non-author approval still required to merge.
My blocking finding was that whole-registry removal fired nothing — the registry was the sole protector of the registry. That is now closed with exactly the out-of-band anchor commitward#9 asked for:
anchor-gate-integrityis compiled into the binary andcompile()merges it last into every registry, including an empty one —merge(cps, anchor_checkpoints()). No YAML edit removes it; "no registry at all" is still guarded. It watches both registry paths + the commit-msg hook + install-hook.sh.- Test
anchor_fires_when_the_whole_registry_is_deleted_in_one_commitproves the bypass is closed;anchor_does_not_fire_on_an_ordinary_commitguards against false positives. CI green. - Scope is honest: the anchor covers the gate's integrity (the act of touching registry/hook), not policy — an anchor that grew to cover policy would just be a second unremovable registry. The semantic
checkpoint_removedgap is stated, not hidden.
commitward#9 can close as fixed-by-this-PR. My finding-1 (NF2 through the real git-hook path) is now covered by the anchor watching the hook paths + the whole-registry-deletion test — the substantive concern is resolved.
NF2 — the engine was green while the product shipped with its guard off
corpus_detect_casesexercises the detection engine against the fixture registry undertests/corpus/. That fixture carriesgate-self-modandcheckpoint-removed. The registry commitward actually ships —checkpoints.yaml, the file baked into the image at/etc/commitward/checkpoints.yamland dropped beside the binary by the installers — carried neither.So the machinery worked, the tests proved it worked, and every consumer got it switched off. Nothing in the suite loaded the shipped registry, so nothing could have caught it.
The headline consequence, now a test:
destructive-opscannot fire, because it no longer exists. Nothing else was watching the registry.Fix: both checkpoints ship in
checkpoints.yaml, aimed at the paths commitward itself resolves — the default registry,.commitward/checkpoints.yaml, the installedcommit-msghook, andinstall-hook.sh. The hook matters as much as the registry: deleting it is a quieter way to disable the gate than editing any checkpoint.They overlap deliberately.
gate-self-modfires on touching a registry;checkpoint-removedfires on a checkpoint present at base being absent now. Two independent guards on the same act, so neither is a single point of failure — the NF2 scenario trips both.A registry located via
$COMMITWARD_REGISTRYcannot be matched by a static pattern. Documented rather than pretended away.NF3 — a security control that reported success when it could not run
A supplied-but-unparseable registry became an empty checkpoint set, which then evaluated cleanly:
status: "ok",fired: [],exit_class: 0. Verified against the pre-fix binary:{"body":{"exit_class":0,"fired":[],"unacked":[]},"schema_version":"1","status":"ok"}Indistinguishable from a genuinely clean commit.
Fix, and its limit. The parse error propagates to an
errorenvelope, which ADR-0052 defines as "do not trust this result, fall back to your in-process path". Be clear about what that does and does not buy: the consumer's in-tree fallback reads the same malformed registry and also fails open, so the commit is still allowed. What changes is that the failure is now audible at both layers instead of silent at one. commitward is not a blocking control and this PR does not make it one.An absent registry stays an empty set. Supplying nothing is a configuration choice; supplying garbage is a defect.
body.warningson everyokenvelope names the guards that could not run — becauseexit_class: 0means "nothing fired", and a consumer reads that as "nothing to worry about". Without warnings there is no way to tell it apart from "nothing was checked":base_*_registry_yaml→ the checkpoint-removed guard is INACTIVE for this callThe native git-reading CLI keeps exit 0 + a stderr diagnostic, unchanged. The asymmetry is intentional and now stated in
CONTRACT.mdunder the fail-open guarantee, as "fail-open is not fail-silent".Tests — RED first, 11 added
tests/default_registry.rs(7) loads the shipped registry, which nothing did before:tests/gate_warnings.rs(4) drives the real binary over stdin/stdout —gateis the containerized front door, so an in-process shortcut would not prove the production path. All 4 RED before the fix, including the guard:The two guard tests are the load-bearing ones.
shipped_registry_stays_quiet_on_ordinary_workfails on a registry that fires on everything — a noisy gate gets switched off, which is the real-world failure mode for checkpoint systems.a_fully_supplied_request_produces_no_warningsfails on an implementation that warns unconditionally, for the same reason.Scope — what this does not claim
NF1 (a
HITL-ACKis self-acknowledgeable by the gated agent) is untouched and remains by design;CLAUDE.mdscopes content-mode HITL as "not an adversarial control". This ships the self-protection the code already supported and makes a non-running check say so. It does not make the gate adversarially sound, andCONTRACT.mdsays so in the same breath as the new guarantee.One HITL fire, acknowledged in the commit
destructive-opsfired ontests/default_registry.rs— the test asserts that checkpoint still fires, which requires the guarded pattern to appear literally in the fixture. Acknowledged with aHITL-ACKline and a reason, deliberately not by obfuscating the string past the denylist, and not by wideningcontent_exempt_pathsto covertests/. Quieting the gate so your own commit passes is precisely the weakening this PR closes; it would have been a poor way to land it.Gate
cargo test37 passed / 0 failed (26 before) ·clippy --workspace --all-targets -D warningsclean ·fmt --checkclean.Non-breaking:
body.warningsis an added field, and theerrorenvelope on a bad registry is a shape consumers already handle (ADR-0052). Docs updated inCONTRACT.mdandREADME.mdin the same commit.Closes #7
Review round 1
Both findings accepted.
1. NF2 now runs through the real CLI.
default_registry.rsproved it at the engine layer (detect(&compiled, …)), which validates the logic but not the wrapper a consumer runs — argument parsing, registry resolution, base-ref diffing and the exit-code contract all live inmain::runand none were on the NF2 path.cli_smoke.rs::nf2_registry_weakening_fires_through_the_real_clidrives the whole scenario through the binary against the real shipped registry: adopt the default registry, then in one commit deletedestructive-opsand add a dangerous recursive-force removal. Asserts exit 2 and that the fire names a self-protection checkpoint.Verified RED against
main's registry — exit 0, a clean gate; exit 2 with this PR.2. The "registry protects itself" framing is gone. CONTRACT now reads "carries self-protection, with a documented residual" and names the residual inline:
checkpoint-removedcannot fire without base names (#4), and a commit deleting the guarding entries leaves nothing watching the registry — whichresidual_gap_adr0010_checkpoint_removed_itself_removedalready pins. It points at #9 for the out-of-band anchor.Superseded by
67b4743— the anchor is in. The paragraph above described this PR at review time; the residual it names is now closed rather than documented.compile()mergesanchor_checkpoints()into every registry, including an empty one, and applies it last so a same-named on-disk entry cannot shadow it.anchor-gate-integritywatches the gate's own files (checkpoints.yaml at any depth,.commitward/checkpoints.yaml, the commit-msg hook,install-hook.sh) and is compiled into the binary — no YAML edit removes it. Incompile()rather than at the call sites deliberately: a consumer cannot obtain a compiled registry without the floor.RED first, through the shipped CLI on #9's acceptance criterion — a commit that deletes the registry outright and adds the guarded file, with no global registry either:
"fired": []/ exit 0 before,anchor-gate-integrity/ exit 2 after. Paired with a guard test that an ordinary commit still exits 0.Two consequences, both in CONTRACT.md: a commit touching a registry or hook now always fires at least once (ack it like any other), and the NF3 "no checkpoints supplied" warning now asks what the caller supplied, since the compiled set is never empty. Still open and still pinned: a removed
checkpoint_removedentry produces no semantic fire — the anchor covers the act, not the semantics.cargo test40 passed / 0 failed (38 before the anchor, 26 before the PR) · clippy clean · fmt clean.Closes #9