fix(fsx): test repository containment on a path boundary, not a string prefix

[dwin-gharibi]

ShouldIgnore decided whether a path was inside the repository with
strings.HasPrefix(absPath, m.repoRoot). repoRoot carries no trailing separator, so
/work/repo is a string prefix of /work/repo-sibling — and files in that sibling were matched
against the repository's .gitignore and silently reported as ignored.

Closes #3922.

The bug in one line

repoRoot is absolute, cleaned and symlink-resolved with no trailing separator
(findRepoRoot, vcs.go:121-140). A string prefix has no component boundary, so a foreign path
passes the containment check; filepath.Rel then yields ../repo-sibling/app.log, and those
components go straight to gitignore.Match, which matches basename patterns at any depth. A
pattern as ordinary as *.log matches the trailing component.

repoRoot = ".../001/repo"
ShouldIgnore(.../001/repo/app.log)          = true    correct
ShouldIgnore(.../001/other/app.log)         = false   correct
ShouldIgnore(.../001/repo-sibling/app.log) = true     <-- outside the repo

The other/ control is the part that matters: it isolates the shared string prefix as the
cause, rather than over-broad pattern matching.

The fix

filepath.Rel was already being called on the next line, and its output is the containment
answer. So the prefix test is deleted rather than patched — net one line of logic, and one
fewer path walk:

-	// Check if path is within this repository
-	if !strings.HasPrefix(absPath, m.repoRoot) {
-		return false
-	}
-
-	// Create a relative path from the repository root for matching
+	// Create a relative path from the repository root for matching. Rel doubles
+	// as the containment check: a path outside the repository climbs out with
+	// "..", which a string-prefix test would miss for a sibling directory whose
+	// name merely starts with the root's ("<root>-sibling").
 	relPath, err := filepath.Rel(m.repoRoot, absPath)
 	if err != nil {
 		return false
 	}
+	if relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
+		return false
+	}

This is the idiom the codebase already uses for exactly this question in five other places —
pkg/fsx/collect.go:131, pkg/tools/builtin/skills/skills.go:179,
pkg/sandbox/kit/kit.go:592, pkg/acp/filesystem.go:157, pkg/path/display.go:67 — so this
brings VCSMatcher in line rather than inventing anything.

Behaviour for the repository root itself is deliberately unchanged: Rel(root, root) is ".",
which is neither ".." nor ".."-prefixed, so it still falls through to the matcher exactly as
before. There's a test pinning that, because it's the one case where "reject anything that isn't
strictly inside" would have been a silent behaviour change.

Why it mattered

ignore_vcs defaults to true (filesystem.go:192), and ShouldIgnore gates two
independent subsystems:

Subsystem	Callsite
directory_tree	filesystem.go:941 (into fsx.DirectoryTree)
list_directory	filesystem.go:1062
search_files_content	filesystem.go:1428
RAG bm25 / semantic / chunked embeddings	pkg/rag/strategy/helpers.go:257

The filesystem toolset takes multiple allow-list roots (WithAllowList, :150), so an agent
configured with /work/repo and /work/repo-data had the first one's .gitignore applied
to the second.

The failure mode was silent omission, which is what made it expensive: no error, no warning.
In search_files_content a matching directory returns fs.SkipDir, so a whole subtree could
disappear and the agent would read a successful empty result as "this content does not exist".

Tests

Two subtests added to TestNewVCSMatcher in pkg/fsx/vcs_test.go:

• sibling directory sharing the root name prefix is outside the repository — the
  regression. Asserts all three cases together: a matching file inside the repo is still
  ignored, the repo-sibling file is not, and an unrelated other/ file is not. Including the
  in-repo assertion is deliberate — a fix that over-corrected into "ignore nothing" would pass a
  test that only checked the sibling.
• repository root itself is matched against its own patterns — pins the Rel == "."
  behaviour described above, so a future tightening of the boundary check can't silently change
  it.

Written test-first. Confirmed the new subtest fails on unpatched code for the right reason —
exactly one assertion (vcs_test.go:191, the sibling case), with the control and in-repo
assertions passing:

--- FAIL: TestNewVCSMatcher/sibling_directory_sharing_the_root_name_prefix_is_outside_the_repository
    vcs_test.go:191: Should be false
        Messages: a file in a sibling directory is outside the repository

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/fsx/...	ok
go test -race -count=1 ./pkg/fsx/...	ok
go test ./pkg/tools/builtin/filesystem/...	ok
go test ./pkg/rag/...	ok (all subpackages)
go build ./...	clean
go vet ./pkg/fsx/	clean
gofmt -l pkg/fsx/	no output
go test ./... (full suite, .env.test loaded)	only pkg/teamloader fails — pre-existing, see below

Scope

Deliberately one bug, one package. The containment fix is independent of the other defects in
this area, and keeping it isolated makes it easy to review and to cherry-pick.

[dwin-gharibi]

@Sayt-0

[aheritier]

