QUALITY-1264: Structured apply-diff errors — Rust client (warp)

[warp-dev-github-integration]

Description

Client-side (warp) implementation of structured apply-diff error reporting (QUALITY-1264). Moves edit_files failure details from opaque rendered strings to a typed DiffApplicationFailure structure that the server can render with per-model/per-harness wording.

This is the Agent C PR in a 3-way cross-repo change:

• proto (Agent P): Add structured ApplyFileDiffsResult.Failure types (QUALITY-1264) warp-proto-apis#351
• server (Agent S): https://github.com/warpdotdev/warp-server/pull/13383
• client (this PR)

What changed

New types in crates/ai (layering-safe):

• DiffApplicationFailure enum mirroring all proto Failure variants, with redacted Debug to prevent sensitive content from leaking into logs
• DiffSearchBlockFailure carrying per-block search text + expected line range
• render(&[DiffApplicationFailure]) -> String — the single rendering path for all consumers, reproducing the current agent-facing wording verbatim
• failures_from_proto_error() — canonical read-back conversion from proto, re-exported from ai::agent::action_result

DiffMatchFailures enrichment (crates/ai/src/diff_validation/):

• Add search_block_failures: Vec<DiffMatchFailure> with #[serde(skip)] to prevent telemetry leakage
• Add MAX_DIFF_MATCH_FAILURE_BYTES = 1024 cap applied at proto-conversion time
• Drop #[derive(Copy)]; add redacted Debug impl
• Populate per-block detail in fuzzy_match_file_diffs when a search block fails to match

Shape change:

• RequestFileEditsResult::DiffApplicationFailed { error: String } → { failures: Vec<DiffApplicationFailure> }
• All consumers updated to call render() instead of interpolating the string

API boundary (convert.rs):

