Suppress diff exceptions#167
Conversation
| _meta: { | ||
| kind: "delete", | ||
| } | ||
| try { |
There was a problem hiding this comment.
Looks like unified diff handling for add/delete are redundant - see fn format_file_change_diff(change: &FileChange) -> String { https://github.com/openai/codex/blob/67b805fc111706aca5b32d465c94d95659bab6aa/codex-rs/app-server-protocol/src/protocol/item_builders.rs#L302
There was a problem hiding this comment.
I'm still not sure that it never leaks via content. We have explicit commits and test for add/delete recovery. I tried to avoid any semantic change here and suggest putting this cleanup out of the scope of this PR.
| * Removes synthetic "Moved to" from the end. | ||
| */ | ||
| function recoverCorruptedDiff(diff: string): string { | ||
| return diff.replace(/\nMoved to: .*$/, ""); |
| type: "diff", | ||
| oldText: oldContent, | ||
| newText: newContent, | ||
| path: change.path, |
There was a problem hiding this comment.
What if file was actually moved? Don't we want to use PatchChangeKind#moved_path?
There was a problem hiding this comment.
Actually I think that it's impossible to express move in a single diff event. We probably need to split it into delete + add pair.
| } | ||
|
|
||
| async function createUpdateFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> { | ||
| const oldContent = await readFile(change.path, { encoding: "utf8" }).catch(() => null); |
There was a problem hiding this comment.
I think I'm seeing an error here where, when running the agent on bypass permissions, the edit is already applied to the file when reading the file here, so the applyPatch fails and the "Editing files" ACP tool call is sent with empty content.
3 participants
No description provided.