From 569b4029b8a0647c1cfcd686e131f878d8fa62a2 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 16 Jul 2026 21:52:28 -0600 Subject: [PATCH] Align persona review verification flow --- README.md | 2 +- apps/server/src/agents/reviews.ts | 52 +++++- .../server/src/agents/tmux/command-builder.ts | 2 +- apps/server/src/reviews/injection-prompts.ts | 11 +- apps/server/src/routes/personas.ts | 2 +- apps/server/src/routes/reviews.ts | 11 +- apps/server/src/server/mcp-review-handlers.ts | 18 +- .../shared/mcp/persona-interaction-tools.ts | 8 +- apps/server/src/shared/mcp/server.ts | 1 + apps/server/test/db/agent-manager.test.ts | 5 + apps/server/test/injection-prompts.test.ts | 33 ++++ apps/server/test/mcp-auth-integration.test.ts | 1 + apps/server/test/mcp-handlers.test.ts | 8 +- .../test/mcp-launch-persona-response.test.ts | 10 +- apps/server/test/mcp-review-handlers.test.ts | 62 +++++++ apps/server/test/review-feedback.test.ts | 158 +++++++++++++++++- .../components/app/docs-sections/personas.tsx | 9 +- .../components/app/docs-sections/tools.tsx | 3 +- 18 files changed, 373 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index cc1dec72..37fd9cd9 100644 --- a/README.md +++ b/README.md @@ -200,7 +200,7 @@ Every agent launched by Dispatch gets access to MCP tools via an agent-scoped en ### Persona agents -Persona review agents get a narrower set focused on reviewing their parent's work: `dispatch_review_submit`, `dispatch_review_add_feedback`, `dispatch_review_list_feedback`, `dispatch_review_add_message`, `dispatch_event`, `dispatch_pin`, `dispatch_share`, and `get_parent_context`. +Persona review agents get a narrower set focused on reviewing their parent's work: `dispatch_review_submit`, `dispatch_review_add_feedback`, `dispatch_review_list_feedback`, `dispatch_review_add_message`, `dispatch_review_resolve`, `dispatch_event`, `dispatch_pin`, `dispatch_share`, and `get_parent_context`. After the parent reports a fix in the feedback thread, the reviewer re-inspects it and either resolves the item or replies with further instructions. ### Job agents diff --git a/apps/server/src/agents/reviews.ts b/apps/server/src/agents/reviews.ts index c5526502..9a4b953d 100644 --- a/apps/server/src/agents/reviews.ts +++ b/apps/server/src/agents/reviews.ts @@ -32,6 +32,18 @@ export type ReviewFeedbackItemRecord = { updatedAt: string; }; +export type ReviewFeedbackResolverRole = "reviewer" | "assignee" | "human"; + +export class ReviewFeedbackResolutionConflictError extends Error { + constructor(public readonly item: ReviewFeedbackItemRecord) { + const resolution = item.resolution ? ` (${item.resolution})` : ""; + super( + `Review feedback item #${item.id} is already ${item.status}${resolution}; refresh the review before acting.` + ); + this.name = "ReviewFeedbackResolutionConflictError"; + } +} + export type ReviewThreadMessageRecord = { id: number; feedbackItemId: number; @@ -363,7 +375,8 @@ export async function resolveReviewFeedbackItem( note?: string | null; resolvedBy?: string | null; authorType?: "human" | "agent"; - } = {} + resolverRole: ReviewFeedbackResolverRole; + } ): Promise<{ item: ReviewFeedbackItemRecord; reviewId: number; @@ -381,7 +394,12 @@ export async function resolveReviewFeedbackItem( resolved_by = $3, resolved_at = NOW(), updated_at = NOW() FROM reviews r WHERE fi.id = $4 AND fi.review_id = r.id - AND (r.agent_id = $5 OR r.assigned_agent_id = $5) + AND fi.status = 'open' + AND ( + (($6 = 'human' OR $6 = 'assignee') + AND (r.agent_id = $5 OR r.assigned_agent_id = $5)) + OR ($6 = 'reviewer' AND r.reviewer_type = 'agent' AND r.reviewer_agent_id = $5) + ) RETURNING fi.id, fi.review_id AS "reviewId", fi.file_path AS "filePath", fi.line_start AS "lineStart", fi.line_end AS "lineEnd", @@ -390,10 +408,38 @@ export async function resolveReviewFeedbackItem( fi.resolved_by AS "resolvedBy", fi.resolved_at AS "resolvedAt", fi.created_at AS "createdAt", fi.updated_at AS "updatedAt", r.agent_id AS "agentId"`, - [resolution, opts.note ?? null, opts.resolvedBy ?? null, itemId, agentId] + [ + resolution, + opts.note ?? null, + opts.resolvedBy ?? null, + itemId, + agentId, + opts.resolverRole, + ] ); const item = itemResult.rows[0]; if (!item) { + const currentResult = await client.query( + `SELECT fi.id, fi.review_id AS "reviewId", fi.file_path AS "filePath", + fi.line_start AS "lineStart", fi.line_end AS "lineEnd", + fi.diff_snapshot AS "diffSnapshot", fi.base_ref AS "baseRef", + fi.status, fi.resolution, fi.resolution_note AS "resolutionNote", + fi.resolved_by AS "resolvedBy", fi.resolved_at AS "resolvedAt", + fi.created_at AS "createdAt", fi.updated_at AS "updatedAt" + FROM review_feedback_items fi + JOIN reviews r ON r.id = fi.review_id + WHERE fi.id = $1 + AND ( + (($3 = 'human' OR $3 = 'assignee') + AND (r.agent_id = $2 OR r.assigned_agent_id = $2)) + OR ($3 = 'reviewer' AND r.reviewer_type = 'agent' AND r.reviewer_agent_id = $2) + )`, + [itemId, agentId, opts.resolverRole] + ); + const currentItem = currentResult.rows[0]; + if (currentItem && currentItem.status !== "open") { + throw new ReviewFeedbackResolutionConflictError(currentItem); + } await client.query("ROLLBACK"); return null; } diff --git a/apps/server/src/agents/tmux/command-builder.ts b/apps/server/src/agents/tmux/command-builder.ts index 03ecbbdb..92759e29 100644 --- a/apps/server/src/agents/tmux/command-builder.ts +++ b/apps/server/src/agents/tmux/command-builder.ts @@ -183,7 +183,7 @@ export function buildLaunchGuidance( ); if (autoReview) { rules.push( - "Autonomous Review is enabled. Before emitting done: commit and push your branch, open a draft PR via create_pr (don't override baseBranch — it defaults correctly), call list_personas, then launch 1 relevant reviewer via dispatch_launch_persona. Wait for the structured REVIEW SUBMITTED prompt instead of polling. If feedback exists, call dispatch_review_list_feedback with the supplied review ID, keep all discussion in item threads via dispatch_review_add_message, and set each outcome with dispatch_review_resolve. Use dispatch_review_reopen if more work is needed. A clean zero-item approval requires no action. Don't emit done until all submitted reviews are resolved." + "Autonomous Review is enabled. Before emitting done: commit and push your branch, open a draft PR via create_pr (don't override baseBranch — it defaults correctly), call list_personas, then launch 1 relevant reviewer via dispatch_launch_persona. After launch, do not poll, sleep, call list_agents, or schedule a wakeup; end the turn and let Dispatch inject the structured REVIEW SUBMITTED prompt when ready. If feedback exists, call dispatch_review_list_feedback with the supplied review ID and keep all discussion in item threads via dispatch_review_add_message. After fixing an item, ask the reviewer to verify it instead of resolving it yourself. The reviewer will resolve verified fixes or reply with further instructions. A clean zero-item approval requires no action. Don't emit done until all submitted reviews are resolved." ); } } diff --git a/apps/server/src/reviews/injection-prompts.ts b/apps/server/src/reviews/injection-prompts.ts index 8e0069bc..44bf984d 100644 --- a/apps/server/src/reviews/injection-prompts.ts +++ b/apps/server/src/reviews/injection-prompts.ts @@ -60,7 +60,9 @@ export function buildReviewSubmittedPrompt(input: { lines.push(`- #${item.id} ${location} — ${item.body}`); } lines.push( - "Call dispatch_review_list_feedback with this reviewId before acting. Use dispatch_review_add_message for questions or explanations, dispatch_review_resolve when an item is fixed or dismissed, and dispatch_review_reopen if a resolved item needs more work. Keep all review discussion in its feedback-item thread." + input.reviewerAgentId + ? "Call dispatch_review_list_feedback with this reviewId before acting. Use dispatch_review_add_message for questions and explanations. After fixing an item, post a concise message asking the reviewer to verify the fix; do not resolve persona-review feedback yourself. The reviewer will re-inspect the fix and resolve the item if complete, or leave it open and reply with further instructions. Keep all review discussion in its feedback-item thread." + : "Call dispatch_review_list_feedback with this reviewId before acting. Use dispatch_review_add_message for questions or explanations, dispatch_review_resolve when an item is fixed or dismissed, and dispatch_review_reopen if a resolved item needs more work. Keep all review discussion in its feedback-item thread." ); } return buildReviewPromptBlock("REVIEW SUBMITTED", lines); @@ -86,13 +88,18 @@ export function buildReviewThreadUpdatePrompt(input: { itemId: number; from: string; body: string; + recipient: "reviewer" | "assignee"; }): string { + const nextStep = + input.recipient === "reviewer" + ? "Re-inspect the claimed fix before deciding. If it fully addresses this item, call dispatch_review_resolve with resolution 'fixed'. If it is incomplete, leave the item open and reply with specific instructions using dispatch_review_add_message. Do not resolve based only on the assignee's claim." + : "Address any requested work, then use dispatch_review_add_message to ask the reviewer to verify the fix. Do not resolve persona-review feedback yourself."; return buildReviewPromptBlock("REVIEW THREAD UPDATE", [ `Review ID: ${input.reviewId}`, `Feedback item ID: ${input.itemId}`, `From: ${input.from}`, `Message: ${input.body}`, - "Call dispatch_event with type 'working' before handling this update, then call dispatch_review_list_feedback with this reviewId for full context. Reply with dispatch_review_add_message only when useful; do not move review discussion to direct agent messages. Finish with dispatch_event type 'done', or 'waiting_user' only after posting a tracked question that needs a reply.", + `Call dispatch_event with type 'working' before handling this update, then call dispatch_review_list_feedback with this reviewId for full context. ${nextStep} Do not move review discussion to direct agent messages. Finish with dispatch_event type 'done', or 'waiting_user' only after posting a tracked question that needs a reply.`, ]); } diff --git a/apps/server/src/routes/personas.ts b/apps/server/src/routes/personas.ts index 4ac17984..7c80eccc 100644 --- a/apps/server/src/routes/personas.ts +++ b/apps/server/src/routes/personas.ts @@ -107,7 +107,7 @@ export async function registerPersonaRoutes( `Use the dispatch_launch_persona MCP tool to launch the "${body.persona}" persona on your current work.`, `Use agentType: "${body.agentType}" and includeDiff: ${includeDiff ? "true" : "false"}.`, "Treat this as an author-requested review for the current worktree/branch.", - "Wait for the structured REVIEW SUBMITTED prompt before acting on findings. Keep all review discussion in feedback-item threads with the dispatch_review_* tools.", + "After launch, do not poll, sleep, call list_agents, or schedule a wakeup. End the turn and wait for Dispatch to inject the structured REVIEW SUBMITTED prompt. Keep all review discussion in feedback-item threads with the dispatch_review_* tools. After fixing an item, ask the reviewer to verify it instead of resolving it yourself.", "Provide a detailed context briefing covering what you built, key files changed, and any areas that need extra attention.", ].join(" "); diff --git a/apps/server/src/routes/reviews.ts b/apps/server/src/routes/reviews.ts index 14e4aeb0..03549108 100644 --- a/apps/server/src/routes/reviews.ts +++ b/apps/server/src/routes/reviews.ts @@ -264,7 +264,7 @@ export async function registerReviewRoutes( itemId, agentId, body.resolution as "fixed" | "dismissed", - { note, authorType: "human" } + { note, authorType: "human", resolverRole: "human" } ); if (!result) { return reply.code(404).send({ error: "Feedback item not found." }); @@ -319,6 +319,14 @@ export async function registerReviewRoutes( return { item: result.item }; } catch (error) { + if ( + error instanceof reviewQueries.ReviewFeedbackResolutionConflictError + ) { + return reply.code(409).send({ + error: error.message, + item: error.item, + }); + } return deps.handleAgentError(reply, error); } } @@ -387,6 +395,7 @@ export async function registerReviewRoutes( itemId, from: "Human collaborator", body: body.body.trim(), + recipient: review?.reviewerAgentId ? "reviewer" : "assignee", }) ); } catch (error) { diff --git a/apps/server/src/server/mcp-review-handlers.ts b/apps/server/src/server/mcp-review-handlers.ts index 236a9907..beccbccf 100644 --- a/apps/server/src/server/mcp-review-handlers.ts +++ b/apps/server/src/server/mcp-review-handlers.ts @@ -301,6 +301,8 @@ export function createReviewHandlers(deps: CreateReviewHandlersDeps) { }; reviewStatus: string; }> { + const resolver = await agentManager.getAgent(agentId); + if (!resolver) throw new Error("Resolving agent not found."); const result = await resolveReviewFeedbackItem( pool, itemId, @@ -310,6 +312,7 @@ export function createReviewHandlers(deps: CreateReviewHandlersDeps) { note: opts.note ?? null, resolvedBy: agentId, authorType: "agent", + resolverRole: resolver.role === "review" ? "reviewer" : "assignee", } ); if (!result) { @@ -317,20 +320,25 @@ export function createReviewHandlers(deps: CreateReviewHandlersDeps) { `Review feedback item #${itemId} not found or not owned by this agent.` ); } + const review = await getReviewRecord(pool, result.reviewId); + const eventAgentId = review?.agentId ?? agentId; publishUiEvent({ type: "review_feedback.updated", - agentId, + agentId: eventAgentId, feedbackItemId: itemId, }); publishUiEvent({ type: "review.updated", - agentId, + agentId: eventAgentId, reviewId: result.reviewId, status: result.reviewStatus, }); - const review = await getReviewRecord(pool, result.reviewId); + const targetAgentId = + review?.reviewerAgentId === agentId + ? (review.assignedAgentId ?? review.agentId) + : (review?.reviewerAgentId ?? null); await sendPromptBestEffort( - review?.reviewerAgentId ?? null, + targetAgentId, buildReviewItemStatePrompt({ reviewId: result.reviewId, itemId, @@ -441,6 +449,8 @@ export function createReviewHandlers(deps: CreateReviewHandlersDeps) { itemId, from: actor?.persona ?? actor?.name ?? agentId, body, + recipient: + review?.reviewerAgentId === targetAgentId ? "reviewer" : "assignee", }) ); return { diff --git a/apps/server/src/shared/mcp/persona-interaction-tools.ts b/apps/server/src/shared/mcp/persona-interaction-tools.ts index bdae2795..d60c9b84 100644 --- a/apps/server/src/shared/mcp/persona-interaction-tools.ts +++ b/apps/server/src/shared/mcp/persona-interaction-tools.ts @@ -357,7 +357,7 @@ export function registerPersonaInteractionTools( "dispatch_review_resolve", { description: - "Parent-only. Resolve a review feedback item as fixed or dismissed. Review status is automatically derived from the current feedback item states.", + "Resolve a review feedback item as fixed or dismissed. For persona reviews, the reviewer uses this only after re-inspecting the assignee's fix; the assignee should request verification with dispatch_review_add_message instead of resolving the item. Review status is automatically derived from the current feedback item states.", inputSchema: { itemId: z .number() @@ -411,7 +411,7 @@ export function registerPersonaInteractionTools( server.registerTool( "dispatch_review_add_message", { - description: `Add a concise message to a review feedback item's thread. Use it only for a necessary clarifying question or essential explanation before resolving. ${AGENT_REVIEW_REPLY_GUIDANCE}`, + description: `Add a concise message to a review feedback item's thread. For persona feedback, the assignee uses this after fixing an item to request reviewer verification; the reviewer uses it to give further instructions when a fix is incomplete. ${AGENT_REVIEW_REPLY_GUIDANCE}`, inputSchema: { itemId: z .number() @@ -460,7 +460,9 @@ export function buildLaunchPersonaResponseText( The reviewer will inspect the target and create a review only when it calls dispatch_review_submit. Dispatch will inject a structured REVIEW SUBMITTED block here with the review summary and any feedback item IDs. -If the review has feedback, call dispatch_review_list_feedback with its reviewId before acting. Keep all questions and explanations tracked in the corresponding item thread with dispatch_review_add_message. Resolve an item with dispatch_review_resolve when it is fixed or intentionally dismissed, and use dispatch_review_reopen if it needs more work. +Do not poll, sleep, call list_agents, or schedule a wakeup while waiting for the review. End this turn after the launch; Dispatch will notify you with a new injected REVIEW SUBMITTED block when the reviewer submits. + +If the review has feedback, call dispatch_review_list_feedback with its reviewId before acting. Keep all questions and explanations tracked in the corresponding item thread with dispatch_review_add_message. After fixing an item, post a concise message asking the reviewer to verify it; do not call dispatch_review_resolve on persona feedback yourself. The reviewer will re-inspect the fix and resolve it if complete, or leave it open and reply with further instructions. A clean approval is also recorded: the reviewer submits a required summary with an empty feedback array, creating a resolved review with no items. No legacy round or recheck lifecycle is required.`; } diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index 56bc01da..3c983911 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -147,6 +147,7 @@ const REVIEW_AGENT_TOOLS = new Set([ "dispatch_review_add_feedback", "dispatch_review_list_feedback", "dispatch_review_add_message", + "dispatch_review_resolve", "get_parent_context", ]); diff --git a/apps/server/test/db/agent-manager.test.ts b/apps/server/test/db/agent-manager.test.ts index 4f38711b..72ab39b0 100644 --- a/apps/server/test/db/agent-manager.test.ts +++ b/apps/server/test/db/agent-manager.test.ts @@ -617,6 +617,11 @@ describe("AgentManager", () => { expect(setupScript).toContain("dispatch_review_list_feedback"); expect(setupScript).toContain("structured REVIEW SUBMITTED prompt"); expect(setupScript).toContain("launch 1 relevant reviewer"); + expect(setupScript).toContain("do not poll, sleep"); + expect(setupScript).toContain("ask the reviewer to verify it"); + expect(setupScript).not.toContain( + "set each outcome with dispatch_review_resolve" + ); expect(setupScript).not.toContain("dispatch_get_feedback"); expect(setupScript).not.toContain("Only launch additional reviewers"); }); diff --git a/apps/server/test/injection-prompts.test.ts b/apps/server/test/injection-prompts.test.ts index efb5da06..ea9e52b1 100644 --- a/apps/server/test/injection-prompts.test.ts +++ b/apps/server/test/injection-prompts.test.ts @@ -39,14 +39,47 @@ describe("unified review prompt blocks", () => { itemId: 9, from: "Parent", body: "Can you clarify?", + recipient: "reviewer", }); expect(text).toContain("Review ID: 7"); expect(text).toContain("Feedback item ID: 9"); expect(text).toContain("dispatch_review_add_message"); + expect(text).toContain("dispatch_review_resolve"); + expect(text).toMatch(/re-inspect/i); expect(text).toContain("type 'working'"); expect(text).toContain("type 'done'"); }); + it("tells the assignee to request verification instead of resolving", () => { + const text = buildReviewSubmittedPrompt({ + reviewId: 7, + reviewerName: "Security", + reviewerAgentId: "agt_reviewer", + summary: null, + items: [{ id: 9, filePath: null, lineStart: null, body: "Finding" }], + }); + + expect(text).toMatch(/asking the reviewer to verify/i); + expect(text).toMatch(/do not resolve persona-review feedback yourself/i); + }); + + it("keeps resolution guidance for human reviews", () => { + const text = buildReviewSubmittedPrompt({ + reviewId: 7, + reviewerName: "Human reviewer", + reviewerAgentId: null, + summary: null, + items: [{ id: 9, filePath: null, lineStart: null, body: "Finding" }], + }); + + expect(text).toMatch(/dispatch_review_resolve when an item is fixed/i); + expect(text).toContain("dispatch_review_reopen"); + expect(text).not.toMatch(/asking the reviewer to verify/i); + expect(text).not.toMatch( + /do not resolve persona-review feedback yourself/i + ); + }); + it("omits an absent summary when feedback items carry the review", () => { const text = buildReviewSubmittedPrompt({ reviewId: 7, diff --git a/apps/server/test/mcp-auth-integration.test.ts b/apps/server/test/mcp-auth-integration.test.ts index 7296aa05..4389c816 100644 --- a/apps/server/test/mcp-auth-integration.test.ts +++ b/apps/server/test/mcp-auth-integration.test.ts @@ -147,6 +147,7 @@ describe("MCP auth integration", () => { expect(response.body).toContain("dispatch_review_add_feedback"); expect(response.body).toContain("dispatch_review_list_feedback"); expect(response.body).toContain("dispatch_review_add_message"); + expect(response.body).toContain("dispatch_review_resolve"); expect(response.body).toContain("get_parent_context"); expect(response.body).not.toContain("dispatch_get_recheck_context"); expect(response.body).not.toContain("dispatch_complete_review"); diff --git a/apps/server/test/mcp-handlers.test.ts b/apps/server/test/mcp-handlers.test.ts index 0d7dfeee..2f8fa6d1 100644 --- a/apps/server/test/mcp-handlers.test.ts +++ b/apps/server/test/mcp-handlers.test.ts @@ -1606,6 +1606,7 @@ describe("createMcpHandlers", () => { { authorType: "agent", note: "addressed in latest commit", + resolverRole: "assignee", resolvedBy: "agt_test1", } ); @@ -1638,7 +1639,12 @@ describe("createMcpHandlers", () => { 10, "agt_test1", "ignored", - { authorType: "agent", note: null, resolvedBy: "agt_test1" } + { + authorType: "agent", + note: null, + resolverRole: "assignee", + resolvedBy: "agt_test1", + } ); }); }); diff --git a/apps/server/test/mcp-launch-persona-response.test.ts b/apps/server/test/mcp-launch-persona-response.test.ts index 31a666a5..0664740b 100644 --- a/apps/server/test/mcp-launch-persona-response.test.ts +++ b/apps/server/test/mcp-launch-persona-response.test.ts @@ -17,7 +17,7 @@ describe("buildLaunchPersonaResponseText", () => { expect(text).toContain("dispatch_review_submit"); expect(text).toContain("dispatch_review_list_feedback"); expect(text).toContain("dispatch_review_resolve"); - expect(text).toContain("dispatch_review_reopen"); + expect(text).toMatch(/asking the reviewer to verify/i); }); it("explains that clean approvals are tracked", () => { @@ -41,6 +41,14 @@ describe("buildLaunchPersonaResponseText", () => { expect(text).not.toContain("dispatch_await_review"); expect(text).not.toContain("dispatch_await_recheck"); expect(text).not.toMatch(/pollAgainInSeconds/i); + expect(text).toMatch(/do not poll, sleep/i); + expect(text).toMatch(/end this turn/i); + }); + + it("keeps persona feedback open until the reviewer verifies it", () => { + const text = buildLaunchPersonaResponseText(persona, agentId); + expect(text).toMatch(/do not call dispatch_review_resolve/i); + expect(text).toMatch(/reviewer will re-inspect/i); }); it("explains that the review signal arrives via structured injection", () => { diff --git a/apps/server/test/mcp-review-handlers.test.ts b/apps/server/test/mcp-review-handlers.test.ts index 2f80749f..6e07d224 100644 --- a/apps/server/test/mcp-review-handlers.test.ts +++ b/apps/server/test/mcp-review-handlers.test.ts @@ -200,6 +200,68 @@ describe("createReviewHandlers", () => { }) ); }); + + it("notifies the parent when the reviewer verifies and resolves a fix", async () => { + const { getReviewRecord, resolveReviewFeedbackItem } = + await import("../src/agents/reviews.js"); + vi.mocked(resolveReviewFeedbackItem).mockResolvedValueOnce({ + item: { id: 1, status: "resolved", resolution: "fixed" }, + reviewId: 10, + reviewStatus: "resolved", + } as never); + vi.mocked(getReviewRecord).mockResolvedValueOnce({ + id: 10, + agentId: "agt_parent", + assignedAgentId: "agt_parent", + reviewerAgentId: "agt_reviewer", + } as never); + + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + await handlers.resolveReviewFeedback("agt_reviewer", 1, "fixed"); + + expect(deps.publishUiEvent).toHaveBeenCalledWith({ + type: "review_feedback.updated", + agentId: "agt_parent", + feedbackItemId: 1, + }); + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_parent", + "item-state-prompt" + ); + }); + + it("authorizes review-role callers only as the reviewer", async () => { + const { resolveReviewFeedbackItem } = + await import("../src/agents/reviews.js"); + vi.mocked(resolveReviewFeedbackItem).mockResolvedValueOnce({ + item: { id: 1, status: "resolved", resolution: "fixed" }, + reviewId: 10, + reviewStatus: "resolved", + } as never); + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getAgent: vi.fn().mockResolvedValue({ + id: "agt_reviewer", + role: "review", + name: "reviewer", + cwd: "/repo", + }), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.resolveReviewFeedback("agt_reviewer", 1, "fixed"); + + expect(resolveReviewFeedbackItem).toHaveBeenCalledWith( + deps.pool, + 1, + "agt_reviewer", + "fixed", + expect.objectContaining({ resolverRole: "reviewer" }) + ); + }); }); describe("submitReview", () => { diff --git a/apps/server/test/review-feedback.test.ts b/apps/server/test/review-feedback.test.ts index ef76bd3e..1a8177bd 100644 --- a/apps/server/test/review-feedback.test.ts +++ b/apps/server/test/review-feedback.test.ts @@ -8,7 +8,11 @@ */ import { beforeEach, describe, expect, it } from "vitest"; -import { addThreadMessage } from "../src/agents/reviews.js"; +import { + addThreadMessage, + ReviewFeedbackResolutionConflictError, + resolveReviewFeedbackItem, +} from "../src/agents/reviews.js"; import { AGENT_REVIEW_REPLY_MAX_CHARS } from "../src/shared/review-limits.js"; import { useInjectApp } from "./helpers/inject-app.js"; @@ -232,6 +236,158 @@ describe("PATCH /api/v1/agents/:id/reviews/items/:itemId", () => { expect(response.statusCode).toBe(200); expect(response.json().item.resolution).toBe("fixed"); }); + + it("returns the current item when a stale resolution conflicts", async () => { + const review = await createReview(AGENT_ID, [{ comment: "Fix this" }]); + const itemId = review.items[0].id; + + const first = await ctx.app.inject({ + method: "PATCH", + url: `/api/v1/agents/${AGENT_ID}/reviews/items/${itemId}`, + headers: { cookie: sessionCookie, "content-type": "application/json" }, + payload: { resolution: "fixed" }, + }); + expect(first.statusCode).toBe(200); + + const stale = await ctx.app.inject({ + method: "PATCH", + url: `/api/v1/agents/${AGENT_ID}/reviews/items/${itemId}`, + headers: { cookie: sessionCookie, "content-type": "application/json" }, + payload: { resolution: "dismissed" }, + }); + + expect(stale.statusCode).toBe(409); + expect(stale.json().item).toMatchObject({ + id: itemId, + status: "resolved", + resolution: "fixed", + }); + }); + + it("allows the persona reviewer to resolve an item after verification", async () => { + const review = await createReview(AGENT_ID, [{ comment: "Fix this" }]); + const reviewerId = "agt_reviewfb_reviewer"; + await ctx.pool.query( + `INSERT INTO agents (id, name, type, role, status, cwd, parent_agent_id, full_access) + VALUES ($1, 'persona-reviewer', 'codex', 'review', 'running', '/tmp', $2, false)`, + [reviewerId, AGENT_ID] + ); + await ctx.pool.query( + "UPDATE reviews SET reviewer_type = 'agent', reviewer_agent_id = $1 WHERE id = $2", + [reviewerId, review.id] + ); + + const result = await resolveReviewFeedbackItem( + ctx.pool, + review.items[0].id, + reviewerId, + "fixed", + { + resolvedBy: reviewerId, + authorType: "agent", + resolverRole: "reviewer", + } + ); + + expect(result?.item.status).toBe("resolved"); + expect(result?.item.resolvedBy).toBe(reviewerId); + expect(result?.reviewStatus).toBe("resolved"); + }); + + it("allows an assignee agent to resolve persona feedback when needed", async () => { + const review = await createReview(AGENT_ID, [{ comment: "Fix this" }]); + const reviewerId = "agt_reviewfb_reviewer"; + await ctx.pool.query( + `INSERT INTO agents (id, name, type, role, status, cwd, parent_agent_id, full_access) + VALUES ($1, 'persona-reviewer', 'codex', 'review', 'running', '/tmp', $2, false)`, + [reviewerId, AGENT_ID] + ); + await ctx.pool.query( + "UPDATE reviews SET reviewer_type = 'agent', reviewer_agent_id = $1 WHERE id = $2", + [reviewerId, review.id] + ); + + const result = await resolveReviewFeedbackItem( + ctx.pool, + review.items[0].id, + AGENT_ID, + "fixed", + { + resolvedBy: AGENT_ID, + authorType: "agent", + resolverRole: "assignee", + } + ); + + expect(result?.item.status).toBe("resolved"); + expect(result?.item.resolvedBy).toBe(AGENT_ID); + }); + + it("prevents a review-role caller from resolving as an assignee", async () => { + const review = await createReview(AGENT_ID, [{ comment: "Fix this" }]); + + const result = await resolveReviewFeedbackItem( + ctx.pool, + review.items[0].id, + AGENT_ID, + "fixed", + { + resolvedBy: AGENT_ID, + authorType: "agent", + resolverRole: "reviewer", + } + ); + + expect(result).toBeNull(); + }); + + it("rejects a stale second actor without overwriting the resolution", async () => { + const review = await createReview(AGENT_ID, [{ comment: "Fix this" }]); + const reviewerId = "agt_reviewfb_reviewer"; + const itemId = review.items[0].id; + await ctx.pool.query( + `INSERT INTO agents (id, name, type, role, status, cwd, parent_agent_id, full_access) + VALUES ($1, 'persona-reviewer', 'codex', 'review', 'running', '/tmp', $2, false)`, + [reviewerId, AGENT_ID] + ); + await ctx.pool.query( + "UPDATE reviews SET reviewer_type = 'agent', reviewer_agent_id = $1 WHERE id = $2", + [reviewerId, review.id] + ); + + await resolveReviewFeedbackItem(ctx.pool, itemId, reviewerId, "fixed", { + resolvedBy: reviewerId, + authorType: "agent", + resolverRole: "reviewer", + }); + + await expect( + resolveReviewFeedbackItem(ctx.pool, itemId, AGENT_ID, "dismissed", { + resolvedBy: AGENT_ID, + authorType: "human", + resolverRole: "human", + }) + ).rejects.toBeInstanceOf(ReviewFeedbackResolutionConflictError); + + const item = await ctx.pool.query<{ + resolution: string; + resolvedBy: string; + }>( + `SELECT resolution, resolved_by AS "resolvedBy" + FROM review_feedback_items WHERE id = $1`, + [itemId] + ); + const messages = await ctx.pool.query<{ count: number }>( + `SELECT COUNT(*)::int AS count FROM review_thread_messages + WHERE feedback_item_id = $1 AND type = 'resolution'`, + [itemId] + ); + expect(item.rows[0]).toEqual({ + resolution: "fixed", + resolvedBy: reviewerId, + }); + expect(messages.rows[0]?.count).toBe(1); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/web/src/components/app/docs-sections/personas.tsx b/apps/web/src/components/app/docs-sections/personas.tsx index 58f58019..7eb80311 100644 --- a/apps/web/src/components/app/docs-sections/personas.tsx +++ b/apps/web/src/components/app/docs-sections/personas.tsx @@ -98,8 +98,10 @@ issues caused or worsened by this diff.`} Reviews use the same flexible feedback-item model whether the reviewer is a human or an agent. The parent reads the current state with{" "} dispatch_review_list_feedback, converses through each - item's thread, and sets the outcome with{" "} - dispatch_review_resolve. Use{" "} + item's thread, and asks the persona reviewer to verify each fix. The + reviewer re-inspects the change, resolves a completed item with{" "} + dispatch_review_resolve, or replies with further + instructions while leaving it open. Use{" "} dispatch_review_reopen if a resolved concern needs more work. Review status is derived automatically: open while any item is open, partially resolved when only some are resolved, and resolved @@ -109,7 +111,8 @@ issues caused or worsened by this diff.`} Every review action is push-based. Dispatch injects clearly delimited blocks when a review is submitted, a thread receives a message, or an item is resolved or reopened. This keeps both agents aware without - using direct messages or imposing a fixed round/recheck sequence. + polling, sleeping, using direct messages, or imposing a fixed + round/recheck sequence.

diff --git a/apps/web/src/components/app/docs-sections/tools.tsx b/apps/web/src/components/app/docs-sections/tools.tsx index e32fd287..f2c1add3 100644 --- a/apps/web/src/components/app/docs-sections/tools.tsx +++ b/apps/web/src/components/app/docs-sections/tools.tsx @@ -160,7 +160,8 @@ export function ToolsContent() {
  • dispatch_review_resolve — resolve a review feedback - item as fixed or dismissed + item as fixed or dismissed; persona reviewers use it after verifying + the parent's fix
  • dispatch_review_add_message — reply to a review