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 0a6e5c4..376dd6e 100644 --- a/crates/agent-runtime/src/claude/mod.rs +++ b/crates/agent-runtime/src/claude/mod.rs @@ -613,12 +613,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); @@ -819,6 +813,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. @@ -868,6 +866,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 f7cd6b7..64ec4ae 100644 --- a/crates/agent-runtime/src/runtime.rs +++ b/crates/agent-runtime/src/runtime.rs @@ -415,6 +415,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 fdea1c8..1c82d2d 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] @@ -1170,6 +1184,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(); @@ -1181,7 +1199,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 @@ -1202,10 +1232,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 fe36edf..4179979 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -626,8 +626,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 });