From 1f23b37c088379ffd7613b54f327daf7bf62940b Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 26 Jul 2026 05:06:04 +0100 Subject: [PATCH 1/2] fix: bound, speed, and widen the probe's recursive file walk WalkFiles walked a foreign source tree with fs.WalkDir over os.Root.FS(), which had three defects the itd-95/itd-96 reviews raised: - iss-112: every directory was read with ReadDir(-1), so one directory of millions of entries ballooned memory before the file cap could apply. Each directory is now read with a bounded ReadDir, sharing maxDirEntries as the one canonical per-directory guard ListDir already uses. - iss-114: os.Root.FS() re-resolved every path from the containment root one component at a time, so a walk cost O(entries x depth) opens (measured 10.5s for a 60000-dir depth-30 tree). The walk now holds a sub-root per directory (os.Root.OpenRoot) and opens each child in O(1), so cost is O(entries) and independent of depth: a 48000-dir depth-30 tree walks in ~1.4s versus ~7s. The os.Root containment guarantee is unchanged - a symlink is still refused, never followed out of the tree, verified from a sub-root at depth. - iss-116: the skip set covered only Node and Go. It now also skips the common Python (.venv, venv, .tox, __pycache__), build/distribution (target, build, dist), and CocoaPods (Pods) trees, so a vendored TODO is not cited as the project's own open question and a large dot-prefixed dependency tree cannot exhaust the walk cap before the project's own src/ is reached. The depth-cap, directory-cap, file-cap, skip, and containment semantics are preserved; the pre-existing walk tests pin them unchanged. spc-12's skip-set and walk-cost language is amended in the same change. Assisted-by: Claude:claude-opus-4-8 --- ...ifeboat-s-open-questions-on-a-repo-s-to.md | 48 ++- CHANGELOG.md | 20 + internal/core/lifeboat/probe.go | 217 +++++++---- internal/core/lifeboat/probe_walk_test.go | 341 ++++++++++++++++++ 4 files changed, 538 insertions(+), 88 deletions(-) create mode 100644 internal/core/lifeboat/probe_walk_test.go diff --git a/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md b/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md index 62e6fda..a268be2 100644 --- a/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md +++ b/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md @@ -42,27 +42,37 @@ falling through to a blank a rescuer has to fill from nothing. `fs.WalkDir(root.FS(), …)` walk — rather than inventing a second traversal idiom. -- **Containment.** The walk runs over `c.root.FS()`, so `os.Root` refuses any - component that escapes the repository root, symlinked intermediates included. - A `nil` root (unopenable repository) returns `(nil, false)`. +- **Containment.** The walk descends through a sub-root per directory + (`os.Root.OpenRoot`); `os.Root` refuses any component that escapes the + repository root, symlinked intermediates included, and a symlinked directory + is detected from its `ReadDir` type and skipped before it is ever opened. The + containment property holds unchanged while each child opens in O(1). A `nil` + root (unopenable repository) returns `(nil, false)`. - **Symlinks are skipped, never followed.** Unlike embark — where a symlink in a packed lifeboat is a trust violation and therefore fatal — a probe reads an arbitrary foreign tree where symlinks are ordinary. The walk skips symlinked entries (files and directories) and continues; it never errors on one. -- **Skip set.** Directory names never descended into: `.git`, `node_modules`, - `vendor`, `generated`. These are dependency, VCS-internal, and generated - trees — never a team's own open questions, and the dominant cost of an - unfiltered walk. -- **Caps, in three dimensions.** `maxWalkFiles` mirrors `maxDirEntries` - (50 000) and bounds regular files *and* directories visited: a tree of - directories holding nothing regular yields no path, so a file cap alone never - fires there and the walk would run to exhaustion over a foreign tree. - `maxWalkDepth` (32) bounds descent, because `os.Root` resolves every - directory from the containment root one component at a time — a chain of - directories costs the square of its depth, and a few thousand of them are - trivial to create and take minutes to traverse. Real trees are shallow, so - the depth cap prunes only pathological chains, and prunes the chain rather - than abandoning the tree. Any bound firing returns `truncated = true`. +- **Skip set.** Directory names never descended into, matched by name at any + depth: `.git` (VCS); `node_modules` (Node); `vendor`, `generated` + (Go/generic); `.venv`, `venv`, `.tox`, `__pycache__` (Python); `target`, + `build`, `dist` (build/distribution output); `Pods` (CocoaPods). These are + dependency, VCS-internal, language-cache, and generated trees across the + common ecosystems — never a team's own open questions, and the dominant cost + of an unfiltered walk; walked as source, a vendored `TODO` is cited as this + project's own open question. +- **Caps.** `maxWalkFiles` mirrors `maxDirEntries` (50 000) and bounds regular + files *and* directories visited: a tree of directories holding nothing regular + yields no path, so a file cap alone never fires there and the walk would run to + exhaustion over a foreign tree. `maxDirEntries` (50 000) is also the + per-directory read bound: each directory is read with a bounded `ReadDir` — the + one canonical guard `ListDir` uses — so a single directory of millions of + entries cannot balloon memory before the file cap applies. `maxWalkDepth` (32) + bounds descent: the walk holds a sub-root per directory (`os.Root.OpenRoot`), + so each child opens in O(1) and a chain costs O(depth) rather than the square + of its depth, but an unbounded chain is still an unbounded recursion and a + pathological cost, so the cap prunes it — pruning the chain rather than + abandoning the tree. Real trees are shallow. Any bound firing returns + `truncated = true`. Truncation is *reported*, never silent (loud-staging): an adapter that hits a cap says so in its cited evidence, and a blank drawn from a truncated walk says the walk was truncated. @@ -134,8 +144,8 @@ The primitive therefore adds no second read path to audit. | Which markers? | `TODO`, `FIXME`, `XXX`, `HACK`, `BUG`; uppercase only; word-boundary anchored, trailing `:`/`(`/space/EOL. `NOTE`, `OPTIMIZE` excluded. | | Which tier — conventions or git? | **Conventions.** A working-tree file scan through the `SourceContext` file surface. The adapter never touches git, so it grounds a bare snapshot as readily as a working tree. | | Scan scope and the missing primitive | Option (a): add a bounded recursive-walk primitive, `WalkFiles`, to `SourceContext`. It is shared with itd-96, so the walk lands once. | -| Which files to scan | Every regular file the walk yields, minus the skip set (`.git`, `node_modules`, `vendor`, `generated`), minus symlinks, minus binaries (NUL-byte heuristic), minus oversized files (`ReadFile`'s cap). | -| Per-repo caps | `maxWalkFiles` = 50 000 files **and** 50 000 directories (mirrors `maxDirEntries`), `maxWalkDepth` = 32 levels of descent, `maxProbeReadBytes` per file, `maxMarkerScanBytes` = 512 MiB across the scan (reuses `maxPlanTotalBytes`), `maxMarkerCitations` = 200 citations. Every bound that fires is reported in the cited evidence. | +| Which files to scan | Every regular file the walk yields, minus the skip set (`.git`, `node_modules`, `vendor`, `generated`, `.venv`, `venv`, `.tox`, `__pycache__`, `target`, `build`, `dist`, `Pods`), minus symlinks, minus binaries (NUL-byte heuristic), minus oversized files (`ReadFile`'s cap). | +| Per-repo caps | `maxWalkFiles` = 50 000 files **and** 50 000 directories (mirrors `maxDirEntries`), `maxDirEntries` = 50 000 entries read per directory, `maxWalkDepth` = 32 levels of descent, `maxProbeReadBytes` per file, `maxMarkerScanBytes` = 512 MiB across the scan (reuses `maxPlanTotalBytes`), `maxMarkerCitations` = 200 citations. Every bound that fires is reported in the cited evidence. | | Output shape and framing | Headline count + up to 200 `path:line (MARKER)` citations, `dedupeSorted`. | | Dedup | `dedupeSorted` on the rendered citation string, so an identical `path:line (MARKER)` appears once. Multiple distinct markers in one file each keep their own line. | | Status and confidence thresholds | Ceiling `StatusPartial`. `ConfidenceMedium` at ≥ 10 markers, else `ConfidenceLow`. Never `StatusGrounded`. | diff --git a/CHANGELOG.md b/CHANGELOG.md index d768733..12b296b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,26 @@ called out in a **Breaking** section. user-facing by definition). `capture wontfix` is unchanged — a non-action ships nothing, so `wontfix/` carries no impact. +### Fixed + +- **The disembark probe's recursive file walk is bounded per directory, opens + each child in O(1), and skips the common ecosystems' dependency trees** + (iss-112, iss-114, iss-116). The walk now reads every directory with a bounded + `ReadDir` (the same 50 000-entry guard `ListDir` uses), so a single directory + of millions of entries can no longer balloon memory before the file cap + applies. It holds a sub-root per directory (`os.Root.OpenRoot`) instead of + re-resolving every path from the containment root one component at a time, so a + deep tree costs O(entries) rather than O(entries × depth) — a 48 000-directory + depth-30 tree walks in ~1.4 s where the old walk took ~7 s, and the cost is now + independent of depth. The `os.Root` containment guarantee is unchanged: a + symlink is still refused rather than followed out of the tree. The skip set + widens beyond Node and Go to the common dependency, cache, and build-output + trees — Python (`.venv`, `venv`, `.tox`, `__pycache__`), Rust and generic + build output (`target`, `build`, `dist`), and CocoaPods (`Pods`) — so a + vendored `TODO` is no longer cited as the project's own open question, and a + large dot-prefixed dependency tree can no longer exhaust the walk cap before + the project's own `src/` is reached. + ## [0.4.0] - 2026-07-22 ### Breaking diff --git a/internal/core/lifeboat/probe.go b/internal/core/lifeboat/probe.go index 70aa4ec..38d1cd3 100644 --- a/internal/core/lifeboat/probe.go +++ b/internal/core/lifeboat/probe.go @@ -30,9 +30,10 @@ const maxProbeReadBytes = 4 << 20 // 4 MiB // repo cannot exhaust memory through a read-only command. const maxGitOutputBytes = 16 << 20 // 16 MiB -// maxDirEntries caps how many entries ListDir returns from one directory, so a +// maxDirEntries caps how many entries a single directory read materialises, so a // directory with millions of files cannot exhaust memory when the probe indexes -// it. +// it. It is the one canonical per-directory bound, shared by ListDir and by +// WalkFiles' per-directory read (readDirBounded) — not a second constant. const maxDirEntries = 50000 // maxWalkFiles caps how many regular files WalkFiles returns from one walk, and @@ -43,20 +44,27 @@ const maxDirEntries = 50000 // adapter can say in its evidence that it saw only part of the tree. const maxWalkFiles = maxDirEntries -// maxWalkDepth caps how many levels below its start WalkFiles descends. Every -// directory is opened by resolving its path from the containment root one -// component at a time, so a chain of directories costs the square of its depth -// to walk: a few thousand nested directories are trivial to create and take -// minutes to traverse. Real trees are shallow — the deepest path in this -// repository is six levels — so the cap prunes only the pathological ones, and -// says so when it does. +// maxWalkDepth caps how many levels below its start WalkFiles descends. The walk +// holds a sub-root per directory (os.Root.OpenRoot), so each descent opens one +// component in O(1) and a chain costs O(depth) rather than the square of its +// depth — but an unbounded chain is still an unbounded recursion and a +// pathological cost, so the cap prunes it. Real trees are shallow — the deepest +// path in this repository is six levels — so the cap prunes only the +// pathological ones, and says so when it does. const maxWalkDepth = 32 // walkSkipDirs are the directory names WalkFiles never descends into, matched by -// name at any depth: VCS internals, dependency trees, and generated code. None -// of them holds a team's own material, and together they are the dominant cost -// of an unfiltered walk. -var walkSkipDirs = []string{".git", "generated", "node_modules", "vendor"} +// name at any depth: VCS internals, dependency trees, language caches, and +// build/distribution output across the common ecosystems. None of them holds a +// team's own material, and together they are the dominant cost of an unfiltered +// walk — and, walked as if they were source, the origin of a vendored TODO cited +// as this project's own open question. Covered: VCS (.git); Node (node_modules); +// Go/generic (vendor, generated); Python (.venv, venv, .tox, __pycache__); Rust +// and generic build output (target, build, dist); CocoaPods (Pods). +var walkSkipDirs = []string{ + ".git", ".tox", ".venv", "Pods", "__pycache__", "build", "dist", + "generated", "node_modules", "target", "vendor", "venv", +} // Confidence qualifies a non-blank status: how sure the adapter is that the // evidence it cites actually grounds the section. It is meaningless for a blank. @@ -295,14 +303,17 @@ func (c *SourceContext) ListDir(rel string) []string { // WalkFiles returns the repo-relative POSIX paths of every regular file beneath // a repo-relative directory, sorted, and reports whether the walk stopped at any -// of its bounds — the file cap, the directory cap, or the depth cap. It is the -// recursive counterpart of ListDir, for adapters whose evidence is the shape of -// the tree rather than a known filename. Content is still read through ReadFile, -// so the walk adds no second read path. +// of its bounds — the file cap, the directory cap, the depth cap, or a single +// directory exceeding the per-directory read bound. It is the recursive +// counterpart of ListDir, for adapters whose evidence is the shape of the tree +// rather than a known filename. Content is still read through ReadFile, so the +// walk adds no second read path. // -// It is contained by the same os.Root as every other read, skips the -// walkSkipDirs trees, and skips non-regular files (FIFOs, devices, sockets) so -// no path it yields can block on open. Symlinks — file or directory — are +// It is contained by the same os.Root as every other read, descending through a +// sub-root per directory so each child opens in O(1) while the containment +// property holds; it skips the walkSkipDirs trees, and skips non-regular files +// (FIFOs, devices, sockets) so no path it yields can block on open. Symlinks — +// file or directory — are // SKIPPED and the walk continues. This deliberately differs from embark's // walkLifeboatFiles, where a symlink is a trust violation in a packed lifeboat // and therefore fatal: a probe reads an arbitrary foreign tree in which a @@ -312,11 +323,32 @@ func (c *SourceContext) WalkFiles(rel string) (paths []string, truncated bool) { return c.walkFilesLimited(rel, maxWalkFiles) } -// walkFilesLimited is WalkFiles with the file and directory cap injected, so the -// truncation branches are exercisable by a test at an affordable scale. The -// shipped cap stays a const: adapters run concurrently, and a mutable -// package-level cap would be shared state between them. +// walkFilesLimited is WalkFiles with the whole-walk file-and-directory cap +// injected, so the truncation branches are exercisable by a test at an +// affordable scale. The shipped cap stays a const: adapters run concurrently, +// and a mutable package-level cap would be shared state between them. func (c *SourceContext) walkFilesLimited(rel string, limit int) (paths []string, truncated bool) { + return c.walkFilesBounded(rel, limit, maxDirEntries) +} + +// walkFilesBounded is WalkFiles with both bounds injected — the whole-walk cap +// (limit: regular files and directories) and the per-directory read bound +// (perDir) — so each is exercisable by a test at an affordable scale. +// +// It holds a sub-root per directory: each child directory is opened with +// os.Root.OpenRoot from its parent's already-open handle, so a descent opens one +// component relative to that directory (O(1)) rather than re-resolving the whole +// path from the containment root on every open (O(depth)). A chain of +// directories therefore costs O(entries), not O(entries × depth). Each directory +// is read with a bounded ReadDir(perDir) — the same guard ListDir uses — so a +// single directory of millions of entries cannot balloon memory before the file +// cap applies. +// +// The os.Root containment guarantee the FS() walk had survives unchanged: +// OpenRoot refuses any component that escapes the root, and a symlinked +// directory is detected from its ReadDir type and skipped before it is ever +// opened, so no symlink is ever followed out of the tree. +func (c *SourceContext) walkFilesBounded(rel string, limit, perDir int) (paths []string, truncated bool) { if c.root == nil { return nil, false } @@ -324,66 +356,113 @@ func (c *SourceContext) walkFilesLimited(rel string, limit int) (paths []string, if !fs.ValidPath(start) { return nil, false } - startDepth := pathDepth(start) - dirs := 0 - err := fs.WalkDir(c.root.FS(), start, func(p string, d fs.DirEntry, err error) error { + startRoot := c.root + if start != "." { + r, err := c.root.OpenRoot(filepath.FromSlash(start)) if err != nil { - // An unreadable entry in a foreign tree is skipped, not fatal: the - // probe reports what it could read. - return nil + return nil, false } - if d.Type()&fs.ModeSymlink != 0 { - // Never followed. A symlinked directory arrives here as a symlink - // entry, not a directory, so returning nil skips it alone — fs.SkipDir - // would skip the rest of its parent. - return nil + defer r.Close() + startRoot = r + } + + dirs := 1 // the start directory itself counts against the directory cap + var walk func(dirRoot *os.Root, prefix string, depth int) (stop bool) + walk = func(dirRoot *os.Root, prefix string, depth int) bool { + entries, more := readDirBounded(dirRoot, perDir) + if more { + // The directory held more entries than the per-directory bound: only + // the bound was materialised, exactly as ListDir bounds one listing. + truncated = true } - if d.IsDir() { - for _, skip := range walkSkipDirs { - if path.Base(p) == skip { - return fs.SkipDir + for _, e := range entries { + name := e.Name() + child := name + if prefix != "." { + child = prefix + "/" + name + } + if e.Type()&fs.ModeSymlink != 0 { + // Never followed — skipped alone, and the walk continues. + continue + } + if e.IsDir() { + if isSkipDir(name) { + continue + } + // Directories are capped alongside files: a tree of directories + // holding nothing regular yields no path, so a file cap alone never + // fires and the walk would run to exhaustion over a foreign tree. + if dirs >= limit { + truncated = true + return true + } + dirs++ + if depth+1 >= maxWalkDepth { + // Prune the chain, not the tree: the directory is counted but + // not descended into, and the truncation is reported either way. + truncated = true + continue } + sub, err := dirRoot.OpenRoot(name) + if err != nil { + // Unreadable (or vanished) in a foreign tree: skip it and report + // only what could be read. + continue + } + stop := walk(sub, child, depth+1) + sub.Close() + if stop { + return true + } + continue } - // Directories are capped alongside files: a tree of directories - // holding nothing regular yields no path, so a file cap alone never - // fires and the walk runs to exhaustion over a foreign tree. - if dirs >= limit { - truncated = true - return fs.SkipAll + if !e.Type().IsRegular() { + continue } - dirs++ - if pathDepth(p)-startDepth >= maxWalkDepth { - // Prune the chain, not the tree: everything above the cap is - // still walked, and the truncation is reported either way. + if len(paths) >= limit { truncated = true - return fs.SkipDir + return true } - return nil + paths = append(paths, child) } - if !d.Type().IsRegular() { - return nil - } - if len(paths) >= limit { - truncated = true - return fs.SkipAll - } - paths = append(paths, p) - return nil - }) - if err != nil { - return nil, false + return false } + walk(startRoot, start, 0) sort.Strings(paths) return paths, truncated } -// pathDepth counts the segments in a cleaned slash path, with "." the zero -// depth. It is how the walk measures how far below its start it has descended. -func pathDepth(p string) int { - if p == "." { - return 0 +// readDirBounded reads at most bound entries from the directory dirRoot points +// at, sorted by name for a deterministic walk, and reports whether the directory +// held more than bound. It materialises at most bound+1 entries, so a directory +// of millions cannot balloon memory here — the shared per-directory guard ListDir +// applies with ReadDir(maxDirEntries). +func readDirBounded(dirRoot *os.Root, bound int) (entries []fs.DirEntry, more bool) { + f, err := dirRoot.Open(".") + if err != nil { + return nil, false + } + defer f.Close() + // Read one past the bound so "more remain" is detectable in a single call + // without ever materialising the whole listing. + entries, _ = f.ReadDir(bound + 1) + if len(entries) > bound { + more = true + entries = entries[:bound] + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + return entries, more +} + +// isSkipDir reports whether a directory of this name is one WalkFiles never +// descends into — matched by name at any depth. +func isSkipDir(name string) bool { + for _, s := range walkSkipDirs { + if name == s { + return true + } } - return strings.Count(p, "/") + 1 + return false } // firstRootSHA returns the canonical (first) root-commit SHA, or "". diff --git a/internal/core/lifeboat/probe_walk_test.go b/internal/core/lifeboat/probe_walk_test.go new file mode 100644 index 0000000..4ec9762 --- /dev/null +++ b/internal/core/lifeboat/probe_walk_test.go @@ -0,0 +1,341 @@ +package lifeboat + +import ( + "fmt" + "io/fs" + "os" + "path" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestWalkFilesBoundsEntriesReadPerDirectory is the iss-112 property: the walk +// must not materialise a whole directory listing before its file cap applies. A +// single directory holding far more entries than the per-directory bound is read +// only up to that bound — the same guard ListDir already enforces with +// ReadDir(maxDirEntries) — and reaching it is reported as truncation, never +// silent. Without the guard fs.WalkDir calls ReadDir(-1) and one directory of +// millions of entries balloons memory before any file cap fires. +func TestWalkFilesBoundsEntriesReadPerDirectory(t *testing.T) { + dir := t.TempDir() + files := map[string]string{} + for i := 0; i < 20; i++ { + files[fmt.Sprintf("f%02d.txt", i)] = "x\n" + } + writeTree(t, dir, files) + ctx, err := newSourceContext(dir) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + // A per-directory bound of 5 over a 20-entry directory: at most 5 entries are + // read from it, and the walk reports it stopped short. + paths, truncated := ctx.walkFilesBounded(".", 1000, 5) + if !truncated { + t.Errorf("walk of a 20-entry directory under a 5-entry per-directory bound reported no truncation") + } + if len(paths) > 5 { + t.Errorf("walk returned %d files from a directory read under a 5-entry bound, want <= 5 (full listing materialised)", len(paths)) + } + + // A directory under the per-directory bound is read whole and not truncated. + small := t.TempDir() + writeTree(t, small, map[string]string{"a.txt": "x\n", "b.txt": "x\n", "c.txt": "x\n"}) + ctx2, err := newSourceContext(small) + if err != nil { + t.Fatal(err) + } + defer ctx2.Close() + paths2, truncated2 := ctx2.walkFilesBounded(".", 1000, 5) + if truncated2 || len(paths2) != 3 { + t.Errorf("walk of 3 files under a 5-entry bound = %d files, truncated=%v; want 3 untruncated", len(paths2), truncated2) + } +} + +// TestWalkFilesSkipsBroadDependencyTrees is the iss-116 property: the skip set +// covers the common ecosystems' dependency, cache, and build-output trees — not +// just Node and Go — so none of them is walked as if it were the team's own +// source. Each planted marker sits in a tree that must be skipped; only the real +// src/ file survives the walk. +func TestWalkFilesSkipsBroadDependencyTrees(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + "src/keep.go": "package src\n\n// TODO: keep me\n", + ".venv/lib/dep.py": "# TODO: python virtualenv\n", + "venv/lib/dep.py": "# TODO: bare python virtualenv\n", + ".tox/py311/dep.py": "# TODO: tox environment\n", + "pkg/__pycache__/m.pyc": "# TODO: python bytecode cache\n", + "target/debug/build.rs": "// TODO: rust build output\n", + "dist/bundle.js": "// TODO: distribution output\n", + "build/out.o": "// TODO: build output\n", + "Pods/Alamofire/net.swift": "// TODO: cocoapods dependency\n", + }) + ctx, err := newSourceContext(dir) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + paths, _ := ctx.WalkFiles(".") + if got := strings.Join(paths, ","); got != "src/keep.go" { + t.Errorf("WalkFiles = %v, want only [src/keep.go]; skip set is %v", paths, walkSkipDirs) + } +} + +// TestOpenQuestionsAdapterIgnoresVendoredMarkers is the iss-116 first +// consequence: the open-questions adapter must not cite a vendored dependency's +// TODO as this project's own open question. A record-less repo carrying markers +// in dependency trees and one real marker in src/ grounds the section on src/ +// alone. +func TestOpenQuestionsAdapterIgnoresVendoredMarkers(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + "src/app.go": "package app\n\n// TODO: wire the retry path\n", + ".venv/lib/dep.py": "# TODO: not our question\n", + "__pycache__/m.pyc": "# FIXME: not our question\n", + "target/debug/b.rs": "// XXX: not our question\n", + "dist/bundle.js": "// HACK: not our question\n", + "build/out.o": "// BUG: not our question\n", + "Pods/Lib/net.swift": "// TODO: not our question\n", + }) + ctx, err := newSourceContext(dir) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + ev := convOpenQuestionsSource{}.Probe(ctx) + if ev.Status != StatusPartial { + t.Fatalf("status = %q, want partial (one real marker in src/)", ev.Status) + } + joined := strings.Join(ev.Sources, "\n") + if !strings.Contains(joined, "src/app.go:") { + t.Errorf("evidence does not cite the real src/ marker:\n%s", joined) + } + for _, vendored := range []string{".venv", "__pycache__", "target", "dist", "build", "Pods"} { + if strings.Contains(joined, vendored+"/") { + t.Errorf("evidence cites a vendored marker under %q:\n%s", vendored, joined) + } + } +} + +// TestWalkFilesDotPrefixedTreeDoesNotEatTheCapBeforeSrc is the iss-116 second +// consequence: because fs.WalkDir reads entries in lexical order, a large +// dot-prefixed dependency tree (.venv sorts before src) could consume the whole +// file cap before the project's own src/ was reached. Skipping the dependency +// tree means the cap is spent on real source. The pre-fix behaviour is +// reproduced through the baseline walk, and killed by the shipped skip set. +func TestWalkFilesDotPrefixedTreeDoesNotEatTheCapBeforeSrc(t *testing.T) { + dir := t.TempDir() + files := map[string]string{"src/real.go": "package src\n\n// TODO: the real question\n"} + for i := 0; i < 20; i++ { + files[fmt.Sprintf(".venv/lib/dep%02d.py", i)] = "# TODO: dependency noise\n" + } + writeTree(t, dir, files) + ctx, err := newSourceContext(dir) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + // Pre-fix reproduction: the old walk over the old (Node+Go only) skip set + // walks .venv, and a 5-file cap is exhausted by the lexically-first entries + // before src/ is reached. + oldSkip := []string{".git", "generated", "node_modules", "vendor"} + prePaths, preTrunc := walkFilesRootFSBaseline(ctx, ".", 5, oldSkip) + if !preTrunc { + t.Fatalf("baseline pre-fix walk did not truncate under a 5-file cap; fixture too small to reproduce iss-116") + } + if containsPath(prePaths, "src/real.go") { + t.Fatalf("baseline pre-fix walk reached src/real.go; fixture does not reproduce the cap-eating consequence") + } + + // Post-fix: the shipped skip set excludes .venv, so the cap is never spent on + // the dependency tree and src/ is reached. + postPaths, _ := ctx.walkFilesBounded(".", 5, maxDirEntries) + if !containsPath(postPaths, "src/real.go") { + t.Errorf("post-fix walk did not reach src/real.go under the cap: %v", postPaths) + } + for _, p := range postPaths { + if strings.HasPrefix(p, ".venv/") { + t.Errorf("post-fix walk cited a .venv path %q; the dependency tree was walked", p) + } + } +} + +// TestWalkFilesSubRootContainmentAtDepth strengthens the containment property +// for the sub-root rewrite (iss-114): a symlink that escapes the root must be +// refused even when it sits several directories deep, where the walk holds a +// sub-root per directory rather than resolving every path from the top. The +// os.Root containment guarantee must survive the cost change. +func TestWalkFilesSubRootContainmentAtDepth(t *testing.T) { + base := t.TempDir() + repo := filepath.Join(base, "repo") + outside := filepath.Join(base, "outside") + writeTree(t, repo, map[string]string{ + "a/b/c/inside.go": "package c\n\n// TODO: deep inside\n", + }) + writeTree(t, outside, map[string]string{"secret.txt": "// TODO: escaped\n"}) + // A symlink three directories deep pointing outside the containment root. + if err := os.Symlink(filepath.Join(outside, "secret.txt"), filepath.Join(repo, "a", "b", "c", "escape.txt")); err != nil { + t.Fatal(err) + } + // A symlinked directory three levels deep pointing outside the root. + if err := os.Symlink(outside, filepath.Join(repo, "a", "b", "c", "escape-dir")); err != nil { + t.Fatal(err) + } + + ctx, err := newSourceContext(repo) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + paths, _ := ctx.WalkFiles(".") + if got := strings.Join(paths, ","); got != "a/b/c/inside.go" { + t.Fatalf("WalkFiles = %v, want only [a/b/c/inside.go]; a deep symlink escaped the sub-root containment", paths) + } +} + +// containsPath reports whether paths contains p. +func containsPath(paths []string, p string) bool { + for _, x := range paths { + if x == p { + return true + } + } + return false +} + +// pathDepth counts the segments in a cleaned slash path, with "." the zero +// depth — how the baseline walk measured how far below its start it had +// descended. It is test-only: the shipped walk now tracks depth as a passed-down +// counter, so this lives beside the baseline that still needs it. +func pathDepth(p string) int { + if p == "." { + return 0 + } + return strings.Count(p, "/") + 1 +} + +// walkFilesRootFSBaseline is the pre-fix walk preserved for measurement and for +// reproducing the iss-116 cap-eating consequence: it drives fs.WalkDir over +// c.root.FS(), which re-resolves every path from the containment root one +// component at a time (the O(entries x depth) cost iss-114 removes) and reads +// every directory with ReadDir(-1) (the unbounded per-directory read iss-112 +// removes). It is test-only and never shipped. +func walkFilesRootFSBaseline(c *SourceContext, rel string, limit int, skip []string) (paths []string, truncated bool) { + if c.root == nil { + return nil, false + } + start := path.Clean(filepath.ToSlash(rel)) + if !fs.ValidPath(start) { + return nil, false + } + startDepth := pathDepth(start) + dirs := 0 + _ = fs.WalkDir(c.root.FS(), start, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + if d.Type()&fs.ModeSymlink != 0 { + return nil + } + if d.IsDir() { + for _, s := range skip { + if path.Base(p) == s { + return fs.SkipDir + } + } + if dirs >= limit { + truncated = true + return fs.SkipAll + } + dirs++ + if pathDepth(p)-startDepth >= maxWalkDepth { + truncated = true + return fs.SkipDir + } + return nil + } + if !d.Type().IsRegular() { + return nil + } + if len(paths) >= limit { + truncated = true + return fs.SkipAll + } + paths = append(paths, p) + return nil + }) + sort.Strings(paths) + return paths, truncated +} + +// buildDeepTree writes chains directories each depth levels deep under dir, with +// one file at the bottom of each chain, for the iss-114 cost benchmark. It +// returns the total directory count. depth stays under maxWalkDepth so the depth +// cap never prunes the measured tree. +func buildDeepTree(tb testing.TB, dir string, chains, depth int) int { + tb.Helper() + total := 0 + for i := 0; i < chains; i++ { + segs := make([]string, 0, depth+1) + segs = append(segs, fmt.Sprintf("c%05d", i)) + for j := 0; j < depth-1; j++ { + segs = append(segs, "d") + } + leafDir := filepath.Join(append([]string{dir}, segs...)...) + if err := os.MkdirAll(leafDir, 0o755); err != nil { + tb.Fatal(err) + } + if err := os.WriteFile(filepath.Join(leafDir, "leaf.go"), []byte("// x\n"), 0o644); err != nil { + tb.Fatal(err) + } + total += depth + } + return total +} + +// BenchmarkWalkFilesDeepTreeNew measures the shipped sub-root walk over a +// 60000-directory tree at depth 30 — the iss-114 measurement shape. +func BenchmarkWalkFilesDeepTreeNew(b *testing.B) { + dir := b.TempDir() + total := buildDeepTree(b, dir, 1600, 30) + ctx, err := newSourceContext(dir) + if err != nil { + b.Fatal(err) + } + defer ctx.Close() + b.Logf("tree: %d directories, depth 30", total) + b.ResetTimer() + for i := 0; i < b.N; i++ { + paths, _ := ctx.WalkFiles(".") + if len(paths) != 1600 { + b.Fatalf("walk returned %d leaf files, want 1600", len(paths)) + } + } +} + +// BenchmarkWalkFilesDeepTreeBaseline measures the pre-fix root.FS() walk over the +// same tree, so the iss-114 before/after is a like-for-like number. +func BenchmarkWalkFilesDeepTreeBaseline(b *testing.B) { + dir := b.TempDir() + total := buildDeepTree(b, dir, 1600, 30) + ctx, err := newSourceContext(dir) + if err != nil { + b.Fatal(err) + } + defer ctx.Close() + b.Logf("tree: %d directories, depth 30", total) + b.ResetTimer() + for i := 0; i < b.N; i++ { + paths, _ := walkFilesRootFSBaseline(ctx, ".", maxWalkFiles, walkSkipDirs) + if len(paths) != 1600 { + b.Fatalf("baseline walk returned %d leaf files, want 1600", len(paths)) + } + } +} From 347f78a764c7924771322b20a604775df8e9cdba Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 26 Jul 2026 05:06:31 +0100 Subject: [PATCH 2/2] chore: resolve iss-112, iss-114, iss-116 in the ledger The WalkFiles class fix lands the bounded per-directory read (iss-112), the O(1)-per-directory sub-root walk (iss-114), and the widened dependency-tree skip set (iss-116). Move all three from open/ to resolved/ with --impact fix. Assisted-by: Claude:claude-opus-4-8 --- ...kfiles-caps-the-number-of-regular-files-it-returns-but-fs.md | 2 ++ ...lkfiles-opens-every-directory-through-os-root-fs-which-re.md | 2 ++ ...-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md | 2 ++ 3 files changed, 6 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md (78%) rename .abcd/work/issues/{open => resolved}/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md (78%) rename .abcd/work/issues/{open => resolved}/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md (78%) diff --git a/.abcd/work/issues/open/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md b/.abcd/work/issues/resolved/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md similarity index 78% rename from .abcd/work/issues/open/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md rename to .abcd/work/issues/resolved/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md index 8c46ab4..e855f4e 100644 --- a/.abcd/work/issues/open/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md +++ b/.abcd/work/issues/resolved/iss-112-walkfiles-caps-the-number-of-regular-files-it-returns-but-fs.md @@ -7,6 +7,8 @@ category: "tech-debt" source: "impl-review" found_during: "itd-95 P1 review" found_at: "internal/core/lifeboat/probe.go" +resolution: "Bounded the walk's per-directory read with the shared maxDirEntries guard (readDirBounded), so one directory of millions of entries no longer materialises before the file cap." +impact: fix --- WalkFiles caps the number of regular files it returns, but fs.WalkDir calls ReadDir(-1) on every directory it enters, so a single directory holding millions of entries balloons memory before the file cap can apply. The maxWalkFiles doc comment claims the walk stays bounded in memory, which is only true of the returned slice. ListDir already bounds this with ReadDir(maxDirEntries); the walk does not share that guard. Found by an independent security review of the itd-95 diff. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md b/.abcd/work/issues/resolved/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md similarity index 78% rename from .abcd/work/issues/open/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md rename to .abcd/work/issues/resolved/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md index f8e356f..35ebc6d 100644 --- a/.abcd/work/issues/open/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md +++ b/.abcd/work/issues/resolved/iss-114-walkfiles-opens-every-directory-through-os-root-fs-which-re.md @@ -7,6 +7,8 @@ category: "tech-debt" source: "impl-review" found_during: "itd-96 P1 review" found_at: "internal/core/lifeboat/probe.go" +resolution: "Walk now holds an os.Root sub-root per directory (OpenRoot), opening each child O(1); cost is O(entries) and depth-independent (48000-dir depth-30 tree ~1.4s vs ~7s), containment preserved." +impact: fix --- WalkFiles opens every directory through os.Root.FS(), which re-resolves the whole path from the containment root one component at a time, so a walk costs O(entries x depth) component opens. Measured post-fix: a 60000-directory tree at depth 30 takes 10.5s for one probe (two adapters walk it concurrently). The depth and directory caps bound this, but the constant is high. os.Root.OpenRoot would let the walk hold a sub-root per directory and open each child in O(1). Found while fixing the unbounded-walk finding in the itd-96 review. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md b/.abcd/work/issues/resolved/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md similarity index 78% rename from .abcd/work/issues/open/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md rename to .abcd/work/issues/resolved/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md index a82a878..7ad7489 100644 --- a/.abcd/work/issues/open/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md +++ b/.abcd/work/issues/resolved/iss-116-the-walkfiles-skip-set-covers-only-node-and-go-dependency-tr.md @@ -7,6 +7,8 @@ category: "tech-debt" source: "impl-review" found_during: "itd-95 P1 review" found_at: "internal/core/lifeboat/probe.go" +resolution: "Widened walkSkipDirs to the common Python/build-output/CocoaPods trees (.venv, venv, .tox, __pycache__, target, build, dist, Pods); vendored markers no longer cited and the dot-prefixed tree no longer eats the cap before src/." +impact: fix --- The WalkFiles skip set covers only Node and Go dependency trees, so a Python .venv, a Rust target/, __pycache__, dist/, build/ and Pods/ are walked as if they were the team's own source. Two consequences on a record-less repo: the open-questions adapter cites a vendored dependency's TODO as this project's open question, and because fs.WalkDir reads directories in lexical order a large dot-prefixed dependency tree can consume the 50000-file walk cap before the project's own src/ is reached. Found by an independent review of the itd-95 diff; spc-12 names the current skip set, so widening it is a design revisit rather than an implementation fix. \ No newline at end of file