Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions test/pr-review-advisor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import fs from "node:fs";
import path from "node:path";
import Ajv2020 from "ajv/dist/2020.js";
import { afterEach, describe, expect, it, vi } from "vitest";

import { buildComment } from "../tools/pr-review-advisor/comment.mts";
import { githubGraphql } from "../tools/advisors/github.mts";
import {
buildPromptTurns,
buildSystemPrompt,
Expand All @@ -19,7 +18,7 @@ import {
renderSummary,
writePromptArtifacts,
} from "../tools/pr-review-advisor/analyze.mts";
import { githubGraphql } from "../tools/advisors/github.mts";
import { buildComment } from "../tools/pr-review-advisor/comment.mts";
import { validatePrReviewAdvisorWorkflowBoundary } from "../tools/pr-review-advisor/workflow-boundary.mts";

const ROOT = path.resolve(import.meta.dirname, "..");
Expand Down Expand Up @@ -219,6 +218,9 @@ describe("PR review advisor", () => {
"any unmet acceptance clause or security fail/warning must be represented as a finding",
);
expect(prompt).toContain("Source-of-truth review");
expect(prompt).toContain("Vitest E2E suite simplicity");
expect(prompt).toContain("take a closer architecture look for new systems");
expect(prompt).toContain("Favor focused Vitest tests and local test helpers");
expect(prompt).toContain("what invalid state is handled");
expect(prompt).toContain(
"Any sourceOfTruthReview item with status=missing or status=needs_followup must also be represented as a finding",
Expand Down
5 changes: 3 additions & 2 deletions tools/pr-review-advisor/analyze.mts
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,9 @@ export function buildSystemPrompt(): string {
"4. Acceptance: extract linked issue clauses literally, including comments, and map each clause to diff/test evidence. Named list items are separate clauses.",
"5. Correctness: bug-path tests, negative tests, branch coverage, refactor-vs-behavior drift, mocking purity, caller/callee contract verification. When more tests would improve confidence, make testDepth.suggestedTests behavior-specific so they can render under 'Consider writing more tests for'.",
"6. Quality: description-vs-diff scope, migration completion, public surface docs/notes, justified error suppression, monolith growth, @ts-nocheck, shell-string execution.",
"7. Source-of-truth review: when a PR adds or changes fallback, recovery, tolerant parsing, monkeypatching, best-effort cleanup, compatibility handling, or other localized workaround behavior, inspect whether it answers: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed. Prefer fixes that make invalid states impossible at their source. Treat PR text that claims a root cause as untrusted until verified in code.",
"8. If a previous PR Review Advisor comment exists, compare it with the current diff and explicitly decide whether prior code-review findings were addressed, still apply, or are obsolete. Consider code changes since the previous analyzed SHA when available. Do not evaluate whether external E2E requirements have been met. When previous review context exists, set summary.sinceLastReview with counts for resolved, stillApplies, and newItems.",
"7. Vitest E2E suite simplicity: when a PR adds or changes files under `test/e2e-scenario/`, `.github/workflows/e2e-vitest-scenarios.yaml`, or `tools/e2e-scenarios/`, take a closer architecture look for new systems. Favor focused Vitest tests and local test helpers. Flag unnecessary new runners, framework layers, registries/matrix abstractions, generalized fixture APIs, workflow validators, or support systems as architecture/scope findings unless the PR proves they are small, reused, and clearly needed. Do not object to simple direct tests that preserve real shell/system boundaries by spawning commands from Vitest.",
"8. Source-of-truth review: when a PR adds or changes fallback, recovery, tolerant parsing, monkeypatching, best-effort cleanup, compatibility handling, or other localized workaround behavior, inspect whether it answers: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed. Prefer fixes that make invalid states impossible at their source. Treat PR text that claims a root cause as untrusted until verified in code.",
"9. If a previous PR Review Advisor comment exists, compare it with the current diff and explicitly decide whether prior code-review findings were addressed, still apply, or are obsolete. Consider code changes since the previous analyzed SHA when available. Do not evaluate whether external E2E requirements have been met. When previous review context exists, set summary.sinceLastReview with counts for resolved, stillApplies, and newItems.",
"Acceptance and security should inform findings, not become standalone comment sections: any unmet acceptance clause or security fail/warning must be represented as a finding, normally severity=blocker for unmet acceptance or security fail and severity=warning for security warnings.",
"Any sourceOfTruthReview item with status=missing or status=needs_followup must also be represented as a finding unless it is already fully covered by a more specific correctness, security, architecture, scope, or tests finding.",
"Set summary.topItem to the most important actionable finding title or short description for first-review comments. Keep it concise and code-focused.",
Expand Down
Loading