Recover from create_file on an existing file without a wasted agent turn - #14717
Recover from create_file on an existing file without a wasted agent turn#14717evelyn-with-warp wants to merge 13 commits into
Conversation
A create_file targeting an existing path fails the whole apply_edits batch
and hands the model only "Could not create {file} because it already
exists." — no content, so it must spend another turn reading or removing
the file before it can retry.
Coerce the create into a full-content update when doing so cannot lose
anything: the target is empty, or it already holds exactly what was
requested. Anything else still errors rather than clobbering the file.
When it does error, carry the existing file's line count and a capped,
line-numbered excerpt in the message so the model can emit a diff on its
next turn. ExistingFileContent keeps contents out of its Debug output,
since the error is Debug-formatted into a log line.
Co-Authored-By: Warp Agent <agent@warp.dev>
…ile-on-exist-file
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR improves recovery when Agent Mode attempts to create a file that already exists by safely coercing empty or byte-identical existing files into full-file update diffs, and by returning a capped, line-numbered excerpt for non-coercible existing files. The changes include focused unit coverage for the recoverable paths and excerpt truncation.
Concerns
No blocking correctness, security, or spec-alignment concerns found in the changed diff. The attached spec context says no approved or repository spec context was found for this PR.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes request-file-edits diff application so create_file attempts against existing empty or identical files can be coerced into update diffs, and otherwise includes a capped line-numbered excerpt of the existing file in the retryable error. It also adds unit coverage for the new recoverability paths.
Concerns
- The exact-content coercion is not byte-preserving for existing files without a trailing newline because the downstream update appliers append a newline to non-empty update insertions.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // cannot lose anything: the file is empty, or it already holds exactly what was requested. | ||
| // Any other overlap falls through to an error rather than blindly clobbering the file. | ||
| FileReadResult::Found(existing_content) | ||
| if existing_content.is_empty() || existing_content == content => |
There was a problem hiding this comment.
\n to non-empty update insertions, so a create_file whose content matches an existing no-newline file will still save a changed file. Handle that case as a true no-op or preserve the file's trailing-newline state before coercing it into an update.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR makes create_file collisions recoverable by coercing empty or byte-identical existing files into full-content update diffs, and by returning a capped, line-numbered excerpt of existing file contents in AlreadyExists errors. The new tests cover content reporting, line-count truncation, empty-file coercion, and identical-content coercion.
Concerns
- The new
AlreadyExistsconversation message unconditionally includes existing file excerpts, so a failed create against a sensitive path can disclose file contents to the model without an explicit read step.
Security
- Gate, redact, or suppress excerpts for sensitive files before adding them to the agent conversation.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| {excerpt}" | ||
| ); | ||
| } else { | ||
| let _ = write!(message, " Its current contents are:\n{excerpt}"); |
There was a problem hiding this comment.
create_file on any existing path now returns file contents to the agent conversation. A prompt-injected or mistaken create against .env, SSH keys, or other sensitive files would disclose up to 16 KiB without an explicit read step; gate this behind the same sensitive-file protections/redaction used for file reads, or suppress excerpts for risky paths.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the agent file-edit application path so create_file attempts against existing empty or byte-identical files are converted into safe full-file updates, and otherwise return a self-healing error message with a bounded line-numbered excerpt of the existing file. It also redacts diff-application failure messages before persisting agent inputs and adds unit coverage for the new cases.
Concerns
- No blocking correctness, security, or spec-drift concerns found in the annotated diff.
spec_context.mdreports that no approved or repository spec context was found for this PR.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes create_file handling so existing-file collisions can recover without an extra read turn, refactors full-file replacement diff construction, adds redaction for failed edit results, and expands unit coverage around existing-file create behavior.
Concerns
- The new
apply_create_filepath replaces any non-identical existing file, even when the file is non-empty. That changescreate_filefrom a non-clobbering operation into a full-file update for arbitrary existing content, which can discard unrelated file contents instead of returning a recoverable conflict. - No approved or repository spec context was available for implementation/spec comparison. I did not find a repo-local
security-review-prcompanion skill in this checkout; the general security pass found no additional diff-line findings beyond the data-loss risk above.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ..Default::default() | ||
| }, | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
create_file can clobber unrelated contents instead of failing with the recoverable existing-content error described in the PR; only empty or byte-identical existing files should be coerced.
b17aee3 to
dc2da06
Compare
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR attempts to make create_file failures on existing files recoverable by quoting existing contents, coercing safe creates into updates, and tracking observed file contents.
Concerns
- The new
AppliedEditsreturn type is not propagated to the request-file-edits executor, so the changed code will not type-check. - The observed-content overwrite branch is never reachable because the PR only adds recording helpers and does not call them for read results, accepted edits, or failed create excerpts.
- The added unit tests still use the old
apply_editsAPI and stale expectations, so they will not compile/pass as written.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| passive_diff: bool, | ||
| ctx: &mut ModelContext<Self>, | ||
| ) -> BoxFuture<'static, Result<Vec<AIRequestedCodeDiff>, Vec1<DiffApplicationError>>> { | ||
| ) -> BoxFuture<'static, Result<AppliedEdits, Vec1<DiffApplicationError>>> { |
There was a problem hiding this comment.
RequestFileEditsExecutor::on_diffs_applied, which still accepts Result<Vec<AIRequestedCodeDiff>, _> and iterates the result as a vec, so the executor call site will no longer type-check. Update that consumer to accept AppliedEdits, handle .diffs, and surface .notes.
| // reviewable-diff pipeline, and note the overwrite in the action result so it is never | ||
| // silent. | ||
| FileReadResult::Found(existing_content) | ||
| if observed.contains(&absolute_path, ContentFingerprint::of(&existing_content)) => |
There was a problem hiding this comment.
record_whole_file_reads/complete_content_fingerprint and never calls them for read_files, accepted edits, or the full excerpt returned by a failed create; after the model receives the quoted contents, a retrying create_file will still miss observed.contains(...) and fail again.
| content: Some("New content".to_string()), | ||
| }; | ||
|
|
||
| let result = apply_edits( |
There was a problem hiding this comment.
apply_edits with the old signature and treat Ok as Vec<AIRequestedCodeDiff>; the function now requires an &ConversationObservedContents argument and returns AppliedEdits, so these tests need to pass observed state and assert through .diffs.
| #[test] | ||
| fn test_create_edit_for_existing_file_truncates_long_contents() { | ||
| let existing: String = (1..=500).map(|n| format!("line {n}\n")).collect(); | ||
| let err = DiffApplicationError::AlreadyExists { |
There was a problem hiding this comment.
AlreadyExists literal omits the new absolute_path field, so the test module will not compile after the enum change.
620a85d to
3b8bd88
Compare
…fore apply replacement Co-Authored-By: Warp Agent <agent@warp.dev>
3b8bd88 to
1baa501
Compare
…ile-on-exist-file
…verwrites Terminal-bench traces showed models hitting the create_file-on-existing-file guard and routing around it with rm + recreate or cat > redirection, because whole-file reads and writes done through the shell never populated ObservedFileContents. - Credit `cat <path>` command output as an observed read when it matches the file on disk byte-for-byte (modulo trailing newline). - Credit single-target shell overwrites (`> path`, heredoc, `| tee path`) when the resulting on-disk content is verifiably what the model authored. - Rewrite the AlreadyExists error to state the sanctioned recovery path: read the whole file, then retry the same create_file. - Elide large replaced bodies from the model-facing result diff of coerced overwrites ([- N lines replaced -]); the review UI keeps the full diff. Co-Authored-By: Warp Agent <agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR teaches create_file to recover from existing files by tracking whole-file observations, coercing informed creates into reviewable replacements, adding overwrite notes, redacting new failure paths, and covering the behavior with unit tests. No approved or repository spec context was provided for implementation comparison.
Concerns
- The shell-write confirmation path can credit computed file contents as model-observed when the final disk content merely appears somewhere in the command string, including the target filename.
- One newly added test asserts an older
AlreadyExistserror message and will fail against the production string introduced in the same diff.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // `ls > f`) matches neither and is never credited. | ||
| ShellFileObservation::Write { .. } => { | ||
| output.trim_end_matches('\n') == trimmed_disk | ||
| || normalize_newlines(command).contains(trimmed_disk) |
There was a problem hiding this comment.
ls > out.txt producing out.txt\n passes because out.txt is in the redirect, so a later create_file may blindly overwrite content the model never saw.
There was a problem hiding this comment.
Good catch — confirmed, and fixed in 7e66bc2.
Traced your example: ls > out.txt parses as Write { path: "out.txt" }, stdout is redirected so the output branch fails, and then command.contains("out.txt") matched the redirect target itself. Credited. The existing test only passed because its listing (a.txt\nb.txt) happened not to be a substring of the command. Same weakness admitted other collisions, e.g. head -c 2 x > y when x starts with he, since he occurs inside head.
Root cause was searching the raw command string, which includes the program name, flags, and the target path — none of which are model-authored content. The fix scopes the match to the heredoc body, the only part of a command the model spells out verbatim; single-line redirects now yield None and are never credited on that path.
Tradeoff: echo hi > f is no longer credited either. That falls back to the AlreadyExists error telling the model to read the file first, which is the safe direction. I considered an echo/printf allowlist to keep it, but it needed pipeline validation (echo hi | sed 's/hi/bye/' > f would otherwise match bye out of the sed script) and wasn't worth the complexity.
Test updates are in the same commit: added your ls > out.txt case as the regression test, plus echo hi > f to pin the tradeoff. Module doc corrected too — it had claimed writes of computed content are never credited, which your example disproved.
| assert_eq!( | ||
| DiffApplicationError::error_for_conversation(&errors), | ||
| format!( | ||
| "{file_path} already exists (2 lines); nothing was written. Read the file first \ |
There was a problem hiding this comment.
DiffApplicationError::AlreadyExists, which now says Read the whole file, then retry: after a full read..., so this new test will fail until the assertion is updated.
Co-Authored-By: Warp <agent@warp.dev>
Matching the disk content against the whole command text credited any file whose content coincided with a substring of it, including the redirect target itself: `ls > out.txt` in a directory listing only `out.txt` passed, so a later create_file could blindly overwrite content the model never saw. Scope the match to the heredoc body, the only part of a command the model spells out verbatim. Redirected literals (`echo hi > f`) are no longer credited; they fall back to the AlreadyExists error, which is the safe direction. Co-Authored-By: Warp <agent@warp.dev>
1 participant
Description
When Warp Agent calls
create_filefor a path that already exists, the edit batch currently fails withCould not create <path> because it already exists.and no other context. The model then spends another turn reading the file—or deletes and recreates it—before it can recover.This PR tracks which file contents the model has verifiably observed (per conversation and absolute path, as CRLF-normalized fingerprints) and branches
create_fileon that knowledge instead of always failing.What counts as observed (
app/src/ai/blocklist/observed_file_contents.rs)read_files/get_filesresults (read_files.rs,get_files.rs).request_file_edits.rs).cat, and single-target writes (>, heredoc,tee) (app/src/ai/blocklist/shell_file_observations.rs).Branches when the target already exists (
apply_create_fileinapp/src/ai/blocklist/action_model/execute/request_file_edits/diff_application.rs)Overwrote existing <path> (N lines replaced)) rides along in the result so the overwrite is never silent; replaced bodies of 100+ lines are elided from the model-facing diff (elide_replaced_contentinrequest_file_edits.rs) while review still shows the full diff.create_file, explicitly ruling out delete-and-recreate or shell redirection.Supporting plumbing: notes surface through
RequestFileEditsResult::Success { notes }, andDiffApplicationFailederrors now go through secret redaction (redaction.rs).apply_editsremains atomic — one invalid edit still rejects the whole batch.Before and after on agent next move
If agent sees file exists error on
create_file:Could not create <path> because it already exists., it will eitherread & editorrm & recreatedirectly editorread & editWe reduced create_file hit file existence error by 75% (254 -> 65), and reduced the rm+recreate by 98% (107 ->2);
The 1% remaining rm+recreate is on model created files in the middle of the trajectory;
https://docs.google.com/spreadsheets/d/1f7zN3NcGPJkKKEmSxd7Z5B3d6axTORX-vwJgVb8iFj8/edit?
Linked Issue
Part of QUALITY-597 — Harness running
rmcommands for file edits instead of changes.This does not close QUALITY-597 because that issue also covers the model proactively choosing delete-and-rewrite for large edits; this PR specifically addresses recovery when
create_fileencounters an existing path.ready-to-specorready-to-implement.Testing
./script/runAgent Mode
Warp conversation
CHANGELOG-OZ: Warp Agent now safely recovers when
create_filetargets an existing file, avoiding unnecessary read or delete-and-recreate turns.Co-Authored-By: Warp Agent agent@warp.dev