fix: add timeout, home/root blocking, and default exclusions to glob tool#637
fix: add timeout, home/root blocking, and default exclusions to glob tool#637anandgupta42 merged 7 commits intomainfrom
Conversation
- Add `abortAfter` timeout that kills the `rg` process after 30 seconds - Return partial results on timeout with actionable guidance message - Fix error handling: only treat `AbortError` from timeout signal as timeout — properly re-throw ENOENT, permission, and user abort errors - Add `localAbort` controller to kill `rg` process on early loop exit (e.g., 100-file limit hit), preventing background resource waste - Add comprehensive glob tool tests (6 tests covering basic matching, truncation, user abort, nested patterns, and custom path) Closes #636 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe glob tool adds a 30s abortable ripgrep scan, blocks searches for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/tool/glob.test.ts (1)
20-135: Add a deterministic timeout-path test for the new behavior.This suite is strong, but it does not directly assert the timeout branch (
timedOutmessaging +metadata.truncatedon timeout). Since timeout handling is the main change, add one focused test that forces timeout deterministically (e.g., mockedabortAfteror injected short timeout).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/glob.test.ts` around lines 20 - 135, Add a new unit test that deterministically exercises the timeout branch of GlobTool.execute: create a temp dir with enough files (or mock/inject a very short timeout/abortAfter value) then call GlobTool.init() and invoke glob.execute with a ctx or option that triggers immediate timeout; assert that result.output contains the "timedOut" message (or exact timeout string used in the code) and that result.metadata.truncated (or metadata.count and truncated flags) reflect a timeout. Reference GlobTool.init, glob.execute, the timeout/abortAfter mechanism or ctx.abort, and the metadata.truncated/timedOut behavior when adding the test. Ensure the test forces an abort deterministically rather than relying on timing flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/tool/glob.ts`:
- Around line 66-73: When handling errors in the catch block inside glob.ts,
don't suppress every error when timeout.signal.aborted is true; instead detect
that the thrown error is an AbortError (e.g., check err.name === 'AbortError' or
use an AbortError type guard), ensure the timeout actually fired
(timeout.signal.aborted) and that the context wasn't a user-initiated abort
(ctx.abort or ctx.signal), then set timedOut = true only for timeout-generated
AbortErrors; for any other error (permission errors, ENOENT, or user aborts via
ctx.abort) rethrow the error so they aren't masked. Reference symbols:
timeout.signal.aborted, err (caught error), timedOut, and ctx.abort/ctx.signal.
---
Nitpick comments:
In `@packages/opencode/test/tool/glob.test.ts`:
- Around line 20-135: Add a new unit test that deterministically exercises the
timeout branch of GlobTool.execute: create a temp dir with enough files (or
mock/inject a very short timeout/abortAfter value) then call GlobTool.init() and
invoke glob.execute with a ctx or option that triggers immediate timeout; assert
that result.output contains the "timedOut" message (or exact timeout string used
in the code) and that result.metadata.truncated (or metadata.count and truncated
flags) reflect a timeout. Reference GlobTool.init, glob.execute, the
timeout/abortAfter mechanism or ctx.abort, and the metadata.truncated/timedOut
behavior when adding the test. Ensure the test forces an abort deterministically
rather than relying on timing flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7699a825-4154-4f6c-bfb4-d012528de821
📒 Files selected for processing (2)
packages/opencode/src/tool/glob.tspackages/opencode/test/tool/glob.test.ts
… tool - Block glob searches from `/` and `~` with immediate helpful message - Add default directory exclusions (`node_modules/`, `dist/`, `build/`, `.cache/`, `.venv/`, etc.) reusing `IGNORE_PATTERNS` from `ls` tool - Document `.gitignore` and `.ignore` file support in glob tool description - Add 5 new tests: home/root blocking, subdirectory of home allowed, `node_modules` excluded, `dist`/`build` excluded - Add `altimate_change` markers to all custom code in upstream file Closes #636 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Wrap `truncated || timedOut` in marker block - Revert glob.txt description changes (triggers marker check on .txt files) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
anandgupta42
changed the title
- Add `AbortError` type check to catch block: only suppress errors that are AbortErrors AND from our timeout AND not user-initiated aborts - Add test for ENOENT propagation (non-abort errors not masked as timeout) - Add test for normal completion without timeout on small directories Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ignal` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
What does this PR do?
Fixes glob tool hanging indefinitely when scanning broad directories (e.g.,
~with**/dbt_project.yml). Closes the 3 gaps vs industry best practices (Roo Code, Cline, Codex CLI).Changes:
rgprocess after 30s, returns partial results with actionable guidance/and~(like Roo Code/Cline)node_modules/,dist/,build/,.cache/,.venv/, etc. (24 patterns fromIGNORE_PATTERNS)AbortErrorfrom timeout as timeout; ENOENT/permission errors propagatelocalAbortcontroller killsrgon early loop exit (100-file limit)Upstream context: Related upstream issues (#18954, #5220) exist but no fixes merged. Our approach matches industry patterns used by Roo Code (10s timeout + 16 dir exclusions + home blocking) and Cline.
Type of change
Issue for this PR
Closes #636
How did you verify your code works?
util/glob(12),tool/grep(11),util/abort(6) tests pass — no regressions--strict)Checklist
bun run script/upstream/analyze.ts --markers --base main --strict)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes