diff --git a/.changeset/fn-8793-workflow-review-items.md b/.changeset/fn-8793-workflow-review-items.md new file mode 100644 index 0000000000..492cc11c72 --- /dev/null +++ b/.changeset/fn-8793-workflow-review-items.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Restore workflow review feedback selection for same-task revisions. +category: fix +dev: Canonical workflow-step review items now prevent forged client feedback from reaching snapshots or steering. diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index 4165e8645f..46af466782 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -172,7 +172,7 @@ export { type ResolveAgentMemoryInclusionModeInput, type ResolvedAgentMemoryInclusionMode, } from "./agents/agent-memory-mode.js"; -export type { TaskReviewData, TaskReviewSummary, TaskReviewItem } from "./types.js"; +export type { TaskReviewData, TaskReviewSummary, TaskReviewItem, TaskReviewVerdict, TaskReviewerType } from "./types.js"; export type { TaskCommitAssociation, TaskCommitAssociationConfidence, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 895a831c41..3a37d6d7da 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -198,7 +198,7 @@ export { type ResolveAgentMemoryInclusionModeInput, type ResolvedAgentMemoryInclusionMode, } from "./agents/agent-memory-mode.js"; -export type { TaskReviewData, TaskReviewSummary, TaskReviewItem } from "./types.js"; +export type { TaskReviewData, TaskReviewSummary, TaskReviewItem, TaskReviewVerdict, TaskReviewerType } from "./types.js"; /* FNXC:TaskVerificationRequest 2026-07-30-00:00: FN-8296 makes the persisted verification read model available to dashboard task and Command Center surfaces without exporting a subprocess runner. */ export type { TaskVerificationRequest, TaskVerificationResultSummary, TaskVerificationStatus, TaskVerificationProfile } from "./types.js"; export type { diff --git a/packages/core/src/types/task/task-review.ts b/packages/core/src/types/task/task-review.ts index a1a4485c46..541cbf1b66 100644 --- a/packages/core/src/types/task/task-review.ts +++ b/packages/core/src/types/task/task-review.ts @@ -6,7 +6,7 @@ export type TaskReviewMode = "pull-request" | "direct"; export type TaskReviewSource = "github-pr" | "reviewer-agent"; export type TaskReviewDecision = "approved" | "changes-requested" | "commented" | "pending"; -export type TaskReviewVerdict = "APPROVE" | "REVISE" | "RETHINK" | "UNAVAILABLE"; +export type TaskReviewVerdict = "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | "RETHINK" | "UNAVAILABLE"; export type TaskReviewerType = "plan" | "code"; export type TaskReviewItemStatus = "queued" | "in-progress" | "addressed" | "failed"; @@ -163,6 +163,10 @@ export interface TaskReviewDataItem { line?: number; threadId?: string; reviewState?: string | null; + /** Machine-readable reviewer verdict when the source supplied one. */ + verdict?: TaskReviewVerdict; + /** Review lane that produced the item when known. */ + reviewType?: TaskReviewerType; isResolved?: boolean; progressStatus?: "queued" | "in-progress" | "addressed" | "failed" | null; } diff --git a/packages/dashboard/app/api/agents/run-audit.ts b/packages/dashboard/app/api/agents/run-audit.ts index b06e8b4278..790e2bd66d 100644 --- a/packages/dashboard/app/api/agents/run-audit.ts +++ b/packages/dashboard/app/api/agents/run-audit.ts @@ -257,6 +257,8 @@ function mapTaskReviewDataToLegacy(data: TaskReviewData): TaskReviewResponse { threadId: item.threadId, htmlUrl: item.url, state: item.reviewState ?? undefined, + verdict: item.verdict, + reviewType: item.reviewType, summary: item.title ?? undefined, isResolved: item.isResolved, ...(typeof item.line === "number" ? { line: item.line } : {}), diff --git a/packages/dashboard/src/__tests__/routes-tasks.test.ts b/packages/dashboard/src/__tests__/routes-tasks.test.ts index e6d852d1f6..523711c43d 100644 --- a/packages/dashboard/src/__tests__/routes-tasks.test.ts +++ b/packages/dashboard/src/__tests__/routes-tasks.test.ts @@ -2749,6 +2749,65 @@ describe("POST /tasks/:id/review/address", () => { }); } + it("normalizes current workflow review results into stable canonical items", async () => { + const taskWithWorkflowReviews = { + ...FAKE_TASK_DETAIL, + id: "FN-009", + updatedAt: "2026-01-01T00:00:00.000Z", + workflowStepResults: [ + { workflowStepId: "code-review", workflowStepName: "Code Review", phase: "pre-merge", status: "passed", verdict: "APPROVE_WITH_NOTES", output: "1. Keep the assertion focused.", completedAt: "2026-01-02T00:00:00.000Z" }, + { workflowStepId: "plan-review", workflowStepName: "Plan Review", phase: "pre-merge", status: "failed", verdict: "REVISE", notes: "Clarify the rollback plan.", startedAt: "2026-01-03T00:00:00.000Z" }, + { workflowStepId: "code-review", workflowStepName: "Code Review", phase: "pre-merge", status: "advisory_failure", verdict: "APPROVE", completedAt: "2026-01-04T00:00:00.000Z" }, + { workflowStepId: "custom-review", workflowStepName: "Custom Review", status: "passed", verdict: "REVISE", output: "Must not be inferred." }, + { workflowStepId: "code-review", workflowStepName: "Code Review", status: "pending", verdict: "REVISE" }, + { workflowStepId: "plan-review", workflowStepName: "Plan Review", status: "skipped", verdict: "REVISE" }, + { workflowStepId: "code-review", workflowStepName: "Code Review", status: "passed", verdict: "REVISE", supersededAt: "2026-01-05T00:00:00.000Z" }, + { workflowStepId: "plan-review", workflowStepName: "Plan Review", status: "failed", verdict: "REVISE", bypassedAt: "2026-01-05T00:00:00.000Z" }, + ], + log: [{ timestamp: reviewerBlockTimestamp, action: "code review Step 1: REVISE - legacy fallback must not duplicate structured data" }], + }; + (store.getTask as ReturnType).mockResolvedValue(taskWithWorkflowReviews); + (store.getAgentLogs as ReturnType).mockResolvedValue([{ agent: "reviewer", type: "text", text: "## Code Review:\\n### Verdict: REVISE" }]); + + const first = await REQUEST(buildApp(), "GET", "/api/tasks/FN-009/review"); + const refreshed = await REQUEST(buildApp(), "POST", "/api/tasks/FN-009/review/refresh"); + + expect(first.status).toBe(200); + expect(refreshed.status).toBe(200); + expect(first.body.items).toHaveLength(3); + expect(first.body.items).toEqual(expect.arrayContaining([ + expect.objectContaining({ title: "Code Review APPROVE_WITH_NOTES", body: "1. Keep the assertion focused.", verdict: "APPROVE_WITH_NOTES", reviewType: "code" }), + expect.objectContaining({ title: "Plan Review REVISE", body: "Clarify the rollback plan.", verdict: "REVISE", reviewType: "plan" }), + expect.objectContaining({ verdict: "APPROVE", body: "No written feedback was provided by this review step." }), + ])); + expect(first.body.summary).toEqual(expect.objectContaining({ verdict: "APPROVE" })); + expect(first.body.items.map((item: { itemId: string }) => item.itemId)).toEqual(refreshed.body.items.map((item: { itemId: string }) => item.itemId)); + expect(store.getAgentLogs).not.toHaveBeenCalled(); + }); + + it("uses legacy activity review feedback when workflow results are absent or unsupported", async () => { + const taskWithUnsupportedWorkflowReview = { + ...FAKE_TASK_DETAIL, + id: "FN-010", + workflowStepResults: [{ workflowStepId: "custom-review", workflowStepName: "Custom Review", status: "passed", verdict: "REVISE", output: "not a current built-in review node" }], + log: [{ timestamp: fallbackTimestamp, action: "plan review Step 2: RETHINK - revise the approach" }], + }; + (store.getTask as ReturnType).mockResolvedValue(taskWithUnsupportedWorkflowReview); + (store.getAgentLogs as ReturnType).mockResolvedValue([]); + + const res = await REQUEST(buildApp(), "GET", "/api/tasks/FN-010/review"); + + expect(res.status).toBe(200); + expect(res.body.items).toEqual([expect.objectContaining({ + itemId: fallbackItemId, + title: "plan review RETHINK", + body: "plan review Step 2: RETHINK - revise the approach", + verdict: "RETHINK", + reviewType: "plan", + })]); + expect(store.getAgentLogs).toHaveBeenCalledWith("FN-010"); + }); + it("resumes reviewer-agent in-review tasks using canonical log-derived review ids when reviewState is absent", async () => { const taskWithoutPersistedItems = { ...FAKE_TASK_DETAIL, @@ -2785,6 +2844,68 @@ describe("POST /tasks/:id/review/address", () => { expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); }); + it("addresses workflow review items by canonical id without trusting forged client feedback", async () => { + const taskWithWorkflowReview = { + ...FAKE_TASK_DETAIL, + id: "FN-009", + column: "in-review", + status: "awaiting-user-review", + assignedAgentId: null, + sessionFile: null, + workflowStepResults: [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + phase: "pre-merge", + status: "passed", + verdict: "APPROVE_WITH_NOTES", + output: "Canonical advisory: preserve this text.", + completedAt: "2026-01-05T00:00:00.000Z", + }], + reviewState: undefined, + }; + const movedTask = { ...taskWithWorkflowReview, column: "in-progress", status: null }; + (store.getTask as ReturnType).mockResolvedValue(taskWithWorkflowReview); + (store.addSteeringComment as ReturnType).mockResolvedValue({ id: "sc-1" }); + (store.moveTask as ReturnType).mockResolvedValue(movedTask); + + const review = await REQUEST(buildApp(), "GET", "/api/tasks/FN-009/review"); + const itemId = review.body.items[0].itemId; + const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-009/review/address", JSON.stringify({ + selectedItems: [{ + id: itemId, + source: "reviewer-agent", + summary: "FORGED summary", + body: "FORGED body", + author: "attacker", + filePath: "forged.ts", + lineNumber: 999, + url: "https://invalid.example/forged", + }], + }), { "Content-Type": "application/json" }); + + expect(res.status).toBe(200); + expect(store.updateTask).toHaveBeenCalledWith("FN-009", { + reviewState: expect.objectContaining({ + addressing: [expect.objectContaining({ + itemId, + snapshot: expect.objectContaining({ + summary: "Code Review APPROVE_WITH_NOTES", + body: "Canonical advisory: preserve this text.", + authorLogin: "reviewer-agent", + filePath: undefined, + lineNumber: undefined, + url: undefined, + }), + })], + }), + }); + const steering = (store.addSteeringComment as ReturnType).mock.calls[0][1] as string; + expect(steering).toContain("Canonical advisory: preserve this text."); + expect(steering).not.toContain("FORGED"); + expect(steering).not.toContain("invalid.example"); + expect(store.moveTask).toHaveBeenCalledWith("FN-009", "in-progress", { preserveProgress: true }); + }); + it("accepts reviewer-agent fallback log review ids when no reviewer text block exists", async () => { const taskWithFallbackLog = { ...FAKE_TASK_DETAIL, diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index ef0b6c78b0..94a6b81fdc 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -9,6 +9,7 @@ const severityAuditLog = createLogger("dashboard-register-task-workflow-routes") * than turning one board load into thousands of file reads. Truncation is logged, never silent. */ const AWAITING_PLANNING_ENRICH_LIMIT = 200; +import { createHash } from "node:crypto"; import { createReadStream } from "node:fs"; import { readFile, stat } from "node:fs/promises"; import { join } from "node:path"; @@ -21,6 +22,8 @@ import type { TaskReviewData, TaskReviewItem, TaskReviewSummary, + TaskReviewVerdict, + WorkflowStepResult, GithubIssueAction, DuplicateCandidate, DuplicateMatch, @@ -810,7 +813,101 @@ function buildReviewerAgentItemId(input: { index: number; reviewType: "plan" | " return `reviewer-${input.reviewType}-${stepPart}-${verdictPart}-${timePart}-${input.index + 1}`; } +const CURRENT_WORKFLOW_REVIEW_STEP_IDS = new Set(["code-review", "plan-review"]); +const CURRENT_WORKFLOW_REVIEW_STATUSES = new Set(["passed", "failed", "advisory_failure"]); +function parseTaskReviewVerdict(value: string | undefined): TaskReviewVerdict | undefined { + switch (value) { + case "APPROVE": + case "APPROVE_WITH_NOTES": + case "REVISE": + case "RETHINK": + case "UNAVAILABLE": + return value; + default: + return undefined; + } +} + +/** + * FNXC:TaskReview 2026-08-05-01:57: + * The current graph-owned Code Review and Plan Review ids are the only structured review sources: + * current, terminal, verdict-bearing results win over compatibility-only reviewer prose/activity + * parsing. Their persisted identity produces canonical ids so GET, refresh, and address reconstruct + * the same server-owned feedback; address snapshots and steering must never trust client prose. + * Custom step names, historical attempts, pending/skipped, superseded, and bypassed results remain + * non-selectable until workflow results gain an authoritative persisted review-kind marker. + */ +function isCurrentWorkflowReviewResult(result: WorkflowStepResult): result is WorkflowStepResult & { verdict: TaskReviewVerdict } { + return CURRENT_WORKFLOW_REVIEW_STEP_IDS.has(result.workflowStepId) + && CURRENT_WORKFLOW_REVIEW_STATUSES.has(result.status) + && result.verdict !== undefined + && result.supersededAt === undefined + && result.bypassedBy === undefined + && result.bypassedAt === undefined + && result.bypassReason === undefined + && result.bypassedFromStatus === undefined + && result.bypassedFromVerdict === undefined; +} + +function buildWorkflowReviewItemId(task: Task, result: WorkflowStepResult): string { + const identity = JSON.stringify({ + taskId: task.id, + workflowStepId: result.workflowStepId, + workflowStepName: result.workflowStepName, + phase: result.phase, + status: result.status, + verdict: result.verdict, + completedAt: result.completedAt, + startedAt: result.startedAt, + output: result.output, + notes: result.notes, + }); + return `workflow-review-${createHash("sha256").update(identity).digest("hex").slice(0, 24)}`; +} + +function buildWorkflowReviewItems(task: Task): TaskReviewItem[] { + return (task.workflowStepResults ?? []) + .filter(isCurrentWorkflowReviewResult) + .map((result): TaskReviewItem => { + const reviewType = result.workflowStepId === "plan-review" ? "plan" as const : "code" as const; + const body = result.output?.trim() || result.notes?.trim() || "No written feedback was provided by this review step."; + const timestamp = result.completedAt ?? result.startedAt ?? task.updatedAt ?? task.createdAt; + return { + itemId: buildWorkflowReviewItemId(task, result), + sourceMode: "reviewer-agent", + title: `${result.workflowStepName || result.workflowStepId} ${result.verdict}`, + body, + author: "reviewer-agent", + createdAt: timestamp, + updatedAt: timestamp, + reviewState: result.verdict, + verdict: result.verdict, + reviewType, + progressStatus: null, + }; + }) + .sort((a, b) => Date.parse(b.createdAt ?? "") - Date.parse(a.createdAt ?? "") || a.itemId.localeCompare(b.itemId)); +} + +function buildDirectReviewSummary(items: TaskReviewItem[]): TaskReviewSummary | null { + const latest = items[0]; + return latest + ? { summary: latest.title, verdict: latest.verdict } + : null; +} + async function buildDirectTaskReviewData(task: Task, store: TaskStore): Promise { + const structuredItems = buildWorkflowReviewItems(task); + if (structuredItems.length > 0) { + return { + mode: "reviewer-agent", + refreshable: true, + fetchedAt: new Date().toISOString(), + summary: buildDirectReviewSummary(structuredItems), + items: structuredItems, + }; + } + const agentLogs = await store.getAgentLogs(task.id); const reviewerText = agentLogs.filter((entry) => entry.agent === "reviewer" && entry.type === "text").map((entry) => entry.text).join("\n"); const fallbackLogs = (task.log ?? []).filter((entry) => REVIEW_STEP_RE.test(entry.action)); @@ -833,6 +930,8 @@ async function buildDirectTaskReviewData(task: Task, store: TaskStore): Promise< createdAt, updatedAt: createdAt, reviewState: verdict ?? null, + verdict: parseTaskReviewVerdict(verdict), + reviewType, progressStatus: null, }); } @@ -851,25 +950,20 @@ async function buildDirectTaskReviewData(task: Task, store: TaskStore): Promise< createdAt: entry.timestamp, updatedAt: entry.timestamp, reviewState: verdict ?? null, + verdict: parseTaskReviewVerdict(verdict), + reviewType, progressStatus: null, }); }); } - const sorted = [...items].sort((a, b) => Date.parse(b.createdAt ?? "") - Date.parse(a.createdAt ?? "")); - const latest = sorted[0]; - const summary: TaskReviewSummary | null = latest - ? { - summary: latest.title, - verdict: (latest.reviewState as "APPROVE" | "REVISE" | "RETHINK" | "UNAVAILABLE" | null | undefined) ?? undefined, - } - : null; + const sorted = [...items].sort((a, b) => Date.parse(b.createdAt ?? "") - Date.parse(a.createdAt ?? "") || a.itemId.localeCompare(b.itemId)); return { mode: "reviewer-agent", refreshable: true, fetchedAt: new Date().toISOString(), - summary, + summary: buildDirectReviewSummary(sorted), items: sorted, }; } @@ -6046,20 +6140,13 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork type SelectedReviewItem = { id: string; source: "pr-review" | "reviewer-agent"; - threadId?: string; - filePath?: string; - lineNumber?: number; - author?: string; - summary: string; - body: string; - url?: string; }; const selectedItems: SelectedReviewItem[] = Array.isArray(req.body?.selectedItems) ? req.body.selectedItems.filter((value: unknown): value is SelectedReviewItem => { if (!value || typeof value !== "object") return false; const item = value as Record; - return typeof item.id === "string" && item.id.trim().length > 0 && typeof item.summary === "string" && typeof item.body === "string"; + return typeof item.id === "string" && item.id.trim().length > 0 && typeof item.source === "string"; }) : []; @@ -6098,6 +6185,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork state: item.reviewState ?? undefined, isResolved: item.isResolved, source: item.sourceMode === "reviewer-agent" ? "reviewer-agent" as const : "github-pr" as const, + verdict: item.verdict, + reviewType: item.reviewType, })); const reviewState = { source: canonicalReviewData.mode, @@ -6117,7 +6206,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork const now = new Date().toISOString(); const selectedSet = new Set(selectedItems.map((item: SelectedReviewItem) => item.id)); const canonicalIds = new Set(reviewState.items.map((item) => item.id)); - const expectedSource = canonicalReviewData.mode === "pull-request" ? "pr-review" : "reviewer-agent"; + const expectedSource: "pr-review" | "reviewer-agent" = canonicalReviewData.mode === "pull-request" ? "pr-review" : "reviewer-agent"; const reviewSourceMismatch = selectedItems.find((item) => canonicalIds.has(item.id) && item.source !== expectedSource); if (reviewSourceMismatch) { throw badRequest("Selected review source does not match task review mode"); @@ -6127,8 +6216,25 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork throw badRequest("selectedItems must reference existing review items"); } - const modeSummary = `${reviewState.source === "pull-request" ? "pull-request" : "reviewer-agent"} · ${selectedItems.length} selected item(s)`; - const steeringItems = selectedItems.map((item: SelectedReviewItem, index: number) => { + const canonicalById = new Map(reviewState.items.map((item) => [item.id, item] as const)); + const canonicalSelections = selectedItems.map((selected) => { + const item = canonicalById.get(selected.id); + if (!item) throw badRequest("selectedItems must reference existing review items"); + return { + id: item.id, + source: expectedSource, + summary: item.summary ?? item.body.slice(0, 120), + body: item.body, + author: item.author.login, + filePath: item.path, + lineNumber: item.line, + threadId: item.threadId, + url: item.htmlUrl, + }; + }); + + const modeSummary = `${reviewState.source === "pull-request" ? "pull-request" : "reviewer-agent"} · ${canonicalSelections.length} selected item(s)`; + const steeringItems = canonicalSelections.map((item, index) => { const location = item.filePath ? `${item.filePath}${typeof item.lineNumber === "number" ? `:${item.lineNumber}` : ""}` : undefined; const snippetSource = item.body.trim() || item.summary.trim(); const snippet = snippetSource.length > 220 ? `${snippetSource.slice(0, 220)}…` : snippetSource; @@ -6140,7 +6246,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork const priorAddressingById = new Map(reviewState.addressing.map((record) => [record.itemId, record] as const)); const nextAddressing = [ ...reviewState.addressing.filter((record) => !selectedSet.has(record.itemId)), - ...selectedItems.map((item: SelectedReviewItem) => { + ...canonicalSelections.map((item) => { const existing = priorAddressingById.get(item.id); return { itemId: item.id,