fix(tools): bound grep search duration
- commit
- 6a87ac51ff1f8c7fa1db56db1f5b4a98563aec30
- author
- brice · · 2026-08-14 03:52 UTC
- parent
- ee6ee37
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +97 -1
M packages/agent/tools/grep.go +29 -1
| @@ -11,6 +11,7 @@ import ( | |||
| 11 | 11 | "path/filepath" | |
| 12 | 12 | "strings" | |
| 13 | 13 | "sync" | |
| 14 | + | "time" | |
| 14 | 15 | "unicode/utf8" | |
| 15 | 16 | ||
| 16 | 17 | "github.com/bnema/zut/packages/core" | |
| @@ -22,6 +23,7 @@ const ( | |||
| 22 | 23 | maxGrepOutputBytes = 50 * 1024 | |
| 23 | 24 | maxGrepOutputLines = 2000 | |
| 24 | 25 | grepTruncationReserve = 128 | |
| 26 | + | grepInternalTimeout = 30 * time.Second | |
| 25 | 27 | ) | |
| 26 | 28 | ||
| 27 | 29 | // GrepTool searches text files below a requested path without invoking a | |
| @@ -54,6 +56,24 @@ func (t *GrepTool) Schema() json.RawMessage { return json.RawMessage(grepSchema) | |||
| 54 | 56 | // checked before the executable is selected or started, so a denied path | |
| 55 | 57 | // cannot be used to probe or read through the subprocess. | |
| 56 | 58 | func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress func(string)) (core.ToolResult, error) { | |
| 59 | + | return t.executeWithTimeout(ctx, raw, progress, grepInternalTimeout) | |
| 60 | + | } | |
| 61 | + | ||
| 62 | + | // grepTimeoutError distinguishes the search's hard deadline from a caller | |
| 63 | + | // cancellation or deadline, so the caller gets actionable search guidance. | |
| 64 | + | type grepTimeoutError struct { | |
| 65 | + | after time.Duration | |
| 66 | + | } | |
| 67 | + | ||
| 68 | + | func (e *grepTimeoutError) Error() string { | |
| 69 | + | return fmt.Sprintf("grep: search timed out after %s; try narrowing the path or glob", e.after) | |
| 70 | + | } | |
| 71 | + | ||
| 72 | + | func (e *grepTimeoutError) Timeout() bool { return true } | |
| 73 | + | ||
| 74 | + | // executeWithTimeout keeps the production deadline in Execute while allowing | |
| 75 | + | // deadline behavior to be tested without waiting 30 seconds. | |
| 76 | + | func (t *GrepTool) executeWithTimeout(ctx context.Context, raw json.RawMessage, progress func(string), timeout time.Duration) (core.ToolResult, error) { | |
| 57 | 77 | args, err := parseGrepArgs(raw) | |
| 58 | 78 | if err != nil { | |
| 59 | 79 | return core.ToolResult{}, err | |
| @@ -86,7 +106,12 @@ func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||
| 86 | 106 | } | |
| 87 | 107 | ||
| 88 | 108 | commandArgs := grepCommandArgs(engine, args.Pattern, root) | |
| 89 | - | cmd := exec.CommandContext(ctx, executable, commandArgs...) | |
| 109 | + | // WithTimeout preserves a sooner deadline or cancellation from ctx; the | |
| 110 | + | // internal deadline can only add a limit when the caller has not already | |
| 111 | + | // supplied a stricter one. | |
| 112 | + | runCtx, cancel := context.WithTimeout(ctx, timeout) | |
| 113 | + | defer cancel() | |
| 114 | + | cmd := exec.CommandContext(runCtx, executable, commandArgs...) | |
| 90 | 115 | if info.IsDir() { | |
| 91 | 116 | cmd.Dir = root | |
| 92 | 117 | } else { | |
| @@ -110,6 +135,9 @@ func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||
| 110 | 135 | if err := ctx.Err(); err != nil { | |
| 111 | 136 | return core.ToolResult{}, err | |
| 112 | 137 | } | |
| 138 | + | if errors.Is(runCtx.Err(), context.DeadlineExceeded) { | |
| 139 | + | return core.ToolResult{}, &grepTimeoutError{after: timeout} | |
| 140 | + | } | |
| 113 | 141 | ||
| 114 | 142 | actualExitCode := 0 | |
| 115 | 143 | if runErr != nil { | |
| @@ -11,6 +11,7 @@ import ( | |||||
| 11 | "path/filepath" | 11 | "path/filepath" | ||
| 12 | "strings" | 12 | "strings" | ||
| 13 | "sync" | 13 | "sync" | ||
| 14 | + | "time" | |||
| 14 | "unicode/utf8" | 15 | "unicode/utf8" | ||
| 15 | 16 | ||||
| 16 | "github.com/bnema/zut/packages/core" | 17 | "github.com/bnema/zut/packages/core" | ||
| @@ -22,6 +23,7 @@ const ( | |||||
| 22 | maxGrepOutputBytes = 50 * 1024 | 23 | maxGrepOutputBytes = 50 * 1024 | ||
| 23 | maxGrepOutputLines = 2000 | 24 | maxGrepOutputLines = 2000 | ||
| 24 | grepTruncationReserve = 128 | 25 | grepTruncationReserve = 128 | ||
| 26 | + | grepInternalTimeout = 30 * time.Second | |||
| 25 | ) | 27 | ) | ||
| 26 | 28 | ||||
| 27 | // GrepTool searches text files below a requested path without invoking a | 29 | // GrepTool searches text files below a requested path without invoking a | ||
| @@ -54,6 +56,24 @@ func (t *GrepTool) Schema() json.RawMessage { return json.RawMessage(grepSchema) | |||||
| 54 | // checked before the executable is selected or started, so a denied path | 56 | // checked before the executable is selected or started, so a denied path | ||
| 55 | // cannot be used to probe or read through the subprocess. | 57 | // cannot be used to probe or read through the subprocess. | ||
| 56 | func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress func(string)) (core.ToolResult, error) { | 58 | func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress func(string)) (core.ToolResult, error) { | ||
| 59 | + | return t.executeWithTimeout(ctx, raw, progress, grepInternalTimeout) | |||
| 60 | + | } | |||
| 61 | + | ||||
| 62 | + | // grepTimeoutError distinguishes the search's hard deadline from a caller | |||
| 63 | + | // cancellation or deadline, so the caller gets actionable search guidance. | |||
| 64 | + | type grepTimeoutError struct { | |||
| 65 | + | after time.Duration | |||
| 66 | + | } | |||
| 67 | + | ||||
| 68 | + | func (e *grepTimeoutError) Error() string { | |||
| 69 | + | return fmt.Sprintf("grep: search timed out after %s; try narrowing the path or glob", e.after) | |||
| 70 | + | } | |||
| 71 | + | ||||
| 72 | + | func (e *grepTimeoutError) Timeout() bool { return true } | |||
| 73 | + | ||||
| 74 | + | // executeWithTimeout keeps the production deadline in Execute while allowing | |||
| 75 | + | // deadline behavior to be tested without waiting 30 seconds. | |||
| 76 | + | func (t *GrepTool) executeWithTimeout(ctx context.Context, raw json.RawMessage, progress func(string), timeout time.Duration) (core.ToolResult, error) { | |||
| 57 | args, err := parseGrepArgs(raw) | 77 | args, err := parseGrepArgs(raw) | ||
| 58 | if err != nil { | 78 | if err != nil { | ||
| 59 | return core.ToolResult{}, err | 79 | return core.ToolResult{}, err | ||
| @@ -86,7 +106,12 @@ func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||||
| 86 | } | 106 | } | ||
| 87 | 107 | ||||
| 88 | commandArgs := grepCommandArgs(engine, args.Pattern, root) | 108 | commandArgs := grepCommandArgs(engine, args.Pattern, root) | ||
| 89 | - | cmd := exec.CommandContext(ctx, executable, commandArgs...) | 109 | + | // WithTimeout preserves a sooner deadline or cancellation from ctx; the |
| 110 | + | // internal deadline can only add a limit when the caller has not already | |||
| 111 | + | // supplied a stricter one. | |||
| 112 | + | runCtx, cancel := context.WithTimeout(ctx, timeout) | |||
| 113 | + | defer cancel() | |||
| 114 | + | cmd := exec.CommandContext(runCtx, executable, commandArgs...) | |||
| 90 | if info.IsDir() { | 115 | if info.IsDir() { | ||
| 91 | cmd.Dir = root | 116 | cmd.Dir = root | ||
| 92 | } else { | 117 | } else { | ||
| @@ -110,6 +135,9 @@ func (t *GrepTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||||
| 110 | if err := ctx.Err(); err != nil { | 135 | if err := ctx.Err(); err != nil { | ||
| 111 | return core.ToolResult{}, err | 136 | return core.ToolResult{}, err | ||
| 112 | } | 137 | } | ||
| 138 | + | if errors.Is(runCtx.Err(), context.DeadlineExceeded) { | |||
| 139 | + | return core.ToolResult{}, &grepTimeoutError{after: timeout} | |||
| 140 | + | } | |||
| 113 | 141 | ||||
| 114 | actualExitCode := 0 | 142 | actualExitCode := 0 | ||
| 115 | if runErr != nil { | 143 | if runErr != nil { | ||
M packages/agent/tools/grep_test.go +68 -0
| @@ -177,6 +177,74 @@ func TestGrepCancellationPropagates(t *testing.T) { | |||
| 177 | 177 | } | |
| 178 | 178 | } | |
| 179 | 179 | ||
| 180 | + | func TestGrepInternalTimeoutStopsSearch(t *testing.T) { | |
| 181 | + | skipGrepScriptTestsOnWindows(t) | |
| 182 | + | root := t.TempDir() | |
| 183 | + | if err := os.WriteFile(filepath.Join(root, "hold"), nil, 0o644); err != nil { | |
| 184 | + | t.Fatal(err) | |
| 185 | + | } | |
| 186 | + | script := writeGrepScript(t, "#!/bin/sh\nprintf 'started\\n'\nexec tail -f hold\n") | |
| 187 | + | tool := &GrepTool{CWD: root, LookPath: lookupScripts(map[string]string{"rg": script})} | |
| 188 | + | ||
| 189 | + | resultCh := make(chan error, 1) | |
| 190 | + | go func() { | |
| 191 | + | _, err := tool.executeWithTimeout(context.Background(), grepArgsJSON("needle", root), nil, 100*time.Millisecond) | |
| 192 | + | resultCh <- err | |
| 193 | + | }() | |
| 194 | + | ||
| 195 | + | err := waitForGrepResult(t, resultCh) | |
| 196 | + | var timeoutErr *grepTimeoutError | |
| 197 | + | if !errors.As(err, &timeoutErr) { | |
| 198 | + | t.Fatalf("error = %v, want internal grep timeout", err) | |
| 199 | + | } | |
| 200 | + | if errors.Is(err, context.DeadlineExceeded) { | |
| 201 | + | t.Fatalf("internal timeout error = %v, want distinct caller deadline error", err) | |
| 202 | + | } | |
| 203 | + | if !strings.Contains(err.Error(), "try narrowing the path or glob") { | |
| 204 | + | t.Fatalf("error = %v, want path/glob narrowing guidance", err) | |
| 205 | + | } | |
| 206 | + | } | |
| 207 | + | ||
| 208 | + | func TestGrepCallerDeadlineTakesPrecedenceOverInternalTimeout(t *testing.T) { | |
| 209 | + | skipGrepScriptTestsOnWindows(t) | |
| 210 | + | root := t.TempDir() | |
| 211 | + | if err := os.WriteFile(filepath.Join(root, "hold"), nil, 0o644); err != nil { | |
| 212 | + | t.Fatal(err) | |
| 213 | + | } | |
| 214 | + | script := writeGrepScript(t, "#!/bin/sh\nprintf 'started\\n'\nexec tail -f hold\n") | |
| 215 | + | tool := &GrepTool{CWD: root, LookPath: lookupScripts(map[string]string{"rg": script})} | |
| 216 | + | ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) | |
| 217 | + | defer cancel() | |
| 218 | + | ||
| 219 | + | resultCh := make(chan error, 1) | |
| 220 | + | go func() { | |
| 221 | + | _, err := tool.executeWithTimeout(ctx, grepArgsJSON("needle", root), nil, time.Second) | |
| 222 | + | resultCh <- err | |
| 223 | + | }() | |
| 224 | + | ||
| 225 | + | err := waitForGrepResult(t, resultCh) | |
| 226 | + | if !errors.Is(err, context.DeadlineExceeded) { | |
| 227 | + | t.Fatalf("error = %v, want caller deadline exceeded", err) | |
| 228 | + | } | |
| 229 | + | var timeoutErr *grepTimeoutError | |
| 230 | + | if errors.As(err, &timeoutErr) { | |
| 231 | + | t.Fatalf("error = %v, want caller deadline rather than internal timeout", err) | |
| 232 | + | } | |
| 233 | + | } | |
| 234 | + | ||
| 235 | + | func waitForGrepResult(t *testing.T, resultCh <-chan error) error { | |
| 236 | + | t.Helper() | |
| 237 | + | timer := time.NewTimer(2 * time.Second) | |
| 238 | + | defer timer.Stop() | |
| 239 | + | select { | |
| 240 | + | case err := <-resultCh: | |
| 241 | + | return err | |
| 242 | + | case <-timer.C: | |
| 243 | + | t.Fatal("grep fixture did not stop before the test deadline") | |
| 244 | + | return nil | |
| 245 | + | } | |
| 246 | + | } | |
| 247 | + | ||
| 180 | 248 | func TestGrepDoesNotRequireBashPermission(t *testing.T) { | |
| 181 | 249 | skipGrepScriptTestsOnWindows(t) | |
| 182 | 250 | root := t.TempDir() | |
| @@ -177,6 +177,74 @@ func TestGrepCancellationPropagates(t *testing.T) { | |||||
| 177 | } | 177 | } | ||
| 178 | } | 178 | } | ||
| 179 | 179 | ||||
| 180 | + | func TestGrepInternalTimeoutStopsSearch(t *testing.T) { | |||
| 181 | + | skipGrepScriptTestsOnWindows(t) | |||
| 182 | + | root := t.TempDir() | |||
| 183 | + | if err := os.WriteFile(filepath.Join(root, "hold"), nil, 0o644); err != nil { | |||
| 184 | + | t.Fatal(err) | |||
| 185 | + | } | |||
| 186 | + | script := writeGrepScript(t, "#!/bin/sh\nprintf 'started\\n'\nexec tail -f hold\n") | |||
| 187 | + | tool := &GrepTool{CWD: root, LookPath: lookupScripts(map[string]string{"rg": script})} | |||
| 188 | + | ||||
| 189 | + | resultCh := make(chan error, 1) | |||
| 190 | + | go func() { | |||
| 191 | + | _, err := tool.executeWithTimeout(context.Background(), grepArgsJSON("needle", root), nil, 100*time.Millisecond) | |||
| 192 | + | resultCh <- err | |||
| 193 | + | }() | |||
| 194 | + | ||||
| 195 | + | err := waitForGrepResult(t, resultCh) | |||
| 196 | + | var timeoutErr *grepTimeoutError | |||
| 197 | + | if !errors.As(err, &timeoutErr) { | |||
| 198 | + | t.Fatalf("error = %v, want internal grep timeout", err) | |||
| 199 | + | } | |||
| 200 | + | if errors.Is(err, context.DeadlineExceeded) { | |||
| 201 | + | t.Fatalf("internal timeout error = %v, want distinct caller deadline error", err) | |||
| 202 | + | } | |||
| 203 | + | if !strings.Contains(err.Error(), "try narrowing the path or glob") { | |||
| 204 | + | t.Fatalf("error = %v, want path/glob narrowing guidance", err) | |||
| 205 | + | } | |||
| 206 | + | } | |||
| 207 | + | ||||
| 208 | + | func TestGrepCallerDeadlineTakesPrecedenceOverInternalTimeout(t *testing.T) { | |||
| 209 | + | skipGrepScriptTestsOnWindows(t) | |||
| 210 | + | root := t.TempDir() | |||
| 211 | + | if err := os.WriteFile(filepath.Join(root, "hold"), nil, 0o644); err != nil { | |||
| 212 | + | t.Fatal(err) | |||
| 213 | + | } | |||
| 214 | + | script := writeGrepScript(t, "#!/bin/sh\nprintf 'started\\n'\nexec tail -f hold\n") | |||
| 215 | + | tool := &GrepTool{CWD: root, LookPath: lookupScripts(map[string]string{"rg": script})} | |||
| 216 | + | ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) | |||
| 217 | + | defer cancel() | |||
| 218 | + | ||||
| 219 | + | resultCh := make(chan error, 1) | |||
| 220 | + | go func() { | |||
| 221 | + | _, err := tool.executeWithTimeout(ctx, grepArgsJSON("needle", root), nil, time.Second) | |||
| 222 | + | resultCh <- err | |||
| 223 | + | }() | |||
| 224 | + | ||||
| 225 | + | err := waitForGrepResult(t, resultCh) | |||
| 226 | + | if !errors.Is(err, context.DeadlineExceeded) { | |||
| 227 | + | t.Fatalf("error = %v, want caller deadline exceeded", err) | |||
| 228 | + | } | |||
| 229 | + | var timeoutErr *grepTimeoutError | |||
| 230 | + | if errors.As(err, &timeoutErr) { | |||
| 231 | + | t.Fatalf("error = %v, want caller deadline rather than internal timeout", err) | |||
| 232 | + | } | |||
| 233 | + | } | |||
| 234 | + | ||||
| 235 | + | func waitForGrepResult(t *testing.T, resultCh <-chan error) error { | |||
| 236 | + | t.Helper() | |||
| 237 | + | timer := time.NewTimer(2 * time.Second) | |||
| 238 | + | defer timer.Stop() | |||
| 239 | + | select { | |||
| 240 | + | case err := <-resultCh: | |||
| 241 | + | return err | |||
| 242 | + | case <-timer.C: | |||
| 243 | + | t.Fatal("grep fixture did not stop before the test deadline") | |||
| 244 | + | return nil | |||
| 245 | + | } | |||
| 246 | + | } | |||
| 247 | + | ||||
| 180 | func TestGrepDoesNotRequireBashPermission(t *testing.T) { | 248 | func TestGrepDoesNotRequireBashPermission(t *testing.T) { | ||
| 181 | skipGrepScriptTestsOnWindows(t) | 249 | skipGrepScriptTestsOnWindows(t) | ||
| 182 | root := t.TempDir() | 250 | root := t.TempDir() | ||