From 81edbeefbd6ced6b408d0ced4deb8e0915187f9e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 22:44:40 -0700 Subject: [PATCH] =?UTF-8?q?feat(workspace):=20Phase=20B=20U2=20=E2=80=94?= =?UTF-8?q?=20per-repo=20review=20(both=20sites)=20+=20fn=5Ftask=5Fdone=20?= =?UTF-8?q?verify=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; +}