diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index cd1b63c1..beacae2a 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -330,7 +330,8 @@ When to add or change one: protection, or bring back a Host-owned browser. - File delivery recognition is a frontend projection, not a persisted or filesystem-verified registry. Settled turns collect local Markdown links - and supported structured successful write/edit outputs; inline basenames + and supported structured write/edit outputs that recorded a change (a call + whose own error came after the write still counts); inline basenames require an unambiguous observed output. References use the existing workspace file API and containment checks, never browser `file:` navigation. Delivery cards (under the closing reply of a turn) toggle only their own @@ -344,6 +345,11 @@ When to add or change one: profile Web sessions use (`edit_file` and `write_file` record the same patches as `edit` / `write`; a `write_file` from older sessions has none and stays untracked; `changed: false` is no change) as well as `edit` / `write`. + Whether a call changed a file is read from its result details (`patch`, or + the reason none was recorded), never from `isError`: a call can fail after + its write landed (a cancelled `then_run`, a result-preparation failure) and + still carry the change, which must stay listed, undoable and replayable, + while a call that failed before writing records neither and is no change. Browser-renderable pages open in the embedded browser on click, with a system-browser action; both browsers load them from `src/site-preview.ts`, a separate token-scoped diff --git a/apps/web/client/src/state/session.ts b/apps/web/client/src/state/session.ts index 511c28ee..929fe7ef 100644 --- a/apps/web/client/src/state/session.ts +++ b/apps/web/client/src/state/session.ts @@ -368,24 +368,32 @@ const FILE_CHANGE_TOOLS: ReadonlySet = new Set(["edit", "write", "edit_f /** * The file change of a finished edit/write call. `edit`, `write` and Step's - * `edit_file` record a unified patch in their result details; `write` says why - * when it recorded none. Step's `write_file` records no patch (an untracked - * change), and `changed: false` when the file already held the content. + * `edit_file` record a unified patch in their result details, and say why when + * they recorded none. Step's `write_file` records the same patch as `write`; a + * `write_file` from older sessions has neither a patch nor a reason, and both + * report `changed: false` when the file already held the content. * `result` is the recorded tool result, or a live `tool_execution_end`'s * result with the event's own `isError`. + * + * Whether the file was written is answered by the change record, not by + * `isError`: a call can fail after its write landed (a cancelled `then_run`, a + * failure preparing the result) and still record the patch. A call that failed + * before writing records neither a patch nor a reason for its absence. */ function fileChange( name: string, args: unknown, result: { readonly details?: unknown; readonly isError?: boolean } | undefined, ): FileChange | undefined { - if (!FILE_CHANGE_TOOLS.has(name) || !result || result.isError) return undefined; + if (!FILE_CHANGE_TOOLS.has(name) || !result) return undefined; const details = (result.details ?? {}) as { patch?: unknown; untracked?: unknown; changed?: unknown; }; if (details.changed === false) return undefined; + if (result.isError === true && typeof details.patch !== "string" && details.untracked === undefined) + return undefined; const input = (args ?? {}) as { path?: unknown; file_path?: unknown }; const path = typeof input.path === "string" ? input.path : input.file_path; if (typeof path !== "string" || !path.trim()) return undefined; diff --git a/apps/web/src/turn-changes.ts b/apps/web/src/turn-changes.ts index e55fd1b0..287ffe30 100644 --- a/apps/web/src/turn-changes.ts +++ b/apps/web/src/turn-changes.ts @@ -27,8 +27,10 @@ import { /** * File-changing tools: the built-in names and the Step tool profile's, which - * Web sessions use. Step's `edit_file` records the same patch as `edit`; its - * `write_file` records none (skipped) and `changed: false` for a no-op. + * Web sessions use. Step's `edit_file` records the same patch as `edit`, and + * its `write_file` records the same patch (or why it recorded none) as + * `write`; a `write_file` from older sessions has neither. Both report + * `changed: false` for a file that already held the content. */ const FILE_CHANGE_TOOLS: ReadonlySet = new Set(["edit", "write", "edit_file", "write_file"]); @@ -37,6 +39,17 @@ function toolPath(input: Readonly>): string | undefined return typeof value === "string" && value.trim() ? value : undefined; } +/** + * Whether a finished edit/write result records what the call left in the file: + * a patch, or why it recorded none. A call can fail after its write landed (a + * cancelled `then_run`, a failure preparing the result) and still carry that + * record, so `isError` does not answer this; a call that failed before writing + * records neither, and must not be counted as a change. + */ +function recordsAppliedChange(details: WriteToolDetails & { changed?: unknown }): boolean { + return typeof details.patch === "string" || details.untracked !== undefined; +} + /** * Why a revert was refused; `path` is the file as the tool call named it. * `modified` means the file no longer matches the turn's patches and nothing @@ -252,7 +265,7 @@ export function turnRevertNotice( ].join("\n"); } -/** Successful edit/write calls of the turn that starts at the user entry `turnId`, in order. */ +/** Edit/write calls of the turn that starts at the user entry `turnId`, in order, that recorded a change. */ export function turnChanges(entries: readonly SessionEntry[], turnId: string): TurnChangeSet { const start = entries.findIndex((entry) => entry.id === turnId && isUserMessage(entry)); const changes: TurnChange[] = []; @@ -276,10 +289,13 @@ export function turnChanges(entries: readonly SessionEntry[], turnId: string): T calls.set(String(part.id), { name: String(part.name ?? ""), path: toolPath(args) }); } } - if (message.role !== "toolResult" || message.isError || !message.toolCallId) continue; + if (message.role !== "toolResult" || !message.toolCallId) continue; const call = calls.get(message.toolCallId); if (!call?.path || !FILE_CHANGE_TOOLS.has(call.name)) continue; const details = (message.details ?? {}) as WriteToolDetails & { changed?: unknown }; + // A call that failed before writing recorded nothing; one that failed after + // writing still carries the change it applied. + if (message.isError === true && !recordsAppliedChange(details)) continue; // Step's write_file left a file that already held the content untouched. if (details.changed === false) continue; if (typeof details.patch !== "string") { @@ -553,10 +569,13 @@ async function replayFile( calls.set(String(part.id), { name: String(part.name ?? ""), path: toolPath(args), content: args.content }); } } - if (message.role !== "toolResult" || message.isError || !message.toolCallId) continue; + if (message.role !== "toolResult" || !message.toolCallId) continue; const call = calls.get(message.toolCallId); if (!call?.path || !FILE_CHANGE_TOOLS.has(call.name) || !(await sameFile(call.path))) continue; const details = (message.details ?? {}) as WriteToolDetails & { changed?: unknown }; + // A call that failed before writing recorded nothing to replay; one that + // failed after writing left exactly what its record describes. + if (message.isError === true && !recordsAppliedChange(details)) continue; const write = call.name === "write" || call.name === "write_file"; // A write names the whole content, even when the file already held it (`changed: false`). if (write && typeof call.content === "string") content = recordedText(call.content); diff --git a/apps/web/test/client-live-turn-changes.test.ts b/apps/web/test/client-live-turn-changes.test.ts index 0257fa41..5953b60c 100644 --- a/apps/web/test/client-live-turn-changes.test.ts +++ b/apps/web/test/client-live-turn-changes.test.ts @@ -90,12 +90,30 @@ test("a finished edit or write shows in the turn's changes before any snapshot", ]); }); -test("a failed, unchanged or unfinished call adds no change", () => { +test("a failed call that changed the file still shows its change", () => { open(); - runTool("c1", "write_file", { path: "a.json" }, { details: { patch: patch("a.json") } }, true); - runTool("c2", "write_file", { path: "b.json" }, { details: { changed: false } }); - agent({ type: "tool_execution_start", toolCallId: "c3", toolName: "edit_file", args: { path: "c.json" } }); - agent({ type: "tool_execution_end", toolCallId: "c4", toolName: "run_command", result: { details: {} } }); + // A cancelled then_run: the edit landed, the call reports an error. + runTool("c1", "edit_file", { path: "a.ts" }, { details: { patch: patch("a.ts") } }, true); + runTool("c2", "write_file", { path: "b.json" }, { details: { changed: true, patch: patch("b.json") } }, true); + assert.deepEqual(shownFiles(), [ + ["a.ts", 1], + ["b.json", 1], + ]); + // The change shows beside a row that still reads as failed; the two facts are separate. + const states = getState() + .timelineItems.filter((item) => item.kind === "tool") + .map((item) => item.state); + assert.deepEqual(states, ["failed", "failed"]); +}); + +test("a failed call that never wrote adds no change", () => { + open(); + // The search string did not match, so the call recorded no change. + runTool("c1", "edit_file", { path: "a.ts" }, { details: { changed: false } }, true); + runTool("c2", "write_file", { path: "b.json" }, {}, true); + runTool("c3", "write_file", { path: "d.json" }, { details: { changed: false } }); + agent({ type: "tool_execution_start", toolCallId: "c4", toolName: "edit_file", args: { path: "c.json" } }); + agent({ type: "tool_execution_end", toolCallId: "c5", toolName: "run_command", result: { details: {} } }); assert.deepEqual(shownFiles(), []); }); diff --git a/apps/web/test/turn-changes.test.ts b/apps/web/test/turn-changes.test.ts index b1ece4f6..e4bc3d2a 100644 --- a/apps/web/test/turn-changes.test.ts +++ b/apps/web/test/turn-changes.test.ts @@ -111,7 +111,7 @@ test("revertTurnChanges restores the turn's files and deletes created ones", asy }), entry("r1", { role: "toolResult", toolCallId: "c1", details: { patch: PATCHES.two } }), entry("r2", { role: "toolResult", toolCallId: "c2", details: { patch: PATCHES.fresh, created: true } }), - entry("r3", { role: "toolResult", toolCallId: "c3", isError: true, details: { patch: "x" } }), + entry("r3", { role: "toolResult", toolCallId: "c3", isError: true }), entry("u3", { role: "user", content: "third" }), ]; assert.deepEqual(turnChanges(entries, "u1"), { changes: [], skipped: [] }); @@ -162,6 +162,69 @@ test("turnChanges lists writes without a patch as skipped", async () => { }); }); +test("a change the call applied stays listed when the call failed afterwards", async () => { + await withRoot(async (root) => { + // The session's own edit tool records the patch; only the check it ran + // alongside was cancelled, so the call reports an error. + const s = session(root); + await writeFile(join(root, "a.txt"), "one\n"); + ask(s, "u1"); + await edit(s, "c1", "a.txt", "one", "two"); + (s.entries.find((candidate) => candidate.id === "r-c1")!.message as { isError?: boolean }).isError = true; + ask(s, "u2"); + + // The change is listed, it is no skip, and the undo the user asked for works + // instead of reporting a turn without changes. + const { changes, skipped } = turnChanges(s.entries, "u1"); + assert.deepEqual( + changes.map((change) => change.path), + ["a.txt"], + ); + assert.deepEqual(skipped, []); + assert.equal(await undo(s, "u1"), "ok"); + assert.equal(await content(join(root, "a.txt")), "one\n"); + assert.deepEqual(s.recorded, ["u1"]); + }); +}); + +test("a failed later turn that did apply its change is suggested before this one", async () => { + await withRoot(async (root) => { + const s = session(root); + await writeFile(join(root, "a.txt"), "zero\n"); + ask(s, "u1"); + await write(s, "w1", "a.txt", "one\n"); + ask(s, "u2"); + await edit(s, "e2", "a.txt", "one", "two"); + // The second turn's call failed afterwards, but its edit is what the file holds. + (s.entries.find((candidate) => candidate.id === "r-e2")!.message as { isError?: boolean }).isError = true; + // Undoing u1 needs u2 undone first, exactly as for a successful turn. + assert.equal(await undo(s, "u1"), "conflict"); + assert.equal(await content(join(root, "a.txt")), "two\n"); + assert.equal(await undo(s, "u2"), "ok"); + assert.equal(await undo(s, "u1"), "ok"); + assert.equal(await content(join(root, "a.txt")), "zero\n"); + }); +}); + +test("a failed call that recorded nothing is neither a change nor a skip", async () => { + await withRoot(async (root) => { + const entries = [ + entry("u1", { role: "user", content: "go" }), + entry("a1", { + role: "assistant", + content: [ + { type: "toolCall", id: "c1", name: "edit", arguments: { path: "a.txt" } }, + { type: "toolCall", id: "c2", name: "write", arguments: { path: "b.txt" } }, + ], + }), + // The search string did not match, and the write never ran. + entry("r1", { role: "toolResult", toolCallId: "c1", isError: true, details: { changed: false } }), + entry("r2", { role: "toolResult", toolCallId: "c2", isError: true }), + ]; + assert.deepEqual(turnChanges(entries, "u1"), { changes: [], skipped: [] }); + }); +}); + test("reverting a write restores the overwritten file's line endings and BOM", async () => { await withRoot(async (root) => { await writeFile(join(root, "lf.txt"), "a\nb\n"); @@ -370,7 +433,8 @@ test("later turns that recorded no change to the file are not suggested", async ask(s, "u3"); await write(s, "w3", "b.txt", "another file\n"); ask(s, "u4"); - // A failed write whose patch would explain the file, and a write without a patch, both to a.txt. + // Two failed writes to a.txt that never reached the file: one that stopped + // before recording anything, and one whose result carries no change record. s.entries.push( entry("a-w4", { role: "assistant", @@ -379,13 +443,8 @@ test("later turns that recorded no change to the file are not suggested", async { type: "toolCall", id: "w5", name: "write", arguments: { path: "a.txt" } }, ], }), - entry("r-w4", { - role: "toolResult", - toolCallId: "w4", - isError: true, - details: { patch: "--- a.txt\n+++ a.txt\n@@ -1,1 +1,1 @@\n-one\n+two\n" }, - }), - entry("r-w5", { role: "toolResult", toolCallId: "w5" }), + entry("r-w4", { role: "toolResult", toolCallId: "w4", isError: true }), + entry("r-w5", { role: "toolResult", toolCallId: "w5", isError: true, details: {} }), ); assert.equal(await undo(s, "u1"), "modified"); assert.equal(await content(join(root, "a.txt")), "two\n"); @@ -562,6 +621,26 @@ test("each turn's delivery is rebuilt as that turn left the file", async () => { }); }); +test("a turn's version stays exact when its call failed after writing", async () => { + await withRoot(async (root) => { + const s = session(root); + ask(s, "u1"); + await write(s, "w1", "index.html", "

one

\n"); + ask(s, "u2"); + await edit(s, "e2", "index.html", "one", "two"); + // The check the edit ran alongside was cancelled: the file changed, the call is an error. + const result = s.entries.find((candidate) => candidate.id === "r-e2") as SessionEntry; + const message = result.message as { details?: { patch?: string } }; + (result.message as { isError?: boolean }).isError = true; + assert.equal(typeof message.details?.patch, "string"); + // The recorded patch is still the truth about that turn, so a later turn's + // delivery rebuilds it instead of falling back to the file on disk. + ask(s, "u3"); + await edit(s, "e3", "index.html", "two", "three"); + assert.deepEqual(await fileAtTurnEnd(s.entries, "u2", "index.html", root), { text: "

two

\n", exact: true }); + }); +}); + test("a turn's version is inexact when neither replay nor reversal reaches it", async () => { await withRoot(async (root) => { const s = session(root); @@ -580,6 +659,7 @@ test("a turn's version is inexact when neither replay nor reversal reaches it", }); /** A Step-profile call as Web sessions record it: `write_file` keeps no patch, `edit_file` the edit's. */ +/** A `write_file` result as version 0.84.4 and older recorded it: no patch, no reason. */ async function stepWrite(s: Session, id: string, path: string, content: string, changed = true): Promise { const input = { path, content }; s.entries.push(entry(`a-${id}`, { role: "assistant", content: [{ type: "toolCall", id, name: "write_file", arguments: input }] })); @@ -601,7 +681,7 @@ async function stepEdit(s: Session, id: string, path: string, search: string, re s.entries.push(entry(`r-${id}`, { role: "toolResult", toolCallId: id, details: result.details })); } -test("Step's edit_file patches are turn changes; a write_file is untracked, or nothing when unchanged", async () => { +test("Step's edit_file patches are turn changes; an older write_file without one is untracked", async () => { await withRoot(async (root) => { const s = session(root); ask(s, "u1");