From 9f52bfaa1ad6eaf514fdbb8035d7ec02436e3846 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Wed, 5 Aug 2026 11:07:04 +0100 Subject: [PATCH 1/2] fix(search-tool): narrow guard/counter false-positive triggers (sc-1359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- .../__tests__/search-tool-hooks.test.mts | 41 ++ .../__tests__/search-tool-lib.test.mts | 416 ++++++++++++++---- .../__tests__/search-tool-shell.test.mts | 108 +++++ gate-engine/search-tool/eval/queries.json | 30 ++ .../search-tool/search-tool-counter.mts | 28 +- gate-engine/search-tool/search-tool-guard.mts | 20 +- gate-engine/search-tool/search-tool-lib.mts | 384 +++++++++++++--- gate-engine/search-tool/search-tool-shell.mts | 209 +++++++++ package.json | 1 + 9 files changed, 1092 insertions(+), 145 deletions(-) create mode 100644 gate-engine/search-tool/__tests__/search-tool-shell.test.mts create mode 100644 gate-engine/search-tool/search-tool-shell.mts diff --git a/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts b/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts index cfcc5df3..3636722c 100644 --- a/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts +++ b/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts @@ -43,6 +43,12 @@ function runCounterRaw(command, toolName = 'Bash') { return execFileSync('node', [COUNTER], { input: JSON.stringify({ tool_name: toolName, tool_input: { command }, session_id: sessionId }), env: { ...process.env, TMPDIR: stateDir }, + // No guard.config.json here (fresh tmpdir) so scanRoots falls back to the + // DEFAULT (['src']), matching this file's `src/`-targeted fixtures — and + // isolates these hook-wiring tests from devkit's OWN dogfood scanRoots + // (["cli","gate-engine"]), which would otherwise read every `src/` + // target here as out-of-scope (sc-1359 #3). + cwd: stateDir, }).toString(); } const runCounter = (command, toolName = 'Bash') => @@ -85,6 +91,20 @@ describe('search-tool-guard (PreToolUse)', () => { expect(guardFires(`cd "${CWD}" && rtk grep -rn "permission prompt rendering" src/`)).toBe(true); }); + it('FIRES on a conceptual grep invoked via unspaced $(...) command substitution (guard-review finding, round 6)', () => { + expect(guardFires(`result=$(grep -r "how does auth work" src/)`)).toBe(true); + }); + + it("advises on the RIGHT invocation's pattern in a compound command, not an excluded one's (guard-review finding, round 8)", () => { + // The node_modules invocation's pattern must never be attributed just because a LATER, + // unrelated invocation elsewhere in the command has a non-excluded target. + const advice = runGuard( + `grep -rn "auth flow logic" node_modules && grep -rn "the retry backoff path" src`, + )?.hookSpecificOutput?.additionalContext; + expect(advice).toContain('the retry backoff path'); + expect(advice).not.toContain('auth flow logic'); + }); + it('steers toward the CONFIGURED search tool (not a hardcoded name)', () => { const advice = runGuard(`grep -rn "auth flow" .`)?.hookSpecificOutput?.additionalContext; expect(advice).toContain(SEARCH_TOOL); @@ -100,6 +120,16 @@ describe('search-tool-guard (PreToolUse)', () => { }); expect(out?.hookSpecificOutput?.permissionDecision).toBe('ask'); }); + + it('stays quiet on a node_modules target regardless of pattern shape (sc-1359 #3)', () => { + expect(guardFires(`grep -rn "where is permission handled" node_modules/foo/lib.js`)).toBe( + false, + ); + }); + + it('stays quiet on a /tmp target regardless of pattern shape (sc-1359 #3)', () => { + expect(guardFires(`grep -oE "FAIL +[^ ]+\\.test\\.tsx?" /tmp/vitest-out.log`)).toBe(false); + }); }); describe('search-tool-counter (PostToolUse) — streak state machine', () => { @@ -143,6 +173,17 @@ describe('search-tool-counter (PostToolUse) — streak state machine', () => { expect(runCounter(`bun x 2>&1 | grep error`)).toBe(false); }); + it('an out-of-index target is a NO-OP on the streak — neither increments nor resets (sc-1359 #3)', () => { + expect(runCounter(`grep -rn "x" src/`)).toBe(false); // streak 1 + expect(runCounter(`grep -rn "y" node_modules/foo`)).toBe(false); // no-op, streak stays 1 + expect(runCounter(`grep -rn "z" src/`)).toBe(false); // streak 2 (not reset by the no-op) + // 3rd IN-SCOPE grep still escalates — the node_modules call above didn't count toward it. + const msg = JSON.parse(runCounterRaw(`grep -rn "w" src/`)).hookSpecificOutput.additionalContext; + expect(msg).toContain('3 consecutive'); + // The excluded call must not appear in the recent-commands list either. + expect(msg).not.toContain('node_modules'); + }); + it('degrades gracefully on a corrupt state file (concurrency safety: no throw, treated as 0)', () => { mkdirSync(join(stateDir, 'devkit-search-state'), { recursive: true }); writeFileSync(stateFile(), '{ this is not valid json'); diff --git a/gate-engine/search-tool/__tests__/search-tool-lib.test.mts b/gate-engine/search-tool/__tests__/search-tool-lib.test.mts index 7cf5a2aa..018523a8 100644 --- a/gate-engine/search-tool/__tests__/search-tool-lib.test.mts +++ b/gate-engine/search-tool/__tests__/search-tool-lib.test.mts @@ -2,99 +2,131 @@ import { describe, expect, it } from 'vitest'; import { classify, extractPattern, - hasCommandSearch, - isPrimarySearchCommand, - normalize, - stripQuotes, + firstAdvisablePattern, + isExcludedTarget, + isOutOfScanRoots, } from '../search-tool-lib.mts'; -// Unit tests for the search-tool hook library (used by search-tool-guard + -// search-tool-counter). These are pure string classifiers — provider-agnostic -// (Cursor vs Claude run the same Bash strings) so there are no provider-specific -// cases. The one OS-relevant case (Windows-style quoted cwd) lives under normalize. +// Unit tests for search-tool-lib.mts's classification logic (used by +// search-tool-guard + search-tool-counter). Generic Bash-string parsing +// (normalize, stripQuotes, hasCommandSearch, isPrimarySearchCommand) has its +// own coverage in search-tool-shell.test.mts, split out alongside +// search-tool-shell.mts once this file grew past the size ratchet. These are +// pure string classifiers — provider-agnostic (Cursor vs Claude run the same +// Bash strings) so there are no provider-specific cases. // A generic working dir WITH SPACES — spaces in the cwd were the original // false-positive trigger (a path split into "3 words"). Kept provider/OS-neutral. const CWD = '/Users/dev/My Projects/cool app'; -describe('normalize — strip cwd + unwrap rtk', () => { - it('strips a leading quoted `cd &&` (cwd with spaces was the #1 false positive)', () => { - expect(normalize(`cd "${CWD}" && grep -n "x" f.ts`)).toBe('grep -n "x" f.ts'); +describe('extractPattern', () => { + it('returns the double-quoted pattern', () => { + expect(extractPattern('grep -rn "auth flow" src/')).toBe('auth flow'); }); - it('strips a backslash-escaped-space unquoted cwd', () => { - expect(normalize('cd /a/My\\ Projects/app && grep -rn "x" src/')).toBe('grep -rn "x" src/'); + it('returns the single-quoted pattern', () => { + expect(extractPattern("grep -rn 'auth flow' src/")).toBe('auth flow'); }); - it('strips a single-quoted cwd', () => { - expect(normalize(`cd '${CWD}' ; rg "x"`)).toBe('rg "x"'); + it('falls back to first non-flag token when unquoted', () => { + expect(extractPattern('grep -rn validateUser src/')).toBe('validateUser'); }); - it('strips a Windows-style quoted cwd (Claude Code uses bash on Windows)', () => { - expect(normalize('cd "C:\\proj dir" && grep -n "x" f')).toBe('grep -n "x" f'); + it('scopes onto the grep after a pipe (find ... | xargs grep "x")', () => { + expect(extractPattern('find src -name "*.ts" | xargs grep "auth flow"')).toBe('auth flow'); }); - it('unwraps the rtk token proxy so the underlying bin is classified', () => { - expect(normalize('rtk grep -n "x" f')).toBe('grep -n "x" f'); - expect(normalize('rtk rg "x"')).toBe('rg "x"'); + it('returns the full quoted pattern even when it contains a pipe (grep -E "a|b")', () => { + // Quotes legitimately contain | ; & — scope must not truncate at them. + expect(extractPattern('grep -E "auth|session" src/')).toBe('auth|session'); }); - it('strips cwd AND unwraps rtk together', () => { - expect(normalize(`cd "${CWD}" && rtk grep -rn "x" src/`)).toBe('grep -rn "x" src/'); + it('returns the first grep pattern across a pipe, not the second', () => { + expect(extractPattern('grep "first" src/ | grep "second"')).toBe('first'); }); - it('leaves a non-cd command untouched', () => { - expect(normalize('grep -rn "x" src/')).toBe('grep -rn "x" src/'); + it('returns null when there is no pattern', () => { + expect(extractPattern('grep')).toBeNull(); }); - it('does not strip a non-leading cd', () => { - // Only a leading `cd ... &&` is cwd noise; a mid-command cd is intentional. - expect(normalize('echo hi && cd /tmp && grep "x"')).toBe('echo hi && cd /tmp && grep "x"'); + it('does NOT bleed a downstream echo argument into an unquoted grep pattern (sc-1359 #2)', () => { + expect( + extractPattern('grep -rln getFlowDriveInfoForSubChat src/ && echo "=== recheck ==="'), + ).toBe('getFlowDriveInfoForSubChat'); }); -}); -describe('stripQuotes', () => { - it('blanks double-quoted content including escaped quotes', () => { - expect(stripQuotes('git commit -m "fix: a | grep thing \\"q\\""')).toBe('git commit -m ""'); + it('does NOT bleed a downstream command across `;` either', () => { + expect(extractPattern('grep -rn x src/ ; grep -rn "the retry backoff path" src/')).toBe('x'); }); - it('blanks single-quoted content', () => { - expect(stripQuotes("echo 'use grep here'")).toBe("echo ''"); + it('falls through to the next segment when an earlier grep-family bin has no operand', () => { + expect(extractPattern('rg --files | xargs grep "the auth flow here"')).toBe( + 'the auth flow here', + ); }); - it('leaves unquoted text intact', () => { - expect(stripQuotes('grep -rn foo src/')).toBe('grep -rn foo src/'); + it('a bin name appearing as a SUBSTRING of an earlier path token is not a phantom invocation (guard-review finding, round 5)', () => { + // "ag" inside "src/ag-tools" is not the `ag` binary — it's part of an unrelated directory + // name in an earlier `find` segment. A phantom match there must not swallow the REAL grep + // invocation in the next pipeline segment. + expect( + extractPattern('find src/ag-tools -type f | xargs grep -l "the authentication flow works"'), + ).toBe('the authentication flow works'); }); -}); -describe('extractPattern', () => { - it('returns the double-quoted pattern', () => { - expect(extractPattern('grep -rn "auth flow" src/')).toBe('auth flow'); + it('a bin invoked via unspaced $(...) command substitution is still recognized (guard-review finding, round 6)', () => { + // The whitespace-or-start requirement that fixed the "ag-tools" phantom match (round 5) must + // not also break `$(grep ...)` — hasCommandSearch (a separate regex) already recognizes this + // shape, so extractPattern silently going null here would desync the guard's gate from its + // own extraction. + expect(extractPattern('result=$(grep -r "how does auth work" src/)')).toBe( + 'how does auth work', + ); }); - it('returns the single-quoted pattern', () => { - expect(extractPattern("grep -rn 'auth flow' src/")).toBe('auth flow'); + it('does not read a flag VALUE as the pattern (sc-1359, pre-existing FN)', () => { + expect(extractPattern('grep --include="*.mts" -rn "auth flow here" src/')).toBe( + 'auth flow here', + ); }); - it('falls back to first non-flag token when unquoted', () => { - expect(extractPattern('grep -rn validateUser src/')).toBe('validateUser'); + it("does not read a SPACE-separated value flag's value as the pattern (guard-review finding)", () => { + // -A/-B/-C/-m take their value as a SEPARATE argv token (not `=`-joined) — that value token + // must never be misread as the pattern, and the real pattern must not then be misread as a + // target. + expect(extractPattern('grep -A 3 cli node_modules/foo.js')).toBe('cli'); + expect(extractPattern('grep -m 5 "the auth flow" src/')).toBe('the auth flow'); }); - it('scopes onto the grep after a pipe (find ... | xargs grep "x")', () => { - expect(extractPattern('find src -name "*.ts" | xargs grep "auth flow"')).toBe('auth flow'); + it("does not read a LONG-FORM space-separated value flag's value as the pattern (guard-review finding, round 5)", () => { + // --context/--after-context/--before-context/--max-count are the long-form spellings of + // -C/-A/-B/-m — same space-separated-value shape, must be skipped the same way. + expect(extractPattern('grep --context 3 "how does auth work" src/')).toBe('how does auth work'); + expect(extractPattern('grep --max-count 5 "the auth flow" src/')).toBe('the auth flow'); }); - it('returns the full quoted pattern even when it contains a pipe (grep -E "a|b")', () => { - // Quotes legitimately contain | ; & — scope must not truncate at them. - expect(extractPattern('grep -E "auth|session" src/')).toBe('auth|session'); + it("-e's value IS the pattern, not a discardable flag value (guard-review finding, round 2)", () => { + // Unlike -A/-B/-C/-m, `-e`'s argument is the search pattern itself (grep's own + // "specify pattern via flag" form) — it must be treated as the pattern, not skipped. + expect(extractPattern('grep -e "unified employee onboarding flow" src/payments.ts')).toBe( + 'unified employee onboarding flow', + ); }); - it('returns the first grep pattern across a pipe, not the second', () => { - expect(extractPattern('grep "first" src/ | grep "second"')).toBe('first'); + it('a -e conceptual query is still flagged end-to-end, not misread as a filesystem path', () => { + // Regression-critical: mistakenly discarding -e's value made extractPattern fall through to + // the trailing path token, which classify() then reads as a literal filesystem path — a + // conceptual query silently vanishing behind the WRONG-but-still-a-verdict "literal" branch. + const pattern = extractPattern('grep -e "unified employee onboarding flow" src/payments.ts'); + expect(classify(pattern).verdict).toBe('conceptual_high'); }); - it('returns null when there is no pattern', () => { - expect(extractPattern('grep')).toBeNull(); + it("fd's -e/--extension (file-extension filter) is NOT grep's -e pattern flag (guard-review finding, round 10)", () => { + // fd spells its extension filter the same as grep spells "specify pattern via flag" — the + // two bins disagree on what -e MEANS, so which treatment applies must depend on the bin. + expect(extractPattern('fd -e ts "the authentication flow logic" src/')).toBe( + 'the authentication flow logic', + ); }); }); @@ -132,6 +164,21 @@ describe('classify — literal cases (grep is correct)', () => { expect(classify('useFooHook bar').verdict).toBe('literal'); expect(classify('foo.bar baz').verdict).toBe('literal'); }); + + it('a verbatim code snippet (keyword + identifier, no connective) is literal (sc-1359 #1)', () => { + expect(classify('export async function getFlowDriveInfoForSubChat').verdict).toBe('literal'); + }); + + it('a natural 3-word query ending in ?/:/= stays conceptual (guard-review finding, round 4)', () => { + // An earlier fix added a metachar check to the 3-word branch as defense-in-depth for the + // echo-bleed-through defect (sc-1359 #5) — but that check fires on ordinary English + // punctuation too, silently flipping queries like this to literal. Reverted: extractPattern's + // segment scan already prevents '=== recheck ===' from ever reaching classify() as a "pattern" + // in the first place (see the extractPattern "does NOT bleed a downstream echo argument" test), + // so the defense-in-depth was never load-bearing and isn't worth this regression. + expect(classify('fix auth bug?').verdict).toBe('conceptual_medium'); + expect(classify('note: check auth').verdict).toBe('conceptual_medium'); + }); }); describe('classify — conceptual cases (steer to searchCode)', () => { @@ -159,44 +206,265 @@ describe('classify — conceptual cases (steer to searchCode)', () => { expect(classify('Error: not found').verdict).toBe('literal'); expect(classify("Missing key 'id'").verdict).toBe('literal'); }); + + it('the code-snippet escape hatch does NOT swallow real conceptual queries (sc-1359 regression guard)', () => { + // Mirrors eval gr-04 — non-lowercase words (Client/Server) alone must not defeat conceptual. + expect(classify('relationship between Client and Server').verdict).toBe('conceptual_high'); + // Leads with a non-anchored word ("explain"), so the question-word check misses it; must not + // fall through to a false "literal" just because it contains a camelCase token. + expect(classify('explain how sessionToken refresh works').verdict).toBe('conceptual_high'); + // A domain acronym (OAuth) in otherwise-plain English must not read as a code keyword. + expect(classify('handles OAuth callback errors').verdict).toBe('conceptual_high'); + }); + + it('a bug report naming a symbol (keyword + identifier NOT last) stays conceptual (guard-review finding)', () => { + // "function getUser" alone would be a snippet — but a trailing English predicate describing + // the symbol ("fails silently", "needs refactoring", "looks wrong") makes these bug reports, + // not verbatim source lines. The identifier must be the LAST word for the snippet escape to fire. + expect(classify('function getUser fails silently').verdict).toBe('conceptual_high'); + expect(classify('class Foo needs refactoring').verdict).toBe('conceptual_high'); + expect(classify('type UserResponse looks wrong').verdict).toBe('conceptual_high'); + }); + + it('a REQUEST about a symbol (leading verb is not a code keyword) stays conceptual (guard-review finding)', () => { + // The keyword+identifier pair alone isn't enough — every word before the identifier must + // itself be a declaration modifier. "explain"/"debug"/"review" are action verbs, not that, + // so these are requests ABOUT a symbol, not a copy-pasted declaration line. + expect(classify('explain function getUserData').verdict).toBe('conceptual_medium'); + expect(classify('debug class UserService').verdict).toBe('conceptual_medium'); + expect(classify('review type ApiResponse').verdict).toBe('conceptual_medium'); + }); }); -describe('hasCommandSearch — bin invoked as a command, not mentioned in a quote', () => { - it('true for a direct grep / rg / fd invocation', () => { - expect(hasCommandSearch('grep "x" src/')).toBe(true); - expect(hasCommandSearch('rg "x"')).toBe(true); - expect(hasCommandSearch('fd "x"')).toBe(true); +describe('isExcludedTarget — target outside the ecosystem-universal exclude roots (sc-1359 #3)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it('true for a node_modules target', () => { + expect(isExcludedTarget('grep -rn "TODO" node_modules/foo/lib.js', EXCLUDE_ROOTS)).toBe(true); }); - it('true for grep after a pipe or via xargs', () => { - expect(hasCommandSearch('tsc | grep "x"')).toBe(true); - expect(hasCommandSearch('find . -name "*.ts" | xargs grep "x"')).toBe(true); + it('true for a /tmp target', () => { + expect(isExcludedTarget('grep -oE "FAIL +[^ ]+" /tmp/out.log', EXCLUDE_ROOTS)).toBe(true); }); - it('FALSE when grep is only inside a quoted arg (commit message / echo)', () => { - expect(hasCommandSearch('git commit -m "fix search-tool-counter | grep false positives"')).toBe( + it('FALSE when there is no explicit path operand (bare grep searches cwd)', () => { + expect(isExcludedTarget('grep -rn "the auth flow"', EXCLUDE_ROOTS)).toBe(false); + }); + + it('FALSE for a multi-target grep with at least one non-excluded target', () => { + expect(isExcludedTarget('grep -rn "the auth flow" src/ node_modules/', EXCLUDE_ROOTS)).toBe( false, ); - expect(hasCommandSearch('echo "use grep here"')).toBe(false); }); - it('FALSE for git --grep flag (not a grep command)', () => { - expect(hasCommandSearch('git log --grep="auth flow"')).toBe(false); + it('FALSE for an ordinary source target', () => { + expect(isExcludedTarget('grep -rn "the auth flow" src/', EXCLUDE_ROOTS)).toBe(false); + }); + + it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { + expect(isExcludedTarget('grep -rn "the auth flow" .', EXCLUDE_ROOTS)).toBe(false); + expect(isExcludedTarget('grep -rn "the auth flow" ..', EXCLUDE_ROOTS)).toBe(false); }); }); -describe('isPrimarySearchCommand — first pipeline segment is the search', () => { - it('true when grep/find is the primary command', () => { - expect(isPrimarySearchCommand('grep "x" src/ | head')).toBe(true); - expect(isPrimarySearchCommand('find . -name "x"')).toBe(true); +describe("isOutOfScanRoots — target outside the consumer's configured scanRoots", () => { + const SCAN_ROOTS = ['cli', 'gate-engine']; + + it('FALSE when the target is inside a configured scanRoot', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" gate-engine/', SCAN_ROOTS)).toBe(false); + }); + + it('true when the target is outside every configured scanRoot', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', SCAN_ROOTS)).toBe(true); }); - it('FALSE for a downstream output filter (tsc | grep, vitest | grep)', () => { - expect(isPrimarySearchCommand('tsc --noEmit | grep -E "FAIL"')).toBe(false); - expect(isPrimarySearchCommand('bun vitest run x 2>&1 | grep error')).toBe(false); + it('FALSE when there is no explicit path operand', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow"', SCAN_ROOTS)).toBe(false); }); - it('FALSE when grep is only inside a quoted arg', () => { - expect(isPrimarySearchCommand('git commit -m "... | grep ..."')).toBe(false); + it('FALSE when scanRoots is empty (never match-nothing)', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', [])).toBe(false); + }); + + it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { + // `grep -r "x" .` is an extremely common shape (search cwd). Without this, the counter's + // out-of-scan-roots no-op fires on it whenever the consumer's scanRoots don't happen to + // literally include "." — silently defeating streak counting for the most common invocation. + expect(isOutOfScanRoots('grep -rn "the auth flow" .', SCAN_ROOTS)).toBe(false); + expect(isOutOfScanRoots('grep -rn "the auth flow" ..', SCAN_ROOTS)).toBe(false); + }); +}); + +describe('root matching — boundary safety (prefix, not substring)', () => { + it('a sibling dir that merely shares a string PREFIX with an exclude root is NOT excluded', () => { + // node_modules_shim/ is a real, distinct directory — must not be swallowed by "node_modules". + expect( + isExcludedTarget('grep -rn "x" node_modules_shim/foo.js', ['node_modules', '.git', '/tmp']), + ).toBe(false); + }); + + it('a sibling dir that merely shares a string PREFIX with a scanRoot is NOT treated as in-scope', () => { + // src-legacy/ is a real, distinct directory from the configured "src" scanRoot. + expect(isOutOfScanRoots('grep -rn "x" src-legacy/foo.ts', ['src'])).toBe(true); + }); + + it('an exact-match token (no subpath) still matches its root', () => { + expect(isExcludedTarget('grep -rn "x" node_modules', ['node_modules'])).toBe(true); + expect(isOutOfScanRoots('grep -rn "x" src', ['src'])).toBe(false); + }); + + it("a space-separated value flag's value is never misread as a target (guard-review finding)", () => { + // -A's value ("3") and the real pattern ("cli") must not leak into the target list — only + // the actual trailing path (node_modules/foo.js) is a target, so this must be excluded. + // Regression-critical: "cli" happens to be a real scanRoot in this repo's own guard.config.json, + // so a wrongly-collected "cli" target would silently defeat the any-in-scope-wins exclusion. + expect(isExcludedTarget('grep -A 3 cli node_modules/foo.js', ['node_modules'])).toBe(true); + }); + + it('a SECOND -e pattern value is never misread as a target (guard-review finding, round 7)', () => { + // `grep -e P1 -e P2` (multi-pattern OR search) has no file/dir operand at all — every value + // after -e is a PATTERN, not a target, however many -e flags are used. Without an explicit + // target, this must never be excluded (matches "no operand searches cwd" semantics). + expect(isExcludedTarget('grep -e "the auth flow" -e "node_modules"', ['node_modules'])).toBe( + false, + ); + }); + + it("an unrelated command's argument earlier in a compound command is NOT read as a grep target", () => { + // A non-leading `cd` to an in-scope dir must not "pollute" a later, unrelated grep's own + // out-of-scope target via any-in-scope-wins — targets are scoped per grep invocation, not + // pulled from the whole command string. + expect(isExcludedTarget('cd apps/web && grep -rn "x" node_modules/foo', ['node_modules'])).toBe( + true, + ); + }); +}); + +describe('firstAdvisablePattern — exclusion and pattern-extraction stay correlated (guard-review finding, round 8)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it("does NOT advise on an excluded invocation's pattern just because a LATER invocation has an in-scope target", () => { + // isExcludedTarget aggregates across every invocation (any-in-scope-wins, correct for the + // counter's "is there real search activity" question) — but the GUARD attributes a specific + // pattern to advise on, and that pattern must come from the SAME invocation whose target was + // checked. Without this, `grep "auth flow logic" node_modules && grep "y" src` would advise + // on "auth flow logic" (the node_modules-excluded invocation's pattern) merely because the + // unrelated second invocation's target ("src") isn't excluded. + expect( + firstAdvisablePattern( + 'grep -rn "auth flow logic" node_modules && grep -rn "y" src', + EXCLUDE_ROOTS, + ), + ).toBe('y'); + }); + + it('skips a single excluded invocation entirely, same as isExcludedTarget', () => { + expect( + firstAdvisablePattern('grep -rn "auth flow" node_modules/foo', EXCLUDE_ROOTS), + ).toBeNull(); + }); + + it('returns the pattern unchanged when nothing is excluded', () => { + expect(firstAdvisablePattern('grep -rn "auth flow" src/', EXCLUDE_ROOTS)).toBe('auth flow'); + }); +}); + +describe('Windows-style backslash paths (sc-1359 follow-up — OS coverage)', () => { + it('a backslash-separated node_modules target is excluded, same as forward-slash', () => { + expect( + isExcludedTarget('grep -rn "x" C:\\project\\node_modules\\foo.js', [ + 'node_modules', + '.git', + '/tmp', + ]), + ).toBe(true); + }); + + it('a backslash-separated scanRoot target is recognized as in-scope', () => { + expect( + isOutOfScanRoots('grep -rn "x" C:\\project\\gate-engine\\lib.mts', ['gate-engine']), + ).toBe(false); + }); + + it('an absolute Windows temp-dir root (backslash, no trailing slash) excludes its own subpaths', () => { + expect( + isExcludedTarget('grep -rn "x" C:\\Users\\dev\\AppData\\Local\\Temp\\out.log', [ + 'node_modules', + 'C:\\Users\\dev\\AppData\\Local\\Temp', + ]), + ).toBe(true); + }); +}); + +describe('find / fd target detection (guard-review finding — isPrimarySearchCommand also treats these as searches)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it('a `find` invocation into an excluded root is recognized (path comes before any flag)', () => { + expect(isExcludedTarget('find node_modules -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(true); + }); + + it('a flag VALUE in a `find` expression is never mistaken for a second target', () => { + // -iname's value ("*.spec.ts") must not count as a non-excluded target that saves the command + // via any-in-scope-wins — only the leading path run (before the first flag) is a target. + expect(isExcludedTarget('find node_modules -iname "*.ts" -type f', EXCLUDE_ROOTS)).toBe(true); + }); + + it('a `find .` (cwd) is NOT excluded', () => { + expect(isExcludedTarget('find . -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(false); + }); + + it('an `fd` invocation (PATTERN [PATH...] convention, like grep) into an excluded root is recognized', () => { + expect(isExcludedTarget('fd "\\.spec\\.ts$" node_modules', EXCLUDE_ROOTS)).toBe(true); + }); + + it('an `fd` invocation with an in-scope path is NOT excluded', () => { + expect(isExcludedTarget('fd "\\.spec\\.ts$" src/', EXCLUDE_ROOTS)).toBe(false); + }); +}); + +describe('extractPattern — multi-line compound commands (sc-1359 follow-up)', () => { + it("does NOT bleed a NEXT LINE's command into an earlier operand-bearing grep", () => { + expect( + extractPattern('grep -rln getFlowDriveInfoForSubChat src/\necho "=== recheck ==="'), + ).toBe('getFlowDriveInfoForSubChat'); + }); + + it('falls through past a newline-separated operand-less grep to find the real pattern', () => { + // Mirrors the existing `rg --files | xargs grep "..."` fall-through case, but for a + // heredoc-style multi-line Bash tool_input instead of a `|` pipeline. + expect(extractPattern('rg --files\ngrep "the auth flow here" src/')).toBe('the auth flow here'); + }); + + it("does NOT read the next line's bare command name as the pattern", () => { + // A grep with only flags (no operand) on its own line, followed by an unrelated command on + // the next line: the unrelated command's NAME must never be returned as "the pattern". + expect(extractPattern('grep --files-with-matches\ncat package.json')).toBeNull(); + }); +}); + +describe('extractPattern / splitUnquotedSegments — crash safety on malformed shell quoting', () => { + it('an unterminated double quote does not throw and returns a graceful result', () => { + expect(() => extractPattern('grep -rn "unterminated src/')).not.toThrow(); + }); + + it('an unterminated single quote does not throw and returns a graceful result', () => { + expect(() => extractPattern("grep -rn 'unterminated src/")).not.toThrow(); + }); + + it('an escaped quote WITHIN the pattern itself is preserved, not treated as a terminator', () => { + expect(extractPattern('grep -rn "she said \\"hi\\"" src/')).toBe('she said "hi"'); + }); + + it('adjacent quoted/unquoted runs with no whitespace merge into one token', () => { + // Real bash semantics: `foo"bar"baz` is ONE word, `foobarbaz` — not just the quoted middle. + expect(extractPattern('grep -rn foo"bar"baz src/')).toBe('foobarbaz'); + }); + + it('a quoted MENTION of a bin name earlier in the segment does not hijack a REAL $(...) invocation later in it (guard-review finding, round 8)', () => { + // "grep" inside the quoted "use grep here" must never be matched as an invocation — only the + // real `$(grep ...)` later in the same (unsplit — no `;`/`&`/`|` between them) segment is. + expect(extractPattern('echo "use grep here" $(grep -r "how does auth work" src/)')).toBe( + 'how does auth work', + ); }); }); diff --git a/gate-engine/search-tool/__tests__/search-tool-shell.test.mts b/gate-engine/search-tool/__tests__/search-tool-shell.test.mts new file mode 100644 index 00000000..ec3402dd --- /dev/null +++ b/gate-engine/search-tool/__tests__/search-tool-shell.test.mts @@ -0,0 +1,108 @@ +import { describe, expect, it } from 'vitest'; +import { + hasCommandSearch, + isPrimarySearchCommand, + normalize, + stripQuotes, +} from '../search-tool-shell.mts'; + +// Unit tests for the generic Bash-command-string parsing primitives shared +// by search-tool-lib.mts (split out once that file grew past the size +// ratchet — see search-tool-shell.mts's header). Pure string functions — +// provider-agnostic (Cursor vs Claude run the same Bash strings) so there +// are no provider-specific cases. The one OS-relevant case (Windows-style +// quoted cwd) lives under normalize. + +// A generic working dir WITH SPACES — spaces in the cwd were the original +// false-positive trigger (a path split into "3 words"). Kept provider/OS-neutral. +const CWD = '/Users/dev/My Projects/cool app'; + +describe('normalize — strip cwd + unwrap rtk', () => { + it('strips a leading quoted `cd &&` (cwd with spaces was the #1 false positive)', () => { + expect(normalize(`cd "${CWD}" && grep -n "x" f.ts`)).toBe('grep -n "x" f.ts'); + }); + + it('strips a backslash-escaped-space unquoted cwd', () => { + expect(normalize('cd /a/My\\ Projects/app && grep -rn "x" src/')).toBe('grep -rn "x" src/'); + }); + + it('strips a single-quoted cwd', () => { + expect(normalize(`cd '${CWD}' ; rg "x"`)).toBe('rg "x"'); + }); + + it('strips a Windows-style quoted cwd (Claude Code uses bash on Windows)', () => { + expect(normalize('cd "C:\\proj dir" && grep -n "x" f')).toBe('grep -n "x" f'); + }); + + it('unwraps the rtk token proxy so the underlying bin is classified', () => { + expect(normalize('rtk grep -n "x" f')).toBe('grep -n "x" f'); + expect(normalize('rtk rg "x"')).toBe('rg "x"'); + }); + + it('strips cwd AND unwraps rtk together', () => { + expect(normalize(`cd "${CWD}" && rtk grep -rn "x" src/`)).toBe('grep -rn "x" src/'); + }); + + it('leaves a non-cd command untouched', () => { + expect(normalize('grep -rn "x" src/')).toBe('grep -rn "x" src/'); + }); + + it('does not strip a non-leading cd', () => { + // Only a leading `cd ... &&` is cwd noise; a mid-command cd is intentional. + expect(normalize('echo hi && cd /tmp && grep "x"')).toBe('echo hi && cd /tmp && grep "x"'); + }); +}); + +describe('stripQuotes', () => { + it('blanks double-quoted content including escaped quotes', () => { + expect(stripQuotes('git commit -m "fix: a | grep thing \\"q\\""')).toBe('git commit -m ""'); + }); + + it('blanks single-quoted content', () => { + expect(stripQuotes("echo 'use grep here'")).toBe("echo ''"); + }); + + it('leaves unquoted text intact', () => { + expect(stripQuotes('grep -rn foo src/')).toBe('grep -rn foo src/'); + }); +}); + +describe('hasCommandSearch — bin invoked as a command, not mentioned in a quote', () => { + it('true for a direct grep / rg / fd invocation', () => { + expect(hasCommandSearch('grep "x" src/')).toBe(true); + expect(hasCommandSearch('rg "x"')).toBe(true); + expect(hasCommandSearch('fd "x"')).toBe(true); + }); + + it('true for grep after a pipe or via xargs', () => { + expect(hasCommandSearch('tsc | grep "x"')).toBe(true); + expect(hasCommandSearch('find . -name "*.ts" | xargs grep "x"')).toBe(true); + }); + + it('FALSE when grep is only inside a quoted arg (commit message / echo)', () => { + expect(hasCommandSearch('git commit -m "fix search-tool-counter | grep false positives"')).toBe( + false, + ); + expect(hasCommandSearch('echo "use grep here"')).toBe(false); + }); + + it('FALSE for git --grep flag (not a grep command)', () => { + expect(hasCommandSearch('git log --grep="auth flow"')).toBe(false); + }); +}); + +describe('isPrimarySearchCommand — first pipeline segment is the search', () => { + it('true when grep/find is the primary command', () => { + expect(isPrimarySearchCommand('grep "x" src/ | head')).toBe(true); + expect(isPrimarySearchCommand('find . -name "x"')).toBe(true); + }); + + it('FALSE for a downstream output filter (tsc | grep, vitest | grep)', () => { + expect(isPrimarySearchCommand('tsc --noEmit | grep -E "FAIL"')).toBe(false); + expect(isPrimarySearchCommand('bun vitest run x 2>&1 | grep error')).toBe(false); + }); + + it('FALSE when grep is only inside a quoted arg', () => { + expect(isPrimarySearchCommand('git commit -m "... | grep ..."')).toBe(false); + }); +}); diff --git a/gate-engine/search-tool/eval/queries.json b/gate-engine/search-tool/eval/queries.json index 87da9897..d6a85c64 100644 --- a/gate-engine/search-tool/eval/queries.json +++ b/gate-engine/search-tool/eval/queries.json @@ -43,6 +43,12 @@ "expected_tool": "find_glob", "why": "filename pattern" }, + { + "id": "lit-08", + "pattern": "export async function getFlowDriveInfoForSubChat", + "expected_tool": "grep", + "why": "verbatim code snippet (sc-1359 #1) — keyword + identifier, not English prose" + }, { "id": "sem-01", @@ -92,6 +98,30 @@ "expected_tool": "searchCode", "why": "English question" }, + { + "id": "sem-09", + "pattern": "explain how sessionToken refresh works", + "expected_tool": "searchCode", + "why": "regression guard (sc-1359) — leads with a non-anchored word, contains one camelCase token, must still flag" + }, + { + "id": "sem-10", + "pattern": "handles OAuth callback errors", + "expected_tool": "searchCode", + "why": "regression guard (sc-1359) — domain acronym must not read as a code keyword" + }, + { + "id": "sem-11", + "pattern": "function getUser fails silently", + "expected_tool": "searchCode", + "why": "regression guard (sc-1359, guard-review finding) — a bug report NAMING a symbol, keyword+identifier not last" + }, + { + "id": "sem-12", + "pattern": "explain function getUserData", + "expected_tool": "searchCode", + "why": "regression guard (sc-1359, guard-review finding) — a REQUEST about a symbol, leading verb is not a code keyword" + }, { "id": "gr-01", diff --git a/gate-engine/search-tool/search-tool-counter.mts b/gate-engine/search-tool/search-tool-counter.mts index 00d5b9b2..eda35cc5 100644 --- a/gate-engine/search-tool/search-tool-counter.mts +++ b/gate-engine/search-tool/search-tool-counter.mts @@ -25,9 +25,19 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { resolveGuardConfig } from '../config.mts'; -import { isPrimarySearchCommand, normalize } from './search-tool-lib.mts'; +import { + isExcludedTarget, + isOutOfScanRoots, + isPrimarySearchCommand, + normalize, +} from './search-tool-lib.mts'; import { resolveSearchTools } from './tools.mts'; +// Ecosystem-universal roots the semantic-search index never covers, +// regardless of consumer config — never a hardcoded stack layout (see +// docs/decisions/synced-assets-layout-agnostic.md). +const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp', tmpdir()]; + // The Bash PostToolUse payload Claude Code writes to stdin (only the fields we read). interface PostToolUsePayload { tool_name?: string; @@ -65,7 +75,9 @@ const stateFile = join(stateDir, `${sessionId}.json`); const state = readState(); -const { searchTool } = resolveSearchTools(resolveGuardConfig()); +const guardConfig = resolveGuardConfig(); +const { searchTool } = resolveSearchTools(guardConfig); +const { scanRoots } = guardConfig; // Any semantic-search use resets the counter, hook stays silent. Match the // configured tool exactly, or any tool whose name ends in the searchCode MCP @@ -94,6 +106,18 @@ if (!isPrimarySearchCommand(norm)) { process.exit(0); } +// The target is outside anywhere the semantic index could cover (universal +// exclusions, or outside the consumer's configured scanRoots) — an agent +// legitimately grepping node_modules or /tmp is not enumerating a concept +// across the indexed codebase, but it isn't a non-search command either, so +// this is a THIRD, explicit transition: no-op. Neither incrementing (the bug +// — the counter would escalate toward a tool that can't answer the query) +// nor resetting (an out-of-index grep sandwiched between two real ones would +// wrongly clear a genuine enumeration streak) is correct here. +if (isExcludedTarget(norm, EXCLUDE_ROOTS) || isOutOfScanRoots(norm, scanRoots)) { + process.exit(0); +} + // Every primary search counts — a run of clean-identifier greps is still // concept-by-enumeration. Only the semantic-search tool / a non-search command // (handled above) breaks the streak. diff --git a/gate-engine/search-tool/search-tool-guard.mts b/gate-engine/search-tool/search-tool-guard.mts index 0056c6f3..8383bbba 100644 --- a/gate-engine/search-tool/search-tool-guard.mts +++ b/gate-engine/search-tool/search-tool-guard.mts @@ -18,16 +18,22 @@ */ import { readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { resolveGuardConfig } from '../config.mts'; import { type Classification, classify, - extractPattern, + firstAdvisablePattern, hasCommandSearch, normalize, } from './search-tool-lib.mts'; import { resolveSearchTools, type SearchTools } from './tools.mts'; +// Ecosystem-universal roots the semantic-search index never covers, +// regardless of consumer config — never a hardcoded stack layout (see +// docs/decisions/synced-assets-layout-agnostic.md). +const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp', tmpdir()]; + // The Bash PreToolUse payload Claude Code writes to stdin (only the fields we read). interface PreToolUsePayload { tool_input?: { command?: string }; @@ -68,9 +74,15 @@ const cmd = normalize(rawCmd); // is ignored. classify() then filters literal vs conceptual. if (!hasCommandSearch(cmd)) process.exit(0); -// Extract the user-facing pattern. We look for the first quoted string, -// falling back to the first non-flag token after the binary. -const pattern = extractPattern(cmd); +// Extract the user-facing pattern, skipping any invocation (in a compound +// command) whose OWN target is outside anywhere the semantic index could +// cover (node_modules, /tmp, .git) — the steered tool cannot answer that +// invocation's query regardless of how its pattern classifies. This must +// stay correlated per-invocation, not a separate whole-command exclusion +// check followed by a separate "first pattern" extraction — the two can +// disagree on WHICH invocation they're each looking at (guard-review +// finding, sc-1359 follow-up round 8). +const pattern = firstAdvisablePattern(cmd, EXCLUDE_ROOTS); if (!pattern) process.exit(0); const classification = classify(pattern); diff --git a/gate-engine/search-tool/search-tool-lib.mts b/gate-engine/search-tool/search-tool-lib.mts index 41d636c1..c5bbf432 100644 --- a/gate-engine/search-tool/search-tool-lib.mts +++ b/gate-engine/search-tool/search-tool-lib.mts @@ -1,33 +1,55 @@ /** - * Shared helpers for search-tool-guard (PreToolUse) and search-tool-counter - * (PostToolUse), kept in one place so the two hooks can't drift on what counts - * as a search. Counter uses normalize + isPrimarySearchCommand; guard uses all - * four (normalize, extractPattern, classify, hasCommandSearch). + * Search-tool-specific classification logic for search-tool-guard + * (PreToolUse) and search-tool-counter (PostToolUse), kept in one place so + * the two hooks can't drift on what counts as a search. Counter uses + * isPrimarySearchCommand (re-exported, from search-tool-shell.mts) + the + * out-of-index helpers (isExcludedTarget / isOutOfScanRoots); guard uses + * hasCommandSearch (re-exported), firstAdvisablePattern (which folds + * exclusion + pattern-extraction into one per-invocation-correlated call — + * see its doc comment for why a separate isExcludedTarget-then-extractPattern + * pair is NOT equivalent), and classify. Generic Bash-string parsing + * (normalize, stripQuotes, hasCommandSearch, isPrimarySearchCommand, + * splitUnquotedSegments, tokenizeArgv, matchUnquoted) lives in + * search-tool-shell.mts — this file only re-exports normalize/ + * hasCommandSearch/isPrimarySearchCommand for convenience since both hooks + * need them alongside this file's own exports. * * Pure string classifiers — provider-agnostic (Cursor vs Claude run the same - * Bash strings) and data-free, so this file ships as-is (no consumer coupling). + * Bash strings) and data-free, so this file ships as-is (no consumer + * coupling): the out-of-index helpers take roots as PARAMETERS rather than + * reading any config themselves — the hooks resolve those from the + * consumer's guard.config.json and pass them in. * * Regexes are hoisted to module scope (devkit lint: useTopLevelRegex) — they are * the classifier's static grammar, reused on every hook invocation. */ -// --- normalize --- -const RE_LEADING_CD = /^\s*cd\s+(?:"[^"]*"|'[^']*'|(?:\\.|[^\s;&|])+)\s*(?:&&|;)\s*/g; -const RE_RTK_WRAPPER = /\brtk\s+(grep|rg|ripgrep|find|fd|ack|ag)\b/g; +import { matchUnquoted, splitUnquotedSegments, tokenizeArgv } from './search-tool-shell.mts'; -// --- stripQuotes --- -const RE_DQUOTED = /"(?:\\.|[^"\\])*"/g; -const RE_SQUOTED = /'[^']*'/g; - -// --- hasCommandSearch / isPrimarySearchCommand --- -const RE_COMMAND_SEARCH = /(^|[;&|]\s*|\$\(\s*|\bxargs\s+)(grep|rg|ripgrep|ack|ag|fd)\b/; -const RE_PRIMARY_SEARCH = /(^|[;&]\s*|\$\(\s*)(grep|rg|ripgrep|ack|ag|fd|find)\b/; +export { hasCommandSearch, isPrimarySearchCommand, normalize } from './search-tool-shell.mts'; // --- extractPattern --- -const RE_GREP_SCOPE = /\b(grep|rg|ripgrep|ack|ag)\b([\s\S]*)/; -const RE_FIRST_DQUOTE = /"([^"\\]*(?:\\.[^"\\]*)*)"/; -const RE_FIRST_SQUOTE = /'([^']*)'/; -const RE_CMD_SEPARATOR = /[|;&]/; +// `fd` shares grep's `PATTERN [PATH...]` argv convention (token 0 after the +// bin name is the pattern, not a path) — unlike `find`, whose paths come +// BEFORE any flag/test (see RE_FIND_SCOPE / findInvocationTargets below). +// +// The bin name must be preceded by start-of-segment, whitespace, or `$(` +// (NOT a bare `\b` word boundary, which also fires on `/`/`-`/`.`/`_` — so a +// plain `\b` regex reads "ag" inside the path `src/ag-tools` or "fd" inside +// `fd-cache/` as if the ag/fd BINARY were invoked there). `$(` is listed +// explicitly (not just whitespace) to keep this in sync with +// hasCommandSearch's RE_COMMAND_SEARCH, which already treats `$(` as a +// command start — without it, `result=$(grep ... )` would pass +// hasCommandSearch's gate but then extractPattern would return null, +// silently dropping all steering advice (guard-review finding, sc-1359 +// follow-up round 6). Segments are already split on `;`/`&`/`|`/newline by +// splitUnquotedSegments, so whitespace/start/`$(` covers every real "start +// of a shell word" this needs within one segment (round 5: `find +// src/ag-tools -type f | xargs grep -l "..."` — the phantom "ag" match +// swallowed find's leftover `-type` value as a fake pattern and never +// reached the real `grep` invocation in the next pipeline segment). +const RE_GREP_SCOPE = /(?:^|\s|\$\()(grep|rg|ripgrep|ack|ag|fd)(?=\s|$)([\s\S]*)/; +const RE_FIND_SCOPE = /(?:^|\s|\$\()find(?=\s|$)([\s\S]*)/; const RE_WHITESPACE = /\s+/; // --- classify --- @@ -45,6 +67,34 @@ const RE_META_OR_PUNCT = /[\\^$|(){}[\]?+*=:]/; const RE_QUOTE_OR_COLON = /['"`:]/; const RE_PLAIN_WORD = /^[a-z]+$/; +// A verbatim code snippet (e.g. `export async function getFooBar`, copy-pasted +// to grep for) reads as multi-word natural language by word count alone but +// is a literal search: see looksLikeCodeSnippet for the exact contract +// (every word but the last must be one of these declaration modifiers). +const CODE_KEYWORDS = new Set([ + 'export', + 'import', + 'async', + 'function', + 'const', + 'let', + 'var', + 'class', + 'interface', + 'type', + 'def', + 'return', + 'public', + 'private', + 'protected', + 'static', + 'struct', + 'fn', + 'impl', + 'enum', + 'namespace', +]); + // A pattern-classification verdict tier and its rationale (see classify()). export type SearchVerdict = 'literal' | 'conceptual_medium' | 'conceptual_high'; export interface Classification { @@ -53,73 +103,263 @@ export interface Classification { wordCount?: number; } +// Flags that consume a SEPARATE argv token as a DISCARDABLE value (not +// `=`-joined — tokenizeArgv already merges that form into one flag token; +// and NOT `-e`/`--regexp`, whose value IS a pattern, not discardable — see +// below). Covers both short (`-A`) and long (`--context`) spellings of the +// same flag, since a value can trail either one the same space-separated +// way (guard-review finding, sc-1359 follow-up round 2: the short-flag-only +// version left `--context 3 "pattern" src/` misreading '3' as the pattern). +// Not an exhaustive grep/fd flag parser — this stays an advisory heuristic, +// not a shell — just the common context/count/pattern-source flags that +// would otherwise leak their value into the pattern or the target list. +const RE_VALUE_FLAG = + /^(-[ABCmfdD]|--(after-context|before-context|context|max-count|file|directories|devices))$/; +const RE_PATTERN_FLAG = /^(-e|--regexp)$/; + +// Classify a grep-family invocation's argv tokens (everything after the bin +// name) into: values passed via `-e`/`--regexp` (each IS a pattern, never a +// target — grep's own contract once `-e` appears at all: with no `-e`, +// grep's FIRST bare positional is the pattern and the rest are targets, but +// with `-e` present every bare positional is a target instead, since the +// pattern is already fully specified) — and plain positional (non-flag, +// non-flag-value) tokens. Two lists, not one flat one, is what lets +// extractPattern and target detection agree on which token is "the +// pattern" without a second `-e` value being misread as a target (guard- +// review finding, sc-1359 follow-up round 7: `grep -e "the auth flow" -e +// "node_modules"` has NO target at all — both values are patterns — but a +// flat token list can't tell the second `-e` value apart from a real path). +// +// `bin` matters here: `fd`'s `-e`/`--extension` is a file-EXTENSION filter +// (its value is discardable, like -A/-B/-C), unlike grep-family's +// `-e`/`--regexp` (whose value IS the pattern) — the two bins spell +// unrelated flags the same way, so `fd -e ts "conceptual query" src/` must +// NOT read "ts" as the pattern (guard-review finding, sc-1359 follow-up +// round 10). +function classifyOperands( + tokens: string[], + bin: string, +): { patternValues: string[]; positionals: string[] } { + const isFd = bin === 'fd'; + const isPatternFlag = (t: string) => !isFd && RE_PATTERN_FLAG.test(t); + const isValueFlag = (t: string) => RE_VALUE_FLAG.test(t) || (isFd && RE_PATTERN_FLAG.test(t)); + const patternValues: string[] = []; + const positionals: string[] = []; + for (let i = 0; i < tokens.length; i++) { + const t = tokens[i]; + if (!t.startsWith('-')) { + positionals.push(t); + continue; + } + if (isPatternFlag(t)) { + if (i + 1 < tokens.length) patternValues.push(tokens[++i]); + continue; + } + if (isValueFlag(t)) i += 1; // discard this flag's value token + } + return { patternValues, positionals }; +} + +// For each segment that actually INVOKES a grep-family bin (not merely +// mentions one inside a quote — matchUnquoted skips any match that falls +// inside a quoted region, scoped to the segment): the pattern is the first +// `-e`/`--regexp` value if any were given, else the first bare positional — +// and the targets are every REMAINING bare positional (all of them, if `-e` +// supplied the pattern; everything after the first, otherwise). Segment- +// scanning (see splitUnquotedSegments) means a LATER command's argument +// across `&&`/`;`/`|`/newline is never folded into an EARLIER grep's +// tokens — the bug that let `grep foo src/ && echo "=== recheck ==="` read +// the echo's argument as the search pattern. Shared by extractPattern +// (wants the pattern) and the out-of-index helpers (want the targets), so +// the two can't drift on what counts as "this grep's own argv". +function grepInvocations(cmd: string): { pattern: string | null; targets: string[] }[] { + const invocations: { pattern: string | null; targets: string[] }[] = []; + for (const segment of splitUnquotedSegments(cmd)) { + const match = matchUnquoted(segment, RE_GREP_SCOPE); + if (!match) continue; + const { patternValues, positionals } = classifyOperands(tokenizeArgv(match[2]), match[1]); + invocations.push( + patternValues.length + ? { pattern: patternValues[0], targets: positionals } + : { pattern: positionals[0] ?? null, targets: positionals.slice(1) }, + ); + } + return invocations; +} + +/** + * Extract the user-facing pattern from a grep-family invocation (see + * grepInvocations for the `-e`/positional contract). A segment whose + * invocation has no pattern at all (e.g. `rg --files`) does not match — + * scanning continues into the next segment, so a real pattern later in the + * pipeline (`rg --files | xargs grep "the auth flow"`) is still found. + */ +export function extractPattern(c: string): string | null { + for (const inv of grepInvocations(c)) { + if (inv.pattern !== null) return inv.pattern; + } + return null; +} + /** - * Strip noise that is never part of the search query: - * - leading `cd (&&|;)` segments (the working dir is not the query), - * - the `rtk` token-proxy wrapper (`rtk grep "x"` → `grep "x"`). + * Like extractPattern, but — for a compound command with more than one + * grep-family invocation — skips any invocation whose OWN targets are + * entirely inside `excludeRoots`, so its pattern is never advised on merely + * because a LATER, unrelated invocation elsewhere in the same command has a + * non-excluded target. Guard-only: isExcludedTarget's whole-command + * aggregate (any-in-scope-wins) stays correct for the counter, which only + * asks "is there real search activity happening anywhere in this command", + * not "which specific pattern should I advise on" — the guard is the one + * that attributes a pattern, so it's the one that needs this per-invocation + * correlation (guard-review finding, sc-1359 follow-up round 8: `grep "auth + * flow logic" node_modules && grep "y" src` used to advise on "auth flow + * logic" — the excluded invocation's pattern — because isExcludedTarget and + * extractPattern picked their answers from two different invocations). */ -export function normalize(c: string): string { - return c.replace(RE_LEADING_CD, '').replace(RE_RTK_WRAPPER, '$1'); +export function firstAdvisablePattern(cmd: string, excludeRoots: string[]): string | null { + for (const inv of grepInvocations(cmd)) { + if (inv.pattern === null) continue; + const excluded = + inv.targets.length > 0 && + inv.targets.every((t) => excludeRoots.some((r) => matchesRoot(t, r))); + if (excluded) continue; + return inv.pattern; + } + return null; } -/** Blank out quoted-string contents so a `grep` mentioned inside a commit - * message / echo arg isn't mistaken for a grep invocation. */ -export function stripQuotes(c: string): string { - return c.replace(RE_DQUOTED, '""').replace(RE_SQUOTED, "''"); +// `find`'s argv is `[path...] [expression]` — its paths are the LEADING +// non-flag tokens, not "everything after token 0" (find has no pattern +// argument the way grep/fd do). Stopping at the first flag/test also keeps +// a flag's value (`-iname "*.ts"`) from being misread as a second target — +// isPrimarySearchCommand already counts `find` as a search (RE_PRIMARY_SEARCH), +// so target detection must recognize it too or the counter's out-of-index +// no-op never fires for it (guard-review finding, sc-1359 follow-up). +function findInvocationTargets(cmd: string): string[] { + const targets: string[] = []; + for (const segment of splitUnquotedSegments(cmd)) { + const match = matchUnquoted(segment, RE_FIND_SCOPE); + if (!match) continue; + for (const t of tokenizeArgv(match[1])) { + if (t.startsWith('-')) break; + targets.push(t); + } + } + return targets; +} + +// A bare `.`/`./`/`..`/`../` target is a cwd/parent-dir reference, not a +// real scoped path — it carries no information about which directory +// hierarchy is being searched, so it must be treated exactly like "no +// operand" (never excluded, never out-of-scanRoots). Without this filter, +// `matchesRoot('.', root)` is false for every root (it isn't literally +// equal to, nor does it start with, any configured root), which +// isOutOfScanRoots' inverted "every target fails to match" check reads as +// "every target is out of scope" — silently no-op'ing the counter on the +// extremely common `grep -r "x" .` shape (guard-review finding, sc-1359 +// follow-up round 9). A real relative sub-path like `./src` is unaffected — +// this only matches the BARE reference, nothing trailing it. +const RE_CWD_REF = /^\.\.?\/?$/; + +// Every non-pattern target across all grep/fd invocations (see +// grepInvocations), plus every leading path token across all find +// invocations — candidate targets. Deliberately NOT filtered by "contains a +// slash": a bare target with no subpath (`grep -rn "x" node_modules`, `find +// node_modules`) is a normal, common shape and must still be seen. +function targetTokens(cmd: string): string[] { + return [ + ...grepInvocations(cmd).flatMap((inv) => inv.targets), + ...findInvocationTargets(cmd), + ].filter((t) => !RE_CWD_REF.test(t)); +} + +// Windows paths can appear literally in a bash command string on Windows +// (Claude Code runs bash there too — see normalize's "Windows-style quoted +// cwd" case) using `\` separators; normalize both sides to `/` so root +// matching doesn't silently no-op on that platform. +function toPosixSlashes(p: string): string { + return p.replace(/\\/g, '/'); +} + +function matchesRoot(token: string, root: string): boolean { + let t = toPosixSlashes(token); + if (t.startsWith('./')) t = t.slice(2); + const rFull = toPosixSlashes(root); + const r = rFull.endsWith('/') ? rFull.slice(0, -1) : rFull; + // NOTE: no bare `t.startsWith(root)` prefix check here — that would also + // match an unrelated SIBLING directory that merely shares root's string + // prefix (e.g. root "src" matching token "src-legacy/x.ts", or root + // "node_modules" matching "node_modules_shim/x.js"). Every check below is + // boundary-safe: exact match, root-then-`/`, or root wrapped in `/`s. + return t === r || t.startsWith(`${r}/`) || t.includes(`/${r}/`); } /** - * Guard: is a grep-family binary actually INVOKED as a command (start of a - * pipeline segment, after a separator/`$(`, or via `xargs`) — not merely - * mentioned inside a quoted arg like `git commit -m "...| grep..."`. + * Is every target in `cmd` inside one of the ecosystem-universal excluded + * roots (node_modules, .git, the OS temp dir — never a hardcoded stack + * layout, see docs/decisions/synced-assets-layout-agnostic.md)? A bare + * pattern with no target searches cwd and is never excluded; a multi-target + * grep with at least one non-excluded target is not excluded either (any + * in-scope target keeps the command worth advising on). */ -export function hasCommandSearch(cmd: string): boolean { - return RE_COMMAND_SEARCH.test(stripQuotes(cmd)); +export function isExcludedTarget(cmd: string, excludeRoots: string[]): boolean { + const targets = targetTokens(cmd); + if (!targets.length) return false; + return targets.every((t) => excludeRoots.some((root) => matchesRoot(t, root))); } /** - * Counter: is the FIRST pipeline segment itself a search command? Distinguishes - * a primary search (`grep x | head`) from a downstream output filter - * (`tsc | grep x`, which is not code search). + * Is every target in `cmd` OUTSIDE all of the consumer's `scanRoots`? + * Mirrors isExcludedTarget's "no operand / any-in-scope-wins" semantics, + * scoped to the consumer's configured roots (resolveGuardConfig) instead of + * the universal exclude list. Counter-only (see search-tool-guard / + * search-tool-counter): applying this to the guard would silence it on + * eval/eval.mts's synthetic `src/` targets whenever a consumer's scanRoots + * don't include `src` (e.g. devkit's own `["cli","gate-engine"]`). */ -export function isPrimarySearchCommand(cmd: string): boolean { - const first = stripQuotes(cmd).split('|')[0]; - return RE_PRIMARY_SEARCH.test(first); +export function isOutOfScanRoots(cmd: string, scanRoots: string[]): boolean { + if (!scanRoots.length) return false; + const targets = targetTokens(cmd); + if (!targets.length) return false; + return targets.every((t) => !scanRoots.some((root) => matchesRoot(t, root))); } /** - * Extract the user-facing pattern from a grep-family invocation. Scopes onto - * the grep/rg segment (even after a pipe), then the first quoted string, else - * the first non-flag token. Returns null when nothing pattern-like is found. + * A verbatim code snippet — one or more declaration-modifier keywords + * (`export`, `async`, `function`, `const`, …) immediately followed by a + * single identifier-shaped token (not a plain lowercase English word) AS + * THE LAST WORD — reads as multi-word prose by shape alone but is a literal + * search: `export async function getFooBar` is a source line, not a + * description. Both halves of the contract matter, each closing a + * guard-review-caught misclassification (sc-1359 follow-up): + * - the identifier must be LAST, not just present anywhere. Without this, + * a bug report that merely NAMES a symbol before describing it in + * English — "function getUser fails silently" / "class Foo needs + * refactoring" / "type UserResponse looks wrong" — has a keyword + + * identifier pair too, but ends in an English predicate + * (`silently`/`refactoring`/`wrong`), so it correctly stays conceptual. + * - EVERY word before the identifier must itself be a declaration + * modifier — not just "a keyword somewhere". Without this, a REQUEST + * about a symbol — "explain function getUserData" / "debug class + * UserService" / "review type ApiResponse" — has a keyword + identifier + * pair too, but leads with an action verb ("explain"/"debug"/"review") + * that isn't a modifier, so it correctly stays conceptual. + * Queries with no keyword at all, e.g. "explain how sessionToken refresh + * works" or "handles OAuth callback errors", never reach the modifier check. */ -export function extractPattern(c: string): string | null { - // Scope from the first grep-family bin to end-of-command. We do NOT truncate - // at | ; & here because a quoted pattern may legitimately contain them - // (e.g. `grep -E "auth|session"`). Scoping from the FIRST bin means the first - // quoted string we find is that bin's own pattern, even across a pipe. - const grepMatch = c.match(RE_GREP_SCOPE); - const after = grepMatch ? grepMatch[2] : c; - - const dq = after.match(RE_FIRST_DQUOTE); - if (dq) return dq[1]; - const sq = after.match(RE_FIRST_SQUOTE); - if (sq) return sq[1]; - // Unquoted fallback: first non-flag token, stopping at a command separator so - // we don't grab a downstream command's argument. - const tokens = after.split(RE_CMD_SEPARATOR)[0].trim().split(RE_WHITESPACE); - for (const t of tokens) { - if (!t) continue; - if (t.startsWith('-')) continue; - return t; - } - return null; +function looksLikeCodeSnippet(words: string[]): boolean { + if (words.length < 2) return false; + const last = words[words.length - 1]; + const lastIsIdentifier = RE_SINGLE_IDENTIFIER.test(last) && !RE_PLAIN_WORD.test(last); + if (!lastIsIdentifier) return false; + return words.slice(0, -1).every((w) => CODE_KEYWORDS.has(w)); } /** * Classify a pattern as literal (grep is correct) or conceptual (steer toward * the semantic-search tool). Verdicts: literal | conceptual_medium | conceptual_high. */ -// Reason: the branches ARE the literal-vs-conceptual classification algorithm: each guard is a distinct verdict tier (filesystem path, regex/glob, single identifier, error-message shape, question word, descriptive phrasing, 4-word/3-word/2-word thresholds) checked in priority order; extracting them hides the heuristic ladder +// Reason: the branches ARE the literal-vs-conceptual classification algorithm: each guard is a distinct verdict tier (filesystem path, regex/glob, single identifier, error-message shape, question word, descriptive phrasing, code-snippet shape, 4-word/3-word/2-word thresholds) checked in priority order; extracting them hides the heuristic ladder // fallow-ignore-next-line complexity export function classify(pattern: string | null | undefined): Classification { const trimmed = (pattern ?? '').trim(); @@ -162,12 +402,26 @@ export function classify(pattern: string | null | undefined): Classification { return { verdict: 'conceptual_high', reason: 'descriptive phrasing', wordCount }; } + // Verbatim code snippet (keyword + identifier token + no connective) → literal. + if (wordCount >= 3 && looksLikeCodeSnippet(words)) { + return { verdict: 'literal', reason: 'code snippet shape', wordCount }; + } + // 4+ words, no metachars → high. if (wordCount >= 4 && !RE_META_OR_PUNCT.test(trimmed)) { return { verdict: 'conceptual_high', reason: '4+ words, no metachars', wordCount }; } - // 3 words → medium (unless error-message shape). + // 3 words → medium (unless error-message shape). No metachar check here, + // deliberately — ordinary English punctuation (a trailing `?`, a `:` after + // an intro phrase) would trip `RE_META_OR_PUNCT` too and silently flip a + // genuine conceptual query like "fix auth bug?" to literal (guard-review + // finding, sc-1359 follow-up). The 4+-word branch's equivalent check + // predates this file's rewrite and has the same limitation, but that's out + // of scope here — extractPattern's segment scan already prevents the + // original echo-bleed-through defect (sc-1359 #2) from ever handing + // classify() a punctuation-laden string like `'=== recheck ==='` as if it + // were a real pattern, so no metachar check is needed here as a backstop. if (wordCount === 3) { if (RE_LEADING_CAP.test(trimmed) && RE_QUOTE_OR_COLON.test(trimmed)) { return { verdict: 'literal', reason: 'error message shape', wordCount }; diff --git a/gate-engine/search-tool/search-tool-shell.mts b/gate-engine/search-tool/search-tool-shell.mts new file mode 100644 index 00000000..23686bb8 --- /dev/null +++ b/gate-engine/search-tool/search-tool-shell.mts @@ -0,0 +1,209 @@ +/** + * Generic Bash-command-string parsing primitives shared by search-tool-lib.mts + * (and, through it, search-tool-guard/search-tool-counter). Split out from + * search-tool-lib.mts once that file grew past the size ratchet — this half + * knows nothing about "search tool" classification semantics; it only knows + * how to strip cwd noise, blank quoted content, detect a search-family bin + * invocation, and tokenize a command string respecting shell quoting. + * + * Pure string functions — provider-agnostic (Cursor vs Claude run the same + * Bash strings) and data-free, so this file ships as-is (no consumer + * coupling). + * + * Regexes are hoisted to module scope (devkit lint: useTopLevelRegex) — they + * are the parser's static grammar, reused on every hook invocation. + */ + +// --- normalize --- +const RE_LEADING_CD = /^\s*cd\s+(?:"[^"]*"|'[^']*'|(?:\\.|[^\s;&|])+)\s*(?:&&|;)\s*/g; +const RE_RTK_WRAPPER = /\brtk\s+(grep|rg|ripgrep|find|fd|ack|ag)\b/g; + +// --- stripQuotes --- +const RE_DQUOTED = /"(?:\\.|[^"\\])*"/g; +const RE_SQUOTED = /'[^']*'/g; + +// --- hasCommandSearch / isPrimarySearchCommand --- +const RE_COMMAND_SEARCH = /(^|[;&|]\s*|\$\(\s*|\bxargs\s+)(grep|rg|ripgrep|ack|ag|fd)\b/; +const RE_PRIMARY_SEARCH = /(^|[;&]\s*|\$\(\s*)(grep|rg|ripgrep|ack|ag|fd|find)\b/; + +// --- splitUnquotedSegments --- +// Includes `\n`: a multi-line Bash tool_input (heredoc-style, or several +// commands joined by literal newlines rather than `&&`/`;`) is just as much +// a command boundary as any other separator — without it, a grep with no +// operand on one line could still read the NEXT line's command name as its +// pattern (the same defect class as the `&&`-echo case, just via `\n`). +const RE_SEGMENT_SEPARATOR = /[;&|\n]/; + +// --- tokenizeArgv --- +const RE_WS_CHAR = /\s/; + +/** + * Strip noise that is never part of the search query: + * - leading `cd (&&|;)` segments (the working dir is not the query), + * - the `rtk` token-proxy wrapper (`rtk grep "x"` → `grep "x"`). + */ +export function normalize(c: string): string { + return c.replace(RE_LEADING_CD, '').replace(RE_RTK_WRAPPER, '$1'); +} + +/** Blank out quoted-string contents so a `grep` mentioned inside a commit + * message / echo arg isn't mistaken for a grep invocation. */ +export function stripQuotes(c: string): string { + return c.replace(RE_DQUOTED, '""').replace(RE_SQUOTED, "''"); +} + +/** + * Guard: is a grep-family binary actually INVOKED as a command (start of a + * pipeline segment, after a separator/`$(`, or via `xargs`) — not merely + * mentioned inside a quoted arg like `git commit -m "...| grep..."`. + */ +export function hasCommandSearch(cmd: string): boolean { + return RE_COMMAND_SEARCH.test(stripQuotes(cmd)); +} + +/** + * Counter: is the FIRST pipeline segment itself a search command? Distinguishes + * a primary search (`grep x | head`) from a downstream output filter + * (`tsc | grep x`, which is not code search). + */ +export function isPrimarySearchCommand(cmd: string): boolean { + const first = stripQuotes(cmd).split('|')[0]; + return RE_PRIMARY_SEARCH.test(first); +} + +/** + * Split a command into segments at unquoted `;`, `&`, `|`, newline — quote + * state is tracked char-by-char (mirroring stripQuotes' escape handling) so + * a separator INSIDE a quoted pattern (e.g. `grep -E "auth|session"`) is + * never mistaken for a command boundary. `&&`/`||` naturally produce a + * harmless empty segment between the two chars — no grep-family bin matches + * an empty string, so it's skipped downstream without special-casing. + */ +export function splitUnquotedSegments(c: string): string[] { + const segments: string[] = []; + let cur = ''; + let quote: '"' | "'" | null = null; + for (let i = 0; i < c.length; i++) { + const ch = c[i]; + if (quote) { + cur += ch; + if (ch === '\\' && quote === '"' && i + 1 < c.length) { + i += 1; + cur += c[i]; + continue; + } + if (ch === quote) quote = null; + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + cur += ch; + continue; + } + if (RE_SEGMENT_SEPARATOR.test(ch)) { + segments.push(cur); + cur = ''; + continue; + } + cur += ch; + } + segments.push(cur); + return segments; +} + +/** + * Tokenize on unquoted whitespace, merging a quoted span into whatever token + * it's adjacent to — so `--include="*.mts"` is ONE token (`--include=*.mts`, + * a flag, correctly skipped) rather than exposing `*.mts` as if it were a + * free-standing quoted pattern. Quote characters are stripped from the + * token text; internal whitespace inside a quoted span is preserved. + */ +export function tokenizeArgv(s: string): string[] { + const tokens: string[] = []; + let cur = ''; + let inToken = false; + let quote: '"' | "'" | null = null; + for (let i = 0; i < s.length; i++) { + const ch = s[i]; + if (quote) { + if (ch === '\\' && quote === '"' && i + 1 < s.length) { + i += 1; + cur += s[i]; + continue; + } + if (ch === quote) { + quote = null; + continue; + } + cur += ch; + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + inToken = true; + continue; + } + if (RE_WS_CHAR.test(ch)) { + if (inToken) { + tokens.push(cur); + cur = ''; + inToken = false; + } + continue; + } + cur += ch; + inToken = true; + } + if (inToken) tokens.push(cur); + return tokens; +} + +// Is `s[index]` inside a quoted region? Scans from the start, tracking quote +// state (mirroring stripQuotes'/tokenizeArgv's escape handling) up to (not +// including) `index`. +function isInsideQuotes(s: string, index: number): boolean { + let quote: '"' | "'" | null = null; + for (let i = 0; i < index; i++) { + const ch = s[i]; + if (quote) { + if (ch === '\\' && quote === '"' && i + 1 < index) { + i += 1; + continue; + } + if (ch === quote) quote = null; + continue; + } + if (ch === '"' || ch === "'") quote = ch; + } + return quote !== null; +} + +/** + * Find the first UNQUOTED match of `re` in `segment` — i.e. skip any + * candidate match whose start position falls inside a quoted region, so a + * bin name merely MENTIONED inside a quote (e.g. `echo "use grep here" + * $(grep ...)`) can never hijack the match ahead of a REAL invocation later + * in the same segment. Operates directly on the ORIGINAL segment text + * throughout — never reuses an index from stripQuotes' output, which isn't + * length-preserving (guard-review finding, sc-1359 follow-up round 8: the + * previous "gate via stripQuotes, extract via raw match" approach let an + * earlier quoted mention's position win the match instead of the real one). + * + * On a rejected candidate, `lastIndex` is reset to just past that match's + * START (not its end) before retrying: `re`'s trailing capture group is + * greedy (`[\s\S]*`, matches to end-of-string), so a rejected match's FULL + * span — and thus the auto-advanced `lastIndex` after a normal failed + * .exec() retry — already covers the rest of the string, including any + * real invocation after it. Re-scanning from just past the rejected + * match's start is what lets that real invocation still be found. + */ +export function matchUnquoted(segment: string, re: RegExp): RegExpExecArray | null { + const globalRe = new RegExp(re.source, re.flags.includes('g') ? re.flags : `${re.flags}g`); + let m: RegExpExecArray | null = globalRe.exec(segment); + while (m !== null) { + if (!isInsideQuotes(segment, m.index)) return m; + globalRe.lastIndex = m.index + 1; + m = globalRe.exec(segment); + } + return null; +} diff --git a/package.json b/package.json index f91c478d..c47dfa3a 100644 --- a/package.json +++ b/package.json @@ -63,6 +63,7 @@ "benchmarks:check": "bun gate-engine/eval/cli.mts check", "benchmarks:render": "bun gate-engine/eval/cli.mts render", "benchmarks:typecheck": "tsc -p gate-engine/eval/tsconfig.json", + "search-eval:check": "node gate-engine/search-tool/eval/eval.mts --fail", "format": "biome check --write .", "typecheck": "tsc -p tsconfig.json", "prepare": "husky", From c28a0b3a2adeae14bccd55457be918de08632ede Mon Sep 17 00:00:00 2001 From: norvalbv Date: Wed, 5 Aug 2026 12:13:58 +0100 Subject: [PATCH 2/2] fix(search-tool): scope guard advice to configured scanRoots too, not just EXCLUDE_ROOTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/search-tool-hooks.test.mts | 21 ++ .../__tests__/search-tool-lib.test.mts | 218 ++------------- .../__tests__/search-tool-targets.test.mts | 253 ++++++++++++++++++ gate-engine/search-tool/eval/eval.mts | 12 +- gate-engine/search-tool/search-tool-guard.mts | 16 +- gate-engine/search-tool/search-tool-lib.mts | 135 ++++++---- 6 files changed, 395 insertions(+), 260 deletions(-) create mode 100644 gate-engine/search-tool/__tests__/search-tool-targets.test.mts diff --git a/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts b/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts index 3636722c..c17386b8 100644 --- a/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts +++ b/gate-engine/search-tool/__tests__/search-tool-hooks.test.mts @@ -32,6 +32,13 @@ function runGuard(command, env = {}) { const out = execFileSync('node', [GUARD], { input: JSON.stringify({ tool_input: { command } }), env: { ...process.env, ...env }, + // No guard.config.json here (fresh tmpdir) so scanRoots falls back to the + // DEFAULT (['src']), matching this file's `src/`-targeted fixtures — and + // isolates these hook-wiring tests from devkit's OWN dogfood scanRoots + // (["cli","gate-engine"]), which would otherwise read every `src/`/`.` + // target here as out-of-scope now that firstAdvisablePattern also scopes + // to scanRoots (PR review finding, sc-1359 follow-up). + cwd: stateDir, }).toString(); return out ? JSON.parse(out) : null; } @@ -105,6 +112,20 @@ describe('search-tool-guard (PreToolUse)', () => { expect(advice).not.toContain('auth flow logic'); }); + it('stays quiet on a target outside the configured scanRoots (PR review finding)', () => { + // This fixture's isolated cwd has no guard.config.json, so scanRoots falls back to the + // DEFAULT (['src']) — "docs/" is out of scope under that default. + expect(guardFires(`grep -rn "how does the docs pipeline render" docs/`)).toBe(false); + }); + + it('in a compound command, skips the out-of-scanRoots invocation and advises on the in-scope one (PR review finding)', () => { + const advice = runGuard( + `grep -rn "how does the docs pipeline render" docs/ && grep -rn "the retry backoff path" src`, + )?.hookSpecificOutput?.additionalContext; + expect(advice).toContain('the retry backoff path'); + expect(advice).not.toContain('how does the docs pipeline render'); + }); + it('steers toward the CONFIGURED search tool (not a hardcoded name)', () => { const advice = runGuard(`grep -rn "auth flow" .`)?.hookSpecificOutput?.additionalContext; expect(advice).toContain(SEARCH_TOOL); diff --git a/gate-engine/search-tool/__tests__/search-tool-lib.test.mts b/gate-engine/search-tool/__tests__/search-tool-lib.test.mts index 018523a8..4b436d9e 100644 --- a/gate-engine/search-tool/__tests__/search-tool-lib.test.mts +++ b/gate-engine/search-tool/__tests__/search-tool-lib.test.mts @@ -1,19 +1,15 @@ import { describe, expect, it } from 'vitest'; -import { - classify, - extractPattern, - firstAdvisablePattern, - isExcludedTarget, - isOutOfScanRoots, -} from '../search-tool-lib.mts'; - -// Unit tests for search-tool-lib.mts's classification logic (used by -// search-tool-guard + search-tool-counter). Generic Bash-string parsing -// (normalize, stripQuotes, hasCommandSearch, isPrimarySearchCommand) has its -// own coverage in search-tool-shell.test.mts, split out alongside -// search-tool-shell.mts once this file grew past the size ratchet. These are -// pure string classifiers — provider-agnostic (Cursor vs Claude run the same -// Bash strings) so there are no provider-specific cases. +import { classify, extractPattern } from '../search-tool-lib.mts'; + +// Unit tests for search-tool-lib.mts's PATTERN classification (extractPattern +// + classify). Generic Bash-string parsing (normalize, stripQuotes, +// hasCommandSearch, isPrimarySearchCommand) has its own coverage in +// search-tool-shell.test.mts, and out-of-index TARGET detection +// (isExcludedTarget / isOutOfScanRoots / firstAdvisablePattern) has its own +// in search-tool-targets.test.mts — both split out once this file grew past +// the size ratchet. These are pure string classifiers — provider-agnostic +// (Cursor vs Claude run the same Bash strings) so there are no +// provider-specific cases. // A generic working dir WITH SPACES — spaces in the cwd were the original // false-positive trigger (a path split into "3 words"). Kept provider/OS-neutral. @@ -236,192 +232,6 @@ describe('classify — conceptual cases (steer to searchCode)', () => { }); }); -describe('isExcludedTarget — target outside the ecosystem-universal exclude roots (sc-1359 #3)', () => { - const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; - - it('true for a node_modules target', () => { - expect(isExcludedTarget('grep -rn "TODO" node_modules/foo/lib.js', EXCLUDE_ROOTS)).toBe(true); - }); - - it('true for a /tmp target', () => { - expect(isExcludedTarget('grep -oE "FAIL +[^ ]+" /tmp/out.log', EXCLUDE_ROOTS)).toBe(true); - }); - - it('FALSE when there is no explicit path operand (bare grep searches cwd)', () => { - expect(isExcludedTarget('grep -rn "the auth flow"', EXCLUDE_ROOTS)).toBe(false); - }); - - it('FALSE for a multi-target grep with at least one non-excluded target', () => { - expect(isExcludedTarget('grep -rn "the auth flow" src/ node_modules/', EXCLUDE_ROOTS)).toBe( - false, - ); - }); - - it('FALSE for an ordinary source target', () => { - expect(isExcludedTarget('grep -rn "the auth flow" src/', EXCLUDE_ROOTS)).toBe(false); - }); - - it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { - expect(isExcludedTarget('grep -rn "the auth flow" .', EXCLUDE_ROOTS)).toBe(false); - expect(isExcludedTarget('grep -rn "the auth flow" ..', EXCLUDE_ROOTS)).toBe(false); - }); -}); - -describe("isOutOfScanRoots — target outside the consumer's configured scanRoots", () => { - const SCAN_ROOTS = ['cli', 'gate-engine']; - - it('FALSE when the target is inside a configured scanRoot', () => { - expect(isOutOfScanRoots('grep -rn "the auth flow" gate-engine/', SCAN_ROOTS)).toBe(false); - }); - - it('true when the target is outside every configured scanRoot', () => { - expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', SCAN_ROOTS)).toBe(true); - }); - - it('FALSE when there is no explicit path operand', () => { - expect(isOutOfScanRoots('grep -rn "the auth flow"', SCAN_ROOTS)).toBe(false); - }); - - it('FALSE when scanRoots is empty (never match-nothing)', () => { - expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', [])).toBe(false); - }); - - it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { - // `grep -r "x" .` is an extremely common shape (search cwd). Without this, the counter's - // out-of-scan-roots no-op fires on it whenever the consumer's scanRoots don't happen to - // literally include "." — silently defeating streak counting for the most common invocation. - expect(isOutOfScanRoots('grep -rn "the auth flow" .', SCAN_ROOTS)).toBe(false); - expect(isOutOfScanRoots('grep -rn "the auth flow" ..', SCAN_ROOTS)).toBe(false); - }); -}); - -describe('root matching — boundary safety (prefix, not substring)', () => { - it('a sibling dir that merely shares a string PREFIX with an exclude root is NOT excluded', () => { - // node_modules_shim/ is a real, distinct directory — must not be swallowed by "node_modules". - expect( - isExcludedTarget('grep -rn "x" node_modules_shim/foo.js', ['node_modules', '.git', '/tmp']), - ).toBe(false); - }); - - it('a sibling dir that merely shares a string PREFIX with a scanRoot is NOT treated as in-scope', () => { - // src-legacy/ is a real, distinct directory from the configured "src" scanRoot. - expect(isOutOfScanRoots('grep -rn "x" src-legacy/foo.ts', ['src'])).toBe(true); - }); - - it('an exact-match token (no subpath) still matches its root', () => { - expect(isExcludedTarget('grep -rn "x" node_modules', ['node_modules'])).toBe(true); - expect(isOutOfScanRoots('grep -rn "x" src', ['src'])).toBe(false); - }); - - it("a space-separated value flag's value is never misread as a target (guard-review finding)", () => { - // -A's value ("3") and the real pattern ("cli") must not leak into the target list — only - // the actual trailing path (node_modules/foo.js) is a target, so this must be excluded. - // Regression-critical: "cli" happens to be a real scanRoot in this repo's own guard.config.json, - // so a wrongly-collected "cli" target would silently defeat the any-in-scope-wins exclusion. - expect(isExcludedTarget('grep -A 3 cli node_modules/foo.js', ['node_modules'])).toBe(true); - }); - - it('a SECOND -e pattern value is never misread as a target (guard-review finding, round 7)', () => { - // `grep -e P1 -e P2` (multi-pattern OR search) has no file/dir operand at all — every value - // after -e is a PATTERN, not a target, however many -e flags are used. Without an explicit - // target, this must never be excluded (matches "no operand searches cwd" semantics). - expect(isExcludedTarget('grep -e "the auth flow" -e "node_modules"', ['node_modules'])).toBe( - false, - ); - }); - - it("an unrelated command's argument earlier in a compound command is NOT read as a grep target", () => { - // A non-leading `cd` to an in-scope dir must not "pollute" a later, unrelated grep's own - // out-of-scope target via any-in-scope-wins — targets are scoped per grep invocation, not - // pulled from the whole command string. - expect(isExcludedTarget('cd apps/web && grep -rn "x" node_modules/foo', ['node_modules'])).toBe( - true, - ); - }); -}); - -describe('firstAdvisablePattern — exclusion and pattern-extraction stay correlated (guard-review finding, round 8)', () => { - const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; - - it("does NOT advise on an excluded invocation's pattern just because a LATER invocation has an in-scope target", () => { - // isExcludedTarget aggregates across every invocation (any-in-scope-wins, correct for the - // counter's "is there real search activity" question) — but the GUARD attributes a specific - // pattern to advise on, and that pattern must come from the SAME invocation whose target was - // checked. Without this, `grep "auth flow logic" node_modules && grep "y" src` would advise - // on "auth flow logic" (the node_modules-excluded invocation's pattern) merely because the - // unrelated second invocation's target ("src") isn't excluded. - expect( - firstAdvisablePattern( - 'grep -rn "auth flow logic" node_modules && grep -rn "y" src', - EXCLUDE_ROOTS, - ), - ).toBe('y'); - }); - - it('skips a single excluded invocation entirely, same as isExcludedTarget', () => { - expect( - firstAdvisablePattern('grep -rn "auth flow" node_modules/foo', EXCLUDE_ROOTS), - ).toBeNull(); - }); - - it('returns the pattern unchanged when nothing is excluded', () => { - expect(firstAdvisablePattern('grep -rn "auth flow" src/', EXCLUDE_ROOTS)).toBe('auth flow'); - }); -}); - -describe('Windows-style backslash paths (sc-1359 follow-up — OS coverage)', () => { - it('a backslash-separated node_modules target is excluded, same as forward-slash', () => { - expect( - isExcludedTarget('grep -rn "x" C:\\project\\node_modules\\foo.js', [ - 'node_modules', - '.git', - '/tmp', - ]), - ).toBe(true); - }); - - it('a backslash-separated scanRoot target is recognized as in-scope', () => { - expect( - isOutOfScanRoots('grep -rn "x" C:\\project\\gate-engine\\lib.mts', ['gate-engine']), - ).toBe(false); - }); - - it('an absolute Windows temp-dir root (backslash, no trailing slash) excludes its own subpaths', () => { - expect( - isExcludedTarget('grep -rn "x" C:\\Users\\dev\\AppData\\Local\\Temp\\out.log', [ - 'node_modules', - 'C:\\Users\\dev\\AppData\\Local\\Temp', - ]), - ).toBe(true); - }); -}); - -describe('find / fd target detection (guard-review finding — isPrimarySearchCommand also treats these as searches)', () => { - const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; - - it('a `find` invocation into an excluded root is recognized (path comes before any flag)', () => { - expect(isExcludedTarget('find node_modules -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(true); - }); - - it('a flag VALUE in a `find` expression is never mistaken for a second target', () => { - // -iname's value ("*.spec.ts") must not count as a non-excluded target that saves the command - // via any-in-scope-wins — only the leading path run (before the first flag) is a target. - expect(isExcludedTarget('find node_modules -iname "*.ts" -type f', EXCLUDE_ROOTS)).toBe(true); - }); - - it('a `find .` (cwd) is NOT excluded', () => { - expect(isExcludedTarget('find . -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(false); - }); - - it('an `fd` invocation (PATTERN [PATH...] convention, like grep) into an excluded root is recognized', () => { - expect(isExcludedTarget('fd "\\.spec\\.ts$" node_modules', EXCLUDE_ROOTS)).toBe(true); - }); - - it('an `fd` invocation with an in-scope path is NOT excluded', () => { - expect(isExcludedTarget('fd "\\.spec\\.ts$" src/', EXCLUDE_ROOTS)).toBe(false); - }); -}); - describe('extractPattern — multi-line compound commands (sc-1359 follow-up)', () => { it("does NOT bleed a NEXT LINE's command into an earlier operand-bearing grep", () => { expect( @@ -444,11 +254,15 @@ describe('extractPattern — multi-line compound commands (sc-1359 follow-up)', describe('extractPattern / splitUnquotedSegments — crash safety on malformed shell quoting', () => { it('an unterminated double quote does not throw and returns a graceful result', () => { + // The unclosed quote runs to end-of-string, so the whole remainder becomes one token — + // not ideal, but deterministic and never a crash. expect(() => extractPattern('grep -rn "unterminated src/')).not.toThrow(); + expect(extractPattern('grep -rn "unterminated src/')).toBe('unterminated src/'); }); - it('an unterminated single quote does not throw and returns a graceful result', () => { + it('an unterminated single quote does not throw and returns a graceful result (same fallback as double)', () => { expect(() => extractPattern("grep -rn 'unterminated src/")).not.toThrow(); + expect(extractPattern("grep -rn 'unterminated src/")).toBe('unterminated src/'); }); it('an escaped quote WITHIN the pattern itself is preserved, not treated as a terminator', () => { diff --git a/gate-engine/search-tool/__tests__/search-tool-targets.test.mts b/gate-engine/search-tool/__tests__/search-tool-targets.test.mts new file mode 100644 index 00000000..3de3e7d0 --- /dev/null +++ b/gate-engine/search-tool/__tests__/search-tool-targets.test.mts @@ -0,0 +1,253 @@ +import { describe, expect, it } from 'vitest'; +import { firstAdvisablePattern, isExcludedTarget, isOutOfScanRoots } from '../search-tool-lib.mts'; + +// Unit tests for search-tool-lib.mts's out-of-index TARGET detection +// (isExcludedTarget / isOutOfScanRoots / firstAdvisablePattern) — split out +// from search-tool-lib.test.mts (which covers extractPattern / classify) +// once that file grew past the size ratchet. Pure string classifiers — +// provider-agnostic (Cursor vs Claude run the same Bash strings) so there +// are no provider-specific cases. The one OS-relevant case (Windows +// backslash paths) lives in its own describe block below. + +describe('isExcludedTarget — target outside the ecosystem-universal exclude roots (sc-1359 #3)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it('true for a node_modules target', () => { + expect(isExcludedTarget('grep -rn "TODO" node_modules/foo/lib.js', EXCLUDE_ROOTS)).toBe(true); + }); + + it('true for a /tmp target', () => { + expect(isExcludedTarget('grep -oE "FAIL +[^ ]+" /tmp/out.log', EXCLUDE_ROOTS)).toBe(true); + }); + + it('FALSE when there is no explicit path operand (bare grep searches cwd)', () => { + expect(isExcludedTarget('grep -rn "the auth flow"', EXCLUDE_ROOTS)).toBe(false); + }); + + it('FALSE for a multi-target grep with at least one non-excluded target', () => { + expect(isExcludedTarget('grep -rn "the auth flow" src/ node_modules/', EXCLUDE_ROOTS)).toBe( + false, + ); + }); + + it('FALSE for an ordinary source target', () => { + expect(isExcludedTarget('grep -rn "the auth flow" src/', EXCLUDE_ROOTS)).toBe(false); + }); + + it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { + expect(isExcludedTarget('grep -rn "the auth flow" .', EXCLUDE_ROOTS)).toBe(false); + expect(isExcludedTarget('grep -rn "the auth flow" ..', EXCLUDE_ROOTS)).toBe(false); + }); + + it('FALSE for a MIXED excluded-root + bare "." target — the "." keeps it in scope (PR review finding, round 2)', () => { + // grep also searches "." (all of cwd, including real source) alongside node_modules — the + // presence of "." must not be silently dropped before the "every target excluded" check runs. + expect(isExcludedTarget('grep -rn "the auth flow" node_modules .', EXCLUDE_ROOTS)).toBe(false); + }); +}); + +describe("isOutOfScanRoots — target outside the consumer's configured scanRoots", () => { + const SCAN_ROOTS = ['cli', 'gate-engine']; + + it('FALSE when the target is inside a configured scanRoot', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" gate-engine/', SCAN_ROOTS)).toBe(false); + }); + + it('true when the target is outside every configured scanRoot', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', SCAN_ROOTS)).toBe(true); + }); + + it('FALSE when there is no explicit path operand', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow"', SCAN_ROOTS)).toBe(false); + }); + + it('FALSE when scanRoots is empty (never match-nothing)', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" docs/', [])).toBe(false); + }); + + it('FALSE for a bare "." or ".." target — same as no operand (guard-review finding, round 9)', () => { + // `grep -r "x" .` is an extremely common shape (search cwd). Without this, the counter's + // out-of-scan-roots no-op fires on it whenever the consumer's scanRoots don't happen to + // literally include "." — silently defeating streak counting for the most common invocation. + expect(isOutOfScanRoots('grep -rn "the auth flow" .', SCAN_ROOTS)).toBe(false); + expect(isOutOfScanRoots('grep -rn "the auth flow" ..', SCAN_ROOTS)).toBe(false); + }); + + it('FALSE for a MIXED out-of-scanRoots + bare "." target — the "." keeps it in scope (PR review finding, round 2)', () => { + expect(isOutOfScanRoots('grep -rn "the auth flow" docs .', SCAN_ROOTS)).toBe(false); + }); +}); + +describe('root matching — boundary safety (prefix, not substring)', () => { + it('a sibling dir that merely shares a string PREFIX with an exclude root is NOT excluded', () => { + // node_modules_shim/ is a real, distinct directory — must not be swallowed by "node_modules". + expect( + isExcludedTarget('grep -rn "x" node_modules_shim/foo.js', ['node_modules', '.git', '/tmp']), + ).toBe(false); + }); + + it('a sibling dir that merely shares a string PREFIX with a scanRoot is NOT treated as in-scope', () => { + // src-legacy/ is a real, distinct directory from the configured "src" scanRoot. + expect(isOutOfScanRoots('grep -rn "x" src-legacy/foo.ts', ['src'])).toBe(true); + }); + + it('an exact-match token (no subpath) still matches its root', () => { + expect(isExcludedTarget('grep -rn "x" node_modules', ['node_modules'])).toBe(true); + expect(isOutOfScanRoots('grep -rn "x" src', ['src'])).toBe(false); + }); + + it("a space-separated value flag's value is never misread as a target (guard-review finding)", () => { + // -A's value ("3") and the real pattern ("cli") must not leak into the target list — only + // the actual trailing path (node_modules/foo.js) is a target, so this must be excluded. + // Regression-critical: "cli" happens to be a real scanRoot in this repo's own guard.config.json, + // so a wrongly-collected "cli" target would silently defeat the any-in-scope-wins exclusion. + expect(isExcludedTarget('grep -A 3 cli node_modules/foo.js', ['node_modules'])).toBe(true); + }); + + it('a SECOND -e pattern value is never misread as a target (guard-review finding, round 7)', () => { + // `grep -e P1 -e P2` (multi-pattern OR search) has no file/dir operand at all — every value + // after -e is a PATTERN, not a target, however many -e flags are used. Without an explicit + // target, this must never be excluded (matches "no operand searches cwd" semantics). + expect(isExcludedTarget('grep -e "the auth flow" -e "node_modules"', ['node_modules'])).toBe( + false, + ); + }); + + it("an unrelated command's argument earlier in a compound command is NOT read as a grep target", () => { + // A non-leading `cd` to an in-scope dir must not "pollute" a later, unrelated grep's own + // out-of-scope target via any-in-scope-wins — targets are scoped per grep invocation, not + // pulled from the whole command string. + expect(isExcludedTarget('cd apps/web && grep -rn "x" node_modules/foo', ['node_modules'])).toBe( + true, + ); + }); +}); + +describe('firstAdvisablePattern — exclusion and pattern-extraction stay correlated (guard-review finding, round 8)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it("does NOT advise on an excluded invocation's pattern just because a LATER invocation has an in-scope target", () => { + // isExcludedTarget aggregates across every invocation (any-in-scope-wins, correct for the + // counter's "is there real search activity" question) — but the GUARD attributes a specific + // pattern to advise on, and that pattern must come from the SAME invocation whose target was + // checked. Without this, `grep "auth flow logic" node_modules && grep "y" src` would advise + // on "auth flow logic" (the node_modules-excluded invocation's pattern) merely because the + // unrelated second invocation's target ("src") isn't excluded. + expect( + firstAdvisablePattern( + 'grep -rn "auth flow logic" node_modules && grep -rn "y" src', + EXCLUDE_ROOTS, + [], + ), + ).toBe('y'); + }); + + it('skips a single excluded invocation entirely, same as isExcludedTarget', () => { + expect( + firstAdvisablePattern('grep -rn "auth flow" node_modules/foo', EXCLUDE_ROOTS, []), + ).toBeNull(); + }); + + it('returns the pattern unchanged when nothing is excluded', () => { + expect(firstAdvisablePattern('grep -rn "auth flow" src/', EXCLUDE_ROOTS, [])).toBe('auth flow'); + }); + + it('skips an invocation whose target is outside the configured scanRoots, not just EXCLUDE_ROOTS (PR review finding)', () => { + // firstAdvisablePattern previously only checked the universal EXCLUDE_ROOTS + // (node_modules/.git/tmp) — the consumer's configured scanRoots must be checked too, per + // invocation, for the SAME reason: a compound command's advice must come from an invocation + // whose own target is actually answerable by the semantic-search tool. + expect(firstAdvisablePattern('grep -rn "auth flow" docs/', EXCLUDE_ROOTS, ['src'])).toBeNull(); + }); + + it('in a compound command, skips the out-of-scanRoots invocation and selects the in-scope one', () => { + expect( + firstAdvisablePattern( + 'grep -rn "auth flow logic" docs/ && grep -rn "the retry backoff path" src/', + EXCLUDE_ROOTS, + ['src'], + ), + ).toBe('the retry backoff path'); + }); + + it('scanRoots empty (unconfigured) never excludes anything — conservative fallback', () => { + expect(firstAdvisablePattern('grep -rn "auth flow" docs/', EXCLUDE_ROOTS, [])).toBe( + 'auth flow', + ); + }); + + it('a MIXED excluded-root + bare "." target stays advisable — the "." keeps it in scope (PR review finding, round 2)', () => { + // grep also searches "." (all of cwd, including real source) alongside node_modules — a + // pre-filter that strips "." before the exclusion check runs would misclassify this as fully + // excluded and silence a genuinely conceptual query. + expect( + firstAdvisablePattern( + 'grep -rn "how does the auth flow work" node_modules .', + EXCLUDE_ROOTS, + [], + ), + ).toBe('how does the auth flow work'); + }); + + it('a MIXED out-of-scanRoots + bare "." target stays advisable (PR review finding, round 2)', () => { + expect( + firstAdvisablePattern('grep -rn "how does the auth flow work" docs .', EXCLUDE_ROOTS, [ + 'cli', + 'gate-engine', + ]), + ).toBe('how does the auth flow work'); + }); +}); + +describe('Windows-style backslash paths (sc-1359 follow-up — OS coverage)', () => { + it('a backslash-separated node_modules target is excluded, same as forward-slash', () => { + expect( + isExcludedTarget('grep -rn "x" C:\\project\\node_modules\\foo.js', [ + 'node_modules', + '.git', + '/tmp', + ]), + ).toBe(true); + }); + + it('a backslash-separated scanRoot target is recognized as in-scope', () => { + expect( + isOutOfScanRoots('grep -rn "x" C:\\project\\gate-engine\\lib.mts', ['gate-engine']), + ).toBe(false); + }); + + it('an absolute Windows temp-dir root (backslash, no trailing slash) excludes its own subpaths', () => { + expect( + isExcludedTarget('grep -rn "x" C:\\Users\\dev\\AppData\\Local\\Temp\\out.log', [ + 'node_modules', + 'C:\\Users\\dev\\AppData\\Local\\Temp', + ]), + ).toBe(true); + }); +}); + +describe('find / fd target detection (guard-review finding — isPrimarySearchCommand also treats these as searches)', () => { + const EXCLUDE_ROOTS = ['node_modules', '.git', '/tmp']; + + it('a `find` invocation into an excluded root is recognized (path comes before any flag)', () => { + expect(isExcludedTarget('find node_modules -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(true); + }); + + it('a flag VALUE in a `find` expression is never mistaken for a second target', () => { + // -iname's value ("*.spec.ts") must not count as a non-excluded target that saves the command + // via any-in-scope-wins — only the leading path run (before the first flag) is a target. + expect(isExcludedTarget('find node_modules -iname "*.ts" -type f', EXCLUDE_ROOTS)).toBe(true); + }); + + it('a `find .` (cwd) is NOT excluded', () => { + expect(isExcludedTarget('find . -iname "*.spec.ts"', EXCLUDE_ROOTS)).toBe(false); + }); + + it('an `fd` invocation (PATTERN [PATH...] convention, like grep) into an excluded root is recognized', () => { + expect(isExcludedTarget('fd "\\.spec\\.ts$" node_modules', EXCLUDE_ROOTS)).toBe(true); + }); + + it('an `fd` invocation with an in-scope path is NOT excluded', () => { + expect(isExcludedTarget('fd "\\.spec\\.ts$" src/', EXCLUDE_ROOTS)).toBe(false); + }); +}); diff --git a/gate-engine/search-tool/eval/eval.mts b/gate-engine/search-tool/eval/eval.mts index 31b630ea..f7009e0c 100644 --- a/gate-engine/search-tool/eval/eval.mts +++ b/gate-engine/search-tool/eval/eval.mts @@ -22,6 +22,7 @@ import { spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { resolveGuardConfig } from '../../config.mts'; const here = dirname(fileURLToPath(import.meta.url)); const SELF_EXT = import.meta.url.endsWith('.mts') ? '.mts' : '.mjs'; @@ -31,10 +32,19 @@ const queriesPath = resolve(here, 'queries.json'); const { queries } = JSON.parse(readFileSync(queriesPath, 'utf8')); const failOnRegression = process.argv.includes('--fail'); +// The guard now scopes advice to the consumer's configured scanRoots (see +// firstAdvisablePattern) — a hardcoded `src/` target would silently score +// every query as "not flagged" on any consumer whose scanRoots don't +// include `src` (e.g. devkit's own `["cli","gate-engine"]`), since the +// spawned guard subprocess below resolves ITS OWN scanRoots from the SAME +// cwd this script runs in. Deriving the target the same way keeps the two +// in sync for any consumer. +const evalScanRoot = resolveGuardConfig().scanRoots[0] ?? 'src'; + const results = queries.map((q) => { const expectedFlag = q.expected_tool !== 'grep' && q.expected_tool !== 'find_glob'; // Build a plausible bash command for this pattern. - const cmd = `grep -rn "${q.pattern}" src/`; + const cmd = `grep -rn "${q.pattern}" ${evalScanRoot}/`; const proc = spawnSync('node', [guard], { input: JSON.stringify({ tool_input: { command: cmd } }), encoding: 'utf8', diff --git a/gate-engine/search-tool/search-tool-guard.mts b/gate-engine/search-tool/search-tool-guard.mts index 8383bbba..6956cc84 100644 --- a/gate-engine/search-tool/search-tool-guard.mts +++ b/gate-engine/search-tool/search-tool-guard.mts @@ -76,20 +76,22 @@ if (!hasCommandSearch(cmd)) process.exit(0); // Extract the user-facing pattern, skipping any invocation (in a compound // command) whose OWN target is outside anywhere the semantic index could -// cover (node_modules, /tmp, .git) — the steered tool cannot answer that -// invocation's query regardless of how its pattern classifies. This must -// stay correlated per-invocation, not a separate whole-command exclusion -// check followed by a separate "first pattern" extraction — the two can -// disagree on WHICH invocation they're each looking at (guard-review +// cover — the ecosystem-universal exclude roots (node_modules, /tmp, .git) +// or the consumer's configured scanRoots — since the steered tool cannot +// answer that invocation's query regardless of how its pattern classifies. +// This must stay correlated per-invocation, not a separate whole-command +// exclusion check followed by a separate "first pattern" extraction — the +// two can disagree on WHICH invocation they're each looking at (guard-review // finding, sc-1359 follow-up round 8). -const pattern = firstAdvisablePattern(cmd, EXCLUDE_ROOTS); +const guardConfig = resolveGuardConfig(); +const pattern = firstAdvisablePattern(cmd, EXCLUDE_ROOTS, guardConfig.scanRoots); if (!pattern) process.exit(0); const classification = classify(pattern); if (classification.verdict === 'literal') process.exit(0); -const tools = resolveSearchTools(resolveGuardConfig()); +const tools = resolveSearchTools(guardConfig); const advice = buildAdvice(classification, pattern, tools); if (MODE === 'block' && classification.verdict === 'conceptual_high') { diff --git a/gate-engine/search-tool/search-tool-lib.mts b/gate-engine/search-tool/search-tool-lib.mts index c5bbf432..b2335c88 100644 --- a/gate-engine/search-tool/search-tool-lib.mts +++ b/gate-engine/search-tool/search-tool-lib.mts @@ -52,6 +52,18 @@ const RE_GREP_SCOPE = /(?:^|\s|\$\()(grep|rg|ripgrep|ack|ag|fd)(?=\s|$)([\s\S]*) const RE_FIND_SCOPE = /(?:^|\s|\$\()find(?=\s|$)([\s\S]*)/; const RE_WHITESPACE = /\s+/; +// A bare `.`/`./`/`..`/`../` target is a cwd/parent-dir reference, not a +// real scoped path — searching it covers everything the configured roots +// would too, so its PRESENCE must keep an invocation in scope (see +// allTargetsMissEveryRoot, the only check this matters for — guard-review +// finding, sc-1359 follow-up round 9, refined in round 2 of the PR-review +// follow-up: this must be a per-element check inside a match predicate, not +// a pre-filter that strips cwd-refs out of a target array, which breaks a +// MIXED target list like `grep ... node_modules .`). A real relative +// sub-path like `./src` is unaffected — this only matches the BARE +// reference, nothing trailing it. +const RE_CWD_REF = /^\.\.?\/?$/; + // --- classify --- const RE_HAS_WHITESPACE = /\s/; const RE_PATH_PREFIX = /^[~/]/; @@ -178,11 +190,13 @@ function grepInvocations(cmd: string): { pattern: string | null; targets: string const match = matchUnquoted(segment, RE_GREP_SCOPE); if (!match) continue; const { patternValues, positionals } = classifyOperands(tokenizeArgv(match[2]), match[1]); - invocations.push( - patternValues.length - ? { pattern: patternValues[0], targets: positionals } - : { pattern: positionals[0] ?? null, targets: positionals.slice(1) }, - ); + // NOT filtering bare cwd-refs (`.`/`..`) out of targets here — see + // allTargetsMissEveryRoot's doc comment for why a pre-filter is wrong + // for a MIXED target list. + invocations.push({ + pattern: patternValues.length ? patternValues[0] : (positionals[0] ?? null), + targets: patternValues.length ? positionals : positionals.slice(1), + }); } return invocations; } @@ -204,25 +218,33 @@ export function extractPattern(c: string): string | null { /** * Like extractPattern, but — for a compound command with more than one * grep-family invocation — skips any invocation whose OWN targets are - * entirely inside `excludeRoots`, so its pattern is never advised on merely - * because a LATER, unrelated invocation elsewhere in the same command has a - * non-excluded target. Guard-only: isExcludedTarget's whole-command - * aggregate (any-in-scope-wins) stays correct for the counter, which only - * asks "is there real search activity happening anywhere in this command", - * not "which specific pattern should I advise on" — the guard is the one - * that attributes a pattern, so it's the one that needs this per-invocation - * correlation (guard-review finding, sc-1359 follow-up round 8: `grep "auth - * flow logic" node_modules && grep "y" src` used to advise on "auth flow - * logic" — the excluded invocation's pattern — because isExcludedTarget and - * extractPattern picked their answers from two different invocations). + * entirely inside `excludeRoots` OR entirely outside `scanRoots`, so its + * pattern is never advised on merely because a LATER, unrelated invocation + * elsewhere in the same command has an answerable target. This must stay a + * PER-INVOCATION check, not a separate whole-command aggregate (isExcludedTarget + * / isOutOfScanRoots) followed by a separate "first pattern" extraction — the + * two can disagree on WHICH invocation they're each looking at (guard-review + * finding, sc-1359 follow-up round 8: `grep "auth flow logic" node_modules && + * grep "y" src` used to advise on "auth flow logic" — the excluded + * invocation's pattern — because isExcludedTarget and extractPattern picked + * their answers from two different invocations). scanRoots scoping was added + * later (PR review finding) once this per-invocation correlation existed to + * carry it correctly — the whole-command isOutOfScanRoots was deliberately + * NOT wired into the guard earlier because eval/eval.mts's synthetic `src/` + * targets would have gone silently unscored whenever a consumer's scanRoots + * don't include `src` (e.g. devkit's own `["cli","gate-engine"]`); eval.mts + * now derives its target from the resolved scanRoots instead of hardcoding + * `src/`, so that's no longer a blocker. */ -export function firstAdvisablePattern(cmd: string, excludeRoots: string[]): string | null { +export function firstAdvisablePattern( + cmd: string, + excludeRoots: string[], + scanRoots: string[], +): string | null { for (const inv of grepInvocations(cmd)) { if (inv.pattern === null) continue; - const excluded = - inv.targets.length > 0 && - inv.targets.every((t) => excludeRoots.some((r) => matchesRoot(t, r))); - if (excluded) continue; + if (allTargetsMatchSomeRoot(inv.targets, excludeRoots)) continue; + if (allTargetsMissEveryRoot(inv.targets, scanRoots)) continue; return inv.pattern; } return null; @@ -248,29 +270,15 @@ function findInvocationTargets(cmd: string): string[] { return targets; } -// A bare `.`/`./`/`..`/`../` target is a cwd/parent-dir reference, not a -// real scoped path — it carries no information about which directory -// hierarchy is being searched, so it must be treated exactly like "no -// operand" (never excluded, never out-of-scanRoots). Without this filter, -// `matchesRoot('.', root)` is false for every root (it isn't literally -// equal to, nor does it start with, any configured root), which -// isOutOfScanRoots' inverted "every target fails to match" check reads as -// "every target is out of scope" — silently no-op'ing the counter on the -// extremely common `grep -r "x" .` shape (guard-review finding, sc-1359 -// follow-up round 9). A real relative sub-path like `./src` is unaffected — -// this only matches the BARE reference, nothing trailing it. -const RE_CWD_REF = /^\.\.?\/?$/; - // Every non-pattern target across all grep/fd invocations (see // grepInvocations), plus every leading path token across all find // invocations — candidate targets. Deliberately NOT filtered by "contains a // slash": a bare target with no subpath (`grep -rn "x" node_modules`, `find -// node_modules`) is a normal, common shape and must still be seen. +// node_modules`) is a normal, common shape and must still be seen. Also NOT +// filtered by cwd-ref (see allTargetsMissEveryRoot's doc comment) — a bare +// `.`/`..` alongside another target must stay visible to that check. function targetTokens(cmd: string): string[] { - return [ - ...grepInvocations(cmd).flatMap((inv) => inv.targets), - ...findInvocationTargets(cmd), - ].filter((t) => !RE_CWD_REF.test(t)); + return [...grepInvocations(cmd).flatMap((inv) => inv.targets), ...findInvocationTargets(cmd)]; } // Windows paths can appear literally in a bash command string on Windows @@ -294,6 +302,41 @@ function matchesRoot(token: string, root: string): boolean { return t === r || t.startsWith(`${r}/`) || t.includes(`/${r}/`); } +// Do ALL of `targets` match SOME root in `roots`? Empty targets => false (no +// operand means "searches cwd", never excluded/out-of-scope). Shared by +// isExcludedTarget (whole-command) and firstAdvisablePattern (per-invocation) +// so the two can't drift on what "excluded" means. +function allTargetsMatchSomeRoot(targets: string[], roots: string[]): boolean { + return targets.length > 0 && targets.every((t) => roots.some((r) => matchesRoot(t, r))); +} + +// Do ALL of `targets` fail to match EVERY root in `roots`? Empty roots => +// false (conservative fallback — an unconfigured scanRoots never excludes +// anything, never "match-nothing"). Shared by isOutOfScanRoots (whole- +// command) and firstAdvisablePattern (per-invocation). +// +// A bare cwd-ref target (`.`/`./`/`..`/`../`) NEVER counts as "missing" — +// it means "search all of cwd", which by definition includes whatever the +// configured roots are, so its mere PRESENCE is enough to keep an +// invocation in scope even alongside another, genuinely out-of-scope +// target. This must be a per-element exception INSIDE the .every(), not a +// pre-filter that removes cwd-refs from the target array before this runs +// (PR review finding, sc-1359 follow-up round 2: an earlier version +// filtered cwd-refs out of grepInvocations'/targetTokens' target arrays +// directly — for a single bare `.` that correctly left an empty array +// (never excluded, via the `targets.length > 0` guard), but for a MIXED +// list like `grep ... node_modules .`, removing `.` left only +// `['node_modules']`, which then matched every remaining root and was +// wrongly classified as fully excluded — silently losing the exact signal +// the `.` was supposed to provide). +function allTargetsMissEveryRoot(targets: string[], roots: string[]): boolean { + return ( + roots.length > 0 && + targets.length > 0 && + targets.every((t) => !RE_CWD_REF.test(t) && !roots.some((r) => matchesRoot(t, r))) + ); +} + /** * Is every target in `cmd` inside one of the ecosystem-universal excluded * roots (node_modules, .git, the OS temp dir — never a hardcoded stack @@ -303,25 +346,17 @@ function matchesRoot(token: string, root: string): boolean { * in-scope target keeps the command worth advising on). */ export function isExcludedTarget(cmd: string, excludeRoots: string[]): boolean { - const targets = targetTokens(cmd); - if (!targets.length) return false; - return targets.every((t) => excludeRoots.some((root) => matchesRoot(t, root))); + return allTargetsMatchSomeRoot(targetTokens(cmd), excludeRoots); } /** * Is every target in `cmd` OUTSIDE all of the consumer's `scanRoots`? * Mirrors isExcludedTarget's "no operand / any-in-scope-wins" semantics, * scoped to the consumer's configured roots (resolveGuardConfig) instead of - * the universal exclude list. Counter-only (see search-tool-guard / - * search-tool-counter): applying this to the guard would silence it on - * eval/eval.mts's synthetic `src/` targets whenever a consumer's scanRoots - * don't include `src` (e.g. devkit's own `["cli","gate-engine"]`). + * the universal exclude list. */ export function isOutOfScanRoots(cmd: string, scanRoots: string[]): boolean { - if (!scanRoots.length) return false; - const targets = targetTokens(cmd); - if (!targets.length) return false; - return targets.every((t) => !scanRoots.some((root) => matchesRoot(t, root))); + return allTargetsMissEveryRoot(targetTokens(cmd), scanRoots); } /**