From 9949c1eb93f1ddd63ad00eda80c6531ca0e56655 Mon Sep 17 00:00:00 2001 From: Aditya Date: Sat, 5 Sep 2026 03:08:31 -0500 Subject: [PATCH] Carry the tool line's result as colour, and hear a call start before it reports The browser's tool line no longer writes its status word beside the phrase. The result is the bullet's colour, from exactly one of activity-ok, activity-fail or activity-running on the line, with the word kept in the line's title and, for a shell call, under the verbatim command in the details. stateForStatus in web/src/activity-line.ts is the one mapping: exit 0 and passed are ok, the tools' failure vocabulary is fail, and the other tools' own success summaries stay ok, the same split the agent loop uses for succeeded. The verify line keeps its word and takes the same classes. Renderer::tool_starting is a new trait method with an empty default, called from Agent::run_tool after approval and before dispatch, so a denied call never draws as running. WebRenderer sends it as a tool_started frame with the name and phrase the tool frame will carry; the browser appends a running line on the first and settles it in place on the second. A replayed session has every result, so no replayed line is in progress, and a turn that ends with a line pending fails it with the reason in its title. Claude-Session: https://claude.ai/code/session_01PG2vuux5tMsjnZBLWJzMyp --- AGENTS.md | 13 ++- CLAUDE.md | 13 ++- docs/DECISIONS.md | 45 ++++++++ web/src/activity-line.ts | 153 ++++++++++++++++++++++--- web/src/api.ts | 15 +++ web/src/main.ts | 61 +++++++--- web/src/streamed-message.ts | 4 +- web/styles.css | 38 ++++++- web/test/activity-line.test.ts | 197 +++++++++++++++++++++++++++++---- zorp-agent/src/agent.rs | 90 ++++++++++++++- zorp-agent/src/render.rs | 10 ++ zorp-web/src/event.rs | 11 ++ zorp-web/src/renderer.rs | 41 +++++++ 13 files changed, 624 insertions(+), 67 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 34cd63e..85f4292 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -290,8 +290,17 @@ resulting artifact, deliver it in the right form. stored call's arguments, and `get_session` replays stored tool calls as `tool` entries with a status derived from the stored result in code. Recall and memory never read `tool_calls`, which is what keeps it out - of the evidence. Run `cargo test --workspace` and the three `web/` - commands whenever it changes. See `docs/DECISIONS.md` (2026-09-04). + of the evidence. The line carries its result as colour and never as a + word: exactly one of `activity-ok`, `activity-fail` or + `activity-running` sits on the `.activity-line`, the status text is in + the line's `title` and under the command in the details, and + `stateForStatus` in `activity-line.ts` is the one mapping. + `Renderer::tool_starting` fires from `Agent::run_tool`, after approval + and before `dispatch`, so a denied call never draws as running; the + browser appends a running line on the `tool_started` frame and settles + it in place on the `tool` frame with the same name. Run `cargo test + --workspace` and the three `web/` commands whenever any of it changes. + See `docs/DECISIONS.md` (2026-09-04, 2026-09-05). - `erbga/` is a standalone, zero-dependency implementation of published prior work (Rao, Janikow, Bhatia, Climer, MWAIS 2018): a genetic algorithm for graph community detection, validated against that work's diff --git a/CLAUDE.md b/CLAUDE.md index d893850..0ca5cee 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -290,8 +290,17 @@ resulting artifact, deliver it in the right form. stored call's arguments, and `get_session` replays stored tool calls as `tool` entries with a status derived from the stored result in code. Recall and memory never read `tool_calls`, which is what keeps it out - of the evidence. Run `cargo test --workspace` and the three `web/` - commands whenever it changes. See `docs/DECISIONS.md` (2026-09-04). + of the evidence. The line carries its result as colour and never as a + word: exactly one of `activity-ok`, `activity-fail` or + `activity-running` sits on the `.activity-line`, the status text is in + the line's `title` and under the command in the details, and + `stateForStatus` in `activity-line.ts` is the one mapping. + `Renderer::tool_starting` fires from `Agent::run_tool`, after approval + and before `dispatch`, so a denied call never draws as running; the + browser appends a running line on the `tool_started` frame and settles + it in place on the `tool` frame with the same name. Run `cargo test + --workspace` and the three `web/` commands whenever any of it changes. + See `docs/DECISIONS.md` (2026-09-04, 2026-09-05). - `erbga/` is a standalone, zero-dependency implementation of published prior work (Rao, Janikow, Bhatia, Climer, MWAIS 2018): a genetic algorithm for graph community detection, validated against that work's diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index ba031e5..9743f79 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -12,6 +12,51 @@ was believed at the time and not only what survived. --- +## 2026-09-05: the tool line carries its result as colour, and the browser hears a call start + +**Decision:** the browser's tool line no longer writes its status word +(`exited 0`, `timed out`, `created a.md (3 lines)`) beside the phrase. The +result is the bullet's colour, from exactly one of three classes on the +`.activity-line`: `activity-ok`, `activity-fail`, `activity-running`. The +word is the line's `title`, and for a shell call it is also under the +verbatim command in the details, so anyone who wants it has it one hover or +one click away. `stateForStatus` in `web/src/activity-line.ts` is the one +mapping. `exited 0` and `passed` are ok. A non-zero exit, `timed out`, +`cancelled`, `failed`, `error`, `denied`, `unknown tool`, a withheld +result, a patch of which nothing applied, and the subagent tool's stopped +states are failures. Everything else is ok, because the other tools +summarise a success in their own words (`a.txt (12 lines)`, `'q' (4 +matches)`, `started PID 512`) and a rule that read an unknown word as a +failure would paint a working session red. That is the split the agent +loop already uses for its own `succeeded`. The `verify` line keeps its +word, since it is not a tool line, and takes the same classes. + +The in-progress state is real rather than implied. +`Renderer::tool_starting(name, description)` is a new trait method with an +empty default, so the CLI and every test renderer are untouched. +`WebRenderer` turns it into a +`tool_started` frame carrying the same `name` and `phrase` the `tool` frame +for that call will carry, and the browser appends a running line on the +first and settles it in place on the second, matching on the name; tools +run one at a time, so one pending reference is enough. A reopened session +has every result, so a replayed line is never in progress. A turn that +ends with a line still pending fails it, with the title saying the turn +ended before the call reported. + +**Why it is sent after approval:** `tool_starting` fires from +`Agent::run_tool`, which only the `Allow` branch and the approved `Ask` +branch reach, immediately before `Registry::dispatch`. Sent before +approval, the line would pulse under the approval card while the person +was still deciding, which reads as a call that ran without permission, and +a denied call would get a running line that nothing ever settles. A denied +call never started, so it gets only its result, as before. `agent.rs` has +a test for the order and a test for the denial. + +**Ruled out:** keeping the word on the line in a smaller face (the line +was already three things wide and wrapped); marking any unknown status as +a failure (above); a second class scheme for the activity group's summary +(the group reads the three line classes and nothing else). + ## 2026-09-04: a 404 that names an upstream provider is the upstream's error and is retried **Finding:** three attempts on OpenRouter's free providers died today to diff --git a/web/src/activity-line.ts b/web/src/activity-line.ts index 149effb..3d0f292 100644 --- a/web/src/activity-line.ts +++ b/web/src/activity-line.ts @@ -2,11 +2,19 @@ * One tool call in the transcript. * * The server names a call as `run_command()` and follows it with a - * short result such as `exited 0`. This draws that as a bullet, a phrase - * saying what the command does, and the result, with the full command one - * click away under the line. A call that carries no command, `write_file` - * or `read_file`, keeps its tool name on the line, and so does a `verify` - * line, since there the name is the point. + * short result such as `exited 0`. This draws that as a bullet and a phrase + * saying what the command does, with the full command one click away under + * the line. A call that carries no command, `write_file` or `read_file`, + * keeps its tool name on the line, and so does a `verify` line, since there + * the name is the point. + * + * The result is not written on the line. It is the bullet's colour, from + * exactly one of the `LINE_STATES` classes on the line: green for a call + * that succeeded, red for one that failed, and a pulsing in-progress colour + * for a call that has started and not yet reported. The result text stays + * reachable: it is the line's `title`, and for a shell call it sits under + * the verbatim command in the details. `stateForStatus` is the one mapping + * from the server's status word to a colour. * * The phrase on the line is the model's own description of its call, given * in the call's `description` argument next to the command, drawn through @@ -30,6 +38,65 @@ export const BRIEF_MAX = 80; /** What hovering the model's phrase says it is. */ const MODEL_PHRASE_TITLE = "The model's own description of this call. Click to see the command that ran."; +/** What hovering a shell line says, after its status, when it has one. */ +const COMMAND_HINT = "Click to see the command that ran."; + +/** The title a shell line has while it is running. */ +const RUNNING_SHELL_TITLE = "Running. Click to see the command."; + +/** + * The state a line is in, carried as exactly one of these classes on the + * `.activity-line` element. The bullet takes its colour from them, and the + * activity group reads them off its lines, so they are a contract. + */ +export const LINE_STATES = ["activity-ok", "activity-fail", "activity-running"] as const; +export type LineState = (typeof LINE_STATES)[number]; + +/** + * The status words that mean a call failed. This is the vocabulary the + * agent's tools produce as `ToolOutput::summary`, and what the agent loop + * itself counts as not succeeded (`denied`, `error`, `unknown tool`), plus + * the shell's own outcomes and the subagent tool's stopped states. `verify` + * says `failed`. + */ +const FAILED = new Set([ + "timed out", + "cancelled", + "failed", + "error", + "denied", + "unknown tool", + "blocked", + "step limit", + "repeated action", + "verification failed", +]); + +/** + * The colour for a finished call's status word. `exited 0` is ok and any + * other exit code is a failure; the words in `FAILED`, and a status that + * starts as an error, a denial or a withheld result, are failures too. A + * patch of which nothing applied failed as well. + * + * Everything else is ok, including an empty status, because the other tools + * summarise a success in their own words: `a.txt (12 lines)`, + * `created b.md (3 lines)`, `'query' (4 matches)`, `started PID 512`, + * `loaded skill x`. A rule that read those as failures would paint a + * working session red. The failure vocabulary is the smaller and the more + * stable of the two, and it is the one the agent loop uses. + */ +export function stateForStatus(status: string): "activity-ok" | "activity-fail" { + const word = status.trim(); + const exited = /^exited (-?\d+)$/.exec(word); + if (exited) { + return exited[1] === "0" ? "activity-ok" : "activity-fail"; + } + if (FAILED.has(word) || /^(error|denied|withheld)\b/.test(word) || /^0\/\d+ blocks applied$/.test(word)) { + return "activity-fail"; + } + return "activity-ok"; +} + /** The command the model asked for, when the server put it in the name. */ export interface ToolCall { tool: string; @@ -280,18 +347,71 @@ function text(doc: Document, tag: string, className: string, value: string): HTM /** * The line for a tool event: `name` as the server sent it, `status` as the * result text that follows it, `phrase` as the model's own description of - * the call when the event carried one. + * the call when the event carried one. Settled on the spot, since the + * result is known. A reopened session draws every stored call this way. */ export function toolLine(doc: Document, name: string, status: string, phrase?: string | null): HTMLElement { const { tool, command } = splitCall(name); - return callLine(doc, tool, command, status, "", phrase); + const node = callLine(doc, tool, command, phrase); + settleLine(node, status); + return node; +} + +/** + * The line for a `tool_started` event: the same call, drawn in progress. + * The `tool` event that follows for it hands the node to `settleLine`. + */ +export function startedLine(doc: Document, name: string, phrase?: string | null): HTMLElement { + const { tool, command } = splitCall(name); + const node = callLine(doc, tool, command, phrase); + const line = lineOf(node); + line.classList.add("activity-running"); + line.title = node === line ? "Running" : RUNNING_SHELL_TITLE; + return node; +} + +/** + * Bring a line to the state of its result, in place. `state` is read off + * `status` unless the caller knows better: a line abandoned by the end of + * the turn has no status word of its own and is failed with a sentence. + * + * The status goes into the line's `title`, and for a shell call under the + * command in the details as well, through `textContent`; it is text the + * server derived from a tool result and it is never markup. The line ends + * up with exactly one of `LINE_STATES` on it whatever it had before. + */ +export function settleLine(node: HTMLElement, status: string, state: LineState = stateForStatus(status)): void { + const line = lineOf(node); + line.classList.remove(...LINE_STATES); + line.classList.add(state); + if (node === line) { + line.title = status; + return; + } + line.title = status ? `${status}. ${COMMAND_HINT}` : "Show the full command"; + let result = node.querySelector(".activity-result"); + if (!status) { + result?.remove(); + return; + } + if (!result) { + result = el(node.ownerDocument, "div", "activity-result"); + node.append(result); + } + result.textContent = status; +} + +/** The `.activity-line` of a node `callLine` returned: the node itself, or the summary of its details. */ +function lineOf(node: HTMLElement): HTMLElement { + return node.classList.contains("activity-line") ? node : (node.querySelector(".activity-line") as HTMLElement); } /** - * A bullet, the phrase for the command when there is one, and the status - * as one piece that never breaks in the middle. The tool name is drawn - * when there is no command to describe, and on a `verify` line, where it - * says what the line is; beside a sentence, `run_command` is noise. + * A bullet and the phrase for the command when there is one. The tool name + * is drawn when there is no command to describe, and on a `verify` line, + * where it says what the line is; beside a sentence, `run_command` is + * noise. `word` is a status word written on the line, which only `verify` + * does: a tool line carries its result as colour, through `settleLine`. * * The phrase is the model's own, clamped, when it gave one, and then the * span says so in its class and its title; otherwise it is computed from @@ -301,15 +421,15 @@ export function toolLine(doc: Document, name: string, status: string, phrase?: s * With a command the line is the summary of a closed `details` element * whose body is the full command, verbatim. Without one there is nothing * more to show, so the line is a plain block and no empty container is - * left under it. + * left under it. The node comes back in no state; `settleLine` and + * `startedLine` put one on it. */ export function callLine( doc: Document, tool: string, command: string | null, - status: string, - statusClass = "", phrase?: string | null, + word?: string, ): HTMLElement { const spans: HTMLElement[] = []; if (command === null || !SHELL_TOOLS.has(tool)) { @@ -325,9 +445,8 @@ export function callLine( spans.push(brief); } } - if (status) { - const className = statusClass ? `activity-status ${statusClass}` : "activity-status"; - spans.push(text(doc, "span", className, status)); + if (word) { + spans.push(text(doc, "span", "activity-status", word)); } const column = el(doc, "span", "activity-text"); column.append(...spans.flatMap((span, index) => (index ? [" ", span] : [span]))); diff --git a/web/src/api.ts b/web/src/api.ts index 35ef621..2464765 100644 --- a/web/src/api.ts +++ b/web/src/api.ts @@ -224,6 +224,19 @@ export interface ToolEvent { phrase?: string; } +/** + * A tool call is about to run. Sent after approval, where the policy asked + * for one, and before the tool executes. The `tool` event for the same call + * follows with its result and carries the same `name` and `phrase`. + */ +export interface ToolStartedEvent { + seq: number; + type: "tool_started"; + name: string; + /** The model's own description of the call, when it gave one; model-authored, display only. */ + phrase?: string; +} + /** A verification command ran and either passed or failed. */ export interface VerifyEvent { seq: number; @@ -524,6 +537,7 @@ export interface InvestigateDoneEvent { export type ZorpEvent = | WorkingEvent | WorkingDoneEvent + | ToolStartedEvent | ToolEvent | VerifyEvent | NoticeEvent @@ -1234,6 +1248,7 @@ export function streamEvents( const EVENT_TYPES_BY_NAME: Record = { working: true, working_done: true, + tool_started: true, tool: true, verify: true, notice: true, diff --git a/web/src/main.ts b/web/src/main.ts index 00f083a..668e1f5 100644 --- a/web/src/main.ts +++ b/web/src/main.ts @@ -47,7 +47,7 @@ import { } from "./onboarding"; import { coerceHits, renderNotice, renderResults, summarize } from "./conversation-search"; import { coerceCitations, renderMemoryNote } from "./memory-note"; -import { callLine, toolLine } from "./activity-line"; +import { callLine, settleLine, startedLine, toolLine } from "./activity-line"; import { needsText, producedSince, @@ -295,6 +295,13 @@ let workingDepth = 0; let lastSeq = -1; let sessions: SessionSummary[] = []; let activityGroup: HTMLElement | null = null; +/** + * The line for the call that has started and not yet reported. The agent + * runs tools one at a time, so there is at most one. `name` is the server's + * name for the call, and the `tool` event for it carries the same one, + * which is how the result finds its line. + */ +let pendingTool: { name: string; node: HTMLElement } | null = null; let spinnerTimer: number | null = null; let spinnerFrame = 0; const pendingApprovals = new Map(); @@ -1237,8 +1244,22 @@ function applyEvent(event: ZorpEvent): void { updateWorking(); break; + case "tool_started": + // Two starts with no result between them cannot happen while tools run + // one at a time; if it ever does, the first must not spin forever. + abandonPendingTool("This call never reported a result"); + pendingTool = { name: event.name, node: startedLine(document, event.name, event.phrase) }; + appendActivity(pendingTool.node); + break; + case "tool": - appendActivity(activityLine(event.name, event.summary, event.phrase)); + if (pendingTool && pendingTool.name === event.name) { + settleLine(pendingTool.node, event.summary); + pendingTool = null; + } else { + abandonPendingTool("This call never reported a result"); + appendActivity(activityLine(event.name, event.summary, event.phrase)); + } // A tool ran, so the workspace may have changed. The name and summary // are not read for a path: what got written is a question for the // directory, not for the tool that claims to have written it. @@ -1383,6 +1404,10 @@ function applyEvent(event: ZorpEvent): void { function finishTurn(): void { setTurnRunning(false); + // A call that started and never reported must not go on spinning once the + // turn is over. `done` follows `error` and `stopped` alike, so this one + // place covers every ending. + abandonPendingTool("The turn ended before this call reported"); workingDepth = 0; updateWorking(); // Settled with the reason, so a card left open by a stop does not claim it @@ -1542,25 +1567,30 @@ function answerControls(text: string): HTMLElement { } /** - * The CLI's shape: a bullet, the tool name, then the result. The line and - * the phrase on it are built in `src/activity-line.ts`: the model's own - * description of the call when it gave one, else a phrase computed from - * the command, and the full command sits under the line for a click. The - * approval card is deliberately not this: what a person approves is shown - * whole. + * The CLI's shape, less the result word: a bullet and the phrase, with the + * result as the bullet's colour. The line is built in `src/activity-line.ts`: + * the model's own description of the call when it gave one, else a phrase + * computed from the command, the full command under the line for a click, + * and the status word in the title and under the command. The approval + * card is deliberately not this: what a person approves is shown whole. */ function activityLine(name: string, summary: string, phrase?: string): HTMLElement { return toolLine(document, name, summary, phrase); } +/** Settle the pending line, if there is one, as failed, with `why` as its status. */ +function abandonPendingTool(why: string): void { + if (!pendingTool) return; + settleLine(pendingTool.node, why, "activity-fail"); + pendingTool = null; +} + +/** Not a tool line, so it keeps its word; the colour classes are the same. */ function verifyLine(command: string, passed: boolean): HTMLElement { - return callLine( - document, - "verify", - command, - passed ? "passed" : "failed", - passed ? "activity-pass" : "activity-fail", - ); + const word = passed ? "passed" : "failed"; + const node = callLine(document, "verify", command, null, word); + settleLine(node, word); + return node; } function noticeLine(text: string): HTMLElement { @@ -2121,6 +2151,7 @@ function resetTranscript(): void { pendingApprovals.clear(); dom.transcript.replaceChildren(); activityGroup = null; + pendingTool = null; setTurnRunning(false); workingDepth = 0; updateWorking(); diff --git a/web/src/streamed-message.ts b/web/src/streamed-message.ts index e134855..c2c550e 100644 --- a/web/src/streamed-message.ts +++ b/web/src/streamed-message.ts @@ -53,7 +53,9 @@ export function endsStreamedMessage(type: ZorpEventType): boolean { // this one and that is exactly the case this line covers. case "session_title": return false; - // Anything a reader can see. + // Anything a reader can see. A call starting puts its line on the page + // before the result does, so it is the boundary `tool` used to be. + case "tool_started": case "tool": case "verify": case "notice": diff --git a/web/styles.css b/web/styles.css index 8ebc8f5..7c9dfcd 100644 --- a/web/styles.css +++ b/web/styles.css @@ -1011,6 +1011,32 @@ button { flex: none; } +/* A tool line carries its result as the bullet's colour, from exactly one + of three classes on the line; the word is in the line's title and, for a + shell call, under the command in the details. A verify line keeps its + word, and the word takes the same colour. */ +.activity-ok > .activity-bullet, +.activity-ok .activity-status { + color: var(--ok); +} + +.activity-fail > .activity-bullet, +.activity-fail .activity-status { + color: var(--danger); +} + +.activity-running > .activity-bullet { + color: var(--warn); + animation: pulse 1.4s ease-in-out infinite; +} + +/* Someone who does not want motion gets the colour and no pulse. */ +@media (prefers-reduced-motion: reduce) { + .activity-running > .activity-bullet { + animation: none; + } +} + /* The text beside the bullet is one flex item in inline flow, so a long command wraps under itself inside the column and the status moves to the next line as one piece. The name used to be its own item that never @@ -1078,12 +1104,12 @@ button { overflow-y: auto; } -.activity-pass { - color: var(--ok); -} - -.activity-fail { - color: var(--danger); +/* The result word, under the command, once the details are open. */ +.activity-result { + margin: -2px 0 8px 24px; + font-family: var(--mono); + font-size: 11.5px; + color: var(--text-faint); } .activity-notice .activity-summary { diff --git a/web/test/activity-line.test.ts b/web/test/activity-line.test.ts index c383e27..101d9fd 100644 --- a/web/test/activity-line.test.ts +++ b/web/test/activity-line.test.ts @@ -18,7 +18,18 @@ import assert from "node:assert/strict"; import { readFileSync } from "node:fs"; import { JSDOM } from "jsdom"; -import { BRIEF_MAX, callLine, clampPhrase, describeCommand, splitCall, toolLine } from "../src/activity-line.ts"; +import { + BRIEF_MAX, + LINE_STATES, + callLine, + clampPhrase, + describeCommand, + settleLine, + splitCall, + startedLine, + stateForStatus, + toolLine, +} from "../src/activity-line.ts"; import type { Message } from "../src/api.ts"; const dom = new JSDOM(""); @@ -38,6 +49,16 @@ function line(name: string, status = "exited 0"): HTMLElement { return node; } +/** The `.activity-line` of what `toolLine` returned: the node, or the summary of its details. */ +function lineOf(node: HTMLElement): HTMLElement { + return node.classList.contains("activity-line") ? node : (node.querySelector(".activity-line") as HTMLElement); +} + +/** The state classes on the line, which the contract says is exactly one. */ +function states(node: HTMLElement): string[] { + return LINE_STATES.filter((state) => lineOf(node).classList.contains(state)); +} + /* injection */ test("a script tag as the command is text on the line and in the details", () => { @@ -71,23 +92,37 @@ test("the full command is byte identical to the input", () => { assert.equal(node.querySelector(".activity-full code")?.textContent, command); }); -test("a status with markup in it is text", () => { +test("a status with markup in it is text, in the title and under the command", () => { const node = line("run_command(ls)", "exited 0"); assert.equal(node.querySelectorAll("b").length, 0); - assert.equal(node.querySelector(".activity-status")?.textContent, "exited 0"); + assert.equal(lineOf(node).title, "exited 0. Click to see the command that ran."); + assert.equal(node.querySelector(".activity-result")?.textContent, "exited 0"); + const bare = line("write_file", ""); + assert.equal(bare.querySelectorAll("img").length, 0); + assert.equal(bare.title, ""); }); /* structure */ -test("the phrase and the status are separate spans in one text column, with no tool name", () => { +test("the phrase is the only text on the line: no tool name and no status word", () => { const node = line("run_command(pandoc in.html -o out.pdf)"); const column = node.querySelector(".activity-line > .activity-text"); assert.ok(column, "the text sits in one column beside the bullet"); assert.equal(column?.querySelectorAll(".activity-name").length, 0); assert.equal(column?.querySelector(".activity-brief")?.textContent, "Converting in.html"); - assert.equal(column?.querySelectorAll(".activity-status").length, 1); - assert.equal(column?.querySelector(".activity-status")?.textContent, "exited 0"); - assert.equal(node.textContent, "●Converting in.html exited 0pandoc in.html -o out.pdf"); + assert.equal(column?.querySelectorAll(".activity-status").length, 0); + assert.equal(lineOf(node).textContent, "●Converting in.html"); + assert.equal(node.textContent, "●Converting in.htmlpandoc in.html -o out.pdfexited 0"); +}); + +test("the status is absent from the line's text and present in its title", () => { + const shell = line("run_command(cargo test)", "exited 101"); + assert.ok(!lineOf(shell).textContent?.includes("exited 101")); + assert.equal(lineOf(shell).title, "exited 101. Click to see the command that ran."); + assert.equal(shell.querySelector(".activity-result")?.textContent, "exited 101"); + const bare = line("read_file", "a.txt (12 lines)"); + assert.equal(bare.textContent, "●read_file"); + assert.equal(bare.title, "a.txt (12 lines)"); }); test("a background process reads as its phrase too", () => { @@ -107,7 +142,7 @@ test("a call with a command is a closed details whose body is the full command", assert.equal(node.tagName, "DETAILS"); assert.equal((node as HTMLDetailsElement).open, false); assert.equal(node.firstElementChild?.tagName, "SUMMARY"); - assert.equal((node.firstElementChild as HTMLElement).title, "Show the full command"); + assert.equal((node.firstElementChild as HTMLElement).title, "exited 0. Click to see the command that ran."); assert.equal(node.querySelector("pre.activity-full code")?.textContent, "pandoc in.html -o out.pdf"); }); @@ -116,24 +151,142 @@ test("a call without a command is a plain line that keeps its tool name", () => assert.equal(node.tagName, "DIV"); assert.equal(node.querySelectorAll("details, pre").length, 0); assert.equal(node.querySelector(".activity-name")?.textContent, "write_file"); - assert.equal(node.querySelector(".activity-status")?.textContent, "created a.html (12 lines)"); + assert.equal(node.querySelectorAll(".activity-status").length, 0); + assert.equal(node.title, "created a.html (12 lines)"); + assert.deepEqual(states(node), ["activity-ok"]); }); -test("an empty status puts no empty span on the page", () => { +test("an empty status leaves the line with no title, no result, and the ok colour", () => { const node = line("read_file", ""); - assert.equal(node.querySelectorAll(".activity-status").length, 0); + assert.equal(node.querySelectorAll(".activity-status, .activity-result").length, 0); assert.equal(node.querySelectorAll("pre").length, 0); assert.equal(node.textContent, "●read_file"); + assert.equal(node.title, ""); + assert.deepEqual(states(node), ["activity-ok"]); }); -test("a verify line keeps its name and carries its verdict class on the status", () => { - const node = callLine(doc as unknown as Document, "verify", "cargo test", "failed", "activity-fail"); +test("a verify line keeps its name and its word, and carries the verdict as the line's colour", () => { + const node = callLine(doc as unknown as Document, "verify", "cargo test", null, "failed"); + settleLine(node, "failed"); assert.equal(node.querySelector(".activity-name")?.textContent, "verify"); assert.equal(node.querySelector(".activity-brief")?.textContent, "Running tests"); - const status = node.querySelector(".activity-status"); - assert.ok(status?.classList.contains("activity-fail")); - assert.equal(status?.textContent, "failed"); + assert.equal(node.querySelector(".activity-status")?.textContent, "failed"); + assert.deepEqual(states(node), ["activity-fail"]); assert.equal(node.querySelector(".activity-full code")?.textContent, "cargo test"); + const passed = callLine(doc as unknown as Document, "verify", "cargo test", null, "passed"); + settleLine(passed, "passed"); + assert.deepEqual(states(passed), ["activity-ok"]); +}); + +/* the colour */ + +test("the status word maps to ok or fail, and the other tools' summaries are not failures", () => { + const ok = [ + "exited 0", + "passed", + "", + "finished", + "a.txt (12 lines)", + "created a.html (12 lines)", + "web/src (9 entries)", + "'briefCommand' (4 matches)", + "'zorp' (3 results)", + "started PID 512", + "killed PID 512", + "listed processes", + "loaded skill pdf", + "2 changed files", + "diff (40 lines)", + "3/3 blocks applied", + "subagent finished", + "mcp tool result", + "ok", + ]; + for (const status of ok) { + assert.equal(stateForStatus(status), "activity-ok", JSON.stringify(status)); + } + const fail = [ + "exited 1", + "exited 101", + "exited -1", + "timed out", + "cancelled", + "failed", + "error", + "denied", + "unknown tool", + "withheld: turn tool output budget", + "error: no such tool", + "denied: approval required", + "blocked", + "step limit", + "repeated action", + "verification failed", + "0/3 blocks applied", + ]; + for (const status of fail) { + assert.equal(stateForStatus(status), "activity-fail", JSON.stringify(status)); + } +}); + +test("a finished line carries exactly one state class, read off its status", () => { + assert.deepEqual(states(line("run_command(ls)", "exited 0")), ["activity-ok"]); + assert.deepEqual(states(line("run_command(ls)", "exited 2")), ["activity-fail"]); + assert.deepEqual(states(line("run_command(sleep 99)", "timed out")), ["activity-fail"]); + assert.deepEqual(states(line("write_file", "denied")), ["activity-fail"]); +}); + +test("the bullet takes its colour from the state, and the pulse stops for reduced motion", () => { + assert.match(css, /\.activity-ok > \.activity-bullet[^{]*\{[^}]*var\(--ok\)/); + assert.match(css, /\.activity-fail > \.activity-bullet[^{]*\{[^}]*var\(--danger\)/); + assert.match(css, /\n\.activity-running > \.activity-bullet\s*\{[^}]*animation:\s*pulse/); + assert.match(css, /prefers-reduced-motion: reduce\)\s*\{\s*\.activity-running > \.activity-bullet\s*\{\s*animation:\s*none/); + assert.equal(rule(".activity-pass"), "", "the old word class is gone"); + assert.equal(rule(".activity-fail"), "", "the state class colours the bullet, not the whole line"); +}); + +/* a call in progress */ + +test("a started line is in progress until the matching result settles it in place", () => { + const node = startedLine(doc as unknown as Document, "run_command(cargo test)", "Running the tests"); + doc.body.append(node); + const summary = lineOf(node); + assert.deepEqual(states(node), ["activity-running"]); + assert.equal(summary.title, "Running. Click to see the command."); + assert.equal(summary.querySelector(".activity-brief")?.textContent, "Running the tests"); + assert.equal(node.querySelectorAll(".activity-status, .activity-result").length, 0); + assert.equal(node.querySelector(".activity-full code")?.textContent, "cargo test"); + + settleLine(node, "exited 101"); + assert.equal(lineOf(node), summary, "the same line, not a second one"); + assert.deepEqual(states(node), ["activity-fail"]); + assert.equal(summary.title, "exited 101. Click to see the command that ran."); + assert.equal(node.querySelector(".activity-result")?.textContent, "exited 101"); + assert.equal(node.querySelectorAll(".activity-result").length, 1); + + settleLine(node, "exited 0"); + assert.deepEqual(states(node), ["activity-ok"]); + assert.equal(node.querySelectorAll(".activity-result").length, 1, "the result is updated, not appended"); + assert.equal(node.querySelector(".activity-result")?.textContent, "exited 0"); +}); + +test("a started line without a command is in progress too, and settles to its status", () => { + const node = startedLine(doc as unknown as Document, "write_file"); + assert.equal(node.tagName, "DIV"); + assert.deepEqual(states(node), ["activity-running"]); + assert.equal(node.title, "Running"); + settleLine(node, "created a.html (12 lines)"); + assert.deepEqual(states(node), ["activity-ok"]); + assert.equal(node.title, "created a.html (12 lines)"); + assert.equal(node.textContent, "●write_file"); +}); + +test("a line the turn abandoned is failed, with the reason in its title", () => { + const node = startedLine(doc as unknown as Document, "run_command(sleep 99)"); + settleLine(node, "The turn ended before this call reported", "activity-fail"); + assert.deepEqual(states(node), ["activity-fail"]); + assert.equal(lineOf(node).title, "The turn ended before this call reported. Click to see the command that ran."); + assert.equal(node.querySelector(".activity-result")?.textContent, "The turn ended before this call reported"); }); /* splitting the name */ @@ -231,7 +384,7 @@ test("a phrase the model gave is drawn as model text, with the command still who assert.ok(brief.classList.contains("activity-brief-model")); assert.equal(brief.title, MODEL_TITLE); assert.equal(node.querySelectorAll(".activity-name").length, 0); - assert.equal(node.querySelector(".activity-status")?.textContent, "exited 0"); + assert.equal(lineOf(node).title, "exited 0. Click to see the command that ran."); assert.equal(node.querySelector(".activity-full code")?.textContent, command); assert.match(rule(".activity-brief-model"), /font-style:\s*italic/); }); @@ -291,7 +444,8 @@ test("a call without a command ignores the phrase and renders as before", () => assert.equal(node.tagName, "DIV"); assert.equal(node.querySelectorAll(".activity-brief, details, pre").length, 0); assert.equal(node.querySelector(".activity-name")?.textContent, "write_file"); - assert.equal(node.textContent, "●write_file created a.html (12 lines)"); + assert.equal(node.textContent, "●write_file"); + assert.equal(node.title, "created a.html (12 lines)"); }); /* a reopened session */ @@ -320,7 +474,8 @@ test("a stored call with a phrase draws the model's words, with the command unde assert.equal(brief.textContent, "Listing files in web/src"); assert.ok(brief.classList.contains("activity-brief-model")); assert.equal(brief.title, MODEL_TITLE); - assert.equal(node.querySelector(".activity-status")?.textContent, "exited 0"); + assert.equal(lineOf(node).title, "exited 0. Click to see the command that ran."); + assert.deepEqual(states(node), ["activity-ok"], "a replayed line is never in progress"); assert.equal(node.querySelector(".activity-full code")?.textContent, "ls web/src"); }); @@ -329,6 +484,8 @@ test("a stored call without a phrase gets the phrase computed from its command, const brief = node.querySelector(".activity-brief") as HTMLElement; assert.equal(brief.textContent, "Listing files in web/src"); assert.ok(!brief.classList.contains("activity-brief-model")); - assert.equal(node.querySelectorAll(".activity-status").length, 0); + assert.equal(node.querySelectorAll(".activity-status, .activity-result").length, 0); + assert.equal(lineOf(node).title, "Show the full command"); + assert.deepEqual(states(node), ["activity-ok"]); assert.equal(node.querySelector(".activity-full code")?.textContent, "ls web/src"); }); diff --git a/zorp-agent/src/agent.rs b/zorp-agent/src/agent.rs index f5d57a9..306717d 100644 --- a/zorp-agent/src/agent.rs +++ b/zorp-agent/src/agent.rs @@ -517,6 +517,17 @@ impl Agent { self.message_metadata.get(index) } + /// Run a call the policy, and approval where it asked for one, has let + /// through. The renderer hears the call start first, with the name and + /// description its result will carry, so a browser can draw it as + /// running and settle that line when the result comes. Both denial + /// branches bypass this on purpose: a denied call never started. + fn run_tool(&mut self, call: &crate::model::ToolCall) -> ToolOutput { + self.renderer + .tool_starting(&call.display_name(), call.description()); + self.registry.dispatch(call, &mut self.cx) + } + fn push_message(&mut self, message: Message, metadata: MessageMetadata) { self.message_metadata .resize(self.messages.len(), MessageMetadata::default()); @@ -1076,10 +1087,8 @@ impl Agent { ) } else { match self.policy.decide(call) { - Decision::Allow => self.registry.dispatch(call, &mut self.cx), - Decision::Ask if self.approval.allows(call) => { - self.registry.dispatch(call, &mut self.cx) - } + Decision::Allow => self.run_tool(call), + Decision::Ask if self.approval.allows(call) => self.run_tool(call), Decision::Ask => ToolOutput::new("denied: approval required", "denied"), Decision::Deny(reason) => { ToolOutput::new(format!("denied: {reason}"), "denied") @@ -1668,7 +1677,17 @@ mod tests { fn tool(&mut self, name: &str, summary: &str) { self.tools.lock().unwrap().push(format!("{name}:{summary}")); } + fn tool_starting(&mut self, name: &str, description: Option<&str>) { + self.events + .lock() + .unwrap() + .push(format!("starting {name}:{}", description.unwrap_or(""))); + } fn tool_described(&mut self, name: &str, summary: &str, description: Option<&str>) { + self.events + .lock() + .unwrap() + .push(format!("tool {name}:{}", description.unwrap_or(""))); self.described .lock() .unwrap() @@ -2146,6 +2165,69 @@ mod tests { ); } + /// The browser draws a call as running from `tool_starting` and settles + /// it on `tool_described`, so the two must arrive in that order, for the + /// same call, with the same name and description. + #[test] + fn renderer_hears_a_call_starting_before_it_hears_the_result() { + let events = Arc::new(Mutex::new(Vec::new())); + let model = Scripted::new(vec![ + wants_tool_with( + "run_command", + json!({"command": "ls web/src", "description": "Listing files in web/src"}), + ), + text("done"), + ]); + let mut a = configured_agent(model, ApprovalMode::AutoApprove) + .register(Box::new(RecordingNamed { + name: "run_command", + ran: Arc::new(AtomicBool::new(false)), + })) + .with_renderer(Box::new(CaptureRenderer { + events: Arc::clone(&events), + ..Default::default() + })); + assert!(matches!(a.run("go"), Outcome::Complete(_))); + let events = events.lock().unwrap().clone(); + let position = |wanted: &str| { + events + .iter() + .position(|e| e == wanted) + .unwrap_or_else(|| panic!("{wanted:?} was never sent: {events:?}")) + }; + let starting = position("starting run_command(ls web/src):Listing files in web/src"); + let result = position("tool run_command(ls web/src):Listing files in web/src"); + assert!(starting < result, "{events:?}"); + } + + /// A call the approval gate refused never ran, so it never started: the + /// renderer hears only the result. A line drawn as running for a denied + /// call would read as a call that ran without permission. + #[test] + fn renderer_does_not_hear_a_denied_call_starting() { + let events = Arc::new(Mutex::new(Vec::new())); + let model = Scripted::new(vec![wants_tool("write_file"), text("done")]); + let mut a = configured_agent(model, ApprovalMode::NonInteractive) + .register(Box::new(RecordingNamed { + name: "write_file", + ran: Arc::new(AtomicBool::new(false)), + })) + .with_renderer(Box::new(CaptureRenderer { + events: Arc::clone(&events), + ..Default::default() + })); + assert!(matches!(a.run("hi"), Outcome::Complete(_))); + let events = events.lock().unwrap().clone(); + assert!( + !events.iter().any(|e| e.starts_with("starting ")), + "{events:?}" + ); + assert!( + events.contains(&"tool write_file:".to_string()), + "{events:?}" + ); + } + /// The description rides the shell call it belongs to and no other: a /// `read_file` reaches the renderer with `None` even when the model put a /// `description` in its arguments. diff --git a/zorp-agent/src/render.rs b/zorp-agent/src/render.rs index 92e2246..9124b49 100644 --- a/zorp-agent/src/render.rs +++ b/zorp-agent/src/render.rs @@ -322,6 +322,16 @@ pub trait Renderer: Send { let _ = description; self.tool(name, summary); } + /// A call is about to run. `name` and `description` are the values the + /// `tool_described` for the same call will carry once it has a result. + /// + /// Empty by default. The terminal renderer says nothing until a call + /// has a result, so the CLI's output is unchanged; the browser draws + /// the line in an in-progress state and settles it when the result + /// comes. The agent sends it after approval and before execution, so a + /// call the policy asked about is not drawn as running while the + /// approval card is still open, and a denied call never gets one. + fn tool_starting(&mut self, _name: &str, _description: Option<&str>) {} fn verify(&mut self, command: &str, passed: bool); fn notice(&mut self, text: &str); fn assistant(&mut self, text: &str); diff --git a/zorp-web/src/event.rs b/zorp-web/src/event.rs index 05e8d95..07875fe 100644 --- a/zorp-web/src/event.rs +++ b/zorp-web/src/event.rs @@ -109,6 +109,17 @@ pub enum EventKind { #[serde(skip_serializing_if = "Option::is_none")] phrase: Option, }, + /// A tool call is about to run: approval, where the policy asked for + /// it, has let it through, and the tool has not started. + /// + /// `name` and `phrase` are the values the `Tool` frame for the same + /// call carries once it has a result. The browser draws the line in an + /// in-progress state on this frame and settles it on that one. + ToolStarted { + name: String, + #[serde(skip_serializing_if = "Option::is_none")] + phrase: Option, + }, Verify { command: String, passed: bool, diff --git a/zorp-web/src/renderer.rs b/zorp-web/src/renderer.rs index 1a0ea5b..3c46dfd 100644 --- a/zorp-web/src/renderer.rs +++ b/zorp-web/src/renderer.rs @@ -65,6 +65,13 @@ impl Renderer for WebRenderer { self.tool_described(name, summary, None); } + fn tool_starting(&mut self, name: &str, description: Option<&str>) { + self.emit(EventKind::ToolStarted { + name: name.to_string(), + phrase: description.map(str::to_string), + }); + } + fn tool_described(&mut self, name: &str, summary: &str, description: Option<&str>) { self.emit(EventKind::Tool { name: name.to_string(), @@ -230,6 +237,40 @@ mod tests { )); } + /// A call starting is its own frame, ahead of the result, with the + /// same name and phrase the result will carry, so the browser can find + /// the pending line to settle. No key at all for a missing phrase, the + /// shape `tool` already uses. + #[test] + fn a_call_starting_is_a_tool_started_frame_ahead_of_its_result() { + let (tx, rx) = std::sync::mpsc::channel(); + let mut r = WebRenderer::new(tx); + r.tool_starting("run_command(ls web/src)", Some("Listing files in web/src")); + r.tool_described( + "run_command(ls web/src)", + "exited 0", + Some("Listing files in web/src"), + ); + r.tool_starting("read_file", None); + drop(r); + + let json: Vec = rx + .iter() + .map(|e| serde_json::to_string(&e).unwrap()) + .collect(); + assert_eq!(json.len(), 3); + assert_eq!( + json[0], + "{\"seq\":0,\"type\":\"tool_started\",\"name\":\"run_command(ls web/src)\",\ + \"phrase\":\"Listing files in web/src\"}" + ); + assert!(json[1].contains("\"type\":\"tool\""), "{}", json[1]); + assert_eq!( + json[2], + "{\"seq\":2,\"type\":\"tool_started\",\"name\":\"read_file\"}" + ); + } + /// A browser that closed its stream must not take the agent down with it. #[test] fn a_dropped_receiver_does_not_panic_the_renderer() {