fix(search-tool): narrow guard/counter false-positive triggers (sc-1359) - #345
Merged
norvalbv merged 2 commits intoAug 5, 2026
Merged
Conversation
## Summary - Fixes sc-1359: `search-tool-guard`/`search-tool-counter` were firing false-positive advisories on exact-identifier greps, `node_modules`/`/tmp` lookups, and echo arguments bleeding through in compound commands. - `extractPattern`/target detection rewritten as a quote-aware, per-invocation segment scanner: the bin name must appear as a whole shell word, a candidate match is rejected if it falls inside a quoted region, short-/long-form space-separated value-flags and `-e`/`--regexp` pattern-flags are handled correctly (bin-aware, since `fd`'s `-e`/`--extension` means something different from grep's), and a bare `.`/`..` target is treated the same as no operand. - The guard uses a single `firstAdvisablePattern` call instead of a separate exclusion-check-then-pattern-extraction pair, keeping which invocation of a compound command they refer to correlated. - `classify()` gains a narrow "code snippet shape" escape hatch: declaration-modifier keywords immediately followed by an identifier AS THE LAST WORD, and nothing else. - Guard/counter recognize out-of-index targets (`node_modules`, `.git`, the OS temp dir; the counter also scopes to `scanRoots`) and skip advising/counting accordingly, with an explicit no-op streak transition. Target detection covers `grep`/`rg`/`ripgrep`/`ack`/`ag`/`fd` (pattern-first argv) and `find` (paths-first argv) separately. - An edge-case pass (manual + this repo's own guard-review gate, across eleven prior ship attempts) found and fixed 17 further bugs in this same new code via TDD. - `search-tool-lib.mts` grew past the 500-line size ratchet; split the generic Bash-command-string parsing primitives into a new `search-tool-shell.mts` (with matching test coverage). No new domain registration needed. ## Test plan - [x] `bun vitest run gate-engine/search-tool/` — 109 unit + e2e tests pass (across 3 files, all well under the 500-line cap) - [x] `node gate-engine/search-tool/eval/eval.mts --fail` — 24/24 (100%), 0 false positives/negatives - [x] Full repo suite — 3362+ tests pass, 0 failures - [x] `bun run typecheck` / `bun run lint` / `bun run lint:structure` — clean - [x] `devkit ship`'s own guard-review gate (correctness-reviewer) — 17 real findings surfaced across eleven prior attempts, all fixed with regression tests before this run
Contributor
📝 WalkthroughWalkthroughThe change adds quote-aware shell parsing, invocation-aware pattern extraction, target-scope filtering, and improved query classification. Guard and counter behavior now ignores excluded or out-of-scope searches. Tests and evaluation cases cover these paths. ChangesSearch classification and scope filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SearchHook
participant SearchToolCounter
participant SearchToolGuard
participant SearchToolLib
SearchHook->>SearchToolCounter: submit search command
SearchToolCounter->>SearchToolLib: check excluded targets and scan roots
SearchToolCounter->>SearchToolGuard: evaluate eligible search
SearchToolGuard->>SearchToolLib: select first advisable pattern
SearchToolCounter-->>SearchHook: update or preserve streak
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gate-engine/search-tool/__tests__/search-tool-lib.test.mts (1)
445-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the returned fallback pattern in the malformed-quoting tests.
These tests say
extractPatternreturns a result, but only assertnot.toThrow(), so a future change returningnullwould pass. Add an explicit expected pattern assertion for both the unterminated double-quote and single-quote cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gate-engine/search-tool/__tests__/search-tool-lib.test.mts` around lines 445 - 452, Update both malformed-quoting tests in the “extractPattern / splitUnquotedSegments” suite to assert the exact fallback pattern returned by extractPattern, while retaining the no-throw expectation. Use the same expected pattern for the unterminated double-quote and single-quote cases if their graceful behavior is identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gate-engine/search-tool/search-tool-guard.mts`:
- Around line 77-85: The firstAdvisablePattern call must exclude invocations
whose targets are outside the configured scanRoots, not only targets under
EXCLUDE_ROOTS. Update firstAdvisablePattern and its caller to receive and apply
scan-root scope per invocation, preserving correlated target-pattern selection.
Add coverage for an out-of-scope-only command and a compound command that skips
the out-of-scope invocation and selects the in-scope one.
---
Nitpick comments:
In `@gate-engine/search-tool/__tests__/search-tool-lib.test.mts`:
- Around line 445-452: Update both malformed-quoting tests in the
“extractPattern / splitUnquotedSegments” suite to assert the exact fallback
pattern returned by extractPattern, while retaining the no-throw expectation.
Use the same expected pattern for the unterminated double-quote and single-quote
cases if their graceful behavior is identical.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62209f11-e275-4584-a136-e99617382324
📒 Files selected for processing (9)
gate-engine/search-tool/__tests__/search-tool-hooks.test.mtsgate-engine/search-tool/__tests__/search-tool-lib.test.mtsgate-engine/search-tool/__tests__/search-tool-shell.test.mtsgate-engine/search-tool/eval/queries.jsongate-engine/search-tool/search-tool-counter.mtsgate-engine/search-tool/search-tool-guard.mtsgate-engine/search-tool/search-tool-lib.mtsgate-engine/search-tool/search-tool-shell.mtspackage.json
… just EXCLUDE_ROOTS Addresses PR review feedback on #345: - `firstAdvisablePattern` now also skips an invocation whose target is outside the consumer's configured `scanRoots` (previously it only checked the universal `EXCLUDE_ROOTS`), keeping the per-invocation correlation between exclusion-check and pattern-selection that this function exists for. `search-tool-guard.mts` passes `scanRoots` through from the same `resolveGuardConfig()` call already used for tool names. - `eval/eval.mts` no longer hardcodes `src/` as its synthetic test target — it now derives the target from the resolved `scanRoots`, so the eval harness stays valid on any consumer (including devkit itself, whose own `scanRoots` is `["cli","gate-engine"]`, not `src`) now that the guard scopes to it. - Found and fixed two real bugs while wiring this up (both surfaced by this repo's own guard-review gate mid-fix, both verified and fixed via TDD before this push): 1. A pre-filter approach that stripped bare `.`/`..` cwd-ref targets before the exclusion checks ran broke MIXED target lists (e.g. `grep ... node_modules .`) — removing `.` left only `node_modules`, which then matched every remaining root and was wrongly classified as fully excluded. Fixed by moving the cwd-ref awareness INTO the match predicate itself (`allTargetsMissEveryRoot`) instead of pre-filtering, so a cwd-ref's presence correctly keeps a mixed-target invocation in scope. 2. A mechanical slip while editing `search-tool-hooks.test.mts` — an edit to `runGuard()` accidentally dropped its `return` statement. - Nitpick: tightened the two "unterminated quote" crash-safety tests to assert the exact fallback pattern returned, not just that the call doesn't throw. - `search-tool-lib.test.mts` grew past the size ratchet again from this round's additions; split the out-of-index target-detection tests (isExcludedTarget/isOutOfScanRoots/firstAdvisablePattern/Windows paths/find-fd) into a new `search-tool-targets.test.mts`, keeping pattern-classification tests (extractPattern/classify) in `search-tool-lib.test.mts`. ## Test plan - [x] `bun vitest run gate-engine/search-tool/` — 118 unit + e2e tests pass (across 4 files, all well under the 500-line cap) - [x] `node gate-engine/search-tool/eval/eval.mts --fail` — 24/24 (100%), 0 false positives/negatives - [x] Full repo suite — 3372 tests pass, 0 failures - [x] `bun run typecheck` / `bun run lint` / `bun run lint:structure` — clean
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gate-engine/search-tool/__tests__/search-tool-targets.test.mts (1)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the attached value-flag form.
Line 104 covers the space-separated form
-A 3. The attached form-A3is equally common and takes a different parsing path. If the parser supports it, add one assertion to pin the behavior.♻️ Suggested extra assertion
expect(isExcludedTarget('grep -A 3 cli node_modules/foo.js', ['node_modules'])).toBe(true); + // Attached form: the value rides on the flag token, so no separate value token is consumed. + expect(isExcludedTarget('grep -A3 cli node_modules/foo.js', ['node_modules'])).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gate-engine/search-tool/__tests__/search-tool-targets.test.mts` around lines 99 - 105, Extend the test case around isExcludedTarget to also assert that the attached value form -A3 does not misclassify the following pattern as a target, while still recognizing the trailing node_modules/foo.js path as excluded. Preserve the existing space-separated assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gate-engine/search-tool/__tests__/search-tool-targets.test.mts`:
- Around line 99-105: Extend the test case around isExcludedTarget to also
assert that the attached value form -A3 does not misclassify the following
pattern as a target, while still recognizing the trailing node_modules/foo.js
path as excluded. Preserve the existing space-separated assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d0cfac1-1167-418c-ab37-729bccf47910
📒 Files selected for processing (6)
gate-engine/search-tool/__tests__/search-tool-hooks.test.mtsgate-engine/search-tool/__tests__/search-tool-lib.test.mtsgate-engine/search-tool/__tests__/search-tool-targets.test.mtsgate-engine/search-tool/eval/eval.mtsgate-engine/search-tool/search-tool-guard.mtsgate-engine/search-tool/search-tool-lib.mts
🚧 Files skipped from review as they are similar to previous changes (3)
- gate-engine/search-tool/search-tool-guard.mts
- gate-engine/search-tool/tests/search-tool-hooks.test.mts
- gate-engine/search-tool/search-tool-lib.mts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
search-tool-guard/search-tool-counterwere firing false-positive advisories on exact-identifier greps,node_modules//tmplookups, and echo arguments bleeding through in compound commands.extractPattern/target detection rewritten as a quote-aware, per-invocation segment scanner: the bin name must appear as a whole shell word, a candidate match is rejected if it falls inside a quoted region, short-/long-form space-separated value-flags and-e/--regexppattern-flags are handled correctly (bin-aware, sincefd's-e/--extensionmeans something different from grep's), and a bare./..target is treated the same as no operand.firstAdvisablePatterncall instead of a separate exclusion-check-then-pattern-extraction pair, keeping which invocation of a compound command they refer to correlated.classify()gains a narrow "code snippet shape" escape hatch: declaration-modifier keywords immediately followed by an identifier AS THE LAST WORD, and nothing else.node_modules,.git, the OS temp dir; the counter also scopes toscanRoots) and skip advising/counting accordingly, with an explicit no-op streak transition. Target detection coversgrep/rg/ripgrep/ack/ag/fd(pattern-first argv) andfind(paths-first argv) separately.search-tool-lib.mtsgrew past the 500-line size ratchet; split the generic Bash-command-string parsing primitives into a newsearch-tool-shell.mts(with matching test coverage). No new domain registration needed.Test plan
bun vitest run gate-engine/search-tool/— 109 unit + e2e tests pass (across 3 files, all well under the 500-line cap)node gate-engine/search-tool/eval/eval.mts --fail— 24/24 (100%), 0 false positives/negativesbun run typecheck/bun run lint/bun run lint:structure— cleandevkit ship's own guard-review gate (correctness-reviewer) — 17 real findings surfaced across eleven prior attempts, all fixed with regression tests before this runSummary by CodeRabbit
grep,fd, andfindsearches.node_modules,.git, temporary directories, and paths outside configured scan roots no longer affect search guidance.