diff --git a/desktop/src/features/projects/lib/projectReviewChecks.d.mts b/desktop/src/features/projects/lib/projectReviewChecks.d.mts index aa9d0b82b6..355b235fe7 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.d.mts +++ b/desktop/src/features/projects/lib/projectReviewChecks.d.mts @@ -15,6 +15,7 @@ export type ProjectReviewCheckFinding = { }; export type ProjectReviewCheckResult = { + requestId?: string; conclusion: ProjectReviewCheckConclusion; summary: string; findings: ProjectReviewCheckFinding[]; @@ -39,6 +40,7 @@ export function buildProjectReviewCheckPrompt(input: { reviewId: string; reviewLink: string; reviewTitle: string; + requestId: string; commit: string | null; branchName: string | null; targetBranch: string | null; diff --git a/desktop/src/features/projects/lib/projectReviewChecks.mjs b/desktop/src/features/projects/lib/projectReviewChecks.mjs index 65c80f0e3f..06bd1d4c9e 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.mjs @@ -196,10 +196,19 @@ export function parseProjectReviewCheckResult(content) { const conclusion = normalizedConclusion(parsed.conclusion); const summary = normalizedText(parsed.summary, 4_000); if (!conclusion || !summary) continue; + const requestId = normalizedText( + parsed.request_id ?? parsed.requestId, + 200, + ); const findings = Array.isArray(parsed.findings) ? parsed.findings.map(normalizedFinding).filter(Boolean).slice(0, 20) : []; - return { conclusion, summary, findings }; + return { + ...(requestId ? { requestId } : {}), + conclusion, + summary, + findings, + }; } catch { // Agents occasionally wrap the JSON with prose. Try the next bounded // candidate instead of accepting an unstructured response as a result. @@ -215,6 +224,7 @@ export function buildProjectReviewCheckPrompt({ reviewId, reviewLink, reviewTitle, + requestId, commit, branchName, targetBranch, @@ -224,6 +234,7 @@ export function buildProjectReviewCheckPrompt({ `Repository address: ${JSON.stringify(repoAddress)}`, `Review: ${JSON.stringify(reviewTitle)} (${reviewId})`, `Review link: ${reviewLink}`, + `Request id: ${JSON.stringify(requestId)}`, `Commit under review: ${commit ?? "unknown"}`, `Branch: ${branchName ?? "unknown"} -> ${targetBranch ?? "unknown"}`, ]; @@ -239,7 +250,20 @@ export function buildProjectReviewCheckPrompt({ "", "End your response with exactly this marker and one JSON object:", PROJECT_REVIEW_CHECK_RESULT_MARKER, - '{"conclusion":"approved|fix-recommended","summary":"short explanation","findings":[{"title":"concise actionable fix","detail":"why this matters and what to change","file":"relative/path.ext","line":123}]}', + JSON.stringify({ + request_id: requestId, + conclusion: "fix-recommended", + summary: "short explanation", + findings: [ + { + title: "concise actionable fix", + detail: "why this matters and what to change", + file: "relative/path.ext", + line: 123, + }, + ], + }), + "Echo request_id exactly as provided so this result is matched to the correct concurrent check.", "Return an empty findings array when approved. For every recommended fix, return one finding object.", "Use null for file or line when the finding is not tied to a precise source location.", "Use approved only when no material fix is recommended. Use fix-recommended otherwise.", diff --git a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs index 1f51066998..8b60e0b926 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs @@ -67,10 +67,11 @@ test("parses an approved structured agent result", () => { ${PROJECT_REVIEW_CHECK_RESULT_MARKER} \`\`\`json -{"conclusion":"approved","summary":"The change follows the contract.","findings":[]} +{"request_id":"request-123","conclusion":"approved","summary":"The change follows the contract.","findings":[]} \`\`\``); assert.deepEqual(result, { + requestId: "request-123", conclusion: "approved", summary: "The change follows the contract.", findings: [], @@ -171,6 +172,7 @@ test("builds a review-only prompt pinned to the exact commit", () => { reviewId: "review-event", reviewLink: "buzz://pr?id=review-event", reviewTitle: "Add checks", + requestId: "request-123", commit: "abc123", branchName: "checks", targetBranch: "main", @@ -179,6 +181,8 @@ test("builds a review-only prompt pinned to the exact commit", () => { assert.match(prompt, /Review only; do not modify code/); assert.match(prompt, /Commit under review: abc123/); assert.match(prompt, /Frontend quality/); + assert.match(prompt, /"request_id":"request-123"/); + assert.match(prompt, /Echo request_id exactly/); assert.match(prompt, new RegExp(PROJECT_REVIEW_CHECK_RESULT_MARKER)); }); diff --git a/desktop/src/features/projects/ui/ProjectReviewChecks.tsx b/desktop/src/features/projects/ui/ProjectReviewChecks.tsx index 610709e7c4..497b6fb48a 100644 --- a/desktop/src/features/projects/ui/ProjectReviewChecks.tsx +++ b/desktop/src/features/projects/ui/ProjectReviewChecks.tsx @@ -71,6 +71,7 @@ type ReviewCheckStatus = "idle" | "running" | "completed" | "failed"; type ReviewCheckRun = { agentPubkey: string | null; + requestId?: string; status: ReviewCheckStatus; targetCommit?: string | null; channelId?: string; @@ -136,6 +137,9 @@ function normalizeStoredRuns(value: Record): ReviewCheckRuns { ...(typeof raw.channelId === "string" ? { channelId: raw.channelId } : {}), + ...(typeof raw.requestId === "string" + ? { requestId: raw.requestId } + : {}), ...(opener ? { opener } : {}), ...(parsedResult ? { result: parsedResult } : {}), ...(typeof raw.resultCreatedAt === "number" @@ -269,6 +273,7 @@ function ReviewCheckResultObserver({ checkId, onResult, opener, + requestId, }: { agentPubkey: string; channel: Channel | null; @@ -279,6 +284,7 @@ function ReviewCheckResultObserver({ createdAt: number, ) => void; opener: ProjectsConversationOpener; + requestId: string | null; }) { useChannelSubscription(channel); const messagesQuery = useChannelMessagesQuery(channel); @@ -310,10 +316,18 @@ function ReviewCheckResultObserver({ ); for (const event of events) { const result = parseProjectReviewCheckResult(event.content); - if (result) return { createdAt: event.created_at, result }; + if (result && (!requestId || result.requestId === requestId)) { + return { createdAt: event.created_at, result }; + } } return null; - }, [agentPubkey, messagesQuery.data, opener, threadReplies.events]); + }, [ + agentPubkey, + messagesQuery.data, + opener, + requestId, + threadReplies.events, + ]); React.useEffect(() => { if (observedResult) { @@ -584,6 +598,7 @@ export function ProjectReviewChecks({ async (check: ProjectReviewCheckDefinition, agent: ReviewCheckAgent) => { const expectedStorageKey = storageKey; if (!expectedStorageKey || !relayScope || !signerScope) return; + const requestId = crypto.randomUUID(); if (!agent.isManaged && !agent.isActive) { toast.error( `${agent.name} is offline and cannot be started from this dev build.`, @@ -654,6 +669,7 @@ export function ProjectReviewChecks({ reviewId: pullRequest.id, reviewLink: pullRequestShareLink(pullRequest) ?? "unavailable", reviewTitle: pullRequest.title, + requestId, commit: targetCommit, branchName: pullRequest.branchName, targetBranch: pullRequest.targetBranch, @@ -681,6 +697,7 @@ export function ProjectReviewChecks({ createdAt: sent.createdAt, eventId: sent.eventId, }, + requestId, result: undefined, resultCreatedAt: undefined, status: "running", @@ -767,7 +784,11 @@ export function ProjectReviewChecks({ ? (channelsById.get(run.channelId) ?? null) : null; return ( -
+
@@ -845,6 +866,7 @@ export function ProjectReviewChecks({ checkId={check.id} onResult={recordResult} opener={run.opener} + requestId={run.requestId ?? null} /> ) : null}
diff --git a/desktop/tests/e2e/project-pr-review.spec.ts b/desktop/tests/e2e/project-pr-review.spec.ts index 2d1f9dbe87..175ae5bcc8 100644 --- a/desktop/tests/e2e/project-pr-review.spec.ts +++ b/desktop/tests/e2e/project-pr-review.spec.ts @@ -103,6 +103,12 @@ function pullRequestRowByAuthor( }); } +function requestIdFromCheckPrompt(content: string | undefined): string { + const requestId = content?.match(/"request_id":"([^"]+)"/)?.[1]; + expect(requestId).toBeTruthy(); + return requestId ?? ""; +} + async function expectLocalRepositoryOpenAction( page: import("@playwright/test").Page, ) { @@ -131,8 +137,8 @@ test("review checks dispatch to an agent authorized for the relay identity", asy const checks = page.getByTestId("project-review-checks"); await expect(checks).toBeVisible(); - await expect(checks.locator("article")).toHaveCount(3); - const interfaceCheck = checks.locator("article").first(); + await expect(checks.getByTestId("project-review-check")).toHaveCount(3); + const interfaceCheck = checks.getByTestId("project-review-check").first(); await interfaceCheck.getByTestId("project-review-check-agent-picker").click(); await page.getByRole("menuitemradio", { name: /charlie/i }).click(); await interfaceCheck @@ -166,6 +172,7 @@ test("review checks dispatch to an agent authorized for the relay identity", asy expect(sentCheck?.content).toContain("Interface & design system"); expect(sentCheck?.content).toContain("Commit under review:"); expect(sentCheck?.mentionPubkeys).toContain(RELAY_REVIEW_AGENT_PUBKEY); + const requestId = requestIdFromCheckPrompt(sentCheck?.content); await expect .poll(() => page.evaluate(() => @@ -175,32 +182,36 @@ test("review checks dispatch to an agent authorized for the relay identity", asy .toBe(true); await waitForMockLiveSubscription(page, "DM"); - await page.evaluate((agentPubkey) => { - window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ - channelName: "DM", - content: `BUZZ_CHECK_RESULT_V1\n${JSON.stringify({ - conclusion: "fix-recommended", - summary: "Two interface fixes are needed before approval.", - findings: [ - { - title: "Use the shared button primitive", - detail: "The custom control misses the standard focus treatment.", - file: "desktop/src/features/projects/ui/ProjectReviewChecks.tsx", - line: 742, - }, - { - title: "Cover the disabled interaction state", - detail: "Add a UI assertion for the pending check state.", - file: "desktop/tests/e2e/project-pr-review.spec.ts", - line: 118, - }, - ], - })}`, - createdAt: Math.floor(Date.now() / 1_000) + 1, - kind: 9, - pubkey: agentPubkey, - }); - }, RELAY_REVIEW_AGENT_PUBKEY); + await page.evaluate( + ({ agentPubkey, requestId }) => { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "DM", + content: `BUZZ_CHECK_RESULT_V1\n${JSON.stringify({ + request_id: requestId, + conclusion: "fix-recommended", + summary: "Two interface fixes are needed before approval.", + findings: [ + { + title: "Use the shared button primitive", + detail: "The custom control misses the standard focus treatment.", + file: "desktop/src/features/projects/ui/ProjectReviewChecks.tsx", + line: 742, + }, + { + title: "Cover the disabled interaction state", + detail: "Add a UI assertion for the pending check state.", + file: "desktop/tests/e2e/project-pr-review.spec.ts", + line: 118, + }, + ], + })}`, + createdAt: Math.floor(Date.now() / 1_000) + 1, + kind: 9, + pubkey: agentPubkey, + }); + }, + { agentPubkey: RELAY_REVIEW_AGENT_PUBKEY, requestId }, + ); const result = interfaceCheck.getByTestId("project-review-check-result"); await expect(result.getByRole("heading")).toHaveText("2 fixes recommended"); @@ -248,7 +259,7 @@ test("review checks dispatch to a running local managed agent", async ({ await page.getByRole("button", { name: "Checks", exact: true }).click(); const checks = page.getByTestId("project-review-checks"); - const interfaceCheck = checks.locator("article").first(); + const interfaceCheck = checks.getByTestId("project-review-check").first(); await expect( checks.getByTestId("project-review-check-agent-picker"), ).toHaveCount(3); @@ -287,6 +298,135 @@ test("review checks dispatch to a running local managed agent", async ({ expect(commands).not.toContain("start_managed_agent"); }); +test("concurrent review checks only accept their correlated result", async ({ + page, +}) => { + await enableProjectsFeature(page); + await installMockBridge(page, { + managedAgents: [ + { + pubkey: RELAY_REVIEW_AGENT_PUBKEY, + name: "Roof", + status: "running", + respondTo: "allowlist", + respondToAllowlist: [], + }, + ], + }); + await openBuzzProject(page); + + await page.getByRole("tab", { name: "Review" }).click(); + const aliceRow = pullRequestRowByAuthor(page, "alice").first(); + await expect(aliceRow).toBeVisible({ timeout: 10_000 }); + await aliceRow.getByRole("button", { name: /^#/ }).click(); + await page.getByRole("button", { name: "Checks", exact: true }).click(); + + const checks = page.getByTestId("project-review-check"); + await expect(checks).toHaveCount(3); + for (const check of await checks.all()) { + await check.getByRole("button", { name: "Run", exact: true }).click(); + } + await expect(checks.filter({ hasText: "Running" })).toHaveCount(3); + + await expect + .poll(() => + page.evaluate( + () => + window.__BUZZ_E2E_COMMAND_PAYLOADS__?.filter( + (entry) => + entry.command === "send_channel_message" && + typeof (entry.payload as { content?: unknown })?.content === + "string" && + (entry.payload as { content: string }).content.includes( + "BUZZ_CHECK_RESULT_V1", + ), + ).length ?? 0, + ), + ) + .toBe(3); + + const prompts = await page.evaluate( + () => + window.__BUZZ_E2E_COMMAND_PAYLOADS__ + ?.filter( + (entry) => + entry.command === "send_channel_message" && + typeof (entry.payload as { content?: unknown })?.content === + "string" && + (entry.payload as { content: string }).content.includes( + "BUZZ_CHECK_RESULT_V1", + ), + ) + .map((entry) => (entry.payload as { content: string }).content) ?? [], + ); + const requestIdFor = (checkName: string) => + requestIdFromCheckPrompt( + prompts.find((prompt) => prompt.includes(`"${checkName}"`)), + ); + const responses = [ + { + request_id: requestIdFor("Frontend quality"), + conclusion: "fix-recommended", + summary: "Frontend-specific result.", + findings: [ + { + title: "Frontend-only finding", + detail: "This must stay on the frontend check.", + file: "desktop/src/frontend.tsx", + line: 12, + }, + ], + }, + { + request_id: requestIdFor("Test quality"), + conclusion: "approved", + summary: "Test-specific result.", + findings: [], + }, + { + request_id: requestIdFor("Interface & design system"), + conclusion: "approved", + summary: "Interface-specific result.", + findings: [], + }, + ]; + + await waitForMockLiveSubscription(page, "DM"); + const targetIndexes = [1, 2, 0]; + for (const [index, response] of responses.entries()) { + await page.evaluate( + ({ agentPubkey, createdAt, response }) => { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "DM", + content: `BUZZ_CHECK_RESULT_V1\n${JSON.stringify(response)}`, + createdAt, + kind: 9, + pubkey: agentPubkey, + }); + }, + { + agentPubkey: RELAY_REVIEW_AGENT_PUBKEY, + createdAt: Math.floor(Date.now() / 1_000) + index + 1, + response, + }, + ); + await expect(checks.nth(targetIndexes[index])).toContainText( + response.summary, + ); + if (index === 0) { + await expect(checks.nth(0)).toContainText("Running"); + await expect(checks.nth(2)).toContainText("Running"); + } + } + + await expect(checks.nth(0)).toContainText("Interface-specific result."); + await expect(checks.nth(1)).toContainText("Frontend-specific result."); + await expect(checks.nth(1)).toContainText("Frontend-only finding"); + await expect(checks.nth(2)).toContainText("Test-specific result."); + await expect(checks.nth(0)).not.toContainText("Frontend-specific result."); + await expect(checks.nth(2)).not.toContainText("Frontend-specific result."); +}); + test("same-second request changes supersedes approval", async ({ page }) => { await enableProjectsFeature(page); await page.addInitScript(() => {