From ea03accc978403a621f23857dff7bb7d8bc9839e Mon Sep 17 00:00:00 2001 From: Alexander Goodkind Date: Sun, 26 Jul 2026 20:35:16 +0200 Subject: [PATCH] Read only shell as shell in the recursive-enumeration scan The enumerator and recursive-structure layers split text into shell fields, so an embedded program's source was read as shell. A ruby body yielded the directory `Dir.glob "/abs/lmd`, and a `Dir[...]` index was mistaken for a glob wildcard, which resolved the base to cwd and displaced the directory the program actually reads. Blank each foreign-language region before those layers run, keeping shell and opaque regions visible because the enumerator needs the searcher inside `xargs grep` and `find -exec grep`. Drop nonPathOperandChars, whose character test rejected real directory names containing a parenthesis and removed the only layer that names that directory for a command whose argv0 is not a declared searcher. Return immediately when an errored target already blocks under a fail-closed rule, since no later target can change that verdict and probing the rest holds the singleflight entry for one background timeout per remaining target. Log a block that follows an unclassified target, which the returned clean verdict would otherwise hide from the evaluation record. Record in regionFoldTools that ruby and javascript parse but expose no read targets, so declaring them in search_tools buys no coverage. Assert the reached target rather than the validator invocation count, so the no-veto test stops depending on target ordering. Move runExpandedCommands and logExpandedCommandError to exec_expand.go to keep exec_gate.go under the file-length limit. Co-authored-by: Claude --- .../rules/concerns/shellread/codesearch.go | 85 +++++++++++- .../concerns/shellread/codesearch_embedded.go | 15 +++ .../concerns/shellread/codesearch_enum.go | 26 ---- .../shellread/codesearch_glob_operand_test.go | 109 +++++++++++++--- internal/rules/exec_expand.go | 121 ++++++++++++++++++ internal/rules/exec_gate.go | 83 ------------ .../rules/exec_gate_errored_target_test.go | 92 ++++++++++++- 7 files changed, 396 insertions(+), 135 deletions(-) create mode 100644 internal/rules/exec_expand.go diff --git a/internal/rules/concerns/shellread/codesearch.go b/internal/rules/concerns/shellread/codesearch.go index cb3255c..d993c11 100644 --- a/internal/rules/concerns/shellread/codesearch.go +++ b/internal/rules/concerns/shellread/codesearch.go @@ -2,6 +2,7 @@ package shellread import ( "os" + "strings" "goodkind.io/agent-gate/internal/rules/concerns/shellparse" "goodkind.io/gksyntax/shelldecomp" @@ -93,12 +94,22 @@ func extractCodeSearchInto(command, cwd, home string, tools map[string]bool, add add(target.Path) } + // The enumerator and recursive-structure layers below split text into shell + // fields, so they must only see shell. An embedded program's source is not + // shell, and reading it as shell fabricates directories out of call syntax + // (a ruby body yields the directory `Dir.glob "/abs/lmd`) or resolves a glob + // base to cwd when a language construct like Dir[...] is mistaken for a + // wildcard. Each embedded region is handled on its own terms further down, + // by its analyzer's reads or by a recursive shell scan of an opaque body, so + // blanking the regions here loses nothing those layers already cover. + shellText := maskEmbeddedRegions(command, decomposition.EmbeddedRegions()) + // Enumerator-driven code search (find/fd/git ls-files feeding a declared // tool over file contents). shelldecomp surfaces find's own operands as // reads, which over- and under-count the enumerated directory, so the // enumerator layer computes the real target and the find/fd/git-ls-files // reads above are skipped by the declared-tools filter. - for _, target := range resolvableTargets(enumeratorCodeSearchTargets(command, cwd, tools)) { + for _, target := range resolvableTargets(enumeratorCodeSearchTargets(shellText, cwd, tools)) { add(target.Path) } @@ -106,7 +117,7 @@ func extractCodeSearchInto(command, cwd, home string, tools map[string]bool, add // without a shallow -maxdepth, git ls-files, a recursive ** glob). shelldecomp // models these as filename enumeration, not a content read, so the directory // they walk is computed here and left for the index-aware validator to judge. - for _, target := range resolvableTargets(recursiveEnumerationTargets(command, cwd)) { + for _, target := range resolvableTargets(recursiveEnumerationTargets(shellText, cwd)) { add(target.Path) } @@ -123,6 +134,76 @@ func parseCommand(command, cwd, home string, resolver shelldecomp.FileResolver) return shelldecomp.ParseWithOptions(command, cwd, shelldecomp.Options{Home: home, FileResolver: resolver}) } +// maskEmbeddedRegions returns command with each foreign-language embedded +// region's source blanked to spaces, so a shell-field scan of the result sees +// only shell. Blanking preserves length, byte offsets, and field boundaries, so +// the surrounding shell splits exactly as it did before. +// +// A region reports a byte span for a heredoc body but reports 0..0 for an +// interpreter -c or -e operand, so the span is used when it is present and +// usable and the region's text is located in the command otherwise. A region +// whose text cannot be located is left alone rather than guessed at. +func maskEmbeddedRegions(command string, regions []shelldecomp.EmbeddedRegion) string { + if len(regions) == 0 { + return command + } + masked := []byte(command) + for _, region := range regions { + if !isForeignLanguageRegion(region.Lang) { + continue + } + start, end, ok := regionSpan(command, region) + if !ok { + continue + } + for index := start; index < end; index++ { + masked[index] = ' ' + } + } + return string(masked) +} + +// isForeignLanguageRegion reports whether a region's body is a language other +// than shell, and therefore must not be read as shell fields. +// +// A shell region stays visible because reading shell as shell is correct and +// because the enumerator layer depends on it: `find DIR | xargs grep` and +// `find DIR -exec grep` both carry the searcher as a nested shell region, and +// blanking it would hide the searcher that makes the enumerated directory a +// content-search target. An opaque region stays visible because it has no +// grammar, so the recursive shell scan of its body is the only reading +// available and the outer scan is a backstop for when the depth budget runs +// out. Every other language is program source with its own analyzer. +func isForeignLanguageRegion(lang shelldecomp.Lang) bool { + switch lang { + case shelldecomp.LangPython, shelldecomp.LangJavaScript, shelldecomp.LangRuby, + shelldecomp.LangPerl, shelldecomp.LangAppleScript, shelldecomp.LangSQL, + shelldecomp.LangAwk, shelldecomp.LangJQ, shelldecomp.LangSed, + shelldecomp.LangRegex: + return true + case shelldecomp.LangShell, shelldecomp.LangOpaque, shelldecomp.LangUnknown: + return false + default: + return false + } +} + +// regionSpan returns the byte range of one embedded region within command, and +// whether a usable range was found. +func regionSpan(command string, region shelldecomp.EmbeddedRegion) (int, int, bool) { + if region.EndByte > region.StartByte && int(region.EndByte) <= len(command) { + return int(region.StartByte), int(region.EndByte), true + } + if region.Text == "" { + return 0, 0, false + } + offset := strings.Index(command, region.Text) + if offset < 0 { + return 0, 0, false + } + return offset, offset + len(region.Text), true +} + // resolvableTargets drops enumerator operands whose path still carries a shell // expansion (a $variable or `command` substitution) that agent-gate cannot // evaluate. The enumerator layer resolves directories with the package's own diff --git a/internal/rules/concerns/shellread/codesearch_embedded.go b/internal/rules/concerns/shellread/codesearch_embedded.go index 176079a..90a75a1 100644 --- a/internal/rules/concerns/shellread/codesearch_embedded.go +++ b/internal/rules/concerns/shellread/codesearch_embedded.go @@ -71,6 +71,21 @@ func extractEmbeddedCodeSearchInto(decomposition *shelldecomp.Decomposition, cwd // language whose analyzer is registered in shelldecomp (python, awk) appears // here; a region whose language is absent folds nothing, so adding a new // analyzer to shelldecomp without a matching entry leaves it inert here. +// +// Two limits decide how much of an interpreter body a rule actually sees, and +// both fail quiet rather than loud. +// +// A rule that does not declare the language's tools folds nothing even when the +// analyzer resolved the reads, so a search_tools list of only grep-family names +// leaves every python body invisible. TestExtractCodeSearchTargetsPythonToolGate +// pins that behavior. +// +// A language with a grammar but no read analyzer parses into a non-nil Parsed +// that yields no read targets, so it folds nothing here no matter what a rule +// declares. As of this writing that covers ruby and javascript, and php parses +// to no region at all. Declaring ruby or node in search_tools therefore buys no +// coverage of a `ruby -e` or `node -e` body; closing that needs read analyzers +// in gksyntax, not config. var regionFoldTools = map[shelldecomp.Lang][]string{ shelldecomp.LangPython: {"python", "python3"}, shelldecomp.LangAwk: {"awk", "gawk"}, diff --git a/internal/rules/concerns/shellread/codesearch_enum.go b/internal/rules/concerns/shellread/codesearch_enum.go index ed46fe7..e9775ee 100644 --- a/internal/rules/concerns/shellread/codesearch_enum.go +++ b/internal/rules/concerns/shellread/codesearch_enum.go @@ -260,29 +260,6 @@ func findIsShallow(fields []string) bool { return false } -// nonPathOperandChars are characters that cannot appear in the directory part -// of a real shell glob operand but do appear when an embedded program's source -// is scanned as if it were shell. A python body quote-stripped to -// glob.glob(/repo/**/*.go, recursive=True) yields the directory -// glob.glob(/repo, which names no directory on disk. The embedded region's own -// analyzer already resolves that program's real read targets, so dropping the -// fabricated operand loses no coverage and stops a validator run against a path -// that cannot exist. -// -// The set is deliberately only the call parentheses. A directory name may -// legitimately contain a space, a comma, an equals sign, or a quote, and -// shellFields preserves those inside a quoted or escaped field, so rejecting -// them would silently drop a real search target and let a read of an indexed -// repository through. A $ or backtick operand is already dropped downstream by -// resolvableTargets, which refuses a path still carrying a shell expansion. -const nonPathOperandChars = "()" - -// isPlausiblePathOperand reports whether dir could be the directory part of a -// shell path operand. -func isPlausiblePathOperand(dir string) bool { - return !strings.ContainsAny(dir, nonPathOperandChars) -} - // recursiveGlobDirs returns the base directory of each recursive ** glob token in // the command: the literal prefix before the first wildcard, taken up to its last // path separator and resolved against cwd. A command reading files matched by a @@ -307,9 +284,6 @@ func recursiveGlobDirs(command, cwd string) []string { dir = "/" } } - if !isPlausiblePathOperand(dir) { - continue - } out = append(out, resolvePath(cwd, dir)) } return out diff --git a/internal/rules/concerns/shellread/codesearch_glob_operand_test.go b/internal/rules/concerns/shellread/codesearch_glob_operand_test.go index 6c3a6f7..f38062b 100644 --- a/internal/rules/concerns/shellread/codesearch_glob_operand_test.go +++ b/internal/rules/concerns/shellread/codesearch_glob_operand_test.go @@ -8,8 +8,8 @@ import ( // TestExtractCodeSearchTargetsPythonGlobRoot covers the glob-driven content // search an agent reaches for when it wants to read a repository without naming // a searcher. The python region's own analyzer resolves the glob root, and the -// shell-level ** scan no longer fabricates a directory out of the program's -// source text, so the enumerated root is the only target. +// shell-level ** scan no longer reads the program's source as shell fields, so +// the enumerated root is the only target. func TestExtractCodeSearchTargetsPythonGlobRoot(t *testing.T) { const cwd = "/repo" @@ -25,31 +25,74 @@ func TestExtractCodeSearchTargetsPythonGlobRoot(t *testing.T) { } } -// TestRecursiveGlobDirsRejectsProgramSourceOperands covers the operand filter -// directly: a real shell glob still resolves its base directory, while an -// operand carrying call syntax from an embedded program's source resolves -// nothing. -func TestRecursiveGlobDirsRejectsProgramSourceOperands(t *testing.T) { +// TestExtractCodeSearchTargetsForeignRegionNotReadAsShell covers the shapes a +// shell-field scan of program source fabricates: a call operand becomes a +// directory that cannot exist, and a language construct opening with a bracket +// is mistaken for a glob wildcard, which resolves the base to cwd and silently +// displaces the directory the program actually reads. Neither may contribute a +// target. +func TestExtractCodeSearchTargetsForeignRegionNotReadAsShell(t *testing.T) { const cwd = "/repo" cases := []struct { name string command string - want []string }{ - {"shell relative glob", "cat src/**/*.go", []string{"/repo/src"}}, - {"shell absolute glob", "cat /abs/pkg/**/*.go", []string{"/abs/pkg"}}, - {"shell glob at root", "cat **/*.go", []string{"/repo"}}, - {"python call operand", `glob.glob("/abs/lmd/**/*.go", recursive=True)`, nil}, - {"python keyword argument operand", `sorted(glob.iglob(root+"/**/*.go"))`, nil}, + {"ruby bracket index is not a glob", `ruby -e 'Dir["/abs/indexed/**/*.rb"].each { |f| puts File.read(f) }'`}, + {"ruby call operand is not a directory", `ruby -e 'Dir.glob "/abs/lmd/**/*.go"'`}, + {"python call operand is not a directory", `python3 -c 'import glob; [open(p).read() for p in glob.glob("/abs/lmd/**/*.go", recursive=True)]'`}, + } - // A directory name may legitimately contain these characters. Rejecting - // them would drop the only layer that names the target for a - // non-searcher command, so an indexed repository would read unblocked. - {"quoted directory with a space", `cat "/abs/My Repo/**/*.go"`, []string{"/abs/My Repo"}}, - {"escaped directory with a space", `cat /abs/My\ Repo/**/*.go`, []string{"/abs/My Repo"}}, - {"directory with a comma", "cat /abs/a,b/**/*.go", []string{"/abs/a,b"}}, - {"directory with an equals sign", "cat /abs/pkg=v1/**/*.go", []string{"/abs/pkg=v1"}}, + tools := []string{"grep", "rg", "python", "python3", "ruby"} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + for _, target := range targetPaths(ExtractCodeSearchTargets(tc.command, cwd, tools, nil)) { + if target == cwd { + t.Fatalf("%q resolved the working directory as a search target", tc.command) + } + if !isCleanAbsoluteDir(target) { + t.Fatalf("%q produced a fabricated target %q", tc.command, target) + } + } + }) + } +} + +// isCleanAbsoluteDir reports whether a resolved target looks like a path a shell +// operand could name, rather than a fragment of program source. It exists only +// so the test above can state what a fabricated target looks like. +func isCleanAbsoluteDir(target string) bool { + if target == "" || target[0] != '/' { + return false + } + for _, r := range target { + if r == '(' || r == ')' || r == '"' || r == '\'' { + return false + } + } + return true +} + +// TestRecursiveGlobDirsResolvesRealShellOperands covers the operands a real +// shell command writes, including directory names holding characters a +// character-level filter would reject. A duplicated macOS download lands at a +// name with parentheses, and dropping it removes the only layer that names that +// directory for a command whose argv0 is not a declared searcher. +func TestRecursiveGlobDirsResolvesRealShellOperands(t *testing.T) { + const cwd = "/repo" + + cases := []struct { + name string + command string + want []string + }{ + {"relative glob", "cat src/**/*.go", []string{"/repo/src"}}, + {"absolute glob", "cat /abs/pkg/**/*.go", []string{"/abs/pkg"}}, + {"glob at root", "cat **/*.go", []string{"/repo"}}, + {"parenthesized directory", `cat "/abs/My Repo (1)/**/*.go"`, []string{"/abs/My Repo (1)"}}, + {"escaped space directory", `cat /abs/My\ Repo/**/*.go`, []string{"/abs/My Repo"}}, + {"comma directory", "cat /abs/a,b/**/*.go", []string{"/abs/a,b"}}, + {"equals directory", "cat /abs/pkg=v1/**/*.go", []string{"/abs/pkg=v1"}}, } for _, tc := range cases { @@ -61,3 +104,29 @@ func TestRecursiveGlobDirsRejectsProgramSourceOperands(t *testing.T) { }) } } + +// TestMaskEmbeddedRegionsKeepsShellRegions covers the enumerator layer's +// dependency on nested shell staying visible: the searcher in `xargs rg` and in +// `find -exec grep` is carried as a nested shell region, and blanking it would +// hide the searcher that makes the enumerated directory a content-search target. +func TestMaskEmbeddedRegionsKeepsShellRegions(t *testing.T) { + const cwd = "/repo" + + cases := []struct { + name string + command string + want []string + }{ + {"xargs searcher", `fd -e swift | xargs rg toolchain`, []string{"/repo"}}, + {"find exec searcher", `find Sources -name '*.go' -exec grep -l x {} +`, []string{"/repo/Sources"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := targetPaths(ExtractCodeSearchTargets(tc.command, cwd, enumTestTools, nil)) + if !slices.Equal(got, tc.want) { + t.Fatalf("ExtractCodeSearchTargets(%q) = %v, want %v", tc.command, got, tc.want) + } + }) + } +} diff --git a/internal/rules/exec_expand.go b/internal/rules/exec_expand.go new file mode 100644 index 0000000..16f4d2b --- /dev/null +++ b/internal/rules/exec_expand.go @@ -0,0 +1,121 @@ +package rules + +import ( + "context" + + "goodkind.io/agent-gate/internal/config" + execconcern "goodkind.io/agent-gate/internal/rules/concerns/exec" +) + +// runExpandedCommands runs one validator invocation per expanded target and +// combines their verdicts into the condition's verdict. +// +// Without for_each there is exactly one command and its verdict is the answer. +// With for_each, match_mode decides how the per-target verdicts combine: "any" +// blocks as soon as one target blocks, and "all" blocks only when every target +// blocks. +func (r *ExecRuntime) runExpandedCommands( + ctx context.Context, + ruleName string, + c *config.Condition, + commands [][]string, + stdin []byte, + env []string, +) execconcern.Verdict { + if len(commands) == 0 { + return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} + } + + forEach := c.ForEachSelector().Selector != config.FieldSelectorInvalid + matchAll := forEach && c.MatchMode == config.ExecMatchAll + firstBlockMessage := "" + firstErrored := execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} + sawErrored := false + for _, command := range commands { + verdict := r.runExpandedCommandWithRetry(ctx, ruleName, c, command, stdin, env) + if !forEach { + return verdict + } + if verdict.Errored { + // Under match_mode = "any" one target the validator cannot classify + // must not veto the rest. A single unresolvable target would + // otherwise decide the whole expansion and every remaining target, + // including an indexed one that would block, is never probed. The + // error is remembered and only decides the condition when no target + // blocked. Under match_mode = "all" an errored target still returns + // immediately, because "every target matches" cannot be proven once + // one target is unknown. + if matchAll { + return verdict + } + // A fail-closed rule already blocks on this error, and no later + // target can change that, so probing the rest only buys a better + // message. It costs a validator run per remaining target, each up to + // the background timeout times retry_count, all under a context that + // cannot be cancelled and while this cache key's singleflight entry + // is held, so a validator outage would stall every concurrent event + // sharing the key. The decided verdict is worth more than the message. + if verdict.Block { + return verdict + } + if !sawErrored { + sawErrored = true + firstErrored = verdict + } + continue + } + if verdict.Block && firstBlockMessage == "" { + firstBlockMessage = verdict.Message + } + if !matchAll && verdict.Block { + // The returned verdict is a clean block, so it carries Errored=false + // and the caller records the evaluation as fully classified. That is + // the right verdict, but it hides that an earlier target was never + // classified, so the partial failure is logged here rather than + // disappearing from the record. + if sawErrored { + r.log.WarnContext(ctx, "exec validator blocked with an unclassified target", + "rule", ruleName, "on_error", c.OnError) + } + return verdict + } + if matchAll && !verdict.Block { + return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} + } + } + if matchAll { + return execconcern.Verdict{Block: true, Message: firstBlockMessage, Output: "", Errored: false} + } + if sawErrored { + return firstErrored + } + return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} +} + +// logExpandedCommandError records why one expanded validator run produced an +// errored verdict, naming the failure mode so a spawn failure, a nonzero exit +// under a JSON predicate, and unparseable predicate output stay distinguishable +// in the log. +func (r *ExecRuntime) logExpandedCommandError( + ctx context.Context, + ruleName string, + c *config.Condition, + command []string, + res execconcern.RunResult, + runErr error, +) { + switch { + case runErr != nil: + r.log.WarnContext(ctx, "exec validator expanded command errored", + "rule", ruleName, "on_error", c.OnError, "command", command, "err", runErr) + case c.BlockOn == config.BlockOnMatch && res.ExitCode != 0: + r.log.WarnContext(ctx, "exec validator expanded command exited nonzero for JSON match", + "rule", ruleName, "on_error", c.OnError, "command", command, "exit_code", res.ExitCode) + case c.BlockOn == config.BlockOnMatch: + r.log.WarnContext(ctx, "exec validator expanded command returned invalid JSON predicate output", + "rule", ruleName, "on_error", c.OnError, "command", command) + default: + r.log.WarnContext(ctx, "exec validator expanded command produced an errored verdict", + "rule", ruleName, "on_error", c.OnError, "command", command, "exit_code", res.ExitCode) + } +} diff --git a/internal/rules/exec_gate.go b/internal/rules/exec_gate.go index 35d6358..b378dcd 100644 --- a/internal/rules/exec_gate.go +++ b/internal/rules/exec_gate.go @@ -608,89 +608,6 @@ func (r *ExecRuntime) runValidator( } } -func (r *ExecRuntime) runExpandedCommands( - ctx context.Context, - ruleName string, - c *config.Condition, - commands [][]string, - stdin []byte, - env []string, -) execconcern.Verdict { - if len(commands) == 0 { - return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} - } - - forEach := c.ForEachSelector().Selector != config.FieldSelectorInvalid - matchAll := forEach && c.MatchMode == config.ExecMatchAll - firstBlockMessage := "" - firstErrored := execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} - sawErrored := false - for _, command := range commands { - verdict := r.runExpandedCommandWithRetry(ctx, ruleName, c, command, stdin, env) - if !forEach { - return verdict - } - if verdict.Errored { - // Under match_mode = "any" one target the validator cannot classify - // must not veto the rest. A single unresolvable target would - // otherwise decide the whole expansion and every remaining target, - // including an indexed one that would block, is never probed. The - // error is remembered and only decides the condition when no target - // blocked. Under match_mode = "all" an errored target still returns - // immediately, because "every target matches" cannot be proven once - // one target is unknown. - if matchAll { - return verdict - } - if !sawErrored { - sawErrored = true - firstErrored = verdict - } - continue - } - if verdict.Block && firstBlockMessage == "" { - firstBlockMessage = verdict.Message - } - if !matchAll && verdict.Block { - return verdict - } - if matchAll && !verdict.Block { - return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} - } - } - if matchAll { - return execconcern.Verdict{Block: true, Message: firstBlockMessage, Output: "", Errored: false} - } - if sawErrored { - return firstErrored - } - return execconcern.Verdict{Block: false, Message: "", Output: "", Errored: false} -} - -func (r *ExecRuntime) logExpandedCommandError( - ctx context.Context, - ruleName string, - c *config.Condition, - command []string, - res execconcern.RunResult, - runErr error, -) { - switch { - case runErr != nil: - r.log.WarnContext(ctx, "exec validator expanded command errored", - "rule", ruleName, "on_error", c.OnError, "command", command, "err", runErr) - case c.BlockOn == config.BlockOnMatch && res.ExitCode != 0: - r.log.WarnContext(ctx, "exec validator expanded command exited nonzero for JSON match", - "rule", ruleName, "on_error", c.OnError, "command", command, "exit_code", res.ExitCode) - case c.BlockOn == config.BlockOnMatch: - r.log.WarnContext(ctx, "exec validator expanded command returned invalid JSON predicate output", - "rule", ruleName, "on_error", c.OnError, "command", command) - default: - r.log.WarnContext(ctx, "exec validator expanded command produced an errored verdict", - "rule", ruleName, "on_error", c.OnError, "command", command, "exit_code", res.ExitCode) - } -} - func (r *ExecRuntime) buildInput( ctx context.Context, fields FieldSet, diff --git a/internal/rules/exec_gate_errored_target_test.go b/internal/rules/exec_gate_errored_target_test.go index 238722c..97e71a6 100644 --- a/internal/rules/exec_gate_errored_target_test.go +++ b/internal/rules/exec_gate_errored_target_test.go @@ -26,6 +26,10 @@ func TestExecForEachAnyErroredTargetDoesNotVetoOthers(t *testing.T) { if err != nil { t.Fatalf("EvalSymlinks erroringTarget: %v", err) } + wantMatching, err := filepath.EvalSymlinks(matchingTarget) + if err != nil { + t.Fatalf("EvalSymlinks matchingTarget: %v", err) + } rule := loadExecRule(t, ` [[rules]] @@ -67,8 +71,88 @@ search_tools = ["grep"] if len(violations) == 0 { t.Fatal("an errored target vetoed a matching target under match_mode=any") } - if len(runner.Commands()) != 2 { - t.Fatalf("expected both targets probed, got %d commands", len(runner.Commands())) + // The contract is that the matching target is reached, not that a fixed + // number of validators ran. Asserting a count would pin the order the two + // targets happen to resolve in, and would also forbid a later change that + // probes targets concurrently or stops early once one blocks. + if !probedTarget(runner, wantMatching) { + t.Fatalf("matching target %q was never probed, commands = %v", wantMatching, runner.Commands()) + } +} + +// probedTarget reports whether the runner was asked to validate a given target. +func probedTarget(runner *recordingCommandRunner, target string) bool { + for _, command := range runner.Commands() { + for _, argument := range command { + if argument == target { + return true + } + } + } + return false +} + +// TestExecForEachAnyFailClosedStopsAtFirstErroredTarget covers the bound on +// validator work: under on_error = "closed" an errored target already decides +// the condition, so the remaining targets are not probed. Continuing would cost +// one validator run per remaining target, each up to the background timeout +// times retry_count, under a context that cannot be cancelled and while the +// singleflight entry for this cache key is held. +func TestExecForEachAnyFailClosedStopsAtFirstErroredTarget(t *testing.T) { + erroringTarget := filepath.Join(t.TempDir(), "repo-unknown") + laterTarget := filepath.Join(t.TempDir(), "repo-indexed") + for _, dir := range []string{erroringTarget, laterTarget} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("MkdirAll %s: %v", dir, err) + } + } + wantErroring, err := filepath.EvalSymlinks(erroringTarget) + if err != nil { + t.Fatalf("EvalSymlinks erroringTarget: %v", err) + } + + rule := loadExecRule(t, ` +[[rules]] +name = "exec-rule" +events = ["PreToolUse"] +action = "block" +violation_message = "static message" + +[[rules.conditions]] +kind = "regex" +field_paths = ["tool_input.command"] +pattern = "grep" + +[[rules.conditions]] +kind = "exec" +command = ["/bin/check-target", "{{item}}"] +for_each = "cmd_read_targets" +match_mode = "any" +stdout_json_field = "searchable" +stdout_json_equals = true +cache_key = "cmd_read_targets" +cache_ttl_ms = 0 +on_error = "closed" +search_tools = ["grep"] +`) + runner := &recordingCommandRunner{ + run: func(command []string) (execconcern.RunResult, error) { + if command[1] == wantErroring { + return execconcern.RunResult{ExitCode: 0, Stdout: "not json"}, nil + } + return execconcern.RunResult{ExitCode: 0, Stdout: `{"searchable":false}`}, nil + }, + } + + violations := evalRule(runner, rule, map[string]any{ + "cwd": t.TempDir(), + "tool_input": map[string]any{"command": "grep -rn x " + erroringTarget + " " + laterTarget}, + }) + if len(violations) == 0 { + t.Fatal("a fail-closed rule should block once a target errors") + } + if len(runner.Commands()) != 1 { + t.Fatalf("fail-closed should stop at the errored target, got %d commands", len(runner.Commands())) } } @@ -121,7 +205,7 @@ search_tools = ["grep"] if len(violations) == 0 { t.Fatal("all targets errored under on_error=closed should block") } - if len(runner.Commands()) != 2 { - t.Fatalf("expected both targets probed, got %d commands", len(runner.Commands())) + if len(runner.Commands()) == 0 { + t.Fatal("no target was probed at all") } }