Propagate has_shell_shebang's Read Failures Instead of Swallowing Them - #956
Conversation
Fixes one real finding from coderabbitai on PR #952 (declines the other; see below). ## Propagate shell-file read failures `has_shell_shebang` caught every `OSError` from `path.open()`/ `readline()` and returned `False`, the same value it returns for a file that legitimately isn't a shell script. Unlike the CI bash side (where `read` fails at true EOF even after filling the variable), Python's `readline()` never raises for EOF, an empty read is just `b''` with no exception, so every `OSError` this caught was a genuine failure (permission denied, the file vanishing between `git ls-files` and the read, disk I/O). Swallowing it meant a tracked file this couldn't open silently dropped out of the lint target list, and `lint()` could report success having never actually checked it. - `scripts/docker_lint.py`: `has_shell_shebang` now raises `CommandFailed` on a genuine read `OSError`, matching the pattern `ls_files` already uses for its own I/O failures. The deliberate `False` cases (a symlink, invalid UTF-8) are unchanged. - `scripts/tests/test_docker_lint.py`: added `test_has_shell_shebang_raises_rather_than_swallowing_a_read_failure`, confirming a mocked `PermissionError` surfaces as `CommandFailed` instead of a silent `False`. ## Declined: reject symlinks in every shell-discovery path The `*.sh`-glob-matched branch (`ls_files(root, linter.patterns)`) never reads file content on the host at all, before or after this chain's own symlink fix (#955): it only builds a path list and passes it to `docker run ... -- files`. Confirmed empirically that a symlink processed *inside* the container cannot escape to the host filesystem regardless of target: `docker run -v "$PWD":/mnt alpine sh -c 'cat /mnt/link-to-etc-shadow'` reads the container's own `/etc/shadow` (byte-identical to reading it directly), and a symlink to a real host tmp file that exists on the host but not in the container's own filesystem tree fails with "No such file or directory" (i.e., the container's own root, not the host's, is what a bind-mounted symlink resolves against). The host-side read this chain actually guards against is specific to `extensionless_shell_scripts`' shebang peek, which already rejects symlinks (#955); the glob-matched branch has no equivalent host-side read to guard. ## Verified Full test suite (798 tests), ruff, mypy, `repo_gate.py`, `prose_lint.py --diff origin/develop`, and the complete `docker_lint.py` run (all 7 linters) all pass clean.
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesShell shebang handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk:🟡 Moderate · up to The change makes shell-file read failures surface instead of being silently ignored, but the path-safety check only covers the final component; a symlinked parent directory or race before opening could make lint read outside the checkout. Merge should wait for that boundary to be fixed or explicitly accepted by the appropriate owner. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo🐞 Bugs (0)📘 Rule violations (0)📎 Requirement gaps (0)Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn these tips off under Display preferences |
PR Summary by QodoPropagate shell-shebang read failures in docker_lint target discovery
AI Description
Diagram
High-Level Assessment
Files changed (2) |
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped, aligns with existing error handling in target discovery, and is covered by a focused regression test.
Pull request overview
This PR makes shell-script discovery failures in scripts/docker_lint.py explicit by propagating host-side read errors from has_shell_shebang() as CommandFailed, preventing unreadable tracked files from being silently skipped during lint target selection.
Changes:
- Raise
CommandFailedwhenhas_shell_shebang()encounters anOSErrorwhile opening/reading a tracked file. - Add a regression test asserting that a read failure (e.g.,
PermissionError) is not swallowed asFalse.
File summaries
| File | Description |
|---|---|
| scripts/docker_lint.py | Propagates shebang-peek read failures as CommandFailed during extensionless shell-script discovery. |
| scripts/tests/test_docker_lint.py | Adds coverage ensuring read failures in has_shell_shebang() raise rather than returning False. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Fixes a Copilot nitpick on PR #956: the function returns False, it doesn't "read" anything as False. Wording only, no behavior change.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, aligns error handling with existing discovery behavior, and includes a focused regression test for the new failure mode.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Propagate has_shell_shebang's Read Failures Instead of Swallowing Them
Fixes one real finding from coderabbitai on PR #952 (declines the
other; see below).
Propagate shell-file read failures
has_shell_shebangcaught everyOSErrorfrompath.open()/readline()and returnedFalse, the same value it returns for afile that legitimately isn't a shell script. Unlike the CI bash side
(where
readfails at true EOF even after filling the variable),Python's
readline()never raises for EOF, an empty read is justb''with no exception, so everyOSErrorthis caught was a genuinefailure (permission denied, the file vanishing between
git ls-filesand the read, disk I/O). Swallowing it meant a tracked file this
couldn't open silently dropped out of the lint target list, and
lint()could report success having never actually checked it.scripts/docker_lint.py:has_shell_shebangnow raisesCommandFailedon a genuine readOSError, matching the patternls_filesalready uses for its own I/O failures. The deliberateFalsecases (a symlink, invalid UTF-8) are unchanged.scripts/tests/test_docker_lint.py: addedtest_has_shell_shebang_raises_rather_than_swallowing_a_read_failure,confirming a mocked
PermissionErrorsurfaces asCommandFailedinstead of a silent
False.Declined: reject symlinks in every shell-discovery path
The
*.sh-glob-matched branch (ls_files(root, linter.patterns))never reads file content on the host at all, before or after this
chain's own symlink fix (#955): it only builds a path list and passes
it to
docker run ... -- files. Confirmed empirically that a symlinkprocessed inside the container cannot escape to the host filesystem
regardless of target:
docker run -v "$PWD":/mnt alpine sh -c 'cat /mnt/link-to-etc-shadow'reads the container's own/etc/shadow(byte-identical to reading it directly), and a symlink to a real host
tmp file that exists on the host but not in the container's own
filesystem tree fails with "No such file or directory" (i.e., the
container's own root, not the host's, is what a bind-mounted symlink
resolves against). The host-side read this chain actually guards
against is specific to
extensionless_shell_scripts' shebang peek,which already rejects symlinks (#955); the glob-matched branch has no
equivalent host-side read to guard.
Verified
Full test suite (798 tests), ruff, mypy,
repo_gate.py,prose_lint.py --diff origin/develop, and the completedocker_lint.pyrun (all 7 linters) all pass clean.Summary by CodeRabbit
Bug Fixes
Tests