Skip to content

fix(workspaceindex): skip git-ignored and build-cache dirs in Scan only - #1117

Open
FabioLeitao wants to merge 1 commit into
Twigpine:mainfrom
FabioLeitao:fix/1108-scan-ignored-dirs
Open

FabioLeitao wants to merge 1 commit into
Twigpine:mainfrom
FabioLeitao:fix/1108-scan-ignored-dirs

Conversation

@FabioLeitao

@FabioLeitao FabioLeitao commented Oct 5, 2026 •

Copy link
Copy Markdown

Summary

Makes workspaceindex.Scan skip git-ignored paths and common build-cache directories, so the scan budget (repo-map, MCP resources, the per-turn workspace seed) is spent on source instead of target/, .venv/ or .locust_env/. This is the shape approved on the issue: everything new lives in Scan, and ShouldSkipDir is unchanged.

What changes

  • ShouldSkipDir is untouched, so glob, grep, list_directory and path autocomplete keep seeing target/, .venv/, __pycache__/ and the rest.
  • Inside a git work tree, git's ignored set decides: git status --porcelain=v1 -z --ignored=matching .. That covers nested .gitignore files, info/exclude, core.excludesFile, and names no list anticipates.
  • Outside a work tree, or when git is missing, too old, fails, or exceeds 5s, a fixed list stands in: target, __pycache__, .venv, venv, .pytest_cache, .terraform, .mypy_cache, .ruff_cache. It is a separate helper, not ShouldSkipDir.
  • The fixed list does not apply inside a work tree: a tracked package named target is real source there, and git already knows what is build output.

What the issue asked for

  • Git version floor: 2.16, stated on gitIgnoredPaths. status --ignored=<mode> landed in 2.16 and --porcelain=v1 in 2.11. Older git rejects the arguments and takes the non-git path. The git-backed tests skip below 2.16 and say so.
  • Cost on the workspace seed. The seed runs once per turn (buildSystemPromptParts → workspaceSeedContext). Measured: 10ms on Zero itself (1,544 tracked files), 20ms warm / 120ms cold on a 2,411-file Python repo with a large virtualenv. Two short-lived processes (rev-parse --show-prefix, then status) under the 5s bound. BuildFromWorkspace's doc comment said "It performs no git operations"; it now names the one read-only lookup.
  • No index lock. The lookup runs with GIT_OPTIONAL_LOCKS=0 (git 2.15; older git ignores it), so a per-turn git status never takes .git/index.lock next to a git commit.

Edge cases covered by tests

  • .gitignore-only names (.locust_env/, *.db), an ignored file inside an untracked directory (ls-files --directory misses it, status does not), and a non-ASCII ignored directory (-z, no C-quoting).
  • Scanning a repository subdirectory: ignore rules are mapped through --show-prefix to Scan's relative paths.
  • Scanning a directory the enclosing repo ignores as a whole still lists its files.
  • A tracked target/ inside a repo is scanned.
  • Outside git, with a 3-file budget and ten target/debug/.fingerprint files, the real source is reached and Truncated stays false.
  • With git absent from PATH, even inside a repo: no error, the fixed list applies.
  • ShouldSkipDir still returns false for every name in the fixed list.
  • An inherited GIT_OPTIONAL_LOCKS=1 does not re-enable the lock.

Tests are hermetic: HOME, USERPROFILE, XDG_CONFIG_HOME, GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM are redirected, and GIT_CEILING_DIRECTORIES keeps a TMPDIR inside some repo from turning a "not a repo" fixture into one.

Known limitation: git status does not descend into a nested independent repository or a submodule, so a build directory inside one is not reported as ignored. main scans those today too, so this is not a regression; I left it out to keep the change focused.

One unrelated fixture changed: repomap's symlink test named its real directory target, which the non-git fallback now skips. It is renamed to real-dir, with a comment.

Linked issue

Fixes #1108

Checklist

Verification

