From bdc270f0a07956ca2487d69c00a9de81cfa3ea9b Mon Sep 17 00:00:00 2001 From: Youngsup Oh Date: Fri, 17 Jul 2026 16:30:47 +0900 Subject: [PATCH] feat(scanner): make the walk's no-follow symlink policy explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk skipped symlinked directories only as a side effect of os.ReadDir's IsDir()==false. Assert it directly with an os.ModeSymlink check so a future refactor can't silently start following links. Following a symlink would double-count its target (real content that lives on disk elsewhere), inflating reported reclaimable space, and a delete of a symlinked artifact reclaims only the link while risking a shared target — so no-follow is the correct policy, not an accident. Behavior is unchanged; symlink cycles stay unwalkable, so no separate cycle guard is needed. Reclaimable content behind such links is the job of the Global Caches scanner and hardlink-aware sizing. Strengthen the test to also prove the walk never descends *through* a symlinked directory to match an artifact inside it, and document the policy in docs/architecture.md. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01YSTf3ozF4SnboZHyEi7Ycc --- CHANGELOG.md | 1 + docs/architecture.md | 2 ++ internal/scanner/walk.go | 13 +++++++++++++ internal/scanner/walk_traversal_test.go | 24 +++++++++++++++++++----- 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7cbe36..ac98a0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Git classification now forks `git` across a bounded worker pool (`min(NumCPU, 8)`) instead of one repo at a time — root resolution, `status`/`log`, and `check-ignore` all run concurrently across repos. On a workspace spanning dozens of repos this cut the classify phase roughly in half (~1.24s → ~0.54s warm cache, ~19% faster end-to-end); protection and activity results are unchanged. - The status-badge, safety-icon, and relative-time display formatters, previously duplicated between the table output and the interactive tree selector, are now single shared functions in `internal/ui` (`StatusBadge`, `SafetyIcon`, `RelativeTime`). Table and selector rendering are unchanged; the formatters gained direct unit tests. - Test coverage: the interactive tree selector's toggle, parent/child selection propagation, cursor movement, quick-select keys, and `Update` key routing are now tested (`internal/ui` 35% → 57%); the scan→classify integration workspace gained Python and Ruby project fixtures plus a Global-scanner (fixed-home) pipeline test; and `DirSize` now has a test that pins the sizing arithmetic (the golden test zeroes sizes out for portability). +- The walk engine's no-follow symlink policy is now explicit: it skips any symlink entry (`os.ModeSymlink`) rather than relying on `os.ReadDir`'s incidental `IsDir()==false`, so a future refactor can't silently start following links. Behavior is unchanged — a symlink is never descended into or matched as an artifact (even a symlinked `node_modules` from pnpm/monorepos), which avoids double-counting the target's disk space and never reclaims a shared target; symlink cycles remain unwalkable. Documented as the engine's symlink policy in `docs/architecture.md`. ### Fixed diff --git a/docs/architecture.md b/docs/architecture.md index 546f326..96f9251 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -66,6 +66,8 @@ Tables can also: **Deduplication**: a directory matching rules of several active ecosystems is reported once, attributed to the first table in order (node → rust → ruby → python → go), and no scanner descends into another's matched artifact (`__pycache__` inside `node_modules` is not reported separately). Attribution can therefore differ between a full scan and an `--eco` subset scan — a shared `coverage/` goes to node in a full scan, to ruby under `--eco ruby`. +**Symlink policy — never follow**: the walk skips any entry that is a symlink (explicit `os.ModeSymlink` check), so a symlink is never descended into and never matched as an artifact, even when it is named like one (a symlinked `node_modules`, as pnpm and some monorepos produce). This is deliberate: a symlink's target is real content that lives on disk elsewhere, so following it would double-count that space and inflate the reported reclaimable total, and deleting a symlinked artifact reclaims only the link (bytes) while risking a target shared by other projects. No-follow also means symlink cycles can never be walked, so no separate cycle guard is needed. The reclaimable content behind these links is surfaced instead by the Global Caches scanner (e.g. the pnpm store) and hardlink-aware sizing, not by following per-project links. + Results are sorted by (table order, path) before returning, keeping output order stable. ### Display Units diff --git a/internal/scanner/walk.go b/internal/scanner/walk.go index 69b74f1..6b7790b 100644 --- a/internal/scanner/walk.go +++ b/internal/scanner/walk.go @@ -217,6 +217,19 @@ func runWalk(ctx context.Context, root string, tables []walkEcosystem) ([]model. } for _, e := range entries { + // No-follow policy: never descend into or match a symlink. Following + // links would (a) double-count a target that also lives on disk + // elsewhere — inflating reported reclaimable space, (b) let a delete + // of a symlinked artifact (e.g. a pnpm/monorepo node_modules) reclaim + // nothing while risking a shared target, and (c) reopen symlink + // cycles. e.IsDir() already excludes symlinks (ReadDir uses lstat + // semantics), so this is an explicit assertion of that guarantee, not + // a behavior change — it keeps a future refactor from silently + // following links. Real reclaimable content behind these links is the + // job of the Global Caches scanner (pnpm store) and hardlink dedup. + if e.Type()&os.ModeSymlink != 0 { + continue + } if !e.IsDir() { continue } diff --git a/internal/scanner/walk_traversal_test.go b/internal/scanner/walk_traversal_test.go index c07472c..b7323f8 100644 --- a/internal/scanner/walk_traversal_test.go +++ b/internal/scanner/walk_traversal_test.go @@ -95,9 +95,10 @@ func TestWalkScan_ReadsEachDirOnce(t *testing.T) { } } -// TestWalkScan_DoesNotFollowSymlinkedArtifact locks the no-follow contract for -// interior symlinks (now an implicit consequence of e.IsDir()): a symlink named -// like an artifact is not matched, and a self-referential symlink does not loop. +// TestWalkScan_DoesNotFollowSymlinkedArtifact locks the explicit no-follow guard +// (walk.go skips any entry with os.ModeSymlink): a symlink named like an artifact +// is not matched, the walk never descends *through* a symlinked directory to +// match an artifact inside it, and a self-referential symlink does not loop. func TestWalkScan_DoesNotFollowSymlinkedArtifact(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink semantics differ on windows") @@ -108,20 +109,33 @@ func TestWalkScan_DoesNotFollowSymlinkedArtifact(t *testing.T) { real := filepath.Join(root, "real") touch(t, filepath.Join(real, "file.js")) + // A directory reachable only through a symlink (kept outside the scan root so + // it is not walked directly), holding a real artifact: it must stay invisible, + // proving the walk does not descend through the link. + linked := filepath.Join(t.TempDir(), "linked") + touch(t, filepath.Join(linked, "package.json")) + if err := os.MkdirAll(filepath.Join(linked, "node_modules", "pkg"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, filepath.Join(proj, "node_modules")); err != nil { t.Skipf("symlink unsupported: %v", err) } + if err := os.Symlink(linked, filepath.Join(proj, "via-link")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } if err := os.Symlink(proj, filepath.Join(proj, "loop")); err != nil { t.Skipf("symlink unsupported: %v", err) } - // Must terminate (no infinite loop) and not match the symlinked node_modules. + // Must terminate (no infinite loop), not match the symlinked node_modules, + // and not reach the node_modules inside the symlinked "via-link" dir. results, err := WalkScan(context.Background(), root, model.EcoNode) if err != nil { t.Fatal(err) } if len(results) != 0 { - t.Errorf("got %d results, want 0 (symlinked node_modules not followed): %+v", len(results), paths(results)) + t.Errorf("got %d results, want 0 (no symlink followed): %+v", len(results), paths(results)) } }