fix(agents): recover sandbox edit after post-write failure#45964
fix(agents): recover sandbox edit after post-write failure#459645Funingyuan wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a false-failure bug in sandboxed edit: when the upstream edit library throws after successfully writing the file (e.g. during diff/result formatting), sandboxed edits now recover in the same way the host edit tool already did. The fix generalizes
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-tools.host-edit.ts
Line: 33-35
Comment:
**Default `readFile` silently drops `signal`**
The default `readFile` lambda captures only `filePath` and ignores the `signal` argument passed on line 68. If an `AbortSignal` is triggered while the recovery read is in flight (host mode, no custom `readFile` provided), the read will not be cancelled, potentially delaying or masking the abort.
```suggestion
const readFile =
options?.readFile ??
((filePath: string, signal?: AbortSignal) => fs.readFile(filePath, { encoding: "utf-8", signal }));
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-tools.read.host-edit-recovery.test.ts
Line: 91-123
Comment:
**Missing sandbox "should rethrow" test cases**
The host tool has three tests covering both recovery and the two rethrow conditions (newText absent, oldText still present). The new sandbox test only covers the success/recovery path. Without parity tests, a regression in the sandbox rethrow logic (e.g. a bad bridge mock returning the wrong content) would go undetected.
Consider adding at minimum:
```ts
it("rethrows for sandbox edit when bridge readFile shows newText is absent", async () => {
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from("unchanged content", "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText: "x", newText: "never-written" }, undefined),
).rejects.toThrow("Simulated post-write failure");
});
it("rethrows for sandbox edit when bridge readFile shows oldText still present (pre-write failure)", async () => {
const oldText = "replace me";
const newText = "new content";
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from(`before ${oldText} after ${newText}`, "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText, newText }, undefined),
).rejects.toThrow("Simulated post-write failure");
});
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 361c450 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/agents/pi-tools.host-edit.ts
Outdated
| const readFile = | ||
| options?.readFile ?? | ||
| ((filePath: string) => fs.readFile(filePath, "utf-8")); |
There was a problem hiding this comment.
Default readFile silently drops signal
The default readFile lambda captures only filePath and ignores the signal argument passed on line 68. If an AbortSignal is triggered while the recovery read is in flight (host mode, no custom readFile provided), the read will not be cancelled, potentially delaying or masking the abort.
| const readFile = | |
| options?.readFile ?? | |
| ((filePath: string) => fs.readFile(filePath, "utf-8")); | |
| const readFile = | |
| options?.readFile ?? | |
| ((filePath: string, signal?: AbortSignal) => fs.readFile(filePath, { encoding: "utf-8", signal })); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.host-edit.ts
Line: 33-35
Comment:
**Default `readFile` silently drops `signal`**
The default `readFile` lambda captures only `filePath` and ignores the `signal` argument passed on line 68. If an `AbortSignal` is triggered while the recovery read is in flight (host mode, no custom `readFile` provided), the read will not be cancelled, potentially delaying or masking the abort.
```suggestion
const readFile =
options?.readFile ??
((filePath: string, signal?: AbortSignal) => fs.readFile(filePath, { encoding: "utf-8", signal }));
```
How can I resolve this? If you propose a fix, please make it concise.| it("returns success for sandbox edit when upstream throws after writing", async () => { | ||
| const filePath = "/workspace/MEMORY.md"; | ||
| const oldText = "# Memory"; | ||
| const newText = "Blog Writing"; | ||
| const bridge: SandboxFsBridge = { | ||
| resolvePath: () => ({ | ||
| hostPath: "/tmp/MEMORY.md", | ||
| relativePath: "MEMORY.md", | ||
| containerPath: filePath, | ||
| }), | ||
| readFile: vi.fn(async () => Buffer.from(`\n\n${newText}\n`, "utf-8")), | ||
| writeFile: vi.fn(async () => {}), | ||
| mkdirp: vi.fn(async () => {}), | ||
| remove: vi.fn(async () => {}), | ||
| rename: vi.fn(async () => {}), | ||
| stat: vi.fn(async () => ({ type: "file", size: newText.length, mtimeMs: Date.now() })), | ||
| }; | ||
|
|
||
| const tool = createSandboxedEditTool({ root: "/workspace", bridge }); | ||
| const result = await tool.execute("call-1", { path: filePath, oldText, newText }, undefined); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| const content = Array.isArray((result as { content?: unknown }).content) | ||
| ? (result as { content: Array<{ type?: string; text?: string }> }).content | ||
| : []; | ||
| const textBlock = content.find((b) => b?.type === "text" && typeof b.text === "string"); | ||
| expect(textBlock?.text).toContain("Successfully replaced text"); | ||
| expect(bridge.readFile).toHaveBeenCalledWith({ | ||
| filePath, | ||
| cwd: "/workspace", | ||
| signal: undefined, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing sandbox "should rethrow" test cases
The host tool has three tests covering both recovery and the two rethrow conditions (newText absent, oldText still present). The new sandbox test only covers the success/recovery path. Without parity tests, a regression in the sandbox rethrow logic (e.g. a bad bridge mock returning the wrong content) would go undetected.
Consider adding at minimum:
it("rethrows for sandbox edit when bridge readFile shows newText is absent", async () => {
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from("unchanged content", "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText: "x", newText: "never-written" }, undefined),
).rejects.toThrow("Simulated post-write failure");
});
it("rethrows for sandbox edit when bridge readFile shows oldText still present (pre-write failure)", async () => {
const oldText = "replace me";
const newText = "new content";
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from(`before ${oldText} after ${newText}`, "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText, newText }, undefined),
).rejects.toThrow("Simulated post-write failure");
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.host-edit-recovery.test.ts
Line: 91-123
Comment:
**Missing sandbox "should rethrow" test cases**
The host tool has three tests covering both recovery and the two rethrow conditions (newText absent, oldText still present). The new sandbox test only covers the success/recovery path. Without parity tests, a regression in the sandbox rethrow logic (e.g. a bad bridge mock returning the wrong content) would go undetected.
Consider adding at minimum:
```ts
it("rethrows for sandbox edit when bridge readFile shows newText is absent", async () => {
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from("unchanged content", "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText: "x", newText: "never-written" }, undefined),
).rejects.toThrow("Simulated post-write failure");
});
it("rethrows for sandbox edit when bridge readFile shows oldText still present (pre-write failure)", async () => {
const oldText = "replace me";
const newText = "new content";
const bridge: SandboxFsBridge = {
...
readFile: vi.fn(async () => Buffer.from(`before ${oldText} after ${newText}`, "utf-8")),
};
const tool = createSandboxedEditTool({ root: "/workspace", bridge });
await expect(
tool.execute("call-1", { path: "/workspace/MEMORY.md", oldText, newText }, undefined),
).rejects.toThrow("Simulated post-write failure");
});
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f528009bfd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| operations: createSandboxEditOperations(params), | ||
| }) as unknown as AnyAgentTool; | ||
| return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.edit); | ||
| const withRecovery = wrapHostEditToolWithPostWriteRecovery(base, params.root, { |
There was a problem hiding this comment.
This new sandbox wiring unconditionally applies wrapHostEditToolWithPostWriteRecovery, whose heuristic returns success whenever the file currently contains newText and not oldText; that also matches pre-write failures like "oldText not found" when the file already had the target text before this call. In that case the edit did not actually apply any replacement, but callers now get a success result in sandbox mode, which can hide real edit failures and lead the agent to continue with incorrect assumptions.
Useful? React with 👍 / 👎.
|
I took another pass through the latest failing checks after fixing the sandbox edit recovery test typing issue. At this point, the remaining failures appear to be in unrelated extension and Windows test shards rather than in the sandbox edit recovery path changed in this PR. The current failures are showing up in areas like The targeted sandbox recovery test for this PR passes locally, and the PR-specific typing issue in |
1 participant
Summary
wrapHostEditToolWithPostWriteRecovery(...)to accept injected path resolution and read-back behavior, then wired sandbox edit to verify through the sandbox fs bridge and return success whennewTextis present andoldTextis no longer present after an upstream post-write throw.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
For sandboxed agent runs, a successful edit no longer emits a false failure when the upstream edit tool throws after the write has already landed (for example, during post-write diff/result formatting).
No config or default behavior changes outside this recovery path.
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
newTextpresent andoldTextabsent.Expected
Actual
Evidence
Attach at least one:
$ volta run --node 22.16.0 pnpm test -- src/agents/pi-tools.read.host-edit-recovery.test.ts RUN v4.1.0 /Users/a1-6/openclaw Test Files 1 passed (1) Tests 4 passed (4)