Correlate concurrent project review checks

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
2026-11-14 05:12:27 +00:00 · 2026-08-26 23:36:22 -07:00
parent ecaced02a1
commit 018252347f
5 changed files with 227 additions and 35 deletions
--- 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;
--- 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.",
--- 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));
 });

--- 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<string, unknown>): 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 (
-            <article className="space-y-3 px-6 py-4" key={check.id}>
+            <article
+              className="space-y-3 px-6 py-4"
+              data-testid="project-review-check"
+              key={check.id}
+            >
              <div className="flex flex-wrap items-start justify-between gap-3">
                <div className="min-w-0 flex-1 space-y-1">
                  <div className="flex flex-wrap items-center gap-2">
@@ -845,6 +866,7 @@ export function ProjectReviewChecks({
                  checkId={check.id}
                  onResult={recordResult}
                  opener={run.opener}
+                  requestId={run.requestId ?? null}
                />
              ) : null}
            </article>
--- 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(() => {