From 4fe6679b60aee8e7966ba90aebf89a4b210c10ea Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 27 Aug 2026 14:21:05 -0700 Subject: [PATCH] Refine prototype review check tracks Signed-off-by: Taylor Ho --- .../projects/lib/projectReviewChecks.mjs | 43 +++++++++--- .../projects/lib/projectReviewChecks.test.mjs | 28 ++++++-- desktop/tests/e2e/project-pr-review.spec.ts | 70 +++++++++++++------ 3 files changed, 106 insertions(+), 35 deletions(-) diff --git a/desktop/src/features/projects/lib/projectReviewChecks.mjs b/desktop/src/features/projects/lib/projectReviewChecks.mjs index 67ab9a2900..272ea5ede7 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.mjs @@ -8,24 +8,47 @@ export const PROJECT_REVIEW_CHECK_RESULT_MARKER = "BUZZ_CHECK_RESULT_V1"; export const DEFAULT_PROJECT_REVIEW_CHECKS = [ { id: "interface", - name: "Interface & design system", + name: "Interface & frontend", description: - "Reviews user-facing changes against the design system and their backend contracts.", + "Reviews the user-facing experience and frontend implementation as one surface.", instructions: [ - "Inspect user-facing changes and the backend behavior that supplies them.", - "Check design-system component usage, interaction states, accessibility, and visual consistency.", + "Inspect the user-facing behavior and frontend implementation changed by this review.", + "Check design-system usage, interaction states, accessibility, loading and error states, performance, and visual consistency.", "Flag hand-rolled UI where the project design system already provides the intended primitive.", ], }, { - id: "frontend", - name: "Frontend quality", + id: "code-correctness", + name: "Code correctness", description: - "Reviews frontend correctness, state boundaries, accessibility, and maintainability.", + "Checks whether the changed code behaves correctly in normal, edge, and failure paths.", instructions: [ - "Inspect the frontend implementation changed by this review.", - "Check data flow, loading and error states, accessibility, performance, and maintainability.", - "Only report issues introduced by or made materially worse by this review.", + "Trace the changed behavior through its callers, data flow, and external contracts.", + "Look for logic errors, invalid state transitions, missed edge cases, unsafe error handling, races, and resource-lifecycle defects.", + "Report behavior-level defects introduced by or made materially worse by this review; leave codebase conventions to their own check.", + ], + }, + { + id: "codebase-patterns", + name: "Codebase patterns", + description: + "Checks whether the implementation follows the architecture and established patterns of this codebase.", + instructions: [ + "Compare the change with contributor guidance and analogous code already in the repository.", + "Check architectural boundaries, shared abstractions, naming, error handling, and framework idioms for consistency with established patterns.", + "Report departures that create meaningful maintenance or integration risk; leave functional defects to the code correctness check.", + ], + }, + { + id: "historical-intent", + name: "Historical intent", + description: + "Explores the history of the changed area to identify intentional or accidental shifts in project philosophy.", + instructions: [ + "Inspect relevant line and path history with git blame and git log, then follow related commits into pull requests or review discussion when available.", + "Look for prior design rationale, analogous changes, reversions, and decisions from maintainers or frequent contributors to this area.", + "Evaluate whether this review continues, deliberately revises, or accidentally contradicts the philosophy behind the existing implementation; clearly distinguish historical evidence from inference.", + "Do not reject a change merely because it differs from precedent. Recommend a fix only when an unexplained philosophical shift creates material product, architecture, or maintenance risk.", ], }, { diff --git a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs index 65d0c3276e..e9ffe3b689 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs @@ -12,13 +12,31 @@ import { writeProjectReviewCheckRuns, } from "./projectReviewChecks.mjs"; -test("starter checks cover interface, frontend, and test quality", () => { +test("starter checks keep experience, correctness, patterns, history, and tests independent", () => { assert.deepEqual( DEFAULT_PROJECT_REVIEW_CHECKS.map((check) => check.id), - ["interface", "frontend", "test-quality"], + [ + "interface", + "code-correctness", + "codebase-patterns", + "historical-intent", + "test-quality", + ], ); }); +test("historical intent check investigates precedent without treating it as a veto", () => { + const check = DEFAULT_PROJECT_REVIEW_CHECKS.find( + (candidate) => candidate.id === "historical-intent", + ); + assert.ok(check); + const instructions = check.instructions.join("\n"); + assert.match(instructions, /git blame and git log/); + assert.match(instructions, /pull requests or review discussion/); + assert.match(instructions, /historical evidence from inference/); + assert.match(instructions, /Do not reject a change merely because/); +}); + test("parses an approved structured agent result", () => { const result = parseProjectReviewCheckResult(`Analysis first. @@ -220,7 +238,7 @@ 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, /Code correctness/); assert.match(prompt, /"request_id":"request-123"/); assert.match(prompt, /Echo request_id exactly/); assert.match(prompt, /process every event independently/); @@ -256,7 +274,9 @@ test("local check state is isolated by relay, signer, repo, and review", () => { getItem: (key) => entries.get(key) ?? null, setItem: (key, value) => entries.set(key, value), }; - const runs = { frontend: { agentPubkey: "agent", status: "idle" } }; + const runs = { + "code-correctness": { agentPubkey: "agent", status: "idle" }, + }; writeProjectReviewCheckRuns(storage, first, runs); assert.deepEqual(readProjectReviewCheckRuns(storage, first), runs); assert.deepEqual(readProjectReviewCheckRuns(storage, second), {}); diff --git a/desktop/tests/e2e/project-pr-review.spec.ts b/desktop/tests/e2e/project-pr-review.spec.ts index 84126aec8f..5b8a32ab29 100644 --- a/desktop/tests/e2e/project-pr-review.spec.ts +++ b/desktop/tests/e2e/project-pr-review.spec.ts @@ -167,7 +167,7 @@ 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.getByTestId("project-review-check")).toHaveCount(3); + await expect(checks.getByTestId("project-review-check")).toHaveCount(5); const interfaceCheck = checks.getByTestId("project-review-check").first(); await page.getByTestId("project-review-debug-harness-trigger").click(); await page @@ -201,7 +201,7 @@ test("review checks dispatch to an agent authorized for the relay identity", asy | { content?: string; mentionPubkeys?: string[] } | undefined; }); - expect(sentCheck?.content).toContain("Interface & design system"); + expect(sentCheck?.content).toContain("Interface & frontend"); expect(sentCheck?.content).toContain("Commit under review:"); expect(sentCheck?.content).toContain("buzz messages send-diff"); expect(sentCheck?.mentionPubkeys).toContain(RELAY_REVIEW_AGENT_PUBKEY); @@ -449,11 +449,11 @@ test("concurrent review checks only accept their correlated result", async ({ await page.getByRole("button", { name: "Checks", exact: true }).click(); const checks = page.getByTestId("project-review-check"); - await expect(checks).toHaveCount(3); + await expect(checks).toHaveCount(5); 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(checks.filter({ hasText: "Running" })).toHaveCount(5); await expect .poll(() => @@ -470,7 +470,7 @@ test("concurrent review checks only accept their correlated result", async ({ ).length ?? 0, ), ) - .toBe(3); + .toBe(5); const prompts = await page.evaluate( () => @@ -490,12 +490,20 @@ test("concurrent review checks only accept their correlated result", async ({ requestIdFromCheckPrompt( prompts.find((prompt) => prompt.includes(`"${checkName}"`)), ); - const interfaceRequestId = requestIdFor("Interface & design system"); - const frontendRequestId = requestIdFor("Frontend quality"); + const interfaceRequestId = requestIdFor("Interface & frontend"); + const correctnessRequestId = requestIdFor("Code correctness"); + const patternsRequestId = requestIdFor("Codebase patterns"); + const historicalIntentRequestId = requestIdFor("Historical intent"); const testRequestId = requestIdFor("Test quality"); const openerByRequestId = new Map( await Promise.all( - [interfaceRequestId, frontendRequestId, testRequestId].map( + [ + interfaceRequestId, + correctnessRequestId, + patternsRequestId, + historicalIntentRequestId, + testRequestId, + ].map( async (requestId) => [requestId, await checkOpenerEventId(page, requestId)] as const, ), @@ -503,14 +511,14 @@ test("concurrent review checks only accept their correlated result", async ({ ); const responses = [ { - request_id: frontendRequestId, + request_id: patternsRequestId, conclusion: "fix-recommended", - summary: "Frontend-specific result.", + summary: "Pattern-specific result.", findings: [ { - title: "Frontend-only finding", - detail: "This must stay on the frontend check.", - file: "desktop/src/frontend.tsx", + title: "Pattern-only finding", + detail: "This must stay on the codebase patterns check.", + file: "desktop/src/pattern.ts", line: 12, }, ], @@ -527,6 +535,18 @@ test("concurrent review checks only accept their correlated result", async ({ summary: "Interface-specific result.", findings: [], }, + { + request_id: correctnessRequestId, + conclusion: "approved", + summary: "Correctness-specific result.", + findings: [], + }, + { + request_id: historicalIntentRequestId, + conclusion: "approved", + summary: "Historical-intent-specific result.", + findings: [], + }, ]; await waitForMockLiveSubscription(page, "DM"); @@ -547,9 +567,9 @@ test("concurrent review checks only accept their correlated result", async ({ response: responses[0], }, ); - await expect(checks.nth(1)).toContainText("Running"); + await expect(checks.nth(2)).toContainText("Running"); - const targetIndexes = [1, 2, 0]; + const targetIndexes = [2, 4, 0, 1, 3]; for (const [index, response] of responses.entries()) { await page.evaluate( ({ agentPubkey, createdAt, openerEventId, response }) => { @@ -574,16 +594,24 @@ test("concurrent review checks only accept their correlated result", async ({ ); if (index === 0) { await expect(checks.nth(0)).toContainText("Running"); - await expect(checks.nth(2)).toContainText("Running"); + await expect(checks.nth(1)).toContainText("Running"); + await expect(checks.nth(3)).toContainText("Running"); + await expect(checks.nth(4)).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."); + await expect(checks.nth(1)).toContainText("Correctness-specific result."); + await expect(checks.nth(2)).toContainText("Pattern-specific result."); + await expect(checks.nth(2)).toContainText("Pattern-only finding"); + await expect(checks.nth(3)).toContainText( + "Historical-intent-specific result.", + ); + await expect(checks.nth(4)).toContainText("Test-specific result."); + await expect(checks.nth(0)).not.toContainText("Pattern-specific result."); + await expect(checks.nth(1)).not.toContainText("Pattern-specific result."); + await expect(checks.nth(3)).not.toContainText("Pattern-specific result."); + await expect(checks.nth(4)).not.toContainText("Pattern-specific result."); }); test("same-second request changes supersedes approval", async ({ page }) => {