Report accurate read_files errors for oversized/unprocessable files (APP-4882)

[warp-dev-github-integration]

Description

When the agent's read_files/get_files/search_codebase tools read a local file that exists but exceeds the per-file size cap (1 MB), the client returned These files do not exist: <path>. That message was wrong — the file exists and is readable, it is just too large. A real user hit this on a ~3.5 MB JPEG that failed repeatedly while an 851 KB downscaled copy of the same image succeeded.

Root cause: a single failure variant (BinaryFileReadResult::Missing) conflated five distinct failure reasons (too large, not found, image-processing errors), so every consumer flattened them into the same "do not exist" message.

Reconciled with master (Suraj's partial-file-read work)

This branch was rebased onto current master, which already landed the partial-file-read changes: ReadFileContextResult carries failed_files: Vec<ReadFilesFailedFile> ({ path, message }), read_files returns partial success (Success { files, failed_files }) when only some files fail, and server_model.rs already forwards each message into the proto FailedFileRead.error.message. This change is layered on top of that work (nothing there is undone); it only fixes the still-conflated failure reason.

Fix

• Split BinaryFileReadResult::Missing into NotFound, TooLarge { size_bytes, limit_bytes }, and ProcessingFailed { detail } (matched exhaustively, no _ arm).
• read_binary_file_context returns the specific variant at each site (oversized file / oversized-after-processing → TooLarge; missing → NotFound; image over-limit or decode/resize error → ProcessingFailed).
• read_local_file_context maps each variant to a ReadFilesFailedFile { path, message } with a concise, path-free reason (the path is carried in its own field, so the existing "{path}: {message}" renderers stay clean):
  • Not found → File does not exist
  • Too large → File is too large to read (3.5 MB > 1.0 MB limit). Downscale/compress it or read a smaller copy.
  • Processing failed → File could not be processed as an image: <detail>
• Added a shared describe_failed_files(&[ReadFilesFailedFile]) helper; read_files, get_files, and search_codebase now build their combined error from it instead of the misleading "These files do not exist" text.
• ProcessImageResult is left API-compatible (unit TooLarge variant unchanged), so its other callers (the TUI attachment path) are untouched; image over-limit/decode failures map to ProcessingFailed.
• passive_suggestions/legacy.rs now logs the failed-files breadcrumb via safe_warn! so absolute paths / per-file details are not uploaded in crash-reporting builds.
• server_model.rs needs no change — it already forwards ReadFilesFailedFile.message into the proto per-file error.

Out of scope: master's partial-success semantics are preserved as-is; the 1 MB cap and image limits are unchanged.

Linked Issue

APP-4882

Testing

This change affects the read_files/get_files/search_codebase tool-result message text (consumed by the agent/model), not a rendered UI surface, so its correctness is captured deterministically by unit tests on the exact strings — no computer_use/screenshot proof required.

Added unit tests (execute_tests.rs, gated all(test, feature = "local_fs")):

• oversized_binary_file_reports_too_large_not_missing — oversized existing file → message names the size and limit and does not say "does not exist" (regression for this bug).
• missing_file_reports_does_not_exist — nonexistent path → File does not exist.
• unprocessable_image_reports_processing_failure — invalid image bytes → "could not be processed as an image", distinct from the other two.
• describe_failed_files_groups_each_reason_per_file — mixed batch names each reason per file.

Validated:

• cargo test -p warp --features local_fs read_file_failures — 4/4 new tests pass; existing binary-detection tests unaffected.

• cargo clippy -p warp --features local_fs --tests -- -D warnings — clean.

• ./script/format — clean.

• I have exercised the affected code paths via the new regression tests.

Agent Mode

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

Originating thread: https://warp-public.slack.com/archives/C0BDQDW8V5E/p1784468227182559

Conversation: https://staging.warp.dev/conversation/0df69751-768d-4ea1-b3ba-cc21d03e1f50
Run: https://oz.staging.warp.dev/runs/019f8557-051d-7ae2-8f09-8f5ad851ba2e
Plans:

• Spec: Fix misleading "These files do not exist" error for oversized files in read_files

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 splits file-read failures into structured reasons and updates the local tool consumers, remote proto mapping, image processing result, and regression tests so oversized or unprocessable files no longer look nonexistent.

Concerns

• Changing ProcessImageResult::TooLarge to a struct variant requires updating all existing match sites; at least the TUI image attachment processing callers still use the old unit pattern, so the workspace will not compile.
• The new tests match reason by value through borrowed/indexed FileReadFailure values, which moves a non-Copy enum out of a borrow and will fail to compile.

Security

• The changed warn-level passive-suggestions log now emits content.failed_files, including absolute paths and processing details, into release-channel breadcrumbs; use safe logging or omit per-file details from the uploaded breadcrumb.

Verdict

Found: 0 critical, 3 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

This change fixes a real, well-scoped bug — read_files reporting an existing-but-oversized file as "These files do not exist" — and the implementation itself is high quality: a structured FileReadFailureReason enum with exhaustive matches (no _ arm), a describe_failures grouping helper, an accurate format_mb-based message, correct threading through all consumers, and thorough unit tests (oversized→TooLarge, missing→NotFound, bad-image→ProcessingFailed, mixed-batch grouping, and the proto mapping). Conventions (inline format args, exhaustive matches) are followed. On its own terms the diff is solid.

Concerns

The blocking problem is that this PR is out of date with master and cannot be merged. GitHub reports it as CONFLICTING / mergeStateStatus: DIRTY. This is not a trivial textual conflict — master has already restructured the exact code this PR touches: ReadFileContextResult.missing_files: Vec<String> was already renamed to failed_files: Vec<ReadFilesFailedFile>, where ReadFilesFailedFile { path: String, message: String } carries a per-file message string end-to-end (through server_model.rs, read_files.rs, and convert_conversation.rs). This PR was branched from the older 69ce372 base and instead introduces a different struct (FileReadFailure { path, reason: FileReadFailureReason }), so the two designs collide directly in execute.rs, server_model.rs, and the tool consumers.

A second, related staleness problem: this PR explicitly declares partial-success batch semantics a non-goal ("when any file in a batch fails, the local consumers still return an Error"), but master has already implemented partial success — ReadFilesResult::Success now carries a failed_files field and read_files.rs distinguishes "all failed" (Error) from "some failed" (Success with failed_files). So the PR's stated scope boundary no longer matches the codebase it must merge into.

The good news for the rework: because master already threads a per-file message: String through ReadFilesFailedFile, the fix becomes much smaller than what this PR built. The underlying bug still exists on master — at the two failure sites in execute.rs (the NotFound metadata push and BinaryFileReadResult::Missing, plus the oversized/image branches in read_binary_file_context) the per-file message is still the generic "File not found or could not be read", and the local tool consumers (get_files.rs, search_codebase.rs) still ignore the message and re-emit "These files do not exist: ". So the fix is still needed and valuable; it just needs to be re-implemented on top of master's current structure.

Requested path forward: rebase this PR onto current master and reconcile with the already-landed ReadFilesFailedFile { path, message } design. Concretely, decide whether to (a) keep ReadFilesFailedFile and simply compute an accurate per-file message at each failure site in read_binary_file_context / read_local_file_context (splitting BinaryFileReadResult::Missing so the size/limit are available), and update get_files.rs / search_codebase.rs to surface f.message instead of the hardcoded "do not exist" text — this is the minimal reconciliation; or (b) layer the structured FileReadFailureReason enum onto master's struct if the grouping/structure is judged worth it. Also re-evaluate the "partial-success out of scope" non-goal against master, which already ships it, and update the spec accordingly. Please also update the spec file's commit-pinned references (currently anchored at 69ce372) after the rebase.

Verdict

Found: 1 critical, 1 important, 0 suggestions

Request changes

[szgupta]

LG - ideally we'd pass a structured error up to server and let it format the error however it wants (makes it easier to adapt the error message for the specific model, for example), but this is a net improvement regardless.

[warp-dev-github-integration]

Overview

This rework cleanly resolves APP-4882. It rebases onto current master and layers on top of the already-landed partial-file-read work (ReadFilesFailedFile { path, message }, partial-success Success { files, failed_files }, and server_model.rs proto forwarding) without undoing any of it. The root cause — BinaryFileReadResult::Missing conflating five distinct failure reasons — is fixed by splitting it into NotFound, TooLarge { size_bytes, limit_bytes }, and ProcessingFailed { detail }, matched exhaustively with no _ arm, and mapping each to a reason-accurate ReadFilesFailedFile message. A shared describe_failed_files helper now backs read_files, get_files, and search_codebase, eliminating the misleading "These files do not exist" text for oversized/unprocessable files. ProcessImageResult is left API-compatible, so the TUI attachment path is untouched. Implementation matches the committed spec (agents/specs/APP-4882 - ...md) across all 7 behaviors and 9 validation criteria — no material drift.

Concerns

No blocking issues. Correctness, standards, and scope are sound: inline format args are used where valid (the "{path}: {message}" positional form is correct since field accesses can't be inlined), matches are exhaustive, unused params aren't introduced, and the safe_warn! change in passive_suggestions/legacy.rs correctly keeps absolute paths out of release-channel breadcrumbs (addressing the prior security thread). All four prior review threads (ProcessImageResult API break, non-Copy enum borrow, safe_warn PII, stale spec section) are addressed and resolved. Four targeted regression tests cover each variant plus the batch helper.

Minor (non-blocking): format_mb renders one decimal, so a file only marginally over the cap (e.g. ~1.04 MB) prints as "1.0 MB > 1.0 MB limit", which reads as self-contradictory for that narrow size window. The message still correctly says "too large to read", so this is cosmetic — worth considering (e.g. show bytes when the two round equal) but not merge-blocking.

Testing note

The warp workspace could not be built/tested in this review sandbox (the compile was OOM-killed under the environment's memory limit), so I relied on CI: Formatting + Clippy (Linux/MacOS/wasm) and Verify compilation with release flags (Linux/MacOS/Windows/wasm) are all green, and the implementation reported the 4 new regression tests passing locally. The Linux/MacOS/Windows test-suite runs were still in progress at review time — confirm they are green before merge.

Verdict

Found: 0 critical, 0 important, 1 suggestion

Approve