diff --git a/.changeset/fn-7727-persist-failed-step-history.md b/.changeset/fn-7727-persist-failed-step-history.md new file mode 100644 index 0000000000..6a4d90cf5c --- /dev/null +++ b/.changeset/fn-7727-persist-failed-step-history.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Preserve prior failed review-step attempts so self-healing re-runs no longer erase the failure history. +category: fix +dev: Adds an optional `priorAttempts?: WorkflowStepResult[]` field (bounded, single-level, capped at `MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS`) plus a shared pure `upsertWorkflowStepResult(existing, incoming, opts?)` helper in `@fusion/core` (`packages/core/src/workflow-step-results.ts`). Both engine recorders — the executor graph adapter's `recordWorkflowStepResult` and triage's `recordPlanReviewWorkflowResult` — now route through this helper instead of a bare replace-in-place upsert, so a self-healing recovery re-run of a failed pre-merge review node (code-review, plan-review, browser-verification) snapshots the prior `failed`/`advisory_failure` attempt into `priorAttempts` rather than overwriting it. Selection (self-healing, merge-blocker, progress/timing) is unchanged and reads only the current entry; `priorAttempts` is read-only history, surfaced in the task-detail Summary tab's Workflow results list as a collapsed "previous failed attempts" disclosure. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 08a4d462f1..46da9c9dc4 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -769,6 +769,21 @@ Results now live on `task.workflowStepResults` (written by the graph executor, k Gate results are recorded on the task's **`workflowStepResults`** field (`WorkflowStepResult[]`), written by the graph executor and keyed by the optional-group node id. Each entry carries `status`, optional `verdict`, `notes`, `output`, and `phase`. The unified progress bar and the Workflow tab read this field directly. + + +A step re-executed after a prior failed attempt (most commonly via self-healing's `recoverReviewTasksWithFailedPreMergeSteps` recovery sweep) preserves that prior attempt's `output`/`notes`/`verdict`/timestamps in a bounded `priorAttempts` array on the surviving entry, rather than losing them. This history is read-only — it never affects merge-blocking, recovery selection, or progress computation — and is surfaced in the task-detail Summary tab as a collapsed "previous failed attempts" disclosure when present. + The persisted `status` values are `pending`, `passed`, `failed`, `advisory_failure`, and `skipped`. The verdict→status mapping is: `APPROVE` / `APPROVE_WITH_NOTES` → `passed`; an advisory `REVISE` (success outcome) → `advisory_failure` (non-blocking); a gate `REVISE` or hard failure → `failed`. The UI derives an additional **`running`** display state from a `pending` entry that has a `startedAt` and no `completedAt`. Advisory failures (`advisory_failure`) are shown as polish feedback and never block merge; only `failed` blocks. Workflow status is visible in multiple places: diff --git a/packages/core/src/__tests__/workflow-step-results.test.ts b/packages/core/src/__tests__/workflow-step-results.test.ts new file mode 100644 index 0000000000..f636778df4 --- /dev/null +++ b/packages/core/src/__tests__/workflow-step-results.test.ts @@ -0,0 +1,138 @@ +import { describe, it, expect } from "vitest"; +import { upsertWorkflowStepResult, MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS } from "../workflow-step-results.js"; +import type { WorkflowStepResult } from "../types.js"; + +function makeResult(overrides: Partial = {}): WorkflowStepResult { + return { + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "failed", + ...overrides, + }; +} + +describe("upsertWorkflowStepResult", () => { + it("appends when the step id is absent", () => { + const result = makeResult({ startedAt: "T1" }); + const next = upsertWorkflowStepResult(undefined, result); + expect(next).toEqual([result]); + expect(next).not.toBe(undefined); + }); + + it("replaces in place preserving array position", () => { + const other = makeResult({ workflowStepId: "plan-review", startedAt: "T0" }); + const first = makeResult({ startedAt: "T1", output: "attempt-1" }); + const existing = [other, first]; + const second = makeResult({ startedAt: "T2", output: "attempt-2" }); + const next = upsertWorkflowStepResult(existing, second); + expect(next).toHaveLength(2); + expect(next[0]).toEqual(other); + expect(next[1].workflowStepId).toBe("code-review"); + expect(next[1].output).toBe("attempt-2"); + }); + + it("snapshots a replaced failed entry into priorAttempts (Symptom Verification)", () => { + const attempt1 = makeResult({ startedAt: "T1", output: "attempt-1 feedback", status: "failed" }); + const attempt2 = makeResult({ startedAt: "T2", output: "attempt-2 feedback", status: "failed" }); + const next = upsertWorkflowStepResult([attempt1], attempt2); + expect(next).toHaveLength(1); + expect(next[0].output).toBe("attempt-2 feedback"); + expect(next[0].priorAttempts).toHaveLength(1); + expect(next[0].priorAttempts?.[0].output).toBe("attempt-1 feedback"); + expect(next[0].priorAttempts?.[0].status).toBe("failed"); + expect(next[0].priorAttempts?.[0].startedAt).toBe("T1"); + }); + + it("snapshots a replaced advisory_failure entry", () => { + const attempt1 = makeResult({ startedAt: "T1", status: "advisory_failure", output: "advisory-1" }); + const attempt2 = makeResult({ startedAt: "T2", status: "passed", output: "attempt-2" }); + const next = upsertWorkflowStepResult([attempt1], attempt2); + expect(next[0].priorAttempts).toHaveLength(1); + expect(next[0].priorAttempts?.[0].output).toBe("advisory-1"); + }); + + it("does NOT snapshot when the replaced entry was passed/skipped/pending", () => { + for (const status of ["passed", "skipped", "pending"] as const) { + const attempt1 = makeResult({ startedAt: "T1", status, output: "attempt-1" }); + const attempt2 = makeResult({ startedAt: "T2", status: "failed", output: "attempt-2" }); + const next = upsertWorkflowStepResult([attempt1], attempt2); + expect(next[0].priorAttempts ?? []).toHaveLength(0); + } + }); + + it("dedupes a same-run pending -> failed transition of the same attempt (no phantom duplicate)", () => { + const pending = makeResult({ startedAt: "T1", status: "pending" }); + const failed = makeResult({ startedAt: "T1", status: "failed", output: "final" }); + const next = upsertWorkflowStepResult([pending], failed); + expect(next).toHaveLength(1); + expect(next[0].priorAttempts ?? []).toHaveLength(0); + expect(next[0].status).toBe("failed"); + }); + + it("bounds priorAttempts to the cap across N successive failed re-runs, dropping oldest, newest-first", () => { + let existing: WorkflowStepResult[] | undefined; + const total = MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS + 3; + for (let i = 1; i <= total; i++) { + existing = upsertWorkflowStepResult(existing, makeResult({ startedAt: `T${i}`, status: "failed", output: `attempt-${i}` })); + } + const finalEntry = existing?.[0]; + expect(finalEntry?.output).toBe(`attempt-${total}`); + expect(finalEntry?.priorAttempts).toHaveLength(MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS); + // Newest-first: the most recently replaced attempt (total - 1) should be first. + expect(finalEntry?.priorAttempts?.[0].output).toBe(`attempt-${total - 1}`); + // Oldest attempts (1..(total - 1 - cap)) should have been dropped. + const outputs = finalEntry?.priorAttempts?.map((r) => r.output) ?? []; + expect(outputs).not.toContain("attempt-1"); + }); + + it("respects a custom maxPriorAttempts option", () => { + let existing: WorkflowStepResult[] | undefined; + for (let i = 1; i <= 4; i++) { + existing = upsertWorkflowStepResult(existing, makeResult({ startedAt: `T${i}`, status: "failed", output: `attempt-${i}` }), { maxPriorAttempts: 1 }); + } + expect(existing?.[0].priorAttempts).toHaveLength(1); + expect(existing?.[0].priorAttempts?.[0].output).toBe("attempt-3"); + }); + + it("never mutates the input array or entries", () => { + const attempt1 = makeResult({ startedAt: "T1", status: "failed", output: "attempt-1" }); + const existing = [attempt1]; + const existingCopy = JSON.parse(JSON.stringify(existing)); + const attempt2 = makeResult({ startedAt: "T2", status: "failed", output: "attempt-2" }); + const next = upsertWorkflowStepResult(existing, attempt2); + expect(existing).toEqual(existingCopy); + expect(next).not.toBe(existing); + }); + + it("strips nested priorAttempts from a snapshot to a single level", () => { + const grandparent = makeResult({ startedAt: "T1", status: "failed", output: "gp" }); + let existing = upsertWorkflowStepResult(undefined, grandparent); + const parent = makeResult({ startedAt: "T2", status: "failed", output: "parent" }); + existing = upsertWorkflowStepResult(existing, parent); + expect(existing[0].priorAttempts).toHaveLength(1); + + const child = makeResult({ startedAt: "T3", status: "failed", output: "child" }); + existing = upsertWorkflowStepResult(existing, child); + expect(existing[0].priorAttempts).toHaveLength(2); + // Every snapshot in the history must itself be single-level (no nested priorAttempts). + for (const snapshot of existing[0].priorAttempts ?? []) { + expect(snapshot.priorAttempts).toBeUndefined(); + } + }); + + it("carries forward already-accumulated priorAttempts across a non-failure re-run", () => { + const attempt1 = makeResult({ startedAt: "T1", status: "failed", output: "attempt-1" }); + let existing = upsertWorkflowStepResult(undefined, attempt1); + const attempt2 = makeResult({ startedAt: "T2", status: "failed", output: "attempt-2" }); + existing = upsertWorkflowStepResult(existing, attempt2); + expect(existing[0].priorAttempts).toHaveLength(1); + + // A later passing attempt should still carry forward the accumulated history, + // plus snapshot the failed attempt-2 entry it replaced. + const attempt3 = makeResult({ startedAt: "T3", status: "passed", output: "attempt-3" }); + existing = upsertWorkflowStepResult(existing, attempt3); + expect(existing[0].status).toBe("passed"); + expect(existing[0].priorAttempts).toHaveLength(2); + expect(existing[0].priorAttempts?.map((r) => r.output)).toEqual(["attempt-2", "attempt-1"]); + }); +}); diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index 59772b722e..cb24589c42 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -2107,3 +2107,7 @@ export { hasSyncPassphraseConfigured, } from "./secrets-sync-passphrase.js"; export { suggestTaskPrefix } from "./task-prefix.js"; +export { + upsertWorkflowStepResult, + MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS, +} from "./workflow-step-results.js"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index aa73d7dd50..c470af7fcc 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2127,3 +2127,7 @@ export { hasSyncPassphraseConfigured, } from "./secrets-sync-passphrase.js"; export { suggestTaskPrefix } from "./task-prefix.js"; +export { + upsertWorkflowStepResult, + MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS, +} from "./workflow-step-results.js"; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 1442bf8d3a..a8c98f5525 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -907,6 +907,25 @@ export interface WorkflowStepResult { bypassedFromStatus?: WorkflowStepResult["status"]; /** The `verdict` (if any) this result carried immediately before the bypass, preserved for audit only — never promoted to `verdict`. */ bypassedFromVerdict?: WorkflowStepResult["verdict"]; + /* + * FNXC:WorkflowStepResults 2026-07-09-00:10: + * FN-7727: self-healing recovery re-runs a failed pre-merge review node + * (`code-review`, `code-review-remediation`, `plan-review`, + * `browser-verification`) in place, and the recorder upsert previously + * REPLACED the prior `status:"failed"` entry — erasing its captured + * `output`/`notes`/`verdict`/timestamps forever (the diagnostic trail + * FN-7642 worked to capture, and the history FN-7720's bypass affordance + * needs to show). `priorAttempts` preserves a BOUNDED, single-level history + * of prior terminal-failure (`failed`/`advisory_failure`) attempts on the + * surviving entry — snapshots never carry their own nested `priorAttempts`, + * so history cannot grow unbounded. This field is READ-ONLY history: it + * never participates in merge-blocking (`getTaskMergeBlocker`), self-healing + * recovery selection (`latestFailedPreMergeStep`), or progress/timing + * computation — only the current (this) entry's fields do. Written by the + * shared `upsertWorkflowStepResult` helper (`workflow-step-results.ts`). + */ + /** Bounded, single-level history of prior terminal-failure attempts this entry replaced. Read-only; never affects merge-blocking or recovery selection. */ + priorAttempts?: WorkflowStepResult[]; } /** diff --git a/packages/core/src/workflow-step-results.ts b/packages/core/src/workflow-step-results.ts new file mode 100644 index 0000000000..4898924802 --- /dev/null +++ b/packages/core/src/workflow-step-results.ts @@ -0,0 +1,99 @@ +import type { WorkflowStepResult } from "./types.js"; + +/* +FNXC:WorkflowStepResults 2026-07-09-00:20: +FN-7727: both engine `WorkflowStepResult` recorders (the executor graph adapter's +`recordWorkflowStepResult` and triage's `recordPlanReviewWorkflowResult`) used to +upsert by `workflowStepId` with a bare `existing[idx] = result` replace-in-place. +When self-healing (`recoverFailedPreMergeWorkflowStep` / +`recoverReviewTasksWithFailedPreMergeSteps`) sends a failed pre-merge review step +back for fix and the graph re-runs that same node, the new attempt silently +overwrote the prior `status:"failed"` record — losing its `output`/`notes`/ +`verdict`/timestamps forever. This shared, PURE helper is the single upsert path +for every recorder: it snapshots a replaced `failed`/`advisory_failure` entry into +the new entry's `priorAttempts` (bounded, oldest-dropped, single-level — snapshots +never carry their own nested `priorAttempts`), and carries forward already- +accumulated history across successive re-runs. `priorAttempts` is read-only +history: callers that select "the current failed step" (self-healing selection, +`getTaskMergeBlocker`, progress/timing) must keep reading the top-level array +entries only and never flatten/inspect `priorAttempts` for that purpose. +*/ + +/** Default cap on the number of prior terminal-failure attempts retained per step. */ +export const MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS = 5; + +const TERMINAL_FAILURE_STATUSES: ReadonlySet = new Set([ + "failed", + "advisory_failure", +]); + +function isTerminalFailure(result: WorkflowStepResult): boolean { + return TERMINAL_FAILURE_STATUSES.has(result.status); +} + +/** + * Strip a result down to a single-level history snapshot: its own + * `priorAttempts` are dropped so nesting never grows beyond one level deep. + */ +function toSnapshot(result: WorkflowStepResult): WorkflowStepResult { + if (!result.priorAttempts || result.priorAttempts.length === 0) return result; + const { priorAttempts: _drop, ...rest } = result; + return rest as WorkflowStepResult; +} + +/** + * Pure upsert of a `WorkflowStepResult` by `workflowStepId`, preserving a + * bounded history of prior terminal-failure attempts on the surviving entry. + * + * - Absent → the incoming result is appended. + * - Present → the existing entry is replaced IN PLACE (array position + * preserved). The existing entry's already-accumulated `priorAttempts` are + * carried forward onto the incoming result. If the existing entry represents + * a DIFFERENT attempt (deduped by `startedAt` — a same-run `pending`→`failed` + * transition of the same attempt is not a new attempt) and its status is a + * terminal failure (`failed` | `advisory_failure`), a single-level snapshot + * of it is pushed onto the incoming result's `priorAttempts`. + * - `priorAttempts` is bounded to `opts.maxPriorAttempts` (default + * `MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS`), newest-first, oldest dropped. + * + * Never mutates `existing` or `incoming`; always returns a new array. + */ +export function upsertWorkflowStepResult( + existing: WorkflowStepResult[] | undefined, + incoming: WorkflowStepResult, + opts?: { maxPriorAttempts?: number }, +): WorkflowStepResult[] { + const maxPriorAttempts = opts?.maxPriorAttempts ?? MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS; + const source = existing ?? []; + const idx = source.findIndex((r) => r.workflowStepId === incoming.workflowStepId); + + if (idx < 0) { + const next = [...source]; + next.push({ ...incoming }); + return next; + } + + const previous = source[idx]; + const isSameAttempt = previous.startedAt !== undefined + && incoming.startedAt !== undefined + && previous.startedAt === incoming.startedAt; + + let priorAttempts = previous.priorAttempts ? [...previous.priorAttempts] : []; + if (!isSameAttempt && isTerminalFailure(previous)) { + priorAttempts = [toSnapshot(previous), ...priorAttempts]; + } + if (priorAttempts.length > maxPriorAttempts) { + priorAttempts = priorAttempts.slice(0, maxPriorAttempts); + } + + const replacement: WorkflowStepResult = { ...incoming }; + if (priorAttempts.length > 0) { + replacement.priorAttempts = priorAttempts; + } else { + delete replacement.priorAttempts; + } + + const next = [...source]; + next[idx] = replacement; + return next; +} diff --git a/packages/dashboard/app/components/TaskDetailModal.css b/packages/dashboard/app/components/TaskDetailModal.css index 9022026b87..cb3638a937 100644 --- a/packages/dashboard/app/components/TaskDetailModal.css +++ b/packages/dashboard/app/components/TaskDetailModal.css @@ -2502,11 +2502,62 @@ The desktop table needs a token-scale min-width and non-anywhere model-name wrap } .task-summary-work-list li { + display: flex; + flex-direction: column; + align-items: stretch; + gap: var(--space-xs); +} + +.task-summary-work-list-row { display: flex; align-items: center; gap: var(--space-sm); } +/* +FNXC:WorkflowStepResults 2026-07-09-00:45: +FN-7727: bounded, read-only history of prior failed pre-merge review attempts +(`WorkflowStepResult.priorAttempts`), surfaced as a collapsed
so it +does not clutter the default workflow-results view. Rendered only when a step +has history; no shell appears otherwise. +*/ +.task-summary-prior-attempts { + margin-left: calc(var(--space-sm) * 2); + font-size: calc(var(--space-sm) + var(--space-xs) * 0.5); + color: var(--text-muted); +} + +.task-summary-prior-attempts summary { + cursor: pointer; + color: var(--text-muted); +} + +.task-summary-prior-attempts-list { + display: flex; + flex-direction: column; + gap: var(--space-xs); + margin: var(--space-xs) 0 0; + padding: 0; + list-style: none; +} + +.task-summary-prior-attempts-list li { + padding: var(--space-xs) var(--space-sm); + background: var(--surface-muted); + border-radius: var(--radius-sm); +} + +.task-summary-prior-attempts-timestamp { + margin-left: var(--space-sm); + color: var(--text-muted); +} + +.task-summary-prior-attempts-output { + margin: var(--space-xs) 0 0; + overflow-wrap: anywhere; + color: var(--text-muted); +} + .task-summary-status { flex-shrink: 0; padding: var(--space-xs) var(--space-sm); @@ -2629,6 +2680,10 @@ The desktop table needs a token-scale min-width and non-anywhere model-name wrap .task-summary-work-list li { align-items: flex-start; } + + .task-summary-prior-attempts { + margin-left: 0; + } } /* Spec tab layout - allows SpecEditor to fill available vertical space */ diff --git a/packages/dashboard/app/components/TaskSummaryTab.tsx b/packages/dashboard/app/components/TaskSummaryTab.tsx index ba302a3f3b..5fd9a50908 100644 --- a/packages/dashboard/app/components/TaskSummaryTab.tsx +++ b/packages/dashboard/app/components/TaskSummaryTab.tsx @@ -307,12 +307,42 @@ export function TaskSummaryTab({ task, pricingOverrides }: TaskSummaryTabProps)
{t("taskDetail.summaryTab.workflowResults", "Workflow results")}
    - {workflowResults.map((result) => ( -
  • - {result.status.replace("_", " ")} - {result.workflowStepName} -
    • - ))} + {workflowResults.map((result) => { + /* + FNXC:WorkflowStepResults 2026-07-09-00:40: + FN-7727: `priorAttempts` is bounded, read-only history + populated by the shared `upsertWorkflowStepResult` core + helper when a pre-merge review node (code-review, + plan-review, browser-verification) is re-run after a prior + failed attempt. Render it only when present — steps with no + history render no extra affordance (no orphaned shell). + */ + const priorAttempts = result.priorAttempts ?? []; + return ( +
  • +
    + {result.status.replace("_", " ")} + {result.workflowStepName} +
    + {priorAttempts.length > 0 && ( +
    + + {t("taskDetail.summaryTab.priorAttempts", "{{count}} previous failed attempt{{plural}}", { count: priorAttempts.length, plural: priorAttempts.length === 1 ? "" : "s" })} + +
      + {priorAttempts.map((attempt, index) => ( +
    • + {attempt.status.replace("_", " ")} + {attempt.startedAt && {attempt.startedAt}} + {attempt.output &&

      {attempt.output}

      } +
      • + ))} +
    +
    + )} +
    • + ); + })}
)} diff --git a/packages/dashboard/app/components/__tests__/TaskSummaryTab.prior-attempts.test.tsx b/packages/dashboard/app/components/__tests__/TaskSummaryTab.prior-attempts.test.tsx new file mode 100644 index 0000000000..e08f04f348 --- /dev/null +++ b/packages/dashboard/app/components/__tests__/TaskSummaryTab.prior-attempts.test.tsx @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { makeTask, readDashboardStylesSource, setupTaskDetailModalHooks } from "./TaskDetailModal.test-helpers"; +import { TaskSummaryTab } from "../TaskSummaryTab"; + +setupTaskDetailModalHooks(); + +function doneTaskWithResults(workflowStepResults: unknown[]) { + return makeTask({ + column: "done", + workflowStepResults: workflowStepResults as import("@fusion/core").WorkflowStepResult[], + }); +} + +describe("TaskSummaryTab prior-attempts history (FN-7727)", () => { + it("renders a collapsed prior-attempts affordance for a step with history", () => { + render( + , + ); + + expect(screen.getByText("Code Review")).toBeTruthy(); + const details = screen.getByTestId("task-summary-prior-attempts"); + expect(details).toBeTruthy(); + expect(details.tagName.toLowerCase()).toBe("details"); + expect(screen.getByText("1 previous failed attempt")).toBeTruthy(); + expect(screen.getByText("attempt-1 feedback")).toBeTruthy(); + }); + + it("renders nothing (no orphaned shell) when priorAttempts is absent or empty", () => { + render( + , + ); + + expect(screen.queryByTestId("task-summary-prior-attempts")).toBeNull(); + }); + + it("pluralizes multiple prior attempts", () => { + render( + , + ); + + expect(screen.getByText("2 previous failed attempts")).toBeTruthy(); + }); + + it("uses design tokens for the prior-attempts block and includes a mobile breakpoint rule", () => { + const css = readDashboardStylesSource(); + expect(css).toContain(".task-summary-prior-attempts"); + const mobileBlock = css.slice(css.indexOf("@media (max-width: 768px)"), css.indexOf("/* Spec tab layout")); + expect(mobileBlock).toContain(".task-summary-prior-attempts"); + expect(css).not.toMatch(/task-summary-prior-attempts[^{}]*#[0-9a-fA-F]{3,8}/); + }); +}); diff --git a/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts b/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts index 08867b7a80..1aa5f73c63 100644 --- a/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts +++ b/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts @@ -56,4 +56,31 @@ describe("clearTerminalWorkflowStepFailures", () => { expect(clearTerminalWorkflowStepFailures(undefined)).toEqual([]); expect(clearTerminalWorkflowStepFailures([])).toEqual([]); }); + + /* + FNXC:WorkflowStepResults 2026-07-09-00:50: + FN-7727: explicit retry clean-slate decision — dropping a failed/advisory_failure + entry that carries `priorAttempts` history must not throw, and the whole entry + (including its history) is dropped, since retry is a deliberate clean slate + distinct from the self-healing recovery re-run path (which goes through + upsertWorkflowStepResult instead and preserves history). + */ + it("does not throw on and fully drops a failed entry carrying priorAttempts history", () => { + const withHistory = result({ + workflowStepId: "code-review", + status: "failed", + priorAttempts: [ + result({ workflowStepId: "code-review", status: "failed", startedAt: "T1" }), + result({ workflowStepId: "code-review", status: "advisory_failure", startedAt: "T2" }), + ], + }); + const input = [ + result({ workflowStepId: "plan-review", status: "passed" }), + withHistory, + ]; + expect(() => clearTerminalWorkflowStepFailures(input)).not.toThrow(); + expect(clearTerminalWorkflowStepFailures(input)).toEqual([ + result({ workflowStepId: "plan-review", status: "passed" }), + ]); + }); }); diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index ba14bdc1b6..4fe89ed866 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -6033,6 +6033,56 @@ describe("SelfHealingManager", () => { managerWithRecovery.stop(); }); + + /* + FNXC:WorkflowStepResults 2026-07-09-01:00: + FN-7727: `priorAttempts` is read-only history and must never re-trigger + recovery. A step whose CURRENT entry is no longer failed (e.g. it was + later passed/skipped) but carries a `priorAttempts` snapshot from an + earlier failed attempt must be treated as satisfied — selection reads + only the current entry's `status`. + */ + it("ignores a historical failed snapshot in priorAttempts when the current entry is no longer failed", async () => { + const recoverFn = vi.fn().mockResolvedValue(true); + const managerWithRecovery = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + recoverFailedPreMergeStep: recoverFn, + }); + (store.getSettings as ReturnType).mockResolvedValue({ + maxPostReviewFixes: 2, + }); + (store.listTasks as ReturnType).mockResolvedValue([{ + ...baseTask, + workflowStepResults: [ + { + workflowStepId: "WS-004", + workflowStepName: "Browser Verification", + phase: "pre-merge" as const, + status: "passed" as const, + startedAt: "2026-04-18T00:00:00.000Z", + completedAt: "2026-04-18T00:05:00.000Z", + priorAttempts: [ + { + workflowStepId: "WS-004", + workflowStepName: "Browser Verification", + phase: "pre-merge" as const, + status: "failed" as const, + output: "SSE reconnect leaks /api/events connections when view toggles.", + startedAt: "2026-04-17T21:08:24.135Z", + completedAt: "2026-04-17T21:35:32.036Z", + }, + ], + }, + ], + }]); + + const result = await managerWithRecovery.recoverReviewTasksWithFailedPreMergeSteps(); + + expect(result).toBe(0); + expect(recoverFn).not.toHaveBeenCalled(); + + managerWithRecovery.stop(); + }); }); describe("surfaceInReviewStalls", () => { diff --git a/packages/engine/src/__tests__/workflow-step-results-self-healing-recovery.test.ts b/packages/engine/src/__tests__/workflow-step-results-self-healing-recovery.test.ts new file mode 100644 index 0000000000..751d9c539d --- /dev/null +++ b/packages/engine/src/__tests__/workflow-step-results-self-healing-recovery.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from "vitest"; +import { upsertWorkflowStepResult } from "@fusion/core"; +import type { TaskDetail, WorkflowIr, WorkflowStepResult } from "@fusion/core"; + +import { WorkflowGraphExecutor, type WorkflowNodeHandler, type WorkflowNodeResult } from "../workflow-graph-executor.js"; + +/* +FNXC:WorkflowStepResults 2026-07-09-00:55: +FN-7727 (Symptom Verification): self-healing (`recoverFailedPreMergeWorkflowStep` / +`recoverReviewTasksWithFailedPreMergeSteps`) re-runs a failed pre-merge review node +in place. Before this fix, the executor graph adapter's `recordWorkflowStepResult` +did `existing[idx] = result` and silently erased the prior failed attempt. This +test drives the EXACT adapter contract production uses (`upsertWorkflowStepResult` +from `@fusion/core`, persisted through a fake store's getTask/updateTask round-trip, +the same shape as `TaskExecutor`'s `recordWorkflowStepResult` closure) across two +separate `WorkflowGraphExecutor.run()` dispatches of the SAME node — simulating a +self-healing recovery re-run — and asserts the prior failed attempt survives in +`priorAttempts`. +*/ + +const settingsOn = () => ({ experimentalFeatures: { workflowGraphExecutor: true } }); + +function taskWith(enabled: string[] | undefined): TaskDetail { + return { id: "FN-CR", enabledWorkflowSteps: enabled } as TaskDetail; +} + +/** A single optional-group ("code-review") node graph, matching the production + * pre-merge review shape closely enough to exercise the recorder contract. */ +function codeReviewGroupIr(): WorkflowIr { + return { + version: "v2", + name: "code-review-recovery-test", + columns: [{ id: "work", name: "Work", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { + id: "code-review", + kind: "optional-group", + config: { + name: "Code review", + defaultOn: false, + template: { + nodes: [{ id: "reviewstep", kind: "prompt", config: { prompt: "review" } }], + edges: [], + }, + }, + }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "code-review" }, + { from: "code-review", to: "end", condition: "success" }, + { from: "code-review", to: "end", condition: "failure" }, + ], + }; +} + +function innerHandler(reviewResult: WorkflowNodeResult): WorkflowNodeHandler { + return async (node) => (node.id === "reviewstep" ? reviewResult : { outcome: "success" }); +} + +/** Fake store + recorder mirroring TaskExecutor.recordWorkflowStepResult (executor.ts + * ~5040): getTask -> upsertWorkflowStepResult -> updateTask, persisted between calls + * so a second dispatch sees the first dispatch's persisted result — exactly the + * self-healing recovery re-run shape (parked in-review, re-dispatched later). */ +function makeFakeStore() { + let workflowStepResults: WorkflowStepResult[] = []; + const record = async (_taskId: string, result: WorkflowStepResult) => { + const live = { workflowStepResults }; + workflowStepResults = upsertWorkflowStepResult(live.workflowStepResults, result); + }; + return { + record, + getResults: () => workflowStepResults, + }; +} + +describe("self-healing recovery re-run preserves prior failed WorkflowStepResult history (FN-7727)", () => { + it("keeps the current failed entry plus the prior failed attempt in priorAttempts across two dispatches", async () => { + const store = makeFakeStore(); + + // First dispatch (initial pre-merge run): code-review REVISEs -> failed. + const firstExecutor = new WorkflowGraphExecutor({ + handlers: { prompt: innerHandler({ outcome: "failure", value: "REVISE" }) }, + recordWorkflowStepResult: store.record, + }); + const firstRun = await firstExecutor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr()); + expect(firstRun.outcome).toBe("failure"); + + const afterFirst = store.getResults(); + const firstEntry = afterFirst.find((r) => r.workflowStepId === "code-review"); + expect(firstEntry?.status).toBe("failed"); + expect(firstEntry?.priorAttempts ?? []).toHaveLength(0); + + // Self-healing sends the task back for fix; the graph re-runs the SAME node. + // Second dispatch: code-review REVISEs again -> a NEW failed attempt. + const secondExecutor = new WorkflowGraphExecutor({ + handlers: { prompt: innerHandler({ outcome: "failure", value: "REVISE" }) }, + recordWorkflowStepResult: store.record, + }); + const secondRun = await secondExecutor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr()); + expect(secondRun.outcome).toBe("failure"); + + const afterSecond = store.getResults(); + // Exactly ONE current code-review entry — the Symptom Verification assertion. + const codeReviewEntries = afterSecond.filter((r) => r.workflowStepId === "code-review"); + expect(codeReviewEntries).toHaveLength(1); + const finalEntry = codeReviewEntries[0]; + expect(finalEntry.status).toBe("failed"); + // The FIRST dispatch's failed attempt must be preserved in priorAttempts, not lost. + expect(finalEntry.priorAttempts?.length).toBeGreaterThanOrEqual(1); + expect(finalEntry.priorAttempts?.[0].status).toBe("failed"); + expect(finalEntry.priorAttempts?.[0].startedAt).toBe(firstEntry?.startedAt); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 8d9e0c3e7f..3362d523d3 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -11,7 +11,7 @@ import { existsSync, lstatSync, realpathSync } from "node:fs"; import { readFile, rm, writeFile } from "node:fs/promises"; import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, WorkflowStep, MissionStore, Slice, AgentState, AgentCapability, RunMutationContext, AgentHeartbeatConfig, Agent, AgentMemoryInclusionMode, ProjectSettings, MergeResult, WorkflowIrNode, WorkflowIrNodeKind, WorkflowStepResult as CoreWorkflowStepResult } from "@fusion/core"; import { getUnmetSchedulingDependencies } from "./scheduler.js"; -import { RetryStormError, TaskDeletedError, serializeRetryStormError, isExperimentalFeatureEnabled, resolveWorkflowIrForTask, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, isSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, COMPLETION_SUMMARY_NODE_ID } from "@fusion/core"; +import { RetryStormError, TaskDeletedError, serializeRetryStormError, isExperimentalFeatureEnabled, resolveWorkflowIrForTask, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, isSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult } from "@fusion/core"; import { finalizeProvenAutoMergeTask } from "./auto-merge-finalization.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; import type { TaskStep, WorkflowIr, WorkflowFieldDefinition, WorkflowColumnAgent, EffectiveAgentInput, WorkflowWorkEngineDispatchResult } from "@fusion/core"; @@ -5041,12 +5041,15 @@ export class TaskExecutor { if (typeof this.store.updateTask !== "function") return; try { const live = await this.store.getTask(taskId); - const existing = Array.isArray(live?.workflowStepResults) - ? [...live.workflowStepResults] - : []; - const idx = existing.findIndex((r) => r.workflowStepId === result.workflowStepId); - if (idx >= 0) existing[idx] = result; - else existing.push(result); + /* + FNXC:WorkflowStepResults 2026-07-09-00:25: + FN-7727: route through the shared, pure upsert helper instead of a + bare `existing[idx] = result` replace-in-place — a self-healing + recovery re-run of this same node (e.g. code-review sent back for + fix) must preserve the prior `status:"failed"` entry's history in + `priorAttempts` rather than silently overwriting it. + */ + const existing = upsertWorkflowStepResult(live?.workflowStepResults, result); await this.store.updateTask(taskId, { workflowStepResults: existing }, this.getRunContextFor(taskId)); } catch { // Result recording is additive visibility — never affect the run. @@ -18233,6 +18236,18 @@ function hasNonTerminalWorkflowSteps(task: Pick): boolean { /* FNXC:ReviewLeniency 2026-07-02-01:00: Retrying a task must clear PRIOR FAILURE states so the retry starts clean — including on optional gate nodes like code-review / browser-verification. Results are upserted by node id, so a re-running node overwrites its own stale entry, but a send-back-for-fix leaves the failed entry in place until (and unless) that node re-runs; meanwhile self-healing's failed-pre-merge scan and the dashboard both see a stale failure, and a node that is skipped/relaxed on the retry never clears it. Drop every terminal failure result (`failed`/`advisory_failure`) on retry while keeping `passed`/`skipped`/`pending` evidence (so a previously-passed Plan Review is not re-run). Returns the same array reference when nothing changed so callers can skip a no-op write. + +FNXC:WorkflowStepResults 2026-07-09-00:30: +FN-7727 explicit decision: an explicit user/agent RETRY remains a clean-slate — +it MAY drop the current `failed`/`advisory_failure` entry entirely (along with +any `priorAttempts` history it carried), since retry is deliberately +discarding prior failure state, not preserving it. This is DIFFERENT from the +self-healing recovery re-run path (`recoverFailedPreMergeWorkflowStep` / +`recoverReviewTasksWithFailedPreMergeSteps`), which does NOT call this +function — that path re-runs the SAME node in place and its result goes +through `upsertWorkflowStepResult`, which is where prior-attempt history is +preserved. This filter must not throw on entries carrying `priorAttempts` +(it only reads `status`, so `priorAttempts` is inert here regardless). */ export function clearTerminalWorkflowStepFailures( results: CoreWorkflowStepResult[] | undefined, diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 30297b9db4..7341ba7aa3 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -36,6 +36,7 @@ import { applyFrontendUxCriteria, extractEffectiveWriteScopeFromPrompt, MAX_TASK_LIST_TEXT_CHARS, + upsertWorkflowStepResult, type NearDuplicateCandidate, } from "@fusion/core"; @@ -1979,12 +1980,13 @@ export class TriageProcessor { planLog.warn(`${task.id}: failed to load existing Plan Review workflow results; preserving in-memory result baseline: ${message}`); return task; }); - const existing = Array.isArray(live?.workflowStepResults) - ? [...live.workflowStepResults] - : []; - const idx = existing.findIndex((entry) => entry.workflowStepId === PLAN_REVIEW_GROUP_ID); - if (idx >= 0) existing[idx] = result; - else existing.push(result); + /* + FNXC:WorkflowStepResults 2026-07-09-00:35: + FN-7727: route through the shared upsert helper (same as the executor + graph adapter) so a re-run of Plan Review after a failed attempt preserves + that prior attempt's history in `priorAttempts` instead of overwriting it. + */ + const existing = upsertWorkflowStepResult(live?.workflowStepResults, result); await this.store.updateTask(task.id, { workflowStepResults: existing }); }