From 4318ae6d7e02f83117e2518b4379cde888a968b4 Mon Sep 17 00:00:00 2001 From: QuintinBotes Date: Mon, 3 Aug 2026 00:57:35 +0200 Subject: [PATCH 1/2] Fix the permission gate, which failed open on every single tool call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running the app rather than the suite. A Thread's timeline held 106 identical lines: PreToolUse:Bash failed (exit 1) — Tervin hook: Tervin did not answer within 5s. `HookHandler::decide` was sync while `PermissionArbiter::decide` is async, and `ArbiterHandler` bridged the two with `Handle::block_on`. That is called from `serve_one`, which runs inside `tokio::spawn`, and `block_on` panics when called from within an async context. The task died, the socket closed with no reply, the client waited out its full 5s timeout and exited 1. Exit 1 is non-blocking, so the gate failed open every time, silently, while claiming in the UI to be gating the session. That is the exact failure mode this project exists to avoid. The trait is now async, so nothing bridges. The `runtime: Handle` field is gone rather than moved to a blocking pool, because the bridge was the bug. Why the suite missed it: `ArbiterHandler` is the only handler Tervin ever constructs and it was the only one with no test. Every gate test, including the live one against the real CLI, used a trivial handler that returned a decision directly and never called `block_on`. Same shape as the BlocksPanel failure the testing guide describes: the code that ships was the code never exercised. So two tests now drive the real handler through the real socket and the real client. Both assert on exit codes rather than on the decision, because exit 1 is the specific signature of "no answer" and a decision assertion would pass for a timeout. Verified by reintroducing `block_on` and confirming the new test fails with the identical message from the screenshot. Also four things that made the app hard to read, all found the same way: - A run of identical consecutive timeline events collapses to one row with a count. The information in the hundredth repeat is the number, not the text. Only consecutive ones merge, so nothing is reordered and an interleaved event breaks the run, which is asserted. - A Block's command was `pre-wrap` with `break-word` inside a flex child that can shrink to nothing, so in a narrow pane it wrapped one character per line. Now one truncated line with the full text in the tooltip. - The Plan tab interpolated a Thread's title into a sentence. Titles come from the first prompt, so it read as gibberish. The Thread is already named in the header. - Titles were 80 characters cut mid-word. Now 48, cut at a word boundary, with whitespace collapsed so a pasted prompt cannot put newlines in a title. rust 680 to 682, vitest 319 to 321. Co-Authored-By: Claude Opus 5 --- crates/agent-runtime/src/claude/hooks.rs | 171 ++++++++++++++++++++--- ui/src/components/BlocksPanel.tsx | 15 +- ui/src/components/PlanSurface.tsx | 5 +- ui/src/components/ThreadPanel.tsx | 52 ++++++- ui/src/components/surfaces.dom.test.tsx | 63 +++++++++ ui/src/lib/store.ts | 22 ++- 6 files changed, 299 insertions(+), 29 deletions(-) diff --git a/crates/agent-runtime/src/claude/hooks.rs b/crates/agent-runtime/src/claude/hooks.rs index c65f578..3d02f87 100644 --- a/crates/agent-runtime/src/claude/hooks.rs +++ b/crates/agent-runtime/src/claude/hooks.rs @@ -237,8 +237,19 @@ impl Drop for HookGate { /// /// Separate from [`PermissionArbiter`] so the gate can be tested without a rules /// engine, and so a caller can record the decision in a Thread's timeline. +/// `decide` is async because the only handler that ships consults +/// [`PermissionArbiter`], which is async. It used to be sync, and `ArbiterHandler` +/// bridged the gap with `Handle::block_on`. That is called from `serve_one`, which +/// runs inside `tokio::spawn`, and `block_on` panics when called from within an +/// async context: the task died, the socket closed with no reply, and the hook +/// client waited its full timeout before failing open. Every single tool call. +/// +/// The suite did not catch it because the only handler with a test was the trivial +/// one, which never called `block_on`. Awaiting properly removes the bridge rather +/// than moving it to another thread. +#[async_trait::async_trait] pub trait HookHandler: Send + Sync { - fn decide(&self, request: &HookRequest) -> HookDecision; + async fn decide(&self, request: &HookRequest) -> HookDecision; } /// Notified of every decision, so a caller can record it. @@ -248,7 +259,6 @@ pub type DecisionObserver = Box, thread_id: ThreadId, - runtime: tokio::runtime::Handle, /// Where decisions go to become timeline events. /// /// A refusal that only appears in a status line is not an audit trail: the @@ -261,7 +271,6 @@ impl ArbiterHandler { Self { arbiter, thread_id, - runtime: tokio::runtime::Handle::current(), observer: None, } } @@ -273,19 +282,18 @@ impl ArbiterHandler { } } +#[async_trait::async_trait] impl HookHandler for ArbiterHandler { - fn decide(&self, request: &HookRequest) -> HookDecision { - let arbiter = self.arbiter.clone(); - let thread_id = self.thread_id.clone(); - let tool = request.tool_name.clone(); - let input = request.tool_input.clone(); - let cwd = request.cwd.clone(); - - // The arbiter is async and this is called from a blocking context, so the - // work is handed back to the runtime rather than blocking on a new one. + async fn decide(&self, request: &HookRequest) -> HookDecision { let decision = self - .runtime - .block_on(async move { arbiter.decide(&thread_id, &tool, &input, &cwd).await }); + .arbiter + .decide( + &self.thread_id, + &request.tool_name, + &request.tool_input, + &request.cwd, + ) + .await; let decision = match decision { ArbiterDecision::Deny { reason } => HookDecision::Deny { @@ -404,7 +412,7 @@ async fn serve_one( } }; - let decision = handler.decide(&request); + let decision = handler.decide(&request).await; let denied = decision.is_deny(); let _ = respond(&mut stream, &decision).await; @@ -553,8 +561,9 @@ mod tests { /// A handler that denies anything containing a marker. struct Marker; + #[async_trait::async_trait] impl HookHandler for Marker { - fn decide(&self, request: &HookRequest) -> HookDecision { + async fn decide(&self, request: &HookRequest) -> HookDecision { if request.tool_input.to_string().contains("DENY-ME") { HookDecision::Deny { reason: "Denied by Tervin Rules: test policy".into(), @@ -913,10 +922,138 @@ mod tests { ); } + /// An arbiter that answers across an await point. + /// + /// The await matters. `ArbiterHandler` used to bridge async to sync with + /// `Handle::block_on`, and an arbiter that returned immediately could mask that. + /// Yielding guarantees the handler is genuinely driven as a future. + struct AsyncArbiter { + deny_tool: String, + } + + #[async_trait::async_trait] + impl PermissionArbiter for AsyncArbiter { + async fn decide( + &self, + _thread_id: &ThreadId, + tool_name: &str, + _input: &Value, + _cwd: &str, + ) -> ArbiterDecision { + tokio::task::yield_now().await; + if tool_name == self.deny_tool { + ArbiterDecision::Deny { + reason: "the rules say no".into(), + } + } else { + ArbiterDecision::Allow + } + } + } + + fn arbiter_gate_handler(thread_id: &ThreadId) -> Arc { + Arc::new(ArbiterHandler::new( + Arc::new(AsyncArbiter { + deny_tool: "Bash".into(), + }), + thread_id.clone(), + )) + } + + /// The handler that actually ships, driven through the real client and socket. + /// + /// This test did not exist, and its absence cost a release. `ArbiterHandler` is + /// the only handler Tervin ever constructs, and it was the only one with no + /// coverage: every other gate test used a trivial handler that returned a + /// decision directly. So `Handle::block_on` inside `decide`, called from a task + /// on a runtime worker, panicked in the real app and never in a test. The socket + /// closed with no reply, the hook client waited out its full timeout, and the + /// gate failed open on *every* tool call while the suite stayed green. + /// + /// Exit code 1 is the specific symptom: that is the client reporting it got no + /// answer. Asserting on 2 and 0 rather than merely "not a deny" is what makes + /// this catch a timeout rather than a wrong decision. + #[tokio::test] + async fn the_arbiter_backed_handler_answers_over_the_real_socket() { + let dir = tempfile::tempdir().unwrap(); + let thread_id = ThreadId::new(); + let gate = start_gate( + dir.path(), + &thread_id, + Path::new("/Apps/Tervin"), + arbiter_gate_handler(&thread_id), + ) + .await + .expect("the gate should start"); + 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; + assert_ne!( + code, 1, + "exit 1 means the client never got an answer: {err}" + ); + assert_eq!( + code, 2, + "a refusal must exit 2 or the tool still runs: {err}" + ); + + 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; + assert_ne!(code, 1, "exit 1 means no answer: {err}"); + assert_eq!(code, 0, "a permitted action defers, which exits 0: {err}"); + + // 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"); + } + + /// Several calls at once, because an agent does not wait its turn. + /// + /// `serve_one` spawns a task per connection specifically so a slow decision does + /// not delay the next, and nothing asserted that. It also fails if the handler + /// blocks a runtime worker, since enough concurrent blocks starve the pool. + #[tokio::test] + async fn concurrent_hook_calls_are_all_answered() { + let dir = tempfile::tempdir().unwrap(); + let thread_id = ThreadId::new(); + let gate = start_gate( + dir.path(), + &thread_id, + Path::new("/Apps/Tervin"), + arbiter_gate_handler(&thread_id), + ) + .await + .unwrap(); + let socket = gate.socket_path().to_path_buf(); + + let mut tasks = Vec::new(); + for i in 0..6 { + let socket = socket.clone(); + let tool = if i % 2 == 0 { "Bash" } else { "Read" }; + let payload = format!( + r#"{{"hook_event_name":"PreToolUse","tool_name":"{tool}","tool_input":{{"n":{i}}},"cwd":"/tmp"}}"# + ); + tasks.push(tokio::spawn(async move { + run_client_capturing(&socket, &payload).await + })); + } + + let mut codes = Vec::new(); + for task in tasks { + let (code, err) = task.await.unwrap(); + assert_ne!(code, 1, "a concurrent call went unanswered: {err}"); + codes.push(code); + } + codes.sort_unstable(); + assert_eq!(codes, vec![0, 0, 0, 2, 2, 2]); + } + /// Refuses any shell command, so the live test has something unambiguous to see. struct DenyBash; + #[async_trait::async_trait] impl HookHandler for DenyBash { - fn decide(&self, request: &HookRequest) -> HookDecision { + async fn decide(&self, request: &HookRequest) -> HookDecision { if request.tool_name == "Bash" { HookDecision::Deny { reason: "Denied by Tervin Rules: shell commands are refused in this test." diff --git a/ui/src/components/BlocksPanel.tsx b/ui/src/components/BlocksPanel.tsx index c6117ca..6e5d80e 100644 --- a/ui/src/components/BlocksPanel.tsx +++ b/ui/src/components/BlocksPanel.tsx @@ -129,14 +129,15 @@ function BlockRow({ title={block.status} style={{ transform: "translateY(-1px)" }} /> + {/* One truncated line, never wrapped. `wordBreak: break-word` with a flex + `min-width: 0` lets this shrink to almost nothing when the siblings claim + their width, and in a narrow pane the result is monospace text wrapping + one character per line. The full command is in the tooltip and in the + expanded body below, so nothing is lost by clipping here. */} {block.command || "(no command recorded)"} diff --git a/ui/src/components/PlanSurface.tsx b/ui/src/components/PlanSurface.tsx index 6904862..fe9626e 100644 --- a/ui/src/components/PlanSurface.tsx +++ b/ui/src/components/PlanSurface.tsx @@ -92,7 +92,10 @@ export function PlanSurface({ narrow }: { narrow: boolean }) { if (!proposed) { return (
- {thread.title} has not proposed a plan. + {/* Not " has not proposed a plan": a title is derived from the first + prompt, so it is arbitrary user text and reads as gibberish mid-sentence. + The Thread is already named in the header above this panel. */} + This Thread has not proposed a plan. {thread.capabilities?.plan_mode.level === "supported" ? ( <> {" "} diff --git a/ui/src/components/ThreadPanel.tsx b/ui/src/components/ThreadPanel.tsx index e4e65c3..a5bb331 100644 --- a/ui/src/components/ThreadPanel.tsx +++ b/ui/src/components/ThreadPanel.tsx @@ -135,6 +135,28 @@ export function ThreadPanel() { }); }, [thread, showReasoning]); + /** + * Collapse runs of identical consecutive events into one row with a count. + * + * A failing hook fires once per tool call, so a broken gate produced 106 + * byte-identical lines and a timeline nobody could read. The information in the + * hundredth repeat is the number, not the text. Only *consecutive* identical + * events collapse, so ordering is never rearranged and an interleaved event + * always breaks the run. + */ + const grouped = useMemo(() => { + const out: { event: (typeof visible)[number]; count: number }[] = []; + for (const event of visible) { + const last = out[out.length - 1]; + if (last && sameEvent(last.event, event)) { + last.count += 1; + } else { + out.push({ event, count: 1 }); + } + } + return out; + }, [visible]); + async function send() { const text = prompt.trim(); if (!text || busy) return; @@ -232,8 +254,8 @@ export function ThreadPanel() { </div> ) : ( <> - {visible.map((event) => ( - <TimelineRow key={event.id} event={event} /> + {grouped.map(({ event, count }) => ( + <TimelineRow key={event.id} event={event} repeated={count} /> ))} <div ref={endRef} /> </> @@ -678,7 +700,22 @@ function CapabilityStrip({ caps }: { caps: api.Capabilities }) { ); } -function TimelineRow({ event }: { event: api.TervinEvent }) { +/** + * Two consecutive events are "the same" when a reader would learn nothing from the + * second. Deliberately compares the rendered summary rather than the payload: an id + * and a timestamp always differ, and it is the visible line that becomes noise. + */ +function sameEvent(a: api.TervinEvent, b: api.TervinEvent): boolean { + return a.payload.type === b.payload.type && a.summary === b.summary; +} + +function TimelineRow({ + event, + repeated = 1, +}: { + event: api.TervinEvent; + repeated?: number; +}) { const [open, setOpen] = useState(false); const kind = event.payload.type; const risk = (event.payload as { risk?: api.RiskAssessment }).risk; @@ -713,6 +750,15 @@ function TimelineRow({ event }: { event: api.TervinEvent }) { > {event.summary} </span> + {repeated > 1 && ( + <span + className="chip tabular" + style={{ flex: "none" }} + title={`This happened ${repeated} times in a row. The timestamp shown is the first.`} + > + ×{repeated} + </span> + )} {risk && risk.level !== "low" && ( <button className={`chip tone-${risk.level === "critical" ? "red" : "amber"}`} diff --git a/ui/src/components/surfaces.dom.test.tsx b/ui/src/components/surfaces.dom.test.tsx index 99436f4..287edde 100644 --- a/ui/src/components/surfaces.dom.test.tsx +++ b/ui/src/components/surfaces.dom.test.tsx @@ -929,3 +929,66 @@ describe("ProjectInstructions", () => { expect(await findByText(/No instruction files here/)).toBeTruthy(); }); }); + +describe("a timeline with a repeating event", () => { + /** + * The case this exists for: a broken hook fires once per tool call, so the real + * timeline held 106 byte-identical lines. The information in the hundredth repeat + * is the count, not the text. + */ + function repeatingThread(): ThreadView { + const base: ThreadView = { + id: "thr_repeat", + profileId: "p1", + runtimeId: "claude-code", + title: "a thread whose hook keeps failing", + state: "executing", + events: [], + capabilities: null, + permissions: null, + info: null, + paneId: null, + }; + const events = []; + for (let i = 0; i < 30; i++) { + events.push({ + id: `e${i}`, + thread_id: base.id, + ts: new Date(Date.now() + i * 10).toISOString(), + summary: "PreToolUse:Bash failed (exit 1) — Tervin hook: Tervin did not answer within 5s.", + payload: { type: "tool.failed" }, + }); + } + // One different event in the middle, so the run must break rather than swallow it. + events.splice(15, 0, { + id: "different", + thread_id: base.id, + ts: new Date().toISOString(), + summary: "cargo test --workspace", + payload: { type: "command.completed" }, + }); + return { ...base, events: events as unknown as ThreadView["events"] }; + } + + it("collapses a run into one row with a count instead of 30 identical lines", async () => { + const t = repeatingThread(); + useWorkspace.setState({ threads: { [t.id]: t }, activeThreadId: t.id }); + const { findAllByText, queryAllByText } = render(<ThreadPanel />); + + // Two runs of 15, split by the interleaved event, so two count chips. + const chips = await findAllByText(/^×15$/); + expect(chips.length).toBe(2); + + // And the identical line is rendered twice, not thirty times. + const lines = queryAllByText(/did not answer within 5s/); + expect(lines.length).toBe(2); + }); + + it("keeps the event that interrupted the run", async () => { + // Collapsing must never drop an event: only consecutive identical ones merge. + const t = repeatingThread(); + useWorkspace.setState({ threads: { [t.id]: t }, activeThreadId: t.id }); + const { findByText } = render(<ThreadPanel />); + expect(await findByText("cargo test --workspace")).toBeTruthy(); + }); +}); diff --git a/ui/src/lib/store.ts b/ui/src/lib/store.ts index 5f9b14b..740b9db 100644 --- a/ui/src/lib/store.ts +++ b/ui/src/lib/store.ts @@ -434,6 +434,23 @@ function reportColorScheme(themeId: string): void { let tabCounter = 0; const nextTabId = () => `tab-${++tabCounter}`; +/** + * A short, readable label from arbitrary prompt text. + * + * Cuts at a word boundary rather than mid-word, and collapses whitespace so a + * pasted multi-line prompt does not become a title with newlines in it. + */ +export function summarise(text: string, max: number): string { + const flat = text.replace(/\s+/g, " ").trim(); + if (flat.length <= max) return flat; + const cut = flat.slice(0, max); + const lastSpace = cut.lastIndexOf(" "); + // Only respect the boundary if it leaves something substantial, otherwise a + // single long token would truncate to almost nothing. + const body = lastSpace > max * 0.6 ? cut.slice(0, lastSpace) : cut; + return `${body.replace(/[\s,;:.\-]+$/, "")}…`; +} + export const useWorkspace = create<WorkspaceState & WorkspaceActions>((set, get) => ({ surface: "terminal", listColumnWidth: 320, @@ -932,7 +949,10 @@ export const useWorkspace = create<WorkspaceState & WorkspaceActions>((set, get) const prompt = events.find((e) => e.payload.type === "user.prompted"); const promptText = typeof prompt?.payload.text === "string" ? prompt.payload.text : null; - const title = promptText ? promptText.slice(0, 80) : "Thread"; + // 80 characters cut mid-word produced titles like "Repo conventions and traps - + // Comments explai", which is unreadable in a narrow list and worse in a + // sentence. Shorter, and cut at a word boundary. + const title = promptText ? summarise(promptText, 48) : "Thread"; const started = events.find((e) => e.payload.type === "thread.started"); // Capabilities and permissions are left null on purpose: this Thread is not running, From 2ed8154e29b19809f982bb0926dc5e1fcce7dcd8 Mon Sep 17 00:00:00 2001 From: QuintinBotes <quintinbotes@outlook.com> Date: Mon, 3 Aug 2026 01:07:06 +0200 Subject: [PATCH 2/2] Group identical hook failures instead of listing every one The gate panel listed one line per failed hook run. A broken hook fires once per tool call, so an hour of work produced 59 byte-identical lines and a panel nobody could read. This is the same reasoning the working hooks already had: that code collapses them into "N of your hooks ran" because "four hooks ran fine" is reassurance rather than information. Failures deserve the same treatment for the same reason. Grouped by name, exit code and message, so two genuinely different failures never merge, and in first-seen order so the earliest stays at the top. Separate from the timeline grouping in the previous commit: this is a different panel, which is why fixing one left the other a wall. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- ui/src/components/ThreadPanel.tsx | 36 ++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/ui/src/components/ThreadPanel.tsx b/ui/src/components/ThreadPanel.tsx index a5bb331..65d42f7 100644 --- a/ui/src/components/ThreadPanel.tsx +++ b/ui/src/components/ThreadPanel.tsx @@ -634,6 +634,22 @@ function HandoffButton({ threadId }: { threadId: string }) { * Tervin's own gate is excluded: it reports itself in the timeline, and listing it * here would present Tervin's work as the user's configuration. */ +/** Collapse identical hook failures, keeping first-seen order. */ +function groupFailures(runs: api.HookRun[]): { run: api.HookRun; count: number }[] { + const out: { run: api.HookRun; count: number }[] = []; + for (const run of runs) { + const existing = out.find( + (g) => + g.run.name === run.name && + g.run.exit_code === run.exit_code && + g.run.message === run.message, + ); + if (existing) existing.count += 1; + else out.push({ run, count: 1 }); + } + return out; +} + function HookRuns({ runs }: { runs: api.HookRun[] }) { const theirs = runs.filter((r) => !r.is_tervin); if (theirs.length === 0) return null; @@ -644,13 +660,27 @@ function HookRuns({ runs }: { runs: api.HookRun[] }) { return ( <div className="meta col" style={{ gap: 2, marginTop: "var(--sp-1)" }}> - {failed.map((run, i) => ( - <div key={`${run.name}-${i}`} className="row" style={{ gap: "var(--sp-2)" }}> + {/* Grouped, not listed. One broken hook fires per tool call, so an hour of + work produced 59 byte-identical lines and a panel nobody could read. The + same reasoning the working hooks already got: the count is the + information, the repetition is not. Grouped by name, exit code and + message so two genuinely different failures never merge. */} + {groupFailures(failed).map(({ run, count }) => ( + <div + key={`${run.name}-${run.exit_code}-${run.message ?? ""}`} + className="row" + style={{ gap: "var(--sp-2)" }} + > <span className="dot dot-amber" /> <span className="mono">{run.name}</span> <span className="tone-amber truncate grow" title={run.message ?? undefined}> - failed (exit {run.exit_code}){run.message ? ` — ${run.message}` : ""} + failed (exit {run.exit_code}){run.message ? `: ${run.message}` : ""} </span> + {count > 1 && ( + <span className="chip tabular" style={{ flex: "none" }} title={`${count} times`}> + ×{count} + </span> + )} </div> ))} {(fine > 0 || blocked.length > 0) && (