Repository navigation
Bound the Guard's Stdin Descriptor Before int() - #2033
Conversation
A redirect's descriptor prefix reached int() after only an isdecimal() check, so a zero-padded descriptor over Python's 4300-digit conversion limit raised ValueError and the hook exited non-blocking. The descriptor is now tested digit by digit for a nonzero value, which reads every decimal digit exactly as int() did without converting the string. Self-test rows pin a 4400-zero descriptor on a while-read loop as descriptor 0: denied reading /dev/zero, allowed reading a file.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe stdin-redirection check now detects zero-valued descriptors by inspecting their decimal digits instead of converting them to integers. Self-tests cover long zero-padded descriptors bound to ChangesDescriptor Zero Detection
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Merge Risk: ⚪ Minimal · up to No supported change-specific risk remains; the guard handles the oversized zero-padded descriptor cases described for this change. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2033 +/- ##
========================================
Coverage 55.66% 55.66%
========================================
Files 16 16
Lines 7317 7317
========================================
Hits 4073 4073
Misses 3244 3244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The one-line logic change is provably equivalent to the prior check (verified across all Unicode codepoints), removes a real crash-to-unchecked-command leak, and is covered by targeted self-test rows with no defects found.
Review effort: Balanced
Findings: None
What changed in this PR
This PR hardens _redirects_stdin in the Claude write guard (host-setup/agent-safety/claude/gh-write-guard.py). Previously the function converted a redirect's descriptor prefix with int(fd) after only an isdecimal() check. A zero-padded descriptor exceeding Python's 4300-digit int() conversion limit raised ValueError, which made the hook exit non-blocking and let the command run entirely unchecked. Since bash resolves a zero-padded descriptor as descriptor 0, the crash was reachable through a command bash actually executes. This guard is part of the fleet's agent-safety layer, so an unchecked-command leak is a security-relevant defect.
Changes:
- Replace the
int(fd) != 0descriptor-0 test withany(unicodedata.decimal(c) for c in fd), which never converts the digit string and returns the same verdictint()gave for everyisdecimal()-accepted string. - Add
import unicodedata(correctly ordered in the stdlib import block). - Add two self-test rows pinning a 4400-zero descriptor as descriptor 0:
denywhen it binds/dev/zero(a stream),allowwhen it binds a file.
I verified the key assumptions independently:
str.isdecimal()andunicodedata.decimal()have identical domains across all 0x110000 codepoints (0 mismatches), sounicodedata.decimal(c)cannot raise onfd, which thetok.isdecimal()guard (line 1887) plus the{-prefix handling (line 1902) guarantee is empty or all-decimal.any(unicodedata.decimal(c) for c in fd)matchesint(fd) != 0for all tested inputs, including all-zero and Arabic-Indic digits, and preserves the empty-fdcase as descriptor 0 (any([])isFalse, matching the oldif fd and ...).- The allow row's
yes |prefix is meaningful: it proves the padded descriptor is recognized as descriptor 0 (overriding the pipe), which a naive text compare would fail.
| File | Description |
|---|---|
| host-setup/agent-safety/claude/gh-write-guard.py | Replaces the unbounded int(fd) descriptor-0 test with an unicodedata.decimal-based check that never converts the digit string, adds the required import, and adds two self-test rows pinning the 4400-zero descriptor behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The change alters fail-open behavior in a security-critical agent-safety write guard, which warrants final human sign-off despite the fix being small and verified equivalent.
Review effort: Balanced
Findings: 1
Open (1)
|
@coderabbitai review |
✅ Action performedReview finished.
|
…d Effort (#2104) ## Summary Copilot code review's default effort moved from Lite to Balanced on 2026-09-28. Balanced rounds write the `ccr-overview-v2` body with a per-file table and almost never a coverage statement. So `pr_review.py` read every such pull request as `COVERAGE_IS_UNSTATED` (exit 45) and parked it on a maintainer decision (#2103, #2095, #2089, #2082, #2077). Measured over the Copilot rounds on the 40 newest pull requests here, 25 of 29 Lite rounds carried the `fleet-review` marker, against 1 of 22 Balanced rounds. Stating the marker literally in the runbook did not help. A Balanced round on this branch read that instruction, quoted it in its own summary, and still left the marker out. So the tooling now accepts a coverage reading that works at any effort level, instead of depending on one setting. ### The rule A head that no coverage statement reaches, meaning none is stated on it and none is carried to it, now reads `coverage=table` and passes when all of these hold: - A round covering the head has its own file table naming exactly the changed files. - No Copilot round on the pull request, on any commit, states or appears to state partial coverage. The scan reads each round body in 12 ways: raw, entity-decoded, and compatibility-folded, each with tags kept and stripped three ways. A partial found in any reading counts. - The whole review history is in view. - The changed-file list is present, well-formed, non-empty, and within the 100-file window. A statement that reaches the head always wins, so a stated partial still blocks. A pull request that ever had a partial round goes to the maintainer. When no table stands in, the digest prints the reason as `NO FILE TABLE STANDS IN: <reason>`. The rule is stated in `GOVERNANCE.md` "PR Review Etiquette", in the `pr-review-conduct` Merge Gate (source and generated copies), in the runbook, in `scripts/README.md`, and in the script's own docstring and exit-45 message. Measured live with this branch: #2102, #2094, #2088, #2081, #2076, #2050, #2046, #2039 and #2033 read `coverage=table`. #2098, which carries the marker, still reads `full`. ## Known limits, all failing closed unless noted - **Glob rows.** Copilot sometimes collapses generated skill copies into glob rows such as `.../skills/*/references/*.md`, so on those pull requests the table does not match and they go to the maintainer. #2053 and #2021 are examples. - **Marker quoted near digits.** A round that quotes the marker next to digits reads as a partial. That includes this pull request's own first round, so this PR goes to the maintainer. - **Early-closed fences.** A fence that Markdown closes early, through a list item or an HTML block, masks a real table after it. - **Prose partials (fail-open).** A partial worded in prose that none of the patterns anticipate can still pass. The scan narrows this gap but cannot close it. - **Malformed marker before a full one (pre-existing, not introduced here).** On the head, a malformed marker followed by a well-formed full marker is read as full by the existing coverage reader. - **Unpinned readings.** Tests do not pin each of the 12 readings individually, the echoed snippet can carry raw control characters into the digest line, and two docstrings overstate slightly. None of these affects a verdict. ## Verification - `python3 -m unittest discover -s tests`: 1841 tests pass. The new tests were mutation-checked, and each guard fails its test when removed. - ruff, mypy, the prose gate, markdownlint, `build_dist.py --check`, `spec/validate.py`, and the eol gate are clean. - The local strict review ran across several rounds, and the last pass found no fail-open. Its differential fuzz over 150k bodies found nothing the previous commit flagged that this one misses. Its receipt is recorded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…pilot Coverage at Balanced Effort (#2105) ## Summary Promotes develop to main, carrying 14 pull requests: - [#2076](#2076) Read the Guard's Token Fallback With a Bash-Aware Context Scan - [#2081](#2081) Keep the Guard's Quote Mask When an Escape Sits Outside the Loop Condition - [#2094](#2094) Reconfigure pr_review.py's Console Streams to UTF-8 - [#2088](#2088) Guard check_settings' Live Read of the Repository Response - [#2102](#2102) Skip a Blockquoted List Marker in the Prose Gate's Dash Rule - [#2104](#2104) Accept a Head Round's Exact File Table as Copilot Coverage at Balanced Effort - [#2098](#2098) Quote the Review Engine's Remaining Bare Shell Placeholders - [#2050](#2050) Hand Unstated Coverage to the Maintainer Instead of Prescribing a Re-Request - [#2046](#2046) Report a Copilot Review Request That Records Nothing Instead of Waiting It Out - [#2053](#2053) State the Python Build Profile by Its Trait, Not One of Its Forms - [#2068](#2068) Strip the CR From configure.sh's Environment-Name Encoding - [#2033](#2033) Bound the Guard's Stdin Descriptor Before int() - [#2036](#2036) Guard ruleset_id() Against Malformed Array Elements - [#2039](#2039) Strip the CR From configure.sh's Workflow-Model Read The lead change is #2104. Copilot code review's default effort moved from Lite to Balanced on 2026-09-28, and Balanced rounds write a per-file table but almost never a coverage statement. `pr_review.py` now accepts a head round's file table as coverage when it names exactly the changed files and no Copilot round on the pull request has ever stated or appeared to state partial coverage. Before this, every pull request reviewed at Balanced effort parked on a maintainer decision. The other pull requests are guard, configure, prose-gate, and review-engine fixes that were waiting on that decision or landed earlier. #2076 leaves #1910, #1958, and #1959 open, per #1960. Closes #1998 Closes #2091 Closes #1260 Closes #1190 Closes #1652 Closes #2047 Closes #1904 Closes #1947 Closes #1882 Closes #2028 Closes #1987 Closes #1881 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
_redirects_stdinin the write guard converted a redirect's descriptor prefix withint()after only anisdecimal()check. A zero-padded descriptor over Python's 4300-digit conversion limit raisedValueError, the hook exited non-blocking, and the command ran with no check. Bash reads such a descriptor as descriptor 0, so the crash was reachable with a command bash runs.unicodedata.decimal, so no digit string is converted. It gives the same verdictint()gave for everyisdecimal()string.while readloop as descriptor 0: denied when it reads/dev/zero, allowed when it reads a file.Closes on promotion: #2028
Verification
gh-write-guard.py --selftestpasses. With the fix reverted it fails on theValueError, and a plain text compare fails the new allow row.isdecimal()accepts, in single, padded, and repeated forms, found no verdict that differs fromint().spec/validate.py, andbuild_dist.py --checkpass.local-strict-reviewpasses ran. The first found two introduced findings, both fixed: an ASCII-only descriptor match that allowed a runnable command the old code denied, and a deny row that passed whichever way the descriptor was read, now paired with the allow row. It also found one pre-existing defect, filed as The Guard's Stdin Redirect Scan Runs Past a Reserved Word #2032. The second found nothing.🤖 Generated with Claude Code
Summary by CodeRabbit
/dev/zeroor a file.