On commit 6e57f7fb, based on main at 99721c76:

  • make fmt-check, go vet ./..., go test -count=1 ./..., go test -race on workspaceindex, repomap and workspaceseed, go run ./cmd/zero-release build, go run ./cmd/zero-release smoke, make vulncheck, git diff HEAD --check: pass.

  • make lint-static: only the 4 findings already on main (installtest, proxydial x2, web_fetch.go); none in changed files.

  • Cross-compile (go vet + go test -c) for windows/amd64, darwin/amd64, darwin/arm64: pass. Tests were not run on macOS or Windows.

  • Each new test fails when its part of the fix is reverted, one revert at a time:

    Reverted Fails
    GIT_OPTIONAL_LOCKS=0 TestGitCommandDisablesOptionalLocks
    names added to ShouldSkipDir (the earlier draft) TestShouldSkipDirLeavesScanOnlyDirsVisibleToTools, TestScanKeepsTrackedBuildCacheNamedDirInsideGitRepo
    fixed list also applied inside git TestScanKeepsTrackedBuildCacheNamedDirInsideGitRepo
    no git lookup (today's main) TestScanHonorsRepoGitignore, …FromSubdirectoryRoot
    no fallback list TestScanSkipsBuildCacheDirsOutsideGit, TestScanFallsBackWhenGitUnavailable
    ls-files --directory instead of status TestScanHonorsRepoGitignore, …FromSubdirectoryRoot
  • gosec and semgrep (p/golang): no new findings compared with main. gitleaks on the branch commit: clean.

  • No dependency changes (go.mod/go.sum untouched).

Notes

Prepared with AI assistance and reviewed by the human author (HITL), per the contribution guidelines.

Summary by CodeRabbit

  • New Features

    • Workspace scans now respect repository ignore rules, helping exclude ignored files and directories from results.
    • Outside Git repositories, scans skip common build and cache directories. If Git is unavailable, scanning continues with this fallback behavior.
  • Bug Fixes

    • Scans from subdirectories correctly apply repository ignore rules while retaining files when the scan root itself is ignored.

Scan has a fixed file budget, and build output (Cargo target/, Python
virtualenvs, caches) could exhaust it before real source was reached:
repo-map reported truncation and ranked fingerprint files above code.

Inside a git work tree, Scan now asks git for the ignored set
(`git status --porcelain=v1 -z --ignored=matching .`), so the repo's own
.gitignore, nested ignore files, info/exclude and core.excludesFile all
apply, including names no fixed list anticipates (.locust_env/). Outside
a work tree, or when git is missing, older than 2.16, fails, or exceeds
5s, a fixed list of common build/cache directory names stands in.

ShouldSkipDir is unchanged: glob, grep, list_directory and path
autocomplete share it and must keep seeing target/, .venv/ and friends.
The lookup runs with GIT_OPTIONAL_LOCKS=0 so a per-turn scan never takes
.git/index.lock next to a concurrent git command.

Fixes Twigpine#1108
@coderabbitai

coderabbitai Bot commented Oct 5, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: Twigpine/zero/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 53abbd41-7931-451b-b24e-a6eb3d6dafe0
📥 Commits

Reviewing files that changed from the base of the PR and between 99721c7 and 6e57f7f.

📒 Files selected for processing (5)
  • internal/repomap/repomap_test.go
  • internal/workspaceindex/gitignore.go
  • internal/workspaceindex/gitignore_test.go
  • internal/workspaceindex/workspaceindex.go
  • internal/workspaceseed/workspaceseed.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

Workspace scans now use Git’s ignored-path data when available. When Git data is unavailable, scans skip a fixed set of build and cache directory names. Tests cover repository scans, fallback behavior, and symlink traversal.

Changes

Workspace scan behavior

Layer / File(s) Summary
Git ignore lookup
internal/workspaceindex/gitignore.go, internal/workspaceindex/gitignore_test.go
A timed Git lookup obtains ignored paths relative to the scan root. Git commands disable optional locks. Tests configure isolated repositories and verify the command environment.
Scan filtering and fallback
internal/workspaceindex/workspaceindex.go, internal/workspaceindex/gitignore_test.go, internal/repomap/repomap_test.go, internal/workspaceseed/workspaceseed.go
Scan filters Git-ignored files and directories when lookup succeeds. If it fails, scanning uses a fixed directory-name list. Tests cover filtering, fallback, and symlink traversal. The BuildFromWorkspace comment documents the read-only lookup and caller-provided Git state.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Scan as workspaceindex.Scan
  participant Lookup as gitIgnoredPaths
  participant Git as git
  participant FS as filesystem
  Scan->>Lookup: Request ignored paths for scan root
  Lookup->>Git: Get repository prefix and ignored status
  Git-->>Lookup: Return command results
  Lookup-->>Scan: Return relative ignored paths or unavailable status
  Scan->>FS: Traverse workspace entries
  Scan->>Scan: Filter ignored paths or apply fallback skips
Loading

Merge Risk: ⚪ Minimal · up to 6e57f

Workspace scans now skip Git-ignored paths and fall back to a fixed build/cache directory list when Git is unavailable. I found no concrete merge-blocking risk in the supplied changes.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 6e57f

Automatic scanning now delegates to Git, whose configuration can enable executable helpers. The scan timeout also does not guarantee that helper processes terminate or that scanning returns promptly. Existing file-access controls remain intact, and exploitation would require control of effective Git configuration rather than merely a source file or ignore rule.

Retained concerns

  • Medium · security · inferred: Implicit scans can cross from workspace enumeration into configuration-driven execution. The new Git status invocation does not disable fsmonitor hooks and inherits the host environment. If an attacker controls effective Git configuration, a configured helper may execute with the scanning process's authority when context collection or MCP resource listing triggers Scan. Ordinary tracked source and .gitignore changes alone do not establish this prerequisite. Equivalent automatic Git execution already existed in some CLI paths, but it was not previously part of Scan's contract.
  • Medium · reliability · inferred: The five-second context bounds cancellation of the direct Git process, not completion of the entire process tree. A configured helper or descendant retaining an output descriptor can keep Output waiting after Git is killed, and descendants are not explicitly terminated. This can prevent fallback from being reached and strand context collection or MCP resource listing, weakening failure containment for the new execution path.
Security review details

Security Blast Radius

  • inferred — The conditional helper-execution exposure follows the scanning process's operating-system identity and inherited environment, not just the enumerated directory. A malicious configured helper could therefore affect resources accessible to that identity. Actual credentials, sandbox boundaries, tenant separation, and deployment privileges were not supplied, so broader cross-service or cross-tenant exposure is not established.

Security Findings and Attack Paths

  • inferred — A conditional attack path is effective Git configuration control, followed by an implicit Scan, configured fsmonitor execution, and use of the host process's authority. This was not reproduced for the exact command and supported Git-version range. A normal clone does not by itself establish attacker control of local Git configuration, and ordinary ignore rules do not supply executable arguments. Existing automatic CLI status execution predates the PR; the newly assessed exposure is propagation through Scan.

Trust Boundaries and Controls

  • observed — Fixed argv and absence of an application-level shell constrain argument injection. Optional index locking is disabled, existing scan traversal filters remain, and MCP read-time containment remains separate. The Git command construction does not override fsmonitor configuration or introduce a helper-specific sandbox.

Resilience and Maintainability Implications

  • inferred — Missing Git and ordinary command errors recover through fallback, but that recovery depends on Output returning. Default direct-process cancellation and unbounded pipe draining do not guarantee completion when descendants retain descriptors. This matters because scans run synchronously during context collection and resource listing.

Hardening Proposals

  • proposed — Make the scan's non-executing intent explicit by disabling configured fsmonitor execution and reviewing other configuration-driven helper paths while preserving intended ignore-rule sources. Bound pipe waiting and define platform-appropriate descendant cleanup. Validate those controls with configuration-driven helper and interruption cases rather than treating fixed argv or the context deadline as sufficient.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: Scan skips Git-ignored paths and build-cache directories without changing behavior for other tools.
Linked Issues check ✅ Passed [#1108] Scan now uses Git’s ignored paths inside a work tree and a scan-only cache-directory list when Git data is unavailable. This addresses ignored build output and common cache directories witho…
Out of Scope Changes check ✅ Passed All reported changes support [#1108]. The repomap fixture rename avoids using a directory that the new non-Git fallback skips. The BuildFromWorkspace documentation update describes the Git lookup …
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant