diff --git a/.changeset/fn-201-workspace-review-findings.md b/.changeset/fn-201-workspace-review-findings.md new file mode 100644 index 0000000000..70c4395599 --- /dev/null +++ b/.changeset/fn-201-workspace-review-findings.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Route multi-repository Code Review fixes to the repository that failed. +category: fix +dev: Preserves repository-qualified review findings through workspace aggregation and named remediation routing. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 4e5f77723f..811f7fa514 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -1025,7 +1025,7 @@ When the merge boundary cannot be proven, Fusion emits the terminal graph value ### Workspace Code Review remediation -Workspace Code Review carries repository-specific outcomes. A failed review is remediated from the failing repository's acquired worktree without persisting a singular workspace-root worktree. Repeated unchanged negative review input parks for operator approval; landing requires current approval evidence for every modified scoped repository. Scope changes clear both approval evidence and the remediation target atomically, while a current-scope APPROVE clears its matching target before graph completion. +Workspace Code Review carries repository-specific outcomes. Structured findings are repository-qualified (including their identifiers and file paths), retained on the aggregate review result, and persisted on the workflow step result so named-remediation workflows can turn a REVISE into explicit fix work. A failed review is remediated from the failing repository's acquired worktree without persisting a singular workspace-root worktree. Repeated unchanged negative review input parks for operator approval; landing requires current approval evidence for every modified scoped repository. Scope changes clear both approval evidence and the remediation target atomically, while a current-scope APPROVE clears its matching target before graph completion. ### Workspace Code Review seal diff --git a/packages/engine/src/__tests__/workspace-review-findings.test.ts b/packages/engine/src/__tests__/workspace-review-findings.test.ts new file mode 100644 index 0000000000..7d2d2f9249 --- /dev/null +++ b/packages/engine/src/__tests__/workspace-review-findings.test.ts @@ -0,0 +1,198 @@ +import { describe, expect, it } from "vitest"; + +import type { Task } from "@fusion/core"; + +import { + buildWorkspaceReviewOutcome, + preserveOutcomeFindingsFromReviewOutput, + toWorkspaceRepoReviewResult, +} from "../executor/run-graph-custom-node.js"; +import { qualifyRepositoryFindings, reviewWorkspacePerRepo } from "../executor/workspace-review-per-repo.js"; +import { reviewInputSignature } from "../executor/request-pre-merge-optional-step-fix.js"; +import { deriveWorkspaceReviewRemediation } from "../executor/workspace-review-remediation.js"; + +function workspaceTask(): Task { + return { + id: "FN-201", + column: "in-review", + repositoryScope: { state: "confirmed", revision: 1, repositories: ["repo-a", "repo-b"] }, + workspaceWorktrees: { + "repo-a": { worktreePath: "/workspace/repo-a", baseCommitSha: "a" }, + "repo-b": { worktreePath: "/workspace/repo-b", baseCommitSha: "b" }, + }, + } as unknown as Task; +} + +async function reviewWorkspace( + results: Record }>, +) { + return reviewWorkspacePerRepo(workspaceTask(), async (cwd) => { + const repo = cwd.slice(cwd.lastIndexOf("/") + 1); + const result = results[repo]; + return { ...result, review: result.verdict, summary: result.verdict }; + }, { + workspaceRepos: ["repo-a", "repo-b"], + workspaceRootDir: "/workspace", + captureModifiedFiles: async (repo) => [`src/${repo}.ts`], + }); +} + +describe("workspace Code Review findings", () => { + it("forwards structured findings from a revised repository outcome", () => { + const findings = [ + { id: "finding-1", title: "First", body: "Fix the first issue", filePath: "src/one.ts" }, + { id: "finding-2", title: "Second", body: "Fix the second issue", filePath: "src/two.ts" }, + ]; + + expect(toWorkspaceRepoReviewResult({ success: false, verdict: "REVISE", output: "revise", findings })).toEqual({ + verdict: "REVISE", + review: "revise", + summary: "revise", + retryable: true, + findings, + }); + }); + + it("keeps successful finding-less outcomes compact", () => { + expect(toWorkspaceRepoReviewResult({ success: true, output: "approved" })).toEqual({ + verdict: "APPROVE", + review: "approved", + summary: "approved", + retryable: false, + }); + }); + + it("maps an errored outcome to unavailable review text", () => { + expect(toWorkspaceRepoReviewResult({ success: false, error: "reviewer unavailable" })).toEqual({ + verdict: "UNAVAILABLE", + review: "reviewer unavailable", + summary: "reviewer unavailable", + retryable: true, + }); + }); + + it("qualifies identifiers, paths, and dispute links without changing other finding fields", () => { + expect(qualifyRepositoryFindings("repo-a", [{ + id: "finding-1", + title: "Title", + body: "Body", + filePath: "src/x.ts", + rebutsDisputedFindingId: "finding-0", + severity: "high", + disputeRationale: "The implementation disagrees.", + }])).toEqual([{ + id: "repo-a:finding-1", + title: "Title", + body: "Body", + filePath: "repo-a/src/x.ts", + rebutsDisputedFindingId: "repo-a:finding-0", + severity: "high", + disputeRationale: "The implementation disagrees.", + }]); + }); + + it("does not double-qualify finding values that already name their repository", () => { + const findings = [{ id: "repo-a:finding-1", title: "Title", body: "Body", filePath: "repo-a/src/x.ts", rebutsDisputedFindingId: "repo-a:finding-0" }]; + expect(qualifyRepositoryFindings("repo-a", findings)).toEqual(findings); + }); + + it("aggregates distinct repository-qualified findings into reviewed outcomes", async () => { + const aggregate = await reviewWorkspace({ + "repo-a": { verdict: "APPROVE", findings: [{ id: "finding-1", title: "A", body: "Body A", filePath: "src/x.ts" }] }, + "repo-b": { verdict: "REVISE", findings: [{ id: "finding-1", title: "B", body: "Body B", filePath: "src/x.ts" }] }, + }); + + expect(aggregate.findings).toEqual([ + { id: "repo-a:finding-1", title: "A", body: "Body A", filePath: "repo-a/src/x.ts" }, + { id: "repo-b:finding-1", title: "B", body: "Body B", filePath: "repo-b/src/x.ts" }, + ]); + expect(aggregate.repositoryReviewOutcomes?.map((outcome) => outcome.findings)).toEqual([ + [{ id: "repo-a:finding-1", title: "A", body: "Body A", filePath: "repo-a/src/x.ts" }], + [{ id: "repo-b:finding-1", title: "B", body: "Body B", filePath: "repo-b/src/x.ts" }], + ]); + }); + + it("omits aggregate findings when reviewers return no structured findings", async () => { + const aggregate = await reviewWorkspace({ + "repo-a": { verdict: "APPROVE" }, + "repo-b": { verdict: "APPROVE" }, + }); + + expect(aggregate).not.toHaveProperty("findings"); + expect(aggregate.repositoryReviewOutcomes?.every((outcome) => !("findings" in outcome))).toBe(true); + }); + + it("carries qualified aggregate findings into the workspace node outcome", () => { + const findings = [{ id: "repo-a:finding-1", title: "Title", body: "Body", filePath: "repo-a/src/x.ts" }]; + expect(buildWorkspaceReviewOutcome({ + verdict: "REVISE", + review: "review", + summary: "review", + findings, + repositoryReviewOutcomes: [], + repositoryScopeRevision: 1, + })).toMatchObject({ + success: false, + verdict: "REVISE", + findings, + repositoryScopeRevision: 1, + }); + }); + + it("keeps structured workspace findings instead of reparsing concatenated review prose", () => { + const findings = [{ id: "repo-a:finding-1", title: "Title", body: "Body", filePath: "repo-a/src/x.ts" }]; + const output = `${JSON.stringify({ verdict: "REVISE", notes: "prose", findings: [{ id: "unqualified", title: "Wrong", body: "Wrong", filePath: "src/x.ts" }] })}`; + expect(preserveOutcomeFindingsFromReviewOutput({ success: false, verdict: "REVISE", output, findings }).findings).toEqual(findings); + }); + + it("still parses findings for a single-repository outcome that has none", () => { + const output = JSON.stringify({ verdict: "REVISE", notes: "prose", findings: [{ id: "finding-1", title: "Title", body: "Body", filePath: "src/x.ts" }] }); + expect(preserveOutcomeFindingsFromReviewOutput({ success: false, verdict: "REVISE", output }).findings).toEqual([ + { id: "finding-1", title: "Title", body: "Body", filePath: "src/x.ts" }, + ]); + }); + + it("keeps superseded workspace outcomes free of actionable findings", () => { + expect(buildWorkspaceReviewOutcome({ + verdict: "UNAVAILABLE", + review: "superseded", + summary: "superseded", + findings: [{ id: "repo-a:finding-1", title: "Title", body: "Body" }], + }, { superseded: true })).not.toHaveProperty("findings"); + }); + + it("wires workspace review through structured mapping and aggregate outcome helpers", async () => { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(new URL("../executor/run-graph-custom-node.ts", import.meta.url), "utf8"); + + expect(source).toContain("return toWorkspaceRepoReviewResult(repoOutcome);"); + expect(source).toContain("outcome = buildWorkspaceReviewOutcome(aggregate, { superseded: reviewSuperseded });"); + }); + + it("treats workspace review findings with volatile identifiers as the same convergence input", () => { + const result = (id: string, overrides: { body?: string; fingerprint?: string; verdict?: "REVISE" | "RETHINK"; revision?: number } = {}) => ({ + workflowStepId: "code-review", + workflowStepName: "Code Review", + verdict: overrides.verdict ?? "REVISE", + repositoryScopeRevision: overrides.revision ?? 1, + repositoryReviewOutcomes: [{ + repository: "repo-a", + status: "REVIEWED", + verdict: overrides.verdict ?? "REVISE", + fingerprint: overrides.fingerprint ?? "fingerprint-a", + episodeId: "episode-a", + reviewedAt: "2026-08-27T12:00:00.000Z", + findings: [{ id, title: "Title", body: overrides.body ?? "Body", filePath: "repo-a/src/x.ts", line: 5 }], + }], + }); + const original = result("repo-a:finding-1"); + + expect(deriveWorkspaceReviewRemediation(original as never)?.inputSignature) + .toBe(deriveWorkspaceReviewRemediation(result("repo-a:finding-2") as never)?.inputSignature); + expect(reviewInputSignature(original as never)).toBe(reviewInputSignature(result("repo-a:finding-2") as never)); + expect(reviewInputSignature(original as never)).not.toBe(reviewInputSignature(result("repo-a:finding-1", { body: "Changed" }) as never)); + expect(reviewInputSignature(original as never)).not.toBe(reviewInputSignature(result("repo-a:finding-1", { fingerprint: "fingerprint-b" }) as never)); + expect(reviewInputSignature(original as never)).not.toBe(reviewInputSignature(result("repo-a:finding-1", { verdict: "RETHINK" }) as never)); + expect(reviewInputSignature(original as never)).not.toBe(reviewInputSignature(result("repo-a:finding-1", { revision: 2 }) as never)); + }); +}); diff --git a/packages/engine/src/__tests__/workspace-review-remediation-routing.test.ts b/packages/engine/src/__tests__/workspace-review-remediation-routing.test.ts new file mode 100644 index 0000000000..bad25e9d53 --- /dev/null +++ b/packages/engine/src/__tests__/workspace-review-remediation-routing.test.ts @@ -0,0 +1,235 @@ +import { describe, expect, it, vi } from "vitest"; +import { getBuiltinWorkflow, type Task, type TaskStep } from "@fusion/core"; + +import { appendReviewRemediationSteps } from "../executor/append-review-remediation-steps.js"; +import { requestPreMergeOptionalStepFix } from "../executor/request-pre-merge-optional-step-fix.js"; + +const PROMPT = [ + "# Task: FN-201", + "", + "## File Scope", + "", + "- `repo-a/src/x.ts`", + "", + "## Steps", + "", + "### Step 1: Implement", +].join("\n"); + +function harness(options: { + findings?: Array<{ id: string; title: string; body: string; filePath: string; severity?: "critical" }>; + workflowStepResults?: Task["workflowStepResults"]; + workspaceWorktreePath?: string; + workspace?: boolean; +} = {}) { + const findings = options.findings ?? [{ + id: "repo-a:finding-1", + title: "Missing guard", + body: "Add the missing concurrency guard.", + filePath: "repo-a/src/x.ts", + severity: "critical" as const, + }]; + const task = { + id: "FN-201", + column: "in-review", + worktree: "/tmp/singular", + prompt: PROMPT, + modifiedFiles: ["repo-a/src/x.ts"], + steps: [{ name: "Implement", status: "done" }] as TaskStep[], + ...(options.workspace === false ? {} : { + repositoryScope: { state: "confirmed", revision: 1, repositories: ["repo-a", "repo-b"] }, + workspaceWorktrees: { + "repo-a": { worktreePath: options.workspaceWorktreePath ?? "/tmp/repo-a", baseCommitSha: "a" }, + "repo-b": { worktreePath: "/tmp/repo-b", baseCommitSha: "b" }, + }, + workflowStepResults: options.workflowStepResults ?? [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + repositoryScopeRevision: 1, + repositoryReviewOutcomes: [{ + repository: "repo-a", + status: "REVIEWED", + verdict: "REVISE", + findings, + fingerprint: "fingerprint-a", + episodeId: "episode-a", + reviewedAt: "2026-08-27T12:00:00.000Z", + }], + }], + }), + } as unknown as Task; + const sendTaskBackForFix = vi.fn(async () => undefined); + const store = { + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({ autoMerge: true })), + logEntry: vi.fn(async () => undefined), + updateTask: vi.fn(async (_id: string, patch: Partial) => { + Object.assign(task, patch); + return task; + }), + appendRemediationSteps: vi.fn(async (_id: string, steps: readonly TaskStep[], options_: { wave?: number }) => { + const appended = steps.map((step) => ({ ...step, status: "pending" as const })); + task.steps = [...(task.steps ?? []), ...appended]; + return { task, appended, appendedCount: appended.length, wave: options_.wave ?? 1 }; + }), + updateTaskAtomic: vi.fn(async (_id: string, mutate: (current: Task) => Partial | null | undefined | Promise | null | undefined>) => { + const patch = await mutate(task); + if (patch) Object.assign(task, patch); + return task; + }), + updateWorkspaceReviewState: vi.fn(async (_id: string, revision: number, remediation: NonNullable["reviewRemediation"]>) => { + if (task.repositoryScope?.revision !== revision) return { task, updated: false }; + task.repositoryScope = { ...task.repositoryScope, reviewRemediation: remediation }; + return { task, updated: true }; + }), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "builtin:coding-ideas-v2" })), + getWorkflowDefinition: vi.fn(async (id: string) => { + const workflow = getBuiltinWorkflow(id); + return workflow ? { ir: workflow.ir } : undefined; + }), + }; + const deps = { + store: store as never, + getRunContextFor: () => undefined, + recoverMissingRequiredArtifacts: vi.fn(async () => undefined), + parkPlanReviewReplanCapExhausted: vi.fn(async () => undefined), + clearPausedAborted: vi.fn(), + readTaskArtifact: async () => task.prompt, + appendReviewRemediationSteps: (live: Task, info: never) => appendReviewRemediationSteps( + { store: store as never, readTaskArtifact: async () => task.prompt, sendTaskBackForFix }, + live, + info, + ), + workflowLifecycleMovesInFlight: new Set(), + sendTaskBackForFix, + }; + return { task, store, deps, sendTaskBackForFix }; +} + +const reviseInfo = (findings: Array<{ id: string; title: string; body: string; filePath: string; severity?: "critical" }>) => ({ + stepName: "Code Review", + feedback: "Review requested changes.", + phase: "pre-merge" as const, + status: "failed" as const, + verdict: "REVISE", + nodeId: "code-review", + findings, +}); + +describe("workspace named Code Review remediation routing", () => { + it("appends qualified work and bounces into the failing repository without persisting a singular worktree", async () => { + const { task, store, deps, sendTaskBackForFix } = harness(); + const findings = task.workflowStepResults?.[0]?.repositoryReviewOutcomes?.[0]?.findings ?? []; + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(true); + expect(task.steps?.at(-1)?.remediation).toMatchObject({ + gate: "Code Review", + filePath: "repo-a/src/x.ts", + findingId: "repo-a:finding-1", + }); + expect(sendTaskBackForFix).toHaveBeenCalledWith( + expect.anything(), "/tmp/repo-a", expect.anything(), expect.anything(), expect.anything(), true, false, + undefined, findings, false, "none", + ); + expect(store.updateWorkspaceReviewState).toHaveBeenCalledWith("FN-201", 1, expect.objectContaining({ repository: "repo-a" })); + expect(store.updateTask).not.toHaveBeenCalledWith("FN-201", expect.objectContaining({ status: "awaiting-approval" })); + expect(store.logEntry).not.toHaveBeenCalledWith("FN-201", "Review remediation requires human action", "review-remediation-no-actionable-findings"); + expect(task.prompt).toContain("- `repo-a/src/x.ts`"); + }); + + it("leaves no remediation steps or prompt edits when a scope CAS reports supersession", async () => { + const { task, store, deps, sendTaskBackForFix } = harness(); + const findings = task.workflowStepResults?.[0]?.repositoryReviewOutcomes?.[0]?.findings ?? []; + const originalSteps = [...(task.steps ?? [])]; + const originalPrompt = task.prompt; + store.updateWorkspaceReviewState.mockImplementationOnce(async () => { + task.repositoryScope = { ...task.repositoryScope!, revision: 2 }; + return { task, updated: false }; + }); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(false); + expect(store.appendRemediationSteps).not.toHaveBeenCalled(); + expect(store.updateTask).not.toHaveBeenCalled(); + expect(task.steps).toEqual(originalSteps); + expect(task.prompt).toBe(originalPrompt); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith("FN-201", "Workspace review remediation superseded by repository scope change"); + }); + + it("does not append stale workspace remediation after a post-CAS scope change", async () => { + const { task, store, deps, sendTaskBackForFix } = harness(); + const findings = task.workflowStepResults?.[0]?.repositoryReviewOutcomes?.[0]?.findings ?? []; + const originalSteps = [...(task.steps ?? [])]; + const originalPrompt = task.prompt; + store.updateWorkspaceReviewState.mockImplementationOnce(async (_id: string, revision: number, remediation: NonNullable["reviewRemediation"]>) => { + expect(revision).toBe(1); + task.repositoryScope = { ...task.repositoryScope!, reviewRemediation: remediation }; + return { task, updated: true }; + }); + store.updateTaskAtomic.mockImplementationOnce(async (_id: string, mutate: (current: Task) => Partial | null | undefined | Promise | null | undefined>) => { + task.repositoryScope = { ...task.repositoryScope!, revision: 2 }; + const patch = await mutate(task); + if (patch) Object.assign(task, patch); + return task; + }); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(false); + expect(task.steps).toEqual(originalSteps); + expect(task.prompt).toBe(originalPrompt); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + expect(store.appendRemediationSteps).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith("FN-201", "Workspace review remediation superseded by repository scope change"); + }); + + it("honestly parks a finding-less revise instead of inventing work", async () => { + const { task, store, deps, sendTaskBackForFix } = harness({ findings: [] }); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo([])); + + expect(scheduled).toBe(false); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith("FN-201", "Review remediation requires human action", "review-remediation-no-actionable-findings"); + }); + + it("parks qualified findings outside the workspace file scope", async () => { + const findings = [{ id: "repo-b:finding-1", title: "Outside", body: "Fix outside scope.", filePath: "repo-b/src/outside.ts", severity: "critical" as const }]; + const { task, store, deps, sendTaskBackForFix } = harness({ findings }); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(false); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith("FN-201", "Review remediation requires human action", "review-remediation-upstream-out-of-scope"); + }); + + it("parks when the failed repository has no acquired workspace worktree", async () => { + const { task, store, deps, sendTaskBackForFix } = harness({ workspaceWorktreePath: "" }); + const findings = task.workflowStepResults?.[0]?.repositoryReviewOutcomes?.[0]?.findings ?? []; + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(false); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith("FN-201", "Review remediation requires human action", "review-remediation-workspace-worktree-missing"); + }); + + it("retains the singular worktree bounce contract outside workspace mode", async () => { + const findings = [{ id: "finding-1", title: "Missing", body: "Fix the guard.", filePath: "repo-a/src/x.ts", severity: "critical" as const }]; + const { task, deps, sendTaskBackForFix } = harness({ workspace: false, findings }); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, reviseInfo(findings)); + + expect(scheduled).toBe(true); + expect(sendTaskBackForFix.mock.calls[0]?.[1]).toBe("/tmp/singular"); + expect(sendTaskBackForFix.mock.calls[0]?.[9]).toBeUndefined(); + }); +}); diff --git a/packages/engine/src/executor/append-review-remediation-steps.ts b/packages/engine/src/executor/append-review-remediation-steps.ts index d3c165b9ac..00658fe991 100644 --- a/packages/engine/src/executor/append-review-remediation-steps.ts +++ b/packages/engine/src/executor/append-review-remediation-steps.ts @@ -1,6 +1,15 @@ -import { AWAITING_APPROVAL_PAUSE_REASON, remediationDeclaredFiles, remediationWaveCount, type Task, type TaskStore } from "@fusion/core"; +import { + AWAITING_APPROVAL_PAUSE_REASON, + hasOpenEquivalentRemediationStep, + remediationDeclaredFiles, + remediationWaveCount, + type Task, + type TaskStep, + type TaskStore, +} from "@fusion/core"; import { deriveRemediationSteps } from "./derive-remediation-steps.js"; import type { RequestPreMergeOptionalStepFixInfo } from "./request-pre-merge-optional-step-fix.js"; +import { deriveWorkspaceReviewRemediation } from "./workspace-review-remediation.js"; export type AppendReviewRemediationStepsDeps = { store: TaskStore; @@ -49,12 +58,96 @@ export async function appendReviewRemediationSteps( return park(deps.store, task.id, "review-remediation-upstream-out-of-scope"); } if (derived.steps.length === 0) return park(deps.store, task.id, "review-remediation-no-actionable-findings"); - const appended = await deps.store.appendRemediationSteps(task.id, derived.steps, { wave }); - const live = await deps.store.getTask(task.id); - if (appended.appendedCount === 0 || !live.steps.some((step) => step.status === "pending")) { + /* + FNXC:WorkspaceReviewRemediation 2026-08-27-12:26: + A named-remediation workflow reaches this appender only under the `none` reopen policy, which + bypasses requestPreMergeOptionalStepFix's workspace-routing branch. Claim the scope generation + before changing steps or PROMPT.md so a superseded Code Review cannot leave stale work behind. + */ + const reviewResult = (task.workflowStepResults ?? []).find((result) => + (result.workflowStepId === info.nodeId || result.workflowStepName === info.stepName) + && result.verdict === "REVISE", + ); + const remediation = task.workspaceWorktrees && reviewResult + ? deriveWorkspaceReviewRemediation(reviewResult) + : undefined; + if (remediation) { + const updateWorkspaceReviewState = (deps.store as TaskStore & { + updateWorkspaceReviewState?: TaskStore["updateWorkspaceReviewState"]; + }).updateWorkspaceReviewState; + if (updateWorkspaceReviewState) { + const persisted = await updateWorkspaceReviewState.call(deps.store, task.id, remediation.scopeRevision, remediation); + if (!persisted.updated) { + await deps.store.logEntry(task.id, "Workspace review remediation superseded by repository scope change"); + return false; + } + } + } + + let appended: TaskStep[]; + let live: Task; + if (remediation) { + let scopeSuperseded = false; + appended = []; + live = await deps.store.updateTaskAtomic(task.id, (current) => { + /* + FNXC:WorkspaceReviewRemediation 2026-08-27-12:32: + A successful review-remediation CAS only claims the target at that instant. Append its named + work and widen PROMPT.md in the same revision-fenced mutation, so an intervening scope edit + cannot leave an invalid review episode's steps or File Scope behind. + */ + if (current.repositoryScope?.revision !== remediation.scopeRevision) { + scopeSuperseded = true; + return null; + } + const existing = current.steps ?? []; + appended = derived.steps + .filter((candidate) => candidate.remediation !== undefined) + .filter((candidate) => !hasOpenEquivalentRemediationStep([...existing, ...appended], candidate)) + .map((candidate) => ({ + ...candidate, + status: "pending" as const, + remediation: { ...candidate.remediation!, wave: candidate.remediation?.wave ?? wave }, + ...(candidate.dependsOn ? { dependsOn: [...candidate.dependsOn] } : {}), + })); + if (appended.length === 0) return null; + const nextPrompt = widenPromptFileScopeContent(current.prompt ?? prompt, remediationDeclaredFiles(appended)); + return { + steps: [...existing, ...appended], + ...(nextPrompt !== current.prompt ? { prompt: nextPrompt } : {}), + }; + }); + if (scopeSuperseded) { + await deps.store.logEntry(task.id, "Workspace review remediation superseded by repository scope change"); + return false; + } + } else { + const appendResult = await deps.store.appendRemediationSteps(task.id, derived.steps, { wave }); + appended = appendResult.appended; + live = await deps.store.getTask(task.id); + await widenPromptFileScope(deps.store, task.id, prompt, remediationDeclaredFiles(appended)); + } + if (appended.length === 0 || !live.steps.some((step) => step.status === "pending")) { return park(deps.store, task.id, "review-remediation-no-pending-work"); } - await widenPromptFileScope(deps.store, task.id, prompt, remediationDeclaredFiles(appended.appended)); + if (remediation) { + const workspaceWorktreePath = live.workspaceWorktrees?.[remediation.repository]?.worktreePath; + if (!workspaceWorktreePath) return park(deps.store, task.id, "review-remediation-workspace-worktree-missing"); + await deps.sendTaskBackForFix( + live, + workspaceWorktreePath, + info.feedback, + info.stepName, + `Review gate ${gate} requested named remediation`, + true, + false, + undefined, + info.findings, + false, + "none", + ); + return true; + } await deps.sendTaskBackForFix( live, options.worktreePath?.trim() || live.worktree || "", @@ -88,10 +181,15 @@ async function park(store: TaskStore, taskId: string, reason: string): Promise { + const updated = widenPromptFileScopeContent(prompt, files); + if (updated !== prompt) await store.updateTask(taskId, { prompt: updated }); +} + +function widenPromptFileScopeContent(prompt: string | undefined, files: readonly string[]): string | undefined { const additions = [...new Set(files.map((file) => file.trim()).filter(Boolean))]; - if (additions.length === 0 || !prompt) return; + if (additions.length === 0 || !prompt) return prompt; const heading = /^##\s+File Scope\s*$/m.exec(prompt); - if (!heading || heading.index === undefined) return; + if (!heading || heading.index === undefined) return prompt; const sectionStart = heading.index + heading[0].length; const rest = prompt.slice(sectionStart); const nextHeading = rest.search(/^##\s/m); @@ -99,9 +197,9 @@ async function widenPromptFileScope(store: TaskStore, taskId: string, prompt: st const section = prompt.slice(sectionStart, sectionEnd); const existing = new Set((section.match(/`([^`]+)`/g) ?? []).map((entry) => entry.slice(1, -1))); const missing = additions.filter((file) => !existing.has(file)); - if (missing.length === 0) return; + if (missing.length === 0) return prompt; const trimmed = section.replace(/\s+$/, ""); const insertion = missing.map((file) => `- \`${file}\``).join("\n"); const replacement = trimmed.length === 0 ? `\n\n${insertion}\n` : `${trimmed}\n${insertion}\n`; - await store.updateTask(taskId, { prompt: prompt.slice(0, sectionStart) + replacement + prompt.slice(sectionEnd) }); + return prompt.slice(0, sectionStart) + replacement + prompt.slice(sectionEnd); } diff --git a/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts b/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts index 5dd64d2083..76eaaf4817 100644 --- a/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts +++ b/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts @@ -80,8 +80,13 @@ export function reviewInputSignature(result: CoreWorkflowStepResult): string | u const blocking = (result.repositoryReviewOutcomes ?? []) .filter((outcome) => outcome.status === "REVIEWED" && (outcome.verdict === "REVISE" || outcome.verdict === "RETHINK")) .map((outcome) => { + /* + FNXC:WorkspaceReviewConvergence 2026-08-27-12:05: + FN-201 makes workspace findings non-empty. Identifier-based signatures would let a model-assigned + ID change defeat the repeat-unchanged hold and turn bounded remediation into an unbounded loop. + */ const findings = (outcome.findings ?? []) - .map((finding) => `${finding.id}:${normalizeConvergenceText(finding.title)}:${normalizeConvergenceText(finding.body)}`) + .map((finding) => `${normalizeConvergenceText(finding.filePath)}:${finding.line ?? ""}:${normalizeConvergenceText(finding.body)}`) .sort() .join("|"); return `${outcome.repository}\u0000${outcome.fingerprint ?? ""}\u0000${outcome.verdict}\u0000${findings}`; diff --git a/packages/engine/src/executor/run-graph-custom-node.ts b/packages/engine/src/executor/run-graph-custom-node.ts index b687ee5aae..9e47dace98 100644 --- a/packages/engine/src/executor/run-graph-custom-node.ts +++ b/packages/engine/src/executor/run-graph-custom-node.ts @@ -100,6 +100,39 @@ export function resolveGraphNodeSessionBoundary(input: { }; } +/* +FNXC:WorkspaceReviewFindings 2026-08-27-12:05: +FN-201 requires the workspace callback to preserve structured reviewer findings. Dropping them here +made a workspace REVISE unremediable even though the per-repository reviewer named actionable work. +*/ +export function toWorkspaceRepoReviewResult(repoOutcome: WorkflowStepOutcome): ReviewResult { + return { + verdict: (repoOutcome.verdict ?? (repoOutcome.success ? "APPROVE" : "UNAVAILABLE")) as ReviewResult["verdict"], + review: repoOutcome.output ?? repoOutcome.error ?? "", + summary: repoOutcome.output ?? repoOutcome.error ?? "", + retryable: !repoOutcome.success, + ...(repoOutcome.findings ? { findings: repoOutcome.findings } : {}), + }; +} + +export function buildWorkspaceReviewOutcome(aggregate: ReviewResult, options: { superseded?: boolean } = {}): WorkflowStepOutcome { + return { + success: aggregate.verdict === "APPROVE", + verdict: aggregate.verdict as WorkflowStepOutcome["verdict"], + output: aggregate.review, + repositoryReviewOutcomes: aggregate.repositoryReviewOutcomes, + repositoryScopeRevision: aggregate.repositoryScopeRevision, + ...(!options.superseded && aggregate.findings ? { findings: aggregate.findings } : {}), + ...(aggregate.verdict === "UNAVAILABLE" ? { failureValue: "workspace-review-unavailable" } : {}), + }; +} + +export function preserveOutcomeFindingsFromReviewOutput(outcome: WorkflowStepOutcome): WorkflowStepOutcome { + if (outcome.findings || typeof outcome.output !== "string") return outcome; + const parsedReviewOutput = parseWorkflowStepOutput(outcome.output, { requireVerdict: false }); + return parsedReviewOutput.findings?.length ? { ...outcome, findings: parsedReviewOutput.findings } : outcome; +} + export async function runGraphCustomNode( deps: RunGraphCustomNodeDeps, node: WorkflowIrNode, @@ -623,12 +656,7 @@ export async function runGraphCustomNode( sessionBoundary: reviewBoundary, ...(repoDiffBaseCommitSha ? { diffBaseCommitSha: repoDiffBaseCommitSha } : {}), }); - return { - verdict: (repoOutcome.verdict ?? (repoOutcome.success ? "APPROVE" : "UNAVAILABLE")) as ReviewResult["verdict"], - review: repoOutcome.output ?? repoOutcome.error ?? "", - summary: repoOutcome.output ?? repoOutcome.error ?? "", - retryable: !repoOutcome.success, - }; + return toWorkspaceRepoReviewResult(repoOutcome); }, { workspaceRepos: workspaceConfig.repos, workspaceRootDir: deps.rootDir, settings }); /* FNXC:RepositoryScope 2026-08-21-02:35: @@ -672,14 +700,7 @@ export async function runGraphCustomNode( repositoryScopeRevision: aggregate.repositoryScopeRevision, }; } - outcome = { - success: aggregate.verdict === "APPROVE", - verdict: aggregate.verdict as WorkflowStepOutcome["verdict"], - output: aggregate.review, - repositoryReviewOutcomes: aggregate.repositoryReviewOutcomes, - repositoryScopeRevision: aggregate.repositoryScopeRevision, - ...(aggregate.verdict === "UNAVAILABLE" ? { failureValue: "workspace-review-unavailable" } : {}), - }; + outcome = buildWorkspaceReviewOutcome(aggregate, { superseded: reviewSuperseded }); } } else if (workspaceConfig && declaredReviewKind === "plan") { /* @@ -723,8 +744,9 @@ export async function runGraphCustomNode( * gain review metadata merely because their output happens to contain a findings key. */ if (declaredReviewKind && typeof outcome.output === "string") { - const parsedReviewOutput = parseWorkflowStepOutput(outcome.output, { requireVerdict: false }); - if (parsedReviewOutput.findings?.length) outcome = { ...outcome, findings: parsedReviewOutput.findings }; + const rawReviewOutput = outcome.output; + outcome = preserveOutcomeFindingsFromReviewOutput(outcome); + const parsedReviewOutput = parseWorkflowStepOutput(rawReviewOutput, { requireVerdict: false }); if (parsedReviewOutput.supersededFindingIds?.length && parsedReviewOutput.supersededFindingSourceWorkflowStepId && !outcome.supersededFindingIds?.length) { outcome = { ...outcome, supersededFindingSourceWorkflowStepId: parsedReviewOutput.supersededFindingSourceWorkflowStepId, supersededFindingIds: parsedReviewOutput.supersededFindingIds }; } diff --git a/packages/engine/src/executor/workspace-review-per-repo.ts b/packages/engine/src/executor/workspace-review-per-repo.ts index 20cf1043d9..0221890130 100644 --- a/packages/engine/src/executor/workspace-review-per-repo.ts +++ b/packages/engine/src/executor/workspace-review-per-repo.ts @@ -12,8 +12,9 @@ * worktrees are never task intent: clean scoped repositories are recorded as not-reviewed and * out-of-scope worktrees are not opened. Modified in-scope verdicts aggregate as a conjunction. * verdict becomes the aggregate verdict (mirroring verifyWorktreeInvariants' first-failing-repo return), and its - * findings are repo-tagged. A zero-acquire workspace task is classified with the completion invariant: proven - * commit-free work approves honestly, while unproven work returns non-retryable UNAVAILABLE. + * findings are repository-qualified before they leave the loop, so aggregate evidence and per-repository + * outcomes match the workspace task's scoped file paths. A zero-acquire workspace task is classified with the + * completion invariant: proven commit-free work approves honestly, while unproven work returns non-retryable UNAVAILABLE. * * 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 @@ -22,17 +23,45 @@ */ import { existsSync } from "node:fs"; import { resolve, sep } from "node:path"; -import type { Settings, Task, WorkflowRepositoryReviewOutcome } from "@fusion/core"; +import type { Settings, Task, WorkflowRepositoryReviewOutcome, WorkflowReviewFinding } from "@fusion/core"; import type { ReviewResult } from "../execution/reviewer.js"; import { captureWorkspaceReviewEvidence } from "../worktree/workspace-review-evidence.js"; import { classifyWorkspaceZeroAcquire, type WorkspaceZeroAcquireOptions } from "./workspace-zero-acquire.js"; import { captureModifiedFiles } from "./worktree-capture-modified-files.js"; +const hasRepositoryPrefix = (value: string | undefined, repoRel: string, separator: "/" | ":") => + value === repoRel || value?.startsWith(`${repoRel}${separator}`) === true; + +/* +FNXC:WorkspaceReviewFindings 2026-08-27-12:05: +FN-201 requires workspace findings to match repository-qualified File Scope and modified-file entries; +unqualified paths are scope-rejected and model-supplied finding identifiers collide across repositories. +*/ +export function qualifyRepositoryFindings(repoRel: string, findings: readonly WorkflowReviewFinding[] | undefined): WorkflowReviewFinding[] | undefined { + if (!findings?.length) return undefined; + return findings.map((finding) => ({ + ...finding, + id: hasRepositoryPrefix(finding.id, repoRel, ":") || hasRepositoryPrefix(finding.id, repoRel, "/") + ? finding.id + : `${repoRel}:${finding.id}`, + ...(finding.filePath + ? { filePath: hasRepositoryPrefix(finding.filePath, repoRel, "/") ? finding.filePath : `${repoRel}/${finding.filePath}` } + : {}), + ...(finding.rebutsDisputedFindingId + ? { + rebutsDisputedFindingId: hasRepositoryPrefix(finding.rebutsDisputedFindingId, repoRel, ":") || hasRepositoryPrefix(finding.rebutsDisputedFindingId, repoRel, "/") + ? finding.rebutsDisputedFindingId + : `${repoRel}:${finding.rebutsDisputedFindingId}`, + } + : {}), + })); +} + export async function 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. + // (Phase C). The loop uses its own iteration key to qualify reviewer findings before aggregation. task: Task, invokeForCwd: (cwd: string) => Promise, options: Omit & { @@ -182,21 +211,24 @@ export async function reviewWorkspacePerRepo( const reviewSections: string[] = notReviewedRepos.map((repoRel) => `### [${repoRel}] NOT_REVIEWED\nNo changes — not reviewed.`); const summarySections: string[] = notReviewedRepos.map((repoRel) => `[${repoRel}] NOT_REVIEWED: no changes`); let firstFailing: { repo: string; result: ReviewResult } | undefined; + const findings: WorkflowReviewFinding[] = []; for (const repoRel of repoKeys) { const repo = workspaceWorktrees[repoRel]; const result = await invokeForCwd(repo.worktreePath); + const qualifiedFindings = qualifyRepositoryFindings(repoRel, result.findings); + if (qualifiedFindings) findings.push(...qualifiedFindings); repositoryReviewOutcomes.push({ repository: repoRel, status: "REVIEWED", verdict: result.verdict, output: result.review, - findings: result.findings, + ...(qualifiedFindings ? { findings: qualifiedFindings } : {}), fingerprint: repositoryDiffFingerprints[repoRel], episodeId: reviewedAt, scopeRevision: repositoryScopeRevision, reviewedAt, }); - // Tag every per-repo finding with its sub-repo so downstream readers attribute it correctly. + // Structured findings are qualified before both durable outcomes and aggregate evidence consume them. reviewSections.push(`### [${repoRel}] ${result.verdict}\n${result.review}`); summarySections.push(`[${repoRel}] ${result.verdict}: ${result.summary}`); if (result.verdict !== "APPROVE") { @@ -222,6 +254,7 @@ export async function reviewWorkspacePerRepo( repositoryDiffFingerprints, repositoryModifiedFiles: modifiedFiles, repositoryReviewOutcomes, + ...(findings.length > 0 ? { findings } : {}), repositoryScopeRevision: repositoryScopeRevision, }; } @@ -234,6 +267,7 @@ export async function reviewWorkspacePerRepo( repositoryDiffFingerprints, repositoryModifiedFiles: modifiedFiles, repositoryReviewOutcomes, + ...(findings.length > 0 ? { findings } : {}), repositoryScopeRevision: repositoryScopeRevision, }; } diff --git a/packages/engine/src/executor/workspace-review-remediation.ts b/packages/engine/src/executor/workspace-review-remediation.ts index 14fc1d4fbf..5a818b7ad7 100644 --- a/packages/engine/src/executor/workspace-review-remediation.ts +++ b/packages/engine/src/executor/workspace-review-remediation.ts @@ -42,8 +42,13 @@ export function deriveWorkspaceReviewRemediation( .filter((outcome) => outcome.status === "REVIEWED" && (outcome.verdict === "REVISE" || outcome.verdict === "RETHINK")) .sort((left, right) => left.repository.localeCompare(right.repository))[0]; if (!blocking) return undefined; + /* + FNXC:WorkspaceReviewConvergence 2026-08-27-12:05: + FN-201 makes workspace findings non-empty. Identifier-based signatures would let a model-assigned + ID change defeat the repeat-unchanged hold and turn bounded remediation into an unbounded loop. + */ const findings = (blocking.findings ?? []) - .map((finding) => `${finding.id}:${normalize(finding.title)}:${normalize(finding.body)}`) + .map((finding) => `${normalize(finding.filePath)}:${finding.line ?? ""}:${normalize(finding.body)}`) .sort() .join("|"); return {