From f3bf9695eb91d414fb355d4f5d69aa646c2270f6 Mon Sep 17 00:00:00 2001 From: Quintin Botes Date: Mon, 3 Aug 2026 09:36:43 +0200 Subject: [PATCH] Stop the gate from ending every Thread at its first tool call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate was killing every Thread it protected. An agent would state its plan, request one tool, and the Thread would report Completed having done nothing — twice in a row, on two different prompts, in under two seconds each. `defer` is Tervin's word for "no objection". It is not a word the runtime knows. The protocol accepts `allow`, `deny` and `ask`; handed anything else it ends the turn immediately and reports success. No tool result, no continuation, no error. Because this gate matches every tool call, that landed on the *first* one, so the agent stopped at its first action while the panel above it said Tervin Rules were protecting the session. The hook now says nothing at all when Rules do not object, which is how this protocol spells "no opinion". Denials are unchanged: a reason on stderr, exit 2. Established by elimination rather than inspection, because every plausible culprit had an alibi. The same prompt, profile, model and flags driven straight at the CLI worked. With a deliberately dead gate socket it worked. Driven through Tervin's own runtime with no arbiter wired, it worked. The audit trail showed the action allowed, "no policy rule matched and the action is low risk", 87ms before the Thread ended. What all the working cases had in common was a hook that printed nothing; substituting a hook that printed only the `defer` line reproduced it exactly. Why the suite missed it, and the same reason as the last gate bug: the tests that drive the real client over the real socket captured its exit code and its stderr and threw its stdout away. Every assertion about this path was correct. The fatal byte was in the stream nobody looked at. `run_client_capturing` now returns stdout and two tests assert it is empty, verified by reintroducing the `println!` and watching them fail with the exact payload from the timeline. Two things found while chasing it, both kept: - **Raw payloads were never stored.** `take_raw_stream` was defined and called from nowhere, so every event wrote a pointer to a body that was never saved: `raw_payloads` held zero rows while the channel grew unbounded for the life of each session. It is now drained alongside the events it belongs to, which is what made this bug diagnosable at all — "what did the runtime actually send?" had no answer when it was first asked. - **Every protocol line is traceable** behind `TERVIN_LOG=trace`. Needing a code change to see the stream is how a one-off becomes unreproducible. rust 691 to 692. Co-Authored-By: Claude Opus 5 --- crates/agent-runtime/src/claude/hooks.rs | 79 ++++++++++++++++++------ crates/agent-runtime/src/claude/mod.rs | 16 +++-- crates/agent-runtime/src/runtime.rs | 13 ++++ crates/tervin-app/src/commands.rs | 55 ++++++++++++++++- ui/src/components/BlocksPanel.tsx | 7 +-- ui/src/lib/api.ts | 4 +- 6 files changed, 140 insertions(+), 34 deletions(-) diff --git a/crates/agent-runtime/src/claude/hooks.rs b/crates/agent-runtime/src/claude/hooks.rs index 0c663d7..7c41643 100644 --- a/crates/agent-runtime/src/claude/hooks.rs +++ b/crates/agent-runtime/src/claude/hooks.rs @@ -13,17 +13,22 @@ //! configuration is never read, rewritten, or overridden. //! 3. Before each tool call the agent runs that command, handing it the tool name //! and arguments on stdin. -//! 4. The command asks the running Tervin, which consults Tervin Rules, and prints -//! the decision. +//! 4. The command asks the running Tervin, which consults Tervin Rules. A refusal is +//! written to stderr and exits 2. Anything else says nothing and exits 0. //! //! ## Tervin can only tighten, never loosen //! //! The protocol offers `allow`, which skips the runtime's *own* permission checks. -//! Tervin never returns it. When Rules do not object it returns `defer`, which -//! means "no opinion — carry on through the normal flow". So enabling this gate can -//! only ever add a refusal; it can never turn an action the runtime would have -//! asked about into one it performs silently. A safety feature that quietly -//! disables another safety feature is not one. +//! Tervin never returns it. When Rules do not object the hook says **nothing at all** +//! and exits 0, which is how this protocol spells "no opinion — carry on through the +//! normal flow". So enabling this gate can only ever add a refusal; it can never turn +//! an action the runtime would have asked about into one it performs silently. A +//! safety feature that quietly disables another safety feature is not one. +//! +//! Saying "no opinion" out loud is not equivalent to staying quiet. The runtime +//! accepts `allow`, `deny` and `ask` and nothing else; anything else ends the turn +//! immediately and reports success. Since this gate sees every tool call, that +//! landed on the first one and killed every Thread at its first action. //! //! ## What this gate cannot do //! @@ -83,7 +88,11 @@ pub enum HookDecision { } impl HookDecision { - /// The exact JSON the runtime expects on stdout. + /// The decision as it travels from Tervin to its own hook client. + /// + /// Not what the client prints. `defer` is Tervin's word for "no objection" and + /// is meaningless to the runtime, which would end the turn on being handed it — + /// so only a denial reaches the runtime, as a reason on stderr with exit 2. pub fn to_json(&self) -> Value { let (decision, reason) = match self { Self::Deny { reason } => ("deny", reason), @@ -518,7 +527,18 @@ pub fn run_hook_client(socket: &Path) -> i32 { return 2; } - println!("{decision}"); + // Silence is how this protocol spells "no opinion", and saying anything else + // here is not the harmless no-op it looks like. + // + // The runtime accepts `allow`, `deny` and `ask`. It does not accept `defer`, and + // on receiving one it **ends the turn on the spot** and reports success: no tool + // result, no continuation, the agent simply stops mid-task. Because Tervin gates + // every tool call, that fired on the *first* one, so every Thread died at its + // first action while the gate panel said it was protecting the session. Printing + // a decision that meant "carry on" was the thing stopping everything. + // + // Tervin only ever tightens. When Rules do not object there is nothing to say, + // and the exit code alone says it. 0 } @@ -742,7 +762,7 @@ mod tests { })) .unwrap(); - let (code, stderr) = run_client_capturing(&socket, &payload).await; + let (code, _out, stderr) = run_client_capturing(&socket, &payload).await; assert_eq!(code, 2, "a denial must exit 2 or the tool runs anyway"); assert!( stderr.contains("test policy"), @@ -768,7 +788,7 @@ mod tests { let socket = gate.socket_path().to_path_buf(); let payload = r#"{"hook_event_name":"PreToolUse","tool_name":"Read","tool_input":{"file_path":"/p/a.rs"},"cwd":"/tmp"}"#; - let (code, _) = run_client_capturing(&socket, payload).await; + let (code, _out, _err) = run_client_capturing(&socket, payload).await; assert_eq!(code, 0); assert!(gate.denials().is_empty()); } @@ -787,7 +807,7 @@ mod tests { .unwrap(); let socket = gate.socket_path().to_path_buf(); - let (code, _) = run_client_capturing(&socket, "this is not json").await; + let (code, _out, _err) = run_client_capturing(&socket, "this is not json").await; assert_eq!(code, 2, "input Tervin cannot read must not be allowed"); } @@ -797,7 +817,7 @@ mod tests { // believes actions are being checked. let dir = tempfile::tempdir().unwrap(); let missing = dir.path().join("nothing.sock"); - let (code, stderr) = run_client_capturing(&missing, "{}").await; + let (code, _out, stderr) = run_client_capturing(&missing, "{}").await; // Not 2: blocking on Tervin being down would make an unrelated crash stop // the user's work. assert_eq!(code, 1); @@ -1000,7 +1020,7 @@ mod tests { let socket = gate.socket_path().to_path_buf(); let deny = r#"{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"rm -rf /"},"cwd":"/tmp"}"#; - let (code, err) = run_client_capturing(&socket, deny).await; + let (code, _out, err) = run_client_capturing(&socket, deny).await; assert_ne!( code, 1, "exit 1 means the client never got an answer: {err}" @@ -1011,10 +1031,23 @@ mod tests { ); let allow = r#"{"hook_event_name":"PreToolUse","tool_name":"Read","tool_input":{"file_path":"/p/a.rs"},"cwd":"/tmp"}"#; - let (code, err) = run_client_capturing(&socket, allow).await; + let (code, out, err) = run_client_capturing(&socket, allow).await; assert_ne!(code, 1, "exit 1 means no answer: {err}"); assert_eq!(code, 0, "a permitted action defers, which exits 0: {err}"); + // Nothing on stdout, and this is the assertion the gate most needs. + // + // The runtime accepts `allow`, `deny` and `ask`. Handed anything else — such + // as Tervin's own word for "no objection" — it ends the turn immediately and + // reports success, so the agent stops at its first tool call having done + // nothing. Every Thread died that way while the exit code, the stderr and + // the audit trail all said the gate was working correctly, which is exactly + // why stdout is captured here at all. + assert!( + out.trim().is_empty(), + "the gate must say nothing when it does not object, got: {out:?}" + ); + // The denial is recorded, because a refusal that is not inspectable // afterwards is not an audit trail. assert_eq!(gate.denials().len(), 1, "the deny should be recorded once"); @@ -1053,8 +1086,12 @@ mod tests { let mut codes = Vec::new(); for task in tasks { - let (code, err) = task.await.unwrap(); + let (code, out, err) = task.await.unwrap(); assert_ne!(code, 1, "a concurrent call went unanswered: {err}"); + assert!( + out.trim().is_empty(), + "no call may print a decision the runtime would choke on: {out:?}" + ); codes.push(code); } codes.sort_unstable(); @@ -1079,12 +1116,17 @@ mod tests { } } - /// Run the real client against a socket, capturing its exit code and stderr. + /// Run the real client against a socket, capturing exit code, stdout and stderr. /// /// The client is deliberately a blocking, process-shaped function, so it is /// driven here the same way the runtime drives it: a child process with the /// payload on stdin. - async fn run_client_capturing(socket: &Path, payload: &str) -> (i32, String) { + /// + /// Stdout is captured and returned because it was once discarded here, and the + /// gate's worst bug lived in it: the client printed a decision the runtime does + /// not accept, which ended every Thread at its first tool call. Exit codes and + /// stderr were asserted throughout and all of them were correct. + async fn run_client_capturing(socket: &Path, payload: &str) -> (i32, String, String) { let socket = socket.to_path_buf(); let payload = payload.to_string(); tokio::task::spawn_blocking(move || { @@ -1111,6 +1153,7 @@ mod tests { let out = child.wait_with_output().expect("client did not finish"); ( out.status.code().unwrap_or(-1), + String::from_utf8_lossy(&out.stdout).to_string(), String::from_utf8_lossy(&out.stderr).to_string(), ) }) diff --git a/crates/agent-runtime/src/claude/mod.rs b/crates/agent-runtime/src/claude/mod.rs index bd20eb5..a27f771 100644 --- a/crates/agent-runtime/src/claude/mod.rs +++ b/crates/agent-runtime/src/claude/mod.rs @@ -560,12 +560,6 @@ pub struct ClaudeSession { } impl ClaudeSession { - /// Take the raw-payload stream, so the store can persist what the runtime - /// actually said behind each event. - pub fn take_raw_stream(&self) -> Option> { - self.raw_rx.lock().take() - } - async fn write_line(&self, value: &Value) -> Result<()> { if !self.shared.running.load(Ordering::SeqCst) { return Err(RuntimeError::SessionEnded); @@ -766,6 +760,10 @@ impl AgentSession for ClaudeSession { self.shared.running.load(Ordering::SeqCst) } + fn take_raw_stream(&self) -> Option> { + self.raw_rx.lock().take() + } + async fn shutdown(&self) -> Result<()> { self.shared.running.store(false, Ordering::SeqCst); // Closing stdin asks the runtime to finish; `kill_on_drop` is the backstop. @@ -815,6 +813,12 @@ async fn read_stream( continue; } + // Every protocol line, verbatim, behind `TERVIN_LOG=trace`. When a Thread + // behaves in a way the timeline cannot explain, this is the only account of + // what the runtime actually said, and needing a code change to obtain it is + // how a one-off oddity becomes unreproducible. + tracing::trace!(target: "tervin::claude::protocol", line = %trimmed); + let Ok(value) = serde_json::from_str::(trimmed) else { // Not JSON. Some builds print banners or warnings on stdout; record // it rather than treating the stream as broken. diff --git a/crates/agent-runtime/src/runtime.rs b/crates/agent-runtime/src/runtime.rs index 3eca08d..7322cb2 100644 --- a/crates/agent-runtime/src/runtime.rs +++ b/crates/agent-runtime/src/runtime.rs @@ -365,6 +365,19 @@ pub trait AgentSession: Send + Sync { /// True while the underlying process is alive. fn is_running(&self) -> bool; + /// The raw payloads behind this session's events, if the runtime keeps them. + /// + /// Handed out once, like the event stream, because there is one consumer. Events + /// already carry a pointer to what the runtime actually said; without someone + /// draining this, that pointer names a body nobody saved, and the question "what + /// did the runtime actually send?" has no answer at the moment it is asked. + /// + /// A runtime with nothing to offer returns `None` and its events simply carry no + /// pointers. + fn take_raw_stream(&self) -> Option> { + None + } + /// End the session and reap the process. async fn shutdown(&self) -> Result<()>; } diff --git a/crates/tervin-app/src/commands.rs b/crates/tervin-app/src/commands.rs index c8300a0..8fc9992 100644 --- a/crates/tervin-app/src/commands.rs +++ b/crates/tervin-app/src/commands.rs @@ -476,10 +476,24 @@ pub async fn block_get( /// Full raw output, including anything that spilled to disk. #[tauri::command] -pub async fn block_output(state: State<'_, Arc>, block_id: String) -> Result> { +/// A Block's full output, as a reader would see it. +/// +/// Escape sequences come off here rather than in the interface, with the same +/// routine the preview and the search index already use — three views of one +/// output that have to agree, and previously did not. Handing the bytes over +/// untouched put the terminal's own bookkeeping in front of the user: zsh's +/// partial-line marker and a window-title sequence rendered as `[1m[7m%[27m`, +/// which is not output they produced and not something they can act on. +pub async fn block_output(state: State<'_, Arc>, block_id: String) -> Result { let store = state.store.clone(); let id = BlockId::from_external(block_id); - blocking(move || store.read_full_output(&id).map_err(CommandError::from)).await + blocking(move || { + let bytes = store.read_full_output(&id).map_err(CommandError::from)?; + Ok(block_engine::parse::strip_ansi(&String::from_utf8_lossy( + &bytes, + ))) + }) + .await } #[tauri::command] @@ -1145,6 +1159,10 @@ pub async fn thread_start( let capabilities = launched.session.capabilities(); let permissions = launched.session.permissions(); + // The bodies behind each event's raw pointer. Taken before the session is + // handed to the registry, because it can only be taken once. + let mut raw_rx = launched.session.take_raw_stream(); + // Drain the event stream into the store and on to the UI. { let store = state.store.clone(); @@ -1156,7 +1174,19 @@ pub async fn thread_start( let thread_id = thread_id.clone(); let mut events = launched.events; tokio::spawn(async move { + // Pointer to body, for events not yet seen. The runtime sends a payload + // before the event that references it, so this is normally empty or + // holds one entry; it is drained on the way past rather than by its own + // task, which would race the event it belongs to. + let mut bodies: std::collections::HashMap = + std::collections::HashMap::new(); + while let Some(event) = events.recv().await { + if let Some(rx) = raw_rx.as_mut() { + while let Ok((pointer, body)) = rx.try_recv() { + bodies.insert(pointer, body); + } + } let _ = app.emit("thread://event", &event); // A command an agent ran is the same kind of thing as one you ran, so it @@ -1177,10 +1207,29 @@ pub async fn thread_start( }), ); } + // Save what the runtime actually said alongside the event. Without + // this the pointer on the event names nothing, and the first + // question asked of a surprising event — what did the runtime + // really send? — cannot be answered after the fact. + let body = event + .raw + .as_ref() + .and_then(|raw| bodies.remove(&raw.pointer)); + + // Anything still held belongs to an event that never arrived. Rare, + // but this map outlives every event in the session, so it is capped + // rather than left to grow. + if bodies.len() > 64 { + bodies.clear(); + } + let store = store.clone(); let event = event.clone(); // Persisting is blocking; keep it off the event loop. - let _ = tokio::task::spawn_blocking(move || store.append_event(&event, None)).await; + let _ = tokio::task::spawn_blocking(move || { + store.append_event(&event, body.as_deref()) + }) + .await; } }); } diff --git a/ui/src/components/BlocksPanel.tsx b/ui/src/components/BlocksPanel.tsx index 6e5d80e..2ef5df9 100644 --- a/ui/src/components/BlocksPanel.tsx +++ b/ui/src/components/BlocksPanel.tsx @@ -52,11 +52,8 @@ export function BlocksPanel({ failuresOnly = false }: Props) { setExpanded(block.id); if (!fullOutput[block.id]) { try { - const bytes = await api.blockOutput(block.id); - setFullOutput((prev) => ({ - ...prev, - [block.id]: new TextDecoder().decode(bytes), - })); + const text = await api.blockOutput(block.id); + setFullOutput((prev) => ({ ...prev, [block.id]: text })); } catch { // The spill file may have been cleaned up; the preview still shows. } diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index b619e4e..5a574ea 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -605,8 +605,8 @@ export const blocksQuery = (filter: BlockFilter) => export const blockGet = (blockId: string) => invoke("block_get", { blockId }); -export const blockOutput = (blockId: string) => - invoke("block_output", { blockId }).then((b) => Uint8Array.from(b)); +/** Full output, already stripped of escape sequences by the same routine as the preview. */ +export const blockOutput = (blockId: string) => invoke("block_output", { blockId }); export const blockSetBookmark = (blockId: string, bookmarked: boolean) => invoke("block_set_bookmark", { blockId, bookmarked });