From 5c2884fe2f5d66fe2ff2eeda36a5b271817c158f Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sat, 17 Oct 2026 16:54:45 -0700 Subject: [PATCH 1/2] fix(desktop): open files a script wrote; read script rows in the step's verb Two renderer bugs measured with GPT-5.5 under Code Execution. The artifact panel never offered a file a script wrote when the command or path sat in a variable (`const cmd = "printf ... > /p/notes.md"; shell({ command: cmd })`). The source scan looked a name up only inside a `${...}` template, never as the argument itself. - fileArtifactPathsFromToolCall takes the tool result. For execute_code it now reads the result meta's `biorouter/tool-calls` records, which carry the arguments each call really ran with; only calls recorded `ok` count. A record cut at the backend's 2 KB cap is read from its complete leading fields, and a cut that hides a field a rule needs marks the answer incomplete. The source scan stays as the fallback for results without records, and fills in (minus paths of failed calls) when the records are incomplete or some calls went unrecorded. - The source scan reads call objects by their top-level properties (string and comment aware, shorthand and quoted keys), resolves a value that is one string literal or a name bound to one, and leaves out names bound or assigned twice and literals followed by more expression (`"/w/" + name` used to yield "/w/"). str_replace_editor gets the same command gate as text_editor. - File-link provenance now trusts a script's `ok` write records, never its source, so an unexecuted branch still promotes nothing. Script rows read "Ran run ls -la", "Failed to run write the requested markdown file": summarizeToolGraph put "Running" in front of a step the model wrote as an imperative. The step's own opening verb now becomes the gerund toolRowLabel conjugates ("Running ls -la", "Ran ls -la", "Failed to write the requested markdown file"); a step that opens with no known verb stands in sentence case and fails as "Failed: Attempt the command", the existing non-verb form. A step named only by its tool still runs that tool. --- .../src/components/BaseChat.artifacts.test.ts | 46 ++ ui/desktop/src/components/BaseChat.tsx | 10 +- .../components/ToolCallWithResponse.test.tsx | 115 ++++- .../src/components/ToolCallWithResponse.tsx | 53 +- .../artifacts/artifactFileProvenance.test.ts | 56 +++ .../artifacts/artifactFileProvenance.ts | 9 +- .../artifacts/artifactUtils.test.ts | 217 ++++++++ .../src/components/artifacts/artifactUtils.ts | 475 +++++++++++++++--- .../src/components/toolCallCopy.test.ts | 45 ++ ui/desktop/src/components/toolCallCopy.ts | 40 +- 10 files changed, 944 insertions(+), 122 deletions(-) diff --git a/ui/desktop/src/components/BaseChat.artifacts.test.ts b/ui/desktop/src/components/BaseChat.artifacts.test.ts index cd1e9c17f..c1730dccf 100644 --- a/ui/desktop/src/components/BaseChat.artifacts.test.ts +++ b/ui/desktop/src/components/BaseChat.artifacts.test.ts @@ -86,6 +86,52 @@ const textToolResponse = (id: string, text: string, isError = false): Message => }); describe('collectArtifactsFromMessages', () => { + // Measured with GPT-5.5: a Code Execution script built its command in a + // variable, the file was written, and the panel never offered it. The + // result records the call with the arguments it really ran with. + it('offers a file a script call wrote, read off the result’s call records', () => { + const scriptResponse = (calls: unknown[], isError = false): Message => ({ + id: crypto.randomUUID(), + role: 'tool', + created: 2, + metadata: { userVisible: false, agentVisible: true }, + content: [ + { + type: 'toolResponse', + id: 'script', + toolResult: { + status: 'success', + value: { + isError, + content: [{ type: 'text', text: 'done' }], + _meta: { 'biorouter/tool-calls': calls }, + }, + }, + }, + ], + }); + const request = writeRequest('script', 'code_execution__execute_code', { + code: 'const cmd = makeCommand(); shell({ command: cmd });', + tool_graph: [{ tool: 'developer/shell', description: 'write the notes' }], + }); + const call = (status: string) => ({ + tool: 'developer__shell', + args: JSON.stringify({ command: "printf '# Notes\\n' > /work/smoke-notes.md" }), + status, + }); + + expect(collectArtifactsFromMessages([request, scriptResponse([call('ok')])], '/work')).toEqual([ + { kind: 'file', path: '/work/smoke-notes.md', title: 'smoke-notes.md' }, + ]); + // A call that failed, or a script that failed, offers nothing. + expect( + collectArtifactsFromMessages([request, scriptResponse([call('error')])], '/work') + ).toEqual([]); + expect( + collectArtifactsFromMessages([request, scriptResponse([call('ok')], true)], '/work') + ).toEqual([]); + }); + it('file-link reliability: does not substitute the launch cwd for an unloaded session', () => { const source = readFileSync(`${process.cwd()}/src/components/BaseChat.tsx`, 'utf8'); expect(source).not.toMatch( diff --git a/ui/desktop/src/components/BaseChat.tsx b/ui/desktop/src/components/BaseChat.tsx index 43bfbc670..189358a1e 100644 --- a/ui/desktop/src/components/BaseChat.tsx +++ b/ui/desktop/src/components/BaseChat.tsx @@ -784,9 +784,15 @@ export function collectArtifactsFromMessages( if (!call) continue; // A `ui://` resource carries its own preview; a written file only leaves - // its path behind, in the arguments of the call that created it. + // its path behind, in the arguments of the call that created it (for a + // Code Execution script, in the result's record of the calls it made). if (isSuccessfulToolResult(content.toolResult)) { - for (const path of fileArtifactPathsFromToolCall(call.name, call.arguments, workingDir)) { + for (const path of fileArtifactPathsFromToolCall( + call.name, + call.arguments, + workingDir, + content.toolResult + )) { if (confirmWritten(path)) continue; addArtifact({ kind: 'file', title: basenameFromPath(path), path }); } diff --git a/ui/desktop/src/components/ToolCallWithResponse.test.tsx b/ui/desktop/src/components/ToolCallWithResponse.test.tsx index 72be4a32b..5c32f704b 100644 --- a/ui/desktop/src/components/ToolCallWithResponse.test.tsx +++ b/ui/desktop/src/components/ToolCallWithResponse.test.tsx @@ -524,12 +524,12 @@ describe('summarizeToolCall', () => { ], }, }) - ).toBe('Running read the manifest → patch the UI'); + ).toBe('Reading the manifest → patch the UI'); }); // V2-R2-4: a script's row carried the bare plan ("list KBs → active KB"), the // one line in a turn with no verb and a lowercase start. - it('puts the state verb before a script plan, the way every other row reads', () => { + it('leads a script plan with a verb the state conjugates, the way every other row reads', () => { const summary = summarizeToolCall({ name: 'code_execution__execute_code', arguments: { @@ -539,10 +539,59 @@ describe('summarizeToolCall', () => { ], }, }); - expect(summary).toBe('Running list KBs → active KB'); - expect(toolRowLabel(summary, 'done')).toBe('Ran list KBs → active KB'); - expect(toolRowLabel(summary, 'failed')).toBe('Failed to run list KBs → active KB'); - expect(toolRowLabel(summary, 'stopped')).toBe('Stopped running list KBs → active KB'); + expect(summary).toBe('Listing KBs → active KB'); + expect(toolRowLabel(summary, 'done')).toBe('Listed KBs → active KB'); + expect(toolRowLabel(summary, 'failed')).toBe('Failed to list KBs → active KB'); + expect(toolRowLabel(summary, 'stopped')).toBe('Stopped listing KBs → active KB'); + }); + + // Measured with GPT-5.5, which writes its steps as imperatives: a "Running" + // put in front of the step read "Ran run ls -la …", "Failed to run write the + // requested markdown file…" and "Ran create the target directory…". + it.each([ + [ + 'run ls -la /tmp/smoke', + ['Running ls -la /tmp/smoke', 'Ran ls -la /tmp/smoke', 'Failed to run ls -la /tmp/smoke'], + ], + [ + 'write the requested markdown file', + [ + 'Writing the requested markdown file', + 'Wrote the requested markdown file', + 'Failed to write the requested markdown file', + ], + ], + [ + 'Create the target directory', + [ + 'Creating the target directory', + 'Created the target directory', + 'Failed to create the target directory', + ], + ], + ])('takes the verb of a step written as an imperative: %s', (description, expected) => { + const summary = summarizeToolCall({ + name: 'code_execution__execute_code', + arguments: { tool_graph: [{ tool: 'developer/shell', description }] }, + }); + expect([ + toolRowLabel(summary, 'running'), + toolRowLabel(summary, 'done'), + toolRowLabel(summary, 'failed'), + ]).toEqual(expected); + expect(toolRowLabel(summary, 'done')).not.toMatch(/^Ran (?:run|write|create)\b/i); + }); + + it('shows a step that opens with no known verb in its own words, failure named before it', () => { + const summary = summarizeToolCall({ + name: 'code_execution__execute_code', + arguments: { tool_graph: [{ tool: 'developer/shell', description: 'tally the counts' }] }, + }); + expect(summary).toBe('Tally the counts'); + expect(toolRowLabel(summary, 'running')).toBe('Tally the counts'); + expect(toolRowLabel(summary, 'done')).toBe('Tally the counts'); + expect(toolRowLabel(summary, 'failed')).toBe('Failed: Tally the counts'); + expect(toolRowLabel(summary, 'stopped')).toBe('Stopped: Tally the counts'); }); it('uses tool identities when graph descriptions are numbered placeholders', () => { @@ -570,7 +619,7 @@ describe('summarizeToolCall', () => { ], }, }) - ).toBe('Running read queue data'); + ).toBe('Reading queue data'); }); it.each([ @@ -601,7 +650,7 @@ describe('summarizeToolCall', () => { ], }, }) - ).toBe('Running step 1 of 3: Read the signed manifest'); + ).toBe('Step 1 of 3: Read the signed manifest'); }); it('ignores malformed graph nodes and falls back for an empty graph', () => { @@ -623,7 +672,7 @@ describe('summarizeToolCall', () => { ], }, }); - expect(label.startsWith('Running read ')).toBe(true); + expect(label.startsWith('Reading 🧬')).toBe(true); expect(Array.from(label).length).toBeLessThanOrEqual(96); expect(() => encodeURIComponent(label)).not.toThrow(); }); @@ -773,7 +822,7 @@ describe('summarizeToolCall', () => { ); fireEvent.click( - screen.getByText('Failed to run attempt the command').closest('button') as HTMLElement + screen.getByText('Failed: Attempt the command').closest('button') as HTMLElement ); const step = screen.getByText('1. Attempt the command'); @@ -784,6 +833,42 @@ describe('summarizeToolCall', () => { expect(screen.queryByText('Tool details unavailable')).not.toBeInTheDocument(); }); + // Measured with GPT-5.5: "Ran run ls -la …" and "Failed to run write the + // requested markdown file…" on a script's row. + it.each([ + ['success', 'run ls -la /tmp/smoke', 'Ran ls -la /tmp/smoke'], + ['error', 'write the requested markdown file', 'Failed to write the requested markdown file'], + ] as const)('reads a %s script row in the step’s own verb', (status, description, label) => { + const toolRequest: ToolRequestMessageContent = { + type: 'toolRequest', + id: `tool-imperative-${status}`, + toolCall: { + status: 'success', + value: { + name: 'code_execution__execute_code', + arguments: { tool_graph: [{ tool: 'developer/shell', description }] }, + }, + }, + }; + render( + + ); + expect(screen.getByText(label)).toBeInTheDocument(); + expect(screen.queryByText(/^(?:Ran|Failed to run) (?:run|write)\b/)).not.toBeInTheDocument(); + }); + it('treats a successful wrapper with missing content as an empty result', () => { const toolRequest: ToolRequestMessageContent = { type: 'toolRequest', @@ -1109,7 +1194,7 @@ describe('ToolCallWithResponse executed-call transparency', () => { // Expand the step row, then the executed-calls section. fireEvent.click( - screen.getByText(/read the manifest → list the files/).closest('button') as HTMLElement + screen.getByText(/read the manifest → list the files/i).closest('button') as HTMLElement ); expect(screen.getByText('Read manifest.json')).toBeInTheDocument(); @@ -1144,7 +1229,7 @@ describe('ToolCallWithResponse executed-call transparency', () => { ); fireEvent.click( - screen.getByText(/read the manifest → list the files/).closest('button') as HTMLElement + screen.getByText(/read the manifest → list the files/i).closest('button') as HTMLElement ); // The highlighter splits the source into token spans, so assert on the @@ -1191,7 +1276,7 @@ describe('ToolCallWithResponse executed-call transparency', () => { ); fireEvent.click( - screen.getByText(/read the manifest → list the files/).closest('button') as HTMLElement + screen.getByText(/read the manifest → list the files/i).closest('button') as HTMLElement ); // The literal markdown source is visible as text… @@ -1232,7 +1317,7 @@ describe('ToolCallWithResponse executed-call transparency', () => { ); fireEvent.click( - screen.getByText(/read the manifest → list the files/).closest('button') as HTMLElement + screen.getByText(/read the manifest → list the files/i).closest('button') as HTMLElement ); expect(screen.getByText('3 not recorded')).toBeInTheDocument(); @@ -1268,7 +1353,7 @@ describe('ToolCallWithResponse executed-call transparency', () => { ); fireEvent.click( - screen.getByText(/read the manifest → list the files/).closest('button') as HTMLElement + screen.getByText(/read the manifest → list the files/i).closest('button') as HTMLElement ); expect(screen.getByText('2 not recorded')).toBeInTheDocument(); expect( diff --git a/ui/desktop/src/components/ToolCallWithResponse.tsx b/ui/desktop/src/components/ToolCallWithResponse.tsx index 76e428d5a..f7d627ba2 100644 --- a/ui/desktop/src/components/ToolCallWithResponse.tsx +++ b/ui/desktop/src/components/ToolCallWithResponse.tsx @@ -39,6 +39,7 @@ import { inSentence, sentenceStart, spellServerName, + stepActionPhrase, toolActionPhrase, toolCallNameWords, toolNamePhrase, @@ -631,38 +632,48 @@ const lineValue = (value: unknown): string => { .trimEnd()}…`; }; +/** A step whose description says nothing ("Step 1 of 3", "Update #2") is named by its tool. */ +const isPlaceholderStep = (text: string): boolean => + text === 'No description was provided.' || + /^(?:tool(?: call)?|step|update|updating|task|operation)(?:\s+(?:tool(?: call)?|step|update|updating|task|operation))?\s*(?:(?:number|no\.?)\s*)?#?\s*\d+(?:\s+of\s+\d+)?[.!]?$/i.test( + text + ); + const toolGraphNodeLabel = ({ tool, description }: ToolGraphNode): string => { const text = description.replace(/\s+/g, ' ').trim(); - const placeholder = - text === 'No description was provided.' || - /^(?:tool(?: call)?|step|update|updating|task|operation)(?:\s+(?:tool(?: call)?|step|update|updating|task|operation))?\s*(?:(?:number|no\.?)\s*)?#?\s*\d+(?:\s+of\s+\d+)?[.!]?$/i.test( - text - ); - return placeholder ? sentenceStart(toolNamePhrase(tool)) : text; + return isPlaceholderStep(text) ? sentenceStart(toolNamePhrase(tool)) : text; }; -/** The verb a script's row carries, so the state reads "Ran …", "Failed to run …". */ -const RUN_SCRIPT = 'Running '; - /** - * A coordinated script as the work its plan names, its first and last steps - * in the model's own words after the verb: "Running list KBs → active KB", - * "Ran …" once done (V2-R2-4). Without the verb the line was the bare - * description, lowercase, the one row in a turn with no state on it. + * A coordinated script as the work its plan names: its first step, and its + * last after an arrow, in the model's own words ("Listing KBs → active KB"). + * The first step leads with a verb the row's state conjugates, so a finished + * script reads "Listed KBs → active KB" and a failed one "Failed to list KBs → + * active KB", the way every other row reads (V2-R2-4). Models write steps as + * imperatives, so the step's own verb is that verb ({@link stepActionPhrase}): + * a "Running " put in front of one read "Ran run ls -la". A step named only by + * its tool runs that tool ("Running Agent Drafter build app"). */ const summarizeToolGraph = (graph: ToolGraphNode[]): string => { - // Each step follows the verb, so it opens in lowercase unless it starts with a name. - const descriptions = graph.map((node) => inSentence(toolGraphNodeLabel(node))); - const actions = [...new Set(descriptions)]; + const steps = graph.map((node) => { + const text = node.description.replace(/\s+/g, ' ').trim(); + return isPlaceholderStep(text) + ? { + lead: toolActionPhrase(toolNameWords(node.tool)), + tail: inSentence(sentenceStart(toolNamePhrase(node.tool))), + } + : { lead: stepActionPhrase(text), tail: inSentence(text) }; + }); + // The same step written twice names one action. + const actions = [...new Map(steps.map((step) => [step.tail, step])).values()]; const shorten = (text: string, limit: number): string => { const characters = Array.from(text); return characters.length <= limit ? text : `${characters.slice(0, limit - 1).join('')}…`; }; - const planLimit = MAX_SUMMARY_VALUE_LENGTH - RUN_SCRIPT.length; - if (actions.length === 1) return `${RUN_SCRIPT}${shorten(actions[0], planLimit)}`; - const actionLimit = Math.floor((planLimit - 3) / 2); - return `${RUN_SCRIPT}${shorten(actions[0], actionLimit)} → ${shorten( - actions[actions.length - 1], + if (actions.length === 1) return shorten(actions[0].lead, MAX_SUMMARY_VALUE_LENGTH); + const actionLimit = Math.floor((MAX_SUMMARY_VALUE_LENGTH - 3) / 2); + return `${shorten(actions[0].lead, actionLimit)} → ${shorten( + actions[actions.length - 1].tail, actionLimit )}`; }; diff --git a/ui/desktop/src/components/artifacts/artifactFileProvenance.test.ts b/ui/desktop/src/components/artifacts/artifactFileProvenance.test.ts index 4c44a1a36..b680a5c30 100644 --- a/ui/desktop/src/components/artifacts/artifactFileProvenance.test.ts +++ b/ui/desktop/src/components/artifacts/artifactFileProvenance.test.ts @@ -157,6 +157,62 @@ describe('file-link reliability: provenance boundaries', () => { expect(filePathsBeforeMessage([request, response], 2, 'local', '/work')).toEqual([]); }); + // A script's record of its calls is proof a write ran, where its source is + // not: only a call recorded `ok` makes a file known. + it.each([ + ['ok', ['/elsewhere/report.md']], + ['error', []], + ])('promotes a script call recorded %s by its record alone', (status, expected) => { + const request: Message = { + id: 'request', + role: 'assistant', + created: 1, + metadata: { userVisible: true, agentVisible: true }, + content: [ + { + type: 'toolRequest', + id: 'script', + toolCall: { + status: 'success', + value: { + name: 'code_execution__execute_code', + arguments: { code: 'text_editor(opts);' }, + }, + }, + }, + ], + }; + const response: Message = { + id: 'response', + role: 'tool', + created: 2, + metadata: { userVisible: false, agentVisible: true }, + content: [ + { + type: 'toolResponse', + id: 'script', + toolResult: { + status: 'success', + value: { + isError: false, + content: [], + _meta: { + 'biorouter/tool-calls': [ + { + tool: 'developer__text_editor', + args: JSON.stringify({ command: 'write', path: '/elsewhere/report.md' }), + status, + }, + ], + }, + }, + }, + }, + ], + }; + expect(filePathsBeforeMessage([request, response], 2, 'local', '/work')).toEqual(expected); + }); + // The index is built for EVERY message of both transcript renderers, so it is // the first thing a shared transcript from a remote `base_url` reaches — and // `message.metadata.provenance` was read there without a guard. Measured in a diff --git a/ui/desktop/src/components/artifacts/artifactFileProvenance.ts b/ui/desktop/src/components/artifacts/artifactFileProvenance.ts index 015cd0e60..48b514805 100644 --- a/ui/desktop/src/components/artifacts/artifactFileProvenance.ts +++ b/ui/desktop/src/components/artifacts/artifactFileProvenance.ts @@ -2,6 +2,7 @@ import type { Message } from '../../api'; import { getTextContent } from '../../types/message'; import { baseToolName, + executedCallFiles, fileArtifactPathsFromToolCall, looksLikePreviewableFile, } from './artifactUtils'; @@ -108,8 +109,9 @@ function buildIndex( const call = calls.get(content.id); if (!call) continue; calls.delete(content.id); + const name = baseToolName(call.name); // Wrapper success does not prove a statically discovered inner write ran. - if (['shell', 'bash', 'execute_code', 'run_code'].includes(baseToolName(call.name))) continue; + if (name === 'shell' || name === 'bash') continue; const result = content.toolResult as { status?: string; value?: { isError?: boolean; is_error?: boolean }; @@ -121,6 +123,11 @@ function buildIndex( result.value.is_error ) continue; + // A script's record of the calls it made does: a write recorded `ok` ran. + if (name === 'execute_code' || name === 'run_code') { + for (const path of executedCallFiles(result, workingDir)?.written ?? []) add(path, index); + continue; + } for (const path of fileArtifactPathsFromToolCall(call.name, call.arguments, workingDir)) { add(path, index); } diff --git a/ui/desktop/src/components/artifacts/artifactUtils.test.ts b/ui/desktop/src/components/artifacts/artifactUtils.test.ts index 5b1a9c404..6ff5f2135 100644 --- a/ui/desktop/src/components/artifacts/artifactUtils.test.ts +++ b/ui/desktop/src/components/artifacts/artifactUtils.test.ts @@ -6,6 +6,7 @@ import { basenameFromPath, decodeResourceHtml, dirnameFromPath, + executedCallFiles, extensionFromPath, fileArtifactPathsFromToolCall, analyzeDelimitedColumns, @@ -1039,6 +1040,222 @@ describe('fileArtifactPathsFromToolCall — code_execution wrapper', () => { fileArtifactPathsFromToolCall('code_execution__execute_code', { code: 42 }, WORKING_DIR) ).toEqual([]); }); + + // Measured with GPT-5.5: the script built the command in a variable and passed + // the name, the file was written, and the panel never offered it. A name was + // looked up only inside a `${…}` template, never as the argument itself. + it('resolves a command passed as a name bound to a string — the measured repro', () => { + const code = [ + 'import { shell } from "developer";', + 'const cmd = "mkdir -p /tmp/smoke && printf \'# Notes\\n\' > /tmp/smoke/smoke-notes.md";', + 'shell({ command: cmd });', + ].join('\n'); + expect(exec(code)).toEqual(['/tmp/smoke/smoke-notes.md']); + }); + + it.each([ + [ + 'a path passed as a name', + 'const p = "/work/b.md"; text_editor({ command: "write", path: p, file_text: "x" });', + ['/work/b.md'], + ], + [ + 'a shorthand path', + 'const path = "/work/c.md"; text_editor({ command: "write", path, file_text: body });', + ['/work/c.md'], + ], + [ + 'a shorthand command', + 'const command = "echo x > /work/d.csv"; shell({ command });', + ['/work/d.csv'], + ], + [ + 'a name bound to a template over a name', + 'const dir = "/work"; const out = `${dir}/e.md`; write_file({ path: out, content: "x" });', + ['/work/e.md'], + ], + [ + 'a String.raw command in a name', + 'const cmd = String.raw`set -e\nprintf "x" > /work/f.md`;\nshell({ command: cmd });', + ['/work/f.md'], + ], + ['a quoted key', 'write_file({ "path": "/work/g.md", content: "x" });', ['/work/g.md']], + ])('resolves %s', (_label, code, expected) => { + expect(exec(code)).toEqual(expected); + }); + + it.each([ + ['a name bound to a concatenation', 'const p = "/work/" + name; write_file({ path: p });'], + ['a literal with a suffix expression', 'write_file({ path: "/work/report" + ext });'], + ['a name assigned twice', 'let p = "/work/a.md"; p = "/work/b.md"; write_file({ path: p });'], + [ + 'a name declared twice', + 'const p = "/work/a.md"; for (const p of paths) write_file({ path: p });', + ], + ['a name nothing binds', 'write_file({ path: target });'], + ['a spread that could supply the path', 'write_file({ path: "/work/a.md", ...overrides });'], + [ + 'a text_editor view through a name', + 'const c = "view"; text_editor({ command: c, path: "/work/a.md" });', + ], + ])('skips %s rather than guessing', (_label, code) => { + expect(exec(code)).toEqual([]); + }); + + it('reads keys only at the top level of the call object', () => { + const code = + 'write_file({ content: "x, path: \'/etc/passwd\'", meta: { path: "/etc/hosts" }, path: "/work/real.md" });'; + expect(exec(code)).toEqual(['/work/real.md']); + }); + + it('applies the command gate to str_replace_editor too', () => { + expect(exec('str_replace_editor({ command: "view", path: "/work/a.md" });')).toEqual([]); + expect(exec('str_replace_editor({ command: "str_replace", path: "/work/a.md" });')).toEqual([ + '/work/a.md', + ]); + }); +}); + +// The result of an execute_code call records every call its script made, with +// the arguments it ran with and whether it succeeded (`biorouter/tool-calls`). +// That is read in preference to the source; the scan is the fallback for a +// result from before the records existed. +describe('fileArtifactPathsFromToolCall — a script result’s call records', () => { + type Call = { tool: string; args: string; status: string }; + const resultWith = (calls: unknown, extra: Record = {}) => ({ + status: 'success', + value: { content: [], _meta: { 'biorouter/tool-calls': calls, ...extra } }, + }); + const ok = (tool: string, args: unknown): Call => ({ + tool, + args: typeof args === 'string' ? args : JSON.stringify(args), + status: 'ok', + }); + const failed = (tool: string, args: unknown): Call => ({ ...ok(tool, args), status: 'error' }); + const exec = (code: string, result: unknown) => + fileArtifactPathsFromToolCall( + 'code_execution__execute_code', + { code, tool_graph: [] }, + WORKING_DIR, + result + ); + // A record's args are cut at 2 KB and end with an ellipsis, exactly as the + // backend's `truncate_record_text` writes them. + const truncated = (args: Record) => `${JSON.stringify(args).slice(0, 2048)}…`; + + it('reads the resolved arguments, whatever the script computed them from', () => { + const code = 'for (const name of names) shell({ command: build(name) });'; + const result = resultWith([ + ok('developer__shell', { command: 'printf hi > /tmp/smoke/a.md' }), + ok('developer__text_editor', { command: 'write', path: 'notes/b.md', file_text: 'x' }), + ]); + expect(exec(code, result)).toEqual(['/tmp/smoke/a.md', '/home/ada/project/notes/b.md']); + }); + + it('adds nothing from a call that failed, though the source names it', () => { + const code = 'shell({ command: "printf hi > /tmp/smoke/a.md" });'; + const result = resultWith([ + failed('developer__shell', { command: 'printf hi > /tmp/smoke/a.md' }), + ]); + expect(exec(code, result)).toEqual([]); + }); + + it('adds nothing for a call the script never made, though the source names it', () => { + const code = 'if (false) write_file({ path: "/tmp/never.md" }); todo_update({ id: "1" });'; + expect(exec(code, resultWith([ok('todo__todo_update', { id: '1' })]))).toEqual([]); + }); + + it('keeps the direct rules: a view and a read-only office operation write nothing', () => { + const result = resultWith([ + ok('developer__text_editor', { command: 'view', path: '/tmp/a.md' }), + ok('computercontroller__xlsx_tool', { operation: 'get_cell', path: '/tmp/b.xlsx' }), + ok('computercontroller__xlsx_tool', { operation: 'update_cell', path: '/tmp/c.xlsx' }), + ]); + expect(exec('', result)).toEqual(['/tmp/c.xlsx']); + }); + + it('reads the path of a write whose args were cut, from the fields before the cut', () => { + const args = { command: 'write', path: '/tmp/big.md', file_text: 'line\n'.repeat(1000) }; + const result = resultWith([ok('developer__text_editor', truncated(args))]); + expect(exec('text_editor(opts);', result)).toEqual(['/tmp/big.md']); + }); + + it('falls back to the source when a cut write named its path after the cut', () => { + const args = { command: 'write', file_text: 'line\n'.repeat(1000), path: '/tmp/late.md' }; + const code = 'text_editor({ command: "write", file_text: body, path: "/tmp/late.md" });'; + expect(exec(code, resultWith([ok('developer__text_editor', truncated(args))]))).toEqual([ + '/tmp/late.md', + ]); + // With no source to fall back on, a cut it cannot read claims nothing. + expect( + exec('text_editor(opts);', resultWith([ok('developer__text_editor', truncated(args))])) + ).toEqual([]); + }); + + it('never reads a cut write without its command', () => { + const args = { path: '/tmp/a.md', file_text: 'x'.repeat(4000), command: 'view' }; + expect(exec('', resultWith([ok('developer__text_editor', truncated(args))]))).toEqual([]); + }); + + it('falls back to the source, minus failed calls, when calls went unrecorded', () => { + const code = [ + 'write_file({ path: "/tmp/scanned.md", content: "x" });', + 'write_file({ path: "/tmp/failed.md", content: "x" });', + ].join('\n'); + const result = resultWith( + [ + ok('developer__write_file', { path: '/tmp/recorded.md' }), + failed('developer__write_file', { path: '/tmp/failed.md' }), + ], + { 'biorouter/tool-calls-dropped': 3 } + ); + expect(exec(code, result)).toEqual(['/tmp/recorded.md', '/tmp/scanned.md']); + }); + + it('scans the source for a result from before the records existed', () => { + const code = 'const cmd = "printf hi > /tmp/old.md"; shell({ command: cmd });'; + expect(exec(code, { status: 'success', value: { content: [] } })).toEqual(['/tmp/old.md']); + expect(exec(code, undefined)).toEqual(['/tmp/old.md']); + }); + + it('reads a call result passed without the message wrapper', () => { + const result = { + content: [], + _meta: { 'biorouter/tool-calls': [ok('developer__write_file', { path: '/tmp/a.md' })] }, + }; + expect(executedCallFiles(result)).toEqual({ + written: ['/tmp/a.md'], + failed: [], + complete: true, + }); + }); + + it('treats a malformed successful record as unreadable rather than as nothing', () => { + const files = executedCallFiles( + resultWith([{ tool: 'developer__shell', status: 'ok' }, null, 7]) + ); + expect(files).toEqual({ written: [], failed: [], complete: false }); + }); + + it('anchors a cut shell command only when its targets need no anchor', () => { + const absolute = { + command: 'printf x > /tmp/abs.md', + timeout: 1, + working_directory: `/w${'x'.repeat(4000)}`, + }; + const relative = { + command: 'printf x > rel.md', + timeout: 1, + working_directory: `/w${'x'.repeat(4000)}`, + }; + expect( + executedCallFiles(resultWith([ok('developer__shell', truncated(absolute))]), WORKING_DIR) + ).toEqual({ written: ['/tmp/abs.md'], failed: [], complete: true }); + expect( + executedCallFiles(resultWith([ok('developer__shell', truncated(relative))]), WORKING_DIR) + ?.complete + ).toBe(false); + }); }); describe('withHostTheme', () => { diff --git a/ui/desktop/src/components/artifacts/artifactUtils.ts b/ui/desktop/src/components/artifacts/artifactUtils.ts index 0b1591776..b60e56a1e 100644 --- a/ui/desktop/src/components/artifacts/artifactUtils.ts +++ b/ui/desktop/src/components/artifacts/artifactUtils.ts @@ -660,6 +660,13 @@ const OFFICE_WRITING_OPERATIONS: Record> = { xlsx_tool: new Set(['update_cell', 'save']), }; +// Own keys only: a tool named `constructor` must not read `Object`'s. +function officeWritingOperations(name: string): Set | undefined { + return Object.prototype.hasOwnProperty.call(OFFICE_WRITING_OPERATIONS, name) + ? OFFICE_WRITING_OPERATIONS[name] + : undefined; +} + // `text_editor`-style commands that leave a file changed on disk. `view` and // `undo_edit` do not produce something new to look at. const MUTATING_EDITOR_COMMANDS = new Set(['create', 'diff', 'insert', 'str_replace', 'write']); @@ -763,16 +770,21 @@ export function resolveMarkdownImageSource(src: string, baseDir?: string): Markd // // Returns absolute paths only — a relative one without a `workingDir` to anchor // it would resolve against the wrong directory in the viewer. +// +// `toolResult` matters only for a Code Execution call: its result meta records +// the arguments every call the script made really ran with, which is read in +// preference to scanning the script's source (see `executedCallFiles`). export function fileArtifactPathsFromToolCall( toolName: string, args: unknown, - workingDir?: string + workingDir?: string, + toolResult?: unknown ): string[] { const argRecord = asRecord(args); if (!argRecord) return []; const name = baseToolName(toolName); - const officeOperations = OFFICE_WRITING_OPERATIONS[name]; + const officeOperations = officeWritingOperations(name); if (officeOperations) { const operation = argRecord.operation; if (typeof operation !== 'string' || !officeOperations.has(operation)) return []; @@ -788,8 +800,16 @@ export function fileArtifactPathsFromToolCall( // Unwrap the code-execution wrapper (default config) before anything else — // the real tool calls live inside its `code` string, not in `argRecord`. if (CODE_EXECUTION_TOOLS.has(name)) { + const executed = toolResult === undefined ? null : executedCallFiles(toolResult, workingDir); + if (executed?.complete) return executed.written; const code = argRecord.code; - return typeof code === 'string' ? pathsFromCodeBlob(code, workingDir) : []; + const scanned = typeof code === 'string' ? pathsFromCodeBlob(code, workingDir) : []; + // A result from before the records existed: the scan is all there is. + if (!executed) return scanned; + // Some successful call could not be read (or went unrecorded), so the scan + // fills in, minus every file a call that FAILED named. + const failed = new Set(executed.failed); + return [...new Set([...executed.written, ...scanned.filter((path) => !failed.has(path))])]; } if (name === 'shell' || name === 'bash') { @@ -899,50 +919,297 @@ function shellRedirectPaths(command: string, workingDir?: string): string[] { // --- code_execution unwrapping ---------------------------------------------- // -// The `code` string is JavaScript source, so extraction is textual and -// deliberately conservative — it mirrors the direct-call rules (mutating -// `text_editor` commands, `*_file` writes, `shell` redirect targets) applied to -// the calls found embedded in the blob, and resolves `` `${dir}/x.md` `` style -// template paths against simple `const dir = "…"` bindings. A path it cannot -// resolve (computed interpolation, a python-heredoc write) is skipped rather -// than guessed — no false artifacts, same posture as the direct path. - -/** `const/let/var NAME = "literal"` string bindings, so a template path like - * `` `${dir}/SCHEMA.md` `` in an embedded call can be resolved. Only plain - * string / interpolation-free backtick right-hand sides are captured. */ -function scanStringBindings(code: string): Map { - const bindings = new Map(); - const re = - /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:"((?:[^"\\]|\\.)*)"|'((?:[^'\\]|\\.)*)'|`([^`$]*)`)/g; - for (const m of code.matchAll(re)) { - bindings.set(m[1], (m[2] ?? m[3] ?? m[4] ?? '').replace(/\\(["'`\\])/g, '$1')); +// Two sources, best first. A Code Execution result records every call its +// script made, with the arguments each one ran with and whether it succeeded +// (`biorouter/tool-calls` in the result meta, issue #28). That is ground truth, +// and `executedCallFiles` reads it. A result from before those records existed +// has only the script, so `pathsFromCodeBlob` reads the source. +// +// The source scan is textual and deliberately conservative — it mirrors the +// direct-call rules (mutating `text_editor` commands, `*_file` writes, `shell` +// redirect targets) applied to the calls found embedded in the blob, and +// resolves an argument that is a name bound to a plain string +// (`const cmd = "…"; shell({ command: cmd })`) or a `` `${dir}/x.md` `` template +// over such names. A path it cannot resolve (computed interpolation, a name +// bound twice, a python-heredoc write) is skipped rather than guessed — no +// false artifacts, same posture as the direct path. + +const EXECUTED_CALLS_META_KEY = 'biorouter/tool-calls'; +const DROPPED_CALLS_META_KEY = 'biorouter/tool-calls-dropped'; + +/** What a Code Execution result's call records say about the files its script wrote. */ +export type ExecutedCallFiles = { + /** Files the script's SUCCESSFUL calls wrote, in call order. */ + written: string[]; + /** Files that calls which failed named: never artifacts, and kept out of a fallback scan. */ + failed: string[]; + /** False when a successful call's arguments could not be read, or a call went unrecorded. */ + complete: boolean; +}; + +/** + * The files a Code Execution script's calls wrote, read off the call records in + * its result meta, or null when the result carries none (a result from before + * the records existed, or a script that made no call). + * + * Only a call recorded `ok` counts. The backend cuts a record's `args` at 2 KB, + * so a real file's write arrives as a JSON prefix: its complete leading fields + * are read (`command` and `path` come before `file_text` in the order models + * write them), and a call cut before its rule can be decided makes the answer + * incomplete rather than empty. + */ +export function executedCallFiles( + toolResult: unknown, + workingDir?: string +): ExecutedCallFiles | null { + // The message's `{ status, value }` wrapper, or the call result itself. + const wrapper = asRecord(toolResult); + const value = typeof wrapper?.status === 'string' ? asRecord(wrapper.value) : wrapper; + const meta = asRecord(value?._meta); + const records = meta?.[EXECUTED_CALLS_META_KEY]; + if (!Array.isArray(records)) return null; + const dropped = meta?.[DROPPED_CALLS_META_KEY]; + const files: ExecutedCallFiles = { + written: [], + failed: [], + complete: dropped === undefined || dropped === 0, + }; + for (const candidate of records) { + const call = asRecord(candidate); + const ok = call?.status === 'ok'; + const paths = + typeof call?.tool === 'string' && typeof call.args === 'string' + ? executedCallPaths(call.tool, call.args, workingDir) + : null; + if (paths === null) { + if (ok) files.complete = false; + continue; + } + (ok ? files.written : files.failed).push(...paths); } - return bindings; + files.written = [...new Set(files.written)]; + return files; } -/** Resolve one JS string-literal token (quoted or template, optionally - * `String.raw`) to its concrete value, substituting `${ident}` from `bindings`. - * Returns null if it isn't a string literal or carries an unresolvable - * interpolation (a computed expression or an unknown identifier). */ -function resolveStringLiteral(token: string, bindings: Map): string | null { - const t = token.trim().replace(/^String\.raw\s*/, ''); - if ((t.startsWith('"') && t.endsWith('"')) || (t.startsWith("'") && t.endsWith("'"))) { - return t.slice(1, -1).replace(/\\(.)/g, '$1'); +/** The files one recorded call wrote: `[]` when it writes none, null when its arguments cannot say. */ +function executedCallPaths(tool: string, argsText: string, workingDir?: string): string[] | null { + const name = baseToolName(tool); + const shell = name === 'shell' || name === 'bash'; + const office = officeWritingOperations(name); + if (!shell && !office && !FILE_WRITING_TOOLS.has(name)) return []; + let args: Record | null = null; + try { + args = asRecord(JSON.parse(argsText)); + } catch { + // Cut at the backend's cap: read what came before the cut. } - if (t.startsWith('`') && t.endsWith('`')) { - let out = ''; - for (const part of t.slice(1, -1).split(/(\$\{[^}]*\})/)) { - if (part.startsWith('${')) { - const id = /^\$\{\s*([A-Za-z_$][\w$]*)\s*\}$/.exec(part); - if (!id || !bindings.has(id[1])) return null; - out += bindings.get(id[1]); - } else { - out += part; + if (args) return fileArtifactPathsFromToolCall(tool, args, workingDir); + const prefix = completeLeadingStrings(argsText); + if (!prefix) return null; + + // A key past the cut is unknown, so every key a rule reads must be present. + if (shell) { + if (typeof prefix.command !== 'string') return null; + if ('working_directory' in prefix) { + return fileArtifactPathsFromToolCall(tool, prefix, workingDir); + } + // A cut `working_directory` would move every relative target, so only an + // answer that needs no anchor stands. + const unanchored = fileArtifactPathsFromToolCall(tool, prefix); + const anchored = fileArtifactPathsFromToolCall(tool, prefix, workingDir); + return unanchored.length === anchored.length ? unanchored : null; + } + if (name === 'text_editor' || name === 'str_replace_editor') { + if (typeof prefix.command !== 'string') return null; + if (!MUTATING_EDITOR_COMMANDS.has(prefix.command)) return []; + } + if (office) { + if (typeof prefix.operation !== 'string') return null; + if (!office.has(prefix.operation)) return []; + } + if (!PATH_ARGUMENT_KEYS.some((key) => typeof prefix[key] === 'string')) return null; + return fileArtifactPathsFromToolCall(tool, prefix, workingDir); +} + +/** + * The string members of a JSON object that are complete before `text` ends, + * for an object the backend cut short. Reading stops at the first member that + * runs off the end. Null when `text` does not open an object. + */ +function completeLeadingStrings(text: string): Record | null { + let index = 0; + const space = () => { + while (index < text.length && /\s/.test(text[index])) index += 1; + }; + // The JSON string opening at `index`, decoded; null when it runs off the end. + const string = (): string | null => { + const start = index; + for (index += 1; index < text.length; index += 1) { + if (text[index] === '\\') { + index += 1; + } else if (text[index] === '"') { + index += 1; + try { + const value: unknown = JSON.parse(text.slice(start, index)); + return typeof value === 'string' ? value : null; + } catch { + return null; + } + } + } + return null; + }; + // Past one value that is not a string; false when it runs off the end. + const skip = (): boolean => { + let depth = 0; + while (index < text.length) { + const char = text[index]; + if (char === '"') { + if (string() === null) return false; + continue; + } + if (char === '{' || char === '[') { + depth += 1; + } else if (char === '}' || char === ']') { + if (depth === 0) return true; + depth -= 1; + } else if (char === ',' && depth === 0) { + return true; + } + index += 1; + } + return false; + }; + + space(); + if (text[index] !== '{') return null; + index += 1; + const fields = new Map(); + for (;;) { + space(); + if (text[index] !== '"') break; + const key = string(); + if (key === null) break; + space(); + if (text[index] !== ':') break; + index += 1; + space(); + if (text[index] === '"') { + const value = string(); + if (value === null) break; + fields.set(key, value); + } else if (!skip()) { + break; + } + space(); + if (text[index] !== ',') break; + index += 1; + } + return Object.fromEntries(fields); +} + +const IDENTIFIER_RE = /^[A-Za-z_$][\w$]*$/; + +const JS_ESCAPES: Record = { + 0: '\0', + b: '\b', + f: '\f', + n: '\n', + r: '\r', + t: '\t', + v: '\v', +}; + +/** A JavaScript string literal's body with its escapes decoded. */ +function decodeJsEscapes(body: string): string { + return body.replace( + /\\(?:x([\da-fA-F]{2})|u\{([\da-fA-F]{1,6})\}|u([\da-fA-F]{4})|(\r\n|[\s\S]))/g, + (_match, hex?: string, point?: string, unit?: string, other?: string) => { + const code = hex ?? point ?? unit; + if (code !== undefined) { + const value = parseInt(code, 16); + return value <= 0x10ffff ? String.fromCodePoint(value) : ''; } + // A backslash before a line break continues the line. + if (other === undefined || /^(?:\r\n|[\n\r\u2028\u2029])$/.test(other)) return ''; + return JS_ESCAPES[other] ?? other; + } + ); +} + +/** Index of the `quote` that closes a literal whose body starts at `from`, or -1. */ +function closingQuote(text: string, quote: string, from: number): number { + for (let index = from; index < text.length; index += 1) { + if (text[index] === '\\') index += 1; + else if (text[index] === quote) return index; + } + return -1; +} + +/** + * The value of a JS expression that is ONE string literal (quoted, or a + * template, optionally `String.raw`) or a bare name bound to one, with + * `${ident}` substituted from `bindings`. Null for anything else: a + * concatenation, a call, a computed interpolation or an unknown name. + */ +function resolveStringLiteral(expression: string, bindings: Map): string | null { + let text = expression.trim(); + if (IDENTIFIER_RE.test(text)) return bindings.get(text) ?? null; + const raw = /^String\.raw\s*`/.test(text); + if (raw) text = text.replace(/^String\.raw\s*/, ''); + const quote = text[0]; + if (quote !== '"' && quote !== "'" && quote !== '`') return null; + if (closingQuote(text, quote, 1) !== text.length - 1) return null; + const body = text.slice(1, -1); + if (quote !== '`') return decodeJsEscapes(body); + let out = ''; + for (const part of body.split(/(\$\{[^}]*\})/)) { + if (part.startsWith('${')) { + const id = /^\$\{\s*([A-Za-z_$][\w$]*)\s*\}$/.exec(part); + const value = id ? bindings.get(id[1]) : undefined; + if (value === undefined) return null; + out += value; + } else { + out += raw ? part : decodeJsEscapes(part); } - return out; } - return null; + return out; +} + +// `const/let/var NAME = ` and the end of the statement: a +// literal followed by `+ x`, `.trim()` or a call is not the value. +const STRING_BINDING_RE = + /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*((?:String\.raw\s*)?`(?:[^`\\]|\\[\s\S])*`|"(?:[^"\\\n]|\\[\s\S])*"|'(?:[^'\\\n]|\\[\s\S])*')(?=[ \t]*(?:[;,)\]}]|\/[/*]|\r?\n(?![ \t]*[-+*/%.?:&|^=<>[(`,])|$))/g; + +/** True when `name` is declared or assigned more than once in `code`. */ +function boundMoreThanOnce(code: string, name: string): boolean { + const escaped = name.replace(/\$/g, '\\$'); + const count = (pattern: string) => code.match(new RegExp(pattern, 'g'))?.length ?? 0; + const writes = count( + `(?>>?|&&|\\|\\||\\?\\?|[-+*/%&|^])?=(?![=>])` + ); + const declarations = count(`\\b(?:const|let|var|function|class)\\s+${escaped}(?![\\w$])`); + return writes > 1 || declarations > 1; +} + +/** Names bound to one plain string, so `shell({ command: cmd })` and a template + * path like `` `${dir}/SCHEMA.md` `` in an embedded call can be resolved. A + * name declared or assigned twice is left out: the scan does not know scopes, + * so it could not say which value a call saw. */ +function scanStringBindings(code: string): Map { + const matches = [...code.matchAll(STRING_BINDING_RE)]; + const ambiguous = new Set(); + for (const name of new Set(matches.map((match) => match[1]))) { + if (boundMoreThanOnce(code, name)) ambiguous.add(name); + } + const bindings = new Map(); + // In source order, so a template can use a name bound before it. + for (const [, name, literal] of matches) { + if (ambiguous.has(name)) continue; + const value = resolveStringLiteral(literal, bindings); + if (value !== null) bindings.set(name, value); + } + return bindings; } /** Source text of each `callName({ … })` argument object found in `code`, via a @@ -977,71 +1244,121 @@ function embeddedCallObjects(code: string, callName: string): string[] { return objects; } -/** The string-literal token assigned to `key` in an object-literal source (only - * at a property boundary, so `file_text: "path: …"` can't spoof a `path`). */ -function literalValueForKey(objectSrc: string, key: string): string | null { - const re = new RegExp( - `(?:^\\s*\\{\\s*|[{,]\\s*)${key}\\s*:\\s*` + - `(String\\.raw\\s*\`[^\`]*\`|\`[^\`]*\`|"(?:[^"\\\\]|\\\\.)*"|'(?:[^'\\\\]|\\\\.)*')` - ); - const m = re.exec(objectSrc); - return m ? m[1] : null; +/** + * The top-level properties of one object-literal source (`{ … }`, as + * `embeddedCallObjects` returns it): each key with its value's source, and a + * shorthand `{ command }` as its own name. Only top-level keys count, so + * neither a nested object nor a string value (`file_text: "path: …"`) can + * spoof one. Null when a spread or a computed key could supply any key. + */ +function objectLiteralProperties(objectSrc: string): Map | null { + if (!objectSrc.startsWith('{') || !objectSrc.endsWith('}')) return null; + const body = objectSrc.slice(1, -1); + const pieces: string[] = []; + let piece = ''; + let depth = 0; + for (let index = 0; index < body.length; index += 1) { + const char = body[index]; + if (char === '"' || char === "'" || char === '`') { + const end = closingQuote(body, char, index + 1); + if (end === -1) return null; + piece += body.slice(index, end + 1); + index = end; + } else if (char === '/' && body[index + 1] === '/') { + // A comment reads as a space, so a quote inside one opens no string. + const end = body.indexOf('\n', index); + index = end === -1 ? body.length : end - 1; + piece += ' '; + } else if (char === '/' && body[index + 1] === '*') { + const end = body.indexOf('*/', index + 2); + if (end === -1) return null; + index = end + 1; + piece += ' '; + } else if (char === ',' && depth === 0) { + pieces.push(piece); + piece = ''; + } else { + if ('([{'.includes(char)) depth += 1; + else if (')]}'.includes(char)) depth -= 1; + piece += char; + } + } + pieces.push(piece); + + const properties = new Map(); + for (const candidate of pieces) { + const text = candidate.trim(); + if (!text) continue; + if (text.startsWith('...') || text.startsWith('[')) return null; + const pair = + /^(?:([A-Za-z_$][\w$]*)|"((?:[^"\\]|\\.)*)"|'((?:[^'\\]|\\.)*)')\s*:([\s\S]*)$/.exec(text); + if (pair) { + properties.set(pair[1] ?? decodeJsEscapes(pair[2] ?? pair[3] ?? ''), pair[4].trim()); + } else if (IDENTIFIER_RE.test(text)) { + properties.set(text, text); + } + // A method or an accessor holds no value this scan can read. + } + return properties; } function pathsFromCodeBlob(code: string, workingDir?: string): string[] { const bindings = scanStringBindings(code); const seen = new Set(); const out: string[] = []; - const add = (resolved: string | null) => { - const anchored = resolved ? resolveArtifactPath(resolved, workingDir) : null; - if (anchored && !seen.has(anchored)) { - seen.add(anchored); - out.push(anchored); + const add = (path: string | null) => { + if (path && !seen.has(path)) { + seen.add(path); + out.push(path); } }; - const pathArgOf = (objectSrc: string): string | null => { + const calls = (callName: string): Map[] => + embeddedCallObjects(code, callName).flatMap((source) => { + const properties = objectLiteralProperties(source); + return properties ? [properties] : []; + }); + const valueOf = (properties: Map, key: string): string | null => { + const source = properties.get(key); + return source === undefined ? null : resolveStringLiteral(source, bindings); + }; + const pathArgOf = (properties: Map): string | null => { for (const key of PATH_ARGUMENT_KEYS) { - const token = literalValueForKey(objectSrc, key); - const value = token ? resolveStringLiteral(token, bindings) : null; - if (value) return value; + const value = valueOf(properties, key); + if (value) return resolveArtifactPath(value, workingDir); } return null; }; + // A `command` multiplexes reads and writes, so a write needs a known mutating one. + const writes = (properties: Map): boolean => { + const command = valueOf(properties, 'command'); + return command !== null && MUTATING_EDITOR_COMMANDS.has(command); + }; // Embedded `text_editor` — mutating commands only, mirroring the direct rule. - for (const objectSrc of embeddedCallObjects(code, 'text_editor')) { - const commandToken = literalValueForKey(objectSrc, 'command'); - const command = commandToken ? resolveStringLiteral(commandToken, bindings) : null; - if (!command || !MUTATING_EDITOR_COMMANDS.has(command)) continue; - add(pathArgOf(objectSrc)); + for (const properties of calls('text_editor')) { + if (writes(properties)) add(pathArgOf(properties)); } - // Embedded `write_file`/`create_file`/… — inherently writes, no command gate. + // Embedded `write_file`/`create_file`/… — no command gate unless one is given. for (const tool of FILE_WRITING_TOOLS) { if (tool === 'text_editor') continue; - for (const objectSrc of embeddedCallObjects(code, tool)) add(pathArgOf(objectSrc)); + for (const properties of calls(tool)) { + if (!properties.has('command') || writes(properties)) add(pathArgOf(properties)); + } } - // Embedded `shell`/`bash` — redirect / `-o` targets in the command literal. + // Embedded `shell`/`bash` — redirect / `-o` targets in the command string. for (const callName of ['shell', 'bash']) { - for (const objectSrc of embeddedCallObjects(code, callName)) { - const commandToken = literalValueForKey(objectSrc, 'command'); - const command = commandToken ? resolveStringLiteral(commandToken, bindings) : null; + for (const properties of calls(callName)) { + const command = valueOf(properties, 'command'); if (!command) continue; - const directoryToken = literalValueForKey(objectSrc, 'working_directory'); - const directory = directoryToken ? resolveStringLiteral(directoryToken, bindings) : null; - const hasDirectory = /[,{]\s*working_directory\s*:/.test(objectSrc); - const shellDir = hasDirectory + const directory = valueOf(properties, 'working_directory'); + const shellDir = properties.has('working_directory') ? directory ? (resolveArtifactPath(directory, workingDir) ?? undefined) : undefined : workingDir; - for (const path of shellRedirectPaths(command, shellDir)) { - if (!seen.has(path)) { - seen.add(path); - out.push(path); - } - } + for (const path of shellRedirectPaths(command, shellDir)) add(path); } } diff --git a/ui/desktop/src/components/toolCallCopy.test.ts b/ui/desktop/src/components/toolCallCopy.test.ts index 231b6b809..9efa7abe8 100644 --- a/ui/desktop/src/components/toolCallCopy.test.ts +++ b/ui/desktop/src/components/toolCallCopy.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { inSentence, + stepActionPhrase, toolActionPhrase, toolCallNameWords, toolCallPhrase, @@ -140,3 +141,47 @@ describe('tool names read in sentence case (V2-04)', () => { } }); }); + +// A Code Execution script's row leads with its first plan step, written by the +// model, usually as an imperative. A verb put in front of that read "Ran run +// ls -la" and "Failed to run write the requested markdown file". +describe('stepActionPhrase: a plan step leads with its own verb', () => { + it.each([ + ['run ls -la', 'Running ls -la'], + ['write the requested markdown file', 'Writing the requested markdown file'], + ['Create the target directory', 'Creating the target directory'], + ['Read the manifest', 'Reading the manifest'], + ['lookup the cohort', 'Looking up the cohort'], + [' list KBs ', 'Listing KBs'], + ])('turns the imperative %j into the gerund the row conjugates', (step, expected) => { + expect(stepActionPhrase(step)).toBe(expected); + }); + + it('keeps a step that already opens with a gerund', () => { + expect(stepActionPhrase('running the smoke test')).toBe('Running the smoke test'); + expect(stepActionPhrase('Writing notes')).toBe('Writing notes'); + }); + + it.each([ + ['attempt the command', 'Attempt the command'], + ['active KB', 'Active KB'], + ['SQL count of samples', 'SQL count of samples'], + ['READ the docs', 'READ the docs'], + ['read/write the config', 'Read/write the config'], + ['constructor of the report', 'Constructor of the report'], + ['step 1 of 3: Read the signed manifest', 'Step 1 of 3: Read the signed manifest'], + ])('leaves %j in its own words, in sentence case', (step, expected) => { + expect(stepActionPhrase(step)).toBe(expected); + }); + + it('never yields a doubled verb in any state', () => { + for (const step of ['run ls -la', 'write the file', 'create the dir', 'attempt the command']) { + const summary = stepActionPhrase(step); + for (const state of ['running', 'done', 'failed', 'stopped'] as const) { + expect(toolRowLabel(summary, state)).not.toMatch( + /^(?:Ran|Running|Failed to run|Stopped running) (?:run|write|create|attempt)\b/i + ); + } + } + }); +}); diff --git a/ui/desktop/src/components/toolCallCopy.ts b/ui/desktop/src/components/toolCallCopy.ts index 77374baea..7d0cac9fa 100644 --- a/ui/desktop/src/components/toolCallCopy.ts +++ b/ui/desktop/src/components/toolCallCopy.ts @@ -269,6 +269,37 @@ export function toolActionPhrase(words: readonly string[]): string { : NAME_VERBS[words[index]]; } +/** + * A plan step, in the model's own words, as a row summary. Models write steps + * as imperatives ("run ls -la", "write the requested markdown file"), so a verb + * put in front of one read "Ran run ls -la" and "Failed to run write the + * requested markdown file". A known opening verb becomes the gerund the row's + * state conjugates ("Running ls -la", done "Ran ls -la", failed "Failed to run + * ls -la"), and a gerund stays one. Any other opening (a verb this module does + * not know, a noun phrase) stands in sentence case in the model's own words, + * which {@link toolRowLabel} shows as is while running and once done and names + * a failure or a stop before ("Failed: Attempt the command"). + */ +export function stepActionPhrase(step: string): string { + const text = step.replace(/\s+/g, ' ').trim(); + const match = /^([A-Za-z]+)(?=\s|$)(.*)$/.exec(text); + if (match) { + const [, word, rest] = match; + const lower = word.toLowerCase(); + const capitalized = sentenceStart(lower); + // An acronym in capitals ("SQL", "KB") is a name, not a verb. + if (word === lower || word === capitalized) { + if (Object.prototype.hasOwnProperty.call(VERB_FORMS, capitalized)) { + return `${capitalized}${rest}`; + } + if (Object.prototype.hasOwnProperty.call(NAME_VERBS, lower)) { + return `${NAME_VERBS[lower]}${rest}`; + } + } + } + return sentenceStart(text); +} + /** * The visible row label for a summary in a given state. * @@ -279,10 +310,11 @@ export function toolActionPhrase(words: readonly string[]): string { * - stopped: "Stopped reading package.json". * * A summary that does not open with a known gerund (a tool's display name, a - * plan step's own words such as "Read the manifest") cannot be conjugated, and - * a prefix verb would read "Ran Read the manifest". So it stands alone while - * running (the pulse says so) and once done, and failure or a stop is named - * before it: "Failed: X", "Stopped: X". + * plan step's own words such as "Attempt the command", see + * {@link stepActionPhrase}) cannot be conjugated, and a prefix verb would read + * "Ran attempt the command". So it stands alone while running (the pulse says + * so) and once done, and failure or a stop is named before it: "Failed: X", + * "Stopped: X". */ export function toolRowLabel(summary: string, state: ToolRowState): string { const match = /^([A-Z][a-z]+)\b(.*)$/s.exec(summary); From 063126fbf760c50580852bd83da5565e851e50be Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sat, 17 Oct 2026 18:06:26 -0700 Subject: [PATCH 2/2] fix: post-merge edge cases in the stream retry and serve behind a proxy Found by the live tests after #403, #404 and #405 merged. Stream retry (#403/#405): - A tool call written as text and cut by a retryable error was shown and saved as a reply, and the retry was told to continue it. The local providers' text recovery now marks text it releases only because of an error; the reply loop drops it when the error will be retried (MistakeTracker::would_retry) and shows it otherwise. - A whitespace-only partial reply counted as a partial reply. - The block was dropped from every request after a stored reply ending in whitespace: inject_moim no longer treats the trailing-whitespace and empty-message cleanups as its own fault. - The mid-task note read "Server does.. No tool ran". - The compute-error notice no longer asks the person to try again right before Biorouter does. Serve behind a proxy (#404): - Every session and app cookie value is read, so a stale cookie at a longer path no longer shadows a fresh token. - Cookies set at an https address are marked Secure; plain http never. - A valid token redeemed outside the declared --public-url path says which address to use instead of "needs its access token". - The shell links its icon relative to , so /favicon.ico is no longer requested outside the prefix. - The browser's marketplace fetches the live registry (no referrer, no credentials) instead of requesting /undefined. - A daemon with no token configured no longer copies ?t= into Set-Cookie. - browser-access.md covers the app "No Biorouter backend" banner behind an https,http chain (fix: --public-url). --- crates/biorouter-server/src/auth.rs | 189 ++++++- crates/biorouter-server/src/routes/apps.rs | 60 ++- crates/biorouter-server/src/routes/web_ui.rs | 496 +++++++++++++++++- crates/biorouter/src/agents/agent.rs | 114 ++++ crates/biorouter/src/agents/mistakes.rs | 117 ++++- crates/biorouter/src/agents/moim.rs | 104 +++- .../src/providers/formats/text_tool_calls.rs | 124 ++++- .../src/providers/llamacpp_sidecar.rs | 27 +- .../tests/output_recovery_agent_loop.rs | 382 +++++++++++++- docs/deployment/browser-access.md | 24 +- docs/deployment/serve-architecture.md | 17 +- ui/desktop/index.html | 8 + .../src/components/baam/registry.test.ts | 87 +++ ui/desktop/src/components/baam/registry.ts | 51 ++ ui/desktop/src/faviconLink.test.ts | 26 + ui/desktop/src/renderer.tsx | 6 +- 16 files changed, 1773 insertions(+), 59 deletions(-) create mode 100644 ui/desktop/src/faviconLink.test.ts diff --git a/crates/biorouter-server/src/auth.rs b/crates/biorouter-server/src/auth.rs index edd9dfdc1..bbc8ca25a 100644 --- a/crates/biorouter-server/src/auth.rs +++ b/crates/biorouter-server/src/auth.rs @@ -257,11 +257,11 @@ pub fn served_operator_capability( } /// Whether `headers` carry the cookie of the document a serve daemon served. +/// Any value of it counts: a browser also sends a stale one a previous launch +/// set at a longer path, and sends it first (`routes::web_ui::session_cookies`). fn from_served_document(headers: &axum::http::HeaderMap, operator: &ServedOperator) -> bool { - served_document_matches( - crate::routes::web_ui::session_cookie(headers), - &operator.browser_token, - ) + crate::routes::web_ui::session_cookies(headers) + .any(|presented| served_document_matches(Some(presented), &operator.browser_token)) } /// Does the presented cookie carry the served document's token? @@ -497,7 +497,9 @@ pub fn mint_app_launch(app_id: &str) -> String { /// never sent back, so every app refused its own page. `HttpOnly` keeps the /// page's own script from reading it, /// and `SameSite=Strict` keeps it off every request another site starts: the -/// two flags `routes::web_ui` sets on the served document's cookie. +/// two flags `routes::web_ui` sets on the served document's cookie. The page +/// route appends the third, `Secure`, when the browser is at an `https` address +/// ([`browser_used_https`]), which this function cannot see. /// /// ⚠ `Path` decides which requests the browser attaches the cookie to, and is /// NOT a boundary between pages: every app is served from the daemon's one @@ -534,8 +536,11 @@ pub fn forget_app_access(app_id: &str) { app_access().forget(app_id); } -/// The access cookie a request carries, if any. -fn app_access_cookie(headers: &axum::http::HeaderMap) -> Option<&str> { +/// Every value of the access cookie a request carries. A browser keeps one per +/// `Path` and sends them all, the longest path first, so a caller asks whether +/// any of them grants access rather than reading the first, as +/// `routes::web_ui::session_cookies` does for the same reason. +fn app_access_cookies(headers: &axum::http::HeaderMap) -> impl Iterator { let name = app_access_cookie_name(); headers .get_all(axum::http::header::COOKIE) @@ -543,7 +548,7 @@ fn app_access_cookie(headers: &axum::http::HeaderMap) -> Option<&str> { .filter_map(|value| value.to_str().ok()) .flat_map(|value| value.split(';')) .filter_map(|pair| pair.split_once('=')) - .find(|(key, _)| key.trim() == name) + .filter(move |(key, _)| key.trim() == name) .map(|(_, value)| value.trim()) } @@ -621,7 +626,7 @@ fn app_access_granted_by( return true; } let access = app_access(); - if app_access_cookie(headers).is_some_and(|value| access.cookie_matches(app.id, value)) { + if app_access_cookies(headers).any(|value| access.cookie_matches(app.id, value)) { return true; } app.page @@ -685,6 +690,69 @@ pub fn reached_over_a_secure_context( || is_a_websocket_upgrade_from_an_https_page(headers) } +/// Whether the browser sent this request to an `https` address, as far as the +/// daemon can tell. It decides whether a cookie set in answer carries `Secure` +/// (`routes::web_ui::secure_attribute`): the session cookie the token exchange +/// sets and an app's access cookie. `declared` is the origin `biorouter serve +/// --public-url` declared (`routes::PublicUrl`), if any. +/// +/// - A `Host` that names the declared `https` origin: the browser addressed it. +/// - Any other address that is not loopback: a browser sends Fetch Metadata only +/// to an https or loopback URL, and only such a request (or a WebSocket +/// handshake from an `https` page) reaches a cookie at all +/// ([`reached_over_a_secure_context`]). A declared `https` address says the +/// same of a client that sends no metadata, which is not a browser and keeps +/// whatever it is given. +/// - A loopback `Host`: plain `http` to this machine, the one case where +/// `Secure` would break the cookie, since the browser would never send it back +/// over `http`. Unless an `https` address was declared AND a proxy forwarded +/// the request (it carries `X-Forwarded-For`, `X-Forwarded-Host`, +/// `X-Forwarded-Proto` or `Forwarded`): a proxy that rewrites `Host` to the +/// daemon's own reaches here so. A browser on this machine sends none of those +/// headers, so a client that writes one marks only its own cookie `Secure`. +/// +/// ⚠ A hardening flag, never a gate: a wrong `false` leaves a cookie that works +/// without the flag, as every cookie did before it, and nothing here admits or +/// refuses a request. +pub fn browser_used_https( + headers: &axum::http::HeaderMap, + uri: &axum::http::Uri, + declared: Option<&str>, +) -> bool { + let host = match headers.get(axum::http::header::HOST) { + Some(value) => match value.to_str() { + Ok(host) => host, + Err(_) => return false, + }, + None => match uri.authority() { + Some(authority) => authority.as_str(), + None => return false, + }, + }; + let declared_https = declared.and_then(|origin| origin.strip_prefix("https://")); + if declared_https.is_some_and(|authority| host.trim().eq_ignore_ascii_case(authority)) { + return true; + } + if host_is_loopback(host) { + return declared_https.is_some() && was_forwarded_by_a_proxy(headers); + } + declared_https.is_some() + || carries_fetch_metadata(headers) + || is_a_websocket_upgrade_from_an_https_page(headers) +} + +/// Whether a request carries a header only a forwarding proxy writes. +fn was_forwarded_by_a_proxy(headers: &axum::http::HeaderMap) -> bool { + [ + "x-forwarded-for", + "x-forwarded-host", + "x-forwarded-proto", + "forwarded", + ] + .iter() + .any(|name| headers.contains_key(*name)) +} + /// Whether a request is a WebSocket handshake opened by a page served over /// `https`: `Upgrade: websocket` and an `Origin` whose scheme is `https`. /// @@ -2234,6 +2302,109 @@ mod tests { )); } + /// A browser keeps one cookie per `Path` under a name and sends every one + /// that covers the request, the longest path first. A stale value in front + /// (a launch that declared a longer `--public-url` path, an app redeemed + /// under a proxy prefix as well as at the root) must not hide the one that + /// grants access: any value that matches counts, and only one that matches. + #[test] + fn a_stale_cookie_in_front_does_not_hide_the_one_that_grants_access() { + let page = is_app_browser_get(&Method::GET, "/apps/stale-cookie-app/").unwrap(); + let granted = |cookie: &str, operator: Option<&ServedOperator>| { + let mut headers = axum::http::HeaderMap::new(); + headers.insert(axum::http::header::COOKIE, cookie.parse().unwrap()); + app_access_granted_by(&page, None, &headers, operator) + }; + + let fresh = cookie_for("stale-cookie-app"); + let (name, _) = fresh.split_once('=').unwrap(); + let stale = format!("{name}={}", "0".repeat(64)); + assert!(granted(&format!("{stale}; {fresh}"), None)); + assert!(!granted(&stale, None)); + assert!(!granted(&format!("{stale}; {stale}"), None)); + + let operator = ServedOperator { + browser_token: "served-document-token".into(), + capability: biorouter::privacy::ProviderTier::Public, + }; + let served = "biorouter_session=served-document-token"; + assert!(granted( + &format!("biorouter_session=stale; {served}"), + Some(&operator) + )); + assert!(!granted( + "biorouter_session=stale; biorouter_session=older", + Some(&operator) + )); + } + + /// `Secure` on a cookie the daemon sets, decided from the request and the + /// declared address. Never on plain `http` to loopback, where it would keep + /// the cookie off every later request and `serve` would refuse its own link. + #[test] + fn a_cookie_is_marked_secure_only_where_the_browser_used_https() { + use super::browser_used_https; + let request = |host: Option<&str>, extra: &[(&str, &str)]| { + let mut headers = axum::http::HeaderMap::new(); + if let Some(host) = host { + headers.insert(axum::http::header::HOST, host.parse().unwrap()); + } + for (name, value) in extra { + headers.insert( + axum::http::HeaderName::from_bytes(name.as_bytes()).unwrap(), + value.parse().unwrap(), + ); + } + headers + }; + let metadata = [ + ("sec-fetch-mode", "navigate"), + ("sec-fetch-dest", "document"), + ]; + let none = axum::http::Uri::default(); + let declared = Some("https://hub.test:18443"); + + // Plain loopback http, with or without a declared https address. + for declared in [None, declared] { + for host in ["127.0.0.1:8765", "localhost:8765", "[::1]:8765"] { + assert!( + !browser_used_https(&request(Some(host), &metadata), &none, declared), + "{host} {declared:?}" + ); + } + } + // A proxy that keeps Host: the browser's metadata says https. + assert!(browser_used_https( + &request(Some("biorouter.example.org"), &metadata), + &none, + None + )); + // The declared https address itself, including on a loopback name. + assert!(browser_used_https( + &request(Some("hub.test:18443"), &[]), + &none, + declared + )); + assert!(browser_used_https( + &request(Some("localhost:18443"), &[]), + &none, + Some("https://localhost:18443") + )); + // A proxy that rewrote Host to the daemon's: only with https declared, + // and only when the request says a proxy forwarded it. + for forwarded in ["x-forwarded-for", "x-forwarded-proto", "forwarded"] { + let headers = request(Some("127.0.0.1:8765"), &[(forwarded, "x")]); + assert!(browser_used_https(&headers, &none, declared), "{forwarded}"); + assert!(!browser_used_https(&headers, &none, None), "{forwarded}"); + assert!( + !browser_used_https(&headers, &none, Some("http://localhost:8765")), + "{forwarded}" + ); + } + // No Host and no authority: not a browser. + assert!(!browser_used_https(&request(None, &metadata), &none, None)); + } + // --- The failed-attempt throttle (QA-D F3) ------------------------------ use super::{check_token, failed_attempt_verdict, FAILED_AUTH_BUDGET, FAILED_AUTH_WINDOW}; diff --git a/crates/biorouter-server/src/routes/apps.rs b/crates/biorouter-server/src/routes/apps.rs index 825b4e92e..bc809c9d6 100644 --- a/crates/biorouter-server/src/routes/apps.rs +++ b/crates/biorouter-server/src/routes/apps.rs @@ -461,13 +461,21 @@ fn launch_bounce(cookie: Option) -> Response { /// so the token does not linger in the address bar or the `Referer` of anything /// the page loads. `auth::check_token` has already admitted the request, by that /// token, the cookie, or the secret; a token that is spent, expired or for -/// another app sets nothing. -async fn serve_index(Path(id): Path, Query(query): Query) -> Response { +/// another app sets nothing. The cookie carries `Secure` when the browser is at +/// an `https` address (`web_ui::secure_attribute`), as behind a TLS proxy. +async fn serve_index( + Path(id): Path, + Query(query): Query, + headers: axum::http::HeaderMap, + uri: axum::http::Uri, +) -> Response { if let Some(token) = query.t.as_deref() { if validate_artifact_id(&id).is_err() { return (StatusCode::BAD_REQUEST, "invalid app id").into_response(); } - let cookie = biorouter_server::auth::redeem_app_launch(&id, token); + let secure = super::web_ui::secure_attribute(&headers, &uri, super::declared_public_url()); + let cookie = biorouter_server::auth::redeem_app_launch(&id, token) + .map(|cookie| format!("{cookie}{secure}")); return launch_bounce(cookie); } let st = store(); @@ -9102,6 +9110,8 @@ mod tests { super::serve_index( Path("hrd1-launch".to_string()), Query(super::PageQuery { t }), + axum::http::HeaderMap::new(), + axum::http::Uri::default(), ) }; let set_cookie = |response: &axum::response::Response| { @@ -9175,6 +9185,48 @@ mod tests { super::store().delete("hrd1-launch").unwrap(); } + /// Behind an https proxy the app's access cookie is `Secure`, as the served + /// interface's own is (`web_ui::secure_attribute`); on plain `http` to + /// loopback, where `Secure` would keep it off every later request, it is not. + #[tokio::test] + async fn the_app_cookie_is_secure_only_behind_https() { + use axum::extract::{Path, Query}; + use axum::http::{header, HeaderMap}; + + let redeem = |host: &'static str| async move { + let mut headers = HeaderMap::new(); + headers.insert(header::HOST, host.parse().unwrap()); + headers.insert("sec-fetch-mode", "navigate".parse().unwrap()); + headers.insert("sec-fetch-dest", "document".parse().unwrap()); + let token = biorouter_server::auth::mint_app_launch("secure-cookie-app"); + let bounce = super::serve_index( + Path("secure-cookie-app".to_string()), + Query(super::PageQuery { t: Some(token) }), + headers, + "/apps/secure-cookie-app/".parse().unwrap(), + ) + .await; + bounce + .headers() + .get(header::SET_COOKIE) + .expect("a fresh link sets the cookie") + .to_str() + .unwrap() + .to_string() + }; + let proxied = redeem("hub.example.org").await; + assert!( + proxied.ends_with("; HttpOnly; SameSite=Strict; Secure"), + "{proxied}" + ); + let loopback = redeem("127.0.0.1:8765").await; + assert!( + loopback.ends_with("; HttpOnly; SameSite=Strict"), + "{loopback}" + ); + assert!(!loopback.contains("Secure"), "{loopback}"); + } + /// The launch answer names the link on the daemon's own listener as well as /// the bare path. The desktop reaches the daemon through a proxy that refuses /// every request without its secret, so a link it built on the proxy's @@ -9438,6 +9490,8 @@ mod tests { let resp = super::serve_index( axum::extract::Path(id.to_string()), axum::extract::Query(super::PageQuery { t: None }), + axum::http::HeaderMap::new(), + axum::http::Uri::default(), ) .await; let status = resp.status(); diff --git a/crates/biorouter-server/src/routes/web_ui.rs b/crates/biorouter-server/src/routes/web_ui.rs index 1d5598372..13c958869 100644 --- a/crates/biorouter-server/src/routes/web_ui.rs +++ b/crates/biorouter-server/src/routes/web_ui.rs @@ -113,6 +113,11 @@ use tower_http::services::ServeDir; /// The name of the session cookie the token exchange sets. const SESSION_COOKIE: &str = "biorouter_session"; +/// The name of the cookie that says a browser redeemed a valid token at the +/// address it is on, set beside the session cookie when that one is scoped to a +/// declared path ([`WebUi::wrong_address`]). +const EXCHANGED_COOKIE: &str = "biorouter_exchanged"; + /// Everything the serving path needs, resolved once at startup. #[derive(Clone)] pub struct WebUi { @@ -128,6 +133,14 @@ pub struct WebUi { /// (`routes::PublicUrl`). Behind JupyterHub every user's server shares one /// hostname, and a cookie at `/` would carry this token to all of them. cookie_path: String, + /// The address `--public-url` declared, the declaration `cookie_path` came + /// from. Its scheme decides `Secure` behind a proxy that rewrites `Host` + /// ([`secure_attribute`]), and a browser that redeemed its token somewhere + /// else is pointed at it ([`WebUi::wrong_address`]). + public: Option, + /// The value of [`EXCHANGED_COOKIE`]: random per daemon run, so the cookie a + /// previous daemon set does not count, and it carries nothing of the token. + exchange_marker: String, } #[derive(Debug, Deserialize)] @@ -147,6 +160,9 @@ impl WebUi { /// /// `public_path` is the path of the address `biorouter serve --public-url` /// declared, `/` or a prefix with no trailing slash, and scopes the cookie. + /// The rest of that declaration is read from where `commands::agent` read + /// the path, [`super::declared_public_url`], because the path alone does not + /// carry the scheme or the address to name. pub fn new( web_dir: &Path, secret_key: &str, @@ -158,9 +174,54 @@ impl WebUi { index_html: inject_runtime_config(&raw, secret_key), browser_token, cookie_path: public_path.unwrap_or("/").to_string(), + public: super::declared_public_url() + .filter(|declared| public_path == Some(declared.path())) + .cloned(), + exchange_marker: hex::encode(rand::random::<[u8; 16]>()), }) } + /// Does the request carry a session cookie holding the token? Any of them + /// counts ([`session_cookies`]); each is compared in constant time. + fn cookie_matches(&self, headers: &HeaderMap) -> bool { + self.browser_token.is_none() + || session_cookies(headers).any(|presented| self.token_matches(Some(presented))) + } + + /// The `Set-Cookie` values a redeemed token answers with: the session + /// cookie, scoped to the declared path, and, when that path is not `/`, the + /// [`EXCHANGED_COOKIE`] marker with no `Path`, which the browser scopes to + /// the address it is really on. The marker comes back on the next request + /// whether or not that address is under the declared path; the session + /// cookie comes back only when it is. That difference is the whole of + /// [`WebUi::wrong_address`]'s evidence. + fn exchange_cookies(&self, token: &str, secure: &str) -> Vec { + let mut cookies = vec![format!( + "{SESSION_COOKIE}={token}; Path={}; HttpOnly; SameSite=Strict{secure}", + self.cookie_path + )]; + if self.cookie_path != "/" { + cookies.push(format!( + "{EXCHANGED_COOKIE}={}; Max-Age=300; HttpOnly; SameSite=Strict{secure}", + self.exchange_marker + )); + } + cookies + } + + /// The declared address, when a request without a usable session cookie + /// comes from a browser that redeemed a valid token at another address: + /// the marker [`WebUi::exchange_cookies`] set came back and the session + /// cookie did not, so the browser is outside the path the cookie is scoped + /// to. The proxy strips that path before the daemon sees it, so the cookies + /// are the only witness the daemon has. + fn wrong_address(&self, headers: &HeaderMap) -> Option<&super::PublicUrl> { + let public = self.public.as_ref()?; + cookie_values(headers, EXCHANGED_COOKIE) + .any(|presented| super::secret_matches(presented, &self.exchange_marker)) + .then_some(public) + } + fn token_matches(&self, presented: Option<&str>) -> bool { match &self.browser_token { // No token configured: the launcher bound loopback and chose not to @@ -237,23 +298,32 @@ fn inject_runtime_config(raw: &str, secret_key: &str) -> String { } } -/// Read one cookie out of a `Cookie` header. +/// Every value a request carries for the cookie `name`, in the order sent. /// -/// Hand-rolled rather than pulling in a cookie crate: one name is read and one -/// is written, and the parsing rule -- split on `;`, then on the first `=` -- -/// is the whole of what is needed. -fn cookie_value<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { +/// Hand-rolled rather than pulling in a cookie crate: the parsing rule -- split +/// on `;`, then on the first `=` -- is the whole of what is needed. +/// +/// ⚠ All of them, not the first. A browser sends every cookie whose `Path` +/// covers the request, the longest path first, and two cookies of one name +/// with different paths are both kept. After `biorouter serve --public-url +/// https://hub/user/a/proxy/9999/` set its cookie at that path, a later +/// `serve` without the flag sets a fresh one at `/` under the same name, and +/// the browser then sends the stale, longer-path value first. Reading only the +/// first refused every new link with "This link needs its access token". +fn cookie_values<'a>(headers: &'a HeaderMap, name: &'a str) -> impl Iterator { headers - .get(header::COOKIE)? - .to_str() - .ok()? - .split(';') + .get_all(header::COOKIE) + .iter() + .filter_map(|value| value.to_str().ok()) + .flat_map(|value| value.split(';')) .filter_map(|pair| pair.split_once('=')) - .find(|(k, _)| k.trim() == name) + .filter(move |(k, _)| k.trim() == name) .map(|(_, v)| v.trim()) } -/// The session cookie the token exchange set, if the request carries one. +/// The session cookies the request carries: every value of the cookie the +/// token exchange sets. A caller asks whether ANY of them is the token, never +/// reads one (see [`cookie_values`]). /// /// The one reader of [`SESSION_COOKIE`]: the shell below asks it whether to /// serve the document, and `auth::served_operator_capability` asks it whether a @@ -264,8 +334,8 @@ fn cookie_value<'a>(headers: &'a HeaderMap, name: &str) -> Option<&'a str> { /// authentication on an API route: `check_token` still demands `X-Secret-Key` /// there, so the cookie can only narrow a caller that already holds the secret, /// never admit one that does not. -pub(crate) fn session_cookie(headers: &HeaderMap) -> Option<&str> { - cookie_value(headers, SESSION_COOKIE) +pub(crate) fn session_cookies(headers: &HeaderMap) -> impl Iterator { + cookie_values(headers, SESSION_COOKIE) } /// The application shell, and the token-for-cookie exchange that gates it. @@ -305,19 +375,28 @@ async fn index( // the `Referer` of anything the page later loads. if let Some(token) = query.t.as_deref() { if ui.token_matches(Some(token)) { - return exchange_bounce( - &format!( - "{SESSION_COOKIE}={token}; Path={}; HttpOnly; SameSite=Strict", - ui.cookie_path - ), - &mount, - ); + // The cookie holds the CONFIGURED token, never the text the URL + // presented. With none configured every `?t=` matches, and copying + // it would let a `;` in it add cookie attributes, and a CR or LF + // fail the response; such a daemon checks no cookie, so it sets none + // and only moves the browser on. + let cookies = match ui.browser_token.as_deref() { + Some(expected) => { + let secure = secure_attribute(&headers, &uri, ui.public.as_ref()); + ui.exchange_cookies(expected, secure) + } + None => Vec::new(), + }; + return exchange_bounce(&cookies, &mount); } return unauthorized(); } - if !ui.token_matches(session_cookie(&headers)) { - return unauthorized(); + if !ui.cookie_matches(&headers) { + return match ui.wrong_address(&headers) { + Some(public) => wrong_address(public), + None => unauthorized(), + }; } Response::builder() @@ -445,8 +524,8 @@ fn not_a_navigation() -> Response { /// because the page's policy allows no script, and no token in the page: the /// cookie carries it, and the address the page moves to is the mount /// ([`mount_href`]), relative, so the browser stays under a proxy's path prefix. -fn exchange_bounce(cookie: &str, mount: &str) -> Response { - Response::builder() +fn exchange_bounce(cookies: &[String], mount: &str) -> Response { + let mut bounce = Response::builder() .status(StatusCode::OK) .header(header::CONTENT_TYPE, "text/html; charset=utf-8") .header(header::CACHE_CONTROL, "no-store") @@ -455,12 +534,34 @@ fn exchange_bounce(cookie: &str, mount: &str) -> Response { header::CONTENT_SECURITY_POLICY, "default-src 'none'; style-src 'unsafe-inline'; base-uri 'none'; \ form-action 'none'; frame-ancestors 'none'", - ) - .header(header::SET_COOKIE, cookie) + ); + for cookie in cookies { + bounce = bounce.header(header::SET_COOKIE, cookie); + } + bounce .body(Body::from(exchange_bounce_html(mount))) .unwrap_or_else(|_| StatusCode::INTERNAL_SERVER_ERROR.into_response()) } +/// `"; Secure"` for a cookie set in answer to a request the browser sent to an +/// `https` address, as `auth::browser_used_https` decides it from the request +/// and the address `--public-url` declared; nothing otherwise. Never on a plain +/// `http` address, loopback included: the browser would then keep the cookie +/// off every request there, and `biorouter serve` on `http://127.0.0.1` would +/// refuse its own link. Shared with an app's launch link (`routes::apps`). +pub(crate) fn secure_attribute( + headers: &HeaderMap, + uri: &Uri, + public: Option<&super::PublicUrl>, +) -> &'static str { + let declared = public.map(|public| public.origin().to_string()); + if biorouter_server::auth::browser_used_https(headers, uri, declared.as_deref()) { + "; Secure" + } else { + "" + } +} + /// `mount` is a [`mount_href`], only `.` and `..` segments, so it needs no /// escaping in an attribute. fn exchange_bounce_html(mount: &str) -> String { @@ -487,6 +588,54 @@ fn unauthorized() -> Response { .into_response() } +/// The refusal for a browser that redeemed a valid token at an address other +/// than the one `--public-url` declared ([`WebUi::wrong_address`]): the +/// session cookie is scoped to the declared path, so the token worked and the +/// cookie never came back. Names the declared address, never the token. The +/// address is the operator's, and `ps` already shows it to every account on +/// the machine. +fn wrong_address(public: &super::PublicUrl) -> Response { + let address = match public.path() { + "/" => format!("{}/", public.origin()), + path => format!("{}{path}/", public.origin()), + }; + let address = html_text(&address); + ( + StatusCode::UNAUTHORIZED, + [ + (header::CONTENT_TYPE, "text/html; charset=utf-8"), + (header::CACHE_CONTROL, "no-store"), + ], + format!( + "Biorouter\ + \ +

