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() {