From 429258354da9b5957bdda7b103bd180db28b8a85 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 00:09:59 -0700 Subject: [PATCH] fix(workspace): address CodeRabbit review findings on the foundation Resolves the actionable CodeRabbit threads on the workspace-mode foundation: - project-resolver: defer saveWorkspaceConfig until after the user confirms init and store.init() succeeds (no partial .fusion/ on a declined/non-interactive run). - git-repository: validate each candidate with a real `git rev-parse` work-tree probe before counting it (no false-positive repos from stray .git markers); loadWorkspaceConfig now rejects absolute paths, `..` escapes, and non-string entries so a corrupt/malicious config can't resolve outside the workspace root. - executor: gate workspace mode on repos.length > 0 at all three sites so an empty { repos: [] } can't bypass the git-repo guard or enable an empty workspace. - worktree-acquisition: thread the configured-command runner through the workspace acquire path (sub-repos run their init setup); validate repoRelPath as an in-root relative path before joining; liveness-check a remembered worktree before reporting it ready (pruned paths fall through to re-acquire); clear the singular task.worktree/branch after persisting per-repo state (per-repo state lives only in workspaceWorktrees). - agent-tools: forward runContext into acquireWorkspaceRepoWorktree for log attribution. The executor-workspace test's mock-the-subject pattern is left for the session-scoping follow-up that rewrites it with a real two-repo fixture (FN-5048). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/project-resolver.ts | 15 ++++- packages/core/src/git-repository.ts | 42 ++++++++++-- packages/engine/src/agent-tools.ts | 10 ++- packages/engine/src/executor.ts | 19 +++++- packages/engine/src/worktree-acquisition.ts | 71 +++++++++++++++++++-- 5 files changed, 139 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/project-resolver.ts b/packages/cli/src/project-resolver.ts index 93eafa1bd9..68ac2f71d1 100644 --- a/packages/cli/src/project-resolver.ts +++ b/packages/cli/src/project-resolver.ts @@ -632,14 +632,22 @@ export async function registerProjectInteractive( const gitCheck = spawnSync("git", ["-C", absPath, "rev-parse", "--is-inside-work-tree"], { encoding: "utf8" }); const isGitRepo = gitCheck.status === 0 && gitCheck.stdout.trim() === "true"; + /* + FNXC:Workspace 2026-06-22-00:00: + Workspace detection only reports candidate sub-repos here; persistence is deferred until + after the user confirms init AND TaskStore.init() succeeds. Writing .fusion/workspace.json + before confirmation would leave a partial .fusion/ dir when the user declines or runs + non-interactively, polluting a plain non-git directory with stray Fusion state. + */ + let detectedSubRepos: string[] | null = null; if (!isGitRepo) { const subRepos = await detectWorkspaceRepos(absPath); if (subRepos.length > 0) { console.log(`\n Found ${subRepos.length} git repositories in ${absPath}:`); subRepos.forEach((r: string) => console.log(` • ${r}`)); console.log(`\n Initializing as a Fusion workspace...\n`); - await saveWorkspaceConfig(absPath, { repos: subRepos }); - // Fall through to normal .fusion init + detectedSubRepos = subRepos; + // workspace.json is written below, only after a confirmed store.init() succeeds. } // else: fall through to existing error path } @@ -653,6 +661,9 @@ export async function registerProjectInteractive( const { TaskStore } = await import("@fusion/core"); const store = new TaskStore(absPath); await store.init(); + if (detectedSubRepos) { + await saveWorkspaceConfig(absPath, { repos: detectedSubRepos }); + } console.log(` ✓ Initialized fn at ${absPath}`); } else { throw new ProjectResolutionError( diff --git a/packages/core/src/git-repository.ts b/packages/core/src/git-repository.ts index 974c5d12a6..adaebeb2ed 100644 --- a/packages/core/src/git-repository.ts +++ b/packages/core/src/git-repository.ts @@ -114,13 +114,23 @@ export async function detectWorkspaceRepos(dir: string): Promise { const { stat } = await import("node:fs/promises"); const { join } = await import("node:path"); const found: string[] = []; + /* + FNXC:Workspace 2026-06-22-00:00: + A bare `.git` marker (e.g. a stray file copied in, or an unrelated tool's artifact) is not + proof of a git repository. Each candidate child is validated with a real `git rev-parse` + work-tree probe before it counts, so stray `.git` entries do not yield false-positive repos. + */ for (const entry of entries) { - const candidate = join(dir, entry, ".git"); + const childDir = join(dir, entry); + // Cheap pre-filter: skip children with no `.git` marker at all before spawning git. try { - const s = await stat(candidate); - if (s.isDirectory() || s.isFile()) found.push(entry); + const s = await stat(join(childDir, ".git")); + if (!s.isDirectory() && !s.isFile()) continue; } catch { - // not a git repo + continue; + } + if (await isInsideGitWorkTree(childDir, runGitCommand, DEFAULT_GIT_TIMEOUT_MS)) { + found.push(entry); } } return found.sort(); @@ -132,9 +142,27 @@ export interface WorkspaceConfig { const WORKSPACE_CONFIG_FILENAME = "workspace.json"; +/* +FNXC:Workspace 2026-06-22-00:00: +Workspace repo entries are later joined onto the workspace root to resolve worktrees, so an +attacker-controlled or corrupted workspace.json with an absolute path or a `..` escape +(`../outside-repo`) would resolve outside the workspace root. Each entry must be a normalized, +relative, in-root path; absolute paths, `..` escapes, and non-string entries are rejected. +*/ +function isInRootRelativePath(entry: unknown, pathMod: typeof import("node:path")): entry is string { + if (typeof entry !== "string" || entry.length === 0) return false; + if (pathMod.isAbsolute(entry)) return false; + const normalized = pathMod.normalize(entry); + if (normalized === ".." || normalized.startsWith(`..${pathMod.sep}`) || normalized.startsWith("../")) { + return false; + } + return true; +} + export async function loadWorkspaceConfig(rootDir: string): Promise { const { readFile } = await import("node:fs/promises"); - const { join } = await import("node:path"); + const pathMod = await import("node:path"); + const { join } = pathMod; const configPath = join(rootDir, ".fusion", WORKSPACE_CONFIG_FILENAME); try { const raw = await readFile(configPath, "utf-8"); @@ -145,7 +173,9 @@ export async function loadWorkspaceConfig(rootDir: string): Promise isInRootRelativePath(entry, pathMod)); + return { ...(parsed as object), repos }; } return null; } catch { diff --git a/packages/engine/src/agent-tools.ts b/packages/engine/src/agent-tools.ts index 99e87b2cb3..1af8949ffe 100644 --- a/packages/engine/src/agent-tools.ts +++ b/packages/engine/src/agent-tools.ts @@ -3601,8 +3601,12 @@ export function createAcquireRepoWorktreeTool(opts: { logger?: { log: (m: string) => void; warn: (m: string) => void }; secretsStore?: Pick; runContext?: RunMutationContext; + audit?: Pick; + // FNXC:Workspace 2026-06-22 — thread the configured worktree-init runner so sub-repo worktrees run configured setup. + runConfiguredCommand?: import("./worktree-acquisition.js").AcquireWorkspaceRepoWorktreeOptions["runConfiguredCommand"]; + taskEnv?: NodeJS.ProcessEnv; }): ToolDefinition { - const { workspaceRootDir, workspaceRepos, task, store, settings, logger, secretsStore, runContext } = opts; + const { workspaceRootDir, workspaceRepos, task, store, settings, logger, secretsStore, runContext, audit, runConfiguredCommand, taskEnv } = opts; return { name: "fn_acquire_repo_worktree", label: "Acquire Repo Worktree", @@ -3629,6 +3633,10 @@ export function createAcquireRepoWorktreeTool(opts: { settings, logger, secretsStore, + runContext, + audit, + runConfiguredCommand, + taskEnv, }); await store.logEntry( task.id, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index fb57716b9d..56610e6b0c 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -7416,7 +7416,15 @@ export class TaskExecutor { if (this.workspaceConfig === undefined) { this.workspaceConfig = await loadWorkspaceConfig(this.rootDir); } - if (!this.workspaceConfig && !await isGitRepository(this.rootDir)) { + /* + FNXC:Workspace 2026-06-22-00:00: + Workspace mode is only meaningful with at least one usable sub-repo. An empty `{ repos: [] }` + must NOT bypass the git-repository guard, inject workspace instructions, or expose the + workspace tool — otherwise a non-git directory with an empty config would skip validation + and enable a workspace with nothing to work on. Gate every workspace check on repos.length > 0. + */ + const hasWorkspaceRepos = (this.workspaceConfig?.repos.length ?? 0) > 0; + if (!hasWorkspaceRepos && !await isGitRepository(this.rootDir)) { await this.store.logEntry( task.id, "Cannot execute task: project directory is not a Git repository. Fusion requires a Git repository for worktree-based task execution.", @@ -8351,7 +8359,7 @@ export class TaskExecutor { ...getEnabledPluginTools(this.options.pluginRunner), ]; - if (this.workspaceConfig) { + if (this.workspaceConfig && this.workspaceConfig.repos.length > 0) { customTools.push(createAcquireRepoWorktreeTool({ workspaceRootDir: this.rootDir, workspaceRepos: this.workspaceConfig.repos, @@ -8361,6 +8369,11 @@ export class TaskExecutor { logger: executorLog, secretsStore: this.options.secretsStore, runContext: engineRunContext, + audit, + taskEnv, + // FNXC:Workspace 2026-06-22 — forward the configured worktree-init runner so sub-repo worktrees run configured setup. + runConfiguredCommand: (command, cwd, timeoutMs, env) => + runConfiguredCommand(command, cwd, timeoutMs, env, audit), })); } @@ -15940,7 +15953,7 @@ Use \`fn_task_create\` for truly separate follow-up work, including unrelated/pr If lint is configured and failing, fix that too before completion. Do not repeatedly rerun a broad failing or hanging workspace command without a new hypothesis and a narrower confirming command.`; - if (workspaceConfig) { + if (workspaceConfig && workspaceConfig.repos.length > 0) { return executionPrompt + `\n\n## Workspace mode\n` + `This project is a workspace containing multiple git repositories.\n` + `Available repos:\n` + diff --git a/packages/engine/src/worktree-acquisition.ts b/packages/engine/src/worktree-acquisition.ts index 6ce4ca696b..91fbc75bcd 100644 --- a/packages/engine/src/worktree-acquisition.ts +++ b/packages/engine/src/worktree-acquisition.ts @@ -604,21 +604,64 @@ export interface AcquireWorkspaceRepoWorktreeOptions { settings: Partial; logger?: { log: (m: string) => void; warn: (m: string) => void; error?: (m: string) => void }; secretsStore?: Pick; + runContext?: RunMutationContext; + audit?: Pick; + runConfiguredCommand?: AcquireTaskWorktreeOptions["runConfiguredCommand"]; + taskEnv?: NodeJS.ProcessEnv; +} + +/* +FNXC:WorkspaceWorktree 2026-06-22-00:00: +`repoRelPath` is an exported, caller-trusted parameter that is joined onto `workspaceRootDir`. +An absolute path or a `..` escape (`../outside`) would resolve a worktree outside the workspace +root. Validate it is a normalized, relative, in-root path before resolving the absolute path. +*/ +function assertInRootRepoRelPath(repoRelPath: string, sep: string, isAbsolute: (p: string) => boolean, normalize: (p: string) => string): void { + if (typeof repoRelPath !== "string" || repoRelPath.length === 0 || isAbsolute(repoRelPath)) { + throw new Error(`Invalid workspace repo path (must be relative and in-root): ${String(repoRelPath)}`); + } + const normalized = normalize(repoRelPath); + if (normalized === ".." || normalized.startsWith(`..${sep}`) || normalized.startsWith("../")) { + throw new Error(`Invalid workspace repo path (escapes workspace root): ${repoRelPath}`); + } } export async function acquireWorkspaceRepoWorktree( opts: AcquireWorkspaceRepoWorktreeOptions, ): Promise<{ worktreePath: string; branch: string; alreadyAcquired: boolean }> { - const { repoRelPath, workspaceRootDir, task, store, settings, logger, secretsStore } = opts; - const { join } = await import("node:path"); + const { repoRelPath, workspaceRootDir, task, store, settings, logger, secretsStore, runContext, audit, runConfiguredCommand, taskEnv } = opts; + const { join, isAbsolute, normalize, sep } = await import("node:path"); + // FNXC:WorkspaceWorktree 2026-06-22 — reject absolute / `..`-escaping repo paths before resolving. + assertInRootRepoRelPath(repoRelPath, sep, isAbsolute, normalize); + const repoAbsPath = join(workspaceRootDir, repoRelPath); + + /* + FNXC:WorkspaceWorktree 2026-06-22-00:00: + A remembered per-repo worktree is only reusable if it still exists and is a registered git + worktree. A pruned/deleted worktree path would otherwise be reported as "ready" without the + resume/classification checks that `acquireTaskWorktree` runs on the singular path. Verify the + remembered path passes the same liveness check (existence + git work-tree classification); + if it is dead, drop it and fall through to re-acquire a fresh worktree. + */ const existing = task.workspaceWorktrees?.[repoRelPath]; if (existing) { - return { ...existing, alreadyAcquired: true }; + let live = existsSync(existing.worktreePath); + if (live) { + try { + const classification = await classifyTaskWorktree(repoAbsPath, existing.worktreePath); + live = classification.ok; + } catch { + live = false; + } + } + if (live) { + return { ...existing, alreadyAcquired: true }; + } + logger?.warn(`${task.id}: remembered workspace worktree for ${repoRelPath} is missing/unusable (${existing.worktreePath}); re-acquiring`); + await store.logEntry(task.id, `Remembered workspace worktree for ${repoRelPath} is no longer usable; re-acquiring`, existing.worktreePath, runContext); } - const repoAbsPath = join(workspaceRootDir, repoRelPath); - /* FNXC:WorkspaceWorktree 2026-06-21-00:00: Workspace mode acquires one worktree per sub-repo for a single task. `acquireTaskWorktree` @@ -629,6 +672,11 @@ export async function acquireWorkspaceRepoWorktree( contamination. Clear the singular worktree/branch fields on the copy handed to the single-repo helper so each sub-repo always gets a fresh worktree; per-repo state is tracked in `task.workspaceWorktrees`, not the singular column. + + FNXC:WorkspaceWorktree 2026-06-22-00:00: + `acquireTaskWorktree` only runs the configured worktree-init command when `runConfiguredCommand` + is threaded through. Forward it (plus runContext/audit/taskEnv) so workspace sub-repos run the + same configured setup as the non-workspace acquire path instead of silently skipping it. */ const result = await acquireTaskWorktree({ task: { ...task, worktree: undefined, branch: undefined }, @@ -637,6 +685,10 @@ export async function acquireWorkspaceRepoWorktree( settings, logger, secretsStore, + runContext, + audit, + runConfiguredCommand, + taskEnv, runInitCommand: true, }); @@ -644,7 +696,14 @@ export async function acquireWorkspaceRepoWorktree( ...(task.workspaceWorktrees ?? {}), [repoRelPath]: { worktreePath: result.worktreePath, branch: result.branch }, }; - await store.updateTask(task.id, { workspaceWorktrees: updated }); + /* + FNXC:WorkspaceWorktree 2026-06-22-00:00: + `acquireTaskWorktree` persists the singular `task.worktree`/`task.branch` on the task row. + For a workspace task that pointer would end up referencing whichever sub-repo was acquired last, + violating the contract that per-repo state lives only in `workspaceWorktrees`. Clear the singular + fields in the same update so a workspace task never carries a misleading singular worktree pointer. + */ + await store.updateTask(task.id, { workspaceWorktrees: updated, worktree: null, branch: null }); return { worktreePath: result.worktreePath, branch: result.branch, alreadyAcquired: false }; }