From b60a74337727612e818bf773938db48626e72a5e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 28 Jun 2026 00:33:26 -0700 Subject: [PATCH] FN-7186: fix reviewer-agent revision requests Route review revision requests through canonical review data so reviewer-agent feedback can be selected reliably. - Rebuild /review/address validation from PR review details or direct reviewer-agent logs before matching selected items. - Preserve addressing state while accepting log-derived reviewer-agent item ids and rejecting mismatched sources. - Cover reviewer-agent, fallback-log, PR, and invalid-selection paths in dashboard route tests. - Document that Review revision feedback may come from PR data or reviewer-agent logs. - Add a patch changeset for the published Fusion package. Files changed: .changeset/fn-7186-review-revision-fix.md | 7 ++ docs/dashboard-guide.md | 4 +- .../dashboard/src/__tests__/routes-tasks.test.ts | 126 +++++++++++++++++---- .../src/routes/register-task-workflow-routes.ts | 76 +++++++++---- 4 files changed, 170 insertions(+), 43 deletions(-) Fusion-Task-Id: FN-7186 Fusion-Task-Lineage: 8b0f6e4c-93b1-489c-ab96-5a2541b7c18a Co-authored-by: Fusion (runfusion.ai) --- .changeset/fn-7186-review-revision-fix.md | 7 + docs/dashboard-guide.md | 4 +- .../src/__tests__/routes-tasks.test.ts | 126 +++++++++++++++--- .../routes/register-task-workflow-routes.ts | 74 +++++++--- 4 files changed, 169 insertions(+), 42 deletions(-) create mode 100644 .changeset/fn-7186-review-revision-fix.md diff --git a/.changeset/fn-7186-review-revision-fix.md b/.changeset/fn-7186-review-revision-fix.md new file mode 100644 index 0000000000..f0bc4700a8 --- /dev/null +++ b/.changeset/fn-7186-review-revision-fix.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix "Request revision" error on reviewer-agent task reviews. +category: fix +dev: review/address now validates selected items against the same canonical review source the UI renders (buildDirectTaskReviewData / getPrReviewDetails) instead of the persisted reviewState.items. diff --git a/docs/dashboard-guide.md b/docs/dashboard-guide.md index dac30f632a..82c7ac8805 100644 --- a/docs/dashboard-guide.md +++ b/docs/dashboard-guide.md @@ -1008,8 +1008,8 @@ Inspect task definition, logs, review feedback, comments, artifacts, workflow ou - The **Artifacts** tab combines task documents written by agents or users with task-scoped registered media artifacts. The gallery uses thumbnail-first image/video cards, image and video previews can expand into a dismissible full-size lightbox, video and audio use native controls, document artifacts show text previews, and generic artifacts open through their media URL. - The **Review** tab is separate from **Comments**: Review shows actionable PR/reviewer feedback and same-task revision controls, while Comments remains the general collaboration thread. - Review comments hide GitHub template HTML comments in both Markdown and Plain modes, show author avatars or User/Bot fallbacks, label Human vs Bot/agent authors, and include All/Human/Bot filtering. -- **Request revision** in Review resumes work on the same task ID (no refinement task): `in-progress` tasks get steering injection, while `in-review` tasks are moved back to `in-progress` for the same branch/worktree revision pass. -- Review supports a manual **Refresh** action in-place: PR mode pulls latest GitHub review state/decision, while direct mode rehydrates reviewer-agent feedback from persisted task data (no GitHub call). +- **Request revision** in Review resumes work on the same task ID (no refinement task): `in-progress` tasks get steering injection, while `in-review` tasks are moved back to `in-progress` for the same branch/worktree revision pass. The selected feedback can come from either PR review data or reviewer-agent feedback shown in the tab. +- Review supports a manual **Refresh** action in-place: PR mode pulls latest GitHub review state/decision, while direct mode rehydrates reviewer-agent feedback from task agent logs (no GitHub call). - For shared `branch_groups` (tasks with `branchContext.groupId`), PR merge mode opens and tracks one group-level PR from the group integration branch to the project default branch; member tasks share that PR state. - In direct/non-PR auto-merge mode, Review renders normalized reviewer-agent feedback (verdict/step/timestamp/detail) with dedicated loading/error/empty states; it does not require users to read raw agent logs. diff --git a/packages/dashboard/src/__tests__/routes-tasks.test.ts b/packages/dashboard/src/__tests__/routes-tasks.test.ts index f7fa15e059..4a29c01401 100644 --- a/packages/dashboard/src/__tests__/routes-tasks.test.ts +++ b/packages/dashboard/src/__tests__/routes-tasks.test.ts @@ -145,6 +145,11 @@ vi.mock("@fusion/engine", async () => { promptWithFallback: vi.fn(async (session: { prompt: (message: string) => Promise }, prompt: string) => { await session.prompt(prompt); }), + /* + FNXC:DashboardRouteTests 2026-06-27-00:08: + Route tests mock @fusion/engine wholesale, but planning/subtask helpers now resolve MCP servers before creating read-only AI sessions. Keep the default MCP result shaped so unrelated route assertions do not fail on the fallback vi.fn() returning undefined. + */ + resolveMcpServersForStore: vi.fn().mockResolvedValue({ servers: [], errors: [] }), AgentReflectionService: class MockAgentReflectionService { async generateReflection(): Promise { throw new Error("Reflection service unavailable in route tests"); @@ -1962,7 +1967,7 @@ describe("POST /subtasks/*", () => { expect(res.status).toBe(200); expect(res.body).toEqual({ success: true, sessionId: "session-123" }); - expect(retrySpy).toHaveBeenCalledWith("session-123", "/fake/root", undefined); + expect(retrySpy).toHaveBeenCalledWith("session-123", "/fake/root", undefined, store); }); it("returns 404 when subtask retry session does not exist", async () => { @@ -2604,6 +2609,10 @@ describe("POST /subtasks/*", () => { describe("POST /tasks/:id/review/address", () => { let store: TaskStore; + const reviewerBlockTimestamp = "2026-01-02T03:04:05.000Z"; + const reviewerBlockItemId = "reviewer-code-step-na-revise-2026-01-02T03-04-05-000Z-1"; + const fallbackTimestamp = "2026-01-03T04:05:06.000Z"; + const fallbackItemId = "reviewer-plan-step-2-rethink-2026-01-03T04-05-06-000Z-1"; beforeEach(() => { store = createMockStore({ updateStep: vi.fn() } as unknown as Partial); @@ -2616,45 +2625,116 @@ describe("POST /tasks/:id/review/address", () => { return app; } - it("resumes in-review tasks to in-progress using selected review payload", async () => { - const taskWithReview = { + function mockReviewerBlockLogs() { + (store.getAgentLogs as ReturnType).mockResolvedValue([ + { + timestamp: reviewerBlockTimestamp, + agent: "reviewer", + type: "text", + text: "## Code Review:\n\n### Verdict: REVISE\n\nFix tests before merge.", + }, + ]); + } + + function mockPrReviewDetails() { + return vi.spyOn(GitHubClient.prototype, "getPrReviewDetails").mockResolvedValue({ + mode: "pull-request", + refreshable: true, + fetchedAt: "2026-01-04T05:06:07.000Z", + summary: { reviewDecision: "CHANGES_REQUESTED", reviewers: [], blockingReasons: [], checks: [] }, + items: [ + { + itemId: "ri-1", + sourceMode: "pull-request", + title: "Fix tests", + body: "Fix tests", + author: "reviewer", + createdAt: "2026-01-04T05:06:07.000Z", + updatedAt: "2026-01-04T05:06:07.000Z", + filePath: "src/a.ts", + line: 4, + reviewState: "CHANGES_REQUESTED", + }, + ], + }); + } + + it("resumes reviewer-agent in-review tasks using canonical log-derived review ids when reviewState is absent", async () => { + const taskWithoutPersistedItems = { ...FAKE_TASK_DETAIL, id: "FN-001", column: "in-review", status: "awaiting-user-review", assignedAgentId: null, + updatedAt: reviewerBlockTimestamp, steps: [{ id: "s1", title: "Step 1", status: "done" }], - reviewState: { - source: "reviewer-agent", - items: [{ id: "ri-1", body: "Fix tests", summary: "Fix tests", author: { login: "reviewer" }, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString() }], - addressing: [], - }, + reviewState: undefined, }; - const movedTask = { ...taskWithReview, column: "in-progress", status: null, sessionFile: null, assignedAgentId: null }; - (store.getTask as ReturnType).mockResolvedValueOnce(taskWithReview).mockResolvedValueOnce({ ...taskWithReview, reviewState: { ...taskWithReview.reviewState, addressing: [{ itemId: "ri-1", status: "queued", selectedAt: new Date().toISOString() }] } }); + const taskAfterAddressing = { + ...taskWithoutPersistedItems, + reviewState: { source: "reviewer-agent", items: [], addressing: [{ itemId: reviewerBlockItemId, status: "queued", selectedAt: reviewerBlockTimestamp }] }, + }; + const movedTask = { ...taskAfterAddressing, column: "in-progress", status: null, sessionFile: null, assignedAgentId: null }; + mockReviewerBlockLogs(); + (store.getTask as ReturnType).mockResolvedValueOnce(taskWithoutPersistedItems).mockResolvedValueOnce(taskAfterAddressing); (store.addSteeringComment as ReturnType).mockResolvedValue({ id: "sc-1" }); (store.moveTask as ReturnType).mockResolvedValue(movedTask); - const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: "ri-1", source: "reviewer-agent", summary: "Fix tests", body: "Fix tests" }] }), { "Content-Type": "application/json" }); + const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: reviewerBlockItemId, source: "reviewer-agent", summary: "code review REVISE", body: "Fix tests before merge." }] }), { "Content-Type": "application/json" }); expect(res.status).toBe(200); + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { + reviewState: expect.objectContaining({ + source: "reviewer-agent", + items: [expect.objectContaining({ id: reviewerBlockItemId, source: "reviewer-agent" })], + addressing: [expect.objectContaining({ itemId: reviewerBlockItemId, status: "queued" })], + }), + }); + expect(store.addSteeringComment).toHaveBeenCalledWith("FN-001", expect.stringContaining("Fix tests before merge."), "user"); expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-progress", { preserveProgress: true }); expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); }); - it("for in-progress tasks injects steering without moving task", async () => { - const taskWithReview = { + it("accepts reviewer-agent fallback log review ids when no reviewer text block exists", async () => { + const taskWithFallbackLog = { ...FAKE_TASK_DETAIL, id: "FN-001", column: "in-progress", sessionFile: "active.session.json", + reviewState: { source: "reviewer-agent", items: [], addressing: [] }, + log: [{ timestamp: fallbackTimestamp, action: "plan review Step 2: RETHINK - revise the approach" }], + }; + (store.getAgentLogs as ReturnType).mockResolvedValue([]); + (store.getTask as ReturnType).mockResolvedValueOnce(taskWithFallbackLog).mockResolvedValueOnce(taskWithFallbackLog); + (store.addSteeringComment as ReturnType).mockResolvedValue({ id: "sc-1" }); + + const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: fallbackItemId, source: "reviewer-agent", summary: "plan review RETHINK", body: "revise the approach" }] }), { "Content-Type": "application/json" }); + + expect(res.status).toBe(200); + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { + reviewState: expect.objectContaining({ + items: [expect.objectContaining({ id: fallbackItemId })], + addressing: [expect.objectContaining({ itemId: fallbackItemId })], + }), + }); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("for PR in-progress tasks validates canonical PR review items without moving task", async () => { + mockPrReviewDetails(); + const taskWithPrReview = { + ...FAKE_TASK_DETAIL, + id: "FN-001", + column: "in-progress", + sessionFile: "active.session.json", + prInfo: { number: 42, url: "https://github.com/acme/repo/pull/42", head: "feature", base: "main" }, reviewState: { source: "pull-request", - items: [{ id: "ri-1", body: "Fix tests", summary: "Fix tests", author: { login: "reviewer" }, createdAt: new Date().toISOString(), path: "src/a.ts", line: 4 }], + items: [{ id: "ri-1", body: "Fix tests", summary: "Fix tests", author: { login: "reviewer" }, createdAt: "2026-01-04T05:06:07.000Z", path: "src/a.ts", line: 4 }], addressing: [], }, }; - (store.getTask as ReturnType).mockResolvedValueOnce(taskWithReview).mockResolvedValueOnce(taskWithReview); + (store.getTask as ReturnType).mockResolvedValueOnce(taskWithPrReview).mockResolvedValueOnce(taskWithPrReview); (store.addSteeringComment as ReturnType).mockResolvedValue({ id: "sc-1" }); const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: "ri-1", source: "pr-review", summary: "Fix tests", body: "Fix tests", filePath: "src/a.ts", lineNumber: 4 }] }), { "Content-Type": "application/json" }); @@ -2671,18 +2751,22 @@ describe("POST /tasks/:id/review/address", () => { }); it("rejects unsupported review source", async () => { - (store.getTask as ReturnType).mockResolvedValue({ ...FAKE_TASK_DETAIL, id: "FN-001", reviewState: { source: "reviewer-agent", items: [{ id: "ri-1", body: "x", summary: "x", author: { login: "reviewer" }, createdAt: new Date().toISOString() }], addressing: [] } }); - const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: "ri-1", source: "other", summary: "x", body: "x" }] }), { "Content-Type": "application/json" }); + mockReviewerBlockLogs(); + (store.getTask as ReturnType).mockResolvedValue({ ...FAKE_TASK_DETAIL, id: "FN-001", updatedAt: reviewerBlockTimestamp, reviewState: { source: "reviewer-agent", items: [], addressing: [] } }); + const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: reviewerBlockItemId, source: "other", summary: "x", body: "x" }] }), { "Content-Type": "application/json" }); expect(res.status).toBe(400); expect(res.body.error).toContain("Unsupported review source"); }); - it("rejects source mismatch and unknown item ids", async () => { - (store.getTask as ReturnType).mockResolvedValue({ + it("rejects source mismatch and unknown item ids against canonical review data", async () => { + mockPrReviewDetails(); + const taskWithPrReview = { ...FAKE_TASK_DETAIL, id: "FN-001", - reviewState: { source: "pull-request", items: [{ id: "ri-1", body: "x", summary: "x", author: { login: "reviewer" }, createdAt: new Date().toISOString() }], addressing: [] }, - }); + prInfo: { number: 42, url: "https://github.com/acme/repo/pull/42", head: "feature", base: "main" }, + reviewState: { source: "pull-request", items: [{ id: "ri-1", body: "x", summary: "x", author: { login: "reviewer" }, createdAt: "2026-01-04T05:06:07.000Z" }], addressing: [] }, + }; + (store.getTask as ReturnType).mockResolvedValue(taskWithPrReview); const mismatch = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/review/address", JSON.stringify({ selectedItems: [{ id: "ri-1", source: "reviewer-agent", summary: "x", body: "x" }] }), { "Content-Type": "application/json" }); expect(mismatch.status).toBe(400); diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index 5e57421585..4ec4bc9556 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -3604,28 +3604,64 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork if (unsupportedSource) { throw badRequest(`Unsupported review source: ${String(unsupportedSource.source)}`); } - if (!task.reviewState) { - throw badRequest("Task has no reviewState payload"); + let canonicalReviewData: TaskReviewData; + if (task.prInfo) { + const badgeParsed = parseGitHubBadgeUrl(task.prInfo.url); + const repoInfo = getCurrentRepo(scopedStore.getRootDir()); + const owner = badgeParsed?.owner ?? repoInfo?.owner; + const repo = badgeParsed?.repo ?? repoInfo?.repo; + if (!owner || !repo) { + throw badRequest("Could not determine GitHub repository for PR review fetch"); + } + canonicalReviewData = await new GitHubClient(options?.githubToken ?? process.env.GITHUB_TOKEN).getPrReviewDetails(owner, repo, task.prInfo.number); + } else { + canonicalReviewData = await buildDirectTaskReviewData(task, scopedStore); } + const canonicalReviewItems = canonicalReviewData.items.map((item) => ({ + id: item.itemId, + body: item.body, + summary: item.title, + author: { login: item.author }, + createdAt: item.createdAt ?? canonicalReviewData.fetchedAt ?? new Date(0).toISOString(), + updatedAt: item.updatedAt ?? undefined, + path: item.filePath, + line: item.line, + threadId: item.threadId, + htmlUrl: item.url, + state: item.reviewState ?? undefined, + isResolved: item.isResolved, + source: item.sourceMode === "reviewer-agent" ? "reviewer-agent" as const : "github-pr" as const, + })); + const reviewState = { + source: canonicalReviewData.mode, + summary: canonicalReviewData.summary ?? task.reviewState?.summary, + items: canonicalReviewItems, + addressing: task.reviewState?.addressing ?? [], + lastRefreshedAt: task.reviewState?.lastRefreshedAt ?? canonicalReviewData.fetchedAt ?? undefined, + refreshSource: task.reviewState?.refreshSource, + refreshStatus: task.reviewState?.refreshStatus, + refreshError: task.reviewState?.refreshError, + } satisfies NonNullable; + + /* + FNXC:TaskReview 2026-06-27-00:00: + Revision validation must use the same canonical review source the UI renders. Reviewer-agent ids come from buildReviewerAgentItemId via buildDirectTaskReviewData, not from persisted reviewState.items, because direct executor review addressing persists only addressing snapshots. + */ const now = new Date().toISOString(); const selectedSet = new Set(selectedItems.map((item: SelectedReviewItem) => item.id)); - const sourceById = new Map(selectedItems.map((item: SelectedReviewItem) => [item.id, item.source] as const)); - const reviewSourceMismatch = task.reviewState.items.find((item: NonNullable["items"][number]) => { - const selectedSource = sourceById.get(item.id); - if (!selectedSource || !task.reviewState) return false; - const expectedSource = task.reviewState.source === "pull-request" ? "pr-review" : "reviewer-agent"; - return selectedSource !== expectedSource; - }); + const canonicalIds = new Set(reviewState.items.map((item) => item.id)); + const expectedSource = 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"); } - const matchedItems = task.reviewState.items.filter((item: NonNullable["items"][number]) => selectedSet.has(item.id)); - if (matchedItems.length !== selectedSet.size) { + const hasUnknownSelection = selectedItems.some((item) => !canonicalIds.has(item.id)); + if (hasUnknownSelection) { throw badRequest("selectedItems must reference existing review items"); } - const modeSummary = `${task.reviewState.source === "pull-request" ? "pull-request" : "reviewer-agent"} · ${selectedItems.length} selected item(s)`; + const modeSummary = `${reviewState.source === "pull-request" ? "pull-request" : "reviewer-agent"} · ${selectedItems.length} selected item(s)`; const steeringItems = selectedItems.map((item: SelectedReviewItem, index: number) => { const location = item.filePath ? `${item.filePath}${typeof item.lineNumber === "number" ? `:${item.lineNumber}` : ""}` : undefined; const snippetSource = item.body.trim() || item.summary.trim(); @@ -3635,9 +3671,9 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork }); const steeringText = ["Selected review feedback to address", modeSummary, ...steeringItems].join("\n"); - const priorAddressingById = new Map(task.reviewState.addressing.map((record) => [record.itemId, record] as const)); + const priorAddressingById = new Map(reviewState.addressing.map((record) => [record.itemId, record] as const)); const nextAddressing = [ - ...task.reviewState.addressing.filter((record) => !selectedSet.has(record.itemId)), + ...reviewState.addressing.filter((record) => !selectedSet.has(record.itemId)), ...selectedItems.map((item: SelectedReviewItem) => { const existing = priorAddressingById.get(item.id); return { @@ -3650,7 +3686,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork stale: false, snapshot: { itemId: item.id, - sourceMode: task.reviewState?.source ?? "pull-request", + sourceMode: reviewState.source, source: item.source, summary: item.summary, body: item.body, @@ -3665,12 +3701,12 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork }), ]; - const reviewState = { - ...task.reviewState, + const nextReviewState = { + ...reviewState, addressing: nextAddressing, }; - await scopedStore.updateTask(task.id, { reviewState }); + await scopedStore.updateTask(task.id, { reviewState: nextReviewState }); let steeringCommentId: string | null = null; const steeringComment = await scopedStore.addSteeringComment(task.id, steeringText, "user"); @@ -3704,7 +3740,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork } await scopedStore.logEntry(task.id, "Same-task review revision requested", `${selectedItems.length} item(s) submitted from review tab`); - res.json({ task: updatedTask, reviewState }); + res.json({ task: updatedTask, reviewState: nextReviewState }); } catch (err: unknown) { if (err instanceof ApiError) { throw err;