Repository navigation
Read a Heredoc on a Line Holding (( Every Way a Shell Might in the Wait-Loop Rule - #2114
Conversation
A line holding `((` beside a `<<` was never read as a heredoc opener, so a real heredoc's body stayed in the command the wait-loop rule reads, and a comparison in that body vouched for a loop whose condition had no bound. Such a line now forks the text into one reading per way it can open, including none, and the wait-loop rule denies when any reading holds an unbounded wait. Past 64 readings, a command that sleeps is denied.
A strip reading with no closing line dropped the rest of a quoted payload holding a shift, and the loop before it was then denied. The reading limit now counts the reading in progress, falls to 16, and past it denies any command naming `sleep` once its quotes are removed, which a quote pairing across two bodies can no longer hide. A command naming no `sleep` is allowed before any reading is built.
The fork sentence now names the tag test, since a tag the rule rejects forks nothing, and the body-end docstring names the index it returns.
Resolve the unbounded-wait rule against the two-reading heredoc opener from develop. The per-`<<` forks on a `((` line now run on the comments-dropped context-lex reading, gated on the `#`-as-text reading finding an opener, and the line-reader fallback forks the same way. Both reading sets are judged together under the reading limit. This also carries the round decision #2066 settled. A `<<` whose body is empty forks nothing, and the limit deny needs a loop keyword as well as `sleep`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The unread-allow text now drops `$` beside quotes and backslashes, since the lexer reads `sl$'e'ep` as `sleep`. The past-limit deny matches a loop keyword in its case, so a capitalized word in a heredoc body no longer counts as one. The docstring and README now say a fork needs a non-empty body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The lexer deletes a backslash-newline, so the text the unread allow reads now deletes one too, and its docstring claim holds for a direct caller as well as for classify, which folds them first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 (2)
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 wait guard now evaluates multiple possible heredoc readings for ambiguous arithmetic-shift lines. It denies commands when a reading contains an unbounded wait, and applies a text-based denial condition when the reading limit is exceeded. ChangesWait guard
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change strengthens wait detection for ambiguous heredocs. No actionable merge-blocking issue is established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change favors denying unsafe waits and remains within the existing command-safety gate, without visibly adding credentials, service access, or execution authority. Risk is low rather than minimal because the baseline comparison and complete shell-syntax coverage remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 #2114 +/- ##
========================================
Coverage 56.47% 56.47%
========================================
Files 16 16
Lines 7455 7455
========================================
Hits 4210 4210
Misses 3245 3245
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
🔵 Needs a closer look
It reworks the core detection path of a security-critical write guard with intricate combinatorial heredoc-reading logic that warrants human verification even though no concrete defect was found.
Review effort: Balanced
Findings: None
What changed in this PR
This PR closes a false-allow in rule 7 of gh-write-guard.py (the agent-safety write guard that denies unbounded shell wait loops). Previously, any command line whose tokens held (( was skipped by the heredoc-body stripper, so a real heredoc's body on such a line was kept as commands, and a comparison inside that body could vouch for a wait loop whose own condition carried no bound (issue #2059). The fix replaces the single-reading strip with a fork-based reader (_heredoc_readings) that produces every plausible heredoc-strip reading and denies if any of them holds an unbounded wait.
Changes:
- Replaces
_heredoc_opener/_line_heredoc_opener/_strip_heredoc_bodieswith_heredoc_openers/_line_heredoc_openers/_heredoc_readings, which read a((line once as opening nothing and once per<<whose non-empty body a later line closes, capping total readings at_HEREDOC_READING_LIMIT(16). - Adds a fast-path in
_check_unbounded_wait: allow unread when the sanitized text names nosleep; past the reading limit, deny only when the text also names a loop keyword (_NAMES_A_LOOP), matching bash's case sensitivity. - Adds 14 self-test rows and updates module docstring item 7 plus README requirement 7 to describe the fork reading and limit-deny behavior consistently.
| File | Description |
|---|---|
host-setup/agent-safety/claude/gh-write-guard.py |
Fork-based heredoc reader, sleep/loop fast-path, reading limit, and new self-test rows implementing the #2059 fix |
host-setup/agent-safety/README.md |
Requirement 7 prose extended to describe the ((-beside-<< fork reading and the reading-limit deny |
Notes from the review: no dangling references remain to the removed helpers; the fork explorer is correctly bounded and its per-fork kept + term snapshots are aliasing-safe; the first/others invariant (a real opener only strips on non-(( lines, forks only on (( lines) holds; the sleep fast-path and _NAMES_A_LOOP casing are over-approximations in the safe (deny) direction and introduce no false-allow regression versus develop; docstring item 7 and README requirement 7 now agree with the code. The known residuals (multiple heredocs per (( line, a loop keyword inside a body/quote past the limit) are pre-existing and filed separately (#2112, #2111, #2113), acknowledged in the PR as safe-direction limitations.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…y a Shell Might in the Wait-Loop Rule (#2115) ## Summary Promotes develop to main, carrying [#2114](#2114). That PR makes the guard's unbounded-wait rule read a line holding `((` beside a `<<` every way a shell might, once as opening nothing and once per `<<` whose non-empty body a later line closes, so a real heredoc's body can no longer vouch for a loop whose own condition carries no bound. It merges that design with develop's two-reading opener, denies past a 16-reading limit only when the command names both `sleep` and a loop keyword per decision #2066, and allows a command naming no `sleep` without building readings. Known residual: past the limit a lowercase `while` or `until` in heredoc prose still counts, which is the safe direction. ## Review coverage Copilot and CodeRabbit each reviewed #2114 through its final head `d16f444d` with no findings. This promotion carries byte-identical content, checkable with two commands: - `git rev-parse d16f444^{tree} 6ee0bc7^{tree}` prints `aecb62d8...` for both. - `git diff origin/main origin/develop | git hash-object --stdin` and `git diff 4c9087d d16f444 | git hash-object --stdin` both print `20c0428e...`. Closes #2059 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
Rule 7 of
gh-write-guard.pystripped no heredoc body on a line holding((, so a comparison in a real heredoc's body could vouch for a wait loop whose own condition carried no bound. This reads such a line once as opening nothing and once per<<whose tag the rule accepts and whose non-empty body a later line closes, and denies where any reading holds an unbounded wait._context_lexreading, gated on the#-as-text reading finding an opener, and the_operator_tokensline reader forks the same way. Both reading sets are judged together.sleepand a loop keyword, the keyword matched in its case as bash reads one. A<<whose body is empty forks nothing, since that reading is the same text as the one stripping nothing.sleeponce its line continuations, quotes, backslashes and$signs are removed is allowed without building readings, which coverssl$'e'ep.host-setup/agent-safety/README.mdrequirement 7 state the same.Known residual
Past the limit, a lowercase
whileoruntilanywhere in the text counts, heredoc bodies and quoted arguments included, so five numbered heredoc writes whose prose says "retry until green" followed bysleep 2are denied. That is the safe direction and follows from the keyword gate #2066 chose.Filed from the local review passes
Pre-existing on develop, each constructed and reproduced there: #2111, #2112, #2113, plus new shapes commented onto #2062 and #2063.
Verification
--selftestpasses, with a revert of each fix failing its new row.unittest discover -s tests(1856 OK), ruff format and check, mypy, prose lint againstorigin/develop, the eol gate,spec/validate.pyandbuild_dist.py --checkare clean. Threelocal-strict-reviewpasses were recorded, the third reporting no regression across about 15,000 generated commands compared against develop.Closes on promotion: #2059
🤖 Generated with Claude Code
Summary by CodeRabbit
sleepand a loop keyword.