Make the git-environment rule a check rather than a helper nobody must use - #32
Conversation
…t use `probe` shells out to `git` for its throwaway worktree and to a hook runner for the hook it drives, and both must run with git's own environment taken away: a hook runner exports `GIT_DIR` and `GIT_INDEX_FILE`, several of them relative to the repository the hook fired in, so a child that inherits them is answered about a repository the run was never about -- with write access to it. The first version of `probe` did that and could not create its worktree at all. `detached()` strips them, and until now nothing made skipping it turn red. A shared helper nobody is required to use is a note. The check reads the module's syntax tree with the tree-sitter grammar this crate already carries for `comment_regexp`, so the tier costs no new dependency. It is a question about a call expression rather than about a line: a regex over `Command::new` cannot tell the call inside `detached` -- the one that is allowed, because it IS the helper -- from the ones that must not exist, and a regex over `detached(` cannot tell a call from a comment mentioning one. The second test is that negative control, and reintroducing the defect in `probe.rs` was driven before this was committed: the check names the line and the enclosing function. It also asserts the helper still strips, because a check that only counted call sites would pass over a `detached` that had quietly stopped removing anything. This is the structural prototype #13 asks for, and what it does NOT do is the finding. One consuming repository carries a 492-line Python checker for the same rule, and the length is not waste: it traces the `env=` argument through wrappers, assignments and parameters, and stops where certainty does. This asks the narrower question -- does any command get built outside the helper -- which is answerable from the syntax tree alone. The moment the rule needs "and the environment it passes is traceable to the helper", it needs more than a syntax tree, and that is the boundary the research issue is about.
|
Warning Review limit reached
Next review available in: 80 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 @@
## main #32 +/- ##
=======================================
Coverage 89.15% 89.15%
=======================================
Files 29 29
Lines 9217 9217
=======================================
Hits 8217 8217
Misses 1000 1000 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
The structural prototype #13 asks for, doing a job this repository needs.
The rule
probeshells out togitfor its throwaway worktree and to a hook runner for the hook it drives. Both must run with git's own environment taken away: a hook runner exportsGIT_DIRandGIT_INDEX_FILE, several of them relative to the repository the hook fired in, so a child that inherits them is answered about a repository the run was never about -- with write access to it. The first version ofprobedid that and could not create its worktree at all (#26).detached()strips them, and until now nothing made skipping it turn red. A shared helper nobody is required to use is a note.Why a parser rather than a regex
The rule is a question about a call expression, not about a line:
Command::newcannot tell the call insidedetached-- the one that is allowed, because it is the helper -- from the calls that must not exist;detached(cannot tell a call from a comment mentioning one.The check reads the module's syntax tree with the tree-sitter grammar this crate already carries for
comment_regexp, so the tier costs no new dependency. The second test is the negative control -- one fixture with the defect, one with only the helper and a comment that mentionsCommand::new-- and reintroducing the defect inprobe.rswas driven before committing: the check names the line and the enclosing function.It also asserts the helper still strips, because a check that only counted call sites would pass over a
detachedthat had quietly stopped removing anything.What it does not do, which is the finding
One consuming repository carries a 492-line Python checker for the same rule, and the length is not waste: it traces the
env=argument through wrappers, assignments and parameters, and stops where certainty does.This asks the narrower question -- does any command get built outside the helper -- which is answerable from the syntax tree alone. The moment the rule needs and the environment it passes is traceable to the helper, it needs more than a syntax tree. That boundary is what #13 is about, and this puts a measured example on both sides of it: 130 lines for the syntactic question, 492 for the traced one.
424 tests pass, 2 of them new.