diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b45820b..889e4ea 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -67,7 +67,7 @@ Tables can also: - opt into **`ScanResult.ProjectRoot` attribution** — the Python table sets the matched root on every result because its artifacts sit at arbitrary depth and output grouping needs explicit attribution (nested project roots win over parents); - declare a **`PruneRoot`** hook that skips the ecosystem's own SDK checkout. -**Pruned trees** are skipped whole — no artifact matching, no project context, no descent — so nothing inside is ever a deletion target: installed-package trees (pnpm store version dirs, macOS `.app` bundles) engine-wide, and an ecosystem's SDK checkout via `PruneRoot` — [decision](decisions/prune-non-project-trees.md). +**Pruned trees** are skipped whole — no artifact matching, no project context, no descent — so nothing inside is ever a deletion target: installed-package trees (`node_modules` outside a project, `site-packages`, package stores, app bundles) engine-wide, and an ecosystem's SDK checkout via `PruneRoot` — [decision](decisions/prune-non-project-trees.md). **Deduplication**: a directory matching rules of several active ecosystems is reported once, attributed to the first table in order (node → rust → ruby → python → go → flutter → android), 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. diff --git a/docs/SPEC.md b/docs/SPEC.md index 0069535..73b31bb 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -328,7 +328,7 @@ Sizes use **decimal SI units** (1 KB = 1000 B). `--min-size` uses the same conve **pnpm-installed `node_modules`**: pnpm clones (macOS) or hard-links (Linux) pnpm store files into `node_modules` by default, so deleting it alone frees little; running `pnpm store prune` afterwards removes packages no project references. The result carries this note when `node_modules/.modules.yaml` (written by pnpm on install, JSON since pnpm 12; `pnpm-lock.yaml` alone does not prove pnpm populated the folder) names a `storeDir` that exists on the same volume. On macOS its `size` leaves out blocks shared with the store or other projects. A store on another volume forces pnpm to copy, so no note is given. `packageImportMethod: copy` is not recorded in `.modules.yaml`, so such installs still get the note. -**Installed-package trees are excluded**: `node_modules`/`dist` inside a pnpm store or a macOS `.app` bundle are package content, not project output — see [decision](decisions/prune-non-project-trees.md). +**Installed-package trees are excluded**: nothing inside a `node_modules` that is not a project's own, Python `site-packages`/`dist-packages`, the Go module cache, a pnpm store or a macOS `.app` bundle is reported — see [decision](decisions/prune-non-project-trees.md). **Monorepo support**: artifacts in sub-packages (apps/, packages/) are grouped under the git root project. Sub-packages are displayed with headers showing their path and total size. diff --git a/docs/decisions/prune-non-project-trees.md b/docs/decisions/prune-non-project-trees.md index 4673c65..ff99fca 100644 --- a/docs/decisions/prune-non-project-trees.md +++ b/docs/decisions/prune-non-project-trees.md @@ -4,9 +4,9 @@ The walk engine skips these trees whole — no artifact matching, no project context, no descent — so nothing inside is ever a deletion target: -- **Installed-package trees**, engine-wide regardless of which ecosystems are active: their marker files belong to shipped packages, not projects. Detected by invariant layout, never install path. - - pnpm store version dir (`v` with `files/` and `index/` or `index.db`) - - macOS app bundle (`*.app` with `Contents/`) +- **Installed-package trees**, whichever ecosystems are active. Detected by layout or by a name only package managers create, never by install path: + - by name, when the directory is not itself a matched artifact: `node_modules`, `site-packages`, `dist-packages` + - by entries: pnpm store version dir (`v` with `files/` and `index/` or `index.db`), macOS app bundle (`*.app` with `Contents/`), Go module cache (`mod/` with `cache/download/`) - **An ecosystem's own SDK checkout**, via the table's `PruneRoot` hook. The SDK root is detected by its invariant bootstrap layout, never a hardcoded install path (Flutter: `bin/flutter` + `bin/internal/engine.version`). When adding an ecosystem or rule, scan that tool's real SDK/toolchain checkout and verify what gets flagged. A scanner's own false positive is fixed in that scanner's PR. @@ -19,14 +19,15 @@ go test ./internal/scanner/ -run 'TestWalkExcludesInstalledPackageTrees|TestFlut ## Rationale +- Each package in an installed-package tree, including the runtime an app ships inside its bundle, carries its own marker (`package.json`, `setup.py`, `go.mod`). Without pruning it roots a project, and its shipped `dist/`, `__pycache__` or `vendor/` matches as `safe` output — deleting the `dist/` of the npm bundled in a GitHub Actions runner breaks the runner. A project's own `node_modules` matches as an artifact before the name check, so it is still reported. +- pnpm v11's `links/` holds unpacked packages; deleting it corrupts the store every project hard-links from. The store itself is reported by the Global Caches scanner. - A directory name like `build`/`dist` is not always regenerable output. Inside an SDK checkout it can be committed *source*: the Flutter SDK's `engine/src/build` / `engine/src/flutter/build` are GN build-system source trees tracked in git. - Gitignore-aware protection does not catch this: committed-clean files are not `protected`, so they classify as `safe` and become deletion targets. -- pnpm v11's `links/` unpacks packages whose `dist/` would match; deleting it corrupts the store every project hard-links from. The store itself is reported by the Global Caches scanner. -- Apps ship their runtime inside the bundle (e.g. Electron `node_modules` in VS Code update copies under `~/Library/Caches`). - The Android SDK ships no `build.gradle`, so it never establishes a project context and needs no `PruneRoot`. ## Rejected alternatives - **Detect by install path** — SDKs and stores live wherever the user put them; a hardcoded path misses relocated checkouts. - **Rely on gitignore-aware protection** — misses committed-clean source (above). +- **Prune every directory a deps rule names** — would also prune `vendor/` and `venv/` outside projects, which are not package-manager trees. - **Defer a scanner's false positive to a separate change** — the scanner ships able to delete real source in the meantime. diff --git a/internal/scanner/walk.go b/internal/scanner/walk.go index 29e9769..7ac60e1 100644 --- a/internal/scanner/walk.go +++ b/internal/scanner/walk.go @@ -186,6 +186,11 @@ func runWalk(ctx context.Context, root string, tables []walkEcosystem) ([]model. ReportProgress(ctx, len(results)) return false // matched artifact: do not descend } + // After artifact matching, so a project's own node_modules is + // still reported. + if installedPackageDirNames[name] { + return false + } // The root is always entered: the hidden-dir rule applies below it. return dir == root || !strings.HasPrefix(name, ".") || hiddenDescend[name] }, @@ -197,7 +202,7 @@ func runWalk(ctx context.Context, root string, tables []walkEcosystem) ([]model. // Prune before establishing any context or descending, so nothing // inside is ever a deletion target. - if isInstalledPackageTree(filepath.Base(dir), names) { + if isInstalledPackageTree(dir, names) { return false } for i := range tables { @@ -263,19 +268,34 @@ func runWalk(ctx context.Context, root string, tables []walkEcosystem) ([]model. return results, nil } +// installedPackageDirNames hold only installed packages when the walk reaches +// them without matching an artifact rule (a global lib/node_modules, a tool's +// bundled dependencies, a Python environment's site-packages). Enter prunes +// them by name, so they are never read. +var installedPackageDirNames = map[string]bool{ + "node_modules": true, + "site-packages": true, + "dist-packages": true, +} + // isInstalledPackageTree reports whether a directory roots a tree of installed -// packages, whose marker files belong to shipped packages, not projects. -// Checked for every ecosystem, so results don't depend on the --eco subset. +// packages identified by its entries: // // - pnpm store version dir: v with files/ and index/ (v10) or index.db -// (v11). v11's links/ unpacks packages whose dist/ would match; deleting -// it corrupts the store every pnpm project hard-links from. -// - macOS app bundle: *.app with Contents/ (e.g. Electron node_modules). -func isInstalledPackageTree(name string, names map[string]bool) bool { - if strings.HasSuffix(name, ".app") && names["Contents"] { +// (v11). Deleting v11's links/ corrupts the store. +// - macOS app bundle: *.app with Contents/. +// - Go module cache: mod/ with cache/download/. +func isInstalledPackageTree(dir string, names map[string]bool) bool { + name := filepath.Base(dir) + switch { + case strings.HasSuffix(name, ".app") && names["Contents"]: + return true + case name == "mod" && names["cache"]: + return isDir(filepath.Join(dir, "cache", "download")) + case isPnpmStoreVersionDir(name) && names["files"] && (names["index"] || names["index.db"]): return true } - return isPnpmStoreVersionDir(name) && names["files"] && (names["index"] || names["index.db"]) + return false } func isPnpmStoreVersionDir(name string) bool { diff --git a/internal/scanner/walk_prune_test.go b/internal/scanner/walk_prune_test.go index bb9a7f9..2db924d 100644 --- a/internal/scanner/walk_prune_test.go +++ b/internal/scanner/walk_prune_test.go @@ -39,6 +39,36 @@ func TestWalkExcludesInstalledPackageTrees(t *testing.T) { }, want: []artifact{{"app/node_modules", model.EcoNode, model.CatDeps, model.SafetySafe, ""}}, }, + { + name: "node_modules outside a project", + ecos: node, + tree: []string{ + "sdk-cache/darwin-arm64/node_modules/hono/package.json", + "sdk-cache/darwin-arm64/node_modules/hono/dist/index.js", + "runner/node20/lib/node_modules/npm/package.json", + "runner/node20/lib/node_modules/npm/node_modules/abbrev/", + "app/package.json", "app/tools/node_modules/x/package.json", "app/tools/node_modules/x/dist/", + }, + }, + { + name: "python site-packages", + ecos: []model.Ecosystem{model.EcoPython}, + tree: []string{ + "tool/.venv/lib/python3.12/site-packages/numpy/f2py/setup.py", + "tool/.venv/lib/python3.12/site-packages/numpy/f2py/__pycache__/x.pyc", + "usr/lib/python3/dist-packages/pkg/setup.py", + "usr/lib/python3/dist-packages/pkg/__pycache__/x.pyc", + }, + }, + { + name: "go module cache", + ecos: []model.Ecosystem{model.EcoGo}, + tree: []string{ + "go/pkg/mod/cache/download/x/@v/list", + "go/pkg/mod/github.com/briandowns/spinner@v1.23.2/go.mod", + "go/pkg/mod/github.com/briandowns/spinner@v1.23.2/vendor/", + }, + }, { name: "app bundle without node in the scan", ecos: []model.Ecosystem{model.EcoPython}, diff --git a/internal/scanner/walk_test.go b/internal/scanner/walk_test.go index 717f8bf..ba1c5ca 100644 --- a/internal/scanner/walk_test.go +++ b/internal/scanner/walk_test.go @@ -74,8 +74,8 @@ func TestWalkScan_DedupCoverage(t *testing.T) { // TestWalkScan_DedupNodeModulesShieldsPython pins the second double-count // fix: a matched artifact is skipped entirely, so no other active ecosystem -// reports anything inside it. With python alone, node_modules is not an -// artifact and its contents are scanned as before. +// reports anything inside it. With python alone, node_modules is pruned as an +// installed-package tree. func TestWalkScan_DedupNodeModulesShieldsPython(t *testing.T) { root := t.TempDir() @@ -108,16 +108,13 @@ func TestWalkScan_DedupNodeModulesShieldsPython(t *testing.T) { t.Errorf("expected node_modules attributed to node, got %s (%s)", results[0].Path, results[0].Ecosystem) } - // python alone: descends into node_modules and finds the __pycache__. + // python alone: node_modules is pruned, so nothing inside is reported. results, err = WalkScan(context.Background(), root, model.EcoPython) if err != nil { t.Fatalf("WalkScan error: %v", err) } - if len(results) != 1 { - t.Fatalf("expected exactly 1 result for python alone, got %d", len(results)) - } - if results[0].Path != pyc || results[0].Ecosystem != model.EcoPython { - t.Errorf("expected __pycache__ attributed to python, got %s (%s)", results[0].Path, results[0].Ecosystem) + if len(results) != 0 { + t.Fatalf("expected no result for python alone, got %d (%s)", len(results), results[0].Path) } }