From e27771eb719ce7e2b51fab792206ba4ec274dd80 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Sun, 19 Jul 2026 22:47:46 -0700 Subject: [PATCH] fix: bound workspace repo discovery so symlink escapes can't stall startup Workspace discovery follows symlinks since #1060, which means the walk can leave the workspace root by design. A link into a huge unrelated tree was enumerated synchronously before the server bound its port, hanging startup with no output. The walk now shares the PLANNOTATOR_FILE_BROWSER_MAX_FILES budget: it visits at most that many directories and returns what it found. Symlinked repos are still discovered under the default budget. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk --- AGENTS.md | 2 +- packages/server/review-workspace.test.ts | 43 ++++++++++++++++++++++++ packages/shared/review-workspace-node.ts | 29 +++++++++++++--- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c6c6eb64..1f1199ff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -141,7 +141,7 @@ claude --plugin-dir ./apps/hook | `PLANNOTATOR_ANNOTATE_HISTORY` | Set to `0` / `false` to disable per-file version history in annotate mode (no copies of annotated files are written to the data dir; the annotate version diff is unavailable). Default: enabled. Can also be set via `~/.plannotator/config.json` (`{ "annotateHistory": false }`); the env var takes precedence. | | `JINA_API_KEY` | Optional Jina Reader API key for higher rate limits (500 RPM vs 20 RPM unauthenticated). Free keys include 10M tokens. | | `PLANNOTATOR_DATA_DIR` | Override the base data directory. Supports `~` expansion. Default: `~/.plannotator`. All data (plans, history, drafts, config, hooks, sessions, debug logs, IPC registry) is stored under this directory. | -| `PLANNOTATOR_FILE_BROWSER_MAX_FILES` | File-discovery limit: regular files inspected by CLI markdown/folder resolution and startup code-file warming, and supported files returned by the file browser. Must be a positive integer; invalid, zero, or negative values use the default of `5000`. | +| `PLANNOTATOR_FILE_BROWSER_MAX_FILES` | File-discovery limit: regular files inspected by CLI markdown/folder resolution and startup code-file warming, supported files returned by the file browser, and directories scanned during multi-repo workspace discovery (symlinks may point outside the workspace, so the budget — not the root — bounds that walk). Must be a positive integer; invalid, zero, or negative values use the default of `5000`. | | `PLANNOTATOR_GLIMPSE` | Set to `0` / `false` to disable the Glimpse native window even when `glimpseui` is installed. Default: enabled. Can also be set via `~/.plannotator/config.json` (`{ "glimpse": false }`). | | `PLANNOTATOR_GLIMPSE_WIDTH` | Width in pixels for the Glimpse native window. Default: `1280`. | | `PLANNOTATOR_GLIMPSE_HEIGHT` | Height in pixels for the Glimpse native window. Default: `900`. | diff --git a/packages/server/review-workspace.test.ts b/packages/server/review-workspace.test.ts index 72baa130..bf6acef5 100644 --- a/packages/server/review-workspace.test.ts +++ b/packages/server/review-workspace.test.ts @@ -745,6 +745,49 @@ describe("review-workspace", () => { expect(discoverWorkspaceRepoPaths(root)).toEqual([]); }); + it("bounds discovery when a symlink escapes into a huge external tree", () => { + // Regression for the #1060 follow-up: a symlink inside the workspace + // pointing at a large unrelated tree must not be enumerated unboundedly + // before the server starts. The walk shares the file-discovery budget. + const root = makeTempDir("plannotator-workspace-symlink-budget-"); + const huge = makeTempDir("plannotator-workspace-symlink-budget-target-"); + // 40 directories, each with 5 subdirectories — 200+ nodes, no repos. + for (let i = 0; i < 40; i++) { + for (let j = 0; j < 5; j++) { + mkdirSync(join(huge, `dir-${String(i).padStart(2, "0")}`, `sub-${j}`), { recursive: true }); + } + } + linkDirectory(huge, join(root, "escape")); + // A legitimate repo that sorts BEFORE the escape link so it is found + // within the budget. + const repo = join(root, "app"); + mkdirSync(repo, { recursive: true }); + initRepo(repo); + + const prior = process.env.PLANNOTATOR_FILE_BROWSER_MAX_FILES; + process.env.PLANNOTATOR_FILE_BROWSER_MAX_FILES = "25"; + try { + const repos = discoverWorkspaceRepoPaths(root); + expect(repos).toEqual([repo]); + } finally { + if (prior === undefined) delete process.env.PLANNOTATOR_FILE_BROWSER_MAX_FILES; + else process.env.PLANNOTATOR_FILE_BROWSER_MAX_FILES = prior; + } + }); + + it("keeps discovering symlinked repos under the default budget", () => { + // The budget must not break the feature it bounds: an external + // symlinked repo is still found with default limits. + const root = makeTempDir("plannotator-workspace-symlink-budget-ok-"); + const targetRoot = makeTempDir("plannotator-workspace-symlink-budget-ok-target-"); + const targetRepo = join(targetRoot, "service"); + mkdirSync(targetRepo, { recursive: true }); + initRepo(targetRepo); + linkDirectory(targetRepo, join(root, "linked")); + + expect(discoverWorkspaceRepoPaths(root)).toEqual([join(root, "linked")]); + }); + it("chooses the first logical alias deterministically for duplicate targets", () => { const root = makeTempDir("plannotator-workspace-symlink-duplicates-"); const targetRoot = makeTempDir("plannotator-workspace-symlink-duplicates-target-"); diff --git a/packages/shared/review-workspace-node.ts b/packages/shared/review-workspace-node.ts index b415a552..fe9628e5 100644 --- a/packages/shared/review-workspace-node.ts +++ b/packages/shared/review-workspace-node.ts @@ -11,6 +11,7 @@ import { parsePatchPathToken, } from "./diff-paths"; import { validateFilePath } from "./review-core"; +import { getFileBrowserMaxFiles } from "./resolve-file"; const SKIP_DIRS = new Set([ ".git", @@ -174,15 +175,21 @@ function compareDirentsByName(a: Dirent, b: Dirent): number { return 0; } +interface WorkspaceScanState { + visitedRealPaths: Set; + readonly limit: number; +} + function collectWorkspaceRepos( root: string, current: string, - visitedRealPaths: Set, + state: WorkspaceScanState, results: string[], ): void { + if (state.visitedRealPaths.size >= state.limit) return; const realPath = resolveDirectoryRealPath(current); - if (!realPath || visitedRealPaths.has(realPath)) return; - visitedRealPaths.add(realPath); + if (!realPath || state.visitedRealPaths.has(realPath)) return; + state.visitedRealPaths.add(realPath); let entries: Dirent[]; try { @@ -197,9 +204,10 @@ function collectWorkspaceRepos( } for (const entry of entries) { + if (state.visitedRealPaths.size >= state.limit) return; if (SKIP_DIRS.has(entry.name)) continue; if (!entry.isDirectory() && !entry.isSymbolicLink()) continue; - collectWorkspaceRepos(root, resolve(current, entry.name), visitedRealPaths, results); + collectWorkspaceRepos(root, resolve(current, entry.name), state, results); } } @@ -210,11 +218,22 @@ function collectWorkspaceRepos( * retained for labels and file routing. Each canonical directory is traversed * at most once, so cycles and duplicate aliases cannot duplicate repositories. * Unreadable, broken, and non-directory entries are ignored. + * + * The walk visits at most `PLANNOTATOR_FILE_BROWSER_MAX_FILES` directories. + * Symlinks may legitimately lead outside the workspace root (that is how + * symlinked repos are discovered), so a link into a huge unrelated tree cannot + * be fenced by path — the visit budget is what keeps discovery from scanning + * indefinitely before the server starts. */ export function discoverWorkspaceRepoPaths(root: string): string[] { const resolvedRoot = resolve(root); const results: string[] = []; - collectWorkspaceRepos(resolvedRoot, resolvedRoot, new Set(), results); + collectWorkspaceRepos( + resolvedRoot, + resolvedRoot, + { visitedRealPaths: new Set(), limit: getFileBrowserMaxFiles() }, + results, + ); return results.sort(); }