From 3ad98cde53cb406c5035bfeb39ec6088049b44e7 Mon Sep 17 00:00:00 2001 From: Ofek Gabay Date: Tue, 29 Sep 2026 13:56:27 +0300 Subject: [PATCH] fix(ack): set the next state's transitions when ack.sh advances ack.sh moved current_state forward but left the old state's transitions in state.json. A second ack before the hook fired again failed with "invalid next state ''", so the agent had to retry the gated command after every step. The gate now stores the flow's transition map in state.json as flow_transitions, and ack.sh sets transitions and next_state for the state it advances to. ensure_ack_sh rewrites an ack.sh that differs from the current script, so sessions started by an older steplock get the fix on their next block. The unused duplicate core/src/ack.sh is gone. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++ core/scripts/ack.sh | 5 +++- core/src/ack.sh | 41 --------------------------- core/src/gate.rs | 3 ++ core/src/run_tests.rs | 3 ++ core/src/scripts.rs | 5 ++-- core/src/scripts_tests.rs | 7 ++--- core/src/state.rs | 5 ++++ core/src/state_tests.rs | 3 ++ core/tests/cli_tests.rs | 47 +++++++++++++++++++++++++++++++ schemas/session-state.schema.json | 5 ++++ 11 files changed, 79 insertions(+), 48 deletions(-) delete mode 100644 core/src/ack.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index ee42145..7d629f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- `ack.sh` left the previous state's `transitions` in `state.json`, so a second ack before the hook fired again failed with `invalid next state ''`. The agent had to retry the gated command after every step. `ack.sh` now sets `transitions` and `next_state` for the new state, and a session started by an older steplock gets the fixed `ack.sh` on its next block + ## [0.2.0] - 2026-09-29 ### Changed diff --git a/core/scripts/ack.sh b/core/scripts/ack.sh index 8553090..4ac944d 100644 --- a/core/scripts/ack.sh +++ b/core/scripts/ack.sh @@ -24,10 +24,13 @@ if [ -z "$MATCHED" ]; then exit 1 fi +# Set the new state's transitions from the flow, so the next ack works +# without the hook firing in between. jq --arg cur "$CURRENT" --arg next "$NEXT" ' .visited += [$cur] | .current_state = $next | - .next_state = null + .transitions = (.flow_transitions[$next] // []) | + .next_state = (if (.transitions | length) == 1 then .transitions[0] else null end) ' "$STATE" > "$TMP" && mv "$TMP" "$STATE" # Append ack event to audit.log — failures silently ignored (audit must never block). diff --git a/core/src/ack.sh b/core/src/ack.sh deleted file mode 100644 index 8553090..0000000 --- a/core/src/ack.sh +++ /dev/null @@ -1,41 +0,0 @@ -#!/bin/sh -DIR="$(cd "$(dirname "$0")" && pwd)" -STATE="$DIR/state.json" -TMP="$STATE.tmp.$$" - -CURRENT=$(jq -r '.current_state' "$STATE") - -if [ "$CURRENT" = "[*]" ]; then - CHECKLIST=$(jq -r '.checklist' "$STATE") - echo "steplock: nothing to acknowledge for '$CHECKLIST' (session already complete)" - exit 0 -fi - -NEXT=$(jq -r '.next_state // empty' "$STATE") -NEXT="${1:-$NEXT}" - -VALID=$(jq -r '.transitions[]' "$STATE") -MATCHED=$(printf '%s\n' $VALID | grep -Fx "$NEXT") - -if [ -z "$MATCHED" ]; then - echo "steplock: invalid next state '$NEXT'" >&2 - echo "Valid transitions from '$CURRENT':" >&2 - printf '%s\n' $VALID | sed 's/^/ /' >&2 - exit 1 -fi - -jq --arg cur "$CURRENT" --arg next "$NEXT" ' - .visited += [$cur] | - .current_state = $next | - .next_state = null -' "$STATE" > "$TMP" && mv "$TMP" "$STATE" - -# Append ack event to audit.log — failures silently ignored (audit must never block). -STEPLOCK_DIR="$(cd "$DIR/../../.." && pwd)" -SESSION="$(basename "$(dirname "$DIR")")" -CHECKLIST="$(jq -r '.checklist' "$STATE")" -TS="$(date -u +%Y-%m-%dT%H:%M:%SZ)" -jq -cn --arg e "ack" --arg c "$CHECKLIST" --arg s "$CURRENT" \ - --arg sess "$SESSION" --arg ts "$TS" \ - '{event:$e,checklist:$c,state:$s,session:$sess,ts:$ts}' \ - >> "$STEPLOCK_DIR/audit.log" 2>/dev/null || true diff --git a/core/src/gate.rs b/core/src/gate.rs index 93f5a73..3d79de5 100644 --- a/core/src/gate.rs +++ b/core/src/gate.rs @@ -1,4 +1,5 @@ //! Checklist gate: decides whether one checklist blocks a hook event. +use std::collections::HashMap; use std::fmt::Write as _; use std::fs; use std::path::Path; @@ -90,6 +91,7 @@ fn block_reset_always( current_state: initial_state.to_owned(), next_state, transitions, + flow_transitions: HashMap::new(), visited: vec![], }; audit::append( @@ -152,6 +154,7 @@ fn block_reset_session( .cloned() .filter(|_| raw_transitions.len() == 1); state.transitions = raw_transitions; + state.flow_transitions.clone_from(&flow.transitions); save_state(&state_path, &state)?; scripts::ensure_ack_sh(&session_dir)?; diff --git a/core/src/run_tests.rs b/core/src/run_tests.rs index 6286d7b..dd6cf5d 100644 --- a/core/src/run_tests.rs +++ b/core/src/run_tests.rs @@ -76,6 +76,7 @@ fn approves_and_resets_state_when_complete() { current_state: "[*]".to_owned(), next_state: None, transitions: vec![], + flow_transitions: HashMap::new(), visited: vec!["clean_code".to_owned()], }; save_state(&session_dir.join("state.json"), &state).unwrap(); @@ -635,6 +636,7 @@ fn unknown_current_state_in_flow_skips_checklist() { current_state: "nonexistent_state".to_owned(), next_state: None, transitions: vec![], + flow_transitions: HashMap::new(), visited: vec![], }; save_state(&session_dir.join("state.json"), &state).unwrap(); @@ -660,6 +662,7 @@ fn complete_event_written_to_audit_log() { current_state: "[*]".to_owned(), next_state: None, transitions: vec![], + flow_transitions: HashMap::new(), visited: vec!["clean_code".to_owned()], }; save_state(&session_dir.join("state.json"), &state).unwrap(); diff --git a/core/src/scripts.rs b/core/src/scripts.rs index 126245d..6b51279 100644 --- a/core/src/scripts.rs +++ b/core/src/scripts.rs @@ -7,14 +7,15 @@ use crate::flow::FlowGraph; static ACK_SH: &str = include_str!("../scripts/ack.sh"); -/// Write ack.sh to `dir` only if it does not already exist. +/// Write ack.sh to `dir` unless it already holds the current script. +/// A session started by an older steplock gets the current script on its next block. /// /// # Errors /// /// Returns `Err` if writing the file or setting its permissions fails. pub fn ensure_ack_sh(dir: &Path) -> Result<()> { let path = dir.join("ack.sh"); - if path.exists() { + if fs::read_to_string(&path).is_ok_and(|s| s == ACK_SH) { return Ok(()); } write_executable(&path, ACK_SH) diff --git a/core/src/scripts_tests.rs b/core/src/scripts_tests.rs index 0992c61..458c6b3 100644 --- a/core/src/scripts_tests.rs +++ b/core/src/scripts_tests.rs @@ -41,13 +41,12 @@ fn ack_sh_appends_audit_event() { } #[test] -fn ensure_ack_sh_is_idempotent() { +fn ensure_ack_sh_replaces_stale_script() { let tmp = TempDir::new().unwrap(); let path = tmp.path().join("ack.sh"); - fs::write(&path, "custom content").unwrap(); + fs::write(&path, "old script").unwrap(); ensure_ack_sh(tmp.path()).unwrap(); - // Should not overwrite existing file - assert_eq!(fs::read_to_string(&path).unwrap(), "custom content"); + assert_eq!(fs::read_to_string(&path).unwrap(), ACK_SH); } #[test] diff --git a/core/src/state.rs b/core/src/state.rs index e749363..57915a1 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -22,6 +22,10 @@ pub struct SessionState { pub next_state: Option, /// All valid next state names from `current_state` (including `"[*]"`). pub transitions: Vec, + /// Outgoing transitions for every state in the flow, so `ack.sh` can set + /// `transitions` and `next_state` for the state it advances to. + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub flow_transitions: HashMap>, /// States that have been acknowledged in this session so far. pub visited: Vec, } @@ -66,6 +70,7 @@ pub fn init_state(checklist: &str, initial_state: &str) -> SessionState { current_state: initial_state.to_owned(), next_state: None, transitions: Vec::new(), + flow_transitions: HashMap::new(), visited: Vec::new(), } } diff --git a/core/src/state_tests.rs b/core/src/state_tests.rs index 0a3e0fe..f371bbc 100644 --- a/core/src/state_tests.rs +++ b/core/src/state_tests.rs @@ -1,5 +1,6 @@ //! Unit tests for `state`. use super::*; +use std::collections::HashMap; use tempfile::TempDir; #[test] @@ -25,6 +26,7 @@ fn is_complete_true_at_end() { current_state: "[*]".to_owned(), next_state: None, transitions: vec![], + flow_transitions: HashMap::new(), visited: vec!["step_one".to_owned()], }; assert!(s.is_complete()); @@ -39,6 +41,7 @@ fn save_and_load_roundtrip() { current_state: "check_one".to_owned(), next_state: Some("check_two".to_owned()), transitions: vec!["check_two".to_owned()], + flow_transitions: HashMap::new(), visited: vec!["prev".to_owned()], }; save_state(&path, &s).unwrap(); diff --git a/core/tests/cli_tests.rs b/core/tests/cli_tests.rs index 55cf6f0..dbc03e1 100644 --- a/core/tests/cli_tests.rs +++ b/core/tests/cli_tests.rs @@ -416,3 +416,50 @@ fn hermes_hook_uses_real_home_global_checklist_when_home_is_sandboxed() { "global checklist from the real home must run: {json}" ); } + +#[test] +fn ack_sh_advances_twice_without_a_hook_call_between() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join(".steplock/checklists/gate"); + fs::create_dir_all(&dir).unwrap(); + fs::write( + dir.join("config.toml"), + "on_event = \"tool:before\"\non_tool = \"bash\"\nreset = \"session\"\n", + ) + .unwrap(); + fs::write( + dir.join("flow.mmd"), + "stateDiagram-v2\n [*] --> one\n one --> two\n two --> left\n two --> right\n left --> three\n right --> three\n three --> [*]\n one: Step one\n two: Step two\n left: Go left\n right: Go right\n three: Step three\n", + ) + .unwrap(); + + let stdin = hook_event("bash", "git push", "sess-ack"); + let (first_code, _, _) = run_steplock(tmp.path(), &stdin); + assert_eq!(first_code, 0, "first call blocks at step one"); + + let ack = tmp.path().join(".steplock/sessions/sess-ack/gate/ack.sh"); + let run_ack = |arg: Option<&str>| { + let mut cmd = Command::new("sh"); + cmd.arg(&ack); + if let Some(a) = arg { + cmd.arg(a); + } + cmd.output().unwrap() + }; + // one -> two (linear), two -> right (branch), right -> three (linear): no hook call between. + for arg in [None, Some("right"), None] { + let out = run_ack(arg); + assert!( + out.status.success(), + "ack {arg:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + let (code, stdout, _) = run_steplock(tmp.path(), &stdin); + assert_eq!(code, 0); + assert!( + stdout.contains("Step three"), + "expected block at step three, got: {stdout}" + ); +} diff --git a/schemas/session-state.schema.json b/schemas/session-state.schema.json index a098d50..5853604 100644 --- a/schemas/session-state.schema.json +++ b/schemas/session-state.schema.json @@ -24,6 +24,11 @@ "description": "Valid next state names from current_state. ack.sh validates $1 against this list.", "items": { "type": "string" } }, + "flow_transitions": { + "type": "object", + "description": "Outgoing transitions for every state in the flow. ack.sh reads it to set transitions and next_state for the state it advances to.", + "additionalProperties": { "type": "array", "items": { "type": "string" } } + }, "visited": { "type": "array", "description": "State node names already acknowledged this scope. Written by ack.sh.",