Repository navigation
test(gate): assert the checklist gate's unasserted state-machine behaviour - #237
Merged
Merged
Conversation
`gate.rs` holds the per-checklist gate decision — the sequential checklist state machine — and was the only module under `core/src` with no `<module>_tests.rs`. `run_tests.rs` exercised it only through the happy single-step path, so several behaviours could regress undetected: - the `fallback-id` scope key used when a caller sends no `session_id` - `next_state` auto-advance vs. a branching state with several transitions - the multi-choice "run one of" block message - the silent skip when `state.json` names a state the flow no longer has - the `step/total` counter derived from `visited` - `reset = "always"`, which must block without persisting session state - the complete-then-restart cycle Add `core/src/gate_tests.rs` with a test per behaviour. No production code changes. Opened by the moadim routine "Polyhook/steplock production-readiness audit → PR + Slack". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`audit::append` swallows every I/O error by design, and `audit_tests.rs` pins only the JSONL format, never which arguments a caller passes. The gate is its only caller and passes different arguments per path: - `reset = "session"` block: `event = "block"`, `session = <scope key>` - `reset = "always"` block: `event = "block"`, `session = "always"` - complete: `event = "complete"`, `state = "[*]"` None of that was asserted anywhere, so a wrong argument could never fail loudly. Assert it in the three gate tests that reach those paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pre-push checklist asks every push to promote one more clippy lint. `verbose_file_reads` fits this crate, which reads config, flow and state files on the gate's hot path. No violations to fix. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Gap
core/src/gate.rsholdsevaluate_checklist— the per-checklist gate decision, i.e. the sequential checklist state machine. It had no companion<module>_tests.rs.Correction to this description as originally written: I claimed
gate.rswas the only module undercore/src/without one. That was wrong —catalog.rs(20 lines) anderror.rs(52 lines) also lack a companion test file.gate.rswas the largest such module at 257 lines, which is why it was picked, but it was not the only one.Measured correction to an earlier draft of this description:
gate.rsline coverage was already 98.01% before this PR. I verified that withcargo llvm-cov --summary-onlyand again with-- --skip gate::tests(confirming 0gate::testsran) — the numbers are identical either way.So the gap is not unexecuted lines.
run_tests.rsandtests/gate_integration.rsalready ran these lines; nothing asserted their results. That is precisely the failure mode a coverage percentage hides: a line counts as covered the moment it executes, whether or not any assertion checks what it produced.Behaviour this PR pins
fallback-idscope key when a caller sends nosession_idnext_stateauto-advance on a single transitionack.shrelies onnext_stateunsetack.shsilently picks a path for the agentrun one of:block messagestate.jsonnames a state the flow no longer hasflow.mmdis edited mid-sessionstep/totalcounter derived fromvisitedreset = "always"audit::appendargumentsThe audit contract
audit::appendswallows every I/O error by design, andaudit_tests.rspins only the JSONL format — never which arguments a caller passes. The gate is its only caller and passes different arguments per path:reset = "session"block →event = "block",session = <scope key>reset = "always"block →event = "block",session = "always"(a literal, not a session id)event = "complete",state = "[*]"None of that was asserted anywhere in the crate, and because
appendnever surfaces an error, a wrong argument could not fail loudly in any other test.Change
core/src/gate_tests.rs— new, 7 tests, wired intogate.rswith the same#[path = "<module>_tests.rs"]pattern the rest of the crate uses (per refactor: move unit tests into <module>_tests.rs files #210).core/src/gate.rs— +5 lines, test-module declaration and its clippy allows only.core/Cargo.toml— promotesclippy::verbose_file_readstodenyper the repo's pre-push lint ratchet. Zero violations to fix.No production logic changed.
Verification
Every runnable repo CI gate, locally:
cargo fmt -- --checkcargo clippy --all-targets --locked -- -D warningscargo test --lockedcargo +1.88 test --locked(MSRV)RUSTDOCFLAGS="-D warnings" cargo doc --no-depscargo package --lockedcargo llvm-covgate.rs98.01% lines / 92.49% regionscargo-macheteandcargo-denyare not installed locally; both pass in CI, and neither can be affected by a diff that adds no dependency.gate_tests.rsis 238 lines — overlinecheck.yml's 200-line warn threshold, well under its 1100-line error threshold.Decision record
Reviewed through the
pr-improvementgate, with each decision put to the local Kev decision model:state.jsondirectly vs. drive throughack.shaudit_tests.rsdoes and does not pin → add the assertions, p=0.8792. Done in 4f0f0ee.6d4d983.Opened by the moadim routine "Polyhook/steplock production-readiness audit → PR + Slack".
🤖 Generated with Claude Code
Merged at merge noul 0.8366 against the 0.8 own-PR gate, with every check green and no administrator bypass.