Read only shell as shell in the recursive-enumeration scan - #85
Conversation
The enumerator and recursive-structure layers split text into shell fields, so an embedded program's source was read as shell. A ruby body yielded the directory `Dir.glob "/abs/lmd`, and a `Dir[...]` index was mistaken for a glob wildcard, which resolved the base to cwd and displaced the directory the program actually reads. Blank each foreign-language region before those layers run, keeping shell and opaque regions visible because the enumerator needs the searcher inside `xargs grep` and `find -exec grep`. Drop nonPathOperandChars, whose character test rejected real directory names containing a parenthesis and removed the only layer that names that directory for a command whose argv0 is not a declared searcher. Return immediately when an errored target already blocks under a fail-closed rule, since no later target can change that verdict and probing the rest holds the singleflight entry for one background timeout per remaining target. Log a block that follows an unclassified target, which the returned clean verdict would otherwise hide from the evaluation record. Record in regionFoldTools that ruby and javascript parse but expose no read targets, so declaring them in search_tools buys no coverage. Assert the reached target rather than the validator invocation count, so the no-veto test stops depending on target ordering. Move runExpandedCommands and logExpandedCommandError to exec_expand.go to keep exec_gate.go under the file-length limit. Co-authored-by: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 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 |
Summary
The layer that resolves what directory a recursive glob reads was splitting an embedded program's source into shell fields, which invented directories that do not exist and, worse, silently redirected a real search to the working directory. This reads only shell as shell and removes the character filter that was standing in for that.
Follow-up to #84, from a review of it.
Why
recursiveGlobDirsscans a whole command withshellFields. A command carrying an interpreter body hands it program source, not shell. A ruby body became the directoryDir.glob "/abs/lmd, which no index can match. ADir["/abs/indexed/**/*.rb"]index opened with a bracket, which the scan treats as a glob wildcard, so the base resolved to.and then to the working directory. That fabricated target displaces the directory the program actually reads, so the validator checks the wrong path and the read of an indexed repository is allowed.#84 tried to suppress the fabrication by rejecting operands containing certain characters. A character test cannot separate program source from a path, because a directory name may legitimately contain those characters. Rejecting parentheses drops a real macOS directory like
indexed-repo (1), and for a command whose argv0 is not a declared searcher that layer is the only one naming the target, so the whole read goes unchecked.Change
Each foreign-language embedded region is blanked to spaces before the enumerator and recursive-structure layers run, so they see only shell. Shell and opaque regions stay visible on purpose: the enumerator depends on the nested shell that carries the searcher in
find DIR | xargs grepandfind DIR -exec grep, and an opaque body has no grammar, so a recursive shell reading of it is the only one available. Every region is still handled on its own terms afterwards, by its analyzer's reads or by that recursive scan.With the scan reading only shell, the character filter has nothing left to do and is gone, so directory names holding a parenthesis, space, comma, or equals sign resolve again.
Two changes to how expanded validator runs combine. A fail-closed rule now returns as soon as a target errors, because that verdict is already a block and no later target can change it, while probing the rest costs one validator run per remaining target under a context that cannot be cancelled and holds the singleflight entry the whole time. A block that follows an unclassified target is now logged, because the returned verdict is clean and would otherwise show the evaluation as fully classified.
regionFoldToolsnow records that ruby and javascript parse into a region that exposes no read targets, so declaringrubyornodeinsearch_toolsbuys no coverage of those bodies. Closing that needs read analyzers in gksyntax.Testing
Deployed to a local daemon and exercised against the live gate.
python3 -c "print(2+2)"runs.Unit coverage was added for each shape above, including the parenthesized directory, the ruby bracket index, and the two enumerator shapes that must keep seeing their nested shell.
Known gaps, not addressed here
A
ruby -e,node -e, orphp -rbody has no read-target coverage. Removing the fabricated operands makes that visible rather than creating it: what existed before was a fabricated path that no index matches, or the working directory, so the outcome depended on whether the program source happened to contain a bracket.