Reviewed at 9d72e46ffa7e152c6384cff009f23da6384054ad. Commenting rather than approving for one reason only: CI has not run on this head SHA — all three workflow runs report conclusion=action_required (fork PR awaiting maintainer authorization), check-runs total is 0, and the combined status is pending with an empty statuses array. A maintainer needs to approve the workflows; I can't approve without green CI. mergeStateStatus=BLOCKED is REVIEW_REQUIRED, not a conflict — mergeable=MERGEABLE, both commits GPG-verified, and although the branch is 7 commits behind main, git log <base>..origin/main -- pkg/fsx/ is empty, so there is no drift in the touched package.

The change itself is correct and I have no blocking objections. Deleting the prefix test and deriving containment from the already-computed filepath.Rel is the right call, and the boundary check is exhaustive for Rel's output on every platform: path/filepath/path.go:242-245 builds the escape prefix as ".." joined by Separator, and sameWord is strings.EqualFold on Windows (path_windows.go:116) vs == on Unix (path_unix.go:42), so Windows case-insensitivity and the cross-volume error path (err != nil → return false) are both handled. It also matches the existing idiom at pkg/acp/filesystem.go:157 and pkg/path/display.go:67.

I verified the fix cannot hide anything new: on Unix the new accept-set is a strict subset of the old one, so this can only un-hide files. I probed trailing separators (ShouldIgnore(dir+"/build/") still ignored, root+"/" still not), deep sibling subtrees, nonexistent sibling files (where EvalSymlinks fails and absPath stays unresolved — still correctly rejected), symlinked root aliases (correctly resolve inside), and in-repo symlinks pointing outside (unchanged from base). No regressions.

Since CI hasn't run, I reproduced it locally at this SHA on Go 1.26.5/darwin-arm64: go test -race -count=1 ./pkg/fsx/... ok; ./pkg/tools/builtin/filesystem/... ./pkg/rag/... ./pkg/path/... ./pkg/acp/... all ok; golangci-lint run pkg/fsx/... (v2.12.2, the version CI pins) 0 issues; gofmt -l pkg/fsx/ clean; GOOS={windows,linux,darwin} build and GOOS=windows go vet clean.

Mutation testing confirms the tests are real. Reverting pkg/fsx/vcs.go to the base commit while keeping the new tests fails exactly one assertion at exactly vcs_test.go:191:

--- FAIL: TestNewVCSMatcher/sibling_directory_sharing_the_root_name_prefix_is_outside_the_repository
    vcs_test.go:191: Should be false
        Messages: a file in a sibling directory is outside the repository

The in-repo and other/ assertions pass on the old code, so they are genuine controls against an over-correction into "ignore nothing" — not padding. Thanks for writing it test-first and for the other/ control; it's what isolates the shared prefix as the cause.

Two non-blocking notes, both about pinning behaviour the code already gets right:

[optional] The precise ".." boundary isn't pinned by a test. Replacing the check with the naive strings.HasPrefix(relPath, "..") leaves the whole package green (ok github.com/docker/docker-agent/pkg/fsx 0.685s), yet that mutation is a real bug — a directory named ..cache inside the repo silently stops being matched:

repo/.gitignore = "*.log"
repo/..cache/app.log  ->  want true, naive-prefix gives false

Your implementation handles this correctly; it's only unguarded. A third assertion in the existing subtest — a ..cache/app.log file expected true — would lock in why the check is relPath == ".." and not a bare HasPrefix, which is the one detail a future refactor is most likely to "simplify" into a regression.

[optional] The root subtest doesn't pin what the description says it pins. Adding relPath == "." to the rejection (i.e. over-tightening into "reject anything not strictly inside") also leaves the suite green — repository root itself is matched against its own patterns passes either way, because both implementations return false for the root. The subtest is still worth keeping: fs.WalkDir visits the root and pkg/fsx/fs.go:204-208 converts a true into fs.SkipDir, which would silently blank the entire tree. But it asserts "the root is not ignored" rather than "the root reaches the matcher," so the PR description's claim that it guards against a future tightening of the boundary check is stronger than what the test actually does. Worth either softening that sentence or strengthening the assertion.

Separately, and explicitly not a finding against this PR: ShouldIgnore returns false for a nonexistent in-repo path on macOS, because EvalSymlinks fails on missing paths and leaves absPath as /var/... while repoRoot is /private/var/.... I confirmed this behaves identically on the base commit, so it is pre-existing and correctly outside this PR's scope. pkg/acp/filesystem.go already solves it with evalSymlinksAllowMissing (walk up to an existing ancestor, resolve, re-append the tail) if someone wants a follow-up.

Docs are fine as-is: docs/tools/filesystem/index.md:65 already describes the intended behaviour, so this fix makes the code match the docs rather than requiring a doc change.

Happy to approve as soon as a maintainer authorizes the workflows and ci reports green — no code changes needed on my account.

[docker-agent]

Assessment: 🟢 APPROVE

The fix is correct and well-scoped. The filepath.Rel-based containment check properly addresses the sibling-directory prefix-sharing bug. No bugs were introduced by this PR's changes.