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..65d42f7 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} /> </> @@ -612,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; @@ -622,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) && ( @@ -678,7 +730,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 +780,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,