spec: unified file-read interface (#10049)#10222
spec: unified file-read interface (#10049)#10222lonexreb wants to merge 5 commits intowarpdotdev:masterfrom
Conversation
|
I'm starting a first review of this spec-only 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 |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec proposes a centralized safe file-read API, staged migration of high-traffic callers, and lint enforcement against raw file reads. No approved spec context was available, so issue alignment is limited to the PR's stated GH-10049 scope.
Concerns
- The Clippy enforcement plan is not implementable as written because it relies on path/test exceptions and warn-vs-deny behavior that the spec does not define.
- The spec bans async read methods while declaring async safe helpers out of scope, leaving no compliant async path once the lint is promoted.
- The text-read contract does not define invalid UTF-8 or non-NUL binary behavior even though the API returns
String.
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
| - B1. `read_text_file_safe` rejects files larger than | ||
| `opts.max_bytes` (default: editor large-file threshold, | ||
| ~8 MiB) with `ReadError::TooLarge { actual, max }`. | ||
| - B2. `read_text_file_safe` rejects files whose first 8 KiB |
There was a problem hiding this comment.
read_text_file_safe returns String; specify how invalid UTF-8 and non-NUL binary-like content are classified and which ReadError variant callers should handle.
| - B4. Both helpers stream-read for files larger than 1 MiB and | ||
| short-circuit on the size check before allocating the full | ||
| buffer. | ||
| - B5. A new clippy `disallowed_methods` config in |
There was a problem hiding this comment.
.clippy.toml disallowed_methods does not by itself define path-scoped or test-scoped exceptions, and this section also says the methods are denied while B6 says V1 is warn-only; specify the actual enforcement mechanism for helper/test exemptions and the V1/V2 lint levels.
|
|
||
| ## Out of scope (V1) | ||
|
|
||
| - Async/await variants (`read_text_file_safe_async`). The five V1 |
There was a problem hiding this comment.
tokio::fs::read{,_to_string} in the restricted methods; either keep Tokio reads out of the deny phase until an async-safe helper exists or include the async API in the migration plan.
|
Round-1 fixes for oz-for-oss review: (1) added Lint Enforcement section defining clippy |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec proposes a centralized safe-read interface with size limits, text/binary checks, migration targets, lint enforcement, and tests. The direction is clear, but several requirements need to be tightened before implementation can proceed reliably.
Concerns
- The public API references an
optsparameter without defining its type, defaults, override rules, or required behavior. - The binary-content contract only checks for NUL bytes in the first 8 KiB, which does not meet the broader stated goal of preventing binary/non-text content from entering text pipelines unless the limitation is explicitly accepted and tested.
- The V2 lint promotion plan says to change lint levels in
.clippy.toml, but that file configures lint data rather than lint severity; the spec needs to state wherewarn/denyis applied.
Verdict
Found: 0 critical, 3 important, 1 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
|
|
||
| V1 (sync only — ships first): | ||
|
|
||
| - `safe_read::read_to_string(path, opts) -> Result<String, SafeReadError>` |
There was a problem hiding this comment.
opts, but the spec never defines the options type, default values, required fields, or how callers override max_bytes; add a SafeReadOptions contract before implementation.
| files larger than `opts.max_bytes` (default: editor large-file | ||
| threshold, ~8 MiB) with | ||
| `SafeReadError::TooLarge { actual, max, path }`. | ||
| - B2. Text reads reject files whose first 8 KiB contain a NUL byte |
There was a problem hiding this comment.
|
|
||
| ### Warn → deny promotion plan | ||
|
|
||
| - **V1**: lint level is `warn`. The four `safe_read` sync helpers |
There was a problem hiding this comment.
💡 [SUGGESTION] V1 lists two sync helpers above, so four safe_read sync helpers contradicts the Public API section; change this to two sync helpers or move the total helper count to the V1.5 step.
| sites. Lint level remains `warn`. | ||
| - **V2**: once the open-warning count from `cargo clippy --all-targets` | ||
| reaches zero on `master` for one release cycle, promote the lint | ||
| level to `deny` in `.clippy.toml`. From this point forward, any new |
There was a problem hiding this comment.
.clippy.toml configures disallowed-method entries but does not set lint severity, so the V2 deny plan is not implementable as written; specify whether warn/deny is applied through workspace lint config, crate attributes, or CI clippy flags.
|
Round-2 fixes: (1) |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec introduces a unified safe file-read API, migration plan, lint enforcement, and test coverage for replacing ad-hoc file reads. The overall structure is clear, but several requirements are incomplete enough that different implementers could produce materially different behavior or accidentally weaken existing lint policy.
Concerns
- The behavior contract does not specify error precedence when size, symlink, binary, and UTF-8 checks overlap.
- The
.clippy.tomlplan treats the file as new/replacement even though the repository already has existing Clippy policy that must be preserved. - The lint rollout defers the exact crate-root coverage list to the implementation PR, leaving the spec and audit test without concrete acceptance criteria.
Security
- The spec recommends
Strictfor AI/UI surfaces, but it ships after V1 while V1 migrates high-traffic call sites; the follow-up migration requirement should be explicit so prompt/UI reads do not remain on the weaker heuristic indefinitely.
Verdict
Found: 0 critical, 3 important, 1 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
| non-UTF-8 content. | ||
| - B5. Byte reads (`read_bytes` / `async_read_bytes`) enforce | ||
| `max_bytes` only — no NUL/UTF-8 check. | ||
| - B6. All four helpers stream-read for files larger than 1 MiB and |
There was a problem hiding this comment.
max_bytes that also has NUL/invalid UTF-8, or a symlink with follow_symlinks = false. Specify check order and expected errors so implementations and tests are deterministic.
| - `crates/warp_files/src/error.rs` (new — `SafeReadError` enum). | ||
| - `crates/warp_files/src/opts.rs` (new — `SafeReadOpts` struct, | ||
| `BinaryCheck` enum, and the `Default` impl). | ||
| - `.clippy.toml` (new file — disallowed-methods data only; no |
There was a problem hiding this comment.
.clippy.toml; this spec treats it as new/replacement and the snippet omits existing disallowed entries. Require appending or merging the file-read entries while preserving current lint policy.
| - Crate-root attributes: `#![warn(clippy::disallowed_methods)]` added | ||
| to the `lib.rs` / `main.rs` of every production crate in the | ||
| workspace in V1; flipped to `#![deny(...)]` in V2. The exact set | ||
| of crate roots is enumerated in the V1 implementation PR. |
There was a problem hiding this comment.
| plus valid multi-byte UTF-8 starts, the read returns | ||
| `BinaryContent`. | ||
|
|
||
| `Strict` ships in V1.5 alongside the async helpers. AI/UI surfaces |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Strict is recommended for AI/UI prompt surfaces but ships after the V1 migrations. Add a tracked V1.5 migration requirement for those call sites so non-text-but-valid-UTF-8 content does not continue flowing into prompts/UI under the default heuristic.
|
Addressed all 3 important + 1 suggestion concerns from latest oz-for-oss review: B7 — Error precedence (important #1) .clippy.toml preserves existing policy (important #2) Concrete crate-root coverage list (important #3) Strict for AI/UI is a hard V1.5 requirement (suggestion) |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec proposes a centralized safe-read API, staged sync/async rollout, and clippy enforcement for raw file reads. The direction is clear, but a few design gaps and contradictions need to be resolved before it is reliable enough to guide implementation. Issue alignment is evaluated from the PR description because no separate approved spec context was provided.
Concerns
- The behavior contract mentions path-based reads of streams without known length but does not define how FIFOs, devices, sockets, or other non-regular files are handled, leaving a denial-of-service and implementation ambiguity.
- The lint allowlist says production code has no escape hatch, but Rust
allowattributes can suppress the clippy lint anywhere unless the spec requires an audit that bans unauthorized suppressions. - The files-touched section contradicts the lint-data section by calling
.clippy.tomla new file even though the spec requires preserving and extending an existing file.
Security
- Non-regular file handling should be specified so safe reads cannot hang or read from special devices unexpectedly.
- Telemetry/error handling should explicitly avoid sending raw local file paths unless redacted.
Verdict
Found: 0 critical, 3 important, 2 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
| 2. **Open / metadata I/O errors** (permission denied, not found, | ||
| etc.). Returns `SafeReadError::Io { source }`. | ||
| 3. **Size check** against `opts.max_bytes`. Evaluated from | ||
| `Metadata::len()` for regular files before any read; for streams |
There was a problem hiding this comment.
| - `**/fixtures/**` — test fixture helpers. | ||
| - Any module gated entirely by `#[cfg(test)]`. | ||
|
|
||
| Production code outside these patterns has no escape hatch other than |
There was a problem hiding this comment.
clippy::disallowed_methods alone because production code can add #[allow(clippy::disallowed_methods)]; add an audit that rejects unauthorized allows outside the documented paths or relax this contract.
| - `crates/warp_files/src/error.rs` (new — `SafeReadError` enum). | ||
| - `crates/warp_files/src/opts.rs` (new — `SafeReadOpts` struct, | ||
| `BinaryCheck` enum, and the `Default` impl). | ||
| - `.clippy.toml` (new file — disallowed-methods data only; no |
There was a problem hiding this comment.
.clippy.toml already exists and must be extended without replacing existing entries; update this to say the existing file is modified.
| /// Ignored by `read_bytes` / `async_read_bytes`. | ||
| pub binary_check: BinaryCheck, | ||
|
|
||
| /// Tracing label for telemetry / errors. Defaults to `"unspecified"`. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Specify that telemetry/logging for safe-read errors must not emit raw local paths from SafeReadError without redaction, since paths can expose usernames, project names, or sensitive filenames.
| ### Warn → deny promotion plan | ||
|
|
||
| - **V1**: every workspace crate root carries | ||
| `#![warn(clippy::disallowed_methods)]`. The four `safe_read` sync |
There was a problem hiding this comment.
💡 [SUGGESTION] V1 defines two sync helpers, not four; rewrite this to avoid confusing the sync V1 scope with the V1.5 async helpers.
- Add B8 non-regular file rule rejecting FIFOs/devices/sockets - Add allow-attribute audit test to prevent silent lint suppression - Fix files-touched contradiction: .clippy.toml is modified, not new - Add path-redaction contract for telemetry and Display/Debug - Renumber error precedence to include non-regular file rejection
|
Addressed the latest oz-for-oss review:
|
|
/oz-review |
1 participant
Spec for #10049. Adds a single
read_text_file_safe/read_binary_file_safehelper inwarp_filescrate with size and binary checks, plus a clippy lint denying rawstd::fs::read*calls outside the helpers. Staged migration: V1 ships helpers + 5 high-traffic migrations + lint-as-warn; V2 promotes to deny.Closes (spec-only) #10049.