fix: refuse a directory read with GNU's errno and exit code - #895
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
f24e77d to
fa5d0ef
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa5d0ef77b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fa5d0ef to
c2efbca
Compare
The read slots forwarded the backend's own error, so a directory on a keyed backend read as ENOENT in 23 commands. Refuse it in the slot instead, so no builder has to remember. - with_dir_guard / withDirGuard on read_bytes/read_stream/read_range, applied where the factory already stamps the namespace fact, so the slot inherits the namespace-only mount parent #887 taught the stat chokepoint about - READ_FAIL_EXIT beside USAGE_EXIT: GNU's code for a failed read is the command's, not the errno's, and it is gated on filesystem errors so a bad sed script keeps exiting 1 - sed aborts on a directory and continues past a missing file, per GNU - a directory no longer routes to a filetype handler chosen by name
c2efbca to
2b10274
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b10274e90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both from the review on #895. - a multi-operand gzip failure keeps the error's code in either order, so `zcat nope dir` is 1 like `zcat dir nope`: gzip's WARN records a code only when nothing has failed yet, while an error records outright - the cross-mount stream classifies a rendered line by its terminal strerror instead of scanning it, so a directory named `No such file or directory` no longer reads as ENOENT and sed answers 4
Summary
base64,sort,xxdand 20 more commands answeredNo such file or directoryfor a directory operand, because the read slots forwarded the backend's own error and a keyed backend has no key at a directory. Refused in the slot now, so no builder has to remember.with_dir_guard/withDirGuardonread_bytes/read_stream/read_range, applied where the factory already stamps the namespace fact. Post-hoc, so a successful read costs what it did.READ_FAIL_EXITbesideUSAGE_EXIT. GNU's code for a failed read belongs to the command, not the errno (sortis 2 either way,catis 1), with four exceptions that split by errno:sed4/2,gzip/gunzip/zcat2/1,zgrep1/2. Gated on filesystem errors, sosed 's/o/O/0'keeps exiting 1.sedaborts on a directory and continues past a missing file, which is GNU's split between a read error and an open error. Every other command in the family continues, which already matched.dir.tallyran a registered renderer and reported ENOENT.Also fixes the missing-file exit code for the same commands, since GNU's code is per command rather than per errno.
GNU truth pinned on
debian:stable-slim: coreutils 9.7, sed 4.9, gzip 1.13, jq 1.7, binutils 2.44, util-linux 2.41.5, bsdmainutils 12.1.8, xxd from vim-common, unzip 6.00, ripgrep 14.1.1. Correcting the original report:xxddoes have upstream truth (it ships with vim), exit 2.Test plan
python/tests/commands/builtin/test_read_failure_exit.pyand its TS twin pin 36 command lines against the docker measurement, plus 9 multi-operand casesinteg/unix/readfail/dir.json, 60 cases onramanddiskpnpm typecheckall packages; layout parity at baseline; pre-commit green and idempotentram2905,disk2863,ram-nested23Out of scope, found while tracing
cat /disk/noperm.txtleaks the host path for any errno but ENOENT.core/disk/errors.py::disk_errorsexists but is applied on the write side only.echo x > /ram/diron a keyed backend exits 0, loses the bytes, andfindthen lists the path twice.CommandOpts.filetype_fnsis set and read by nothing.dir_aware_stream(7 python call sites, 18 TypeScript) is left for its own change: it removes an eager refusal that the lazy-stream commands rely on for GNU's report-and-continue ordering.CLAUDE.mdwas not edited. Nothing there describes the read chokepoint today; worth adding if you want it recorded.