diff --git a/docs/vrs/05-harness-state/.experiments/2026-08-23-claude-batched-permission.md b/docs/vrs/05-harness-state/.experiments/2026-08-23-claude-batched-permission.md new file mode 100644 index 00000000..33830528 --- /dev/null +++ b/docs/vrs/05-harness-state/.experiments/2026-08-23-claude-batched-permission.md @@ -0,0 +1,83 @@ +# Claude batched tool calls under an open permission prompt + +Date: 2026-08-23. Binary: Claude Code 2.1.237 (Fable 5 bundle), Linux, driven under `pty`. +Purpose: take the capture DQ-H1 (and #268 §C) said was missing — a batch where one call needs +permission — and settle whether the shipped blocked-exit rule +(`src/claude_session.rs::observe_hook_event`: clear on next `PreToolUse`/`PostToolUse`/`Stop`) +false-clears while a prompt is open. + +## Method + +A scratch project (isolated CWD; hooks are project-scoped) with `.claude/settings.local.json` +allowlisting `Bash(echo:*)` and registering logging hooks that append every payload as one JSON +line and always exit 0, for: `PreToolUse`, `PostToolUse`, `PermissionRequest`, `Stop`, +`SubagentStop` — later runs added `PermissionDenied`, `UserPromptSubmit`, `SessionEnd`. + +- Runs 1–2: `claude -p` (non-interactive), 120–180 s timeouts. +- Runs 3–4: interactive `claude --permission-mode default` in a detached ephemeral `pty` session + (`pty run -d -e --id --cwd --env LOGFILE=… -- claude --permission-mode default`), + prompts sent with `pty send --seq … --seq key:return`, screen sampled with `pty peek --plain`, + the log dumped **while the prompt was open** and again after answering. + +Four short turns total, ≈$0.61. + +## Measured sequences + +**Run 2, `-p`, batch allowlisted-first** (`echo a` + `touch scratch-file.txt`): + +``` +PreToolUse Bash "echo a" tool_use_id=toolu_014yt… +PostToolUse Bash "echo a" (before the second call starts) +PreToolUse Bash "touch scratch-file.txt" tool_use_id=toolu_01RrZ… +PreToolUse Write … (fallback attempt, then waits) +Stop +``` + +No `PermissionRequest` fires in `-p` mode; the non-interactive path blocks/denies without one. +Execution is strictly serial: the first call's `Post` precedes the second call's `Pre`. + +**Run 3, interactive, batch permission-first** (`touch scratch2.txt` + allowlisted `echo b`): + +``` +481.34 PreToolUse Bash "touch scratch2.txt" tool_use_id=toolu_01HK5… prompt_id=0ea832de… +481.67 PermissionRequest Bash "touch scratch2.txt" NO tool_use_id prompt_id=0ea832de… + — prompt open 33 s; log dumped during: NO further event of any kind, + although the TUI already rendered the batched `echo b` — +514.10 PostToolUse Bash "touch scratch2.txt" tool_use_id=toolu_01HK5… (the grant) +514.30 PreToolUse Bash "echo b" tool_use_id=toolu_01NhR… +514.59 PostToolUse Bash "echo b" +516.17 Stop +519.44 SubagentStop agent_id="a5c61ec4ef268c3cc" agent_type="" (phantom, +3.3 s) +``` + +**Runs 3b/4, deny path** ("3. No" on the prompt, run 4 with `PermissionDenied` + `SessionEnd` +registered): after `PreToolUse` + `PermissionRequest`, denial produced **zero further events** — +no `PostToolUse`, no `Stop`, no `PermissionDenied`. The turn ends silently. + +## Findings + +1. **Execution serializes around an open permission prompt.** In both orderings no hook event + fires while a prompt is up — a parallel-batched allowlisted call waits out the grant. The + false-clear #268 §C predicted (first call's `Post` clearing `blocked` during the second call's + prompt) is unobservable in this build: the next `Pre`/`Post`/`Stop` after `PermissionRequest` + is always the blocked call's own resolution. The shipped exit rule is correct, not merely + conservative. +2. **`PermissionRequest` still carries no call identity.** `tool_use_id` is present on + `PreToolUse`/`PostToolUse` but absent on `PermissionRequest`; its `prompt_id` is turn-scoped + (identical across every event of the turn) and cannot correlate a call. +3. **Denial is eventless.** Even with `PermissionDenied` registered, "No" ends the turn with no + event, so `blockedOn: human` stands until the next `UserPromptSubmit`/`SessionStart`. Half-true + semantically (a person's direction is still what the session waits on), but the state axis + reads `active` while the model is stopped. Pinned as DQ-H1's residual limit. +4. **The phantom `SubagentStop` reproduces**: 3.3 s after `Stop`, `agent_id` non-empty, + `agent_type` the empty string, no subagent ran — the guard's emergent-property basis holds in + this build. +5. `-p` mode never fires `PermissionRequest`; permission evidence exists only interactively. + +## VRS Impact + +- DQ-H1 re-resolved: the batched-capture gate is met; the exit rule stands on measured ground and + the open question narrows to the eventless deny window. +- The measured grant sequence is replayed verbatim by + `src/claude_session.rs::measured_batched_grant_sequence_holds_blocked_until_the_granted_calls_own_post`. +- No requirement text changes: OHS-R05's producer rule is confirmed, not amended. diff --git a/docs/vrs/05-harness-state/open-questions.md b/docs/vrs/05-harness-state/open-questions.md index 9e9d32ec..2caf89e4 100644 --- a/docs/vrs/05-harness-state/open-questions.md +++ b/docs/vrs/05-harness-state/open-questions.md @@ -4,16 +4,27 @@ Each entry links a spec `DQ-H*`. Questions leave this file when resolved — into [spec.md](./spec.md) as decisions or `.experiments/` as tested hypotheses. -- **DQ-H1 Claude blocked-exit edge.** `PermissionRequest` carries no - `tool_use_id`, so leaving `blockedOn: human` can only match on tool *name* — - and Claude batches tool calls (`PostToolBatch` carries plural `tool_calls`), - so the first call's `PostToolUse` would clear `blocked` while the human still - faces the second call's prompt. The corpus enters `blocked` in 2 of 9 - captures and exits in 1, with one tool and no batching: a rule validated on - a single exit path is not validated. Until then the producer holds - `blockedOn: human` until turn end (`Stop`) rather than encoding an exit rule - that cannot hold. Resolves by: a capture with batched tool calls where the - second call needs permission, then specifying the exit edge against it. +- **DQ-H1 Claude blocked-exit edge.** The batched capture #268 §C asked for + was taken on 2026-08-23 against Claude Code 2.1.237 + (`.experiments/2026-08-23-claude-batched-permission.md`), and it resolves + the predicted false-clear in the rule's favor: **tool execution is + serialized around an open permission prompt.** In both batch orderings + (allowlisted-first and permission-first), no hook event of any kind fires + while the prompt is open — a parallel-batched allowlisted call renders on + screen but its `PreToolUse` waits 33 s for the grant — so the next + `PreToolUse`/`PostToolUse`/`Stop` after `PermissionRequest` is precisely the + blocked call's own resolution, and the shipped rule + (`src/claude_session.rs::observe_hook_event`) is correct, not merely + conservative. `PermissionRequest` still carries no `tool_use_id` (its + `prompt_id` is turn-scoped, shared by every event in the turn — not a call + correlator). The residual limit is the **deny path**: selecting "No" ends + the turn with *zero* further events — no `PostToolUse`, no `Stop`, and no + `PermissionDenied` even when that hook is registered — so `blockedOn: human` + stands until the next `UserPromptSubmit` or `SessionStart`. That reading is + semantically half-true (a human's direction is still what the session + waits on) and it under-reports nothing, but the state axis says `active` + while the model is not running. Resolves by: a Claude build whose denial + emits any hook event; until then the deny window is the pinned limit. - **DQ-H2 Transport cost of per-transition writes.** Presence refreshes every five minutes; turn boundaries are far more frequent, and burst coalescing measured 4 transitions per turn 0.1–0.4 ms apart. No measurement establishes @@ -48,12 +59,13 @@ hypotheses. `unknown` is no fresh observation, never proof of ill health, and never gates local work. Resolves by: specifying remote-reader semantics with a proof, or explicitly scoping the record same-host advisory. -- **DQ-H6 OpenCode state source.** The producer needs a verified source. - Candidate: OpenCode's server/SDK event surface (session state, message - lifecycle); fallback: the shipped composer adapter's positive markers - (`ctrl+p commands` footer) with documented limits. Nothing is measured yet — - the DING adapter (#313) proves only composer classification. Resolves by: - an `.experiments/` capture of OpenCode's event surface on a pinned version, - then choosing the source and its skew policy — the repo's standing rule - (pin where version skew fails silently, as `checks.pi-extension-types` and - `SUPPORTED_CODEX_CLI_VERSIONS` do) applies. +- **DQ-H6 OpenCode blocked-entry capture.** The state source itself is + resolved: the server's SSE event surface, measured on 1.18.19 + (`.experiments/2026-08-23-opencode-surface.md`) and gated by + `SUPPORTED_OPENCODE_VERSIONS` plus the live `/doc` subset check. What + remains open is the blocked-on-human pair: `permission.asked` / + `permission.replied` are schema-backed with explicit `^per` ids — the exit + edge is clean by construction, unlike Claude's — but no live capture of a + real permission prompt exists (headless runs with `{"bash":"ask"}` never + asked). Resolves by: one capture from a TUI seat with a real permission + prompt, confirming the events fire and carry the id the producer matches. diff --git a/docs/vrs/05-harness-state/spec.md b/docs/vrs/05-harness-state/spec.md index 9ad8dfeb..ef399467 100644 --- a/docs/vrs/05-harness-state/spec.md +++ b/docs/vrs/05-harness-state/spec.md @@ -167,13 +167,24 @@ classified from the payload's `tool_name` (`AskUserQuestion` → `question`, anything else → `permission`) — (its meaning is specifically "a human is about to be asked" — it fires only under permission modes that ask). Events carrying `agent_id` are subagent-nested and never move -top-level state. The blocked *exit* edge under batched tool calls is -unspecified until a capture proves a rule (`DQ-H1`); until then the spec -states the limit rather than a rule that cannot hold. The wrapper side owns -liveness: it re-stamps the heartbeat on its existing presence cadence while -the child is alive, and writes the terminal record from its `try_wait` reap -and its SIGTERM path — before any SIGKILL escalation into its own process -group, which no in-process write survives (OHS-T04). +top-level state. The blocked *exit* edge is the next `PreToolUse`, +`PostToolUse`, or `Stop` — measured-correct, not merely conservative: the +2026-08-23 batched-permission capture (`DQ-H1`) shows tool execution +serializes around an open permission prompt, so no event can clear the +blocked state early. The residual limit is the eventless deny path pinned in +`DQ-H1`. The wrapper side owns liveness: it re-stamps the heartbeat on its +existing presence cadence while the child is alive — through a fresh writer +each time, so it never clobbers a state a hook process wrote in between — +and writes the terminal record from its `try_wait` reap and its SIGTERM +path, before any SIGKILL escalation into its own process group, which no +in-process write survives (OHS-T04). Hook registration has one canonical +shape (`hooks::claude_settings_registration`): the maintained example +declaration (`examples/native/agent-claude.kdl`) carries it by hand, and +`expand_claude` renders the same `.claude/settings.local.json` upsert for +driver-declared seats, so both surfaces register identical hooks and a test +fails if they drift. A live seat converges without disruption: render output +is not part of the launch fingerprint, the merged settings land on the next +materialization pass, and the hooks take effect at the next session start. ## pi producer (OHS-R05, OHS-R08) diff --git a/examples/native/agent-claude.kdl b/examples/native/agent-claude.kdl index 48f95d19..0822e36d 100644 --- a/examples/native/agent-claude.kdl +++ b/examples/native/agent-claude.kdl @@ -18,7 +18,13 @@ agent "" { copy "assets/bus.st2.md" ".st2/bus.md" ensure-line ".claude/rules/st2.md" "@../../.st2/PERSONA.md" ensure-line ".claude/rules/st2.md" "@../../.st2/bus.md" - json-upsert ".claude/settings.local.json" #""" + // Hooks-only observability: this seat launches claude directly (no session wrapper), so the + // registrations below give it transitions and blocked-on-you — but no heartbeat owner and no + // terminal record. A live-but-idle seat ages to `unknown` after the staleness horizon and an + // exit leaves the last state to age out; both read indeterminate, never wrong. The full + // producer (heartbeats, terminal exits) comes with the session wrapper, i.e. a typed + // `claude {}` driver seat or `deliver "mcp"`. + json-upsert ".claude/settings.local.json" arrays="union" #""" { "$schema": "https://json.schemastore.org/claude-code-settings.json", "hooks": { @@ -29,7 +35,11 @@ agent "" { "type": "command", "async": true, "asyncRewake": true, - "command": "$ST_HOOKS/claude-session-start.sh" + "command": "\"$ST_HOOKS/claude-session-start.sh\"" + }, + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" SessionStart" } ] } @@ -39,7 +49,7 @@ agent "" { "hooks": [ { "type": "command", - "command": "$ST_HOOKS/claude-pre-compact.sh" + "command": "\"$ST_HOOKS/claude-pre-compact.sh\"" } ] } @@ -49,7 +59,57 @@ agent "" { "hooks": [ { "type": "command", - "command": "$ST_HOOKS/claude-stop-failure.sh" + "command": "\"$ST_HOOKS/claude-stop-failure.sh\"" + } + ] + } + ], + "UserPromptSubmit": [ + { + "hooks": [ + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" UserPromptSubmit" + } + ] + } + ], + "Stop": [ + { + "hooks": [ + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" Stop" + } + ] + } + ], + "PermissionRequest": [ + { + "hooks": [ + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" PermissionRequest" + } + ] + } + ], + "PreToolUse": [ + { + "hooks": [ + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" PreToolUse" + } + ] + } + ], + "PostToolUse": [ + { + "hooks": [ + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" PostToolUse" } ] } diff --git a/hooks/claude-observe.sh b/hooks/claude-observe.sh new file mode 100755 index 00000000..d3bc2677 --- /dev/null +++ b/hooks/claude-observe.sh @@ -0,0 +1,21 @@ +#!/usr/bin/env bash +# st2 Claude observe hook: forward one hook event (name in $1, payload on stdin) to the agent's +# observed-harness-state record. Fail-open; observation must never wedge or slow the harness. + +set -u + +event="${1:-}" +identity="${ST_AGENT:-}" +# CATALOG-first, deliberately diverging from the sibling hooks' ST_ROOT-first order: their +# ST_ROOT is a bus root for message writes, while --catalog here resolves the agent DECLARATION — +# with a custom bus root (ST_ROOT != CATALOG) declaration resolution under ST_ROOT finds nothing +# and every transition would silently drop. +root="${CATALOG:-${ST_ROOT:-}}" +runtime_id="${ST2_CLAUDE_RUNTIME_ID:-$identity}" +if [[ -z "$event" || -z "$identity" || -z "$root" ]] || ! command -v st2 >/dev/null 2>&1; then + exit 0 +fi + +st2 --catalog "$root" driver claude-observe --identity "$identity" --runtime-id "$runtime_id" \ + --event "$event" >/dev/null 2>&1 || true +exit 0 diff --git a/src/catalog_transaction.rs b/src/catalog_transaction.rs index 243bde09..091e0f39 100644 --- a/src/catalog_transaction.rs +++ b/src/catalog_transaction.rs @@ -922,6 +922,7 @@ fn normalize_agent(spec: &agent_spec::AgentSpec) -> Result { insert_value( &mut fields, @@ -945,6 +946,14 @@ fn normalize_agent(spec: &agent_spec::AgentSpec) -> Result { insert_value( diff --git a/src/claude_session.rs b/src/claude_session.rs index 753e3827..cc7db370 100644 --- a/src/claude_session.rs +++ b/src/claude_session.rs @@ -4,13 +4,22 @@ //! provider still lives. This wrapper launches the provider and refreshes presence while that exact //! child remains alive. It uses the provider's existing terminal process group. The launch body //! itself lives in [`crate::provider_session`], which every interactive harness wrapper shares. +//! +//! Observed harness state for Claude has two producers with one owner each: hook invocations +//! (`st2 driver claude-observe`, [`run_observe`]) write turn transitions, and the wrapper's poll +//! loop re-stamps and terminates the record through [`SessionObserver`] without ever overwriting a +//! state a hook wrote in between. +use std::io::Read as _; use std::path::Path; use anyhow::{Context as _, Result}; -use crate::provider_session::{PROVIDER_POLL, STOP, install_signal_handler, run_provider}; -use crate::{message, status}; +use crate::harness_state::{Activity, Ask, BlockedOn, InputBuffer, Observation}; +use crate::provider_session::{ + PROVIDER_POLL, STOP, SessionObserver, install_signal_handler, run_provider, +}; +use crate::{harness_state, message, status}; /// Run one interactive Claude provider and maintain its presence until it exits. pub fn run( @@ -27,18 +36,187 @@ pub fn run( "Claude driver '{runtime_id}' has no provider argv" ); install_signal_handler(); + let observer = SessionObserver::new(&agent_dir, &identity, "claude", &runtime_id)?; + // The runtime ID reaches hook subprocesses through the provider environment, so their + // transitions carry the same pty session the wrapper's records do. + let env = [ + (RUNTIME_ID_ENV.to_string(), runtime_id.clone()), + // Hook subprocesses adopt the wrapper's incarnation token, so their transitions are this + // session's records: the wrapper can re-stamp them, and its terminal record fences them. + (SESSION_ENV.to_string(), observer.session().to_string()), + (SESSION_SEQ_ENV.to_string(), observer.seq().to_string()), + ]; run_provider( "Claude", &status::status_path(&agent_dir), &claude_argv, - &[], + &env, status::STATUS_REFRESH, PROVIDER_POLL, &STOP, + Some(&observer), ) .with_context(|| format!("running Claude driver '{runtime_id}'")) } +/// Apply one Claude hook event (payload on stdin) to the agent's observed-harness-state record. +/// +/// Invoked per event by the fail-open `claude-observe.sh` hook, so each invocation is its own +/// short-lived writer; the transition counter continues from disk. +/// The env var carrying the wrapper's runtime/task ID into Claude's hook subprocesses. +pub const RUNTIME_ID_ENV: &str = "ST2_CLAUDE_RUNTIME_ID"; +/// The env var carrying the wrapper's session incarnation token into Claude's hook subprocesses. +pub const SESSION_ENV: &str = "ST2_CLAUDE_SESSION"; +/// The env var carrying the wrapper's claimed ownership sequence beside the token. +pub const SESSION_SEQ_ENV: &str = "ST2_CLAUDE_SESSION_SEQ"; + +pub fn run_observe( + catalog_root: &Path, + identity: &str, + runtime_id: Option<&str>, + event: &str, +) -> Result<()> { + let agent_dir = message::resolve_agent_dir(catalog_root, identity, &crate::run::detect_host())? + .with_context(|| format!("Claude driver agent '{identity}' is not declared"))?; + let mut raw = String::new(); + let _ = std::io::stdin().read_to_string(&mut raw); + let payload = serde_json::from_str(&raw).unwrap_or(serde_json::Value::Null); + let Some(observation) = observe_hook_event(event, &payload) else { + return Ok(()); + }; + let mut writer = observe_writer( + &agent_dir, + identity, + runtime_id, + event, + &payload, + std::env::var(SESSION_ENV).ok().filter(|t| !t.is_empty()), + std::env::var(SESSION_SEQ_ENV) + .ok() + .and_then(|seq| seq.parse::().ok()), + ); + if event == "SessionStart" { + // The one event that names a session boundary: even if the new session's first state + // matches a fresh predecessor record, continuity must not be claimed across the restart. + writer.interrupt(); + } + // A late hook finishing after the wrapper reaped Claude must not replace the terminal record + // with a live state: the wrapper's `ended` carries this same token and is the session's last + // word. (`false` = suppressed; the hook has nothing else to do with it.) + writer.observe_unless_ended(observation).map(|_wrote| ()) +} + +/// Select the ownership a hook write acts under. The wrapper's exported token makes hook writes +/// this session's records (adopted ownership when the claimed sequence travels beside it). A +/// wrapperless seat falls back to Claude's own session_id — and because token-only writers never +/// claim, the SessionStart arm IS that path's session boundary and performs the WRITTEN claim +/// (degrading to token-only with a warning if the claim cannot be written); later hooks of the +/// same session adopt its records by token. What such a seat still lacks is a heartbeat and +/// terminal owner — the documented hooks-only limitation. +#[allow(clippy::too_many_arguments)] +fn observe_writer( + agent_dir: &Path, + identity: &str, + runtime_id: Option<&str>, + event: &str, + payload: &serde_json::Value, + exported_session: Option, + exported_seq: Option, +) -> harness_state::Writer { + let pty_session = runtime_id.unwrap_or(identity).to_string(); + let writer = harness_state::Writer::new(agent_dir, identity, "claude", Some(pty_session)); + if let Some(session) = exported_session { + return match exported_seq { + Some(seq) => writer.with_ownership(session, seq), + None => writer.with_session(session), + }; + } + if let Some(id) = payload + .get("session_id") + .and_then(serde_json::Value::as_str) + { + let token = format!("{}{id}", harness_state::WRAPPERLESS_PREFIX); + if event == "SessionStart" { + // Eligibility and the written takeover are ONE act under the record lock: a + // hooks-only SessionStart racing a wrapper's startup can no longer steal the + // sequence between the wrapper's read and its write. Ineligible (a live wrapper or + // its fresh claim placeholder owns the record) or unwritable both degrade to + // token-only. + return match harness_state::claim_wrapperless(agent_dir, identity, "claude", &token) { + Ok(Some(seq)) => writer.with_ownership(token, seq), + Ok(None) => writer.with_session(token), + Err(error) => { + eprintln!( + "st2 claude-observe: observed-state claim failed; degrading to token-only: {error:#}" + ); + writer.with_session(token) + } + }; + } + return writer.with_session(token); + } + writer +} + +/// Map one Claude hook event to an observation, or `None` when the event says nothing about +/// top-level harness state. +/// +/// Claude gives no call identity on the event that enters `blocked` (`PermissionRequest` carries +/// no `tool_use_id`; its `prompt_id` is turn-scoped), so the exit edge is the next +/// `PreToolUse`/`PostToolUse`/`Stop`. Measured 2026-08-23 (Claude Code 2.1.237, DQ-H1): tool +/// execution serializes around an open permission prompt — no hook event fires while a prompt is +/// up, even for a parallel-batched allowlisted call — so that next event is the blocked call's own +/// resolution and the batched false-clear #268 §C predicted cannot occur. The residual limit is +/// denial: "No" ends the turn with zero further events (no Stop, and no PermissionDenied even when +/// registered), so `blocked` stands until the next `UserPromptSubmit`/`SessionStart`. +pub fn observe_hook_event(event: &str, payload: &serde_json::Value) -> Option { + // Any event carrying an agent identity is a subagent's and must never move top-level state: + // a phantom `SubagentStop` trails every completed turn, 1.5-2.9s after `Stop`. That phantom + // populating `agent_id` is an undocumented emergent property — if a future Claude build omits + // it, subagent completions read as top-level activity again and nothing here would catch it. + if payload + .get("agent_id") + .and_then(serde_json::Value::as_str) + .is_some_and(|id| !id.is_empty()) + { + return None; + } + match event { + "SessionStart" => Some( + Observation::new(Activity::Idle, BlockedOn::None, InputBuffer::Unknown) + .with_reason("sessionStart"), + ), + "UserPromptSubmit" | "PreToolUse" | "PostToolUse" => Some(Observation::new( + Activity::Active, + BlockedOn::None, + InputBuffer::Unknown, + )), + "Stop" => Some(Observation::new( + Activity::Idle, + BlockedOn::None, + InputBuffer::Unknown, + )), + "PermissionRequest" => { + // Driver-side classification (#162): the payload's tool_name distinguishes Claude's + // question form from an ordinary permission prompt — the DQ-H1 captures show + // AskUserQuestion arriving as a PermissionRequest like any other tool. + let ask = if payload.get("tool_name").and_then(serde_json::Value::as_str) + == Some("AskUserQuestion") + { + Ask::Question + } else { + Ask::Permission + }; + Some( + Observation::new(Activity::Active, BlockedOn::Human, InputBuffer::Unknown) + .with_ask(ask) + .with_reason("permissionRequest"), + ) + } + _ => None, + } +} + #[cfg(test)] mod tests { use std::fs; @@ -46,6 +224,7 @@ mod tests { use std::time::Duration; use super::*; + use crate::harness_state::harness_state_path; #[test] fn idle_provider_refreshes_presence_without_mcp_input() { @@ -63,6 +242,7 @@ mod tests { Duration::from_millis(25), Duration::from_millis(5), &stop, + None, ) .unwrap(); @@ -70,4 +250,334 @@ mod tests { assert_ne!(after, before); assert_eq!(status::read_state(&presence), status::State::Available); } + + #[test] + fn hook_events_map_to_observations_with_the_blocked_edges() { + let none = serde_json::Value::Null; + + let blocked = observe_hook_event("PermissionRequest", &none).unwrap(); + assert_eq!(blocked.state, Activity::Active); + assert_eq!(blocked.blocked_on, BlockedOn::Human); + assert_eq!(blocked.reason.as_deref(), Some("permissionRequest")); + + // The exit edges: tool progress or a turn boundary clears the human hold. + for event in ["PreToolUse", "PostToolUse"] { + let cleared = observe_hook_event(event, &none).unwrap(); + assert_eq!(cleared.state, Activity::Active); + assert_eq!(cleared.blocked_on, BlockedOn::None); + } + let stop = observe_hook_event("Stop", &none).unwrap(); + assert_eq!(stop.state, Activity::Idle); + assert_eq!(stop.blocked_on, BlockedOn::None); + + assert_eq!( + observe_hook_event("UserPromptSubmit", &none).unwrap().state, + Activity::Active + ); + assert_eq!( + observe_hook_event("SessionStart", &none).unwrap().state, + Activity::Idle + ); + + // Unmapped events say nothing rather than guessing. + assert_eq!(observe_hook_event("Notification", &none), None); + assert_eq!(observe_hook_event("SubagentStop", &none), None); + } + + #[test] + fn subagent_events_never_move_top_level_state() { + let subagent = serde_json::json!({"agent_id": "sub-1", "agent_type": ""}); + for event in [ + "Stop", + "UserPromptSubmit", + "PermissionRequest", + "PostToolUse", + ] { + assert_eq!(observe_hook_event(event, &subagent), None, "{event}"); + } + // An empty agent_id is the top-level shape. + let top = serde_json::json!({"agent_id": ""}); + assert!(observe_hook_event("Stop", &top).is_some()); + } + + /// The measured grant-path sequence from the DQ-H1 capture (2026-08-23, Claude Code 2.1.237, + /// `docs/vrs/05-harness-state/.experiments/2026-08-23-claude-batched-permission.md`): in a + /// two-call batch where the first call needs permission, execution serializes around the open + /// prompt, so the event after `PermissionRequest` is the granted call's own `PostToolUse` and + /// the exit rule clears `blocked` at exactly the right moment. Replayed verbatim so a future + /// mapping change that breaks the measured sequence fails here, not in the field. + #[test] + fn measured_batched_grant_sequence_holds_blocked_until_the_granted_calls_own_post() { + let pre_touch = serde_json::json!({ + "hook_event_name": "PreToolUse", "tool_name": "Bash", + "tool_input": {"command": "touch scratch2.txt"}, + "tool_use_id": "toolu_01HK5aLKavjdbrCk48cfd58k", + "prompt_id": "0ea832de-ece1-4575-900f-4dab5e2f6849", + }); + let permission_request = serde_json::json!({ + "hook_event_name": "PermissionRequest", "tool_name": "Bash", + "tool_input": {"command": "touch scratch2.txt"}, + "permission_suggestions": [], + "prompt_id": "0ea832de-ece1-4575-900f-4dab5e2f6849", + }); + let post_touch = serde_json::json!({ + "hook_event_name": "PostToolUse", "tool_name": "Bash", + "tool_input": {"command": "touch scratch2.txt"}, + "tool_use_id": "toolu_01HK5aLkavjdbrCk48cfd58k", + "prompt_id": "0ea832de-ece1-4575-900f-4dab5e2f6849", + }); + // The phantom SubagentStop trailing the turn: non-empty agent_id, EMPTY agent_type, no + // subagent ran — the exact emergent shape the guard keys on, reproduced in this build. + let phantom_subagent_stop = serde_json::json!({ + "hook_event_name": "SubagentStop", + "agent_id": "a5c61ec4ef268c3cc", "agent_type": "", + "prompt_id": "0ea832de-ece1-4575-900f-4dab5e2f6849", + }); + + let entered = observe_hook_event("PermissionRequest", &permission_request).unwrap(); + assert_eq!( + (entered.state, entered.blocked_on), + (Activity::Active, BlockedOn::Human) + ); + // 33 s of open prompt produced no intervening event in the capture; the very next event + // is the granted call's own PostToolUse, which correctly releases the block. + let released = observe_hook_event("PostToolUse", &post_touch).unwrap(); + assert_eq!( + (released.state, released.blocked_on), + (Activity::Active, BlockedOn::None) + ); + let _ = observe_hook_event("PreToolUse", &pre_touch); + assert_eq!( + observe_hook_event("SubagentStop", &phantom_subagent_stop), + None, + "the phantom SubagentStop must not resurrect activity after Stop" + ); + } + + #[test] + fn wrapper_heartbeat_re_stamps_without_clobbering_hook_written_state() { + let tmp = tempfile::tempdir().unwrap(); + let record = harness_state_path(tmp.path()); + let observer = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + + // A hook process wrote a blocked observation between wrapper ticks — carrying the + // wrapper's exported token, exactly as the env plumbing arranges in a real seat. + harness_state::Writer::new( + tmp.path(), + "hetz.worker", + "claude", + Some("hetz.worker".to_string()), + ) + .with_session(observer.session()) + .observe(observe_hook_event("PermissionRequest", &serde_json::Value::Null).unwrap()) + .unwrap(); + let before = fs::read(&record).unwrap(); + + std::thread::sleep(Duration::from_millis(2)); + observer.heartbeat(); + let after = fs::read(&record).unwrap(); + assert_ne!(before, after, "heartbeat must re-stamp bytes"); + let observed = harness_state::read(&record, None).unwrap(); + assert_eq!(observed.state, Activity::Active); + assert_eq!(observed.blocked_on, BlockedOn::Human); + assert_eq!(observed.reason.as_deref(), Some("permissionRequest")); + } + + #[test] + fn a_provider_killed_mid_turn_reads_ended_rather_than_active() { + let tmp = tempfile::tempdir().unwrap(); + let presence = status::status_path(tmp.path()); + let record = harness_state_path(tmp.path()); + let observer = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + let stop = AtomicBool::new(false); + + // A turn is in flight when the provider dies by signal. + harness_state::Writer::new( + tmp.path(), + "hetz.worker", + "claude", + Some("hetz.worker".to_string()), + ) + .observe(observe_hook_event("UserPromptSubmit", &serde_json::Value::Null).unwrap()) + .unwrap(); + + let result = run_provider( + "Claude", + &presence, + &["sh".into(), "-c".into(), "kill -9 $$".into()], + &[], + Duration::from_millis(25), + Duration::from_millis(5), + &stop, + Some(&observer), + ); + assert!(result.is_err(), "a signalled provider is a failed run"); + + let observed = harness_state::read(&record, None).unwrap(); + assert_eq!(observed.state, Activity::Ended); + assert_eq!(observed.exit.as_deref(), Some("signal 9")); + } + + #[test] + fn a_clean_provider_exit_writes_the_terminal_record() { + let tmp = tempfile::tempdir().unwrap(); + let presence = status::status_path(tmp.path()); + let observer = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + let stop = AtomicBool::new(false); + + run_provider( + "Claude", + &presence, + &["true".into()], + &[], + Duration::from_millis(25), + Duration::from_millis(5), + &stop, + Some(&observer), + ) + .unwrap(); + + let observed = harness_state::read(&harness_state_path(tmp.path()), None).unwrap(); + assert_eq!(observed.state, Activity::Ended); + assert_eq!(observed.exit.as_deref(), Some("exit 0")); + } + + #[test] + fn permission_requests_classify_their_ask_kind_from_the_tool_name() { + use crate::harness_state::Ask; + let permission = observe_hook_event( + "PermissionRequest", + &serde_json::json!({ "tool_name": "Bash", "tool_input": {} }), + ) + .unwrap(); + assert_eq!(permission.ask, Ask::Permission); + + let question = observe_hook_event( + "PermissionRequest", + &serde_json::json!({ "tool_name": "AskUserQuestion", "tool_input": {} }), + ) + .unwrap(); + assert_eq!(question.ask, Ask::Question); + + // Non-blocking events carry no ask. + let idle = observe_hook_event("Stop", &serde_json::json!({})).unwrap(); + assert_eq!(idle.ask, Ask::None); + } + + /// T2: a hook that finishes after the wrapper reaped Claude must not replace the terminal + /// record — the wrapper's `ended` carries the shared token and is the session's last word — + /// while a NEW session's boundary event still supersedes an old terminal record. + #[test] + fn a_late_hook_never_overwrites_this_sessions_terminal_record() { + use crate::harness_state::{self, Activity}; + let tmp = tempfile::tempdir().unwrap(); + let record = harness_state_path(tmp.path()); + let observer = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + observer.ended("exit 0"); + + // The straggler hook shares the session token (env plumbing) and is suppressed. + let mut late = harness_state::Writer::new( + tmp.path(), + "hetz.worker", + "claude", + Some("hetz.worker".to_string()), + ) + .with_session(observer.session()); + assert!( + !late + .observe_unless_ended( + observe_hook_event("PostToolUse", &serde_json::Value::Null).unwrap() + ) + .unwrap() + ); + assert_eq!( + harness_state::read(&record, None).unwrap().state, + Activity::Ended + ); + + // A wrapperless fresh session (token-only, the session_id fallback) cannot take over a + // claimed record: only a written claim supersedes. + let mut fallback = harness_state::Writer::new( + tmp.path(), + "hetz.worker", + "claude", + Some("hetz.worker".to_string()), + ) + .with_session("claude-session-fresh"); + fallback.interrupt(); + assert!( + !fallback + .observe_unless_ended( + observe_hook_event("SessionStart", &serde_json::Value::Null).unwrap() + ) + .unwrap() + ); + assert_eq!( + harness_state::read(&record, None).unwrap().state, + Activity::Ended + ); + + // A claimed new session — the wrapper path — supersedes the old terminal record. + let next = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + let mut fresh = harness_state::Writer::new( + tmp.path(), + "hetz.worker", + "claude", + Some("hetz.worker".to_string()), + ) + .with_ownership(next.session().to_string(), next.seq()); + assert!( + fresh + .observe_unless_ended( + observe_hook_event("SessionStart", &serde_json::Value::Null).unwrap() + ) + .unwrap() + ); + assert_eq!( + harness_state::read(&record, None).unwrap().state, + Activity::Idle + ); + } + + /// W8-12: two sessions of a WRAPPERLESS seat. Session A's hooks write; session B's + /// SessionStart performs the written claim and takes over; A's straggler is refused and B's + /// later hooks adopt B's records. + #[test] + fn a_wrapperless_seat_survives_its_own_session_succession() { + use crate::harness_state::{self, Activity}; + let tmp = tempfile::tempdir().unwrap(); + let record = harness_state_path(tmp.path()); + let payload_a = serde_json::json!({ "session_id": "aaa" }); + let payload_b = serde_json::json!({ "session_id": "bbb" }); + let drive = |event: &str, payload: &serde_json::Value| { + let mut writer = + observe_writer(tmp.path(), "hetz.worker", None, event, payload, None, None); + writer + .observe_unless_ended(observe_hook_event(event, payload).unwrap()) + .unwrap() + }; + + assert!(drive("SessionStart", &payload_a), "A claims"); + assert!(drive("UserPromptSubmit", &payload_a), "A's hooks adopt"); + assert_eq!( + harness_state::read(&record, None).unwrap().state, + Activity::Active + ); + + assert!(drive("SessionStart", &payload_b), "B claims over A"); + assert!( + !drive("PostToolUse", &payload_a), + "A's straggler is refused" + ); + assert!(drive("UserPromptSubmit", &payload_b), "B's hooks adopt"); + assert_eq!( + harness_state::read(&record, None).unwrap().state, + Activity::Active + ); + } } diff --git a/src/driver.rs b/src/driver.rs index fe09648b..4c0f8dce 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -119,11 +119,25 @@ fn expand_claude(driver: &ClaudeDriver, bus_id: &str) -> Result { } }); let mcp = serde_json::to_string_pretty(&mcp)?; + // The same registration a hand-authored seat carries: without it a driver-declared + // seat has no observed-state producer and no lifecycle hooks at all. + let settings = serde_json::to_string_pretty(&crate::hooks::claude_settings_registration())?; let mut render = KdlNode::new("render"); - render.set_children(document([node( - "json-upsert", - vec![".mcp.json".to_string(), mcp], - )])); + render.set_children(document([ + node("json-upsert", vec![".mcp.json".to_string(), mcp]), + { + // Hook arrays join whatever the workspace already declares: replacement would clobber + // user-registered hooks on every materialization, and union is idempotent. + let mut upsert = node( + "json-upsert", + vec![".claude/settings.local.json".to_string(), settings], + ); + upsert + .entries_mut() + .push(KdlEntry::new_prop("arrays", "union")); + upsert + }, + ])); let mut provider = vec!["claude".to_string()]; if let Some(model) = &driver.model { @@ -263,8 +277,15 @@ mod tests { assert_eq!(output.nodes().len(), 2); let render = output.get("render").unwrap(); - let upsert = render.children().unwrap().get("json-upsert").unwrap(); - let upsert = strings(upsert); + let upserts: Vec<&KdlNode> = render + .children() + .unwrap() + .nodes() + .iter() + .filter(|node| node.name().value() == "json-upsert") + .collect(); + assert_eq!(upserts.len(), 2); + let upsert = strings(upserts[0]); assert_eq!(upsert[0], ".mcp.json"); let mcp: serde_json::Value = serde_json::from_str(upsert[1]).unwrap(); assert_eq!(mcp["mcpServers"]["st2"]["type"], "stdio"); @@ -280,6 +301,10 @@ mod tests { "host.worker" ]) ); + let settings = strings(upserts[1]); + assert_eq!(settings[0], ".claude/settings.local.json"); + let settings: serde_json::Value = serde_json::from_str(settings[1]).unwrap(); + assert_eq!(settings, crate::hooks::claude_settings_registration()); assert_eq!( strings(output.get("argv").unwrap()), [ diff --git a/src/eval_run.rs b/src/eval_run.rs index 53907a4d..bf1e4503 100644 --- a/src/eval_run.rs +++ b/src/eval_run.rs @@ -1709,6 +1709,12 @@ mod tests { fn canonical_eval_runtime_inventory_compiles_driver_launches() { let catalog = tempfile::tempdir().unwrap(); std::fs::create_dir_all(catalog.path().join("worker")).unwrap(); + // The Claude driver expansion registers `$ST_HOOKS` hooks, and materialization refuses an + // unverified hook set — the same contract an eval host satisfies with `st2 hooks install`. + // The root outlives the test (env stays set for the process), so it is kept, not dropped. + let hooks = tempfile::tempdir().unwrap().keep(); + crate::hooks::install_at(&hooks, false).unwrap(); + unsafe { std::env::set_var("ST_HOOKS", &hooks) }; write_eval_agent( catalog.path(), "agents/evalhost/worker/agent.kdl", diff --git a/src/hooks.rs b/src/hooks.rs index e741488e..9feda222 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -20,6 +20,7 @@ const CODEX_STOP: &[u8] = include_bytes!("../hooks/codex-stop.sh"); const CLAUDE_SESSION_START: &[u8] = include_bytes!("../hooks/claude-session-start.sh"); const CLAUDE_PRE_COMPACT: &[u8] = include_bytes!("../hooks/claude-pre-compact.sh"); const CLAUDE_STOP_FAILURE: &[u8] = include_bytes!("../hooks/claude-stop-failure.sh"); +const CLAUDE_OBSERVE: &[u8] = include_bytes!("../hooks/claude-observe.sh"); // pi has no lifecycle-hook mechanism of its own; an extension is where a pi session exposes the // same surface, so it is published and verified as part of the same immutable set. const PI_CHANNEL: &[u8] = include_bytes!("../hooks/pi-channel.ts"); @@ -28,13 +29,14 @@ const SCHEMA: u32 = 1; const RECEIPT_FILE: &str = "current.json"; const SET_MANIFEST_FILE: &str = "manifest.json"; const SETS_DIR: &str = "sets"; -const HOOKS: [(&str, &[u8]); 7] = [ +const HOOKS: [(&str, &[u8]); 8] = [ ("codex-session-start.sh", CODEX_SESSION_START), ("codex-pre-compact.sh", CODEX_PRE_COMPACT), ("codex-stop.sh", CODEX_STOP), ("claude-session-start.sh", CLAUDE_SESSION_START), ("claude-pre-compact.sh", CLAUDE_PRE_COMPACT), ("claude-stop-failure.sh", CLAUDE_STOP_FAILURE), + ("claude-observe.sh", CLAUDE_OBSERVE), ("pi-channel.ts", PI_CHANNEL), ]; static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); @@ -101,6 +103,94 @@ pub fn hookset_id() -> String { expected_manifest().hookset } +/// The canonical Claude workspace hook registration: the exact +/// `.claude/settings.local.json` payload a maintained Claude seat carries. The +/// hand-authored example declaration and `expand_claude` both resolve to this one +/// shape, so a driver-declared seat and a hand-authored seat register identical +/// hooks and there is a single place a hook event can be added. +pub fn claude_settings_registration() -> serde_json::Value { + fn observe(event: &str) -> serde_json::Value { + serde_json::json!([{ "hooks": [{ + "type": "command", + // Quoted so a whitespace-containing `$ST_HOOKS` root — a custom or XDG state + // layout — still resolves to one executable when Claude runs the command. + "command": format!("\"$ST_HOOKS/claude-observe.sh\" {event}"), + }] }]) + } + serde_json::json!({ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "hooks": { + "SessionStart": [{ "hooks": [ + { + "type": "command", + "async": true, + "asyncRewake": true, + "command": "\"$ST_HOOKS/claude-session-start.sh\"", + }, + { + "type": "command", + "command": "\"$ST_HOOKS/claude-observe.sh\" SessionStart", + }, + ] }], + "PreCompact": [{ "hooks": [{ + "type": "command", + "command": "\"$ST_HOOKS/claude-pre-compact.sh\"", + }] }], + "StopFailure": [{ "hooks": [{ + "type": "command", + "command": "\"$ST_HOOKS/claude-stop-failure.sh\"", + }] }], + "UserPromptSubmit": observe("UserPromptSubmit"), + "Stop": observe("Stop"), + "PermissionRequest": observe("PermissionRequest"), + "PreToolUse": observe("PreToolUse"), + "PostToolUse": observe("PostToolUse"), + } + }) +} + +/// Whether one rendered string refers to a file of ANY st2 hook set, structurally — used by the +/// union merge to supersede st2's own prior registrations without ever touching a foreign entry. +/// Two spellings are owned: the `$ST_HOOKS` variable at a token boundary (`$ST_HOOKS/...`, +/// `${ST_HOOKS}/...` — `$ST_HOOKS_SUFFIX` is somebody else's variable), and an expanded path +/// whose basename is a managed hook file sitting under a set-shaped directory +/// (`.../sets/sha256-.../`), which recognizes every set version under every past +/// or relocated root without consulting the current environment. Each spelling may be +/// double-quoted — the generated commands quote the executable so whitespace-containing roots +/// survive the shell — and the quoted executable token (not the first whitespace token) is +/// what is inspected. +pub(crate) fn is_managed_hook_reference(text: &str) -> bool { + let command = if let Some(quoted) = text.strip_prefix('"') { + quoted.split('"').next().unwrap_or("") + } else { + text.split_whitespace().next().unwrap_or("") + }; + if let Some(rest) = command.strip_prefix("$ST_HOOKS") { + return rest.is_empty() || rest.starts_with('/'); + } + if let Some(rest) = command.strip_prefix("${ST_HOOKS}") { + return rest.is_empty() || rest.starts_with('/'); + } + let path = std::path::Path::new(command); + let managed_basename = path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| HOOKS.iter().any(|(managed, _)| *managed == name)); + if !managed_basename { + return false; + } + let mut components = path.components().rev().skip(1); + let set_shaped = components + .next() + .and_then(|segment| segment.as_os_str().to_str()) + .is_some_and(|segment| segment.starts_with("sha256-")); + let sets_dir = components + .next() + .and_then(|segment| segment.as_os_str().to_str()) + .is_some_and(|segment| segment == SETS_DIR); + set_shaped && sets_dir +} + /// Install-owned hook root. `$ST_HOOKS` can pin a scratch or custom state layout; otherwise use /// `$XDG_STATE_HOME/st2/hooks` or `~/.local/state/st2/hooks`. pub fn hooks_root() -> Result { @@ -513,6 +603,43 @@ pub fn install_at(root: &Path, replace: bool) -> Result { mod tests { use super::*; + /// The example declaration is the hand-authored half of the one canonical Claude hook + /// registration; expansion emits the other half from [`claude_settings_registration`]. If the + /// two drift, a driver-declared and a hand-authored seat stop registering the same hooks. + #[test] + fn example_claude_declaration_registers_the_canonical_hook_settings() { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/examples/native/agent-claude.kdl" + ); + let raw = std::fs::read_to_string(path).unwrap(); + let document: kdl::KdlDocument = raw.parse().unwrap(); + let agent = document.get("agent").unwrap(); + let render = agent.children().unwrap().get("render").unwrap(); + let content = render + .children() + .unwrap() + .nodes() + .iter() + .filter(|node| node.name().value() == "json-upsert") + .find_map(|node| { + let mut entries = node + .entries() + .iter() + // Positional arguments only: properties (e.g. `arrays="union"`) are merge + // strategy, not payload. + .filter(|entry| entry.name().is_none()) + .filter_map(|entry| match entry.value() { + kdl::KdlValue::String(value) => Some(value.as_str()), + _ => None, + }); + (entries.next() == Some(".claude/settings.local.json")).then(|| entries.next())? + }) + .expect("example declares the settings.local.json upsert"); + let registered: serde_json::Value = serde_json::from_str(content).unwrap(); + assert_eq!(registered, claude_settings_registration()); + } + /// The gate that holds a pi launch until the set is verified keys on this predicate, so a /// fencepost here silently ungates every pi agent. The `driver pi-session` shape is the one /// expansion actually emits. diff --git a/src/main.rs b/src/main.rs index 11586395..0b03825d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -351,6 +351,17 @@ enum DriverCmd { #[arg(required = true, trailing_var_arg = true, allow_hyphen_values = true)] argv: Vec, }, + /// Apply one Claude hook event (payload on stdin) to observed harness state. + ClaudeObserve { + #[arg(long)] + identity: String, + /// The wrapper's runtime/task ID; the record's pty session. Defaults to the identity. + #[arg(long)] + runtime_id: Option, + /// The Claude hook event name, e.g. `Stop` or `PermissionRequest`. + #[arg(long)] + event: String, + }, /// Run pi under the session-owned presence wrapper. PiSession { #[arg(long)] @@ -1053,6 +1064,15 @@ fn main() -> Result<()> { let catalog = catalog.canonicalize().unwrap_or(catalog); st2::claude_session::run(&catalog, identity, runtime_id, argv) } + Command::Driver(DriverCmd::ClaudeObserve { + identity, + runtime_id, + event, + }) => { + let catalog = catalog_arg(None)?; + let catalog = catalog.canonicalize().unwrap_or(catalog); + st2::claude_session::run_observe(&catalog, &identity, runtime_id.as_deref(), &event) + } Command::Driver(DriverCmd::Expand { spec, agent, host }) => { let catalog = catalog_arg(None)?; driver_expand_cmd(&catalog, &spec, agent.as_deref(), host.as_deref()) diff --git a/src/materialize.rs b/src/materialize.rs index b0cd6a97..6940af5d 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -26,6 +26,7 @@ pub enum RenderOp { JsonUpsert { destination: String, content: String, + arrays: ArrayMerge, }, EnsureLine { destination: String, @@ -36,6 +37,17 @@ pub enum RenderOp { }, } +/// How a json-upsert treats an array both sides declare. `Replace` is the default and the +/// original contract; `union` appends patch elements the target lacks (exact-equality dedupe), so +/// registrations can join arrays other owners also write — user-declared entries survive and +/// re-materialization is idempotent. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum ArrayMerge { + #[default] + Replace, + Union, +} + #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct RenderPlan { pub ops: Vec, @@ -68,6 +80,7 @@ impl RenderOp { | Self::JsonUpsert { destination, content, + .. } => { references_variable(destination, variable) || references_variable(content, variable) } @@ -162,9 +175,22 @@ fn parse_render_node(node: &KdlNode, agent: &str) -> Result { serde_json::from_str::(&content).with_context(|| { format!("agent '{agent}': json-upsert content is not valid JSON") })?; + let arrays = match directive + .entries() + .iter() + .find(|entry| entry.name().is_some_and(|name| name.value() == "arrays")) + .and_then(|entry| entry.value().as_string()) + { + None | Some("replace") => ArrayMerge::Replace, + Some("union") => ArrayMerge::Union, + Some(other) => anyhow::bail!( + "agent '{agent}': json-upsert arrays=\"{other}\" (expected replace|union)" + ), + }; plan.ops.push(RenderOp::JsonUpsert { destination: destination.clone(), content, + arrays, }); } "ensure-line" => { @@ -267,10 +293,17 @@ fn resolve_driver_render_executable(plan: &mut RenderPlan, agent: &str) -> Resul let RenderOp::JsonUpsert { destination, content, + .. } = operation else { continue; }; + // The hook registration ships verbatim: its `$ST_HOOKS` references are render + // variables the materializer resolves against the verified set, not an executable + // path this binary should rewrite. + if destination == ".claude/settings.local.json" { + continue; + } anyhow::ensure!( destination == ".mcp.json", "agent '{agent}' driver expansion produced an unexpected JSON destination" @@ -306,6 +339,16 @@ fn effective_plan(root: &Path, spec: &AgentSpec, this_host: &str) -> Result Result { Ok(true) } -fn deep_merge(target: &mut serde_json::Value, patch: serde_json::Value) { +fn deep_merge( + target: &mut serde_json::Value, + patch: serde_json::Value, + arrays: ArrayMerge, + supersede_managed_hooks: bool, +) { match (target, patch) { (serde_json::Value::Object(target), serde_json::Value::Object(patch)) => { for (key, value) in patch { match target.get_mut(&key) { - Some(existing) => deep_merge(existing, value), + Some(existing) => deep_merge(existing, value, arrays, supersede_managed_hooks), None => { target.insert(key, value); } } } } + (serde_json::Value::Array(target), serde_json::Value::Array(patch)) + if arrays == ArrayMerge::Union => + { + // Exact-equality union alone would accumulate st2's own entries across hook-set + // upgrades: `$ST_HOOKS` expands content-addressed, so every upgrade renders each + // entry with a new path and the old one would be retained beside it. Supersession + // DESCENDS: only the managed nested entries (structurally st2's — a managed hook + // file under any set-shaped path, or `$ST_HOOKS` at a token boundary) the patch no + // longer states are removed, so a matcher group holding a user hook beside an st2 + // one keeps the user's; containers left with nothing but empty arrays are dropped. + target.retain_mut(|element| { + if patch.contains(element) || !supersede_managed_hooks + || !contains_owned_string(element) + { + return true; + } + keep_after_supersession(element) + }); + for element in patch { + if !target.contains(&element) { + target.push(element); + } + } + } (target, patch) => *target = patch, } } +/// Prune superseded managed entries INSIDE `element`, returning whether the element itself is +/// still worth keeping. A managed leaf (no nested arrays) never survives here: it belongs only +/// inside its canonical group, which the caller already retained by whole-element equality — a +/// managed entry nested in a USER's group would otherwise register twice. Containers are pruned +/// recursively and dropped once every nested array is empty — husks that only ever held +/// superseded registrations. +fn keep_after_supersession(element: &mut serde_json::Value) -> bool { + if !has_nested_array(element) { + return !contains_owned_string(element); + } + let mut any_array_content = false; + match element { + serde_json::Value::Array(items) => { + items.retain_mut(|item| keep_after_supersession(item)); + any_array_content = !items.is_empty(); + } + serde_json::Value::Object(map) => { + for value in map.values_mut() { + if let serde_json::Value::Array(items) = value { + items.retain_mut(|item| keep_after_supersession(item)); + if !items.is_empty() { + any_array_content = true; + } + } else if has_nested_array(value) { + if keep_after_supersession(value) { + any_array_content = true; + } + } + } + } + _ => {} + } + any_array_content +} + +fn has_nested_array(value: &serde_json::Value) -> bool { + match value { + serde_json::Value::Array(_) => true, + serde_json::Value::Object(map) => map.values().any(has_nested_array), + _ => false, + } +} + +/// Whether any string inside `value` marks it as an st2-rendered element, structurally (see +/// [`crate::hooks::is_managed_hook_reference`]). +fn contains_owned_string(value: &serde_json::Value) -> bool { + match value { + serde_json::Value::String(text) => crate::hooks::is_managed_hook_reference(text), + serde_json::Value::Array(items) => items.iter().any(contains_owned_string), + serde_json::Value::Object(map) => map.values().any(contains_owned_string), + _ => false, + } +} + fn git_exclude(workspace: &Path, line: &str) -> Result { let output = Command::new("git") .args(["-C"]) @@ -557,7 +683,7 @@ enum PreparedOp { #[derive(Debug, Clone, PartialEq)] enum RenderClaim { Replace(Vec), - JsonUpsert(serde_json::Value), + JsonUpsert(serde_json::Value, ArrayMerge), EnsureLine(String), } @@ -621,6 +747,7 @@ fn claims_for_agent( RenderOp::JsonUpsert { destination: raw_destination, content, + arrays, } => { let patch = serde_json::from_str(&expand(&content, &env)).with_context(|| { format!( @@ -630,7 +757,7 @@ fn claims_for_agent( })?; ( destination(&workspace, &raw_destination, &env)?, - RenderClaim::JsonUpsert(patch), + RenderClaim::JsonUpsert(patch, arrays), ) } RenderOp::EnsureLine { @@ -778,6 +905,7 @@ pub fn materialize_agent(root: &Path, spec: &AgentSpec, this_host: &str) -> Resu RenderOp::JsonUpsert { destination: raw_destination, content, + arrays, } => { let destination = destination(&workspace, &raw_destination, &env)?; let current = virtual_files @@ -798,7 +926,11 @@ pub fn materialize_agent(root: &Path, spec: &AgentSpec, this_host: &str) -> Resu spec.identity ) })?; - deep_merge(&mut target, patch); + // st2-hook supersession is the CANONICAL Claude registration's maintenance + // rule, not a property of union merges: an unrelated union merge must never + // delete user array elements that merely match the managed-path heuristic. + let supersede_managed_hooks = raw_destination == ".claude/settings.local.json"; + deep_merge(&mut target, patch, arrays, supersede_managed_hooks); let mut bytes = serde_json::to_vec_pretty(&target)?; bytes.push(b'\n'); let note = format!("{}: upserted {}", spec.identity, raw_destination); @@ -1050,6 +1182,8 @@ mod tests { "nested": {"right": 2, "replace": "new"}, "array": [2] }), + ArrayMerge::Replace, + false, ); assert_eq!( target, @@ -1060,4 +1194,161 @@ mod tests { }) ); } + + /// Union mode joins arrays idempotently: foreign entries survive and repeating the same + /// patch adds nothing — the contract the generated hook registration relies on. + #[test] + fn deep_merge_union_preserves_foreign_array_entries_and_is_idempotent() { + let mut target = serde_json::json!({ + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": "user-audit.sh"}]}]} + }); + let ours = serde_json::json!({ + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": "$ST_HOOKS/claude-observe.sh Stop"}]}]} + }); + deep_merge(&mut target, ours.clone(), ArrayMerge::Union, false); + deep_merge(&mut target, ours, ArrayMerge::Union, false); + assert_eq!( + target, + serde_json::json!({ + "hooks": {"Stop": [ + {"hooks": [{"type": "command", "command": "user-audit.sh"}]}, + {"hooks": [{"type": "command", "command": "$ST_HOOKS/claude-observe.sh Stop"}]} + ]} + }) + ); + } + + /// A hook-set upgrade renders every entry under a new content-addressed path. Union must + /// supersede st2's prior entries — recognizable by the hook root — rather than accumulate + /// them, while a user's entry under any other path survives every merge. + #[test] + fn union_supersedes_prior_hook_set_entries_but_never_foreign_ones() { + // The prior set lives under a RELOCATED root — recognition is structural (set-shaped + // path + managed basename), not derived from the current environment — and a MIXED + // matcher group keeps its user hook while losing only the superseded st2 entry. + let mut target = serde_json::json!({ + "hooks": {"Stop": [ + {"matcher": "Bash", "hooks": [ + {"type": "command", "command": "user-guard.sh"}, + {"type": "command", "command": "/old/root/sets/sha256-aaa/claude-observe.sh Stop"} + ]}, + {"hooks": [{"type": "command", "command": "user-audit.sh"}]}, + {"hooks": [{"type": "command", "command": "/old/root/sets/sha256-aaa/claude-observe.sh Stop"}]}, + {"hooks": [{"type": "command", "command": "/home/x/claude-observe.sh Stop"}]}, + {"hooks": [{"type": "command", "command": "$ST_HOOKS_SUFFIX/tool.sh"}]} + ]} + }); + let upgraded = serde_json::json!({ + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": "/new/root/sets/sha256-bbb/claude-observe.sh Stop"}]}]} + }); + deep_merge(&mut target, upgraded.clone(), ArrayMerge::Union, true); + deep_merge(&mut target, upgraded, ArrayMerge::Union, true); + assert_eq!( + target, + serde_json::json!({ + "hooks": {"Stop": [ + // The mixed group survives with only its user hook. + {"matcher": "Bash", "hooks": [ + {"type": "command", "command": "user-guard.sh"} + ]}, + {"hooks": [{"type": "command", "command": "user-audit.sh"}]}, + // A managed basename OUTSIDE a set-shaped path is a user's wrapper: foreign. + {"hooks": [{"type": "command", "command": "/home/x/claude-observe.sh Stop"}]}, + // `$ST_HOOKS_SUFFIX` is somebody else's variable, not ours at a boundary. + {"hooks": [{"type": "command", "command": "$ST_HOOKS_SUFFIX/tool.sh"}]}, + {"hooks": [{"type": "command", "command": "/new/root/sets/sha256-bbb/claude-observe.sh Stop"}]} + ]} + }) + ); + } + + /// W8-14: a CURRENT managed entry nested inside a USER's group is still superseded — the + /// canonical registration lives only in its own group, or Claude runs it twice. + #[test] + fn a_managed_leaf_survives_only_inside_its_canonical_group() { + let current = "/root/sets/sha256-bbb/claude-observe.sh Stop"; + let mut target = serde_json::json!({ + "hooks": {"Stop": [ + {"matcher": "Bash", "hooks": [ + {"type": "command", "command": "user-guard.sh"}, + {"type": "command", "command": current} + ]} + ]} + }); + let patch = serde_json::json!({ + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": current}]}]} + }); + deep_merge(&mut target, patch.clone(), ArrayMerge::Union, true); + deep_merge(&mut target, patch, ArrayMerge::Union, true); + assert_eq!( + target, + serde_json::json!({ + "hooks": {"Stop": [ + {"matcher": "Bash", "hooks": [{"type": "command", "command": "user-guard.sh"}]}, + {"hooks": [{"type": "command", "command": current}]} + ]} + }) + ); + } + + /// Supersession is the CANONICAL Claude registration's maintenance rule, not a property + /// of union merges: the same merge against any other destination must never delete user + /// array elements that merely match the managed-path heuristic. + #[test] + fn union_outside_the_canonical_settings_never_prunes_managed_shaped_entries() { + let mut target = serde_json::json!({ + "tools": {"audit": [ + {"command": "user-audit.sh"}, + {"command": "/old/root/sets/sha256-aaa/claude-observe.sh Stop"} + ]} + }); + let patch = serde_json::json!({ + "tools": {"audit": [{"command": "vendor-check.sh"}]} + }); + deep_merge(&mut target, patch, ArrayMerge::Union, false); + assert_eq!( + target, + serde_json::json!({ + "tools": {"audit": [ + {"command": "user-audit.sh"}, + // A managed-SHAPED path is only superseded where st2 owns the file; + // here it is somebody's data. + {"command": "/old/root/sets/sha256-aaa/claude-observe.sh Stop"}, + {"command": "vendor-check.sh"} + ]} + }) + ); + } + + /// W8-13: legacy `deliver "mcp"` seats render the same canonical hook registration the typed + /// driver does — they run under claude-session too. + #[test] + fn legacy_mcp_seats_render_the_canonical_hook_registration() { + let tmp = tempfile::tempdir().unwrap(); + let declaration = tmp.path().join("agents/h/worker/agent.kdl"); + std::fs::create_dir_all(declaration.parent().unwrap()).unwrap(); + std::fs::write( + &declaration, + r#"agent "worker" { host "h"; command "claude"; deliver "mcp"; workspace "$CATALOG" }"#, + ) + .unwrap(); + let found = crate::discover(tmp.path()); + assert!(found.errors.is_empty(), "{:?}", found.errors); + let plan = effective_plan(tmp.path(), &found.specs[0], "h").unwrap(); + let settings = plan + .ops + .iter() + .find_map(|op| match op { + RenderOp::JsonUpsert { + destination, + content, + arrays, + } if destination == ".claude/settings.local.json" => Some((content, arrays)), + _ => None, + }) + .expect("legacy mcp seats register hooks"); + assert_eq!(*settings.1, ArrayMerge::Union); + let rendered: serde_json::Value = serde_json::from_str(settings.0).unwrap(); + assert_eq!(rendered, crate::hooks::claude_settings_registration()); + } } diff --git a/src/pi_session.rs b/src/pi_session.rs index 551d253c..e7200c34 100644 --- a/src/pi_session.rs +++ b/src/pi_session.rs @@ -110,6 +110,18 @@ pub fn run( } }; install_signal_handler(); + // Terminal-only: the channel owns the live record and its heartbeat, but only this wrapper + // survives long enough to see the stop path — its pre-escalation `ended` write is the one + // that makes `Stopped(None)` observable at all. Same token as the channel, so the terminal + // record fences exactly this session's live records. + let observer = crate::provider_session::SessionObserver::terminal_only( + &agent_dir, + &identity, + "pi", + &runtime_id, + &session, + seq, + ); let outcome = run_provider_observed( "pi", &status::status_path(&agent_dir), @@ -118,6 +130,7 @@ pub fn run( status::STATUS_REFRESH, PROVIDER_POLL, &STOP, + Some(&observer), ) .with_context(|| format!("running pi driver '{runtime_id}'"))?; record_session_end(&agent_dir, &identity, &runtime_id, &session, seq, &outcome); @@ -242,6 +255,7 @@ mod tests { Duration::from_millis(10), Duration::from_millis(5), &stop, + None, ) .unwrap(); @@ -315,6 +329,7 @@ mod tests { Duration::from_secs(60), Duration::from_millis(5), &stop, + None, ) .unwrap(); match outcome { @@ -396,4 +411,43 @@ mod tests { ] ); } + + /// W6: the terminal-only observer records how the session ended but never re-stamps live + /// state — the channel owns the heartbeat — and its token makes the write this session's. + #[test] + fn the_terminal_only_observer_ends_but_never_heartbeats() { + use crate::harness_state::{self, Activity}; + let tmp = tempfile::tempdir().unwrap(); + let record = harness_state::harness_state_path(tmp.path()); + let session = harness_state::session_token(); + // Real wiring order: the wrapper's written claim first, then the channel adopts it. + let seq = harness_state::claim(tmp.path(), "h.worker", "pi", &session).unwrap(); + let mut channel = + harness_state::Writer::new(tmp.path(), "h.worker", "pi", Some("h.worker".to_string())) + .with_ownership(session.clone(), seq); + channel + .observe(harness_state::Observation::new( + Activity::Active, + harness_state::BlockedOn::None, + harness_state::InputBuffer::Unknown, + )) + .unwrap(); + let live = std::fs::read(&record).unwrap(); + + let observer = crate::provider_session::SessionObserver::terminal_only( + tmp.path(), + "h.worker", + "pi", + "h.worker", + &session, + seq, + ); + observer.heartbeat(); + assert_eq!(std::fs::read(&record).unwrap(), live, "no heartbeat"); + + observer.ended("signal 9"); + let observed = harness_state::read(&record, None).unwrap(); + assert_eq!(observed.state, Activity::Ended); + assert_eq!(observed.exit.as_deref(), Some("signal 9")); + } } diff --git a/src/provider_session.rs b/src/provider_session.rs index 44933278..09648ff3 100644 --- a/src/provider_session.rs +++ b/src/provider_session.rs @@ -7,8 +7,8 @@ //! exact child remains alive. Each harness module keeps only what is genuinely harness-specific: //! how its provider argv is assembled and what environment the harness needs to reach st2 back. -use std::os::unix::process::CommandExt as _; -use std::path::Path; +use std::os::unix::process::{CommandExt as _, ExitStatusExt as _}; +use std::path::{Path, PathBuf}; use std::process::{Child, Command, ExitStatus, Stdio}; use std::sync::atomic::{AtomicBool, Ordering}; use std::thread; @@ -16,7 +16,7 @@ use std::time::{Duration, Instant}; use anyhow::{Context as _, Result}; -use crate::status; +use crate::{harness_state, status}; pub(crate) const PROVIDER_POLL: Duration = Duration::from_millis(250); const STOP_GRACE: Duration = Duration::from_secs(5); @@ -52,9 +52,131 @@ pub(crate) enum ProviderOutcome { Stopped(Option), } +/// The observed-harness-state handle a wrapper threads through its poll loop. Every operation +/// constructs a fresh writer over the on-disk record, so the wrapper re-stamps or terminates +/// whatever state a hook process wrote in between and never clobbers a fresher observation. +pub(crate) struct SessionObserver { + agent_dir: PathBuf, + identity: String, + harness: &'static str, + pty_session: String, + session: String, + seq: u64, + /// A terminal-only observer records how the session ended but never re-stamps live state — + /// for wrappers whose heartbeat belongs to another sibling process (pi's channel). + heartbeats: bool, +} + +impl SessionObserver { + /// `pty_session` is the wrapper's runtime/task ID — the registry entry whose liveness vouches + /// for the record. The observer mints the session incarnation token and performs the WRITTEN + /// ownership claim (superseding whatever a predecessor left, fresh live records included); + /// the wrapper exports both to its sibling writer processes (hooks, a channel) so ownership + /// — coalescing, heartbeat eligibility, terminal fencing — is decided by the claim across + /// all of them. A claim that cannot be written is fatal at construction: acting without + /// ownership would silently produce a writer every record refuses. + pub(crate) fn new( + agent_dir: &Path, + identity: &str, + harness: &'static str, + pty_session: &str, + ) -> anyhow::Result { + let session = harness_state::session_token(); + let seq = harness_state::claim(agent_dir, identity, harness, &session)?; + Ok(Self { + seq, + agent_dir: agent_dir.to_path_buf(), + identity: identity.to_string(), + harness, + pty_session: pty_session.to_string(), + session, + heartbeats: true, + }) + } + + /// An observer that records only how the session ended: `heartbeat` is a no-op because a + /// sibling process owns the live record and its freshness. Adopts that sibling's token so + /// the terminal record fences exactly this session's records. + pub(crate) fn terminal_only( + agent_dir: &Path, + identity: &str, + harness: &'static str, + pty_session: &str, + session: &str, + seq: u64, + ) -> Self { + Self { + agent_dir: agent_dir.to_path_buf(), + identity: identity.to_string(), + harness, + pty_session: pty_session.to_string(), + session: session.to_string(), + seq, + heartbeats: false, + } + } + + /// The session incarnation token sibling writer processes must adopt. + pub(crate) fn session(&self) -> &str { + &self.session + } + + /// The ownership sequence this session claimed — exported beside the token. + pub(crate) fn seq(&self) -> u64 { + self.seq + } + + fn writer(&self) -> harness_state::Writer { + harness_state::Writer::new( + &self.agent_dir, + &self.identity, + self.harness, + Some(self.pty_session.clone()), + ) + .with_ownership(self.session.clone(), self.seq) + } + + /// Re-stamp whatever live state is on disk. The wrapper's evidence is the provider child it is + /// polling, so this is called only while that child is alive. + pub(crate) fn heartbeat(&self) { + if self.heartbeats { + let _ = self.writer().heartbeat(); + } + } + + /// Best-effort terminal record; observation must never turn a clean teardown into an error. + pub(crate) fn ended(&self, exit: &str) { + let _ = self.writer().ended(exit); + } + + /// The terminal record for a session whose provider never ran (or could no longer be + /// checked): a real ended record, so the claim placeholder is not the last word. + pub(crate) fn launch_error(&self) { + let _ = self.writer().observe( + harness_state::Observation::new( + harness_state::Activity::Ended, + harness_state::BlockedOn::None, + harness_state::InputBuffer::Unknown, + ) + .with_reason("launch-error") + .with_exit("exit unknown"), + ); + } +} + +fn describe_exit(exit: ExitStatus) -> String { + match (exit.code(), exit.signal()) { + (Some(code), _) => format!("exit {code}"), + (None, Some(signal)) => format!("signal {signal}"), + (None, None) => "exit unknown".to_string(), + } +} + /// Run one interactive provider in this wrapper's terminal process group, refreshing presence on /// `refresh_interval` for exactly as long as the spawned child lives. Fails on a nonzero exit; -/// wrappers that need the exit itself use [`run_provider_observed`]. +/// wrappers that need the exit itself use [`run_provider_observed`]. With an observer, the +/// terminal record lands on every exit path this process survives. +#[allow(clippy::too_many_arguments)] pub(crate) fn run_provider( provider: &str, status_path: &Path, @@ -63,6 +185,7 @@ pub(crate) fn run_provider( refresh_interval: Duration, poll: Duration, stop: &AtomicBool, + observed: Option<&SessionObserver>, ) -> Result<()> { match run_provider_observed( provider, @@ -72,14 +195,23 @@ pub(crate) fn run_provider( refresh_interval, poll, stop, + observed, )? { - ProviderOutcome::Exited(exit) => completed_provider(provider, exit), + ProviderOutcome::Exited(exit) => { + if let Some(observed) = observed { + observed.ended(&describe_exit(exit)); + } + completed_provider(provider, exit) + } ProviderOutcome::Stopped(_) => Ok(()), } } /// [`run_provider`], but reporting how the session ended instead of judging it, so a wrapper can -/// record a terminal observation before deciding what the exit means. +/// record its own terminal observation before deciding what the exit means. The stop path still +/// writes the observer's terminal record in-line, because after SIGKILL escalation no caller code +/// is guaranteed to run. +#[allow(clippy::too_many_arguments)] pub(crate) fn run_provider_observed( provider: &str, status_path: &Path, @@ -88,6 +220,7 @@ pub(crate) fn run_provider_observed( refresh_interval: Duration, poll: Duration, stop: &AtomicBool, + observed: Option<&SessionObserver>, ) -> Result { let (program, args) = argv .split_first() @@ -108,23 +241,39 @@ pub(crate) fn run_provider_observed( Ok(()) }); } - let mut child = command - .spawn() - .with_context(|| format!("starting {provider} provider {program}"))?; + // The error arms are terminal outcomes too: the claim placeholder must not stand as the + // visible state after a launch that never ran — while the ordinary nonzero-exit path keeps + // its real exit and is deliberately not covered here. + let mut child = match command.spawn() { + Ok(child) => child, + Err(error) => { + if let Some(observed) = observed { + observed.launch_error(); + } + return Err(error).with_context(|| format!("starting {provider} provider {program}")); + } + }; let mut next_refresh = Instant::now(); loop { if stop.load(Ordering::SeqCst) { - return stop_provider_group(&mut child).map(ProviderOutcome::Stopped); + return stop_provider_group(&mut child, observed).map(ProviderOutcome::Stopped); } - if let Some(exit) = child - .try_wait() - .with_context(|| format!("checking {provider} provider"))? - { - return Ok(ProviderOutcome::Exited(exit)); + match child.try_wait() { + Ok(Some(exit)) => return Ok(ProviderOutcome::Exited(exit)), + Ok(None) => {} + Err(error) => { + if let Some(observed) = observed { + observed.launch_error(); + } + return Err(error).with_context(|| format!("checking {provider} provider")); + } } let now = Instant::now(); if now >= next_refresh { let _ = status::refresh(status_path); + if let Some(observed) = observed { + observed.heartbeat(); + } next_refresh = now + refresh_interval; } thread::sleep(poll.min(next_refresh.saturating_duration_since(Instant::now()))); @@ -136,7 +285,10 @@ fn completed_provider(provider: &str, exit: ExitStatus) -> Result<()> { Ok(()) } -fn stop_provider_group(child: &mut Child) -> Result> { +fn stop_provider_group( + child: &mut Child, + observed: Option<&SessionObserver>, +) -> Result> { let process_group = unsafe { libc::getpgrp() }; anyhow::ensure!( process_group > 1, @@ -148,12 +300,52 @@ fn stop_provider_group(child: &mut Child) -> Result> { let deadline = Instant::now() + STOP_GRACE; while Instant::now() < deadline { if let Some(exit) = child.try_wait()? { + if let Some(observed) = observed { + observed.ended(&describe_exit(exit)); + } return Ok(Some(exit)); } thread::sleep(Duration::from_millis(25)); } + // The escalation SIGKILLs this wrapper's own process group, so the wrapper dies with the + // provider and nothing after the kill is guaranteed to run. The terminal record must land + // first: a liveness record that stops being written is still being read. + if let Some(observed) = observed { + observed.ended("signal 9"); + } unsafe { libc::kill(-process_group, libc::SIGKILL); } Ok(child.wait().ok()) } + +#[cfg(test)] +mod tests { + use super::*; + + /// n6: an unspawnable provider is a terminal outcome — the claim placeholder must not stand. + #[test] + fn an_unspawnable_provider_writes_a_real_terminal_record() { + use crate::harness_state::{self, Activity}; + let tmp = tempfile::tempdir().unwrap(); + let observer = + SessionObserver::new(tmp.path(), "hetz.worker", "claude", "hetz.worker").unwrap(); + let stop = AtomicBool::new(false); + let result = run_provider_observed( + "test", + &crate::status::status_path(tmp.path()), + &["/nonexistent/provider-binary".to_string()], + &[], + Duration::from_secs(60), + Duration::from_millis(5), + &stop, + Some(&observer), + ); + assert!(result.is_err()); + let record = + harness_state::read(&harness_state::harness_state_path(tmp.path()), None).unwrap(); + assert_eq!(record.state, Activity::Ended); + assert_eq!(record.exit.as_deref(), Some("exit unknown")); + assert_eq!(record.reason.as_deref(), Some("launch-error")); + } +} diff --git a/tests/driver_expansion.rs b/tests/driver_expansion.rs index ef3a830f..ee24fd80 100644 --- a/tests/driver_expansion.rs +++ b/tests/driver_expansion.rs @@ -250,6 +250,11 @@ fn claude_driver_matches_deliver_after_normalizing_the_legacy_command_namespace( ); assert_eq!(driver_task, legacy_task); + // The driver expansion registers `$ST_HOOKS` hooks, and materialization refuses an unverified + // hook set — install one in a scratch root, as `st2 hooks install` does on a real host. + let hooks = tempfile::tempdir().unwrap().keep(); + st2::hooks::install_at(&hooks, false).unwrap(); + unsafe { std::env::set_var("ST_HOOKS", &hooks) }; materialize_agent(&catalog, &legacy, "h").unwrap(); materialize_agent(&catalog, &driver, "h").unwrap(); let legacy_mcp: serde_json::Value = diff --git a/tests/fixtures/driver/claude.out.kdl b/tests/fixtures/driver/claude.out.kdl index 7cb3605c..4ffc439f 100644 --- a/tests/fixtures/driver/claude.out.kdl +++ b/tests/fixtures/driver/claude.out.kdl @@ -1,4 +1,5 @@ render { json-upsert .mcp.json "{\n \"mcpServers\": {\n \"st2\": {\n \"args\": [\n \"--catalog\",\n \"$CATALOG\",\n \"driver\",\n \"claude-mcp\",\n \"--identity\",\n \"Silber.fabric\"\n ],\n \"command\": \"st2\",\n \"type\": \"stdio\"\n }\n }\n}" + json-upsert ".claude/settings.local.json" "{\n \"$schema\": \"https://json.schemastore.org/claude-code-settings.json\",\n \"hooks\": {\n \"PermissionRequest\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" PermissionRequest\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"PostToolUse\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" PostToolUse\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"PreCompact\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-pre-compact.sh\\\"\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"PreToolUse\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" PreToolUse\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"SessionStart\": [\n {\n \"hooks\": [\n {\n \"async\": true,\n \"asyncRewake\": true,\n \"command\": \"\\\"$ST_HOOKS/claude-session-start.sh\\\"\",\n \"type\": \"command\"\n },\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" SessionStart\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"Stop\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" Stop\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"StopFailure\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-stop-failure.sh\\\"\",\n \"type\": \"command\"\n }\n ]\n }\n ],\n \"UserPromptSubmit\": [\n {\n \"hooks\": [\n {\n \"command\": \"\\\"$ST_HOOKS/claude-observe.sh\\\" UserPromptSubmit\",\n \"type\": \"command\"\n }\n ]\n }\n ]\n }\n}" arrays=union } argv st2 --catalog $CATALOG driver claude-session --identity Silber.fabric --runtime-id Silber.fabric -- claude --model opus --effort xhigh "--dangerously-load-development-channels=server:st2" --permission-mode bypassPermissions --model override "Start the assigned work."