From 358b628b8d30819b5de8a6aab2de08fcc406b66f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 18 Jul 2026 17:24:49 -0700 Subject: [PATCH] fix(FN-8288): preserve approved review and recovery state Fusion-Task-Id: FN-8288 --- .changeset/calm-review-recovery.md | 7 ++ .../ce-workflow-step-executor.test.ts | 29 +++++++ .../src/__tests__/executor-worktree.test.ts | 34 ++++++++ packages/engine/src/executor.ts | 84 ++++++++++++------- 4 files changed, 123 insertions(+), 31 deletions(-) create mode 100644 .changeset/calm-review-recovery.md diff --git a/.changeset/calm-review-recovery.md b/.changeset/calm-review-recovery.md new file mode 100644 index 0000000000..08eba28370 --- /dev/null +++ b/.changeset/calm-review-recovery.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Preserve approved task scope during review and committed work during worktree recovery. +category: fix +dev: Injects approved PROMPT.md into review gates and reclaims inactive same-task branches before cleanup. diff --git a/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts b/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts index 2f76df9d29..cceb274e6b 100644 --- a/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts +++ b/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts @@ -857,6 +857,35 @@ describe("CE workflow-step executor integration", () => { // ── Item 5: FUSION_HEADLESS gating on stepEnv ─────────────────────────────── describe("executeWorkflowStep FUSION_HEADLESS (U3)", () => { + it("makes the approved PROMPT.md contract authoritative during Code Review", async () => { + const store = createMockStore(); + const { executor } = makeExecutor(store); + const cap = captureSession(); + vi.spyOn(executor as any, "readTaskArtifact").mockResolvedValue(` +# Approved contract + +Ship FIVE kinds. Do NOT add roadmap-item in this task. + `.trim()); + + await (executor as any).executeWorkflowStep( + baseStepTask({ description: "Original request: ship SIX kinds including roadmap-item." }), + makeStep({ + id: "graph:code-review-step", + name: "Code Review", + optionalGroupId: "code-review", + prompt: "Review the implementation against the approved task contract.", + gateMode: "gate", + }), + "/tmp/wt", + {}, + ); + + expect(cap.last?.systemPrompt).toContain("--- BEGIN APPROVED PROMPT.md ---"); + expect(cap.last?.systemPrompt).toContain("Ship FIVE kinds. Do NOT add roadmap-item in this task."); + expect(cap.last?.systemPrompt).toContain("PROMPT.md is the authoritative current contract"); + expect(cap.last?.systemPrompt).toContain("Do not enforce superseded requirements from the original Task Description"); + }); + it("sets FUSION_HEADLESS=1 only when unattended=true; always sets FUSION_WORKFLOW_STEP", async () => { const store = createMockStore(); const { executor } = makeExecutor(store); diff --git a/packages/engine/src/__tests__/executor-worktree.test.ts b/packages/engine/src/__tests__/executor-worktree.test.ts index 28d210105b..aeda3090e5 100644 --- a/packages/engine/src/__tests__/executor-worktree.test.ts +++ b/packages/engine/src/__tests__/executor-worktree.test.ts @@ -878,6 +878,40 @@ describe("TaskExecutor worktree recovery", () => { ); }); + it("reclaims an inactive same-task conflict when the branch preserves task commits", async () => { + const store = createMockStore(); + const executor = new TaskExecutor(store, "/tmp/test"); + const conflictPath = "/tmp/test/.worktrees/light-cedar"; + vi.spyOn(executor as any, "shouldGenerateNewWorktreeName").mockResolvedValue(false); + const cleanup = vi.spyOn(executor as any, "cleanupConflictingWorktree").mockResolvedValue(true); + vi.spyOn(branchConflictModule, "inspectBranchConflict").mockResolvedValueOnce({ + kind: "reclaimable", + livePath: conflictPath, + tipSha: "70b47804bc6f27659638e17ac7cf279ed343ff6f", + taskAttributedCommitCount: 10, + strandedCommits: [{ sha: "70b47804bc6f27659638e17ac7cf279ed343ff6f", subject: "fix(FN-8288): preserve implementation" }], + } as any); + + const result = await (executor as any).handleWorktreeConflict( + conflictPath, + "fusion/fn-8288", + "/tmp/test/.worktrees/pearl-otter", + "FN-8288", + "main", + 0, + false, + {}, + ); + + expect(result).toEqual({ path: conflictPath, branch: "fusion/fn-8288" }); + expect(cleanup).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-8288", + expect.stringContaining("10 commits preserved"), + "70b47804bc6f27659638e17ac7cf279ed343ff6f", + ); + }); + it("records recovery context when handling a branch conflict (FN-4847: now discards + requeues instead of pausing)", async () => { // FN-4847: branch-conflict-unrecoverable previously paused the task with // status=failed + pausedReason="branch-conflict-unrecoverable". The user has diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index d2b0377425..6a794ae711 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -16436,10 +16436,11 @@ ${scopeGuard} * FNXC:PlanReviewSpecInjection 2026-07-05-17:20: * FN-7561: the Plan Review reviewer runs readonly with cwd=worktree, but the spec artifact lives at the project root under `.fusion/tasks//PROMPT.md` — OUTSIDE the task worktree. Instructing the agent to "Read PROMPT.md" therefore had it search the worktree, fail to find the file, and emit "no PROMPT.md file was found / task data lives in a DB" prose instead of a parseable verdict. That malformed/hard-failed output fed the unbounded triage↔plan-review replan loop (FN-7525 looped 13+ times overnight; FN-7575 too). Load the spec text from the store (document layer → on-disk PROMPT.md) ONCE and inject it directly into the reviewer prompt so the verdict never depends on the agent locating the file. Read from the store, not fs, so it is correct regardless of worktree vs project-root layout. */ - const planReviewSpecArtifact = isPlanReviewStep + const workflowReviewSpecArtifact = isReviewTypeWorkflowStep ? await this.readTaskArtifact(task.id, "PROMPT.md") : undefined; - const planReviewSpecText = typeof planReviewSpecArtifact === "string" ? planReviewSpecArtifact : ""; + const workflowReviewSpecText = typeof workflowReviewSpecArtifact === "string" ? workflowReviewSpecArtifact : ""; + const planReviewSpecText = isPlanReviewStep ? workflowReviewSpecText : ""; if (isPlanReviewStep && requireExternalIntegrationEvidence) { /* @@ -16500,6 +16501,18 @@ ${scopeGuard} * unrelated local commits can make a plan-only gate reject implementation * state and loop back to triage after the planner already approved the spec. */ + const approvedContractBlock = !isPlanReviewStep && workflowReviewSpecText + ? ` + +Approved Task Contract: +- PROMPT.md is the authoritative current contract for this review. It includes any approved planning revisions and scope decisions. +- The Task Description is historical input only. Do not enforce superseded requirements from the original Task Description when they conflict with PROMPT.md. +- Do not request behavior that PROMPT.md explicitly defers, excludes, or forbids. Review the implementation against the approved contract reproduced below. + +--- BEGIN APPROVED PROMPT.md --- +${workflowReviewSpecText} +--- END APPROVED PROMPT.md ---` + : ""; const scopeBlock = isPlanReviewStep ? `Plan Review Scope: - Review the task plan artifact (PROMPT.md), reproduced verbatim below, and task metadata only. @@ -16516,7 +16529,7 @@ ${scopeFileBlock}${diffShortstat ? `\nDiff stat: ${diffShortstat}` : ""} CRITICAL SCOPING RULES — read before doing anything else: - Review ONLY the files listed above. Do NOT analyze unmodified files or unrelated parts of the codebase. - If NONE of the files in the diff scope are relevant to your review category (e.g. a UX/design reviewer with no UI/CSS/component files in scope, a security reviewer with no auth/network code in scope, an a11y reviewer with no markup changes), respond IMMEDIATELY with a single short approval line such as "No relevant changes in scope — approved." and STOP. Do not start exploring the codebase. -- Your wall-clock budget is short. Spending it browsing unmodified files will cause this step to time out and block merge.`; +- Your wall-clock budget is short. Spending it browsing unmodified files will cause this step to time out and block merge.${approvedContractBlock}`; const latestTaskForUserComments = await this.store.getTask(task.id).catch(() => task); const workflowStepUserComments = selectUserCommentsForAgentContext(latestTaskForUserComments, { limit: null }); @@ -18196,17 +18209,44 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB taskId, ); - if (shouldGenerateNewName) { - const inspection = await inspectBranchConflict({ - repoDir: this.rootDir, - branchName: branch, - conflictingWorktreePath: conflictPath, - requestingTaskId: taskId, - ownerTaskId: taskId, - startPoint, - integrationRef: await resolveIntegrationBranch(this.rootDir, settings), - }); + /* + * FNXC:ExecutorWorktree 2026-07-18-17:20: + * Inspect every branch/worktree collision before cleanup, including inactive + * same-task bindings. The old inactive path skipped inspection and called + * cleanupConflictingWorktree directly, which force-deleted a branch carrying + * completed task commits during workflow-node recovery. Liveness determines + * whether a sibling checkout is needed; it must never determine whether task + * history is disposable. + */ + const inspection = await inspectBranchConflict({ + repoDir: this.rootDir, + branchName: branch, + conflictingWorktreePath: conflictPath, + requestingTaskId: taskId, + ownerTaskId: taskId, + startPoint, + integrationRef: await resolveIntegrationBranch(this.rootDir, settings), + }); + if (inspection.kind === "reclaimable") { + await this.store.logEntry( + taskId, + `[recovery] reclaimed existing worktree for ${taskId} at ${inspection.livePath} (${inspection.taskAttributedCommitCount} commits preserved)`, + inspection.tipSha, + ); + return { path: inspection.livePath, branch }; + } + + if (inspection.kind === "fully-subsumed") { + await this.store.logEntry( + taskId, + `[recovery] reclaimed existing worktree for ${taskId} at ${inspection.livePath} (0 commits preserved)`, + inspection.tipSha, + ); + return { path: inspection.livePath, branch }; + } + + if (shouldGenerateNewName) { if (inspection.kind === "stale" || inspection.kind === "stale-resolved" || inspection.kind === "tip-already-merged") { const cleanupSuccess = await this.cleanupConflictingWorktree(conflictPath, branch, taskId); if (cleanupSuccess) { @@ -18220,24 +18260,6 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB // worktree name. } - if (inspection.kind === "reclaimable") { - await this.store.logEntry( - taskId, - `[recovery] reclaimed existing worktree for ${taskId} at ${inspection.livePath} (${inspection.taskAttributedCommitCount} commits preserved)`, - inspection.tipSha, - ); - return { path: inspection.livePath, branch }; - } - - if (inspection.kind === "fully-subsumed") { - await this.store.logEntry( - taskId, - `[recovery] reclaimed existing worktree for ${taskId} at ${inspection.livePath} (0 commits preserved)`, - inspection.tipSha, - ); - return { path: inspection.livePath, branch }; - } - if (inspection.kind === "live-foreign") { const cleanupSuccess = await this.cleanupConflictingWorktree(inspection.livePath, branch, taskId); if (cleanupSuccess) {