spec: unified file-read interface (#10049)#10222
spec: unified file-read interface (#10049)#10222lonexreb wants to merge 1 commit 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.
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.