This is not the address Biorouter was started with

\ +

The access token was accepted, but Biorouter was started for \ + {address}, and its access cookie works only there.

\ +

Open that address with the ?t= part \ + biorouter serve printed. To use this address instead, \ + restart biorouter serve with --public-url set to it.

" + ), + ) + .into_response() +} + +/// `text` safe to place in HTML text or a quoted attribute. +fn html_text(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + for c in text.chars() { + match c { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + c => out.push(c), + } + } + out +} + /// Add the web interface to an already-layered API router. /// /// Call this *after* `check_token` has been applied, so the shell and the @@ -518,6 +667,8 @@ mod tests { index_html: "shell".to_string(), browser_token: token.map(str::to_string), cookie_path: "/".to_string(), + public: None, + exchange_marker: "marker".to_string(), } } @@ -579,8 +730,11 @@ mod tests { header::COOKIE, "other=1; biorouter_session=tok; third=3".parse().unwrap(), ); - assert_eq!(cookie_value(&headers, SESSION_COOKIE), Some("tok")); - assert_eq!(cookie_value(&headers, "absent"), None); + assert_eq!( + cookie_values(&headers, SESSION_COOKIE).collect::>(), + ["tok"] + ); + assert_eq!(cookie_values(&headers, "absent").next(), None); } #[test] @@ -590,7 +744,7 @@ mod tests { header::COOKIE, "evil_biorouter_session=tok".parse().unwrap(), ); - assert_eq!(cookie_value(&headers, SESSION_COOKIE), None); + assert_eq!(cookie_values(&headers, SESSION_COOKIE).next(), None); } /// The daemon secret rides in the document. A cache anywhere between the @@ -776,6 +930,8 @@ mod tests { ), browser_token: Some("tok".into()), cookie_path: "/".to_string(), + public: None, + exchange_marker: "marker".to_string(), }; for forwarded in [None, Some("https")] { for token in [None, Some("tok")] { @@ -870,6 +1026,8 @@ mod tests { ), browser_token: None, cookie_path: "/".to_string(), + public: None, + exchange_marker: "marker".to_string(), }; for (uri, mount) in [("/", "./"), ("/x/y/z", "../../")] { let res = index( @@ -894,6 +1052,33 @@ mod tests { } } + /// A daemon with no token configured accepts any `?t=`, so the exchange + /// must not copy it into a cookie: `x; Domain=evil.test` would have added an + /// attribute, and a CR or LF would have failed the response with a 500. It + /// sets no cookie at all and still moves the browser on. + #[tokio::test] + async fn a_daemon_without_a_token_writes_no_cookie_from_the_url() { + let ui = ui_with(None); + for presented in ["x; Domain=evil.test", "x\r\nX-Injected: 1", "anything"] { + let res = index( + State(ui.clone()), + cookie_request(Some("navigate"), Some("document")), + Query(IndexQuery { + t: Some(presented.into()), + }), + Uri::default(), + ) + .await; + assert_eq!(res.status(), StatusCode::OK, "{presented:?}"); + assert!( + res.headers().get(header::SET_COOKIE).is_none(), + "{presented:?}: {:?}", + res.headers().get_all(header::SET_COOKIE) + ); + assert!(res.headers().get("x-injected").is_none(), "{presented:?}"); + } + } + /// Behind a proxy's path prefix (`biorouter serve --public-url /// https://hub/user/a/proxy/8765/`) the session cookie is scoped to that /// prefix: every JupyterHub user's server shares the hostname, and a cookie @@ -918,6 +1103,257 @@ mod tests { assert!(cookie.contains("; Path=/user/a/proxy/8765; "), "{cookie}"); } + /// Every `Set-Cookie` a response carries, in order. + fn set_cookies(res: &Response) -> Vec { + res.headers() + .get_all(header::SET_COOKIE) + .iter() + .map(|value| value.to_str().unwrap().to_string()) + .collect() + } + + async fn body_text(res: Response) -> String { + let body = axum::body::to_bytes(res.into_body(), 64 * 1024) + .await + .unwrap(); + String::from_utf8(body.to_vec()).unwrap() + } + + /// A navigation through an https proxy to the shell, carrying `cookie`. + fn proxied_navigation(host: &str, cookie: Option<&str>) -> HeaderMap { + let mut headers = HeaderMap::new(); + headers.insert(header::HOST, host.parse().unwrap()); + headers.insert("sec-fetch-mode", "navigate".parse().unwrap()); + headers.insert("sec-fetch-dest", "document".parse().unwrap()); + if let Some(cookie) = cookie { + headers.insert(header::COOKIE, cookie.parse().unwrap()); + } + headers + } + + /// The live test of PR #404 (2026-10-17): `serve --public-url + /// https://hub.test:18443/user/minrk/proxy/9999/` set its cookie at that + /// path; restarted without the flag, `serve` set a fresh one at `/` under the + /// same name. Chrome keeps both and sends the longer path first, so a reader + /// of the first value answered every new link "This link needs its access + /// token". + #[tokio::test] + async fn a_stale_cookie_at_a_longer_path_does_not_shadow_the_fresh_one() { + let ui = ui_with(Some("fresh")); + for headers in [ + proxied_navigation( + "hub.test:18443", + Some("biorouter_session=stale; biorouter_session=fresh"), + ), + // Split across two header lines, as HTTP/2 allows. + { + let mut headers = + proxied_navigation("hub.test:18443", Some("other=1; biorouter_session=stale")); + headers.append(header::COOKIE, "biorouter_session=fresh".parse().unwrap()); + headers + }, + ] { + let res = index( + State(ui.clone()), + headers, + Query(IndexQuery { t: None }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::OK); + } + // Neither stale value alone, nor a pair of wrong ones, opens it. + let res = index( + State(ui), + proxied_navigation( + "hub.test:18443", + Some("biorouter_session=stale; biorouter_session=older"), + ), + Query(IndexQuery { t: None }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::UNAUTHORIZED); + } + + /// The cookie names `Secure` exactly when the browser is at an `https` + /// address, and never on plain `http` to loopback, where the browser would + /// then never send it back and `serve` on `http://127.0.0.1` would refuse + /// its own link. + #[tokio::test] + async fn the_session_cookie_is_secure_only_behind_https() { + let declared = crate::routes::PublicUrl::parse("https://hub.test:18443/u/a/").unwrap(); + let exchange = |public: Option, headers: HeaderMap| { + let ui = WebUi { + cookie_path: public + .as_ref() + .map_or("/", crate::routes::PublicUrl::path) + .to_string(), + public, + ..ui_with(Some("tok")) + }; + async move { + let res = index( + State(ui), + headers, + Query(IndexQuery { + t: Some("tok".into()), + }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::OK); + set_cookies(&res) + } + }; + let rewritten_by_a_proxy = { + let mut headers = proxied_navigation("127.0.0.1:8765", None); + headers.insert("x-forwarded-for", "10.0.0.7".parse().unwrap()); + headers + }; + for (case, public, headers, secure) in [ + ( + "loopback http", + None, + proxied_navigation("127.0.0.1:8765", None), + false, + ), + ( + "loopback http with an https address declared", + Some(declared.clone()), + proxied_navigation("127.0.0.1:8765", None), + false, + ), + ( + "an https proxy that keeps Host", + None, + proxied_navigation("biorouter.example.org", None), + true, + ), + ( + "the declared https address", + Some(declared.clone()), + proxied_navigation("hub.test:18443", None), + true, + ), + ( + "a proxy that rewrites Host, https declared", + Some(declared.clone()), + rewritten_by_a_proxy.clone(), + true, + ), + ( + "a proxy that rewrites Host, nothing declared", + None, + rewritten_by_a_proxy, + false, + ), + ] { + let cookies = exchange(public, headers).await; + assert!(!cookies.is_empty(), "{case}"); + for cookie in cookies { + assert_eq!(cookie.ends_with("; Secure"), secure, "{case}: {cookie}"); + } + } + } + + /// `serve --public-url https://hub.test:18443/user/minrk/biorouter/` + /// opened at `/user/minrk/proxy/9999/` instead: the token is valid and the + /// cookie, scoped to the declared path, never comes back. The refusal says + /// which address to open rather than that the link lacks its token. + #[tokio::test] + async fn a_valid_token_redeemed_at_another_address_names_the_declared_one() { + let public = + crate::routes::PublicUrl::parse("https://hub.test:18443/user/minrk/biorouter/") + .unwrap(); + let ui = WebUi { + cookie_path: public.path().to_string(), + public: Some(public), + ..ui_with(Some("c0ffee5ecre7")) + }; + let exchanged = index( + State(ui.clone()), + proxied_navigation("hub.test:18443", None), + Query(IndexQuery { + t: Some("c0ffee5ecre7".into()), + }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(exchanged.status(), StatusCode::OK); + let cookies = set_cookies(&exchanged); + assert_eq!(cookies.len(), 2, "{cookies:?}"); + assert!( + cookies[0].starts_with("biorouter_session=c0ffee5ecre7; Path=/user/minrk/biorouter;"), + "{cookies:?}" + ); + // The marker carries no `Path`, so the browser scopes it to where it + // really is, and none of the token. + assert!( + cookies[1].starts_with("biorouter_exchanged=marker;"), + "{cookies:?}" + ); + assert!(!cookies[1].contains("Path="), "{cookies:?}"); + assert!(!cookies[1].contains("c0ffee5ecre7"), "{cookies:?}"); + + // Back with the marker and no session cookie: outside the declared path. + let res = index( + State(ui.clone()), + proxied_navigation("hub.test:18443", Some("biorouter_exchanged=marker")), + Query(IndexQuery { t: None }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::UNAUTHORIZED); + assert_eq!(res.headers()[header::CACHE_CONTROL], "no-store"); + let body = body_text(res).await; + assert!( + body.contains("not the address Biorouter was started with"), + "{body}" + ); + assert!( + body.contains("https://hub.test:18443/user/minrk/biorouter/"), + "{body}" + ); + assert!(!body.contains("c0ffee5ecre7"), "{body}"); + + // A marker of another daemon run, or none, gets the usual answer. + for cookie in [Some("biorouter_exchanged=another"), None] { + let res = index( + State(ui.clone()), + proxied_navigation("hub.test:18443", cookie), + Query(IndexQuery { t: None }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::UNAUTHORIZED); + assert!( + body_text(res).await.contains("needs its access token"), + "{cookie:?}" + ); + } + + // At the root a cookie always comes back, so no marker is set. + let res = index( + State(ui_with(Some("tok"))), + proxied_navigation("hub.test:18443", None), + Query(IndexQuery { + t: Some("tok".into()), + }), + "/".parse().unwrap(), + ) + .await; + assert_eq!(set_cookies(&res).len(), 1); + } + + #[test] + fn html_text_escapes_what_a_url_path_may_hold() { + assert_eq!( + html_text("https://h/a&b'c\""), + "https://h/a&b'c"<d>" + ); + } + #[tokio::test] async fn authority_without_a_host_header_still_checks_the_shell_address() { let res = index( @@ -939,6 +1375,8 @@ mod tests { index_html: inject_runtime_config("", "s3cr3t"), browser_token: Some("tok".into()), cookie_path: "/".to_string(), + public: None, + exchange_marker: "marker".to_string(), }; for (mode, dest) in [ (Some("cors"), Some("empty")), diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index e5614e857..4e9f5fe0d 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -2495,6 +2495,84 @@ fn provider_retry_notice(error: &ProviderError, attempt: u32, limit: u32) -> Str format!("The model call failed: {reason}. Trying again ({attempt}/{limit})…") } +/// What becomes of text a local provider held back as a possible tool call +/// and released only because an error cut it +/// (`text_tool_calls::is_released_at_error`), once `next`, that error, is read. +/// +/// Nothing, when the error will be retried: nobody has seen the text, and it +/// is most often part of a call written as text. Live, against a scripted +/// llama-server, `{"name": "developer__shell", "arguments": {"comm` and the +/// shared-cache frame were shown and saved as a reply, and the retry, told to +/// continue that reply, wrote the rest of the JSON as text. Dropped, nothing +/// reaches the conversation and the retry is the same request. After any +/// other error, including one that ends the turn, the text is shown and kept +/// without its mark, as before, so the person sees what the model wrote. +fn released_at_error_outcome( + held: Message, + next: &Result, + mistakes: &crate::agents::mistakes::MistakeTracker, + config: &crate::agents::mistakes::MistakeConfig, +) -> Option { + match next { + Err(error) if mistakes.would_retry(config, error) => None, + _ => Some(crate::providers::formats::text_tool_calls::without_released_at_error_mark(held)), + } +} + +#[cfg(test)] +mod released_at_error_outcome_tests { + use super::*; + use crate::agents::mistakes::{MistakeConfig, MistakeTracker}; + + /// Text released at an error, as `text_tool_calls` marks it. + fn released() -> Message { + let mut message = Message::assistant().with_text("{\"name\": \"developer__shell\""); + if let Some(MessageContent::Text(text)) = message.content.first_mut() { + let mut meta = rmcp::model::Meta::new(); + meta.insert( + crate::providers::formats::text_tool_calls::RELEASED_AT_ERROR_KEY.to_string(), + serde_json::Value::Bool(true), + ); + text.meta = Some(meta); + } + assert!(crate::providers::formats::text_tool_calls::is_released_at_error(&message)); + message + } + + #[test] + fn dropped_only_when_the_error_will_be_retried() { + let config = MistakeConfig::default(); + let fresh = MistakeTracker::default(); + let transient = Err(ProviderError::ServerError( + "Context size has been exceeded.".into(), + )); + assert_eq!( + released_at_error_outcome(released(), &transient, &fresh, &config), + None + ); + + let fatal = Err(ProviderError::Authentication("bad key".into())); + let kept = released_at_error_outcome(released(), &fatal, &fresh, &config).expect("shown"); + assert!(!crate::providers::formats::text_tool_calls::is_released_at_error(&kept)); + assert_eq!(kept.as_concat_text(), "{\"name\": \"developer__shell\""); + + let mut spent = MistakeTracker::default(); + let _ = spent.observe_provider_error( + &config, + &ProviderError::ServerError("first".into()), + crate::agents::mistakes::RetryPoint::Resend, + ); + assert!( + released_at_error_outcome(released(), &transient, &spent, &config).is_some(), + "a spent budget ends the turn, so the text is shown" + ); + assert!( + released_at_error_outcome(released(), &Ok((None, None, None)), &fresh, &config) + .is_some() + ); + } +} + #[cfg(test)] mod provider_retry_notice_tests { use super::*; @@ -11510,6 +11588,10 @@ impl Agent { // failure. The terminal event is emitted only after usage and // conversation state are durable. let mut pending_turn_abort: Option<(TurnAbortCode, String)> = None; + // Text a local provider held back as a possible tool call and + // released only because an error cut it. It waits for that + // error, the stream's next item: see `released_at_error_outcome`. + let mut released_at_error: Option> = None; // #107: race the provider's own output against a request for a // human decision, exactly as the tool-batch loop below does and @@ -11649,6 +11731,27 @@ impl Agent { break; } + if let Some(held) = released_at_error.take() { + if let Some(message) = + released_at_error_outcome(*held, &next, &mistakes, &mistake_config) + { + let message = named(message); + made_user_visible_progress |= message_has_user_visible_progress(&message); + ever_made_user_visible_progress |= made_user_visible_progress; + yield AgentEvent::Message(message.clone()); + messages_to_add.push(message); + } + } + let next = match next { + Ok((Some(response), None, None)) + if crate::providers::formats::text_tool_calls::is_released_at_error(&response) => + { + released_at_error = Some(Box::new(response)); + continue; + } + next => next, + }; + match next { Ok((response, usage, pending)) => { // A pending tool-call notification: the model has @@ -12635,6 +12738,17 @@ impl Agent { } } } + // The loop left (a cancel, a steer restart) before the error + // behind released text was read: it is kept, as before. + if let Some(held) = released_at_error.take() { + let message = named( + crate::providers::formats::text_tool_calls::without_released_at_error_mark(*held), + ); + made_user_visible_progress |= message_has_user_visible_progress(&message); + ever_made_user_visible_progress |= made_user_visible_progress; + yield AgentEvent::Message(message.clone()); + messages_to_add.push(message); + } // D13: the provider call is over, so it owes no more // acknowledgements. Land the ones that already arrived — one diff --git a/crates/biorouter/src/agents/mistakes.rs b/crates/biorouter/src/agents/mistakes.rs index 786e0686a..87e07620d 100644 --- a/crates/biorouter/src/agents/mistakes.rs +++ b/crates/biorouter/src/agents/mistakes.rs @@ -198,12 +198,23 @@ pub enum RetryPoint { /// inserted as its own user row after trailing tool results, so the request as /// sent never ends on one. `added` is what this iteration produced before it /// failed. +/// +/// Whitespace alone is not a partial reply, as it is not an answer anywhere +/// else in the stream decoding (R14): a first call that streamed `"\n\n"` and +/// then failed used to be told to continue a reply it had not begun. pub fn retry_point( conversation: &[crate::conversation::message::Message], added: &[crate::conversation::message::Message], ) -> RetryPoint { use crate::conversation::message::MessageContent; - if added.iter().any(|message| message.is_agent_visible()) { + let began_a_reply = added.iter().any(|message| { + message.is_agent_visible() + && message.content.iter().any(|content| match content { + MessageContent::Text(text) => !text.text.trim().is_empty(), + _ => true, + }) + }); + if began_a_reply { return RetryPoint::AfterPartialReply; } let follows_tool_results = conversation @@ -356,6 +367,15 @@ impl MistakeTracker { self.provider_errors = 0; } + /// Whether [`Self::observe_provider_error`] would retry `error` now. The + /// reply loop asks before it records the error, to decide what becomes of + /// text that was released only because the error cut it. + pub fn would_retry(&self, config: &MistakeConfig, error: &ProviderError) -> bool { + config.enabled + && is_recoverable(error) + && self.provider_errors < config.provider_error_retries + } + /// A provider call failed. Decide whether this reply gets another attempt. pub fn observe_provider_error( &mut self, @@ -364,7 +384,7 @@ impl MistakeTracker { point: RetryPoint, ) -> ProviderErrorAction { let spent = self.provider_errors; - if !config.enabled || !is_recoverable(error) || spent >= config.provider_error_retries { + if !self.would_retry(config, error) { return ProviderErrorAction::Stop { notice: stop_notice(error, spent), }; @@ -502,16 +522,25 @@ fn recovery_notice( {NOT_FROM_THE_USER}" )), RetryPoint::AfterToolResults => Some(format!( - "The model call after the tool results above did not complete: {error}. No tool ran \ + "The model call after the tool results above did not complete: {}. No tool ran \ in it, and Biorouter is retrying ({attempt}/{limit}). If the failure looks like \ something about the request itself (an oversized or malformed payload, an \ unsupported argument), take a smaller or simpler next step than the one you were \ about to take; otherwise continue the user's task as you would have. \ - {NOT_FROM_THE_USER}" + {NOT_FROM_THE_USER}", + without_end_stop(&error.to_string()) )), } } +/// `text` without the full stop it ends in, for a frame that supplies its own: +/// the shared-cache error ends "as Biorouter's own Llama Server does.", and +/// the note read "does.. No tool ran". The user-facing retry notice trims the +/// same way. +fn without_end_stop(text: &str) -> &str { + text.trim_end().trim_end_matches(['.', '!', '?']).trim_end() +} + /// The user-facing message when the turn ends on a provider error. /// /// ⚠ **The retry invitation is only offered for an error a retry could survive.** @@ -940,6 +969,86 @@ mod tests { assert_eq!(retry_point(&asked, &shown_only_partial), RetryPoint::Resend); } + /// Live against a scripted llama-server, a first call that streamed only + /// "\n\n" before the shared-cache frame was told to continue a reply it had + /// not begun. Whitespace is not a reply (R14), so the retry is a re-send. + #[test] + fn whitespace_alone_is_not_a_partial_reply() { + use crate::conversation::message::Message; + + let asked = vec![Message::user().with_text("test")]; + let blank = vec![ + Message::assistant().with_text("\n\n"), + Message::assistant().with_text(" \t"), + ]; + assert_eq!(retry_point(&asked, &blank), RetryPoint::Resend); + + let mut mid_task = asked.clone(); + mid_task.push(Message::user().with_tool_response( + "call-1", + Ok(CallToolResult::success(vec![Content::text("done")])), + )); + assert_eq!(retry_point(&mid_task, &blank), RetryPoint::AfterToolResults); + + let begun = vec![Message::assistant().with_text("\n\nThe")]; + assert_eq!(retry_point(&asked, &begun), RetryPoint::AfterPartialReply); + } + + /// The error text ends in a full stop of its own, and the note's frame + /// supplies one, which read "…as Biorouter's own Llama Server does.. No + /// tool ran in it". + #[test] + fn the_mid_task_note_has_one_full_stop_after_the_error() { + let error = ProviderError::ServerError( + "A server that runs one request at a time cannot hit this: start it with \ + --parallel 1, as Biorouter's own Llama Server does." + .to_string(), + ); + let notice = recovery_notice(&error, RetryPoint::AfterToolResults, 1, 1).expect("a note"); + assert!( + notice.contains("Llama Server does. No tool ran in it"), + "{notice}" + ); + assert!(!notice.contains(".."), "{notice}"); + + let bare = ProviderError::ServerError("502".to_string()); + let notice = recovery_notice(&bare, RetryPoint::AfterToolResults, 1, 1).expect("a note"); + assert!( + notice.contains("Server error: 502. No tool ran"), + "{notice}" + ); + } + + /// `would_retry` is the decision `observe_provider_error` makes, asked + /// without spending the budget. + #[test] + fn would_retry_answers_as_the_error_is_then_handled() { + let config = MistakeConfig::default(); + let mut tracker = MistakeTracker::default(); + let transient = ProviderError::ServerError("Context size has been exceeded.".to_string()); + let fatal = ProviderError::Authentication("bad key".to_string()); + + assert!(!tracker.would_retry(&config, &fatal)); + assert!(!tracker.would_retry(&MistakeConfig::disabled(), &transient)); + assert!(tracker.would_retry(&config, &transient)); + assert!( + tracker.would_retry(&config, &transient), + "asking spends nothing" + ); + assert!(matches!( + tracker.observe_provider_error(&config, &transient, RetryPoint::Resend), + ProviderErrorAction::Recover { .. } + )); + assert!( + !tracker.would_retry(&config, &transient), + "the budget is spent" + ); + assert!(matches!( + tracker.observe_provider_error(&config, &transient, RetryPoint::Resend), + ProviderErrorAction::Stop { .. } + )); + } + #[test] fn a_fatal_provider_error_is_never_retried() { let config = MistakeConfig::default(); diff --git a/crates/biorouter/src/agents/moim.rs b/crates/biorouter/src/agents/moim.rs index 5da42a16c..436f4e9d3 100644 --- a/crates/biorouter/src/agents/moim.rs +++ b/crates/biorouter/src/agents/moim.rs @@ -72,6 +72,26 @@ fn strip_existing_moim(messages: &mut Vec) { const MOIM_PREAMBLE: &str = "Automatic context from Biorouter, not a message from the user. \ Do not reply to it; continue the user's task."; +/// What normalizing the conversation with the block in it may report without +/// the block being dropped: the merges the block's own user row causes, and +/// two cleanups of rows the conversation already holds (trailing whitespace +/// trimmed from an assistant reply, and a row that is empty, or left empty by +/// that trim, removed). A request sent without the block is normalized the +/// same way, so neither says anything about where the block went. +/// +/// The cleanups used to drop the block. A reply cut off by a retried server +/// error is kept, and it often ends in whitespace (`"The quick brown fox "`), +/// so the retry was sent without it (live, against a scripted llama-server). +/// The same went for every later request in a chat whose stored history has +/// an assistant reply ending in a newline. Anything else, such as a tool +/// request or response the block's position orphaned, still drops it. +const EXPECTED_ISSUES: [&str; 4] = [ + "Merged consecutive user messages", + "Merged consecutive assistant messages", + "Trimmed trailing whitespace from assistant message", + "Removed empty message", +]; + /// Put [`MOIM_PREAMBLE`] first in the block, directly after the opening tag and /// ahead of any reminder, so the size cap (which keeps the head) never removes it. fn with_preamble(moim: String) -> String { @@ -151,8 +171,9 @@ pub async fn inject_moim( let (fixed, issues) = normalizer.normalize(Conversation::new_unvalidated(messages)); let has_unexpected_issues = issues.iter().any(|issue| { - !issue.contains("Merged consecutive user messages") - && !issue.contains("Merged consecutive assistant messages") + !EXPECTED_ISSUES + .iter() + .any(|expected| issue.contains(expected)) }); if has_unexpected_issues { @@ -492,6 +513,85 @@ mod tests { assert_eq!(block.matches(MOIM_PREAMBLE).count(), 1, "{block}"); } + /// A reply cut off by a retried server error is kept, and it often ends in + /// whitespace. Trimming it used to drop the block from the retry (live, + /// against a scripted llama-server: "MOIM injection caused unexpected + /// issues: [Trimmed trailing whitespace from assistant message, ...]"), and + /// from every later request of a chat whose history holds such a reply. + #[tokio::test] + async fn trailing_whitespace_in_a_reply_keeps_the_block() { + let temp_dir = tempfile::tempdir().unwrap(); + let em = ExtensionManager::new_without_provider(temp_dir.path().to_path_buf()); + let note = Message::user() + .with_text("\ncontinue\n") + .with_visibility(false, true); + for conversation in [ + // The retry after a cut-off reply. + vec![ + Message::user().with_text("Write one sentence about foxes."), + Message::assistant().with_text("The quick brown fox "), + note.clone(), + ], + // A reply that was only whitespace. + vec![ + Message::user().with_text("test"), + Message::assistant().with_text("\n\n"), + note, + ], + // A later turn, after a stored reply that ends in a newline. + vec![ + Message::user().with_text("Write two lines about foxes."), + Message::assistant().with_text("Foxes are quick.\nFoxes are clever.\n"), + Message::user().with_text("Thanks"), + ], + ] { + let (result, injected) = inject_moim( + "test-session-id", + Conversation::new_unvalidated(conversation), + &em, + &PathBuf::from("/test/dir"), + &SharedNormalizer::new(), + None, + None, + ) + .await; + assert!(injected, "{:?}", result.messages()); + assert_eq!(count_info_msgs(&result), 1); + } + } + + /// A cleanup the block's position can cause still drops it: here the + /// normalizer removes a tool request nothing answers. + #[tokio::test] + async fn an_orphaned_tool_request_still_drops_the_block() { + let temp_dir = tempfile::tempdir().unwrap(); + let em = ExtensionManager::new_without_provider(temp_dir.path().to_path_buf()); + let conv = Conversation::new_unvalidated(vec![ + Message::user().with_text("Search for something"), + Message::assistant().with_tool_request( + "search_1", + Ok(CallToolRequestParams { + task: None, + name: "search".into(), + arguments: None, + meta: None, + }), + ), + Message::user().with_text("never mind"), + ]); + let (_, injected) = inject_moim( + "test-session-id", + conv, + &em, + &PathBuf::from("/test/dir"), + &SharedNormalizer::new(), + None, + None, + ) + .await; + assert!(!injected); + } + /// BR-2: a cap of 0 disables MOIM truncation. #[test] fn test_cap_moim_block_disabled_with_zero() { diff --git a/crates/biorouter/src/providers/formats/text_tool_calls.rs b/crates/biorouter/src/providers/formats/text_tool_calls.rs index 0d00e9a36..cb832222e 100644 --- a/crates/biorouter/src/providers/formats/text_tool_calls.rs +++ b/crates/biorouter/src/providers/formats/text_tool_calls.rs @@ -81,6 +81,9 @@ use std::borrow::Cow; pub const RECOVERED_FROM_TEXT_KEY: &str = "recovered_from_text"; /// Metadata key naming which leak shape (L1-L5) a recovered call came from. pub const RECOVERED_FORMAT_KEY: &str = "recovered_format"; +/// `_meta` key on the text [`recover_leaked_tool_calls`] releases only because +/// the stream failed while it was held back; see [`is_released_at_error`]. +pub const RELEASED_AT_ERROR_KEY: &str = "released_at_error"; /// TIER2's prose limit: the observed L3/L4 preambles were 50-70 characters. const TIER2_MAX_PROSE_CHARS: usize = 400; @@ -1079,7 +1082,10 @@ enum Mode { /// - At the turn's usage item — the complete-turn signal, carrying the finish /// reason — held text is recovered if the finish reason is not `length` /// (G3); otherwise, or if recovery fails, it is released as one text item. -/// - An error releases held text first, so a partial reply is not lost. +/// - An error releases held text first, so a partial reply is not lost, but +/// marked ([`is_released_at_error`]): nobody has seen it, and whether it +/// should be seen depends on whether the error is retried, which only the +/// agent knows. Whitespace alone is dropped (R14). // One `try_stream!` state machine, like the stream decoders beside it: the // hold-back state (mode, held text, whether a structured call was seen, the // reply id) is the generator's locals, read on every arm. @@ -1097,8 +1103,9 @@ pub fn recover_leaked_tool_calls( let item = match stream.next().await { None => break, Some(Err(error)) => { - if !held.is_empty() { - yield text_item(std::mem::take(&mut held), message_id.as_deref()); + if !held.trim().is_empty() { + let text = held.trim_start().to_string(); + yield (Some(released_at_error(text_message(text, message_id.as_deref()))), None, None); } Err(error)?; break; @@ -1313,6 +1320,54 @@ fn text_item(text: String, id: Option<&str>) -> ProviderStreamItem { (Some(text_message(text, id)), None, None) } +/// `message` with every text item marked [`RELEASED_AT_ERROR_KEY`]. +fn released_at_error(mut message: Message) -> Message { + for content in &mut message.content { + if let MessageContent::Text(text) = content { + text.meta + .get_or_insert_with(rmcp::model::Meta::new) + .insert(RELEASED_AT_ERROR_KEY.to_string(), Value::Bool(true)); + } + } + message +} + +/// Whether `message` is text [`recover_leaked_tool_calls`] held back as a +/// possible tool call and released only because an error followed it, which +/// is always the stream's next item. Nobody has seen it. +/// +/// The agent decides what becomes of it once it reads that error. Retried, the +/// text is dropped: shown and kept, it was a visible, saved fragment of a call +/// (`{"name": "developer__shell", "arguments": {"comm`), and the retry was told +/// to continue it, so the model wrote the rest of the JSON as text. When the +/// error ends the turn, the text is shown and kept like any partial reply, +/// after [`without_released_at_error_mark`]. +pub fn is_released_at_error(message: &Message) -> bool { + !message.content.is_empty() && message.content.iter().all(|content| { + matches!( + content, + MessageContent::Text(text) + if text.meta.as_ref().is_some_and(|meta| meta.contains_key(RELEASED_AT_ERROR_KEY)) + ) + }) +} + +/// `message` without the mark [`is_released_at_error`] reads, so it is stored +/// and shown as the plain text it is. +pub fn without_released_at_error_mark(mut message: Message) -> Message { + for content in &mut message.content { + if let MessageContent::Text(text) = content { + if let Some(meta) = text.meta.as_mut() { + meta.remove(RELEASED_AT_ERROR_KEY); + if meta.is_empty() { + text.meta = None; + } + } + } + } + message +} + #[cfg(test)] mod tests { use super::*; @@ -1788,6 +1843,69 @@ mod tests { ); } + /// `items`, then the in-stream llama-server shared-cache frame, through + /// recovery: what came out ahead of the error, and the error. + async fn run_into_error( + items: Vec, + ) -> (Vec, ProviderError) { + let mut items: Vec> = + items.into_iter().map(Ok).collect(); + items.push(Err(ProviderError::ServerError( + "Context size has been exceeded.".to_string(), + ))); + let mut stream = recover_leaked_tool_calls( + Box::pin(futures::stream::iter(items)), + RecoveryContext { + tools: tools(), + provider: "test", + model: "m".to_string(), + }, + ); + let mut out = Vec::new(); + loop { + match stream.next().await { + Some(Ok(item)) => out.push(item), + Some(Err(error)) => return (out, error), + None => panic!("the error must come through"), + } + } + } + + /// Text held back as a possible call goes out ahead of the error that cut + /// it, marked, because nobody has seen it and only the agent knows whether + /// the error will be retried. Live against a scripted llama-server, the + /// unmarked fragment was shown, saved, and continued by the retry. + #[tokio::test] + async fn an_error_releases_held_text_marked_and_whitespace_not_at_all() { + let fragment = r#"{"name": "developer__shell", "arguments": {"comm"#; + let (out, error) = run_into_error(chunks(fragment)).await; + assert!(matches!(error, ProviderError::ServerError(_)), "{error}"); + assert_eq!(out.len(), 1, "the held text, once, ahead of the error"); + let message = out[0].0.clone().expect("the held text"); + assert!(is_released_at_error(&message)); + assert_eq!(message.id.as_deref(), Some("m1")); + let plain = without_released_at_error_mark(message); + assert!(!is_released_at_error(&plain)); + assert_eq!(plain.as_concat_text(), fragment); + assert!( + plain.content.iter().all( + |content| matches!(content, MessageContent::Text(text) if text.meta.is_none()) + ), + "nothing of the mark is stored" + ); + + // R14: whitespace alone is not a reply, so nothing goes out. + let (out, _) = run_into_error(chunks("\n\n")).await; + assert_eq!(out.len(), 0, "whitespace is not released"); + + // Text already shown was never held, so nothing is marked. + let (out, _) = run_into_error(chunks("Hello there, friend.")).await; + assert!(!out.is_empty()); + assert!(out + .iter() + .all(|(message, _, _)| !message.as_ref().is_some_and(is_released_at_error))); + } + /// L8: a whole-message payload in the template's CALL tag is recovered, /// and the tag is held back while it streams. #[test] diff --git a/crates/biorouter/src/providers/llamacpp_sidecar.rs b/crates/biorouter/src/providers/llamacpp_sidecar.rs index eed36b511..c7caba3f5 100644 --- a/crates/biorouter/src/providers/llamacpp_sidecar.rs +++ b/crates/biorouter/src/providers/llamacpp_sidecar.rs @@ -1327,9 +1327,12 @@ fn backend_failure_message(model: &str, ctx_size: Option, tail: &str) -> {advice}" ) } else { + // No "try again": the chat shows this right before "Trying again + // (1/1)…", while Biorouter is already retrying, and the notice that + // ends a turn adds its own advice. format!( - "Llama Server's GPU backend failed while running {model} and has been restarted; \ - try again.\n\nllama-server output:\n{tail}" + "Llama Server's GPU backend failed while running {model}. Biorouter stopped that \ + server, and the next request starts a fresh one.\n\nllama-server output:\n{tail}" ) } } @@ -3667,6 +3670,26 @@ mod tests { )); } + /// The chat shows a compute failure right before "Trying again (1/1)…", so + /// it must not also tell the person to try again (live, against a scripted + /// llama-server: "…has been restarted; try again. … Trying again (1/1)…"). + #[test] + fn a_compute_failure_does_not_ask_the_person_to_try_again() { + let message = backend_failure_message("qwen3.8-27b", Some(131_072), "E decode: failed"); + assert!( + message.starts_with( + "Llama Server's GPU backend failed while running qwen3.8-27b. Biorouter stopped \ + that server, and the next request starts a fresh one." + ), + "{message}" + ); + assert!(!message.to_lowercase().contains("try again"), "{message}"); + assert!( + message.ends_with("llama-server output:\nE decode: failed"), + "{message}" + ); + } + /// A GPU out-of-memory mid-request offers a smaller window only while there /// is one above the floor, and names the floor when it does. The old advice /// ("set a smaller LLAMACPP_CONTEXT_SIZE") sent a machine already at 64k to diff --git a/crates/biorouter/tests/output_recovery_agent_loop.rs b/crates/biorouter/tests/output_recovery_agent_loop.rs index 7e30f17c5..cdfa80821 100644 --- a/crates/biorouter/tests/output_recovery_agent_loop.rs +++ b/crates/biorouter/tests/output_recovery_agent_loop.rs @@ -20,7 +20,9 @@ use biorouter::conversation::message::{Message, MessageContent}; use biorouter::model::ModelConfig; use biorouter::privacy::affiliation::ModelAffiliation; use biorouter::privacy::ProviderTier; -use biorouter::providers::base::{Provider, ProviderMetadata, ProviderUsage, Usage}; +use biorouter::providers::base::{ + MessageStream, Provider, ProviderMetadata, ProviderStreamItem, ProviderUsage, Usage, +}; use biorouter::providers::errors::{ProviderError, ProviderErrorKind}; use biorouter::providers::utils::map_http_error_to_provider_error; use biorouter::session::session_manager::SessionType; @@ -1257,3 +1259,381 @@ async fn reasoning_only_max_tokens_replays_signed_reasoning_once_and_finishes() let counts = session_manager.get_token_counts(&session_id).await.unwrap(); assert_eq!(counts.accumulated_output_tokens, Some(10)); } + +/// What a tool call written as text looks like when llama-server's shared-cache +/// collision cuts it partway (live, against a scripted llama-server). +const CUT_CALL: &str = r#"{"name": "developer__shell", "arguments": {"comm"#; +const CUT_ANSWER: &str = "The command printed hi."; + +/// A streaming provider whose first `failures` calls stream `first` in small +/// chunks and then fail with llama-server's in-stream shared-cache error, a +/// retryable server fault. With `recover`, the chunks go through the local +/// providers' text recovery, as on Llama Server and Ollama, which holds back +/// text that could still be a tool call. +struct CutStreamProvider { + calls: AtomicUsize, + failures: usize, + first: &'static str, + recover: bool, + /// The agent-visible text of every request, `` block stripped. + sent: Mutex>>, + /// Whether each request carried the `` block. + with_block: Mutex>, +} + +impl CutStreamProvider { + fn new(first: &'static str, failures: usize, recover: bool) -> Self { + Self { + calls: AtomicUsize::new(0), + failures, + first, + recover, + sent: Mutex::default(), + with_block: Mutex::default(), + } + } + + fn record(&self, messages: &[Message]) { + let visible: Vec<&Message> = messages + .iter() + .filter(|message| message.is_agent_visible()) + .collect(); + self.with_block + .lock() + .unwrap() + .push(visible.iter().any(|message| { + message + .as_concat_text() + .contains(biorouter::agents::extension_manager::MOIM_OPEN_TAG) + })); + self.sent.lock().unwrap().push( + visible + .iter() + .map(|message| { + format!( + "{:?}: {}", + message.role, + without_per_request_context(&message.as_concat_text()).trim() + ) + }) + .collect(), + ); + } +} + +#[async_trait] +impl Provider for CutStreamProvider { + async fn complete( + &self, + _system_prompt: &str, + _messages: &[Message], + _tools: &[Tool], + ) -> Result<(Message, ProviderUsage), ProviderError> { + Ok(( + Message::assistant().with_text(CUT_ANSWER), + ProviderUsage::new("mock-model".to_string(), Usage::default()), + )) + } + + async fn complete_with_model( + &self, + _model_config: &ModelConfig, + system_prompt: &str, + messages: &[Message], + tools: &[Tool], + ) -> Result<(Message, ProviderUsage), ProviderError> { + self.complete(system_prompt, messages, tools).await + } + + async fn stream( + &self, + _system: &str, + messages: &[Message], + tools: &[Tool], + ) -> Result { + use biorouter::providers::formats::text_tool_calls::{ + recover_leaked_tool_calls, RecoveryContext, + }; + self.record(messages); + let call = self.calls.fetch_add(1, Ordering::SeqCst); + let id = format!("reply-{call}"); + let items: Vec> = if call < self.failures { + let mut items: Vec> = self + .first + .as_bytes() + .chunks(5) + .map(|chunk| { + let chunk = String::from_utf8(chunk.to_vec()).unwrap(); + Ok(( + Some(Message::assistant().with_id(id.clone()).with_text(chunk)), + None, + None, + )) + }) + .collect(); + items.push(Err(ProviderError::ServerError( + "Context size has been exceeded.".to_string(), + ))); + items + } else { + let mut usage = ProviderUsage::new( + "mock-model".to_string(), + Usage::new(Some(10), Some(5), Some(15)), + ); + usage.finish_reason = Some("stop".to_string()); + vec![Ok(( + Some(Message::assistant().with_id(id).with_text(CUT_ANSWER)), + Some(usage), + None, + ))] + }; + let stream: MessageStream = Box::pin(futures::stream::iter(items)); + Ok(if self.recover { + recover_leaked_tool_calls( + stream, + RecoveryContext { + tools: tools.to_vec(), + provider: "llamacpp", + model: "mock-model".to_string(), + }, + ) + } else { + stream + }) + } + + fn supports_streaming(&self) -> bool { + true + } + + fn get_model_config(&self) -> ModelConfig { + ModelConfig::new_or_fail("mock-model") + } + + fn metadata() -> ProviderMetadata { + AlternatingStormProvider::metadata() + } + + fn get_name(&self) -> &str { + "output-recovery-mock" + } +} + +#[derive(Default)] +struct CutStreamReply { + /// User-visible text and notices, in the order they were yielded. + shown: Vec, + /// The stored conversation's rows, as `"{role}: {text}"`. + stored: Vec, + /// Whether any stored text item still carries a `_meta`. + stored_meta: bool, + aborted: bool, +} + +/// One reply to "Run echo hi in the shell." against `provider`, with one +/// provider-error retry, as shipped. +async fn reply_through_a_cut_stream(provider: Arc) -> CutStreamReply { + let overrides = HashMap::from([ + ( + "BIOROUTER_MISTAKE_STREAK_DETECTION".to_string(), + "true".to_string(), + ), + ( + "BIOROUTER_PROVIDER_ERROR_RETRIES".to_string(), + "1".to_string(), + ), + ]); + with_config_overrides(overrides, async { + let (agent, sessions, session_id, _work, _data, _permissions) = test_agent(provider).await; + let mut reply = CutStreamReply::default(); + tokio::time::timeout(Duration::from_secs(10), async { + let stream = agent + .reply( + Message::user().with_text("Run echo hi in the shell."), + SessionConfig { + id: session_id.clone(), + schedule_id: None, + max_turns: Some(4), + max_tool_calls: None, + budget: None, + retry_config: None, + reasoning_effort: None, + }, + None, + ) + .await + .unwrap(); + tokio::pin!(stream); + while let Some(event) = stream.next().await { + match event.unwrap() { + AgentEvent::Message(message) if message.is_user_visible() => { + for content in &message.content { + match content { + MessageContent::Text(text) => reply.shown.push(text.text.clone()), + MessageContent::SystemNotification(notice) => { + reply.shown.push(notice.msg.clone()) + } + _ => {} + } + } + } + AgentEvent::TurnAborted { .. } => reply.aborted = true, + _ => {} + } + } + }) + .await + .expect("the reply settles"); + let stored = sessions + .get_session(&session_id, true) + .await + .unwrap() + .conversation + .unwrap(); + for message in stored.messages() { + reply + .stored + .push(format!("{:?}: {}", message.role, message.as_concat_text())); + reply.stored_meta |= message.content.iter().any( + |content| matches!(content, MessageContent::Text(text) if text.meta.is_some()), + ); + } + reply + }) + .await +} + +/// Live, against a scripted llama-server: the fragment was shown and saved as a +/// reply, the retry was told to "continue the reply from exactly where it +/// stopped", and the model wrote the rest of the JSON as text. Nobody had seen +/// the fragment (it was held back as a possible call), so a retried error now +/// drops it, and the retry is the request that was sent. +#[tokio::test] +async fn a_call_cut_by_a_retried_error_is_neither_shown_nor_kept() { + let provider = Arc::new(CutStreamProvider::new(CUT_CALL, 1, true)); + let reply = reply_through_a_cut_stream(provider.clone()).await; + + assert_eq!(provider.calls.load(Ordering::SeqCst), 2); + let sent = provider.sent.lock().unwrap().clone(); + assert_eq!(sent[0], sent[1], "the retry re-sends the same request"); + assert!( + sent[1] + .iter() + .all(|row| !row.contains("biorouter-loop-guard")), + "{sent:?}" + ); + assert!( + reply + .shown + .iter() + .all(|text| !text.contains("developer__shell")), + "{:?}", + reply.shown + ); + assert!(reply + .shown + .iter() + .any(|text| text.contains("Trying again (1/1)"))); + assert!(reply.shown.iter().any(|text| text == CUT_ANSWER)); + assert!(!reply.aborted); + assert!( + reply + .stored + .iter() + .all(|row| !row.contains("developer__shell") && !row.contains("biorouter-loop-guard")), + "{:?}", + reply.stored + ); +} + +/// When the error ends the turn, the person still sees what the model wrote, +/// ahead of the notice, and it is stored as plain text. +#[tokio::test] +async fn a_call_cut_by_an_error_that_ends_the_turn_is_shown_before_the_notice() { + let provider = Arc::new(CutStreamProvider::new(CUT_CALL, 2, true)); + let reply = reply_through_a_cut_stream(provider.clone()).await; + + assert_eq!(provider.calls.load(Ordering::SeqCst), 2); + let sent = provider.sent.lock().unwrap().clone(); + assert_eq!(sent[0], sent[1], "the first retry was still a re-send"); + assert!(reply.aborted); + let fragment = reply + .shown + .iter() + .position(|text| text == CUT_CALL) + .unwrap_or_else(|| panic!("the fragment is shown: {:?}", reply.shown)); + let notice = reply + .shown + .iter() + .position(|text| text.starts_with("Ran into this error")) + .unwrap_or_else(|| panic!("the stop notice: {:?}", reply.shown)); + assert!(fragment < notice, "{:?}", reply.shown); + assert_eq!( + reply + .shown + .iter() + .filter(|text| text.contains("developer__shell")) + .count(), + 1, + "the first attempt's fragment was dropped: {:?}", + reply.shown + ); + assert!( + reply + .stored + .iter() + .any(|row| row == &format!("Assistant: {CUT_CALL}")), + "{:?}", + reply.stored + ); + assert!(!reply.stored_meta, "the mark is not stored"); +} + +/// Live, against a scripted llama-server: a first call that streamed only +/// "\n\n" was told to continue a reply it had not begun, and the retry went +/// without the `` block because trimming that row counted as an +/// unexpected issue. Through a provider without text recovery, so the +/// whitespace reaches the conversation. +#[tokio::test] +async fn a_whitespace_partial_is_a_resend_that_keeps_the_block() { + let provider = Arc::new(CutStreamProvider::new("\n\n", 1, false)); + let reply = reply_through_a_cut_stream(provider.clone()).await; + + assert_eq!(provider.calls.load(Ordering::SeqCst), 2); + let sent = provider.sent.lock().unwrap().clone(); + assert!( + sent[1] + .iter() + .all(|row| !row.contains("biorouter-loop-guard")), + "{sent:?}" + ); + assert_eq!(sent[0], sent[1], "the retry re-sends the same request"); + assert_eq!( + provider.with_block.lock().unwrap().as_slice(), + [true, true], + "both requests carry the block" + ); + assert!(reply.shown.iter().any(|text| text == CUT_ANSWER)); +} + +/// The retry after a cut-off reply that ends in a space keeps the block. +#[tokio::test] +async fn a_partial_reply_ending_in_whitespace_keeps_the_block() { + let provider = Arc::new(CutStreamProvider::new("The quick brown fox ", 1, false)); + let reply = reply_through_a_cut_stream(provider.clone()).await; + + assert_eq!(provider.calls.load(Ordering::SeqCst), 2); + let sent = provider.sent.lock().unwrap().clone(); + assert!( + sent[1] + .iter() + .any(|row| row.contains("biorouter-loop-guard")), + "a cut-off reply is still continued: {sent:?}" + ); + assert_eq!( + provider.with_block.lock().unwrap().as_slice(), + [true, true], + "both requests carry the block" + ); + assert!(reply.shown.iter().any(|text| text == CUT_ANSWER)); +} diff --git a/docs/deployment/browser-access.md b/docs/deployment/browser-access.md index 4365b5b1f..7276f39c2 100644 --- a/docs/deployment/browser-access.md +++ b/docs/deployment/browser-access.md @@ -109,8 +109,9 @@ carries a credential instead. `?t=` parameter, and different every time. It is shown once, in the terminal; there is nowhere else to read it back from. - **Opening the address exchanges it for a cookie.** The daemon validates the token, sets an - `HttpOnly`, `SameSite=Strict` session cookie named `biorouter_session`, and answers a short page - that moves the browser on to `/`. The token then disappears from the address bar and from the + `HttpOnly`, `SameSite=Strict` session cookie named `biorouter_session` (also `Secure` when the + browser reached the address over `https`), and answers a short page that moves the browser on + to `/`. The token then disappears from the address bar and from the tab's back and forward list, and it is not in the `Referer` of anything the page later loads. A page rather than a redirect, because a redirect would lose the cookie when the address was opened from a file, which is how `--open` opens it. @@ -221,7 +222,8 @@ biorouter serve --port 8765 --public-url https://hub.example.org/biorouter/ page whose address matches the `Host` and `X-Forwarded-Proto` headers the daemon receives, which a chain of proxies may not preserve. 3. The access cookie is limited to that path, so it is not sent to other servers on the same - hostname. + hostname. A valid link opened at any other address therefore cannot keep you signed in, and the + page says *This is not the address Biorouter was started with* and names the declared one. The proxy must pass the browser's `Sec-Fetch-*` request headers through, which proxies do unless configured to remove them. The browser sends them only to an `https` or loopback address, and the @@ -333,6 +335,7 @@ differs: | Resetting app data (Settings → App → Danger zone → Reset data) | **Not available, and it says so before you try.** A reset deletes chats, knowledge bases and the rest for good, private ones included, so the daemon performs one only for a request that proves a person asked, which only the desktop application can send. The Reset data button is disabled, with a note under it saying why. On the machine running `biorouter serve`, reset from Settings in the Biorouter app there, or remove items from a terminal there with `biorouter session remove`, `schedule remove`, `skill remove` and `extension remove` ([SD-8](serve-decisions.md#sd-8--a-control-that-can-never-work-here-says-so-rather-than-failing-on-click)). | | Model and provider selection | **Not available.** See [The model is fixed before you start](#the-model-is-fixed-before-you-start). | | Crew | **Not available, and it says so before you try.** Opening **Crew** in the browser shows "Crew needs the Biorouter desktop app", and nothing on that page asks the daemon for Crew data. A chat shows no Crew bar. Every Crew action, even listing saved workspaces, needs proof that a person is at this computer, which the desktop application and `biorouter crew` supply themselves; the daemon `biorouter serve` starts never holds it. A Crew request that reaches it anyway is refused with `crew_human_authority_unavailable` and the sentence "Crew isn't available in a browser opened with biorouter serve…", and no setting in the browser changes that. Use Crew in the desktop application, or with `biorouter crew` in a terminal, on your own computer. See the [Crew user manual](../crew/README.md) and [SD-8](serve-decisions.md#sd-8--a-control-that-can-never-work-here-says-so-rather-than-failing-on-click). | +| Browsing the marketplace (Browse extensions, Browse skills) | Works. The page reads the live catalogue from `https://biorouter.ucsf.edu/registry.json` itself, so the browser, not the serving machine, has to reach that site. When it cannot, the catalogue bundled with Biorouter is shown, and the dialog says *showing bundled catalog (offline)*. Installs download through the serving machine. | | File and folder pickers | No native dialog. You type a path, and it is a path **on the machine running the daemon**, not on the machine holding the browser. | | Artifacts and diagnostics bundles | The artifact side panel works as usual. Opening an artifact outside the panel opens a new tab; a diagnostics bundle downloads as a file. | | Sending a region of the preview to the chat, and the agent's screenshot of the panel | **Not available, and it says so before you try.** Both are a picture taken by the desktop application's window, the only thing that can see into the sandboxed frames most previews draw in, and a browser page has no such window. The camera button in the panel's header is disabled and its tooltip says why; an agent that asks for a screenshot is told to read the panel's text instead, which works ([SD-8](serve-decisions.md#sd-8--a-control-that-can-never-work-here-says-so-rather-than-failing-on-click)). | @@ -409,6 +412,11 @@ chat client shortening the link, or a bookmark saved after the address changed t printed. If the launch has since restarted, the token has changed; read the new one from the terminal. +**The tab says this is not the address Biorouter was started with.** The token is valid, but +`serve` was started with `--public-url` naming another address, and the access cookie works only +under that address's path. Open the address the page names, with the `?t=` part `serve` printed. +To use the address you are on instead, restart `serve` with `--public-url` set to it. + **The page loads but nothing connects.** Check that you are on the address `serve` printed. The live views are WebSockets, and the daemon admits one only from the scheme, host and port the browser used. Behind a reverse proxy, pass that address with `--public-url` (see @@ -418,6 +426,16 @@ set `X-Forwarded-Proto: https`, and a chain of proxies that appends `http` to th every live view. The daemon logs each refusal as `rejected workspace WS: cross-origin connect rejected`, with the origin it was given. +**Behind a proxy, a built app says "No Biorouter backend" and suggests `biorouterd agent`.** +Biorouter is running: it served the app's page. What failed is the app's live connection, a +WebSocket, which the daemon refused for the reason in the entry above. JupyterHub's chain causes +it when `serve` has no `--public-url`, because the scheme reaches the daemon as `https,http`. Pass +`--public-url` with the address in the browser's address bar (see +[Behind a reverse proxy](#behind-a-reverse-proxy)) and reopen the app. The daemon logs each refusal +as `rejected app agent WebSocket: cross-origin connect rejected`. The banner cannot tell a refused +connection from a daemon that is not running, so its advice to start `biorouterd agent` is for an +exported app opened with no daemon behind it. + **Behind a proxy, the page says to open Biorouter through localhost or HTTPS.** The browser's `Sec-Fetch-*` request headers did not arrive. Check that the address in the browser is `https`, and that no proxy in the chain removes those headers. diff --git a/docs/deployment/serve-architecture.md b/docs/deployment/serve-architecture.md index 0974a007f..7cf62970a 100644 --- a/docs/deployment/serve-architecture.md +++ b/docs/deployment/serve-architecture.md @@ -128,10 +128,23 @@ cannot gate the initial document. The exchange is therefore: answers a short page of the daemon's own origin that moves the browser on to the interface's mount (`./`, so it stays under a proxy's path prefix). It is a page and not a redirect because a redirect keeps a `SameSite=Strict` cookie off a navigation another - site started, and `serve --open` starts one from a `file:` page (below). -3. `GET /` with that cookie returns the shell, with the daemon's secret injected into it. + site started, and `serve --open` starts one from a `file:` page (below). The cookie is also + `Secure` when the browser reached the address over `https` (`auth::browser_used_https`), and + never on plain `http` to loopback, where the browser would then never send it back. +3. `GET /` with that cookie returns the shell, with the daemon's secret injected into it. Any + `biorouter_session` cookie the request carries counts, not only the first: a browser keeps one + per `Path`, sends the longest path first, and an earlier launch with a longer `--public-url` + path leaves one behind. 4. From then on the application presents `X-Secret-Key` exactly as the desktop renderer does. +**A valid link opened at the wrong address.** With `--public-url` naming a path, the session +cookie is scoped to that path, and a browser that redeems the token anywhere else never sends it +back. The proxy strips the path, so the daemon cannot see where the browser is. The exchange +therefore also sets `biorouter_exchanged`, a marker with no `Path` (the browser scopes it to the +address it is really on), a value random per daemon run and nothing of the token. When the marker +comes back without a session cookie, the refusal names the declared address +("This is not the address Biorouter was started with") instead of asking for the token. + **How `--open` hands the address to a browser.** The address carries the token, which works until the daemon stops, and whoever redeems it gets the shell and the daemon's secret with it. A browser opener's arguments can be read by every account on the machine (`ps` on macOS, and diff --git a/ui/desktop/index.html b/ui/desktop/index.html index 3cba2bc81..cdde41c62 100644 --- a/ui/desktop/index.html +++ b/ui/desktop/index.html @@ -27,6 +27,14 @@ --> Biorouter + +