From fc9423e465328c1d3632c1426690f8b7b56c9ca3 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 22:29:25 -0700 Subject: [PATCH 1/6] =?UTF-8?q?feat(workspace):=20Phase=20B=20U1=20?= =?UTF-8?q?=E2=80=94=20per-repo=20change=20capture,=20contamination,=20and?= =?UTF-8?q?=20verify?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In workspace mode the executor now captures changes and verifies worktree invariants per acquired sub-repo instead of degrading to empty against the non-git root. Post-session capture (:7898) gains a workspace branch that loops task.workspaceWorktrees and reuses captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, …) per repo — inheriting resolveDiffBaseRef's merge-base fallback (repo baseCommitSha may be undefined) and the filterFilesToOwnTaskCommits contamination/divergence audit — then prefixes each repo's files with the repo path into task.modifiedFiles. Branch attribution runs per sub-repo (cwd), never against the root. The no-op assertCleanBranchAtBase is not iterated. verifyWorktreeInvariants is un-stubbed for workspace mode: it iterates every workspaceWorktrees entry asserting toplevel match + HEAD on fusion/, and returns the FIRST failing repo while preserving the exact discriminated union {ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected} (the :10889 consumer switches on reason for requeue/handoff) — the new repo field is additive. Singular non-workspace path unchanged. Real two-repo fixture tests (capture A+B repo-prefixed vs own base, undefined-base fallback, foreign-commit contamination audit, wrong_branch verify failure, single-repo regression). Gate green: typecheck, lint, test:gate (649+58). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...pace-phase-b-u1-per-repo-capture-verify.md | 5 + .../executor-workspace-capture.test.ts | 244 ++++++++++++++++++ packages/engine/src/executor.ts | 145 ++++++++++- 3 files changed, 391 insertions(+), 3 deletions(-) create mode 100644 .changeset/workspace-phase-b-u1-per-repo-capture-verify.md create mode 100644 packages/engine/src/__tests__/executor-workspace-capture.test.ts diff --git a/.changeset/workspace-phase-b-u1-per-repo-capture-verify.md b/.changeset/workspace-phase-b-u1-per-repo-capture-verify.md new file mode 100644 index 0000000000..e4a27b33dc --- /dev/null +++ b/.changeset/workspace-phase-b-u1-per-repo-capture-verify.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": minor +--- + +Workspace mode (Phase B, U1): per-repo post-session change capture, contamination detection, and worktree-invariant verification. In workspace mode the executor now loops `task.workspaceWorktrees`, reusing `captureModifiedFiles` per sub-repo (diffing each against its own `baseCommitSha`, with a merge-base fallback when undefined) to aggregate repo-prefixed `task.modifiedFiles` and surface per-repo contamination, and un-stubs `verifyWorktreeInvariants` to assert each acquired worktree's git toplevel and `fusion/` branch. Single-repo behavior is unchanged. diff --git a/packages/engine/src/__tests__/executor-workspace-capture.test.ts b/packages/engine/src/__tests__/executor-workspace-capture.test.ts new file mode 100644 index 0000000000..fa19b2f915 --- /dev/null +++ b/packages/engine/src/__tests__/executor-workspace-capture.test.ts @@ -0,0 +1,244 @@ +/* +FNXC:Workspace 2026-06-21-23:30: +U1 per-repo capture + contamination + worktree-invariant tests (KTD1/KTD2). These drive the REAL TaskExecutor methods against a REAL two-repo git fixture under a NON-git workspace root (createWorkspaceFixture), so any leaked rootDir git preflight would actually fail and a hand-built `git diff` against an undefined base would blow up. + +Seam choice (FN-5048): we set `(executor as any).workspaceConfig` directly (loadWorkspaceConfig has its own unit) and create real `fusion/` worktrees per sub-repo with real commits — no mock-the-world child_process. Capture is exercised through `captureWorkspaceModifiedFiles` (the helper the post-session path at executor.ts:7900 calls) and verification through `verifyWorktreeInvariants`. Real git is used only where the invariant requires it. + +Coverage: +- happy: edits in repo A + B → aggregated modifiedFiles carry repo-prefixed paths from BOTH, each diffed against its own baseCommitSha. +- edge: a repo with baseCommitSha undefined → capture still works via resolveDiffBaseRef's merge-base fallback (no `git diff undefined..HEAD`). +- contamination: a foreign commit (feat(FN-OTHER):) in a sub-repo's range → the filterFilesToOwnTaskCommits divergence audit fires (task:worktree-contamination-detected) for that repo, and the foreign file is excluded from attributed files. +- error: a worktree HEAD off fusion/ → verifyWorktreeInvariants returns {ok:false, reason:'wrong_branch', repo, observed, expected} (NOT {ok:true}); the reason enum is preserved for the :10889 consumer. +- regression: a single-repo (non-workspace) task → capture/verify identical to today. +*/ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import { execSync } from "node:child_process"; +import { mkdirSync, writeFileSync } from "node:fs"; +import path from "node:path"; +import type { Task, TaskStore, WorkspaceConfig } from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { createWorkspaceFixture, hasGit, type WorkspaceFixture } from "./_workspace-fixture.js"; + +const describeIfGit = hasGit ? describe : describe.skip; + +function createStore(overrides: Partial> = {}): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + updateTask: vi.fn().mockResolvedValue(undefined), + logEntry: vi.fn().mockResolvedValue(undefined), + getSettings: vi.fn().mockResolvedValue({ autoMerge: false }), + getRunContextFor: vi.fn(), + on: emitter.on.bind(emitter), + ...overrides, + }) as unknown as TaskStore & EventEmitter; +} + +function makeTask(id = "FN-WS-1", overrides: Partial = {}): Task { + return { + id, + title: "Workspace task", + description: "", + column: "in-progress", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + } as Task; +} + +// Capture attribution requires a digit-form task id (`FN-\d+`); the branch-attribution +// subject parser only attributes `feat(FN-1001):` style subjects, so the KTD2-era +// `FN-WS-1` placeholder would never attribute a commit. Use a real numeric id here. +const TASK_ID = "FN-1001"; +const BRANCH = "fusion/fn-1001"; + +/** Configure git identity in a freshly-created worktree (worktrees don't inherit user.* on all platforms). */ +function configureIdentity(dir: string): void { + execSync('git config user.email "test@example.com"', { cwd: dir, stdio: "pipe" }); + execSync('git config user.name "Test"', { cwd: dir, stdio: "pipe" }); +} + +/** + * Add a real fusion/ worktree to a sub-repo, commit one own-attributed edit + * onto that branch, and return { worktreePath, baseCommitSha } for task.workspaceWorktrees. + * baseCommitSha is the sub-repo's pre-edit HEAD so the diff range is base..HEAD. + */ +function addRepoWorktreeWithOwnEdit( + fx: WorkspaceFixture, + repoRel: string, + fileName: string, +): { worktreePath: string; baseCommitSha: string } { + const repoDir = fx.repoPath(repoRel); + const baseCommitSha = fx.git(repoRel, "git rev-parse HEAD"); + const worktreePath = path.join(repoDir, ".worktrees", "fn-ws-1"); + fx.git(repoRel, `git worktree add -b ${BRANCH} ${worktreePath} HEAD`); + configureIdentity(worktreePath); + mkdirSync(path.dirname(path.join(worktreePath, fileName)), { recursive: true }); + writeFileSync(path.join(worktreePath, fileName), "// own change\n", "utf-8"); + execSync(`git add ${fileName}`, { cwd: worktreePath, stdio: "pipe" }); + execSync(`git commit -m "feat(${TASK_ID}): edit ${fileName}"`, { cwd: worktreePath, stdio: "pipe" }); + return { worktreePath, baseCommitSha }; +} + +function workspaceExecutor(fx: WorkspaceFixture, store = createStore()): TaskExecutor { + const executor = new TaskExecutor(store, fx.rootDir); + (executor as any).workspaceConfig = { repos: fx.repos } as WorkspaceConfig; + return executor; +} + +describeIfGit("U1 KTD1 — per-repo capture aggregates repo-prefixed paths", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("happy: edits in repo A + B are diffed against their own base and repo-prefixed", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktreeWithOwnEdit(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktreeWithOwnEdit(fx, "repo-b", "src/b.ts"); + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const files = await (executor as any).captureWorkspaceModifiedFiles(task); + expect(files).toContain("repo-a/src/a.ts"); + expect(files).toContain("repo-b/src/b.ts"); + expect(files).toHaveLength(2); + }); + + it("edge: a repo with undefined baseCommitSha still captures via merge-base fallback (no `git diff undefined..HEAD`)", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktreeWithOwnEdit(fx, "repo-a", "src/a.ts"); + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { + branch: BRANCH, + workspaceWorktrees: { + // baseCommitSha intentionally undefined → resolveDiffBaseRef merge-base(HEAD, main). + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH }, + }, + }); + + const files = await (executor as any).captureWorkspaceModifiedFiles(task); + expect(files).toEqual(["repo-a/src/a.ts"]); + }); + + it("contamination: a foreign commit in a sub-repo range fires the divergence audit and is excluded from attributed files", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktreeWithOwnEdit(fx, "repo-a", "src/a.ts"); + // Land a FOREIGN commit (different FN-id) onto the same fusion/ branch range. + const foreignFile = "src/foreign.ts"; + writeFileSync(path.join(a.worktreePath, "src", "foreign.ts"), "// foreign\n", "utf-8"); + execSync(`git add ${foreignFile}`, { cwd: a.worktreePath, stdio: "pipe" }); + execSync('git commit -m "feat(FN-OTHER): sneaky foreign change"', { cwd: a.worktreePath, stdio: "pipe" }); + + const dbAudit = vi.fn().mockResolvedValue(undefined); + const audit = { + database: dbAudit, + filesystem: vi.fn().mockResolvedValue(undefined), + git: vi.fn().mockResolvedValue(undefined), + }; + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + }, + }); + + const files = await (executor as any).captureWorkspaceModifiedFiles(task, audit as any, "post-session"); + // Own file attributed, foreign file excluded from the attributed set. + expect(files).toEqual(["repo-a/src/a.ts"]); + expect(files).not.toContain("repo-a/src/foreign.ts"); + // The contamination/divergence audit fired for this repo (raw 2 files vs attributed 1). + const contaminationCall = dbAudit.mock.calls.find( + ([evt]) => evt?.type === "task:worktree-contamination-detected", + ); + expect(contaminationCall).toBeTruthy(); + expect(contaminationCall![0].metadata.rawDiffFileCount).toBeGreaterThan(contaminationCall![0].metadata.attributedFileCount); + }); +}); + +describeIfGit("U1 KTD2 — verifyWorktreeInvariants iterates per worktree, preserving the result union", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("happy: every worktree on fusion/ with matching toplevel → {ok:true}", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktreeWithOwnEdit(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktreeWithOwnEdit(fx, "repo-b", "src/b.ts"); + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).verifyWorktreeInvariants(task); + expect(result).toEqual({ ok: true }); + }); + + it("error: a worktree HEAD off fusion/ → {ok:false, reason:'wrong_branch', repo, observed, expected} (NOT {ok:true})", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktreeWithOwnEdit(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktreeWithOwnEdit(fx, "repo-b", "src/b.ts"); + // Drift repo-b's worktree off fusion/ onto a different branch. + execSync("git checkout -b some-other-branch", { cwd: b.worktreePath, stdio: "pipe" }); + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).verifyWorktreeInvariants(task); + expect(result.ok).toBe(false); + expect(result.reason).toBe("wrong_branch"); + expect(result.repo).toBe("repo-b"); + expect(result.observed).toBe("some-other-branch"); + expect(result.expected).toBe(BRANCH); + }); + + it("regression: a zero-acquire workspace task (empty map) verifies vacuously → {ok:true}", async () => { + fx = await createWorkspaceFixture(); + const executor = workspaceExecutor(fx); + const task = makeTask(TASK_ID, { branch: BRANCH, workspaceWorktrees: {} }); + const result = await (executor as any).verifyWorktreeInvariants(task); + expect(result).toEqual({ ok: true }); + }); +}); + +describeIfGit("U1 — single-repo (non-workspace) task: capture/verify unchanged", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("regression: non-workspace verifyWorktreeInvariants still runs the singular path and passes for a real worktree", async () => { + fx = await createWorkspaceFixture(); + // Single-repo executor rooted at repo-a itself (no workspaceConfig). + const repoDir = fx.repoPath("repo-a"); + const worktreePath = path.join(repoDir, ".worktrees", "fn-001"); + const base = execSync("git rev-parse HEAD", { cwd: repoDir, encoding: "utf-8" }).trim(); + execSync(`git worktree add -b fusion/fn-001 ${worktreePath} HEAD`, { cwd: repoDir, stdio: "pipe" }); + configureIdentity(worktreePath); + writeFileSync(path.join(worktreePath, "single.ts"), "// x\n", "utf-8"); + execSync("git add single.ts", { cwd: worktreePath, stdio: "pipe" }); + execSync('git commit -m "feat(FN-001): single"', { cwd: worktreePath, stdio: "pipe" }); + + const store = createStore(); + const executor = new TaskExecutor(store, repoDir); // no workspaceConfig → singular path + const task = makeTask("FN-001", { branch: "fusion/fn-001", worktree: worktreePath, baseCommitSha: base }); + + const result = await (executor as any).verifyWorktreeInvariants(task); + expect(result).toEqual({ ok: true }); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index a5e9063a5b..16bb0d4a56 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -66,7 +66,7 @@ import { VERIFICATION_LOG_MAX_CHARS, type VerificationResult, } from "./verification-utils.js"; -import { canonicalStepInstanceBranchName, generateWorktreeName, resolveTaskWorkingBranch } from "./worktree-names.js"; +import { canonicalFusionBranchName, canonicalStepInstanceBranchName, generateWorktreeName, resolveTaskWorkingBranch } from "./worktree-names.js"; import { resolveTaskWorktreePath, resolveWorktreesDir } from "./worktree-paths.js"; import { Type, type Static } from "@earendil-works/pi-ai"; import { describeModel, promptWithFallback, compactSessionContext } from "./pi.js"; @@ -7895,6 +7895,47 @@ export class TaskExecutor { const allSuccess = results.every(r => r.success); if (allSuccess) { const updatedTask = await this.store.getTask(task.id); + // FNXC:Workspace 2026-06-21-23:30: KTD1 — per-repo post-session capture. + // The singular call below runs UNGATED with worktreePath = the browse-only non-git workspace root and silently returns [] (resolveDiffBaseRef swallows the git failure at the root). In workspace mode there is nothing to diff at the root; the real changes live in each acquired sub-repo worktree. So we ADD (not replace) a workspace branch that loops `task.workspaceWorktrees` and reuses the EXISTING captureModifiedFiles per repo — reusing it (rather than hand-building `git diff ..HEAD`) gives us the merge-base fallback for an undefined repo.baseCommitSha (resolveDiffBaseRef) AND restores the contamination/divergence audit (filterFilesToOwnTaskCommits) for free per repo. Returned files are repo-prefixed (e.g. `repo-a/src/foo.ts`) and aggregated into task.modifiedFiles. + if (this.workspaceConfig) { + const workspaceWorktrees = updatedTask.workspaceWorktrees ?? {}; + const aggregated = await this.captureWorkspaceModifiedFiles(updatedTask, audit, "post-session"); + for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + // Per-repo branch-attribution audit (cwd = sub-repo). Run against repo.worktreePath/repo.branch, NOT the non-git root (a root call would fail and surface nothing). The contamination signal already rides on captureWorkspaceModifiedFiles above; this is the supplementary commit-attribution surface (FN-5233 pattern). + try { + const attributionBase = await this.resolveContaminationBaseRef(repo.worktreePath); + if (attributionBase && repo.branch) { + const attribution = await reportBranchAttribution(repo.worktreePath, repo.branch, attributionBase, task.id); + const hasAnomaly = attribution.foreign.length > 0 || attribution.unattributed.length > 0 || attribution.ownUntrailed.length > 0; + if (hasAnomaly) { + const summary = `branch-attribution anomalies on ${repoRel}@${repo.branch}: foreign=${attribution.foreign.length}, unattributed=${attribution.unattributed.length}, ownUntrailed=${attribution.ownUntrailed.length}, ownTrailed=${attribution.ownTrailed}`; + executorLog.warn(`${task.id}: ${summary}`); + await this.store.logEntry(task.id, `[branch-attribution] ${summary}`, undefined, this.getRunContextFor(task.id)); + await audit.git({ + type: "branch:attribution-anomaly", + target: repo.branch, + metadata: { + taskId: task.id, + repo: repoRel, + baseSha: attributionBase, + ownTrailed: attribution.ownTrailed, + foreign: attribution.foreign, + unattributed: attribution.unattributed, + ownUntrailed: attribution.ownUntrailed, + }, + }); + } + } + } catch (attributionErr: unknown) { + executorLog.warn(`${task.id}: post-session per-repo branch-attribution audit failed for ${repoRel}: ${attributionErr instanceof Error ? attributionErr.message : String(attributionErr)}`); + } + } + if (aggregated.length > 0) { + await this.store.updateTask(task.id, { modifiedFiles: aggregated }); + executorLog.log(`${task.id}: captured ${aggregated.length} modified files across ${Object.keys(workspaceWorktrees).length} sub-repo(s)`); + await audit.filesystem({ type: "file:capture-modified", target: task.id, metadata: { files: aggregated } }); + } + } else { const modifiedFiles = await this.captureModifiedFiles(worktreePath, updatedTask.baseCommitSha, task.id, audit, "post-session"); if (modifiedFiles.length > 0) { await this.store.updateTask(task.id, { modifiedFiles }); @@ -7936,6 +7977,7 @@ export class TaskExecutor { } catch (attributionErr: unknown) { executorLog.warn(`${task.id}: post-session branch-attribution audit failed: ${attributionErr instanceof Error ? attributionErr.message : String(attributionErr)}`); } + } // end !this.workspaceConfig singular capture (FNXC:Workspace KTD1) this.scheduleCompletedTaskWatchdog(task.id, "step-session completion"); if (await this.shouldDeferCompletionForGlobalPause(task.id, "before workflow steps after step-session completion")) { @@ -10502,10 +10544,87 @@ export class TaskExecutor { worktreePathOverride?: string, allowReanchor = true, options?: { noOpCompletion?: boolean; noOpCompletionReason?: string }, - ): Promise<{ ok: true } | { ok: false; reason: "wrong_toplevel" | "wrong_branch" | "no_commits"; observed: string; expected: string }> { + ): Promise<{ ok: true } | { ok: false; reason: "wrong_toplevel" | "wrong_branch" | "no_commits"; observed: string; expected: string; repo?: string }> { const settings = await this.store.getSettings(); - // FNXC:Workspace 2026-06-21-12:00: KTD1/KTD2 — workspace tasks have no root worktree and no single `task.worktree`; the singular per-task invariant is meaningless against the non-git root. Phase B (master U3) iterates this check per sub-repo worktree. Until then it is gated OFF in workspace mode so fn_task_done (its only caller path) does not requeue a zero-acquire workspace task for "missing task.worktree". + // FNXC:Workspace 2026-06-21-23:30: KTD2 — un-stubbed per-repo worktree-invariant verification. + // Phase A returned a flat {ok:true} stub here (no root worktree to verify against the non-git root). Phase B iterates every `task.workspaceWorktrees` entry, asserting (a) the sub-repo worktree's git toplevel matches the recorded repo.worktreePath and (b) its HEAD is on the recorded `fusion/` branch (repo.branch). The result union is PRESERVED EXACTLY — `{ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected}` — because the :10889 consumer switches on `reason` to drive requeue/handoff (:10894-10936). We ADD an optional `repo` field to the failure shape (purely additive; the consumer only reads reason/observed/expected) and return the FIRST failing repo. A zero-acquire workspace task (empty map) verifies vacuously → {ok:true}, matching Phase A so fn_task_done does not requeue it. if (this.workspaceConfig) { + const workspaceWorktrees = task.workspaceWorktrees ?? {}; + for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + const expectedBranch = repo.branch || canonicalFusionBranchName(task.id); + // Skip git checks if the worktree dir is gone (mirrors the singular FN-009 carve-out below): completion does not require a live worktree on disk. + if (!existsSync(repo.worktreePath)) { + executorLog.log(`${task.id}: workspace worktree for ${repoRel} not found at ${repo.worktreePath} — skipping git validation`); + continue; + } + let expectedWorktreeRealpath: string; + try { + expectedWorktreeRealpath = canonicalizePath(repo.worktreePath); + } catch (error) { + return { + ok: false, + reason: "wrong_toplevel", + repo: repoRel, + observed: `unresolvable repo worktree (${repo.worktreePath}): ${error instanceof Error ? error.message : String(error)}`, + expected: `resolvable worktree for ${repoRel}`, + }; + } + try { + const { stdout } = await execAsync("git rev-parse --show-toplevel", { + cwd: repo.worktreePath, + encoding: "utf-8", + timeout: 10_000, + maxBuffer: 1024 * 1024, + }); + const observedTopLevelRaw = stdout.trim(); + if (observedTopLevelRaw) { + const observedTopLevel = canonicalizePath(observedTopLevelRaw); + if (observedTopLevel !== expectedWorktreeRealpath) { + return { + ok: false, + reason: "wrong_toplevel", + repo: repoRel, + observed: observedTopLevel, + expected: expectedWorktreeRealpath, + }; + } + } + } catch (error) { + return { + ok: false, + reason: "wrong_toplevel", + repo: repoRel, + observed: error instanceof Error ? error.message : String(error), + expected: expectedWorktreeRealpath, + }; + } + try { + const { stdout } = await execAsync("git rev-parse --abbrev-ref HEAD", { + cwd: repo.worktreePath, + encoding: "utf-8", + timeout: 10_000, + maxBuffer: 1024 * 1024, + }); + const observedBranch = stdout.trim(); + if (observedBranch && observedBranch !== expectedBranch) { + return { + ok: false, + reason: "wrong_branch", + repo: repoRel, + observed: observedBranch, + expected: expectedBranch, + }; + } + } catch (error) { + return { + ok: false, + reason: "wrong_branch", + repo: repoRel, + observed: error instanceof Error ? error.message : String(error), + expected: expectedBranch, + }; + } + } return { ok: true }; } const branchName = resolveTaskWorkingBranch(task); @@ -12264,6 +12383,26 @@ ${failureFeedback} } } + /** + * FNXC:Workspace 2026-06-21-23:30: KTD1 — per-repo modified-file capture for workspace tasks. + * Loops `task.workspaceWorktrees` and REUSES `captureModifiedFiles` per sub-repo (NOT a hand-built `git diff`), so each repo gets: (a) resolveDiffBaseRef's merge-base fallback when repo.baseCommitSha is undefined, and (b) the filterFilesToOwnTaskCommits raw-vs-attributed divergence/contamination audit for free. Returned files are repo-prefixed (`/`) and aggregated, so a downstream File-Scope check / merge can attribute each change to its sub-repo. Returns [] for a zero-acquire workspace task. + */ + private async captureWorkspaceModifiedFiles( + task: Task, + audit?: RunAuditor, + source = "post-session", + ): Promise { + const workspaceWorktrees = task.workspaceWorktrees ?? {}; + const aggregated: string[] = []; + for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + const repoFiles = await this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source); + for (const file of repoFiles) { + aggregated.push(`${repoRel}/${file}`); + } + } + return aggregated; + } + private async captureUncommittedModifiedFiles(worktreePath: string): Promise { try { const [unstaged, staged] = await Promise.all([ From 81edbeefbd6ced6b408d0ced4deb8e0915187f9e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 22:44:40 -0700 Subject: [PATCH 2/6] =?UTF-8?q?feat(workspace):=20Phase=20B=20U2=20?= =?UTF-8?q?=E2=80=94=20per-repo=20review=20(both=20sites)=20+=20fn=5Ftask?= =?UTF-8?q?=5Fdone=20verify=20+=20scope-leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In workspace mode both review entry points and the completion guards now iterate every acquired sub-repo. A shared reviewWorkspacePerRepo loops task.workspaceWorktrees and invokes the existing single-cwd reviewStep once per repo (cwd = the sub-repo — the reviewer agent runs its own git diff there), aggregating repo-tagged verdicts as a conjunction: the task is reviewed only if every repo APPROVEs; the first non-APPROVE repo's verdict becomes the aggregate. Both call sites loop — the in-session fn_review_step tool AND the step-inversion seam (createReviewStepTool and the stepReview workflow seam) — so no review surface silently scopes to the non-git root (FN-5893). reviewStep itself stays single-cwd; the callers loop. fn_task_done completion verification iterates per repo: verifyWorktreeInvariants (from U1) already covers all worktrees, and evaluateTaskDoneScopeLeak now loops each sub-repo (cwd + repo.baseCommitSha, repo-prefixed touched files vs the repo-prefixed declared File Scope), blocking on the first repo with off-scope files and naming it. Both return shapes preserved (ReviewResult; {blocked,message}). New workspace-paths.ts repo-prefix helper (deriveRepoForPath/splitRepoScopedPath/ deriveRepoScopeSubset; segment-wise longest-prefix match, unscoped fallback) — master U5 reuses it. Singular non-workspace path unchanged. 16 new fixture tests. Gate green: typecheck, lint, build, test:gate (649+58). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ace-phase-b-u2-per-repo-review-taskdone.md | 5 + .../executor-workspace-taskdone.test.ts | 220 ++++++++++++++++++ .../src/__tests__/reviewer-workspace.test.ts | 213 +++++++++++++++++ packages/engine/src/executor.ts | 180 +++++++++++--- packages/engine/src/workspace-paths.ts | 117 ++++++++++ 5 files changed, 708 insertions(+), 27 deletions(-) create mode 100644 .changeset/workspace-phase-b-u2-per-repo-review-taskdone.md create mode 100644 packages/engine/src/__tests__/executor-workspace-taskdone.test.ts create mode 100644 packages/engine/src/__tests__/reviewer-workspace.test.ts create mode 100644 packages/engine/src/workspace-paths.ts diff --git a/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md b/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md new file mode 100644 index 0000000000..efd5004886 --- /dev/null +++ b/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": minor +--- + +Workspace mode (Phase B, U2): per-repo review at both review entry points plus per-repo `fn_task_done` completion + scope-leak verification. In workspace mode both review call sites (the in-session `fn_review_step` tool and the step-inversion review seam) now loop the single-cwd `reviewStep` once per acquired sub-repo (cwd = each repo's worktree) and aggregate the repo-tagged verdicts as a conjunction — the task is reviewed only when every sub-repo approves, and the first failing sub-repo's verdict (with repo-tagged findings) drives the existing verdict→edge mapping. `fn_task_done` now verifies worktree invariants per acquired repo and iterates the scope-leak guard per sub-repo (cwd = repo worktree, repo `baseCommitSha`), blocking completion on any sub-repo carrying off-scope changes and naming the repo. Adds a minimal shared repo-prefix-derivation helper (`workspace-paths.ts`). Single-repo behavior is unchanged. diff --git a/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts new file mode 100644 index 0000000000..9f24aaa75c --- /dev/null +++ b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts @@ -0,0 +1,220 @@ +/* +FNXC:Workspace 2026-06-22-00:30: +U2 KTD4 — per-repo fn_task_done completion verification: per-repo scope-leak guard + per-repo worktree-invariant +verify. These drive the REAL TaskExecutor methods against a REAL two-repo git fixture under a NON-git workspace +root (createWorkspaceFixture), so a leaked singular-root capture/verify would silently pass and the test would +catch it. Narrow seams (FN-5048): we set `(executor as any).workspaceConfig` directly and stub only the store +methods the guards read (parseFileScopeFromPrompt, logEntry, getRunContextFor) — no mock-the-world child_process. + +Coverage: +- scope-leak error: an uncommitted in-scope vs OFF-scope change in repo A → evaluateTaskDoneScopeLeak blocks, + message NAMES repo-a (per-repo guard fires; singular root would silently pass). +- verify error: a worktree HEAD off fusion/ → verifyWorktreeInvariants blocks (wrong_branch, repo-tagged). +- all-clean: a two-repo task with only in-scope changes → scope-leak does NOT block. +- helper: deriveRepoForPath / deriveRepoScopeSubset / splitRepoScopedPath unit cases (wolf-server/src/** → wolf-server; + non-matching first segment → unscoped). +- regression: single-repo (non-workspace) task → singular scope-leak path unchanged. +*/ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import { execSync } from "node:child_process"; +import { mkdirSync, writeFileSync } from "node:fs"; +import path from "node:path"; +import type { Task, TaskStore, WorkspaceConfig, Settings } from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { + deriveRepoForPath, + deriveRepoScopeSubset, + splitRepoScopedPath, + UNSCOPED_REPO, +} from "../workspace-paths.js"; +import { createWorkspaceFixture, hasGit, type WorkspaceFixture } from "./_workspace-fixture.js"; + +const describeIfGit = hasGit ? describe : describe.skip; + +const TASK_ID = "FN-1001"; +const BRANCH = "fusion/fn-1001"; + +// reviewLevel=1 + block enforcement is the only mode that BLOCKS (else warn). +const SETTINGS: Settings = { autoMerge: false, planOnlyScopeLeakEnforcement: "block" } as Settings; +const PROMPT = "## Review Level: 1 (Plan Only)\n"; + +function createStore(declaredScope: string[]): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + parseFileScopeFromPrompt: vi.fn().mockResolvedValue(declaredScope), + logEntry: vi.fn().mockResolvedValue(undefined), + getRunContextFor: vi.fn(), + getSettings: vi.fn().mockResolvedValue(SETTINGS), + }) as unknown as TaskStore & EventEmitter; +} + +function makeTask(overrides: Partial = {}): Task { + return { + id: TASK_ID, + title: "WS", + description: "", + column: "in-progress", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + } as Task; +} + +function configureIdentity(dir: string): void { + execSync('git config user.email "test@example.com"', { cwd: dir, stdio: "pipe" }); + execSync('git config user.name "Test"', { cwd: dir, stdio: "pipe" }); +} + +/** Add a fusion/ worktree to a sub-repo with one committed in-scope edit; return its handle. */ +function addRepoWorktree(fx: WorkspaceFixture, repoRel: string, fileName: string): { worktreePath: string; baseCommitSha: string } { + const repoDir = fx.repoPath(repoRel); + const baseCommitSha = fx.git(repoRel, "git rev-parse HEAD"); + const worktreePath = path.join(repoDir, ".worktrees", "fn-ws-1"); + fx.git(repoRel, `git worktree add -b ${BRANCH} ${worktreePath} HEAD`); + configureIdentity(worktreePath); + mkdirSync(path.dirname(path.join(worktreePath, fileName)), { recursive: true }); + writeFileSync(path.join(worktreePath, fileName), "// in-scope\n", "utf-8"); + execSync(`git add ${fileName}`, { cwd: worktreePath, stdio: "pipe" }); + execSync(`git commit -m "feat(${TASK_ID}): edit ${fileName}"`, { cwd: worktreePath, stdio: "pipe" }); + return { worktreePath, baseCommitSha }; +} + +function workspaceExecutor(fx: WorkspaceFixture, store: TaskStore & EventEmitter): TaskExecutor { + const executor = new TaskExecutor(store, fx.rootDir); + (executor as any).workspaceConfig = { repos: fx.repos } as WorkspaceConfig; + return executor; +} + +describe("U2 — workspace-paths repo-prefix helper (unit)", () => { + const repos = ["wolf-server", "repo-a", "apps/web"]; + it("deriveRepoForPath: first-segment match → that repo", () => { + expect(deriveRepoForPath("wolf-server/src/index.ts", repos)).toBe("wolf-server"); + expect(deriveRepoForPath("repo-a/src/a.ts", repos)).toBe("repo-a"); + }); + it("deriveRepoForPath: longest nested-key match wins", () => { + expect(deriveRepoForPath("apps/web/page.tsx", repos)).toBe("apps/web"); + }); + it("deriveRepoForPath: non-matching first segment → unscoped", () => { + expect(deriveRepoForPath(".changeset/x.md", repos)).toBe(UNSCOPED_REPO); + expect(deriveRepoForPath("other/thing.ts", repos)).toBe(UNSCOPED_REPO); + expect(deriveRepoForPath("repo-ab/x.ts", repos)).toBe(UNSCOPED_REPO); // segment-wise, not substring + }); + it("splitRepoScopedPath: strips the repo prefix for the repo-local remainder", () => { + expect(splitRepoScopedPath("wolf-server/src/x.ts", repos)).toEqual({ repo: "wolf-server", relativePath: "src/x.ts" }); + expect(splitRepoScopedPath("other/x.ts", repos)).toEqual({ repo: UNSCOPED_REPO, relativePath: "other/x.ts" }); + }); + it("deriveRepoScopeSubset: returns repo-local scope patterns for one repo", () => { + const scope = ["wolf-server/src/**", "repo-a/lib/x.ts", "apps/web/page.tsx"]; + expect(deriveRepoScopeSubset(scope, "wolf-server")).toEqual(["src/**"]); + expect(deriveRepoScopeSubset(scope, "repo-a")).toEqual(["lib/x.ts"]); + // repo-root scope entry maps to whole-repo ** + expect(deriveRepoScopeSubset(["repo-a"], "repo-a")).toEqual(["**"]); + }); +}); + +describeIfGit("U2 KTD4 — per-repo scope-leak guard in fn_task_done", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("error: an off-scope change in repo A blocks completion and NAMES repo-a", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + // Off-scope STAGED-but-uncommitted change in repo-a (outside declared `repo-a/src/**`). + // captureUncommittedModifiedFiles reads `git diff`/`--cached`, so the leak must be tracked + // (staged) to register — an untracked file is invisible to the guard by design. + writeFileSync(path.join(a.worktreePath, "OFFSCOPE.md"), "// leak\n", "utf-8"); + execSync("git add OFFSCOPE.md", { cwd: a.worktreePath, stdio: "pipe" }); + // Declared scope is repo-prefixed and only covers src/** in each repo. + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("repo-a"); + expect(result.message).toContain("OFFSCOPE.md"); + }); + + it("all-clean: only in-scope changes in both repos → not blocked", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(false); + }); +}); + +describeIfGit("U2 KTD4 — per-repo worktree-invariant verify in fn_task_done", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("error: a worktree off fusion/ blocks completion via per-repo verify", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + execSync("git checkout -b drifted-branch", { cwd: b.worktreePath, stdio: "pipe" }); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).verifyWorktreeInvariants(task); + expect(result.ok).toBe(false); + expect(result.reason).toBe("wrong_branch"); + expect(result.repo).toBe("repo-b"); + }); +}); + +describeIfGit("U2 — single-repo (non-workspace) task: scope-leak unchanged", () => { + let fx: WorkspaceFixture; + afterEach(() => fx?.cleanup()); + + it("regression: singular scope-leak path still flags an off-scope change in the singular worktree", async () => { + fx = await createWorkspaceFixture(); + const repoDir = fx.repoPath("repo-a"); + const worktreePath = path.join(repoDir, ".worktrees", "fn-001"); + const base = execSync("git rev-parse HEAD", { cwd: repoDir, encoding: "utf-8" }).trim(); + execSync(`git worktree add -b fusion/fn-001 ${worktreePath} HEAD`, { cwd: repoDir, stdio: "pipe" }); + configureIdentity(worktreePath); + // Off-scope STAGED change (declared scope is `src/**`). Tracked so the guard sees it. + writeFileSync(path.join(worktreePath, "OFFSCOPE.md"), "// leak\n", "utf-8"); + execSync("git add OFFSCOPE.md", { cwd: worktreePath, stdio: "pipe" }); + + const store = createStore(["src/**"]); + const executor = new TaskExecutor(store, repoDir); // no workspaceConfig → singular path + const task = makeTask({ id: "FN-001", branch: "fusion/fn-001", worktree: worktreePath, baseCommitSha: base }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, worktreePath, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("OFFSCOPE.md"); + // Singular message carries no repo tag. + expect(result.message).not.toContain("repo="); + }); +}); diff --git a/packages/engine/src/__tests__/reviewer-workspace.test.ts b/packages/engine/src/__tests__/reviewer-workspace.test.ts new file mode 100644 index 0000000000..cab774f3aa --- /dev/null +++ b/packages/engine/src/__tests__/reviewer-workspace.test.ts @@ -0,0 +1,213 @@ +/* +FNXC:Workspace 2026-06-22-00:30: +U2 KTD3 — per-repo review (BOTH call sites) + conjunction aggregation tests. The reviewer is an AGENT +spawned with `cwd = worktree`; per-repo review means ONE reviewer agent per sub-repo with the CALLERS +looping the single-cwd `reviewStep`. These tests assert the LOOP + aggregation, not the reviewer's content: +`reviewStep` is mocked (the narrow AI seam — FN-5048: no mock-the-world, no real AI spawn) and we record +the cwd of each call. Coverage: +- conjunction: two-repo task → two reviewer passes (one per repo cwd); review record reflects both; reviewed + only when BOTH pass; one repo REVISE → aggregate REVISE tagged with that repo. +- finding tag: a finding in repo B is repo-tagged in the aggregated review body. +- in-session seam (createReviewStepTool / fn_review_step): a workspace task reviews each sub-repo cwd, not the root. +- step-inversion seam (createAuthoritativeWorkflowSeams().stepReview, executor.ts:5668): same — each sub-repo, not root. +- regression: single-repo (non-workspace) task → exactly one reviewStep call at the singular worktree. +*/ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import type { ReviewResult } from "../reviewer.js"; + +// Narrow AI seam: only reviewStep (the agent boundary) is mocked. Everything else is the real executor. +vi.mock("../reviewer.js", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, reviewStep: vi.fn() }; +}); + +import { reviewStep as mockedReviewStepFn } from "../reviewer.js"; +import { TaskExecutor } from "../executor.js"; +import { FOREACH_ACTIVE_CONTEXT_KEY } from "../workflow-node-handlers.js"; +import type { Task, TaskStore, WorkspaceConfig } from "@fusion/core"; + +const mockedReviewStep = vi.mocked(mockedReviewStepFn); + +const ROOT = "/tmp/ws-root"; // NON-git workspace root — must never be a review cwd in workspace mode. +const WT_A = "/tmp/ws-root/repo-a/.worktrees/fn-1"; +const WT_B = "/tmp/ws-root/repo-b/.worktrees/fn-1"; + +function makeStore(task: Task): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + getTask: vi.fn().mockResolvedValue(task), + getSettings: vi.fn().mockResolvedValue({ autoMerge: false }), + updateStep: vi.fn().mockResolvedValue(undefined), + logEntry: vi.fn().mockResolvedValue(undefined), + getRunContextFor: vi.fn(), + // mergeEffectiveSettings degrades to base on any resolver error; these reject → base used. + getTaskWorkflowSelection: vi.fn().mockRejectedValue(new Error("no workflow")), + getWorkflowDefinition: vi.fn().mockRejectedValue(new Error("no workflow")), + getWorkflowSettingValues: vi.fn().mockRejectedValue(new Error("no workflow")), + }) as unknown as TaskStore & EventEmitter; +} + +function makeTask(overrides: Partial = {}): Task { + return { + id: "FN-1", + title: "WS", + description: "", + column: "in-progress", + dependencies: [], + steps: [ + { name: "Step 0", status: "done" }, + { name: "Step 1", status: "in-progress" }, + ], + currentStep: 1, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + } as Task; +} + +const TWO_REPO_WORKTREES = { + "repo-a": { worktreePath: WT_A, branch: "fusion/fn-1", baseCommitSha: "aaa" }, + "repo-b": { worktreePath: WT_B, branch: "fusion/fn-1", baseCommitSha: "bbb" }, +}; + +/** Script reviewStep to return a per-cwd verdict and record the cwd it was called with. */ +function scriptReviewByCwd(byCwd: Record): string[] { + const seenCwds: string[] = []; + mockedReviewStep.mockImplementation((async (cwd: string) => { + seenCwds.push(cwd); + return byCwd[cwd] ?? { verdict: "APPROVE", review: `ok ${cwd}`, summary: `ok ${cwd}` }; + }) as any); + return seenCwds; +} + +function workspaceExecutor(store: TaskStore & EventEmitter): TaskExecutor { + const executor = new TaskExecutor(store, ROOT); + (executor as any).workspaceConfig = { repos: ["repo-a", "repo-b"] } as WorkspaceConfig; + return executor; +} + +beforeEach(() => { + mockedReviewStep.mockReset(); +}); +afterEach(() => { + vi.clearAllMocks(); +}); + +describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared loop both call sites use)", () => { + it("conjunction: two repos both APPROVE → aggregate APPROVE, one reviewer pass per repo cwd", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const seen: string[] = []; + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string, repo: string) => { + seen.push(cwd); + return { verdict: "APPROVE", review: `clean in ${repo}`, summary: `clean ${repo}` }; + }); + expect(seen).toEqual([WT_A, WT_B]); // one pass per sub-repo cwd, never ROOT + expect(result.verdict).toBe("APPROVE"); + expect(result.review).toContain("repo-a"); + expect(result.review).toContain("repo-b"); + }); + + it("conjunction: one repo REVISE → aggregate REVISE, tagged with the failing repo", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const result = await (executor as any).reviewWorkspacePerRepo(task, async (_cwd: string, repo: string) => { + return repo === "repo-b" + ? { verdict: "REVISE", review: `bug in ${repo}`, summary: `revise ${repo}` } + : { verdict: "APPROVE", review: `clean ${repo}`, summary: `clean ${repo}` }; + }); + expect(result.verdict).toBe("REVISE"); + expect(result.review).toContain("repo-b"); // finding repo-tagged + expect(result.review).toContain("bug in repo-b"); + expect(result.summary).toMatch(/^repo-b:/); + }); + + it("zero-acquire workspace task → UNAVAILABLE (caller routes; no fabricated APPROVE)", async () => { + const task = makeTask({ workspaceWorktrees: {} }); + const executor = workspaceExecutor(makeStore(task)); + const invoke = vi.fn(); + const result = await (executor as any).reviewWorkspacePerRepo(task, invoke); + expect(result.verdict).toBe("UNAVAILABLE"); + expect(invoke).not.toHaveBeenCalled(); + }); +}); + +describe("U2 KTD3 — in-session fn_review_step (createReviewStepTool) loops per sub-repo", () => { + it("workspace task: code review spawns one reviewer per sub-repo cwd, not the root", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const store = makeStore(task); + const executor = workspaceExecutor(store); + const seen = scriptReviewByCwd({ + [WT_A]: { verdict: "APPROVE", review: "a ok", summary: "a" }, + [WT_B]: { verdict: "APPROVE", review: "b ok", summary: "b" }, + }); + const tool = (executor as any).createReviewStepTool( + task.id, + ROOT, // singular worktreePath = the non-git root; workspace mode must NOT review it + "PROMPT", + new Map(), + { current: null }, + new Map(), + task, + undefined, + ); + const res = await tool.execute("call-1", { step: 1, type: "code", step_name: "Step 1", baseline: "base" }); + expect(seen).toEqual([WT_A, WT_B]); + expect(seen).not.toContain(ROOT); + // Aggregate APPROVE flows through the tool's verdict→text mapping unchanged. + expect(res.content[0].text).toBe("APPROVE"); + }); + + it("regression: single-repo (non-workspace) task → exactly one reviewStep call at the singular worktree", async () => { + const task = makeTask(); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); // no workspaceConfig → singular path + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "ok", summary: "ok" } }); + const tool = (executor as any).createReviewStepTool( + task.id, + WT_A, + "PROMPT", + new Map(), + { current: null }, + new Map(), + task, + undefined, + ); + await tool.execute("call-1", { step: 1, type: "code", step_name: "Step 1", baseline: "base" }); + expect(seen).toEqual([WT_A]); + }); +}); + +describe("U2 KTD3 — step-inversion review seam (executor.ts:5668) loops per sub-repo", () => { + it("workspace task: stepReview spawns one reviewer per sub-repo cwd, not active.worktreePath/root", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES, worktree: ROOT }); + const store = makeStore(task); + const executor = workspaceExecutor(store); + const seen = scriptReviewByCwd({ + [WT_A]: { verdict: "APPROVE", review: "a", summary: "a" }, + [WT_B]: { verdict: "APPROVE", review: "b", summary: "b" }, + }); + const seams = executor.createAuthoritativeWorkflowSeams({ autoMerge: false } as any); + // Drive the foreach-active step-review handler directly with a scripted active context. + const context = { + [FOREACH_ACTIVE_CONTEXT_KEY]: { stepIndex: 1, worktreePath: ROOT, baselineSha: "base" }, + } as any; + const result = await seams.stepReview!(task as any, context, { type: "code", advisory: true } as any); + expect(seen).toEqual([WT_A, WT_B]); + expect(seen).not.toContain(ROOT); + expect(result.verdict).toBe("APPROVE"); + }); + + it("regression: single-repo stepReview reviews the active worktree once", async () => { + const task = makeTask({ worktree: WT_A }); + const store = makeStore(task); + const executor = new TaskExecutor(store, ROOT); // no workspaceConfig + const seen = scriptReviewByCwd({ [WT_A]: { verdict: "APPROVE", review: "a", summary: "a" } }); + const seams = executor.createAuthoritativeWorkflowSeams({ autoMerge: false } as any); + const context = { [FOREACH_ACTIVE_CONTEXT_KEY]: { stepIndex: 1, worktreePath: WT_A, baselineSha: "base" } } as any; + await seams.stepReview!(task as any, context, { type: "code", advisory: true } as any); + expect(seen).toEqual([WT_A]); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 16bb0d4a56..74d0f45a2b 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -77,7 +77,7 @@ import { resolveExecutorSessionModel, } from "./agent-session-helpers.js"; import { buildSessionSkillContext } from "./session-skill-context.js"; -import { reviewStep, type ReviewVerdict } from "./reviewer.js"; +import { reviewStep, type ReviewVerdict, type ReviewResult } from "./reviewer.js"; import { resolveSandboxBackend } from "./sandbox/index.js"; import type { SandboxBackend } from "./sandbox/types.js"; import { ModelRegistry, SessionManager, type ToolDefinition, type AgentSession } from "@earendil-works/pi-coding-agent"; @@ -5664,9 +5664,14 @@ export class TaskExecutor { const settings = await mergeEffectiveSettings(this.store, detail, await this.store.getSettings()); const sem = this.options.semaphore; - const invokeReviewer = () => + // FNXC:Workspace 2026-06-22-00:30: KTD3 — step-inversion review seam loops per sub-repo. + // `reviewStep` stays single-cwd; THIS CALLER loops. Single-cwd by default reviews + // `worktreePath`; in workspace mode that is the browse-only non-git root, so we instead spawn + // one reviewer per acquired sub-repo (cwd = repo.worktreePath) via reviewWorkspacePerRepo and + // aggregate as a conjunction. `invokeReviewerForCwd` is the per-cwd reviewStep call both modes share. + const invokeReviewerForCwd = (cwd: string) => reviewStep( - worktreePath, + cwd, seamTask.id, stepIndex, stepName, @@ -5702,10 +5707,18 @@ export class TaskExecutor { onSessionEnded: (s) => this.unregisterSubagentSession(seamTask.id, s), }, ); + const runForCwd = (cwd: string) => { + const invoke = () => invokeReviewerForCwd(cwd); + return sem ? sem.runNested(invoke) : invoke(); + }; + const invokeReviewer = () => + this.workspaceConfig + ? this.reviewWorkspacePerRepo(detail, (cwd) => runForCwd(cwd)) + : runForCwd(worktreePath); let review: { verdict: ReviewVerdict; review: string; summary: string }; try { - review = sem ? await sem.runNested(invokeReviewer) : await invokeReviewer(); + review = await invokeReviewer(); } catch (err) { const message = err instanceof Error ? err.message : String(err); reviewerLog.error(`${seamTask.id}: step-review failed: ${message}`); @@ -10855,24 +10868,62 @@ export class TaskExecutor { return { blocked: false }; } - const [uncommittedTouchedFiles, branchCommittedFiles] = await Promise.all([ - this.captureUncommittedModifiedFiles(worktreePath), - this.captureModifiedFiles(worktreePath, task.baseCommitSha, task.id, audit, "scope-leak-guard"), - ]); - - const touchedFiles = [...new Set([...uncommittedTouchedFiles, ...branchCommittedFiles])]; - if (touchedFiles.length === 0) { - return { blocked: false }; + // FNXC:Workspace 2026-06-22-00:30: KTD4 — per-repo scope-leak guard. + // The singular capture below runs `captureUncommittedModifiedFiles` + `captureModifiedFiles` + // against `worktreePath`. In workspace mode `worktreePath` is the browse-only non-git workspace + // root, so both silently return [] (git failures swallowed) and the uncommitted-in-scope block + // never fires — a workspace task could complete with off-scope changes in any sub-repo. So we + // ITERATE every acquired sub-repo (cwd = repo.worktreePath, base = repo.baseCommitSha), + // repo-prefix each repo's touched files (`/`) so they compare against the task's + // repo-prefixed declared File Scope, and block on the FIRST repo carrying off-scope changes — + // naming the repo. The task-level preamble above (scopeOverride / declaredScope / enforcementMode) + // is shared and runs once. Return shape is preserved: `{blocked:false} | {blocked:true; message}`. + let touchedFiles: string[]; + let offendingRepo: string | undefined; + if (this.workspaceConfig) { + const workspaceWorktrees = task.workspaceWorktrees ?? {}; + const aggregatedOffScope: string[] = []; + for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + const [repoUncommitted, repoCommitted] = await Promise.all([ + this.captureUncommittedModifiedFiles(repo.worktreePath), + this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, "scope-leak-guard"), + ]); + const repoTouched = [...new Set([...repoUncommitted, ...repoCommitted])].map((f) => `${repoRel}/${f}`); + const repoOffScope = repoTouched + .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope)) + .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)); + if (repoOffScope.length > 0) { + // First offending repo wins (mirrors verifyWorktreeInvariants' first-failing-repo return). + if (!offendingRepo) offendingRepo = repoRel; + aggregatedOffScope.push(...repoOffScope); + } + } + touchedFiles = aggregatedOffScope; + if (touchedFiles.length === 0) { + return { blocked: false }; + } + } else { + const [uncommittedTouchedFiles, branchCommittedFiles] = await Promise.all([ + this.captureUncommittedModifiedFiles(worktreePath), + this.captureModifiedFiles(worktreePath, task.baseCommitSha, task.id, audit, "scope-leak-guard"), + ]); + touchedFiles = [...new Set([...uncommittedTouchedFiles, ...branchCommittedFiles])]; + if (touchedFiles.length === 0) { + return { blocked: false }; + } } - const offScopeFiles = touchedFiles - .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope)) - // FN-4811 follow-up: by convention every task may add its own changeset entry - // under `.changeset/`, so changeset files are always considered in-scope and - // never flagged by the scope-leak guard. The file-scope invariant at squash and - // the broader contamination guards still catch cross-task changeset leakage at - // a higher signal-to-noise ratio than the per-execution scope-leak warning. - .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)); + const offScopeFiles = (this.workspaceConfig + // In workspace mode `touchedFiles` is already the off-scope set (filtered per repo above). + ? touchedFiles + : touchedFiles + .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope)) + // FN-4811 follow-up: by convention every task may add its own changeset entry + // under `.changeset/`, so changeset files are always considered in-scope and + // never flagged by the scope-leak guard. The file-scope invariant at squash and + // the broader contamination guards still catch cross-task changeset leakage at + // a higher signal-to-noise ratio than the per-execution scope-leak warning. + .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath))); if (offScopeFiles.length === 0) { return { blocked: false }; } @@ -10887,14 +10938,16 @@ export class TaskExecutor { const offScopePreview = renderListPreview(offScopeFiles); const declaredScopePreview = renderListPreview(declaredScope); - const message = `[scope-leak] reviewLevel=${reviewLevel} enforcement=${enforcementMode} off-scope touched files [${offScopePreview}]; declared scope [${declaredScopePreview}]; total off-scope=${offScopeFiles.length} total scope=${declaredScope.length}`; + // Name the offending sub-repo in workspace mode so the operator/agent knows where to revert. + const repoTag = offendingRepo ? ` repo=${offendingRepo}` : ""; + const message = `[scope-leak] reviewLevel=${reviewLevel} enforcement=${enforcementMode}${repoTag} off-scope touched files [${offScopePreview}]; declared scope [${declaredScopePreview}]; total off-scope=${offScopeFiles.length} total scope=${declaredScope.length}`; executorLog.warn(`${task.id}: ${message}`); await this.store.logEntry(task.id, message, undefined, this.getRunContextFor(task.id)); if (enforcementMode === "block") { return { blocked: true, - message: `Plan-Only scope-leak guard refused fn_task_done. Off-scope paths: [${offScopePreview}]. Revert them before retrying (for example: git checkout -- ).`, + message: `Plan-Only scope-leak guard refused fn_task_done${offendingRepo ? ` (sub-repo ${offendingRepo})` : ""}. Off-scope paths: [${offScopePreview}]. Revert them before retrying (for example: git checkout -- ).`, }; } @@ -11337,8 +11390,13 @@ export class TaskExecutor { // result, so the soft breach of `limit` does not push real // LLM-active concurrency above the configured cap. const sem = options.semaphore; - const invokeReviewer = () => reviewStep( - worktreePath, taskId, step, step_name, + // FNXC:Workspace 2026-06-22-00:30: KTD3 — in-session fn_review_step loops per sub-repo. + // `reviewStep` stays single-cwd; THIS CALLER loops. Single-cwd by default reviews `worktreePath`; + // in workspace mode that is the browse-only non-git root, so we spawn one reviewer per acquired + // sub-repo (cwd = repo.worktreePath) via reviewWorkspacePerRepo and aggregate as a conjunction. + // `invokeReviewerForCwd` is the per-cwd reviewStep call both modes share. + const invokeReviewerForCwd = (cwd: string) => reviewStep( + cwd, taskId, step, step_name, reviewType, promptContent, baseline, { onText: (delta) => options.onAgentText?.(taskId, delta), @@ -11377,9 +11435,13 @@ export class TaskExecutor { onSessionEnded: (s) => this.unregisterSubagentSession(taskId, s), }, ); - const result = sem - ? await sem.runNested(invokeReviewer) - : await invokeReviewer(); + const runForCwd = (cwd: string) => { + const invoke = () => invokeReviewerForCwd(cwd); + return sem ? sem.runNested(invoke) : invoke(); + }; + const result = this.workspaceConfig + ? await this.reviewWorkspacePerRepo(currentTask, (cwd) => runForCwd(cwd)) + : await runForCwd(worktreePath); await store.logEntry( taskId, @@ -12403,6 +12465,70 @@ ${failureFeedback} return aggregated; } + /** + * FNXC:Workspace 2026-06-22-00:30: KTD3 — per-repo review by looping the EXISTING single-cwd reviewStep. + * The reviewer is an AGENT spawned with `cwd = worktree`, told (in prompt text, reviewer.ts) to run `git diff` + * itself — it does NOT read a diff passed in code. So per-repo review = ONE reviewer agent per sub-repo. We keep + * `reviewStep` single-cwd; the CALLERS loop. This helper is the shared loop+aggregate so both review entry points + * (`createReviewStepTool` and the step-inversion `stepReview` seam) iterate identically: it invokes the caller's + * own `invokeForCwd(cwd)` once per acquired worktree (cwd = repo.worktreePath) and aggregates the repo-tagged + * verdicts as a CONJUNCTION — the task is "reviewed" only if EVERY repo passes; the FIRST non-APPROVE repo's + * verdict becomes the aggregate verdict (mirroring verifyWorktreeInvariants' first-failing-repo return), and its + * findings are repo-tagged. A zero-acquire workspace task (empty map) returns UNAVAILABLE so the caller routes it + * rather than fabricating an APPROVE. + * + * Verdict severity for the conjunction: any RETHINK/REVISE/UNAVAILABLE fails the whole review; only all-APPROVE + * (or all-skipped UNAVAILABLE-advisory, handled by the caller) approves. We surface the first failing repo's exact + * verdict so the caller's existing verdict→edge mapping (APPROVE done-marking, REVISE block, RETHINK reset, + * UNAVAILABLE retry) is unchanged. + */ + private async reviewWorkspacePerRepo( + task: Task, + invokeForCwd: (cwd: string, repoRel: string) => Promise, + ): Promise { + const workspaceWorktrees = task.workspaceWorktrees ?? {}; + const entries = Object.entries(workspaceWorktrees); + if (entries.length === 0) { + // No acquired worktree — surface UNAVAILABLE so the caller routes it rather than + // fabricating an authoritative APPROVE for an un-reviewable workspace task. + return { + verdict: "UNAVAILABLE", + review: "No acquired sub-repo worktree to review (workspace task with zero worktrees).", + summary: "Skipped: no sub-repo worktree", + }; + } + + const reviewSections: string[] = []; + const summarySections: string[] = []; + let firstFailing: { repo: string; result: ReviewResult } | undefined; + for (const [repoRel, repo] of entries) { + const result = await invokeForCwd(repo.worktreePath, repoRel); + // Tag every per-repo finding with its sub-repo so downstream readers attribute it correctly. + reviewSections.push(`### [${repoRel}] ${result.verdict}\n${result.review}`); + summarySections.push(`[${repoRel}] ${result.verdict}: ${result.summary}`); + if (result.verdict !== "APPROVE" && !firstFailing) { + firstFailing = { repo: repoRel, result }; + } + } + + if (firstFailing) { + // Conjunction failed: the aggregate carries the FIRST failing repo's verdict (so the caller's + // verdict→edge mapping is identical to single-cwd), with the full repo-tagged review body. + return { + verdict: firstFailing.result.verdict, + review: `Workspace review failed in sub-repo \`${firstFailing.repo}\` (verdict ${firstFailing.result.verdict}). Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, + summary: `${firstFailing.repo}: ${firstFailing.result.verdict} — ${summarySections.join(" | ")}`, + }; + } + + // Every sub-repo approved → the task is reviewed (conjunction satisfied). + return { + verdict: "APPROVE", + review: `All ${entries.length} sub-repo(s) approved. Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, + summary: `APPROVE across ${entries.length} sub-repo(s): ${summarySections.join(" | ")}`, + }; + } + private async captureUncommittedModifiedFiles(worktreePath: string): Promise { try { const [unstaged, staged] = await Promise.all([ diff --git a/packages/engine/src/workspace-paths.ts b/packages/engine/src/workspace-paths.ts new file mode 100644 index 0000000000..308c559341 --- /dev/null +++ b/packages/engine/src/workspace-paths.ts @@ -0,0 +1,117 @@ +/* +FNXC:Workspace 2026-06-22-00:30: +Minimal shared repo-prefix-derivation helper for workspace mode (Phase B U2; master U5 reuses it). A workspace task's File Scope, modified-file list, and review/scope-leak findings are all repo-prefixed (`/`). Per-repo review and per-repo scope-leak need to map a path → its owning sub-repo, and to derive each repo's File-Scope subset (so a reviewer at `cwd = repo.worktreePath` and a per-repo scope-leak check evaluate only that repo's declared paths). + +NO lease logic lives here (file-scope leases are Phase C / master U7). This module is intentionally dependency-light (pure string/path math) so it can be reused across the executor, reviewer callers, and the later merge loop without pulling in executor state. + +Matching rule: canonicalize the path to forward-slash relative segments, then pick the LONGEST configured repo key that is a path-segment prefix of the file path. Longest-prefix (not naive first-segment) correctly handles nested repo keys like `apps/web` while still satisfying the simple `wolf-server/src/** → wolf-server` case. A path that matches no configured repo (absolute paths outside the workspace, root-level files like `.changeset/x.md`, or a first segment that is not a repo) derives to the `UNSCOPED` sentinel. +*/ + +/** Sentinel returned when a path does not belong to any configured sub-repo. */ +export const UNSCOPED_REPO = "unscoped" as const; + +/** + * Normalize a workspace-relative path token to forward-slash form with no leading + * `./`, no leading/trailing slashes, and collapsed duplicate slashes. Mirrors the + * executor's `normalizeWorkflowScopePath` shape so File-Scope tokens and modified + * files compare consistently, but kept local to avoid an executor import cycle. + */ +function normalizeRepoRelPath(value: string): string { + return value + .trim() + .replace(/\\/g, "/") + .replace(/^\.\//, "") + .replace(/\/+/g, "/") + .replace(/^\/+/, "") + .replace(/\/+$/, ""); +} + +/** Split a normalized path into non-empty segments. */ +function segmentsOf(value: string): string[] { + const normalized = normalizeRepoRelPath(value); + return normalized ? normalized.split("/") : []; +} + +/** + * Return true when `repoSegs` is a leading segment-prefix of `pathSegs`. + * Segment-wise (not substring) so `repo-a` does NOT match `repo-ab/...`. + */ +function isSegmentPrefix(repoSegs: string[], pathSegs: string[]): boolean { + if (repoSegs.length === 0 || repoSegs.length > pathSegs.length) return false; + for (let i = 0; i < repoSegs.length; i++) { + if (repoSegs[i] !== pathSegs[i]) return false; + } + return true; +} + +/** + * Derive the configured sub-repo that owns `filePath`, or {@link UNSCOPED_REPO}. + * + * `repos` are the configured workspace sub-repo relative keys (from + * `workspaceConfig.repos` or `Object.keys(task.workspaceWorktrees)`). The LONGEST + * matching repo key wins so nested repos (`apps/web` vs `apps`) resolve to the + * most specific owner. + */ +export function deriveRepoForPath(filePath: string, repos: readonly string[]): string { + const pathSegs = segmentsOf(filePath); + if (pathSegs.length === 0) return UNSCOPED_REPO; + let best: string | null = null; + let bestLen = 0; + for (const repo of repos) { + const repoSegs = segmentsOf(repo); + if (repoSegs.length === 0) continue; + if (isSegmentPrefix(repoSegs, pathSegs) && repoSegs.length > bestLen) { + best = normalizeRepoRelPath(repo); + bestLen = repoSegs.length; + } + } + return best ?? UNSCOPED_REPO; +} + +/** + * Result of splitting a repo-prefixed File-Scope entry into its owning repo and + * the repo-relative remainder (the path AS the reviewer at `cwd = repo` sees it). + */ +export interface RepoScopedPath { + /** Owning sub-repo key, or {@link UNSCOPED_REPO}. */ + repo: string; + /** The path with the repo prefix stripped (repo-local). Equals `path` when unscoped. */ + relativePath: string; +} + +/** + * Split a repo-prefixed path into `{ repo, relativePath }`. For `repo-a/src/x.ts` + * with `repos=["repo-a"]` → `{ repo:"repo-a", relativePath:"src/x.ts" }`. An + * unscoped path returns the whole normalized path as `relativePath`. + */ +export function splitRepoScopedPath(filePath: string, repos: readonly string[]): RepoScopedPath { + const repo = deriveRepoForPath(filePath, repos); + const normalized = normalizeRepoRelPath(filePath); + if (repo === UNSCOPED_REPO) { + return { repo, relativePath: normalized }; + } + const repoNormalized = normalizeRepoRelPath(repo); + const remainder = normalized.slice(repoNormalized.length).replace(/^\/+/, ""); + return { repo, relativePath: remainder }; +} + +/** + * Derive a single sub-repo's File-Scope subset from the task's full (repo-prefixed) + * declared scope. Returns the repo-LOCAL scope patterns (prefix stripped) so a + * per-repo reviewer or per-repo scope-leak check — operating with `cwd = repo` — + * can compare repo-local paths directly. Entries owned by other repos (or unscoped) + * are excluded. A scope entry whose prefix-stripped remainder is empty (the repo + * root itself, e.g. `repo-a` or `repo-a/`) maps to `**` (whole-repo scope). + */ +export function deriveRepoScopeSubset(declaredScope: readonly string[], repoRel: string): string[] { + const repoSegs = segmentsOf(repoRel); + if (repoSegs.length === 0) return []; + const subset: string[] = []; + for (const entry of declaredScope) { + const entrySegs = segmentsOf(entry); + if (!isSegmentPrefix(repoSegs, entrySegs)) continue; + const remainder = entrySegs.slice(repoSegs.length).join("/"); + subset.push(remainder === "" ? "**" : remainder); + } + return subset; +} From 0367fa54d9876f9a010634b2101f896e6022db62 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 22:44:53 -0700 Subject: [PATCH 3/6] docs(workspace): Phase B implementation plan (U3/U4) --- ...6-06-21-005-feat-workspace-phase-b-plan.md | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 docs/plans/2026-06-21-005-feat-workspace-phase-b-plan.md diff --git a/docs/plans/2026-06-21-005-feat-workspace-phase-b-plan.md b/docs/plans/2026-06-21-005-feat-workspace-phase-b-plan.md new file mode 100644 index 0000000000..15dd907348 --- /dev/null +++ b/docs/plans/2026-06-21-005-feat-workspace-phase-b-plan.md @@ -0,0 +1,145 @@ +--- +title: "feat: Workspace mode Phase B — per-repo capture, contamination, review, completion verify" +status: active +date: 2026-06-21 +type: feat +origin: docs/plans/2026-06-21-002-feat-workspace-mode-execution-model-plan.md (master plan, Phase B / U3·U4) +depth: deep +--- + +# feat: Workspace mode Phase B — per-repo capture, contamination, review, completion verify + +> **ID namespace:** local `U1·U2` decompose master-plan **U3, U4**. +> **Anchors below are feasibility-verified against the Phase-B base** (not the master plan's approximate numbers). + +## Summary + +Phase B makes the executor's capture / contamination / verify / review / completion paths iterate `task.workspaceWorktrees` per sub-repo, using each repo's own `baseCommitSha` (Phase A, U2). It does **not** simply "un-gate stubs" — the feasibility pass found capture/contamination/scope-leak are not gated at all today; they **silently degrade to empty** against the non-git root (git failures swallowed). Phase B adds the missing workspace branches and reuses the existing `captureModifiedFiles` machinery (whose `resolveDiffBaseRef` merge-base fallback + `filterFilesToOwnTaskCommits` contamination audit are exactly what's needed) per repo. + +Builds on Phase A (PR #1713). **Scope out:** the merge loop (master U6 = Phase C), self-healing (master U8 = Phase D). + +**Stacking:** off the Phase-A branch; PR diff includes the stack; must not merge until it lands. + +--- + +## Problem Frame + +Phase A rooted workspace sessions at the non-git workspace root and acquired per-repo worktrees, but the executor's change-capture, contamination, worktree-invariant, review, and completion-verify paths still operate on a single `task.worktree`. Against the non-git root they either are explicitly stubbed (one site) or silently produce empty results (the rest). Phase B routes each of these through every acquired sub-repo worktree, `cwd` = the sub-repo, diffing against that repo's `workspaceWorktrees[repo].baseCommitSha`, with repo-prefixed file lists so review/dashboard/later-merge keep repo context. + +--- + +## Key Technical Decisions + +### KTD1 — Per-repo change capture by **reusing `captureModifiedFiles`**, not a raw diff (master KTD7) +**Verified reality:** capture is **not** workspace-gated. The post-session call `captureModifiedFiles(worktreePath, …, "post-session")` (executor.ts **:7898**) runs ungated with `worktreePath` = the browse-only non-git root and returns `[]` only because `resolveDiffBaseRef`/`resolveContaminationBaseRef` swallow the git failure. So U1 **adds** a workspace branch at :7898 (and the sibling branch-attribution audit at **:7914**), it does not replace one. + +Per repo, call the **existing** `captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source)` — NOT a hand-built `git diff ..HEAD`. Reasons (all verified): (a) `repo.baseCommitSha` may be **undefined** (Phase A made base capture non-fatal); `resolveDiffBaseRef` (:~12184) handles that via a merge-base fallback. (b) the real **contamination** signal is the `filterFilesToOwnTaskCommits` raw-vs-attributed divergence audit **inside** `captureModifiedFiles` (:~12225-12246) — reusing it restores contamination for free. Prefix each repo's returned files with the repo path and aggregate into `task.modifiedFiles`. + +> **`assertCleanBranchAtBase` is a no-op** (branch-conflicts.ts: `void`s all params — "informational only"). Do **not** add a per-repo iteration of it; it would restore zero protection. Contamination comes from per-repo `captureModifiedFiles`. + +### KTD2 — `verifyWorktreeInvariants` iterates per acquired worktree, preserving its result union (master KTD7) +The **one** workspace stub in this region is `verifyWorktreeInvariants` returning `{ok:true}` at executor.ts **:10508** (def **:10500**). Un-stub it: iterate every `workspaceWorktrees` entry, asserting each HEAD is on `fusion/` and toplevel matches the recorded `worktreePath`. **Preserve the exact discriminated union** `{ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected}` (consumed at **:10889**; the `reason` enum drives the requeue/handoff branches at :10894-10936) — add a `repo` field to the failure shape; return the **first** failing repo. + +### KTD3 — Per-repo review by looping the **existing single-cwd `reviewStep`** N times (master KTD7) +**Decision (user-confirmed): accept the N× reviewer cost.** The reviewer is an **agent** spawned with `cwd` = worktree and told (in prompt text, reviewer.ts:~760) to run `git diff` itself — it does not read a diff passed in code. So per-repo review = spawning **one reviewer agent per sub-repo**. Architecture: the **callers loop** and call the existing single-cwd `reviewStep` (reviewer.ts **:122**) once per acquired worktree (cwd = repo, scope = prefix-derived subset); aggregate repo-tagged verdicts into the task's single review record as a **conjunction** (reviewed only if every repo passes). `reviewStep` itself stays single-cwd. + +**Both review call sites iterate (user-confirmed FN-5893 coverage):** +- `createReviewStepTool` → `reviewStep` (executor.ts **:11148**, the in-session `fn_review_step` path). +- the **step-inversion seam** `reviewStep(worktreePath=active.worktreePath || detail.worktree || this.rootDir, …)` at executor.ts **:5668** (foreach/step-inversion path). + +### KTD4 — `fn_task_done` completion verification iterates per repo, including the scope-leak guard (master KTD7) +`fn_task_done` (`createTaskDoneTool` executor.ts **:10832**) must, in workspace mode: (a) call the per-repo `verifyWorktreeInvariants` (KTD2) for every acquired worktree; (b) iterate the **scope-leak guard** `evaluateTaskDoneScopeLeak` (executor.ts **:10711**, invoked at **:11009**) per repo — it currently runs `captureUncommittedModifiedFiles(worktreePath)` + `captureModifiedFiles(worktreePath, task.baseCommitSha, …)` against the singular root and silently passes; per-repo iteration (cwd = sub-repo, `repo.baseCommitSha`) restores the uncommitted-in-scope block. Block completion on any dirty/misbound repo or uncommitted in-scope change, naming the repo. + +> **Repo-prefix derivation helper** (shared, master U5 will reuse): canonicalize → match first path segment to a configured repo → `unscoped` fallback. New `packages/engine/src/workspace-paths.ts`. Keep it minimal — no lease logic (Phase C / master U7). + +--- + +## Implementation Units + +> **Standing requirements:** `FNXC:Workspace ` comments; a `.changeset/*.md` (`@runfusion/fusion: minor`); FN-5048 (reuse the Phase-A `_workspace-fixture.ts` harness; real git only where the invariant requires it; fake timers; no mock-the-world); FN-5893 surface enumeration; the merge gate. Branch off Phase A (already checked out: `gsxdsm/workspace-phase-b`). + +### U1. Per-repo capture, contamination, and worktree-invariant verification (master U3) + +**Goal:** Change-capture, contamination, and `verifyWorktreeInvariants` cover every acquired sub-repo worktree with repo context and correct cwd. + +**Requirements:** KTD1, KTD2. + +**Dependencies:** none beyond Phase A. + +**Files:** +- `packages/engine/src/executor.ts` — **add** a workspace branch at the post-session capture **:7898** (+ attribution audit **:7914**) that loops `workspaceWorktrees` calling `captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, …)` per repo, repo-prefixing results; **un-stub** `verifyWorktreeInvariants` **:10508** to iterate per worktree preserving the `{ok|reason|observed|expected}` union (+ `repo`). +- `packages/engine/src/__tests__/executor-workspace-capture.test.ts` (new — real two-repo fixture via `_workspace-fixture.ts`) + +**Approach:** Per KTD1/KTD2. Reuse `captureModifiedFiles` (do not hand-build `git diff`); do not iterate the no-op `assertCleanBranchAtBase`. Singular non-workspace path unchanged. + +**Execution note:** Reuse `_workspace-fixture.ts`; commit edits onto each sub-repo's `fusion/` branch to exercise real diffs + the divergence audit. + +**Test scenarios:** +- Edits in repo A and B → `task.modifiedFiles` carries repo-prefixed paths from both, each diffed against its own `baseCommitSha`. (happy path) +- A repo with `baseCommitSha` undefined → capture still works via the merge-base fallback (no `git diff undefined..HEAD`). (edge — Phase A non-fatal base) +- A foreign commit in a sub-repo's range → the `filterFilesToOwnTaskCommits` divergence/contamination audit fires for that repo. (contamination) +- A worktree HEAD drifted off `fusion/` → `verifyWorktreeInvariants` returns `{ok:false, reason:'wrong_branch', repo, observed, expected}` (not `{ok:true}`); the `reason` enum is preserved for the :10889 consumer. (error path) +- Single-repo (non-workspace) task → capture/verify byte-for-byte identical. (regression) + +**Verification:** Capture + contamination audit + invariant verify run per acquired worktree with repo context; the result union is intact; single-repo unchanged. + +--- + +### U2. Per-repo review (both call sites) + `fn_task_done` completion + scope-leak verification (master U4) + +**Goal:** Review every acquired sub-repo (both review entry points) and block completion until every sub-repo passes review, invariant, and scope-leak checks. + +**Requirements:** KTD3, KTD4, KTD2. + +**Dependencies:** U1 (per-repo verify + capture). + +**Files:** +- `packages/engine/src/executor.ts` — `createReviewStepTool` **:11148** and the step-inversion seam **:5668** loop `reviewStep` per acquired worktree; `createTaskDoneTool` **:10832** calls per-repo verify (U1) + iterates `evaluateTaskDoneScopeLeak` **:10711** per repo. +- `packages/engine/src/reviewer.ts` — `reviewStep` (**:122**) stays single-cwd; callers loop. Aggregate repo-tagged verdicts (conjunction) into the task review record; reviewer findings carry the repo tag. +- `packages/engine/src/workspace-paths.ts` (new — the repo-prefix-derivation helper; master U5 reuses) +- `packages/engine/src/__tests__/reviewer-workspace.test.ts`, `packages/engine/src/__tests__/executor-workspace-taskdone.test.ts` (new) + +**Approach:** Per KTD3/KTD4. Both review sites loop the existing single-cwd `reviewStep` once per sub-repo (N reviewer agents — accepted cost) and aggregate as a conjunction. `fn_task_done` per-repo verify + per-repo scope-leak. + +**Test scenarios:** +- Two-repo task → two reviewer passes (one per repo cwd); review record reflects both; reviewed only when both pass. (conjunction) +- A reviewer finding in repo B is repo-tagged. (integration) +- Step-inversion review seam (:5668) for a workspace task reviews each sub-repo, not the non-git root. (FN-5893 second surface) +- `fn_task_done` with an uncommitted in-scope change in repo A → completion blocked, naming repo A (the scope-leak guard fires per-repo). (error path) +- `fn_task_done` with a worktree off `fusion/` → blocked via per-repo verify. (error path) +- The prefix helper: `wolf-server/src/**` → repo `wolf-server`; non-matching first segment → `unscoped`. (helper) +- Single-repo task → one review pass + singular scope-leak/verify, unchanged. (regression) + +**Verification:** A workspace task is reviewed/complete only when every sub-repo passes review + invariant + scope-leak; both review entry points iterate; single-repo unchanged. + +--- + +## Scope Boundaries + +**In scope:** per-repo capture/contamination/verify (U1); per-repo review at both call sites + `fn_task_done` verify + scope-leak (U2); the repo-prefix helper. + +### Deferred to Follow-Up Work (later phases) +- The per-repo merge loop, the landed predicate, the file-scope leases (master U5/U6/U7 = Phase C). +- Self-healing reconcilers, e2e (master U8/U9 = Phase D). +- Per-repo worktree teardown (carried Phase-A residual). +- Store-level **atomic** per-repo `workspaceWorktrees` merge — Phase A added a re-read mitigation; the fully-atomic merge is still open and **becomes reachable in Phase B** (multi-repo acquisition first exercised here). Track for Phase C. + +--- + +## Risks & Dependencies + +- **R1 — "Add a branch" vs "replace a stub" confusion.** Capture/contamination/scope-leak silently degrade (not gated); an implementer expecting a stub to replace won't find one. Mitigation: KTD1/KTD4 + U1/U2 cite the exact add sites (:7898/:7914, :10711) and the one real stub (:10508). +- **R2 — Hand-built `git diff` breaks on undefined base.** Mitigation: KTD1 mandates reusing `captureModifiedFiles`; test covers the undefined-base repo. +- **R3 — `verifyWorktreeInvariants` union shape.** The `reason` enum is load-bearing at :10889. Mitigation: KTD2 preserves the union; test asserts the `reason`. +- **R4 — No-op contamination function.** Mitigation: KTD1 explicitly forbids iterating `assertCleanBranchAtBase`; contamination rides on per-repo `captureModifiedFiles`. +- **R5 — N× reviewer cost.** Accepted (user decision). Mitigation: note in the PR; cost scales with repo count (typically 2-3). +- **Stacking dependency:** off Phase A (#1713); diff includes the stack. + +--- + +## Sources & Research + +- Master plan (U3/U4, KTD7, contamination-window caveat). +- Phase B feasibility pre-check (verified anchors: capture not gated/:7898 add-site, `assertCleanBranchAtBase` no-op, undefined-base via `resolveDiffBaseRef`, verify union :10508/:10889, review agent N× cost + the :5668 second surface, scope-leak :10711, anchor corrections). +- Phase A (#1713): per-repo `baseCommitSha`, `activeWorktrees` Set, `_workspace-fixture.ts`. +- `docs/solutions/logic-errors/files-changed-inflated-by-origin-first-base-commit.md`. From 453ed92dbffbeb65ddc9c8ab888880108920f046 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 23:02:46 -0700 Subject: [PATCH 4/6] =?UTF-8?q?fix(review):=20Phase=20B=20workspace=20hard?= =?UTF-8?q?ening=20=E2=80=94=20fail-closed=20scope=20guard,=20review=20con?= =?UTF-8?q?junction,=20.changeset=20carve-out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ce-code-review (4 personas) on Phase B. No P0; the review conjunction was confirmed safe (no false-done — empty map and per-repo throws both route to UNAVAILABLE, which blocks). Applied: P1: the fn_task_done scope-leak guard now fails CLOSED in workspace mode — a per-repo capture throw blocks completion ("refusing as a precaution") instead of the outer .catch returning {blocked:false} and letting an incomplete check pass. A scoped task that acquired ZERO sub-repo worktrees is now blocked rather than silently passing scope enforcement. P2: reviewWorkspacePerRepo breaks on the first non-APPROVE repo so a later repo's throw can't discard an already-determined REVISE (callers were seeing UNAVAILABLE instead). captureWorkspaceModifiedFiles isolates each per-repo capture in try/catch so one repo's throw can't skip the modifiedFiles write. The .changeset always-allowed carve-out is honored in workspace mode: the scope-leak branch now filters repo-LOCAL paths via the (previously dead) workspace-paths.ts deriveRepoScopeSubset helper through the same filter as the singular path, so a sub-repo .changeset/* no longer falsely blocks fn_task_done. All four per-repo loops iterate sorted keys for deterministic offending-repo reporting; the dead repoRel callback param and the duplicate path-normalizer are removed. Verified safe (no change): the reviewer semaphore releases on throw (try/finally), and per-repo reviewers inherit the task abort via session disposal. Deferred to Phase C: extracting a workspace-executor.ts module (before the merge loop lands). Gate green: typecheck, lint, build, test:gate (649+58). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../executor-workspace-taskdone.test.ts | 71 ++++++++ .../src/__tests__/reviewer-workspace.test.ts | 40 ++++- packages/engine/src/executor.ts | 151 +++++++++++++----- packages/engine/src/workspace-paths.ts | 12 +- 4 files changed, 227 insertions(+), 47 deletions(-) diff --git a/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts index 9f24aaa75c..8d62639a6d 100644 --- a/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts +++ b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts @@ -164,6 +164,77 @@ describeIfGit("U2 KTD4 — per-repo scope-leak guard in fn_task_done", () => { const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); expect(result.blocked).toBe(false); }); + + // FNXC:Workspace 2026-06-21-15:00: F5 — per-repo `.changeset/` carve-out honored in workspace mode. + // A legit sub-repo changeset (`repo-a/.changeset/x.md`) must NOT be flagged off-scope: the always-allowed + // filter now runs against the repo-LOCAL remainder (`.changeset/x.md`), so the carve-out matches. Before + // the fix the file was prefixed BEFORE filtering, the `.changeset/` startsWith never matched, and + // fn_task_done was wrongly REFUSED. + it("F5: a sub-repo `.changeset/` file is NOT flagged off-scope (always-allowed honored)", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + // A per-repo changeset OUTSIDE the declared `repo-a/src/**` scope — only the always-allowed + // carve-out can keep this from being a leak. + mkdirSync(path.join(a.worktreePath, ".changeset"), { recursive: true }); + writeFileSync(path.join(a.worktreePath, ".changeset", "tidy-foo.md"), "---\n'@x': patch\n---\n", "utf-8"); + execSync("git add .changeset/tidy-foo.md", { cwd: a.worktreePath, stdio: "pipe" }); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(false); + }); + + // FNXC:Workspace 2026-06-21-15:00: F2 — scoped task that acquired ZERO sub-repo worktrees is blocked. + // declaredScope is non-empty but `workspaceWorktrees` is empty → scope cannot be verified at all. The + // guard must refuse fn_task_done rather than silently aggregating zero off-scope files and passing. + it("F2: scoped task with zero acquired worktrees → blocked (cannot verify scope)", async () => { + fx = await createWorkspaceFixture(); + const store = createStore(["repo-a/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ branch: BRANCH, workspaceWorktrees: {} }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("acquired no sub-repo worktrees"); + }); + + // FNXC:Workspace 2026-06-21-15:00: F1 — fail CLOSED on a mid-loop capture throw. + // If one repo's capture throws (scope is UNVERIFIED for that repo), the guard must BLOCK naming the + // repo — not let the outer `.catch()` fail open and proceed with an incomplete scope check. + it("F1: a mid-loop capture throw → blocked (fail-closed), names the repo", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + // Narrow seam: force the per-repo uncommitted capture to throw for repo-a's worktree only. + const realCapture = (executor as any).captureUncommittedModifiedFiles.bind(executor); + vi.spyOn(executor as any, "captureUncommittedModifiedFiles").mockImplementation(async (wt: unknown) => { + if (wt === a.worktreePath) throw new Error("simulated capture failure"); + return realCapture(wt as string); + }); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("repo-a"); + expect(result.message).toContain("refusing fn_task_done"); + }); }); describeIfGit("U2 KTD4 — per-repo worktree-invariant verify in fn_task_done", () => { diff --git a/packages/engine/src/__tests__/reviewer-workspace.test.ts b/packages/engine/src/__tests__/reviewer-workspace.test.ts index cab774f3aa..4f5306d190 100644 --- a/packages/engine/src/__tests__/reviewer-workspace.test.ts +++ b/packages/engine/src/__tests__/reviewer-workspace.test.ts @@ -96,13 +96,17 @@ afterEach(() => { }); describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared loop both call sites use)", () => { + // FNXC:Workspace 2026-06-21-15:00: F7 — the per-repo callback is single-arg `(cwd)` now; tests map + // cwd→repo themselves (the loop no longer passes repoRel through to runForCwd). + const repoOfCwd = (cwd: string): string => (cwd === WT_A ? "repo-a" : cwd === WT_B ? "repo-b" : cwd); + it("conjunction: two repos both APPROVE → aggregate APPROVE, one reviewer pass per repo cwd", async () => { const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); const executor = workspaceExecutor(makeStore(task)); const seen: string[] = []; - const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string, repo: string) => { + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { seen.push(cwd); - return { verdict: "APPROVE", review: `clean in ${repo}`, summary: `clean ${repo}` }; + return { verdict: "APPROVE", review: `clean in ${repoOfCwd(cwd)}`, summary: `clean ${repoOfCwd(cwd)}` }; }); expect(seen).toEqual([WT_A, WT_B]); // one pass per sub-repo cwd, never ROOT expect(result.verdict).toBe("APPROVE"); @@ -113,7 +117,8 @@ describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared l it("conjunction: one repo REVISE → aggregate REVISE, tagged with the failing repo", async () => { const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); const executor = workspaceExecutor(makeStore(task)); - const result = await (executor as any).reviewWorkspacePerRepo(task, async (_cwd: string, repo: string) => { + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + const repo = repoOfCwd(cwd); return repo === "repo-b" ? { verdict: "REVISE", review: `bug in ${repo}`, summary: `revise ${repo}` } : { verdict: "APPROVE", review: `clean ${repo}`, summary: `clean ${repo}` }; @@ -124,6 +129,35 @@ describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared l expect(result.summary).toMatch(/^repo-b:/); }); + // FNXC:Workspace 2026-06-21-15:00: F3 — break on the FIRST non-APPROVE repo. + it("F3: repo-a APPROVE + repo-b REVISE (no throw) → aggregate REVISE tagged repo-b", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + const repo = repoOfCwd(cwd); + return repo === "repo-a" + ? { verdict: "APPROVE", review: "clean repo-a", summary: "clean a" } + : { verdict: "REVISE", review: "bug repo-b", summary: "revise b" }; + }); + expect(result.verdict).toBe("REVISE"); + expect(result.summary).toMatch(/^repo-b:/); + }); + + it("F3: repo-a REVISE + repo-b throws → REVISE preserved (break before repo-b; NOT masked to UNAVAILABLE)", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const seen: string[] = []; + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + seen.push(cwd); + if (cwd === WT_B) throw new Error("repo-b reviewer blew up"); + return { verdict: "REVISE", review: "bug repo-a", summary: "revise a" }; + }); + // repo-a recorded the first non-APPROVE and the loop BROKE, so repo-b's reviewer is never invoked. + expect(seen).toEqual([WT_A]); + expect(result.verdict).toBe("REVISE"); + expect(result.summary).toMatch(/^repo-a:/); + }); + it("zero-acquire workspace task → UNAVAILABLE (caller routes; no fabricated APPROVE)", async () => { const task = makeTask({ workspaceWorktrees: {} }); const executor = workspaceExecutor(makeStore(task)); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 74d0f45a2b..be43f645c9 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -82,6 +82,12 @@ import { resolveSandboxBackend } from "./sandbox/index.js"; import type { SandboxBackend } from "./sandbox/types.js"; import { ModelRegistry, SessionManager, type ToolDefinition, type AgentSession } from "@earendil-works/pi-coding-agent"; import { PRIORITY_EXECUTE, type AgentSemaphore } from "./concurrency.js"; +// FNXC:Workspace 2026-06-21-15:00: F5/F8 — wire in the previously dead workspace-path helpers. +// `normalizeRepoRelPath` is the single shared scope-path normalizer (F8); `deriveRepoScopeSubset` +// maps the task's repo-prefixed declared File Scope to a repo-LOCAL subset so the per-repo scope-leak +// filter reuses the SAME always-allowed/scope-match surface as the non-workspace path (F5). One-way +// executor→workspace-paths edge (workspace-paths imports nothing). +import { deriveRepoScopeSubset, normalizeRepoRelPath } from "./workspace-paths.js"; import { RemovalReason, classifyTaskWorktree, describeRegisteredWorktrees, detectNestedWorktreeRoot, getRegisteredWorktreePaths, isGitRepository, isInsideWorktreesDir, isRegisteredGitWorktree, removeWorktree, type WorktreePool } from "./worktree-pool.js"; import { attemptBranchAutocorrect } from "./branch-autocorrect.js"; import { ActiveSessionWorktreeRemovalError } from "./worktree-backend.js"; @@ -592,13 +598,14 @@ export interface WorkflowRevisionFeedbackPartition { const WORKFLOW_SCRIPT_OUTPUT_MAX_CHARS = 4_000; const WORKFLOW_FEEDBACK_PATH_REGEX = /`([^`\n]+)`|(?` branch (repo.branch). The result union is PRESERVED EXACTLY — `{ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected}` — because the :10889 consumer switches on `reason` to drive requeue/handoff (:10894-10936). We ADD an optional `repo` field to the failure shape (purely additive; the consumer only reads reason/observed/expected) and return the FIRST failing repo. A zero-acquire workspace task (empty map) verifies vacuously → {ok:true}, matching Phase A so fn_task_done does not requeue it. if (this.workspaceConfig) { const workspaceWorktrees = task.workspaceWorktrees ?? {}; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + // FNXC:Workspace 2026-06-21-15:00: F6 — iterate sorted repo keys so the FIRST failing repo + // returned here is deterministic across runs/rehydrate (the value is surfaced to the operator). + for (const repoRel of Object.keys(workspaceWorktrees).sort()) { + const repo = workspaceWorktrees[repoRel]; const expectedBranch = repo.branch || canonicalFusionBranchName(task.id); // Skip git checks if the worktree dir is gone (mirrors the singular FN-009 carve-out below): completion does not require a live worktree on disk. if (!existsSync(repo.worktreePath)) { @@ -10873,29 +10883,74 @@ export class TaskExecutor { // against `worktreePath`. In workspace mode `worktreePath` is the browse-only non-git workspace // root, so both silently return [] (git failures swallowed) and the uncommitted-in-scope block // never fires — a workspace task could complete with off-scope changes in any sub-repo. So we - // ITERATE every acquired sub-repo (cwd = repo.worktreePath, base = repo.baseCommitSha), - // repo-prefix each repo's touched files (`/`) so they compare against the task's - // repo-prefixed declared File Scope, and block on the FIRST repo carrying off-scope changes — - // naming the repo. The task-level preamble above (scopeOverride / declaredScope / enforcementMode) - // is shared and runs once. Return shape is preserved: `{blocked:false} | {blocked:true; message}`. + // ITERATE every acquired sub-repo (cwd = repo.worktreePath, base = repo.baseCommitSha) and block + // on the FIRST repo carrying off-scope changes — naming the repo. The task-level preamble above + // (scopeOverride / declaredScope / enforcementMode) is shared and runs once. Return shape is + // preserved: `{blocked:false} | {blocked:true; message}`. + // + // FNXC:Workspace 2026-06-21-15:00: F1/F2/F5/F6 hardening of the per-repo scope-leak guard. + // F5 (false-block fix + dead-code wiring + single filter surface): we previously repo-prefixed each + // touched file (`${repoRel}/${file}`) BEFORE filtering, so `isAlwaysAllowedScopeLeakPath`'s + // `startsWith(".changeset/")` carve-out never matched a sub-repo changeset (`repo-a/.changeset/x.md`) + // and a legit per-repo changeset was wrongly flagged off-scope → fn_task_done wrongly REFUSED. Now we + // derive each repo's repo-LOCAL declared-scope subset (`deriveRepoScopeSubset`) and run the SAME + // `workflowPathMatchesDeclaredScope` + `isAlwaysAllowedScopeLeakPath` filter the non-workspace path + // uses against the repo-LOCAL touched file — one filter surface, not two. This wires in the formerly + // dead `deriveRepoScopeSubset`/`splitRepoScopedPath` helpers. + // F1 (fail CLOSED on throw): each repo iteration is wrapped in its own try/catch (like the + // attribution-audit loop). A thrown capture/diff error in workspace mode surfaces as a BLOCK naming + // the repo instead of bubbling to the outer `.catch()` that fails OPEN — an incomplete scope check + // must never let fn_task_done proceed. + // F2 (scoped-but-zero-acquire): a scoped task that acquired NO sub-repo worktrees aggregates zero + // off-scope files and would silently pass; we block it (scope is declared but unverifiable). + // F6 (deterministic ordering): iterate sorted repo keys so the reported offending repo is stable + // across runs/rehydrate. let touchedFiles: string[]; let offendingRepo: string | undefined; if (this.workspaceConfig) { const workspaceWorktrees = task.workspaceWorktrees ?? {}; + const repoKeys = Object.keys(workspaceWorktrees).sort(); + // F2: declaredScope is non-empty here (the `declaredScope.length === 0` early-return above + // handled the unscoped case). A scoped task that acquired no sub-repo worktrees cannot have its + // scope verified at all — refuse rather than silently passing scope enforcement. + if (repoKeys.length === 0) { + const message = "workspace task declares File Scope but acquired no sub-repo worktrees — cannot verify scope"; + executorLog.warn(`${task.id}: [scope-leak] ${message}`); + await this.store.logEntry(task.id, `[scope-leak] ${message}`, undefined, this.getRunContextFor(task.id)); + return { blocked: true, message }; + } const aggregatedOffScope: string[] = []; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { - const [repoUncommitted, repoCommitted] = await Promise.all([ - this.captureUncommittedModifiedFiles(repo.worktreePath), - this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, "scope-leak-guard"), - ]); - const repoTouched = [...new Set([...repoUncommitted, ...repoCommitted])].map((f) => `${repoRel}/${f}`); - const repoOffScope = repoTouched - .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope)) - .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)); - if (repoOffScope.length > 0) { - // First offending repo wins (mirrors verifyWorktreeInvariants' first-failing-repo return). - if (!offendingRepo) offendingRepo = repoRel; - aggregatedOffScope.push(...repoOffScope); + for (const repoRel of repoKeys) { + const repo = workspaceWorktrees[repoRel]; + try { + const [repoUncommitted, repoCommitted] = await Promise.all([ + this.captureUncommittedModifiedFiles(repo.worktreePath), + this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, "scope-leak-guard"), + ]); + // Repo-LOCAL touched files (no `${repoRel}/` prefix) so the always-allowed `.changeset/` + // carve-out and the scope match operate as the reviewer/cwd=repo sees them (F5). + const repoTouched = [...new Set([...repoUncommitted, ...repoCommitted])]; + // Repo-LOCAL declared-scope subset for THIS repo (prefix stripped). Same filter as the + // non-workspace branch below — one surface. + const repoScopeSubset = deriveRepoScopeSubset(declaredScope, repoRel); + const repoOffScope = repoTouched + .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, repoScopeSubset)) + .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)) + // Re-prefix the surviving off-scope files for the operator-facing message/attribution. + .map((filePath) => `${repoRel}/${filePath}`); + if (repoOffScope.length > 0) { + // First offending repo wins (mirrors verifyWorktreeInvariants' first-failing-repo return). + if (!offendingRepo) offendingRepo = repoRel; + aggregatedOffScope.push(...repoOffScope); + } + } catch (repoErr: unknown) { + // F1: fail CLOSED. A capture/diff throw means scope is UNVERIFIED for this repo; refuse + // fn_task_done as a precaution rather than letting the outer `.catch()` fail open. + const errMessage = repoErr instanceof Error ? repoErr.message : String(repoErr); + const message = `workspace scope-leak guard failed to evaluate (${repoRel}/${errMessage}) — refusing fn_task_done as a precaution`; + executorLog.warn(`${task.id}: [scope-leak] ${message}`); + await this.store.logEntry(task.id, `[scope-leak] ${message}`, undefined, this.getRunContextFor(task.id)); + return { blocked: true, message }; } } touchedFiles = aggregatedOffScope; @@ -12455,11 +12510,21 @@ ${failureFeedback} source = "post-session", ): Promise { const workspaceWorktrees = task.workspaceWorktrees ?? {}; + // FNXC:Workspace 2026-06-21-15:00: F4/F6 — per-repo error isolation + deterministic ordering. + // F4: an unexpected throw from one repo's `captureModifiedFiles` must NOT escape and skip the + // downstream `updateTask({modifiedFiles})` write — that would leave `task.modifiedFiles` empty and + // blind the merge file audit. Wrap each per-repo call (log + continue), mirroring the post-session + // branch-attribution loop. F6: iterate sorted repo keys so aggregation order is stable across runs. const aggregated: string[] = []; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { - const repoFiles = await this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source); - for (const file of repoFiles) { - aggregated.push(`${repoRel}/${file}`); + for (const repoRel of Object.keys(workspaceWorktrees).sort()) { + const repo = workspaceWorktrees[repoRel]; + try { + const repoFiles = await this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source); + for (const file of repoFiles) { + aggregated.push(`${repoRel}/${file}`); + } + } catch (repoErr: unknown) { + executorLog.warn(`${task.id}: per-repo modified-file capture failed for ${repoRel}: ${repoErr instanceof Error ? repoErr.message : String(repoErr)}`); } } return aggregated; @@ -12483,12 +12548,18 @@ ${failureFeedback} * UNAVAILABLE retry) is unchanged. */ private async reviewWorkspacePerRepo( + // FNXC:Workspace 2026-06-21-15:00: F7 — drop the dead `repoRel` callback param. + // Both call sites bind `(cwd) => runForCwd(cwd)` and discard the second arg, so the type wrongly + // implied repo identity is observable inside `runForCwd`. Removed until a real consumer needs it + // (Phase C). The loop below still tags findings with `repoRel` from its own iteration key. task: Task, - invokeForCwd: (cwd: string, repoRel: string) => Promise, + invokeForCwd: (cwd: string) => Promise, ): Promise { const workspaceWorktrees = task.workspaceWorktrees ?? {}; - const entries = Object.entries(workspaceWorktrees); - if (entries.length === 0) { + // FNXC:Workspace 2026-06-21-15:00: F6 — sort repo keys so the reported FIRST failing repo is + // deterministic across runs/rehydrate. + const repoKeys = Object.keys(workspaceWorktrees).sort(); + if (repoKeys.length === 0) { // No acquired worktree — surface UNAVAILABLE so the caller routes it rather than // fabricating an authoritative APPROVE for an un-reviewable workspace task. return { @@ -12501,13 +12572,19 @@ ${failureFeedback} const reviewSections: string[] = []; const summarySections: string[] = []; let firstFailing: { repo: string; result: ReviewResult } | undefined; - for (const [repoRel, repo] of entries) { - const result = await invokeForCwd(repo.worktreePath, repoRel); + for (const repoRel of repoKeys) { + const repo = workspaceWorktrees[repoRel]; + const result = await invokeForCwd(repo.worktreePath); // Tag every per-repo finding with its sub-repo so downstream readers attribute it correctly. reviewSections.push(`### [${repoRel}] ${result.verdict}\n${result.review}`); summarySections.push(`[${repoRel}] ${result.verdict}: ${result.summary}`); - if (result.verdict !== "APPROVE" && !firstFailing) { + if (result.verdict !== "APPROVE") { + // FNXC:Workspace 2026-06-21-15:00: F3 — BREAK on the first non-APPROVE repo. + // The contract is "the FIRST non-APPROVE repo's verdict becomes the aggregate". Without the + // break, a LATER repo's reviewer throwing would discard this already-determined REVISE/RETHINK + // and the caller would see UNAVAILABLE — masking the real verdict. Stop at the first failure. firstFailing = { repo: repoRel, result }; + break; } } @@ -12524,8 +12601,8 @@ ${failureFeedback} // Every sub-repo approved → the task is reviewed (conjunction satisfied). return { verdict: "APPROVE", - review: `All ${entries.length} sub-repo(s) approved. Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, - summary: `APPROVE across ${entries.length} sub-repo(s): ${summarySections.join(" | ")}`, + review: `All ${repoKeys.length} sub-repo(s) approved. Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, + summary: `APPROVE across ${repoKeys.length} sub-repo(s): ${summarySections.join(" | ")}`, }; } diff --git a/packages/engine/src/workspace-paths.ts b/packages/engine/src/workspace-paths.ts index 308c559341..299dbfe357 100644 --- a/packages/engine/src/workspace-paths.ts +++ b/packages/engine/src/workspace-paths.ts @@ -10,13 +10,11 @@ Matching rule: canonicalize the path to forward-slash relative segments, then pi /** Sentinel returned when a path does not belong to any configured sub-repo. */ export const UNSCOPED_REPO = "unscoped" as const; -/** - * Normalize a workspace-relative path token to forward-slash form with no leading - * `./`, no leading/trailing slashes, and collapsed duplicate slashes. Mirrors the - * executor's `normalizeWorkflowScopePath` shape so File-Scope tokens and modified - * files compare consistently, but kept local to avoid an executor import cycle. - */ -function normalizeRepoRelPath(value: string): string { +/* +FNXC:Workspace 2026-06-21-15:00: +F8 — single normalize helper. The executor previously kept its own `normalizeWorkflowScopePath` that was a near-duplicate of this function, differing only in leading-slash stripping (`/^\/+/` here vs none there) and trailing-slash greediness (`/\/+$/` here vs `/\/$/` there). Two slightly-different normalizers meant an absolute or trailing-slash-laden path could derive a different scope key in the two code paths. We promote THIS (more aggressive: strips leading slash + collapses repeated trailing slashes) to the single exported normalizer and have the executor import it for scope-path normalization, so workspace and non-workspace scope matching canonicalize identically. workspace-paths.ts stays dependency-light (imports nothing), so executor→workspace-paths is a one-way, acyclic edge. +*/ +export function normalizeRepoRelPath(value: string): string { return value .trim() .replace(/\\/g, "/") From 14114f5f5158d7c2c64dba6b32dd0d9cd6d05caf Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 23:07:35 -0700 Subject: [PATCH 5/6] docs(workspace): note Phase B review hardening in the U2 changeset --- .changeset/workspace-phase-b-u2-per-repo-review-taskdone.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md b/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md index efd5004886..bdb9b95252 100644 --- a/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md +++ b/.changeset/workspace-phase-b-u2-per-repo-review-taskdone.md @@ -3,3 +3,5 @@ --- Workspace mode (Phase B, U2): per-repo review at both review entry points plus per-repo `fn_task_done` completion + scope-leak verification. In workspace mode both review call sites (the in-session `fn_review_step` tool and the step-inversion review seam) now loop the single-cwd `reviewStep` once per acquired sub-repo (cwd = each repo's worktree) and aggregate the repo-tagged verdicts as a conjunction — the task is reviewed only when every sub-repo approves, and the first failing sub-repo's verdict (with repo-tagged findings) drives the existing verdict→edge mapping. `fn_task_done` now verifies worktree invariants per acquired repo and iterates the scope-leak guard per sub-repo (cwd = repo worktree, repo `baseCommitSha`), blocking completion on any sub-repo carrying off-scope changes and naming the repo. Adds a minimal shared repo-prefix-derivation helper (`workspace-paths.ts`). Single-repo behavior is unchanged. + +Phase-B hardening: the per-repo scope-leak guard now fails CLOSED — a thrown capture/diff error in any sub-repo refuses `fn_task_done` (naming the repo) instead of failing open, and a scoped task that acquired zero sub-repo worktrees is blocked rather than silently passing. A legitimate per-repo `.changeset/` file is no longer falsely flagged off-scope (the always-allowed carve-out now runs against the repo-local path). Per-repo review stops at the first non-APPROVE sub-repo so a later repo's reviewer error can't mask an already-determined REVISE/RETHINK. Per-repo capture failures are isolated (one repo's error no longer drops the whole modified-files write), and the reported offending/failing repo is now deterministic (sorted repo iteration). Single-repo behavior remains unchanged. From f4a9c655099d02557cc2eec692c5603f5a077add Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 00:21:49 -0700 Subject: [PATCH 6/6] fix(review): address PR #1714 review findings - base-commit-capture: POSIX single-quote integration branch refs instead of JSON.stringify (double quotes are subject to $-expansion in the shell) - executor: add per-repo no_commits guard to the workspace verifyWorktreeInvariants branch (parity with the singular path), gated by the same task-wide no-commit eligibility - executor: reviewWorkspacePerRepo failure message now states the per-repo verdict list is partial (evaluation stops at first failure) - worktree-acquisition: defensively wrap non-fatal/outer-catch logEntry/audit so a logging throw cannot promote a non-fatal error to fatal or mask the original error - docs/plans: add code-fence language tags and fix MD028 blank-line-in-blockquote Co-Authored-By: Claude Opus 4.8 (1M context) --- ...eat-workspace-mode-execution-model-plan.md | 2 +- ...003-refactor-merger-unification-u0-plan.md | 4 +- packages/engine/src/base-commit-capture.ts | 14 ++-- packages/engine/src/executor.ts | 67 ++++++++++++++++++- packages/engine/src/worktree-acquisition.ts | 56 +++++++++++----- 5 files changed, 117 insertions(+), 26 deletions(-) diff --git a/docs/plans/2026-06-21-002-feat-workspace-mode-execution-model-plan.md b/docs/plans/2026-06-21-002-feat-workspace-mode-execution-model-plan.md index 050ff5aa11..c5b50e1d03 100644 --- a/docs/plans/2026-06-21-002-feat-workspace-mode-execution-model-plan.md +++ b/docs/plans/2026-06-21-002-feat-workspace-mode-execution-model-plan.md @@ -155,7 +155,7 @@ The surface-enumeration spine (FN-5893). Every row is a single-worktree / `cwd:r Additive only — no migration to existing single-repo tasks: -``` +```ts Task.workspaceWorktrees: Record **Units `U1–U4` below are local to this plan** (they decompose master-plan U0); they are **not** the master plan's `U1–U10`. U4 (audit) may run in parallel with U1–U3. - +> > **Standing requirements:** `FNXC:Workspace ` dated comments at each non-obvious decision point (dispatch collapse, the R7 guard, the deprecation warning). A `.changeset/*.md` (`@runfusion/fusion: minor`). Respect the merge gate (`pnpm lint`, typecheck, `pnpm build`, `pnpm test:gate`) and FN-5048 (narrow seams, fake timers, no real polling / mock-the-world). **Base branch (decided):** branch off the **foundation** (`pr-1710` / `feat/workspace-multi-repo` head) — the R7 guard (U3) reads `task.workspaceWorktrees`, which the foundation adds and `main` lacks. Do **not** commit onto `pr-1710` directly; use a new branch and open a **stacked PR targeting `feat/workspace-multi-repo`** so the diff is only U0's changes. ### U1. Collapse the engine dispatch and route the two direct callers to `runAiMerge` diff --git a/packages/engine/src/base-commit-capture.ts b/packages/engine/src/base-commit-capture.ts index 4d9e778774..6deb9d5c93 100644 --- a/packages/engine/src/base-commit-capture.ts +++ b/packages/engine/src/base-commit-capture.ts @@ -39,10 +39,16 @@ export async function resolveCapturedBaseCommitSha( integrationBranch: string = "main", ): Promise { const branch = integrationBranch.trim() || "main"; - // Shell-quote defensively; integration branch names are normalized upstream - // but may carry slashes (e.g. "release/2026-06") that are valid in refs. - const localRef = JSON.stringify(branch); - const originRef = JSON.stringify(`origin/${branch}`); + // FNXC:Workspace 2026-06-22-00:00: + // Shell-quote with POSIX single quotes, NOT JSON.stringify. JSON.stringify wraps + // in double quotes, under which the shell expands `$VAR`/backticks — a branch like + // `release/$2.0` would expand `$2` to a positional. Admin-configured integration + // branch names are not guaranteed to exclude `$`, and `$` is valid in git refs, so + // double-quoting is an injection/correctness risk. Single-quoting (with the embedded + // `'` → `'\''` escape) is literal and safe for slashes (e.g. "release/2026-06") too. + const shellQuote = (s: string): string => `'${s.replace(/'/g, "'\\''")}'`; + const localRef = shellQuote(branch); + const originRef = shellQuote(`origin/${branch}`); let baseCommitSha: string | undefined; try { const { stdout } = await execAsync( diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index be43f645c9..2bc7feefeb 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -10570,6 +10570,26 @@ export class TaskExecutor { // Phase A returned a flat {ok:true} stub here (no root worktree to verify against the non-git root). Phase B iterates every `task.workspaceWorktrees` entry, asserting (a) the sub-repo worktree's git toplevel matches the recorded repo.worktreePath and (b) its HEAD is on the recorded `fusion/` branch (repo.branch). The result union is PRESERVED EXACTLY — `{ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected}` — because the :10889 consumer switches on `reason` to drive requeue/handoff (:10894-10936). We ADD an optional `repo` field to the failure shape (purely additive; the consumer only reads reason/observed/expected) and return the FIRST failing repo. A zero-acquire workspace task (empty map) verifies vacuously → {ok:true}, matching Phase A so fn_task_done does not requeue it. if (this.workspaceConfig) { const workspaceWorktrees = task.workspaceWorktrees ?? {}; + // FNXC:Workspace 2026-06-22-00:00: KTD2 — resolve the SAME task-wide no-commit eligibility the singular path + // uses (getNoCommitEligibilityReason / no-op-completion sentinel / prompt-derived), once, before the per-repo + // loop. When eligible (Plan-Only, verified no-op, etc.) the per-repo no_commits guard below is skipped so an + // intentionally commit-free workspace task is not blocked from completion. + const workspacePromptContent = (task as Task & { prompt?: unknown }).prompt; + const workspacePromptEligibility = evaluatePromptDerivedNoCommitEligibility( + task, + typeof workspacePromptContent === "string" ? workspacePromptContent : "", + ); + const workspaceNoCommitEligibilityReason = + getNoCommitEligibilityReason(task) ?? + (options?.noOpCompletion + ? options.noOpCompletionReason ?? "verified no-op/duplicate completion sentinel" + : null) ?? + (workspacePromptEligibility.eligible + ? workspacePromptEligibility.reason ?? "prompt-derived no-commit eligibility" + : null); + if (workspaceNoCommitEligibilityReason) { + executorLog.log(`${task.id}: workspace fn_task_done no_commits guard skipped (${workspaceNoCommitEligibilityReason})`); + } // FNXC:Workspace 2026-06-21-15:00: F6 — iterate sorted repo keys so the FIRST failing repo // returned here is deterministic across runs/rehydrate (the value is surfaced to the operator). for (const repoRel of Object.keys(workspaceWorktrees).sort()) { @@ -10647,6 +10667,48 @@ export class TaskExecutor { expected: expectedBranch, }; } + // FNXC:Workspace 2026-06-22-00:00: KTD2 — per-repo no_commits guard (parity with the singular path at :10821). + // Phase B originally returned {ok:true} after the toplevel/branch checks, so a workspace task could call + // fn_task_done having committed NOTHING in any sub-repo (scope-leak sees zero touched files, branch names match) + // and still advance to in-review. Enforce the same `git rev-list --count ..HEAD > 0` invariant per repo, + // gated by the SAME task-wide no-commit eligibility below so Plan-Only / no-op-sentinel tasks stay exempt. + // The first sub-repo with zero commits fails with reason:'no_commits' (consumer-stable union). + if (!workspaceNoCommitEligibilityReason) { + const repoBaseRef = await this.resolveDiffBaseRef(repo.worktreePath, repo.baseCommitSha); + if (repoBaseRef) { + try { + const { stdout } = await execAsync(`git rev-list --count ${repoBaseRef}..HEAD`, { + cwd: repo.worktreePath, + encoding: "utf-8", + timeout: 10_000, + maxBuffer: 1024 * 1024, + }); + const trimmedCount = stdout.trim(); + if (trimmedCount) { + const count = Number.parseInt(trimmedCount, 10); + if (!Number.isFinite(count) || count <= 0) { + return { + ok: false, + reason: "no_commits", + repo: repoRel, + observed: Number.isFinite(count) ? String(count) : trimmedCount, + expected: "> 0", + }; + } + } + } catch (error) { + return { + ok: false, + reason: "no_commits", + repo: repoRel, + observed: error instanceof Error ? error.message : String(error), + expected: `git rev-list --count ${repoBaseRef}..HEAD > 0`, + }; + } + } else { + executorLog.warn(`${task.id}: unable to resolve diff base for ${repoRel} no_commits guard; skipping for this sub-repo`); + } + } } return { ok: true }; } @@ -12593,7 +12655,10 @@ ${failureFeedback} // verdict→edge mapping is identical to single-cwd), with the full repo-tagged review body. return { verdict: firstFailing.result.verdict, - review: `Workspace review failed in sub-repo \`${firstFailing.repo}\` (verdict ${firstFailing.result.verdict}). Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, + // FNXC:Workspace 2026-06-22-00:00: the conjunction BREAKS on the first non-APPROVE repo, + // so reviewSections holds only the repos evaluated up to (and including) the failure — not + // every sub-repo. Label it honestly so operators don't read a partial list as exhaustive. + review: `Workspace review failed in sub-repo \`${firstFailing.repo}\` (verdict ${firstFailing.result.verdict}). Per-repo verdicts (evaluation stopped at first failure; later repos not reviewed):\n\n${reviewSections.join("\n\n")}`, summary: `${firstFailing.repo}: ${firstFailing.result.verdict} — ${summarySections.join(" | ")}`, }; } diff --git a/packages/engine/src/worktree-acquisition.ts b/packages/engine/src/worktree-acquisition.ts index 352ef4a036..0e32b88cbe 100644 --- a/packages/engine/src/worktree-acquisition.ts +++ b/packages/engine/src/worktree-acquisition.ts @@ -746,14 +746,21 @@ export async function acquireWorkspaceRepoWorktree( }); } catch (guardErr) { // FNXC:Workspace 2026-06-21-22:30: F3 — identity-guard install is non-fatal; worktree is usable without it. + // FNXC:Workspace 2026-06-22-00:00: the non-fatal logEntry/audit are themselves best-effort — if either throws + // (e.g. a DB write hiccup) it must NOT promote this non-fatal guard failure into a fatal acquisition failure. + // Swallow logging errors so acquisition continues (matching the F6 busy-path defensive wrap above). const message = guardErr instanceof Error ? guardErr.message : String(guardErr); logger?.warn(`${task.id}: identity-guard install failed for sub-repo ${repoRelPath} (non-fatal): ${message}`); - await store.logEntry(task.id, `Workspace sub-repo identity-guard install failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); - await audit?.git({ - type: "worktree:workspace-repo-acquire-failed", - target: repoAbsPath, - metadata: { repoRelPath, taskId: task.id, error: message, stage: "identity-guard" }, - }); + try { + await store.logEntry(task.id, `Workspace sub-repo identity-guard install failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); + await audit?.git({ + type: "worktree:workspace-repo-acquire-failed", + target: repoAbsPath, + metadata: { repoRelPath, taskId: task.id, error: message, stage: "identity-guard" }, + }); + } catch { + // best-effort observability only — keep the (non-fatal) guard failure non-fatal + } } /* @@ -777,14 +784,20 @@ export async function acquireWorkspaceRepoWorktree( baseCommitSha = await resolveCapturedBaseCommitSha(result.worktreePath, logger, integrationBranch); } catch (baseErr) { // FNXC:Workspace 2026-06-21-22:30: F3 — base-SHA capture is non-fatal; an undefined baseCommitSha is an accepted state. + // FNXC:Workspace 2026-06-22-00:00: guard the best-effort logEntry/audit so a logging throw cannot promote this + // non-fatal capture failure into a fatal acquisition failure (parity with the F6 busy-path defensive wrap). const message = baseErr instanceof Error ? baseErr.message : String(baseErr); logger?.warn(`${task.id}: base-SHA capture failed for sub-repo ${repoRelPath} (non-fatal): ${message}`); - await store.logEntry(task.id, `Workspace sub-repo base-SHA capture failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); - await audit?.git({ - type: "worktree:workspace-repo-acquire-failed", - target: repoAbsPath, - metadata: { repoRelPath, taskId: task.id, error: message, stage: "base-sha-capture" }, - }); + try { + await store.logEntry(task.id, `Workspace sub-repo base-SHA capture failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); + await audit?.git({ + type: "worktree:workspace-repo-acquire-failed", + target: repoAbsPath, + metadata: { repoRelPath, taskId: task.id, error: message, stage: "base-sha-capture" }, + }); + } catch { + // best-effort observability only — keep the (non-fatal) capture failure non-fatal + } } /* @@ -814,14 +827,21 @@ export async function acquireWorkspaceRepoWorktree( sub-repo. */ if (!(err instanceof WorkspaceRepoAcquireBusyError)) { + // FNXC:Workspace 2026-06-22-00:00: wrap the failure logEntry/audit so a throw here cannot replace the ORIGINAL + // acquisition `err` the caller must observe — losing it would mask the real cause and the re-throw below would + // surface a logging error instead. Best-effort observability; `err` is always re-thrown. const message = err instanceof Error ? err.message : String(err); logger?.error?.(`${task.id}: workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`); - await store.logEntry(task.id, `Workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`, undefined, runContext); - await audit?.git({ - type: "worktree:workspace-repo-acquire-failed", - target: repoAbsPath, - metadata: { repoRelPath, taskId: task.id, error: message }, - }); + try { + await store.logEntry(task.id, `Workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`, undefined, runContext); + await audit?.git({ + type: "worktree:workspace-repo-acquire-failed", + target: repoAbsPath, + metadata: { repoRelPath, taskId: task.id, error: message }, + }); + } catch { + // best-effort observability only — ensure the original acquisition error propagates + } } throw err; } finally {