Skip to content

fix(search): stop gitignored-directory matches from exhausting the rg kill budget (#1341) - #1369

Open
tusharui wants to merge 3 commits into
Nano-Collective:mainfrom
tusharui:fix/1341-search-ignored-dir-budget
Open

tusharui wants to merge 3 commits into
Nano-Collective:mainfrom
tusharui:fix/1341-search-ignored-dir-budget

Conversation

@tusharui

Copy link
Copy Markdown
Contributor

Summary

Closes #1341. search_file_contents could return 0 matches (with truncated: true)
when ripgrep's early-kill match budget (rgMaxCount) was consumed by matches inside
ignored directories. ripgrep still counts matches in ignored paths toward its
budget, so those lines were burned before reaching any kept result — and since the
only drop to ripgrep was the --max-count, the JS filter downstream never got a
kept line at all.

Fix

Two layers, applied together:

Layer 1 — don't count ignored output against the budget
runRipgrep now streams results via onLine and increments the kill counter only
for kept (non-ignored) type: "match" lines, against rgMaxCount = maxResults + contextLines.

Layer 2 — keep ignored paths out of ripgrep entirely
A merged ignore set (DEFAULT_IGNORE_DIRS.gitignore.nanocoderignore) is
written to a temp --ignore-file and wired into both searchProjectContents and
walkProjectEntries, so rg never traverses/matches ignored directories in the first place.

Correctness backstop: the existing downstream JS filter (projectIgnore.ignores) still applies.

Hardening (follow-up commit, from maintainer review)

  • **/-prefixed rules kept verbatim (git depth-agnostic semantics)
  • cross-drive guard for rules outside the project root
  • temp ignore file written with mode: 0o600
  • git-compliant trailing-whitespace trimming (escaped trailing space preserved)
  • empty/comment-only .gitignore skips the temp file entirely
  • bounded Layer-1 buffer: onLine is tri-state (keep | skip | stop); stdout holds only kept lines
  • per-file prune propagated to walkUnsortedFileStream / searchProjectContents

Dev tooling (same branch)

  • pnpm test:all now runs via scripts/test-all.mjs (cross-platform Node orchestrator,
    same 7 gates as scripts/test.sh), so it works on Windows too.

Testing

Changeset

.changeset/search-ignored-dir-budget.mdpatchCloses #1341

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important, 2 nits

@tusharui — there is a blocking item below.

PR #1369 addresses a documented bug (issue #1341) where ripgrep's match-count kill budget could be exhausted by matches in default-ignored directories, leaving downstream JS filters with zero kept results. The described two-layer fix (only count kept matches; pass a merged ignore file to rg) is conceptually correct and aligns with the architecture in source/utils/file-search.ts. The changeset file is appropriate for a user-facing bug fix. However, only one of the seven claimed new tests ('a .nanocoderignore directory is pruned during traversal') is visible in source/utils/file-search.spec.ts on disk; the regression tests named for the bug (#1341 plain, #1341 with context, **/-anchored rebase guard, rebaseIgnoreLine unit suite, ! re-include, abort temp-dir cleanup) are not present, so the new behaviour has no test that would fail if it regressed. The cross-platform scripts/test-all.mjs change is unrelated to the bug fix and is bundled into this PR.

🔴 blocking · tests · source/utils/file-search.spec.ts

The PR description claims seven new AVA tests: '#1341 plain', '#1341 with context', '**/-anchored rebase guard', 'walkProjectEntries pruning', 'rebaseIgnoreLine unit suite', '! re-include', and 'abort temp-dir cleanup'. Only one — 'a .nanocoderignore directory is pruned during traversal, not just filtered from results' (around line 411) — is visible in the spec file on disk. The two regression tests that would actually fail if the Layer 1 (kept-match-only counting) fix regressed — ones that build a fixture with many matches in a default-ignored directory (e.g. dist/) and a few matches in source/, then call searchProjectContents with a small maxResults — are absent. Per the project rubric, a bug fix must include a regression test that actually exercises the bug; the existing tests in the file all have matches concentrated in a single non-ignored file and so would not catch a regression where rg again exhausts its budget on ignored matches. The Layer 2 path has the prune test above, but Layer 1 has no equivalent.

Reproducing #1341 in a test looks like this (sketch — adapt to the actual API):

mkdirSync(join(testDir, 'dist'), {recursive: true});
mkdirSync(join(testDir, 'src'), {recursive: true});
// dist/ contains 20 matches for the query
for (let i = 0; i < 20; i++) writeFileSync(join(testDir, 'dist', `f${i}.js`), 'searchTarget');
writeFileSync(join(testDir, 'src', 'app.ts'), 'searchTarget');
const result = await searchProjectContents('searchTarget', testDir, 5, false);
t.is(result.matches.length, 1);
t.is(result.matches[0]?.file, 'src/app.ts');

A test like this would have failed on the base code (0 matches) and passes after the fix (1 match). Without it, the fix could regress silently.

🟠 important · scope · scripts/test-all.mjs

The diff adds a new scripts/test-all.mjs cross-platform orchestrator and updates package.json to point test:all at it. This is a drive-by dev-tooling change bundled into a bug-fix PR. The two changes have different reviewers, different rollback profiles, and different test gates. Per the project rubric, 'drive-by refactors mixed into a functional change are harder to review and harder to revert'. The Windows-compatibility fix for the test runner deserves its own PR with its own discussion, especially since it changes the default for every contributor who runs pnpm test:all.

⚪ nit · correctness · source/utils/file-search.ts

I cannot fully verify the correctness of the new runRipgrep budget logic without seeing the actual diff. The base code increments on every type: 'match' JSON line regardless of which file it came from, so the conceptual fix (only increment for lines that pass a kept-filter callback) is sound, but the contract between runRipgrep's new onLine tri-state return and the call sites in searchProjectContents / walkUnsortedFileStream / the sorted walkProjectEntries path is worth checking: the sorted path in walkProjectEntries does not pass onLine and just consumes stdout wholesale, so the Layer 1 fix only applies where onLine is wired in. If the sorted path still runs rg on content search (it doesn't, it uses --files), it would need the same treatment; for the --files walk the budget concern is maxRawFilesScanned, which is already capped correctly. Worth a maintainer's eye during merge review.

⚪ nit · completeness · .changeset/search-ignored-dir-budget.md

The changeset is present and well-shaped (closes the issue, patch bump, names the right package). No issue here — flagging only because the rubric asks reviewers to confirm the changeset, and this one is correct.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] search_file_contents returns 0 matches when ripgrep kills early on gitignored directory matches

1 participant