diff --git a/CHANGELOG.md b/CHANGELOG.md index fecf8c25..eac47a3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ opt-out. ### Fixed - The file tools' `deny` list (`.git` by default) is now also checked where a path really leads: a symlink or Windows junction inside the workspace pointing at `.git` or into it (`gitj -> .git`) let `edit_file gitj/config` rewrite `.git/config` (for example `core.hooksPath`, which runs code on the next git command) and `write_file gitj/PWNED` create files in `.git`. Reads, writes, edits, `list_dir`, and `glob`/`grep` given such a link as `path` are now refused. Providers with links can implement the new optional `FsProvider.realPath(path)`; `NodeWorkspace` does. See [Workspace tools](docs/workspace-tools.md#security-model). - `NodeWorkspace.writeFile` (and so `write_file` / `edit_file`) writes a temp file in the same directory and renames it into place, so a process killed mid-write no longer leaves a truncated file. A replaced file keeps its permission bits; a read-only file is still refused. +- `WorkspaceCheckpoints` stores a file's backup before `write_file` / `edit_file` writes it, not after. A process killed between the write and the backup lost the original: on resume the re-run call backed up the agent's own content, so `rewind()` reported `restored` but put back the agent's version (or, for `edit_file`, recorded nothing). A re-run now keeps the turn's first backup, and rewind treats a file still holding its backed-up content as nothing to undo. `track()` also accepts a `PreparedWorkspaceWrite` (`{ prepare, write }`), which the built-in tools use. +- `edit_file` re-run after a crash (the edit already in the file: `old_string` gone, a non-empty `new_string` present, also with `replace_all`) now succeeds with "the edit was already applied" and writes nothing, instead of failing with `old_string was not found` and sending the model into a loop. A `write_file` / `edit_file` that leaves the file unchanged no longer records a checkpoint backup. +- [Sessions](docs/sessions.md) no longer says to call `resume()` before resolving an approval after a restart: `agent.approvals.resolve()` / `streamResolve()` from a fresh process finds the session itself. It and [Build a coding agent](docs/build-a-coding-agent.md) now say how to show a resumed turn's progress (`session.on()`, `onEvent`), and the guide points long coding turns at compaction. - MCP tool schemas with OpenAPI's `nullable: true` (any type) now accept `null`; the model's valid `null` was rejected with `ToolArgumentsValidationError`. - A failed MCP tool call (server exited, timeout, protocol error) now rejects with `LOUSHO_MCP_CALL_FAILED`, naming `__` with the MCP error as `cause`, instead of a bare `MCP error -32000: Connection closed`. [MCP](docs/mcp.md) now says stdio `env` is merged onto the MCP SDK's default allow-list, not `process.env`. - Flows (consumer harness): a `{{variable}}` holding an object (a `toolCall` result, an MCP result) no longer reaches a prompt as `[object Object]`: an object whose only field is `text` (an MCP tool result) is that text, a raw MCP `{ content }` result its text parts, and any other object or array its JSON. Placeholders take dotted paths (`{{res.user.name}}`, `{{list.0}}`, own properties only), also as whole tool-argument values. An `evaluator` step stores its result in `outputVariable`. See [Template values](docs/flows.md#template-values). diff --git a/api/index.api.md b/api/index.api.md index b31574c5..85efa448 100644 --- a/api/index.api.md +++ b/api/index.api.md @@ -4758,6 +4758,14 @@ export function prepareCompaction(messages: Message[], options: CompactMessagesO input: CompactionInput; }; +// @public +export interface PreparedWorkspaceWrite { + // (undocumented) + prepare(): Promise; + // (undocumented) + write(content: string): Promise; +} + // @public export interface PreToolCallDecision { inputBy: string[]; @@ -7177,7 +7185,7 @@ export class WorkspaceCheckpoints { dryRun?: boolean; force?: boolean; }): Promise; - track(ctx: ToolExecutionContext | undefined, path: string, write: () => Promise): Promise; + track(ctx: ToolExecutionContext | undefined, path: string, write: (() => Promise) | PreparedWorkspaceWrite): Promise; } // @public diff --git a/api/tools.api.md b/api/tools.api.md index 9ae5314e..f9ad0a1c 100644 --- a/api/tools.api.md +++ b/api/tools.api.md @@ -1877,6 +1877,14 @@ type PermissionToolMatcher = string | readonly string[] | RegExp; // @public type PiiType = 'email' | 'phone' | 'credit-card' | 'iban' | 'us-ssn' | 'ip-address'; +// @public +export interface PreparedWorkspaceWrite { + // (undocumented) + prepare(): Promise; + // (undocumented) + write(content: string): Promise; +} + // @public interface Principal { authenticator: string; @@ -2869,7 +2877,7 @@ export class WorkspaceCheckpoints { dryRun?: boolean; force?: boolean; }): Promise; - track(ctx: ToolExecutionContext | undefined, path: string, write: () => Promise): Promise; + track(ctx: ToolExecutionContext | undefined, path: string, write: (() => Promise) | PreparedWorkspaceWrite): Promise; } // @public diff --git a/docs/build-a-coding-agent.md b/docs/build-a-coding-agent.md index d35e52b2..2c9851b1 100644 --- a/docs/build-a-coding-agent.md +++ b/docs/build-a-coding-agent.md @@ -214,6 +214,34 @@ after each answer the program asks for the next line, and an empty line or keep a conversation across runs, give the agent a store (see [Choosing a store](./sessions.md#choosing-a-store)). +### Long turns and restarts + +- **Context.** A coding turn reads files, runs tests and edits over many + steps; every tool result stays in the transcript, so a 40-step turn can + outgrow a 128k-token window before it finishes. Pass + `createAgent({ compaction: true })` (or a summarizer, see + [Context compaction](./compaction.md)): it compacts before each model call, + between the steps of one turn too. +- **Crashes.** With a durable store, a turn killed mid-way is finished by the + next `session.send()` or by `session.resume()`. A tool call that was running + when the process died runs again ([at-least-once](./durable-execution.md#at-least-once-tools-make-side-effects-idempotent)). + The built-in `write_file` and `edit_file` are safe to re-run: `write_file` + writes the same content again, and an `edit_file` whose edit is already in + the file (`old_string` gone, `new_string` present) succeeds with "the edit + was already applied" instead of failing. With + [`WorkspaceCheckpoints`](./workspace-tools.md#the-tools), each change's backup is stored + before the file is written, and a re-run keeps the first backup, so + `rewind()` still restores the original after a crash. +- **Progress while resuming.** `session.resume()` returns a result, not a + stream, and `session.stream()` does not stream the pending turn it finishes + before the new message. To show progress, register + `session.on((event) => ...)` before calling it, or pass + `createAgent({ onEvent })`: both get the resumed turn's `AgentEvent`s + (`tool.start`, `text.delta`, ..., `run.done`) as they happen. +- **Approvals after a restart.** `agent.approvals.streamResolve({ id, approved })` + (or `resolve()`) from a fresh process, given the same durable store, + continues the paused turn in its session; no `session.resume()` first. + ## 6. Test it without a model The agent needs no network to test. A `MemoryWorkspace` keeps the files in diff --git a/docs/durable-execution.md b/docs/durable-execution.md index 60beeaae..599c7360 100644 --- a/docs/durable-execution.md +++ b/docs/durable-execution.md @@ -451,6 +451,16 @@ const chargeCard = defineTool({ (`toolCallId` is set for every tool run, including tools that route through `sandboxExecute`.) +The built-in file tools are safe to re-run. `write_file` writes the same +content again; an `edit_file` whose edit is already in the file (`old_string` +gone, a non-empty `new_string` present) succeeds and says the edit was already +applied, instead of telling the model that an applied edit failed. With +`WorkspaceCheckpoints`, each change's backup is stored before the file is +written and a re-run keeps the first backup, so `rewind()` restores the +original content after a crash too (see +[Workspace tools](./workspace-tools.md#the-tools)). The `shell` tool is not: +a command that was running when the process died runs again. + `toolCallId` (and `ctx.approval.id` for an approved call) identifies one call the model made. It only protects against that call running twice. If the process dies before the call is recorded, the model may make the call again diff --git a/docs/sessions.md b/docs/sessions.md index a58fb319..b6e5ad5b 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -257,9 +257,16 @@ console.log(finished?.text, await agent.session({ id: 'user-42' }).pending()); / the transcript, until it finishes. While it waits, `resume()`, `send()` and `stream()` throw `SessionAwaitingApprovalError` (with its `approvalId`), and `agent.approvals.resolve({ id, approved })` continues the turn in this - session. After a restart, open the session and call `resume()` (or `send()`) - once before resolving, so the agent knows which session the approval belongs - to; give both agents the same durable `store` (or `approvalStore`). + session. After a restart, `agent.approvals.resolve()` or `streamResolve()` + from the new process continues it too: the agent finds the session from the + pause record (for session ids of letters, digits, `_` and `-`), so there is + no need to open the session or call `resume()` first. Give both agents the + same durable `store` (or `approvalStore`). +- `resume()` returns the finished turn's result; it does not stream, and + `stream()` does not stream the pending turn it finishes first either. To + show a resumed turn's progress, register `session.on(listener)` before the + call (or pass `createAgent({ onEvent })`): it gets the turn's `AgentEvent`s + as they happen. - A paused turn locks the session id: the session records it under `.paused` in the checkpoint store, so `agent.send(message, { sessionId: id })`, `agent.stream()` with that `sessionId`, and a session object over a different diff --git a/docs/workspace-tools.md b/docs/workspace-tools.md index 2b59e242..733d48a7 100644 --- a/docs/workspace-tools.md +++ b/docs/workspace-tools.md @@ -81,7 +81,7 @@ const paused = await AgentExecutor.execute({ agent, input, provider, toolRegistr | ------------ | ------------------------------------------------- | ------- | | `read_file` | `path`, optional `offset` (1-based line), `limit` | Numbered lines (`" 12\tcode"`). When the output is cut, the last line says which lines were shown and the `offset` to continue from. Files over 10 MB are refused. | | `write_file` | `path`, `content` | Creates or overwrites the file and creates missing parent directories. | -| `edit_file` | `path`, `old_string`, `new_string`, optional `replace_all` | Replaces the exact text. Fails with a message the model can act on when `old_string` is missing (including a hint about CRLF line endings) or appears more than once without `replace_all`. Refuses a file that is not UTF-8 (latin1, Windows-1252, UTF-16) and leaves it unchanged, when the provider has `readFileBytes` (as `NodeWorkspace` does). A UTF-8 BOM is kept. | +| `edit_file` | `path`, `old_string`, `new_string`, optional `replace_all` | Replaces the exact text. Fails with a message the model can act on when `old_string` is missing (including a hint about CRLF line endings) or appears more than once without `replace_all`. When `old_string` is missing but a non-empty `new_string` is in the file, the edit was already applied (a call re-run after a crash): it succeeds without writing and says so. Refuses a file that is not UTF-8 (latin1, Windows-1252, UTF-16) and leaves it unchanged, when the provider has `readFileBytes` (as `NodeWorkspace` does). A UTF-8 BOM is kept. | | `list_dir` | optional `path` | Sorted entries; directories end with `/`. | | `glob` | `pattern`, optional `path` | Sorted paths of matching files. `*` stays within one directory, `**` crosses directories, plus `?`, `[abc]` and `{a,b}`. | | `grep` | `pattern` (JavaScript regex), optional `path`, `glob`, `ignore_case` | `path:line: text` for each matching line. An invalid regex, or one with nested repetition like `(a+)+`, is a tool error. Binary files and files over 2 MB are skipped. | @@ -186,6 +186,17 @@ well, fork the session with `session.fork({ fromStep })` (see `session.history()` is the turn number used here), or a single run with `agent.fork()` (see [Fork and replay](./durable-execution.md#fork-and-replay)). +Backups survive a crash. The backup is stored before the file is written, so +a process killed between the two loses nothing: the file still holds the +backed-up content, and rewind treats it as nothing to undo. A tool call that +was running when the process died runs again on resume (see +[At-least-once tools](./durable-execution.md#at-least-once-tools-make-side-effects-idempotent)); +the re-run finds the turn's backup of that path and keeps it, so `rewind()` +restores the content from before the first attempt, not the agent's own +half-finished write. A `write_file` or `edit_file` that would leave the file +as it is (the same content, an edit already applied) records and writes +nothing. + What is not tracked: changes made by the `shell` tool, files written by a sub-agent with its own file tools (unless it gets the same `WorkspaceCheckpoints`, in which case they are recorded under its own diff --git a/llms-full.txt b/llms-full.txt index a5b4fc27..6d478dd3 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -3825,6 +3825,34 @@ after each answer the program asks for the next line, and an empty line or keep a conversation across runs, give the agent a store (see [Choosing a store](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/sessions.md#choosing-a-store)). +### Long turns and restarts + +- **Context.** A coding turn reads files, runs tests and edits over many + steps; every tool result stays in the transcript, so a 40-step turn can + outgrow a 128k-token window before it finishes. Pass + `createAgent({ compaction: true })` (or a summarizer, see + [Context compaction](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/compaction.md)): it compacts before each model call, + between the steps of one turn too. +- **Crashes.** With a durable store, a turn killed mid-way is finished by the + next `session.send()` or by `session.resume()`. A tool call that was running + when the process died runs again ([at-least-once](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/durable-execution.md#at-least-once-tools-make-side-effects-idempotent)). + The built-in `write_file` and `edit_file` are safe to re-run: `write_file` + writes the same content again, and an `edit_file` whose edit is already in + the file (`old_string` gone, `new_string` present) succeeds with "the edit + was already applied" instead of failing. With + [`WorkspaceCheckpoints`](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/workspace-tools.md#the-tools), each change's backup is stored + before the file is written, and a re-run keeps the first backup, so + `rewind()` still restores the original after a crash. +- **Progress while resuming.** `session.resume()` returns a result, not a + stream, and `session.stream()` does not stream the pending turn it finishes + before the new message. To show progress, register + `session.on((event) => ...)` before calling it, or pass + `createAgent({ onEvent })`: both get the resumed turn's `AgentEvent`s + (`tool.start`, `text.delta`, ..., `run.done`) as they happen. +- **Approvals after a restart.** `agent.approvals.streamResolve({ id, approved })` + (or `resolve()`) from a fresh process, given the same durable store, + continues the paused turn in its session; no `session.resume()` first. + ## 6. Test it without a model The agent needs no network to test. A `MemoryWorkspace` keeps the files in @@ -7666,6 +7694,16 @@ const chargeCard = defineTool({ (`toolCallId` is set for every tool run, including tools that route through `sandboxExecute`.) +The built-in file tools are safe to re-run. `write_file` writes the same +content again; an `edit_file` whose edit is already in the file (`old_string` +gone, a non-empty `new_string` present) succeeds and says the edit was already +applied, instead of telling the model that an applied edit failed. With +`WorkspaceCheckpoints`, each change's backup is stored before the file is +written and a re-run keeps the first backup, so `rewind()` restores the +original content after a crash too (see +[Workspace tools](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/workspace-tools.md#the-tools)). The `shell` tool is not: +a command that was running when the process died runs again. + `toolCallId` (and `ctx.approval.id` for an approved call) identifies one call the model made. It only protects against that call running twice. If the process dies before the call is recorded, the model may make the call again @@ -16203,9 +16241,16 @@ console.log(finished?.text, await agent.session({ id: 'user-42' }).pending()); / the transcript, until it finishes. While it waits, `resume()`, `send()` and `stream()` throw `SessionAwaitingApprovalError` (with its `approvalId`), and `agent.approvals.resolve({ id, approved })` continues the turn in this - session. After a restart, open the session and call `resume()` (or `send()`) - once before resolving, so the agent knows which session the approval belongs - to; give both agents the same durable `store` (or `approvalStore`). + session. After a restart, `agent.approvals.resolve()` or `streamResolve()` + from the new process continues it too: the agent finds the session from the + pause record (for session ids of letters, digits, `_` and `-`), so there is + no need to open the session or call `resume()` first. Give both agents the + same durable `store` (or `approvalStore`). +- `resume()` returns the finished turn's result; it does not stream, and + `stream()` does not stream the pending turn it finishes first either. To + show a resumed turn's progress, register `session.on(listener)` before the + call (or pass `createAgent({ onEvent })`): it gets the turn's `AgentEvent`s + as they happen. - A paused turn locks the session id: the session records it under `.paused` in the checkpoint store, so `agent.send(message, { sessionId: id })`, `agent.stream()` with that `sessionId`, and a session object over a different @@ -19953,7 +19998,7 @@ const paused = await AgentExecutor.execute({ agent, input, provider, toolRegistr | ------------ | ------------------------------------------------- | ------- | | `read_file` | `path`, optional `offset` (1-based line), `limit` | Numbered lines (`" 12\tcode"`). When the output is cut, the last line says which lines were shown and the `offset` to continue from. Files over 10 MB are refused. | | `write_file` | `path`, `content` | Creates or overwrites the file and creates missing parent directories. | -| `edit_file` | `path`, `old_string`, `new_string`, optional `replace_all` | Replaces the exact text. Fails with a message the model can act on when `old_string` is missing (including a hint about CRLF line endings) or appears more than once without `replace_all`. Refuses a file that is not UTF-8 (latin1, Windows-1252, UTF-16) and leaves it unchanged, when the provider has `readFileBytes` (as `NodeWorkspace` does). A UTF-8 BOM is kept. | +| `edit_file` | `path`, `old_string`, `new_string`, optional `replace_all` | Replaces the exact text. Fails with a message the model can act on when `old_string` is missing (including a hint about CRLF line endings) or appears more than once without `replace_all`. When `old_string` is missing but a non-empty `new_string` is in the file, the edit was already applied (a call re-run after a crash): it succeeds without writing and says so. Refuses a file that is not UTF-8 (latin1, Windows-1252, UTF-16) and leaves it unchanged, when the provider has `readFileBytes` (as `NodeWorkspace` does). A UTF-8 BOM is kept. | | `list_dir` | optional `path` | Sorted entries; directories end with `/`. | | `glob` | `pattern`, optional `path` | Sorted paths of matching files. `*` stays within one directory, `**` crosses directories, plus `?`, `[abc]` and `{a,b}`. | | `grep` | `pattern` (JavaScript regex), optional `path`, `glob`, `ignore_case` | `path:line: text` for each matching line. An invalid regex, or one with nested repetition like `(a+)+`, is a tool error. Binary files and files over 2 MB are skipped. | @@ -20058,6 +20103,17 @@ well, fork the session with `session.fork({ fromStep })` (see `session.history()` is the turn number used here), or a single run with `agent.fork()` (see [Fork and replay](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/durable-execution.md#fork-and-replay)). +Backups survive a crash. The backup is stored before the file is written, so +a process killed between the two loses nothing: the file still holds the +backed-up content, and rewind treats it as nothing to undo. A tool call that +was running when the process died runs again on resume (see +[At-least-once tools](https://github.com/LinuxDevil/agent-sdk/blob/main/docs/durable-execution.md#at-least-once-tools-make-side-effects-idempotent)); +the re-run finds the turn's backup of that path and keeps it, so `rewind()` +restores the content from before the first attempt, not the agent's own +half-finished write. A `write_file` or `edit_file` that would leave the file +as it is (the same content, an edit already applied) records and writes +nothing. + What is not tracked: changes made by the `shell` tool, files written by a sub-agent with its own file tools (unless it gets the same `WorkspaceCheckpoints`, in which case they are recorded under its own diff --git a/src/session/durableSession.test.ts b/src/session/durableSession.test.ts index fc4d838c..9a2a6bca 100644 --- a/src/session/durableSession.test.ts +++ b/src/session/durableSession.test.ts @@ -119,6 +119,24 @@ describe('checkpointed sessions', () => { expect(roles((await store.sessions.load('chat'))!)).toEqual(['user', 'assistant', 'tool', 'assistant']); }); + it('resume() is not streamed, but session.on() and createAgent({ onEvent }) see the resumed turn as it runs', async () => { + const runs: Runs = {}; + const tools = [tool('a', runs), tool('b', runs, { crashOnce: true })]; + const store = { sessions: new MemorySessionStore(), checkpoints: checkpointStore() }; + await expect(createAgent({ provider: mockModel([calling('a'), calling('b')]), tools }).session({ id: 'chat', store }).send('go')).rejects.toThrow( + 'process died' + ); + + const fromConfig: string[] = []; + const session = createAgent({ provider: mockModel(['Done.']), tools, onEvent: (event) => fromConfig.push(event.type) }).session({ id: 'chat', store }); + const seen: string[] = []; + session.on((event) => seen.push(event.type === 'tool.start' ? `tool.start:${event.toolName}` : event.type)); + expect((await session.resume())?.text).toBe('Done.'); + expect(seen).toContain('tool.start:b'); + expect(seen.at(-1)).toBe('run.done'); + expect(fromConfig).toContain('tool.start'); + }); + it('send() while a turn is pending finishes that turn first, then sends the new message', async () => { const runs: Runs = {}; const tools = [tool('a', runs), tool('b', runs, { crashOnce: true })]; diff --git a/src/tools/workspace/checkpoints.test.ts b/src/tools/workspace/checkpoints.test.ts index c0ed9c9e..684a7b39 100644 --- a/src/tools/workspace/checkpoints.test.ts +++ b/src/tools/workspace/checkpoints.test.ts @@ -248,6 +248,97 @@ describe('WorkspaceCheckpoints details', () => { expect(workspace.snapshot()).toEqual({ 'a.txt': 'a1\n' }); }); + /** A store whose next append fails once the backup is stored, like a process killed right after it. */ + class CrashingStore extends MemoryWorkspaceCheckpointStore { + crashAfterAppend = false; + async append(backup: Parameters[0]): Promise { + await super.append(backup); + if (this.crashAfterAppend) { + this.crashAfterAppend = false; + throw new Error('killed'); + } + } + } + + /** A workspace whose next writeFile fails after writing, like a process killed before the result was recorded. */ + class CrashingWorkspace extends MemoryWorkspace { + crashAfterWrite = false; + async writeFile(path: string, content: string): Promise { + await super.writeFile(path, content); + if (this.crashAfterWrite) { + this.crashAfterWrite = false; + throw new Error('killed'); + } + } + } + + function crashSetup(files: Record) { + const workspace = new CrashingWorkspace({ files }); + const store = new CrashingStore(); + const checkpoints = new WorkspaceCheckpoints(workspace, { store }); + const [, writeFile, editFile] = createFsTools(workspace, { checkpoints }); + return { workspace, store, checkpoints, writeFile, editFile }; + } + + it('stores the backup before writing: a crash between the two changes nothing and leaves nothing to undo', async () => { + const { workspace, store, checkpoints, writeFile } = crashSetup({ 'a.txt': 'a0\n' }); + store.crashAfterAppend = true; + await expect(writeFile.tool.execute!({ path: 'a.txt', content: 'a1\n' }, ctx('s', 1))).rejects.toThrow('killed'); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'a0\n' }); + expect(await checkpoints.rewind(0, { sessionId: 's', dryRun: true })).toEqual({ dryRun: true, restored: [], deleted: [], skipped: [] }); + // The resumed run re-runs the call; rewind then restores the original. + await writeFile.tool.execute!({ path: 'a.txt', content: 'a1\n' }, ctx('s', 1)); + expect(await checkpoints.rewind(0, { sessionId: 's' })).toMatchObject({ restored: ['a.txt'] }); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'a0\n' }); + }); + + it('a write_file re-run after a crash keeps the original backup, so rewind restores the original', async () => { + const { workspace, checkpoints, writeFile } = crashSetup({ 'a.txt': 'a0\n' }); + workspace.crashAfterWrite = true; + await expect(writeFile.tool.execute!({ path: 'a.txt', content: 'a1\n' }, ctx('s', 1))).rejects.toThrow('killed'); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'a1\n' }); + await writeFile.tool.execute!({ path: 'a.txt', content: 'a1\n' }, ctx('s', 1)); + expect(await checkpoints.rewind(0, { sessionId: 's' })).toMatchObject({ restored: ['a.txt'], skipped: [] }); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'a0\n' }); + }); + + it('an edit_file re-run after a crash succeeds as already applied, and rewind restores the original', async () => { + const { workspace, checkpoints, editFile } = crashSetup({ 'a.txt': 'x = 0\n' }); + workspace.crashAfterWrite = true; + const args = { path: 'a.txt', old_string: 'x = 0', new_string: 'x = 1' }; + await expect(editFile.tool.execute!(args, ctx('s', 1))).rejects.toThrow('killed'); + expect(await editFile.tool.execute!(args, ctx('s', 1))).toMatch(/already applied/); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'x = 1\n' }); + expect(await checkpoints.rewind(0, { sessionId: 's' })).toMatchObject({ restored: ['a.txt'], skipped: [] }); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'x = 0\n' }); + }); + + it('track() with a plain function stores the backup first, so a crash during the write keeps the original', async () => { + const workspace = new MemoryWorkspace({ files: { 'a.txt': 'a0\n' } }); + const store = new MemoryWorkspaceCheckpointStore(); + const checkpoints = new WorkspaceCheckpoints(workspace, { store }); + const writeThenDie = async (): Promise => { + await workspace.writeFile('a.txt', 'a1\n'); + throw new Error('killed'); + }; + await expect(checkpoints.track(ctx('s', 1), 'a.txt', writeThenDie)).rejects.toThrow('killed'); + expect((await store.list('s')).map((b) => b.before)).toEqual(['a0\n']); + // The written content's hash was never recorded, so rewind needs force - but the original is there. + expect((await checkpoints.rewind(0, { sessionId: 's', dryRun: true })).skipped).toEqual([{ path: 'a.txt', reason: 'changed-since' }]); + expect(await checkpoints.rewind(0, { sessionId: 's', force: true })).toMatchObject({ restored: ['a.txt'] }); + expect(workspace.snapshot()).toEqual({ 'a.txt': 'a0\n' }); + // A plain-function write that finishes adds a second record carrying the written content's hash. + await checkpoints.track(ctx('s', 1), 'a.txt', async () => { + await workspace.writeFile('a.txt', 'a2\n'); + return 'a2\n'; + }); + expect((await store.list('s')).map((b) => [b.before, b.omitted])).toEqual([ + ['a0\n', undefined], + [null, 'same-turn'], + ]); + expect(await checkpoints.rewind(0, { sessionId: 's' })).toMatchObject({ restored: ['a.txt'] }); + }); + it('validates its options and arguments', async () => { const workspace = new MemoryWorkspace(); expect(() => new WorkspaceCheckpoints(workspace, { maxFileBytes: 0 })).toThrow(/maxFileBytes/); diff --git a/src/tools/workspace/checkpoints.ts b/src/tools/workspace/checkpoints.ts index 220bb4bb..42d5a586 100644 --- a/src/tools/workspace/checkpoints.ts +++ b/src/tools/workspace/checkpoints.ts @@ -28,13 +28,17 @@ export interface WorkspaceFileBackup { /** Permission bits before the change, when the provider has `getMode` and the file existed. */ mode?: number; /** - * Set when the file existed but its content was not kept: `'too-large'` - * (over `maxFileBytes`; rewind skips the file and never deletes it) or - * `'same-turn'` (a later write in a turn that already backed the path up; - * only the turn's first backup is needed to rewind). + * Set when the content was not kept: `'too-large'` (the file existed but + * was over `maxFileBytes`; rewind skips the file and never deletes it) or + * `'same-turn'` (the turn already backed the path up: a later write in + * that turn, or the record a write adds once it finished, carrying its + * `afterHash`; only the turn's first backup is needed to rewind). */ omitted?: 'too-large' | 'same-turn'; - /** sha256 (hex) of what the tool wrote. */ + /** + * sha256 (hex) of what the tool wrote (for the first record of a + * plain-function `track` write, stored before it ran: of the content it found). + */ afterHash: string; /** ISO time of the change. */ at: string; @@ -115,6 +119,16 @@ export interface RewindResult { skipped: Array<{ path: string; reason: 'changed-since' | 'too-large' | 'not-a-file' }>; } +/** + * A change {@link WorkspaceCheckpoints.track} can back up before it happens: + * `prepare` computes the new content (throwing to refuse the change), and + * `write` stores it once the backup is recorded. + */ +export interface PreparedWorkspaceWrite { + prepare(): Promise; + write(content: string): Promise; +} + /** Options for {@link WorkspaceCheckpoints}. */ export interface WorkspaceCheckpointsOptions { /** Where backups live. Defaults to a {@link MemoryWorkspaceCheckpointStore}. */ @@ -143,6 +157,12 @@ interface Captured { tooLarge: boolean; } +/** What a backup leaves out: the content of a too-large file, or of a path the turn already backed up. */ +function omittedFor(captured: Captured, firstInTurn: boolean): WorkspaceFileBackup['omitted'] { + if (captured.tooLarge) return 'too-large'; + return !firstInTurn && captured.before !== null ? 'same-turn' : undefined; +} + /** One file's planned rewind. */ type PlannedAction = | { kind: 'restore'; path: string; content: string; mode?: number } @@ -245,17 +265,42 @@ export class WorkspaceCheckpoints { } /** - * Used by `createFsTools`: captures `path` as it is now, runs `write` - * (which returns the content it wrote), then records the backup. A write - * that throws records nothing. Writes to one path are serialized. + * Used by `createFsTools`: backs `path` up, then changes it. Writes to one + * path are serialized. + * + * With a {@link PreparedWorkspaceWrite}, `prepare` computes the new content + * (a `prepare` that throws records nothing; content equal to the file's + * records and writes nothing), the backup is stored, then `write` runs. The + * backup is stored before the file changes, so a crash in between never + * loses the original, and rewind treats a file still holding its backed-up + * content as nothing to undo. + * + * With a plain function (which writes and returns the content it wrote), + * the backup is stored first and a second record with the written + * content's hash is added after it returns. + * + * A path's first write in a turn records the backup; a later one - + * including a resumed run re-running an interrupted call, which finds the + * file already written - keeps that first backup. */ - async track(ctx: ToolExecutionContext | undefined, path: string, write: () => Promise): Promise { + async track(ctx: ToolExecutionContext | undefined, path: string, write: (() => Promise) | PreparedWorkspaceWrite): Promise { const target = normalizeWorkspacePath(path); const origin = originOf(ctx); await this.paths.run(target, async () => { const captured = await this.capture(target); - const written = await write(); - await this.record(origin, target, captured, written); + if (typeof write === 'function') { + const first = await this.isFirstInTurn(origin, target); + if (first) await this.record(origin, target, captured, captured.before ?? '', captured.tooLarge ? 'too-large' : undefined); + const written = await write(); + const omitted = captured.tooLarge ? 'too-large' : first ? 'same-turn' : omittedFor(captured, false); + await this.record(origin, target, captured, written, omitted); + } else { + const content = await write.prepare(); + if (content === captured.before) return; + await this.record(origin, target, captured, content, omittedFor(captured, await this.isFirstInTurn(origin, target))); + await write.write(content); + } + await this.prune(origin.sessionId, origin.turn); }); } @@ -315,20 +360,27 @@ export class WorkspaceCheckpoints { return { before, mode, tooLarge: false }; } - private async record(origin: WriteOrigin, path: string, captured: Captured, written: string): Promise { - const existing = await this.store.list(origin.sessionId); - const repeat = existing.some((b) => b.turn === origin.turn && b.path === path); - const omitted = captured.tooLarge ? 'too-large' : repeat && captured.before !== null ? 'same-turn' : undefined; + /** Whether this turn has no backup of `path` yet (so this write's backup is the one rewind restores). */ + private async isFirstInTurn(origin: WriteOrigin, path: string): Promise { + return !(await this.store.list(origin.sessionId)).some((b) => b.turn === origin.turn && b.path === path); + } + + private async record( + origin: WriteOrigin, + path: string, + captured: Captured, + content: string, + omitted: WorkspaceFileBackup['omitted'] + ): Promise { await this.store.append({ ...origin, path, before: omitted ? null : captured.before, ...(captured.mode !== undefined && { mode: captured.mode }), ...(omitted && { omitted }), - afterHash: hashContent(written), + afterHash: hashContent(content), at: new Date().toISOString(), }); - await this.prune(origin.sessionId, origin.turn); } /** Retention: keeps the newest `maxTurns` turns. */ @@ -345,8 +397,10 @@ export class WorkspaceCheckpoints { const stat = await this.fs.stat(path); if (stat && stat.type !== 'file') return { kind: 'skip', path, reason: 'not-a-file' }; const current = stat ? await this.fs.readFile(path) : undefined; - // Created by the agent and already gone: the file is as it was before the turn. + // Created by the agent and already gone, or holding its earlier content + // (e.g. a write that crashed after its backup was stored): nothing to undo. if (current === undefined && first.before === null) return undefined; + if (current !== undefined && current === first.before) return undefined; const untouched = current !== undefined && hashContent(current) === changes[changes.length - 1].afterHash; if (!untouched && !force) return { kind: 'skip', path, reason: 'changed-since' }; if (first.before === null) return { kind: 'delete', path }; diff --git a/src/tools/workspace/fsOperations.ts b/src/tools/workspace/fsOperations.ts index d7195515..14db75d6 100644 --- a/src/tools/workspace/fsOperations.ts +++ b/src/tools/workspace/fsOperations.ts @@ -136,6 +136,8 @@ export interface PreparedEdit { path: string; content: string; message: string; + /** The edit was already applied: `content` is the file as it is, and nothing needs writing. */ + unchanged?: boolean; } /** Reads the file and computes an `edit_file` change without writing it; throws for an edit that would fail. */ @@ -149,6 +151,12 @@ export async function prepareEdit(fs: FsProvider, args: EditArgs): Promise 1 && !args.replace_all) { throw new WorkspaceError( @@ -166,7 +174,7 @@ export async function prepareEdit(fs: FsProvider, args: EditArgs): Promise { const edit = await prepareEdit(fs, args); - await fs.writeFile(edit.path, edit.content); + if (edit.unchanged !== true) await fs.writeFile(edit.path, edit.content); return edit.message; } diff --git a/src/tools/workspace/fsTools.test.ts b/src/tools/workspace/fsTools.test.ts index bb728aef..b8e28dbe 100644 --- a/src/tools/workspace/fsTools.test.ts +++ b/src/tools/workspace/fsTools.test.ts @@ -133,6 +133,28 @@ describe('createFsTools on MemoryWorkspace', () => { await expect(call(edit_file, { path: 'a.ts', old_string: 'one\ntwo', new_string: 'x' })).rejects.toThrow(/CRLF line endings/); }); + it('succeeds without writing when the edit was already applied (a re-run after a crash), also with replace_all', async () => { + const ws = new MemoryWorkspace({ files: { 'a.ts': 'const b = 3;\nconst c = 3;\n' } }); + let writes = 0; + const original = ws.writeFile.bind(ws); + ws.writeFile = async (path, content) => { + writes++; + return original(path, content); + }; + const { edit_file } = toolsByName(ws); + const message = 'a.ts already contains new_string and no old_string; the edit was already applied, so nothing changed.'; + expect(await call(edit_file, { path: 'a.ts', old_string: 'b = 2', new_string: 'b = 3' })).toBe(message); + expect(await call(edit_file, { path: 'a.ts', old_string: '= 2', new_string: '= 3', replace_all: true })).toBe(message); + expect(writes).toBe(0); + expect(ws.snapshot()['a.ts']).toBe('const b = 3;\nconst c = 3;\n'); + }); + + it('still fails when old_string is missing and new_string is absent too, or empty', async () => { + const { edit_file } = toolsByName(new MemoryWorkspace({ files: { 'a.ts': 'one\n' } })); + await expect(call(edit_file, { path: 'a.ts', old_string: 'two', new_string: 'three' })).rejects.toThrow(/old_string was not found/); + await expect(call(edit_file, { path: 'a.ts', old_string: 'two', new_string: '' })).rejects.toThrow(/old_string was not found/); + }); + it('rejects empty and no-op edits', async () => { const { edit_file } = toolsByName(new MemoryWorkspace({ files: { 'a.ts': 'x' } })); await expect(call(edit_file, { path: 'a.ts', old_string: '', new_string: 'y' })).rejects.toThrow(/use write_file/); diff --git a/src/tools/workspace/fsTools.ts b/src/tools/workspace/fsTools.ts index ca570a95..cacdaba7 100644 --- a/src/tools/workspace/fsTools.ts +++ b/src/tools/workspace/fsTools.ts @@ -242,11 +242,8 @@ function mutatingTools(links: LinkGuard, limits: FsLimits, approvals: FsToolAppr const target = allowedPath(path, limits); // Before `track`, which would read the old content into a backup. await links.check(target); - const write = async () => { - await fs.writeFile(target, content); - return content; - }; - await (checkpoints ? checkpoints.track(ctx, target, write) : write()); + if (checkpoints) await checkpoints.track(ctx, target, { prepare: async () => content, write: (text) => fs.writeFile(target, text) }); + else await fs.writeFile(target, content); return `Wrote ${Buffer.byteLength(content, 'utf8')} bytes to ${target}.`; }, }), @@ -258,14 +255,17 @@ function mutatingTools(links: LinkGuard, limits: FsLimits, approvals: FsToolAppr input: editFileInput, needsApproval: approvals.edit_file, async execute(args, ctx) { - await links.check(allowedPath(args.path, limits)); + const target = allowedPath(args.path, limits); + await links.check(target); if (!checkpoints) return editFile(fs, args); let message = ''; - await checkpoints.track(ctx, args.path, async () => { - const edit = await prepareEdit(fs, args); - await fs.writeFile(edit.path, edit.content); - message = edit.message; - return edit.content; + await checkpoints.track(ctx, target, { + async prepare() { + const edit = await prepareEdit(fs, args); + message = edit.message; + return edit.content; + }, + write: (text) => fs.writeFile(target, text), }); return message; }, diff --git a/src/tools/workspace/index.ts b/src/tools/workspace/index.ts index b9d48081..0a8e8d11 100644 --- a/src/tools/workspace/index.ts +++ b/src/tools/workspace/index.ts @@ -32,6 +32,7 @@ export { export { WorkspaceCheckpoints, MemoryWorkspaceCheckpointStore, + type PreparedWorkspaceWrite, type RewindResult, type WorkspaceCheckpointStore, type WorkspaceCheckpointsOptions,