• Populates both Error.failures (structured) and Error.message (back-compat fallback)
• diff_application_failure_to_proto() handles the UnmatchedDiffs → also emits ChangesAlreadyApplied when noop_deltas > 0
• 0 sentinel for unknown line ranges (matches server renderer's > 0 checks)

Read-back (convert_conversation.rs):

• Delegates to ai::agent::action_result::failures_from_proto_error() — reconstructs from proto when present, wraps message in Opaque for old entries

Three construction sites updated:

• Diff-match failures (request_file_edits.rs) → errors_to_failures()
• Save failures (diff_storage.rs) → Opaque
• Review-surface-vanished (code_diff_view.rs) → Opaque

Linked Issue

• The linked issue is labeled ready-to-spec or ready-to-implement.
• Where appropriate, screenshots or a short video of the implementation are included below.

Linear: https://linear.app/warpdotdev/issue/QUALITY-1264/structured-apply-diff-errors-server-controlled-edit-files-failure

Testing

Tests added:

File	Coverage
diff_application_failure_tests.rs (new, per repo convention)	render() wording lock-in for all variants; redacted Debug for file paths and search text
convert_tests.rs (extended)	Each DiffApplicationFailure variant → expected proto Failure.kind; Error.message always populated; truncated set when capped; 0 sentinel for unknown ranges; ChangesAlreadyApplied emitted separately when noop; read-back: message-only → Opaque; failures → category; unknown kind skipped
diff_application_tests.rs (updated)	DiffMatchFailures struct literal updated for new search_block_failures field
tui_diff_storage_tests.rs (updated)	DiffApplicationFailed { failures: _ } match pattern

Checks that ran in this environment:

Check	Result
cargo test -p ai	✅ 365/365 passing
./script/format	✅ clean
cargo clippy -p ai --all-targets --tests -- -D warnings	✅ clean
Full ./script/presubmit (including cargo check on the full warp binary)	❌ could not run — sandbox OOM kills the warp compilation. The warp crate is ~1M LOC and exceeds sandbox memory. Reviewer should run ./script/presubmit locally.

No UI changes — this is a wire-format + agent-string change. The agent-facing strings are unchanged (render() reproduces the current wording exactly).

Agent Mode

• Warp Agent Mode - This PR was created via Warp's AI Agent Mode

CHANGELOG-NONE

Conversation: https://staging.warp.dev/conversation/f0f183a2-1073-4220-b2e6-4f1fd8e145c1
Run: https://oz.staging.warp.dev/runs/019fa5f3-f6aa-7721-92f8-b272e2dc7eb8

This PR was generated with Oz.

[oz-for-oss]

@warp-dev-github-integration[bot]

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 /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This PR moves edit_files apply-diff failures from a string-only client result to structured Rust/client types with proto conversion, read-back support, local rendering, tests, and related specs.

Concerns

• The real diff-application path truncates search-block text before proto conversion, so over-cap search blocks lose the truncated signal even though direct conversion tests pass.
• Read-back can drop the entire failure message when structured proto failures are present but all entries are unrecognized/empty.

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

[warp-dev-github-integration]

Overview

Structured DiffApplicationFailure for edit_files failures in the Rust client, per specs/structured-apply-diff-errors/{PRODUCT,TECH}.md (carried in this PR). Reviewed against the spec, the linked issue, and the sibling proto (#351) / server (#13383) PRs.

The design is sound and several of the spec's hardest requirements are met well:

• Single rendering path confirmed. Display (action_result/mod.rs:748), the markdown render (app/src/ai/agent/mod.rs:1246), both SDK sites (agent_sdk/driver/output.rs:102, :894), and the proto message field all route through the one render() helper. The TUI (tui_file_edits_view.rs) deliberately keeps a static label and re-implements no wording. Nothing is scattered.
• No-regression wording is genuinely locked in. render_one() reproduces to_conversation_message() from master byte-for-byte, and diff_application_failure_tests.rs asserts against the old strings rather than against current output.
• Sensitive-data hardening is real. Manual redacted Debug on DiffMatchFailure, DiffMatchFailures, DiffApplicationFailure, and RequestFileEditsResult; #[serde(skip)] on search_block_failures; three leak tests that assert the search text and path do not appear in {:?}. PRODUCT 7 is satisfied.
• Back-compat both directions. convert.rs populates both Error.failures and Error.message; failures_from_proto_error reconstructs Opaque from a message-only proto and the right category from structured failures; unknown kinds are skipped rather than panicking. Proto is pinned to e10758e, and the 0 sentinel / >0 guard for unknown line ranges matches the server's > 0 checks.
• Test placement follows the repo's ${file}_tests.rs + #[path] convention throughout.

However, the branch does not build, and one spec invariant is not actually satisfied on the production code path despite a passing test. Requesting changes.

Concerns

Blocking

1. CI is red — the branch does not compile. Formatting + Clippy fails on Linux, MacOS, and wasm (Linux job). Two distinct errors:

• app/src/ai/blocklist/diff_storage_tests.rs:108 is an unmigrated consumer — it still destructures DiffApplicationFailed { error }:

  error[E0026]: variant `RequestFileEditsResult::DiffApplicationFailed` does not have a field named `error`
  error[E0027]: pattern does not mention field `failures`
     --> app/src/ai/blocklist/diff_storage_tests.rs:108:13
  

  This site is not in TECH.md's consumer list, which is likely why it was missed. It is the only remaining one — I grepped every DiffApplicationFailed occurrence on the branch and the other eight are all migrated.

• to_conversation_message (diff_application.rs:97) and error_for_conversation (:143) are now unreachable, and -D warnings denies dead_code:

  error: associated items `to_conversation_message` and `error_for_conversation` are never used
  

  TECH.md "Follow-ups" defers deleting this path, but it can't be deferred here — it no longer compiles. Since render() now owns the wording, deleting both (and the now-unused Vec1/itertools imports, if they fall out) is the right fix rather than #[allow(dead_code)].

  This is exactly the gap flagged in the implementation notes: cargo test -p ai passing does not cover the app-crate consumer edits. CI does cover them, and it caught both.

2. SearchBlockFailure.truncated is always false in production — PRODUCT invariant 8 is not met. The byte cap is applied twice, and the flag is dropped at the site that can actually observe it:

• error_to_failures (diff_application.rs:851-865) truncates b.search to MAX_DIFF_MATCH_FAILURE_BYTES, computes truncated, and then discards it (let _ = truncated;) because DiffSearchBlockFailure has no truncated field.
• convert.rs:359 then re-tests b.search.len() > MAX_DIFF_MATCH_FAILURE_BYTES — but the text has already been capped upstream, so this is never true and truncated is hardcoded-false in practice.

The two truncation tests pass only because they construct DiffSearchBlockFailure { search: "x".repeat(MAX + 100) } directly, bypassing error_to_failures. The server therefore can never tell the agent a block was shortened.

Suggested fix: apply the cap in exactly one place. Either drop the truncation from error_to_failures and let convert.rs own it (simplest — matches the doc comment on DiffSearchBlockFailure, which says it carries the raw, pre-truncation text), or add a truncated: bool to DiffSearchBlockFailure and have convert.rs pass it through. Please also add a test that drives the real path (error_to_failures → convert) end-to-end, since the current tests would stay green either way.

Major (please address)

3. render() can return an empty string — PRODUCT invariant 9 regression. error_for_conversation(&Vec1<...>) was type-level non-empty; render(&[...]) is not, and the doc comment concedes 0 failures → "". Two reachable paths produce an empty agent-facing message:

• an empty failures slice;
• UnmatchedDiffs { fuzzy_match_failure_count: 0, changes_already_applied_count: 0 }, which render_one renders to "" and which diff_application_failure_to_proto maps to zero proto entries — yielding Error { message: "", failures: [] }, so the server has nothing to fall back on either.

A cheap guard in render() (fall back to a generic non-empty message when the composed string is empty) closes both, and matches what the spec asks the server-side renderer to do.

4. V4A unmatched diffs ship no per-block detail. fuzzy_match_v4a_diffs (crates/ai/src/diff_validation/mod.rs:477) increments failures.fuzzy_match_failures but never pushes a DiffMatchFailure, unlike the search/replace path at :680. So for V4A diffs the server receives search_block_failures: [] and cannot enumerate the failed blocks (PRODUCT criterion 2). Either populate it from the V4A hunk context or add a comment explaining why V4A is intentionally detail-free.

5. Proto round-trip is not stable for UnmatchedDiffs + no-op. Encoding splits one UnmatchedDiffs { changes_already_applied_count: N } into two proto entries (UnmatchedDiffs + ChangesAlreadyApplied), but proto_failure_to_diff_application_failure reads UnmatchedDiffs back with changes_already_applied_count: 0. A read-back therefore yields two failures, and render() flips from the combined single-line wording to bulleted output:

before: "Could not apply all diffs to f.rs. The changes to f.rs were already made."
after:  "* Could not apply all diffs to f.rs.\n* The changes to f.rs were already made."

PRODUCT 13 only requires read-back not to error, so this isn't blocking, but it does change how historical conversations render locally. Worth either fixing (re-merge adjacent entries for the same file on read-back) or documenting as accepted.

Minor

6. u.fuzzy_match_failure_count as u8 (convert.rs:484) silently wraps a proto u32 (e.g. 256 → 0). Prefer u8::try_from(...).unwrap_or(u8::MAX).

7. hint_range_for_failure (diff_validation/mod.rs:565) is snapshotted before the out-of-bounds line_range.filter(...) at :572, so a range the matcher subsequently rejected can still be reported to the agent as the expected location. Probably fine as a hint, but worth a comment.

Cross-repo (FYI only — not defects in this PR)

The 0-sentinel encoding, the >0 guards, and the proto pin all line up with #351 and #13383. The 1-indexed exclusive→inclusive conversion (r.end - 1) is correct given parse_line_numbers returns start..end+1.

Not applicable

This is a headless wire-format + agent-facing-string change with no user-visible UI surface, so visual/computer-use proof is not required and its absence is not held against the PR.

Verdict

Rejected — changes required. The branch does not compile (two -D warnings/type errors caught by CI, one an unmigrated consumer and one a now-dead wording path), and SearchBlockFailure.truncated is inert on the real code path despite green tests, so PRODUCT invariant 8 is not actually met. Items 3-5 are also worth addressing before merge. Everything else — the single render path, byte-for-byte wording preservation, the redaction work, and both back-compat directions — looks good and should survive the fix unchanged.

Review run

https://oz.staging.warp.dev/runs/019fa63c-73fb-7e99-82b6-6eb8613343c3

[warp-dev-github-integration]

Carry-forward note (cumulative review). Two prior unresolved threads from the implementation pass are still outstanding at head 850c86b, and my review independently reproduces both — leaving them open rather than duplicating:

• diff_application.rs:849 — the double byte-cap making truncated always false. Same root cause as blocking item 2.
• convert.rs:467 — failures_from_proto_error collecting an empty Vec when every proto entry has an unrecognised kind, which drops the message fallback. This is a third empty-render path on top of the two in major item 3, and the suggested fix there (fall back to Opaque { message } when decoding yields nothing) is the right one; please apply it alongside the render() guard.

[warp-dev-github-integration]

Overview

Re-review of the rework (cycle 1) at f5f043f, cumulative against the prior review at 850c86b. I verified each prior finding against the diff rather than the rework summary. All 7 are genuinely fixed, and I found no regressions in the parts of the change that were already sound. The cycle-1 blocker "branch does not compile" is closed: gh pr checks at this head is fully green — Formatting + Clippy on Linux/macOS/Windows/wasm, Run Linux/MacOS/Windows tests, and every release-flag compilation job all SUCCESS. That CI run (fmt + clippy + the full test suites on three platforms at this exact SHA) is the build/test gate for this review; a cold local rebuild of the workspace would be strictly weaker.

Prior findings — confirmed fixed

1. [was BLOCKING] Double byte cap (PRODUCT 8). Real fix. error_to_failures now stores b.search.clone() raw (diff_application.rs:790-793) and the cap exists only in diff_application_failure_to_proto (convert.rs:358-367). I scrutinised the new test as asked: error_to_failures_preserves_raw_search_and_proto_sets_truncated starts from a DiffApplicationError, runs the real errors_to_failures, asserts the raw text survives uncapped, then runs the real TryFrom<RequestFileEditsResult> proto conversion and asserts truncated == true with search.len() <= MAX. It exercises both production functions and would have failed before the fix — not a shortcut. (One narrow gap noted below.)
2. [was BLOCKING/IMPORTANT] failures_from_proto_error dropping the message fallback. Fixed at convert.rs:470-476. It cannot return an empty Vec: the all-unrecognised path returns Opaque, and the merge loop only removes an entry when a preceding UnmatchedDiffs survives. Covered by read_back_all_unrecognised_falls_back_to_opaque.
3. [was MAJOR] Empty render() for UnmatchedDiffs{0,0} (PRODUCT 9). Fixed — render_one has a defensive fallback (diff_application_failure.rs:235-237). No DiffApplicationFailure variant can now render to "". See the minor note on the empty-slice case.
4. [was MAJOR] Round-trip asymmetry for UnmatchedDiffs + ChangesAlreadyApplied. I judged this one specifically, since merging on read-back is a behavioural choice rather than obviously-lossless. I think it is correct here. The mis-merge you would worry about — a genuine standalone ChangesAlreadyApplied legitimately following an UnmatchedDiffs for the same file — is unreachable: DiffApplicationError has no ChangesAlreadyApplied variant, so error_to_failures can never emit that pairing, and the only producer of it is diff_application_failure_to_proto's own split. The decode path is used solely by convert_conversation.rs:807 to read back client-authored persisted data. So the merge is unambiguous in practice, and PRODUCT 13 ("must not error") holds. Two non-blocking caveats inline.
5. [was MAJOR] V4A unmatched diffs carrying no per-block detail. Fixed — the None arm of fuzzy_match_v4a_diffs pushes DiffMatchFailure { search: diff.old.clone(), expected_range: None }.
6. [was MINOR] u32 as u8 silent truncation. Fixed with u8::try_from(...).unwrap_or(u8::MAX) (convert.rs:527). It composes correctly with #3: clamping to MAX guarantees > 0, so the decode path can never land in the zero-count case; the #3 fallback covers only a genuinely zero-count encode. No interaction bug.
7. [was NIT] Snapshot comment. Fixed — the comment now explains that hint_range_for_failure is captured before the out-of-bounds filter and that this is intentional.

No regressions in the previously-sound work

• Single render() path holds: Display (action_result/mod.rs:751), markdown (agent/mod.rs:1246), SDK text + JSON (output.rs:102/:894) and the proto message all call diff_application_failure::render. The TUI shows a static "File edits failed" label with no duplicated wording.
• Wording lock-in is byte-identical to master's to_conversation_message/error_for_conversation, including the single-entry no-prefix vs. "* "-joined multi-entry shape. Verified against af6fc40.
• Redaction (PRODUCT 7) is thorough: manual Debug on DiffMatchFailure, DiffMatchFailures, DiffSearchBlockFailure, DiffApplicationFailure and RequestFileEditsResult, plus #[serde(skip)] on the telemetry-serialised DiffMatchFailures::search_block_failures, with leak tests. Worth calling out that the pre-existing safe_warn!(safe: ("Failure(s) applying diff: {failures:?}")) sites now route through the redacted Debug — that is exactly the leak invariant 7 targets, and it is closed.
• Back-compat both directions (PRODUCT 10-13), the 0-sentinel range encoding (round-trips 5..11 → (5,10) → 5..11), and the proto pin to e10758eae44a20a6c681bdda40bab0a35e4e858e all check out.

Scope

Headless wire-format + agent-facing-string change with no user-visible UI surface, so visual/computer-use proof is not applicable and its absence is not held against this PR.

Concerns

All non-blocking; see the inline comments.

• MINOR render(&[]) still returns "", and that value feeds Error.message for old servers. Unreachable today, but guarded only by a doc comment.
• MINOR The two fixes that are pure defensive code (#3's zero-count fallback and #6's clamp) have no regression tests — the same shape of "fixed but unexercised" code that regressed in cycle 1.
• MINOR The merge loop's doc says "consecutive" but the code scans every earlier index.
• MINOR error_to_failures' variant mapping is only covered for UnmatchedDiffs; TECH.md's testing plan asked for the full DiffApplicationError → DiffApplicationFailure table.

Cross-repo observation (not a defect)

The server keeps richer unmatched-diffs wording (the retry instruction plus a per-block enumeration) rather than matching this client's render(). That is correct and I am not flagging it: PRODUCT item 3 requires that content in the agent-facing message, which the server now owns, and TECH.md's risk section explicitly sanctions the client's render() diverging once the server owns wording. PRODUCT non-goal 3 also puts the client's local display out of scope. I do not think the client's render() should be updated to match — enumerating raw search blocks in the GUI/SDK/TUI transcript would be noise for a human reader, and keeping render() pinned to today's strings is what makes the no-regression lock-in test meaningful. Flagging only so the divergence is a recorded decision rather than drift.

Verdict

ACCEPTED. All seven prior findings are genuinely resolved, the invariant-8 fix is backed by a test that drives the real code path, the round-trip merge is sound for this codebase, and CI is green at f5f043f. The four remaining items are minor and can be picked up here or as follow-ups at the author's discretion — none should block the merge. Reminder that merge order is proto → server → client, and this client PR lands last.

(Posted as a COMMENT event rather than a formal approval: GitHub rejects an approve/request-changes event from the PR author's own app identity.)

Review run

https://oz.staging.warp.dev/runs/019fa699-07f3-7da1-89a7-4f65d0ddf4c2

[warp-dev-github-integration]

MINOR: render(&[]) returns "". The docstring above acknowledges this ("callers should treat this as unexpected"), but this value is also what populates Error.message, which is the only thing an old server shows the agent (PRODUCT 11). An empty failure list would therefore produce an empty agent-facing error — the exact class of thing PRODUCT 9 exists to prevent.

It is unreachable today, and I checked all four construction sites to confirm: request_file_edits.rs:156 maps from a Vec1 so it yields >= 1; diff_storage.rs:178 and code_diff_view.rs:363 each push exactly one Opaque; and failures_from_proto_error can't return empty. So this is latent, not live.

Still, given the whole point of the change is invariant 9, a hard guarantee is cheap and removes the need for the caller contract:

pub fn render(failures: &[DiffApplicationFailure]) -> String {
    let messages: Vec<String> = failures.iter().map(render_one).collect();
    match messages.len() {
        0 => "Could not apply the requested file edits.".to_string(),
        1 => messages.into_iter().next().unwrap_or_default(),
        _ => messages.iter().format_with("\n", |msg, f| f(&format_args!("* {msg}"))).to_string(),
    }
}

Non-blocking.

[warp-dev-github-integration]

MINOR (doc/code mismatch): the comment above and the doc on failures_from_proto_error both say consecutive ChangesAlreadyApplied entries are merged, but (0..i).rev().find(...) scans every earlier index. So [UnmatchedDiffs{A}, MissingFile{B}, ChangesAlreadyApplied{A}] merges across the intervening MissingFile{B}, which is not what "consecutive" describes and quietly reorders the failure list that PRODUCT item 1 says is order-preserving.

This is unreachable from the encoder (the split always emits the pair adjacently), so it's a documentation-accuracy issue rather than a live bug. Either tighten the search to i.checked_sub(1) so it genuinely only looks at the immediately-preceding entry, or reword the comments to say "the most recent preceding". I'd slightly prefer the former since it makes the inverse of diff_application_failure_to_proto exact.

Separately, worth a one-line comment: changes_already_applied_count: N > 1 encodes to a single ChangesAlreadyApplied entry and decodes back to 1. That's invisible to render() (which only tests > 0), so it's harmless today — but it is a real fidelity loss that a future consumer of the count could trip over.

[warp-dev-github-integration]

MINOR (test gap): the clamp is the right fix and I verified it composes correctly with the render_one zero-count fallback — unwrap_or(u8::MAX) guarantees > 0, so the decode path can never reach the empty-render case. But there is no test for it, and likewise none for the UnmatchedDiffs { fuzzy_match_failure_count: 0, changes_already_applied_count: 0 } fallback in render_one.

Both are pure defensive branches, which is precisely the shape of code that silently rotted in cycle 1 (the byte cap looked correct and was dead). Two cheap asserts would pin them:

#[test]
fn read_back_oversized_fuzzy_count_clamps_to_u8_max() {
    // 300 must not wrap to 44, and must never become 0 (PRODUCT 9).
}

#[test]
fn render_unmatched_with_zero_counts_is_non_empty() {
    // PRODUCT 9: no category renders to "".
}

Non-blocking.

[warp-dev-github-integration]

Nit / coverage: this test is the right shape and genuinely drives the production path, so the invariant-8 fix is properly pinned. Two small gaps worth noting for a follow-up:

1. It hand-builds DiffMatchFailures, so it would not catch a cap accidentally reintroduced upstream inside fuzzy_match_file_diffs. I confirmed by reading that function that it stores search.to_owned() uncapped, so the invariant holds today — but the test does not defend that end of the pipeline. Driving one oversized search block through apply_edits against a temp file would close the loop.
2. errors_to_failures mapping is only asserted for UnmatchedDiffs. TECH.md's testing plan asks for the full DiffApplicationError -> DiffApplicationFailure table; the other nine arms are trivial 1:1 maps but currently untested. A single table-driven test would cover them.

Non-blocking.