From 9f24a517cfc202412bd2afd177cd1962e7beb84d Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 11 Aug 2026 13:34:27 -0700 Subject: [PATCH] FN-8956: track resolved review findings Add durable, scoped resolution states for workflow review findings. - Persist reviewer-applied and superseded finding receipts without making them actionable. - Scope supersession claims to a named prior workflow result, preserving duplicate IDs in other review lanes. - Render informational resolution badges and reject resolved items from revision requests. Files changed: .changeset/fn-8956-review-finding-resolution.md | 7 + docs/dashboard-guide.md | 2 +- docs/workflow-steps.md | 8 +- .../src/__tests__/review-severity-gate.test.ts | 25 ++++ .../src/__tests__/workflow-step-results.test.ts | 45 +++++- packages/core/src/index.gate.ts | 6 + packages/core/src/index.ts | 6 + packages/core/src/types.ts | 2 + packages/core/src/types/task/task-review.ts | 4 + packages/core/src/types/workflow/workflow-steps.ts | 15 +- .../src/workflows/builtin-code-review-group.ts | 2 +- .../src/workflows/builtin-plan-review-group.ts | 2 +- .../core/src/workflows/review-severity-gate.ts | 37 ++++- .../core/src/workflows/workflow-step-results.ts | 54 ++++++- packages/dashboard/app/api/agents/run-audit.ts | 1 + .../dashboard/app/components/TaskReviewTab.css | 22 +++ .../dashboard/app/components/TaskReviewTab.tsx | 36 +++-- .../components/__tests__/TaskReviewTab.test.tsx | 46 ++++++ .../dashboard/src/__tests__/routes-tasks.test.ts | 48 ++++++ .../src/routes/register-task-workflow-routes.ts | 14 +- .../__tests__/review-finding-supersession.test.ts | 163 +++++++++++++++++++++ .../__tests__/review-findings-injection.test.ts | 22 +++ .../workflow-step-verdict-parsing.test.ts | 16 +- .../engine/src/executor/execute-workflow-graph.ts | 129 ++++++++-------- .../engine/src/executor/execute-workflow-step.ts | 25 +++- .../engine/src/executor/run-graph-custom-node.ts | 7 + .../executor/workflow-step-failure-injection.ts | 8 +- .../engine/src/executor/workflow-step-verdict.ts | 17 ++- .../src/workflows/workflow-graph-executor.ts | 15 ++ packages/i18n/locales/en/app.json | 4 +- packages/i18n/locales/es/app.json | 4 +- packages/i18n/locales/fr/app.json | 4 +- packages/i18n/locales/ko/app.json | 4 +- packages/i18n/locales/pt-BR/app.json | 4 +- packages/i18n/locales/zh-CN/app.json | 4 +- packages/i18n/locales/zh-TW/app.json | 4 +- 36 files changed, 703 insertions(+), 109 deletions(-) Fusion-Task-Id: FN-8956 Fusion-Task-Lineage: 80568280-85aa-4a49-a60a-99b75f88f486 Co-authored-by: Fusion (runfusion.ai) --- .../fn-8956-review-finding-resolution.md | 7 + docs/dashboard-guide.md | 2 +- docs/workflow-steps.md | 8 +- .../__tests__/review-severity-gate.test.ts | 25 +++ .../__tests__/workflow-step-results.test.ts | 45 ++++- packages/core/src/index.gate.ts | 6 + packages/core/src/index.ts | 6 + packages/core/src/types.ts | 2 + packages/core/src/types/task/task-review.ts | 4 + .../core/src/types/workflow/workflow-steps.ts | 15 +- .../workflows/builtin-code-review-group.ts | 2 +- .../workflows/builtin-plan-review-group.ts | 2 +- .../src/workflows/review-severity-gate.ts | 35 +++- .../src/workflows/workflow-step-results.ts | 54 +++++- .../dashboard/app/api/agents/run-audit.ts | 1 + .../app/components/TaskReviewTab.css | 22 +++ .../app/components/TaskReviewTab.tsx | 36 ++-- .../__tests__/TaskReviewTab.test.tsx | 46 +++++ .../src/__tests__/routes-tasks.test.ts | 48 ++++++ .../routes/register-task-workflow-routes.ts | 14 +- .../review-finding-supersession.test.ts | 163 ++++++++++++++++++ .../review-findings-injection.test.ts | 22 +++ .../workflow-step-verdict-parsing.test.ts | 16 +- .../src/executor/execute-workflow-graph.ts | 129 +++++++------- .../src/executor/execute-workflow-step.ts | 25 ++- .../src/executor/run-graph-custom-node.ts | 7 + .../workflow-step-failure-injection.ts | 8 +- .../src/executor/workflow-step-verdict.ts | 17 +- .../src/workflows/workflow-graph-executor.ts | 15 ++ packages/i18n/locales/en/app.json | 4 +- packages/i18n/locales/es/app.json | 4 +- packages/i18n/locales/fr/app.json | 4 +- packages/i18n/locales/ko/app.json | 4 +- packages/i18n/locales/pt-BR/app.json | 4 +- packages/i18n/locales/zh-CN/app.json | 4 +- packages/i18n/locales/zh-TW/app.json | 4 +- 36 files changed, 702 insertions(+), 108 deletions(-) create mode 100644 .changeset/fn-8956-review-finding-resolution.md create mode 100644 packages/engine/src/__tests__/review-finding-supersession.test.ts diff --git a/.changeset/fn-8956-review-finding-resolution.md b/.changeset/fn-8956-review-finding-resolution.md new file mode 100644 index 0000000000..f1cfce0f61 --- /dev/null +++ b/.changeset/fn-8956-review-finding-resolution.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Keep resolved review findings visible without allowing no-op revision requests. +category: feature +dev: Adds WorkflowReviewFinding.resolution and prompt/script supersededFindingIds claims persisted at the result sink; resolved findings bypass gate/remediation actions and POST /tasks/:id/review/address rejects them. diff --git a/docs/dashboard-guide.md b/docs/dashboard-guide.md index f2daa2b5fb..632d45f82a 100644 --- a/docs/dashboard-guide.md +++ b/docs/dashboard-guide.md @@ -2417,7 +2417,7 @@ Todo Lists is an optional first-party plugin. Enable `fusion-plugin-todos` for a ## Workflow direct-review items -The Review tab shows a custom workflow result only when its selected workflow declares the exact top-level node and result source, the result explicitly snapshots `reviewKind: "plan"` or `"code"`, and it is current, terminal, and not bypassed or superseded. Each persisted structured finding becomes one independently selectable reviewer-agent item with its server-owned identity, optional location, and severity. Selecting a subset sends only those canonical items for revision; client-supplied text and metadata are ignored. A current result without findings retains one prose/notes fallback item. Pending, skipped, historical prior attempts, blank results, and records without that declared top-level identity (including template instances) are not selectable or addressable. Node-ID punctuation alone does not identify a template instance. Existing historical `plan-review` and `code-review` results retain narrow compatibility; Fusion does not infer or backfill custom review meaning from names, verdicts, prose, or gate settings. +The Review tab shows a custom workflow result only when its selected workflow declares the exact top-level node and result source, the result explicitly snapshots `reviewKind: "plan"` or `"code"`, and it is current, terminal, and not bypassed or superseded. Each persisted open structured finding becomes one independently selectable reviewer-agent item with its server-owned identity, optional location, and severity. Findings marked `resolved-in-review` or `superseded` remain visible as audit-only, badged rows and cannot be selected; the revision route enforces this as well. Selecting a subset sends only those canonical open items for revision; client-supplied text and metadata are ignored. A current result without findings retains one prose/notes fallback item. Pending, skipped, historical prior attempts, blank results, and records without that declared top-level identity (including template instances) are not selectable or addressable. Node-ID punctuation alone does not identify a template instance. Existing historical `plan-review` and `code-review` results retain narrow compatibility; Fusion does not infer or backfill custom review meaning from names, verdicts, prose, or gate settings. ### Workflow agent routing diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 9f97349f13..f8cc99f185 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -785,7 +785,7 @@ The gate **fails closed** and only ever relaxes a verdict: - a `REVISE` carrying any finding that omits `severity` still blocks; - an `APPROVE`/`APPROVE_WITH_NOTES` is never promoted to a block, whatever its findings say. -Set either setting to `any` to restore the previous behavior where every `REVISE` blocks. The built-in Plan Review and Code Review prompts request the structured findings schema, define severity as P0/P1/P2 by consequence, instruct reviewers to omit nits entirely rather than file them as low-severity findings, and use an incremental re-review contract that forbids introducing new non-blocking findings as grounds for another round. Remediation instructions render findings grouped by priority — P0 must fix, P1 fix or explicitly decline with a rationale, P2 optional. +Set either setting to `any` to restore the previous behavior where every `REVISE` blocks. The built-in Plan Review and Code Review prompts request the structured findings schema, define severity as P0/P1/P2 by consequence, instruct reviewers to omit nits entirely rather than file them as low-severity findings, and use an incremental re-review contract that forbids introducing new non-blocking findings as grounds for another round. Remediation instructions render findings grouped by priority — P0 must fix, P1 fix or explicitly decline with a rationale, P2 optional. Findings marked `resolved-in-review` or `superseded` are audit receipts: they never block, become advisory notes, or enter remediation priority lists; remediation shows them only in an explicit do-not-redo block, and the Review tab plus revision route do not allow them to be selected. Remediation now also **preserves the implementation session**: a review bounce keeps `task.sessionFile`, so the next round continues the same conversation instead of re-reading the repository and re-deriving the change it just wrote. The resume prompt directs the agent to re-read PROMPT.md, which is where the new findings were written. Paths that genuinely need a fresh session (context overflow, stale continuation, worktree reacquisition, task-done refusal) still clear the session explicitly at their own site, and the resume guard re-validates the persisted worktree before reopening. @@ -991,9 +991,11 @@ A top-level `prompt`, `gate`, `script`, or `optional-group` node may set `config { "id": "architecture-review", "kind": "prompt", "config": { "name": "Architecture review", "reviewKind": "code", "prompt": "Review the proposed architecture." } } ``` -When a marked supported node runs, its pending and terminal workflow-step result snapshots the declared value. Review-kind prompt and script output may end with one JSON object containing `verdict`, `notes`, and `findings`. Each finding has a stable `id`, actionable `title` and `body`, plus optional `filePath`, positive `line`, and `low`/`medium`/`high`/`critical` severity. Fusion trims and bounds strings, drops malformed entries, and suffixes duplicate IDs; it never splits Markdown prose into findings. +When a marked supported node runs, its pending and terminal workflow-step result snapshots the declared value. Review-kind prompt and declared `reviewKind` script output may end with one JSON object containing `verdict`, `notes`, `findings`, and optional `supersededFindingIds`. Each finding has a stable `id`, actionable `title` and `body`, optional `filePath`, positive `line`, severity, and optional resolution: `resolved-in-review` is a self-fixed receipt and `superseded` is a re-verified stale finding. Absent resolution means open; explicit `open` and invalid resolutions are dropped while the finding remains. Fusion trims and bounds strings, drops malformed entries, and suffixes duplicate IDs; it never splits Markdown prose into findings. -Findings persist through both ordinary-node and optional-group result writers in the existing JSONB result. A retry moves the replaced result (including its findings) into bounded single-level `priorAttempts`; only current findings are actionable. Findings are advisory metadata: they do not alter verdict parsing, gate status, merge blocking, recovery, or retry routing. A row without findings keeps its one prose/notes fallback item. Omission means the node is **not** a direct review, regardless of its ID, label, verdict, output prose, phase, or gate mode. Markers are rejected on foreach and loop templates and optional-group source/template nodes: those executions do not yet have an instance-safe current-result or Review-tab address contract. +Review prompts list earlier open finding IDs. A later reviewer may claim only those IDs in `supersededFindingIds`; the audit-only list is carried on `WorkflowStepResult` and applied to earlier persisted findings at the engine result sink. This works for prompt and declared review scripts, while unmarked scripts cannot emit review metadata. Supersession is always an explicit reviewer claim: Fusion never infers it from commit timestamps. + +Findings persist through both ordinary-node and optional-group result writers in the existing JSONB result. A retry moves the replaced result (including its findings) into bounded single-level `priorAttempts`; only current findings are actionable. Findings do not alter verdict parsing, merge blocking, recovery, or retry routing. Open findings continue through the severity gate and advisory/remediation paths; resolved receipts are excluded from those actionable paths without rewriting an explicit `REVISE` verdict. A row without findings keeps its one prose/notes fallback item. Omission means the node is **not** a direct review, regardless of its ID, label, verdict, output prose, phase, or gate mode. Markers are rejected on foreach and loop templates and optional-group source/template nodes: those executions do not yet have an instance-safe current-result or Review-tab address contract. ## Durable workflow principals diff --git a/packages/core/src/__tests__/review-severity-gate.test.ts b/packages/core/src/__tests__/review-severity-gate.test.ts index bccf379c91..c816821459 100644 --- a/packages/core/src/__tests__/review-severity-gate.test.ts +++ b/packages/core/src/__tests__/review-severity-gate.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { applyReviewSeverityGate, formatFindingsByPriority, + formatResolvedFindings, isBlockingFinding, resolveReviewBlockingSeverity, DEFAULT_CODE_REVIEW_BLOCKING_SEVERITY, @@ -65,6 +66,13 @@ describe("isBlockingFinding", () => { it("blocks everything at threshold \"any\"", () => { expect(isBlockingFinding(finding({ severity: "low" }), "any")).toBe(true); }); + + it("never blocks review receipts or superseded findings", () => { + for (const resolution of ["resolved-in-review", "superseded"] as const) { + expect(isBlockingFinding(finding({ severity: "critical", resolution }), "critical")).toBe(false); + expect(isBlockingFinding(finding({ severity: "critical", resolution }), "any")).toBe(false); + } + }); }); describe("applyReviewSeverityGate", () => { @@ -133,6 +141,16 @@ describe("applyReviewSeverityGate", () => { expect(result.downgraded).toBe(false); }); + it("keeps an all-resolved REVISE fail-closed while exposing only audit receipts", () => { + const result = applyReviewSeverityGate({ + verdict: "REVISE", + findings: [finding({ id: "receipt", severity: "critical", resolution: "resolved-in-review" })], + threshold: "any", + }); + expect(result).toMatchObject({ verdict: "REVISE", downgraded: false, blocking: [], advisory: [] }); + expect(result.resolved.map((item) => item.id)).toEqual(["receipt"]); + }); + it("only ever relaxes — an APPROVE carrying a critical finding is left alone", () => { for (const verdict of ["APPROVE", "APPROVE_WITH_NOTES", "CLOSE_NO_OP", undefined]) { const result = applyReviewSeverityGate({ @@ -172,4 +190,11 @@ describe("formatFindingsByPriority", () => { const out = formatFindingsByPriority([finding({ title: "unknown", body: "x" })]); expect(out).toContain("### Unclassified — treat as must fix"); }); + + it("omits non-open findings from priorities and renders audit receipts separately", () => { + const receipt = finding({ id: "receipt", title: "Fixed", body: "Already handled", resolution: "resolved-in-review" }); + expect(formatFindingsByPriority([receipt])).toBe(""); + expect(formatResolvedFindings([receipt])).toContain("Already resolved during this review pass — do NOT redo"); + expect(formatResolvedFindings([receipt])).toContain("[resolved-in-review]"); + }); }); diff --git a/packages/core/src/__tests__/workflow-step-results.test.ts b/packages/core/src/__tests__/workflow-step-results.test.ts index 282da83a00..1fdef952b3 100644 --- a/packages/core/src/__tests__/workflow-step-results.test.ts +++ b/packages/core/src/__tests__/workflow-step-results.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { normalizeWorkflowReviewFindings, upsertWorkflowStepResult, MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS } from "../workflows/workflow-step-results.js"; +import { applySupersededFindingIds, MAX_WORKFLOW_REVIEW_FINDINGS, MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS, normalizeSupersededFindingIds, normalizeWorkflowReviewFindings, upsertWorkflowStepResult } from "../workflows/workflow-step-results.js"; import type { WorkflowStepResult } from "../types.js"; function makeResult(overrides: Partial = {}): WorkflowStepResult { @@ -31,6 +31,49 @@ describe("normalizeWorkflowReviewFindings", () => { { title: "title", body: "x".repeat(4001) }, ])).toBeUndefined(); }); + + it("preserves valid non-open resolutions but normalizes open and invalid values away", () => { + expect(normalizeWorkflowReviewFindings([ + { id: "receipt", title: "Receipt", body: "Fixed", resolution: "resolved-in-review" }, + { id: "stale", title: "Stale", body: "Fixed elsewhere", resolution: "superseded" }, + { id: "open", title: "Open", body: "Fix", resolution: "open" }, + { id: "invalid", title: "Invalid", body: "Still valid", resolution: "fixed" }, + { id: "null", title: "Null", body: "Still valid", resolution: null }, + ])).toEqual([ + { id: "receipt", title: "Receipt", body: "Fixed", resolution: "resolved-in-review" }, + { id: "stale", title: "Stale", body: "Fixed elsewhere", resolution: "superseded" }, + { id: "open", title: "Open", body: "Fix" }, + { id: "invalid", title: "Invalid", body: "Still valid" }, + { id: "null", title: "Null", body: "Still valid" }, + ]); + }); +}); + +describe("superseded finding claims", () => { + it("normalizes bounded, deduplicated string ids", () => { + expect(normalizeSupersededFindingIds([" c1 ", 4, "c1", "", "c2"])).toEqual(["c1", "c2"]); + expect(normalizeSupersededFindingIds(Array.from({ length: MAX_WORKFLOW_REVIEW_FINDINGS + 1 }, (_, index) => `f${index}`))).toHaveLength(MAX_WORKFLOW_REVIEW_FINDINGS); + expect(normalizeSupersededFindingIds({})).toBeUndefined(); + }); + + it("stamps only unresolved findings outside the claiming result", () => { + const prior = makeResult({ workflowStepId: "cleanup", findings: [ + { id: "c1", title: "Open", body: "Fix" }, + { id: "receipt", title: "Receipt", body: "Fixed", resolution: "resolved-in-review" }, + ], priorAttempts: [{ ...makeResult({ workflowStepId: "cleanup", findings: [{ id: "c1", title: "Old", body: "Old" }] }) }] }); + const claimant = makeResult({ workflowStepId: "code", findings: [{ id: "c1", title: "Own", body: "Own" }] }); + const next = applySupersededFindingIds([prior, claimant], ["c1", "receipt"], { excludeWorkflowStepId: "code", sourceWorkflowStepId: "cleanup" }); + expect(next?.[0].findings).toEqual([ + { id: "c1", title: "Open", body: "Fix", resolution: "superseded" }, + { id: "receipt", title: "Receipt", body: "Fixed", resolution: "resolved-in-review" }, + ]); + const unrelated = makeResult({ workflowStepId: "other-review", findings: [{ id: "c1", title: "Different lane", body: "Must remain open" }] }); + const scoped = applySupersededFindingIds([prior, unrelated, claimant], ["c1"], { excludeWorkflowStepId: "code", sourceWorkflowStepId: "cleanup" }); + expect(scoped?.[1].findings?.[0]).not.toHaveProperty("resolution"); + expect(next?.[0].priorAttempts).toEqual(prior.priorAttempts); + expect(next?.[1]).toBe(claimant); + expect(applySupersededFindingIds(next, ["missing"], { excludeWorkflowStepId: "code", sourceWorkflowStepId: "cleanup" })).toBe(next); + }); }); describe("upsertWorkflowStepResult", () => { diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index ca8101b670..9ff9c6f04c 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -569,6 +569,7 @@ THIS file, so a gate export present only in index.ts resolves to `undefined` und export { applyReviewSeverityGate, formatFindingsByPriority, + formatResolvedFindings, isBlockingFinding, isReviewBlockingSeverity, resolveReviewBlockingSeverity, @@ -2323,8 +2324,13 @@ export { upsertWorkflowStepResult, normalizeWorkflowReviewFindings, isWorkflowReviewFindingSeverity, + isWorkflowReviewFindingResolution, + isOpenWorkflowReviewFinding, + normalizeSupersededFindingIds, + applySupersededFindingIds, MAX_WORKFLOW_REVIEW_FINDINGS, WORKFLOW_REVIEW_FINDING_SEVERITIES, + WORKFLOW_REVIEW_FINDING_RESOLUTIONS, MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS, PLAN_REVIEW_LEASE_STALENESS_MS, classifyReviewLease, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0bff1fd539..6ff944a23f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -664,6 +664,7 @@ export { export { applyReviewSeverityGate, formatFindingsByPriority, + formatResolvedFindings, isBlockingFinding, isReviewBlockingSeverity, resolveReviewBlockingSeverity, @@ -2755,8 +2756,13 @@ export { upsertWorkflowStepResult, normalizeWorkflowReviewFindings, isWorkflowReviewFindingSeverity, + isWorkflowReviewFindingResolution, + isOpenWorkflowReviewFinding, + normalizeSupersededFindingIds, + applySupersededFindingIds, MAX_WORKFLOW_REVIEW_FINDINGS, WORKFLOW_REVIEW_FINDING_SEVERITIES, + WORKFLOW_REVIEW_FINDING_RESOLUTIONS, MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS, PLAN_REVIEW_LEASE_STALENESS_MS, classifyReviewLease, diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 2f7b4da13e..5893d7c9bc 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -273,6 +273,7 @@ import type { WorkflowStepPhase, WorkflowReviewKind, WorkflowReviewFindingSeverity, + WorkflowReviewFindingResolution, WorkflowReviewFinding, WorkflowStep, NtfyNotificationEvent, @@ -295,6 +296,7 @@ export type { WorkflowStepPhase, WorkflowReviewKind, WorkflowReviewFindingSeverity, + WorkflowReviewFindingResolution, WorkflowReviewFinding, WorkflowStep, NtfyNotificationEvent, diff --git a/packages/core/src/types/task/task-review.ts b/packages/core/src/types/task/task-review.ts index 69e9c295ba..7dd62ec7de 100644 --- a/packages/core/src/types/task/task-review.ts +++ b/packages/core/src/types/task/task-review.ts @@ -15,6 +15,7 @@ export type TaskReviewVerdict = "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "R export type TaskReviewerType = "plan" | "code"; export type TaskReviewItemStatus = "queued" | "in-progress" | "addressed" | "failed"; export type TaskReviewFindingSeverity = "low" | "medium" | "high" | "critical"; +export type TaskReviewFindingResolution = "open" | "resolved-in-review" | "superseded"; export interface LegacyTaskReviewItem { id: string; @@ -100,6 +101,7 @@ export interface TaskReviewStateItem { step?: number; summary?: string; severity?: TaskReviewFindingSeverity; + resolution?: TaskReviewFindingResolution; } export type ReviewAddressingStatus = "queued" | "in-progress" | "addressed" | "failed"; @@ -114,6 +116,7 @@ export interface ReviewAddressingSnapshot { filePath?: string; lineNumber?: number; severity?: TaskReviewFindingSeverity; + resolution?: TaskReviewFindingResolution; threadId?: string; url?: string; } @@ -171,6 +174,7 @@ export interface TaskReviewDataItem { filePath?: string; line?: number; severity?: TaskReviewFindingSeverity; + resolution?: TaskReviewFindingResolution; threadId?: string; reviewState?: string | null; /** Machine-readable reviewer verdict when the source supplied one. */ diff --git a/packages/core/src/types/workflow/workflow-steps.ts b/packages/core/src/types/workflow/workflow-steps.ts index 83c32f6d88..05429f2b38 100644 --- a/packages/core/src/types/workflow/workflow-steps.ts +++ b/packages/core/src/types/workflow/workflow-steps.ts @@ -33,12 +33,14 @@ export type WorkflowStepGateMode = "gate" | "advisory"; /** Closed severity vocabulary shared by persisted workflow findings and Review-tab items. */ export type WorkflowReviewFindingSeverity = "low" | "medium" | "high" | "critical"; +export type WorkflowReviewFindingResolution = "open" | "resolved-in-review" | "superseded"; /** - * FNXC:WorkflowReviewFindings 2026-08-05-06:29: - * Review-kind nodes persist independently actionable feedback in the existing JSONB result so - * Review-tab selection never depends on model prose or client-provided metadata. Finding identity - * is normalized before persistence; historical prose-only rows intentionally omit this field. + * FNXC:WorkflowReviewFindings 2026-08-11-19:39: + * Absent resolution means open and is never persisted as `"open"`. An explicit reviewer claim + * marks a receipt as resolved-in-review or a prior finding as superseded; Fusion never infers this + * from commit timestamps. This lets already-handled findings remain auditable without becoming + * actionable Review-tab revision items. */ export interface WorkflowReviewFinding { id: string; @@ -47,6 +49,7 @@ export interface WorkflowReviewFinding { filePath?: string; line?: number; severity?: WorkflowReviewFindingSeverity; + resolution?: WorkflowReviewFindingResolution; } /** Lifecycle phase for workflow step execution. */ @@ -277,6 +280,10 @@ export interface WorkflowStepResult { output?: string; /** Normalized structured advisory findings from an explicitly classified review node. */ findings?: WorkflowReviewFinding[]; + /** Prior result containing the finding IDs this review step explicitly declared superseded. */ + supersededFindingSourceWorkflowStepId?: string; + /** Prior-lane finding IDs this review step explicitly declared superseded; audit-only. */ + supersededFindingIds?: string[]; /** * Machine-readable verdict from prompt-mode structured output. * Absent for script-mode steps and legacy prose-only prompt outputs. diff --git a/packages/core/src/workflows/builtin-code-review-group.ts b/packages/core/src/workflows/builtin-code-review-group.ts index 8074769025..f5fd893fd4 100644 --- a/packages/core/src/workflows/builtin-code-review-group.ts +++ b/packages/core/src/workflows/builtin-code-review-group.ts @@ -82,7 +82,7 @@ Be specific: cite \`file:line\` for every finding and explain the concrete failu - REVISE: a correctness bug, regression, or contract break requires changes before merge. Requires at least one \`critical\` finding in \`findings\`; a REVISE without one will be treated as APPROVE_WITH_NOTES. - Every blocking issue MUST appear as an entry in \`findings\` with its severity and \`filePath\`/\`line\`. Prose in \`notes\` alone does not block. - Final output: output exactly one trailing JSON object on the final line (no markdown fences, no surrounding prose): -{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"...","findings":[{"id":"stable-id","title":"concise issue","body":"concrete failure and remediation","filePath":"path/to/file.ts","line":1,"severity":"critical|high|medium|low"}]}`; +{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"...","findings":[{"id":"stable-id","title":"concise issue","body":"concrete failure and remediation","filePath":"path/to/file.ts","line":1,"severity":"critical|high|medium|low","resolution":"open|resolved-in-review|superseded"}]}`; /** * Build the `code-review` optional-group node placed on a workflow's pre-merge path. diff --git a/packages/core/src/workflows/builtin-plan-review-group.ts b/packages/core/src/workflows/builtin-plan-review-group.ts index 4956e6a5ae..cd163aea8d 100644 --- a/packages/core/src/workflows/builtin-plan-review-group.ts +++ b/packages/core/src/workflows/builtin-plan-review-group.ts @@ -46,7 +46,7 @@ Be specific: cite the plan section or file path for every finding and explain th - CLOSE_NO_OP: implementation must not proceed because the premise is stale, the work is already satisfied, redundant, or a duplicate. The notes field MUST begin with exactly one existing completion sentinel: PREMISE STALE:, NO-OP:, NOOP:, REDUNDANT:, or DUPLICATE:. For duplicates, use DUPLICATE: FN-NNNN ... when the canonical task is known. - Every blocking issue MUST appear as an entry in \`findings\` with its severity. Prose in \`notes\` alone does not block, and is not a durable input to the next planning round. - Final output: output exactly one trailing JSON object on the final line (no markdown fences, no surrounding prose): -{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE|CLOSE_NO_OP","notes":"...","findings":[{"id":"stable-id","title":"concise issue","body":"actionable correction","severity":"critical|high|medium|low"}]}`; +{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE|CLOSE_NO_OP","notes":"...","findings":[{"id":"stable-id","title":"concise issue","body":"actionable correction","severity":"critical|high|medium|low","resolution":"open|resolved-in-review|superseded"}]}`; /* FNXC:PlanReviewStep 2026-07-27-06:10: diff --git a/packages/core/src/workflows/review-severity-gate.ts b/packages/core/src/workflows/review-severity-gate.ts index afa2e43ea6..ec77a48768 100644 --- a/packages/core/src/workflows/review-severity-gate.ts +++ b/packages/core/src/workflows/review-severity-gate.ts @@ -24,6 +24,7 @@ every review that does not opt into the structured contract. */ import type { WorkflowReviewFinding, WorkflowReviewFindingSeverity, WorkflowReviewKind } from "../types.js"; +import { isOpenWorkflowReviewFinding } from "./workflow-step-results.js"; /** * Blocking threshold for a review gate. A severity value blocks at that level and above; @@ -108,6 +109,7 @@ export function resolveReviewBlockingSeverity({ * An UNCLASSIFIED finding always blocks — see the fail-closed contract in the module header. */ export function isBlockingFinding(finding: WorkflowReviewFinding, threshold: ReviewBlockingSeverity): boolean { + if (!isOpenWorkflowReviewFinding(finding)) return false; if (threshold === "any") return true; if (!finding.severity) return true; return SEVERITY_RANK[finding.severity] >= SEVERITY_RANK[threshold]; @@ -128,6 +130,8 @@ export interface ReviewSeverityGateResult { blocking: WorkflowReviewFinding[]; /** Findings below the threshold. Still persisted and still handed to the implementer. */ advisory: WorkflowReviewFinding[]; + /** Audit-only receipts and superseded findings; never actionable. */ + resolved: WorkflowReviewFinding[]; } /** @@ -139,15 +143,22 @@ export interface ReviewSeverityGateResult { */ export function applyReviewSeverityGate({ verdict, findings, threshold }: ReviewSeverityGateInput): ReviewSeverityGateResult { const all = findings ?? []; - const blocking = all.filter((finding) => isBlockingFinding(finding, threshold)); - const advisory = all.filter((finding) => !isBlockingFinding(finding, threshold)); + const open = all.filter(isOpenWorkflowReviewFinding); + const resolved = all.filter((finding) => !isOpenWorkflowReviewFinding(finding)); + const blocking = open.filter((finding) => isBlockingFinding(finding, threshold)); + const advisory = open.filter((finding) => !isBlockingFinding(finding, threshold)); - if (verdict !== "REVISE") return { verdict, downgraded: false, blocking, advisory }; + if (verdict !== "REVISE") return { verdict, downgraded: false, blocking, advisory, resolved }; // Fail closed: an unstructured REVISE keeps its blocking power. - if (all.length === 0) return { verdict, downgraded: false, blocking, advisory }; - if (blocking.length > 0) return { verdict, downgraded: false, blocking, advisory }; + /* + FNXC:ReviewSeverityGate 2026-08-11-19:39: + Resolution is audit metadata, never authority to rewrite an explicit REVISE. Receipts avoid + no-op rework through remediation's do-not-redo block while an all-resolved REVISE stays fail-closed. + */ + if (all.length === 0 || (open.length === 0 && resolved.length > 0)) return { verdict, downgraded: false, blocking, advisory, resolved }; + if (blocking.length > 0) return { verdict, downgraded: false, blocking, advisory, resolved }; - return { verdict: "APPROVE_WITH_NOTES", downgraded: true, blocking, advisory }; + return { verdict: "APPROVE_WITH_NOTES", downgraded: true, blocking, advisory, resolved }; } /** @@ -157,6 +168,7 @@ export function applyReviewSeverityGate({ verdict, findings, threshold }: Review * consistent shape whether the review blocked or was downgraded. */ export function formatFindingsByPriority(findings: WorkflowReviewFinding[]): string { + findings = findings.filter(isOpenWorkflowReviewFinding); if (findings.length === 0) return ""; const groups: Array<{ label: string; obligation: string; severities: WorkflowReviewFindingSeverity[] }> = [ { label: "P0 — must fix", obligation: "Fix every item in this group before returning.", severities: ["critical"] }, @@ -192,3 +204,14 @@ export function formatFindingsByPriority(findings: WorkflowReviewFinding[]): str return sections.join("\n\n"); } + +/** Render audit receipts separately so implementers do not redo completed review work. */ +export function formatResolvedFindings(findings: WorkflowReviewFinding[]): string { + const resolved = findings.filter((finding) => !isOpenWorkflowReviewFinding(finding)); + if (resolved.length === 0) return ""; + const lines = resolved.map((finding) => { + const location = finding.filePath ? ` (${finding.filePath}${finding.line ? `:${finding.line}` : ""})` : ""; + return `- **${finding.title}**${location} [${finding.resolution}]\n ${finding.body}`; + }); + return `### Already resolved during this review pass — do NOT redo\n\n${lines.join("\n")}`; +} diff --git a/packages/core/src/workflows/workflow-step-results.ts b/packages/core/src/workflows/workflow-step-results.ts index 80e0f579df..8ae2d51ee3 100644 --- a/packages/core/src/workflows/workflow-step-results.ts +++ b/packages/core/src/workflows/workflow-step-results.ts @@ -1,6 +1,7 @@ -import type { WorkflowReviewFinding, WorkflowReviewFindingSeverity, WorkflowStepResult } from "../types.js"; +import type { WorkflowReviewFinding, WorkflowReviewFindingResolution, WorkflowReviewFindingSeverity, WorkflowStepResult } from "../types.js"; export const WORKFLOW_REVIEW_FINDING_SEVERITIES = ["low", "medium", "high", "critical"] as const; +export const WORKFLOW_REVIEW_FINDING_RESOLUTIONS = ["open", "resolved-in-review", "superseded"] as const; export const MAX_WORKFLOW_REVIEW_FINDINGS = 20; const MAX_FINDING_ID_LENGTH = 128; const MAX_FINDING_TITLE_LENGTH = 240; @@ -33,7 +34,10 @@ export function normalizeWorkflowReviewFindings(raw: unknown): WorkflowReviewFin ? Math.floor(value.line) : undefined; const severity = isWorkflowReviewFindingSeverity(value.severity) ? value.severity : undefined; - normalized.push({ id, title, body, ...(filePath ? { filePath } : {}), ...(line ? { line } : {}), ...(severity ? { severity } : {}) }); + const resolution = isWorkflowReviewFindingResolution(value.resolution) && value.resolution !== "open" + ? value.resolution + : undefined; + normalized.push({ id, title, body, ...(filePath ? { filePath } : {}), ...(line ? { line } : {}), ...(severity ? { severity } : {}), ...(resolution ? { resolution } : {}) }); } return normalized.length > 0 ? normalized : undefined; } @@ -48,6 +52,52 @@ export function isWorkflowReviewFindingSeverity(value: unknown): value is Workfl return typeof value === "string" && (WORKFLOW_REVIEW_FINDING_SEVERITIES as readonly string[]).includes(value); } +export function isWorkflowReviewFindingResolution(value: unknown): value is WorkflowReviewFindingResolution { + return typeof value === "string" && (WORKFLOW_REVIEW_FINDING_RESOLUTIONS as readonly string[]).includes(value); +} + +/** Historical findings and explicit `open` findings remain actionable. */ +export function isOpenWorkflowReviewFinding(finding: WorkflowReviewFinding): boolean { + return finding.resolution === undefined || finding.resolution === "open"; +} + +/** Normalize untrusted reviewer claims before they can target persisted prior-lane findings. */ +export function normalizeSupersededFindingIds(raw: unknown): string[] | undefined { + if (!Array.isArray(raw)) return undefined; + const ids = [...new Set(raw + .map((value) => boundedTrimmedString(value, MAX_FINDING_ID_LENGTH)) + .filter((value): value is string => value !== undefined))] + .slice(0, MAX_WORKFLOW_REVIEW_FINDINGS); + return ids.length > 0 ? ids : undefined; +} + +/** + * Apply a later review step's explicit supersession claim only to its named prior result, + * preserving other lanes that may use the same finding IDs. + */ +export function applySupersededFindingIds( + results: WorkflowStepResult[] | undefined, + ids: string[], + options: { excludeWorkflowStepId: string; sourceWorkflowStepId: string }, +): WorkflowStepResult[] | undefined { + if (!results || ids.length === 0 || !options.sourceWorkflowStepId) return results; + const claimed = new Set(ids); + let changed = false; + const next = results.map((result) => { + if (result.workflowStepId !== options.sourceWorkflowStepId || result.workflowStepId === options.excludeWorkflowStepId || !result.findings?.length) return result; + let findingsChanged = false; + const findings = result.findings.map((finding) => { + if (!claimed.has(finding.id) || !isOpenWorkflowReviewFinding(finding)) return finding; + findingsChanged = true; + return { ...finding, resolution: "superseded" as const }; + }); + if (!findingsChanged) return result; + changed = true; + return { ...result, findings }; + }); + return changed ? next : results; +} + /* FNXC:WorkflowStepResults 2026-07-09-00:20: FN-7727: both engine `WorkflowStepResult` recorders (the executor graph adapter's diff --git a/packages/dashboard/app/api/agents/run-audit.ts b/packages/dashboard/app/api/agents/run-audit.ts index 842863f873..fcb3ee6830 100644 --- a/packages/dashboard/app/api/agents/run-audit.ts +++ b/packages/dashboard/app/api/agents/run-audit.ts @@ -259,6 +259,7 @@ function mapTaskReviewDataToLegacy(data: TaskReviewData): TaskReviewResponse { state: item.reviewState ?? undefined, verdict: item.verdict, reviewType: item.reviewType, + resolution: item.resolution, summary: item.title ?? undefined, isResolved: item.isResolved, ...(typeof item.line === "number" ? { line: item.line } : {}), diff --git a/packages/dashboard/app/components/TaskReviewTab.css b/packages/dashboard/app/components/TaskReviewTab.css index d26e3831b5..414ae07ce9 100644 --- a/packages/dashboard/app/components/TaskReviewTab.css +++ b/packages/dashboard/app/components/TaskReviewTab.css @@ -538,3 +538,25 @@ PR-linked tasks must expose decision-adjacent reviewers/checks/blockers plus ite min-height: calc(var(--space-lg) * 2 + var(--space-xs)); } } + +.task-review-tab__item--informational { + border-color: color-mix(in srgb, var(--color-success) 30%, var(--border)); +} + +.task-review-tab__resolution-badge { + display: inline-flex; + align-items: center; + border: var(--btn-border-width) solid color-mix(in srgb, var(--color-success) 28%, transparent); + border-radius: var(--radius-pill); + background: color-mix(in srgb, var(--color-success) 14%, transparent); + color: var(--color-success); + font-size: var(--font-size-xs); + font-weight: 600; + padding: 0 var(--space-sm); +} + +@media (max-width: 768px) { + .task-review-tab__resolution-badge { + align-self: flex-start; + } +} diff --git a/packages/dashboard/app/components/TaskReviewTab.tsx b/packages/dashboard/app/components/TaskReviewTab.tsx index a2562fdfb1..4e9c9b1493 100644 --- a/packages/dashboard/app/components/TaskReviewTab.tsx +++ b/packages/dashboard/app/components/TaskReviewTab.tsx @@ -56,6 +56,7 @@ type DisplayReviewItem = { path?: string; line?: number; severity?: "low" | "medium" | "high" | "critical"; + resolution?: "open" | "resolved-in-review" | "superseded"; createdAt?: string; status: "queued" | "in-progress" | "addressed" | "failed"; addressing?: AddressingRecord; @@ -135,6 +136,7 @@ function getDisplayReviewItems(review: ReviewState): DisplayReviewItem[] { path: item.path, line: item.line, severity: item.severity, + resolution: item.resolution, createdAt: item.createdAt, status: addressing?.status ?? "queued", addressing, @@ -212,7 +214,7 @@ export function TaskReviewTab({ return authorTypeFilter === "bot" ? authorInfo.authorIsBot : !authorInfo.authorIsBot; }); }, [authorTypeFilter, displayItems]); - const visibleItemIds = useMemo(() => new Set(filteredDisplayItems.map((item) => item.id)), [filteredDisplayItems]); + const visibleItemIds = useMemo(() => new Set(filteredDisplayItems.filter((item) => !item.resolution || item.resolution === "open").map((item) => item.id)), [filteredDisplayItems]); const canRevise = selected.length > 0 && !revising; const canAddressPrFeedback = isPrMode && Boolean(getTaskPrimaryPrInfo(task)) @@ -286,7 +288,10 @@ export function TaskReviewTab({ ? "status-dot status-dot--pending" : "status-dot status-dot--online"; - const toggleSelected = (id: string) => setSelected((prev) => (prev.includes(id) ? prev.filter((v) => v !== id) : [...prev, id])); + const toggleSelected = (id: string) => { + if (displayItems.some((item) => item.id === id && item.resolution && item.resolution !== "open")) return; + setSelected((prev) => (prev.includes(id) ? prev.filter((value) => value !== id) : [...prev, id])); + }; const onRefresh = async () => { try { @@ -567,19 +572,30 @@ export function TaskReviewTab({ const prState = isPrMode ? item.item?.state : undefined; const prUrl = isPrMode ? item.item?.htmlUrl ?? item.addressing?.snapshot?.url : undefined; const summaryPrefix = item.path && !isPrMode ? `${item.path}: ` : ""; + const isOpen = !item.resolution || item.resolution === "open"; + const resolutionLabel = item.resolution === "resolved-in-review" + ? t("taskReview.fixedInReview", "Fixed in review") + : t("taskReview.superseded", "Superseded"); return ( -
  • +
  • -