From 9d9d07e9a901a5e893d21594147235a326be807e Mon Sep 17 00:00:00 2001 From: tupe12334 Date: Mon, 26 Oct 2026 03:18:34 +0200 Subject: [PATCH 1/3] test(gate): cover the checklist gate's untested state-machine paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gate.rs` holds the per-checklist gate decision — the sequential checklist state machine — and was the only module under `core/src` with no `_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) --- core/src/gate.rs | 5 + core/src/gate_tests.rs | 209 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+) create mode 100644 core/src/gate_tests.rs diff --git a/core/src/gate.rs b/core/src/gate.rs index 3d79de5..6643e9f 100644 --- a/core/src/gate.rs +++ b/core/src/gate.rs @@ -250,3 +250,8 @@ fn build_block_message( msg } + +#[cfg(test)] +#[allow(clippy::panic, clippy::unwrap_used)] +#[path = "gate_tests.rs"] +mod tests; diff --git a/core/src/gate_tests.rs b/core/src/gate_tests.rs new file mode 100644 index 0000000..b3c9009 --- /dev/null +++ b/core/src/gate_tests.rs @@ -0,0 +1,209 @@ +//! Unit tests for `gate`. +use super::*; +use crate::state::save_state; +use std::collections::HashMap; +use std::path::PathBuf; +use tempfile::TempDir; + +const LINEAR_FLOW: &str = r"stateDiagram-v2 + [*] --> clean_code + clean_code --> tests_pass + tests_pass --> [*] + clean_code: Did you write clean code? + tests_pass: Do the tests pass? +"; + +const BRANCHING_FLOW: &str = r"stateDiagram-v2 + [*] --> review + review --> ship + review --> abort + ship --> [*] + abort --> [*] + review: Reviewed the diff? + ship: Ship it + abort: Abandon the change +"; + +fn make_event(session: &str) -> HookEvent { + let mut input = HashMap::new(); + input.insert( + "command".to_owned(), + serde_json::Value::String("git push".to_owned()), + ); + HookEvent { + event: "tool:before".to_owned(), + tool: "bash".to_owned(), + input, + output: HashMap::new(), + session_id: session.to_owned(), + caller: "claude-code".to_owned(), + } +} + +/// Writes `.steplock/checklists//` and returns `(steplock_dir, checklist_dir)`. +fn setup(root: &Path, name: &str, reset: &str, flow: &str) -> (PathBuf, PathBuf) { + let steplock_dir = root.join(".steplock"); + let checklist_dir = steplock_dir.join("checklists").join(name); + fs::create_dir_all(&checklist_dir).unwrap(); + fs::write( + checklist_dir.join("config.toml"), + format!( + r#"on_event = "tool:before" +on_tool = "bash" +match_input = "input.command.contains('git push')" +reset = "{reset}" +"# + ), + ) + .unwrap(); + fs::write(checklist_dir.join("flow.mmd"), flow).unwrap(); + (steplock_dir, checklist_dir) +} + +fn state_path(steplock_dir: &Path, scope: &str, checklist: &str) -> PathBuf { + steplock_dir + .join("sessions") + .join(scope) + .join(checklist) + .join("state.json") +} + +fn block_message(resp: Option) -> String { + match resp { + Some(HookResponse::Block { message }) => message, + other => panic!("expected a block, got {other:?}"), + } +} + +#[test] +fn empty_session_id_creates_and_reuses_a_fallback_scope_key() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", LINEAR_FLOW); + let event = make_event(""); + + evaluate_checklist(&event, &steplock_dir, &checklist_dir).unwrap(); + let fallback_path = steplock_dir.join("sessions").join("fallback-id"); + let scope = fs::read_to_string(&fallback_path) + .unwrap() + .trim() + .to_owned(); + assert!(!scope.is_empty(), "fallback id should be persisted"); + assert!(state_path(&steplock_dir, &scope, "gate").exists()); + + // A second event with no session id must land in the same scope, so + // checklist progress survives callers that do not supply a session id. + evaluate_checklist(&event, &steplock_dir, &checklist_dir).unwrap(); + let scope_again = fs::read_to_string(&fallback_path) + .unwrap() + .trim() + .to_owned(); + assert_eq!(scope, scope_again); +} + +#[test] +fn single_transition_state_precomputes_next_state() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", LINEAR_FLOW); + + evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); + + let state = load_state(&state_path(&steplock_dir, "sess-1", "gate")).unwrap(); + assert_eq!(state.current_state, "clean_code"); + assert_eq!(state.next_state.as_deref(), Some("tests_pass")); + assert_eq!(state.transitions, vec!["tests_pass".to_owned()]); + // ack.sh needs the whole graph to advance beyond the current state. + assert!(state.flow_transitions.contains_key("tests_pass")); +} + +#[test] +fn branching_state_leaves_next_state_unset_and_lists_every_choice() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", BRANCHING_FLOW); + + let resp = evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); + let message = block_message(resp); + + let state = load_state(&state_path(&steplock_dir, "sess-1", "gate")).unwrap(); + assert_eq!( + state.next_state, None, + "an ambiguous branch must not auto-advance" + ); + assert_eq!( + state.transitions, + vec!["ship".to_owned(), "abort".to_owned()] + ); + + assert!(message.contains("run one of"), "message was: {message}"); + assert!(message.contains("ack.sh ship"), "message was: {message}"); + assert!(message.contains("ack.sh abort"), "message was: {message}"); +} + +#[test] +fn state_missing_from_the_flow_is_skipped() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", LINEAR_FLOW); + let path = state_path(&steplock_dir, "sess-1", "gate"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + // Simulates flow.mmd being edited mid-session: the stored state is gone. + save_state(&path, &init_state("gate", "state_that_was_removed")).unwrap(); + + let resp = evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); + assert!(resp.is_none(), "an unknown state must not block forever"); +} + +#[test] +fn step_counter_counts_visited_states() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", LINEAR_FLOW); + let path = state_path(&steplock_dir, "sess-1", "gate"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + let mut state = init_state("gate", "tests_pass"); + state.visited = vec!["clean_code".to_owned()]; + save_state(&path, &state).unwrap(); + + let message = block_message( + evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(), + ); + assert!( + message.starts_with("[gate: 2/2] Do the tests pass?"), + "message was: {message}" + ); +} + +#[test] +fn reset_always_blocks_without_writing_session_state() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "always", LINEAR_FLOW); + + let message = block_message( + evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(), + ); + assert!(message.contains("When done, retry your original command.")); + assert!(!message.contains("ack.sh"), "message was: {message}"); + assert!( + !steplock_dir.join("sessions").exists(), + "reset=always keeps no session state" + ); +} + +#[test] +fn completed_checklist_approves_once_and_restarts() { + let tmp = TempDir::new().unwrap(); + let (steplock_dir, checklist_dir) = setup(tmp.path(), "gate", "session", LINEAR_FLOW); + let path = state_path(&steplock_dir, "sess-1", "gate"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + let mut state = init_state("gate", "[*]"); + state.visited = vec!["clean_code".to_owned(), "tests_pass".to_owned()]; + save_state(&path, &state).unwrap(); + + let resp = evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); + assert!(resp.is_none(), "a complete checklist approves the attempt"); + + let reset = load_state(&path).unwrap(); + assert_eq!(reset.current_state, "clean_code"); + assert!(reset.visited.is_empty()); + + // The next attempt blocks again from the top. + let next = evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); + assert!(matches!(next, Some(HookResponse::Block { .. }))); +} From 4f0f0ee1ac8d0bc4b399785b8681aec1a69813d4 Mon Sep 17 00:00:00 2001 From: tupe12334 Date: Mon, 26 Oct 2026 03:41:02 +0200 Subject: [PATCH 2/3] test(gate): assert the audit arguments the gate passes per path `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 = ` - `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) --- core/src/gate.rs | 2 +- core/src/gate_tests.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/core/src/gate.rs b/core/src/gate.rs index 6643e9f..30c13cf 100644 --- a/core/src/gate.rs +++ b/core/src/gate.rs @@ -252,6 +252,6 @@ fn build_block_message( } #[cfg(test)] -#[allow(clippy::panic, clippy::unwrap_used)] +#[allow(clippy::indexing_slicing, clippy::panic, clippy::unwrap_used)] #[path = "gate_tests.rs"] mod tests; diff --git a/core/src/gate_tests.rs b/core/src/gate_tests.rs index b3c9009..b5f1bee 100644 --- a/core/src/gate_tests.rs +++ b/core/src/gate_tests.rs @@ -75,6 +75,16 @@ fn block_message(resp: Option) -> String { } } +/// `audit::append` swallows every error, so the arguments the gate passes are +/// only observable through the log itself. +fn audit_lines(steplock_dir: &Path) -> Vec { + fs::read_to_string(steplock_dir.join("audit.log")) + .unwrap() + .lines() + .map(|l| serde_json::from_str(l).unwrap()) + .collect() +} + #[test] fn empty_session_id_creates_and_reuses_a_fallback_scope_key() { let tmp = TempDir::new().unwrap(); @@ -113,6 +123,13 @@ fn single_transition_state_precomputes_next_state() { assert_eq!(state.transitions, vec!["tests_pass".to_owned()]); // ack.sh needs the whole graph to advance beyond the current state. assert!(state.flow_transitions.contains_key("tests_pass")); + + let lines = audit_lines(&steplock_dir); + assert_eq!(lines.len(), 1); + assert_eq!(lines[0]["event"], "block"); + assert_eq!(lines[0]["checklist"], "gate"); + assert_eq!(lines[0]["state"], "clean_code"); + assert_eq!(lines[0]["session"], "sess-1"); } #[test] @@ -184,6 +201,13 @@ fn reset_always_blocks_without_writing_session_state() { !steplock_dir.join("sessions").exists(), "reset=always keeps no session state" ); + + // reset=always has no session scope, so the gate logs the literal "always". + let lines = audit_lines(&steplock_dir); + assert_eq!(lines.len(), 1); + assert_eq!(lines[0]["event"], "block"); + assert_eq!(lines[0]["state"], "clean_code"); + assert_eq!(lines[0]["session"], "always"); } #[test] @@ -203,6 +227,11 @@ fn completed_checklist_approves_once_and_restarts() { assert_eq!(reset.current_state, "clean_code"); assert!(reset.visited.is_empty()); + let lines = audit_lines(&steplock_dir); + assert_eq!(lines[0]["event"], "complete"); + assert_eq!(lines[0]["state"], "[*]"); + assert_eq!(lines[0]["session"], "sess-1"); + // The next attempt blocks again from the top. let next = evaluate_checklist(&make_event("sess-1"), &steplock_dir, &checklist_dir).unwrap(); assert!(matches!(next, Some(HookResponse::Block { .. }))); From 6d4d983ddc9d2c9ff4543cbd840f89127220e580 Mon Sep 17 00:00:00 2001 From: tupe12334 Date: Mon, 26 Oct 2026 03:46:25 +0200 Subject: [PATCH 3/3] chore(lint): ratchet clippy::verbose_file_reads to deny 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) --- core/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/core/Cargo.toml b/core/Cargo.toml index 8111cd8..a103440 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -89,6 +89,7 @@ manual_let_else = "deny" items_after_statements = "deny" needless_pass_by_value = "deny" trivially_copy_pass_by_ref = "deny" +verbose_file_reads = "deny" [dependencies] cel-interpreter = "0.10"