From 9589a08026936a3847734d0f8b46417a637f5396 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:03:50 -0400 Subject: [PATCH 01/20] feat(phases): resolve staged phases; empty phases input means combined Co-Authored-By: Claude Fable 5.1 --- action.yml | 2 +- src/__tests__/config.test.ts | 13 ++++ src/__tests__/orchestrate.test.ts | 16 ++--- src/config.ts | 12 +++- src/orchestrate.ts | 18 +++-- src/review/__tests__/phases.test.ts | 101 ++++++++++++++++++++++------ src/review/__tests__/prompt.test.ts | 7 +- src/review/phases.ts | 75 ++++++++++++++++++--- 8 files changed, 189 insertions(+), 55 deletions(-) diff --git a/action.yml b/action.yml index 903ba46..ae0d6f9 100644 --- a/action.yml +++ b/action.yml @@ -37,7 +37,7 @@ inputs: required: false default: AGENTS.md phases: - description: "Review phases to run. V1 supports: combined" + description: "How the review dimensions are dispatched: combined (one model call carrying every dimension) | parallel (three focused calls at once — faster, roughly 3x the prompt tokens) | sequential (the same three calls in order, each seeing the earlier findings). Empty = combined, so workflows can wire an unset repo variable directly" required: false default: combined context_budget_tokens: diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index bfc99b0..451dc38 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -85,6 +85,19 @@ describe("parseConfig", () => { expect(config.requestTimeoutSeconds).toBe(900) }) + it("falls back to combined for an empty phases", () => { + const config = parseConfig(makeRawInputs({ phases: "" })) + + expect(config.phases).toBe("combined") + }) + + it("passes phases through as a string for domain validation", () => { + // Value validation lives in review/phases.ts resolveStages + const config = parseConfig(makeRawInputs({ phases: "everything" })) + + expect(config.phases).toBe("everything") + }) + it("rejects a zero request_timeout_seconds", () => { expect(() => parseConfig(makeRawInputs({ requestTimeoutSeconds: "0" })), diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 5b717df..f5edb30 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -28,6 +28,7 @@ import { type ReviewComment, } from "../review/comment-mapping.js" import { filterNonFindings } from "../review/filter-non-findings.js" +import { COMBINED_PHASE } from "../review/phases.js" import { selectFindings } from "../review/select-findings.js" import { renderCostSummary } from "../openrouter/cost-summary.js" import { @@ -2548,10 +2549,7 @@ describe("createPromptedGenerateFindings", () => { const files = parseDiff(sampleDiff) const { annotateDiff } = await import("../diff/annotate-diff.js") - const { resolvePhases } = await import("../review/phases.js") - const phases = resolvePhases("combined") - const phase = phases[0] - if (phase === undefined) throw new Error("expected a phase") + const phase = COMBINED_PHASE await generate({ prContext: fixturePrContext, @@ -2594,10 +2592,7 @@ describe("createPromptedGenerateFindings", () => { const files = parseDiff(sampleDiff) const { annotateDiff } = await import("../diff/annotate-diff.js") - const { resolvePhases } = await import("../review/phases.js") - const phases = resolvePhases("combined") - const phase = phases[0] - if (phase === undefined) throw new Error("expected a phase") + const phase = COMBINED_PHASE const annotated = annotateDiff(files) await generate({ @@ -2636,10 +2631,7 @@ describe("createPromptedGenerateFindings", () => { const files = parseDiff(sampleDiff) const { annotateDiff } = await import("../diff/annotate-diff.js") - const { resolvePhases } = await import("../review/phases.js") - const phases = resolvePhases("combined") - const phase = phases[0] - if (phase === undefined) throw new Error("expected a phase") + const phase = COMBINED_PHASE const annotated = annotateDiff(files) const context: ReviewContext = { diff --git a/src/config.ts b/src/config.ts index c2fa179..bc2cd9c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -45,6 +45,16 @@ const timerSafeSeconds = z.string().transform((value, ctx) => { return parsed }) +/** Mirrors the action.yml default — keep the two in sync. */ +const defaultPhases = "combined" + +/** Shape-only: the value is validated by its domain owner + * (review/phases.ts resolveStages) at startup. Empty string means "not + * provided" for the same reason as timerSafeSeconds. */ +const phasesOrDefault = z + .string() + .transform((value) => (value ? value : defaultPhases)) + const configSchema = z.object({ githubToken: z.string().min(1, "github_token is required"), openrouterApiKey: z.string().min(1, "openrouter_api_key is required"), @@ -56,7 +66,7 @@ const configSchema = z.object({ // (review/finding.ts resolveSeverityThreshold) at startup severityThreshold: z.string().min(1, "severity_threshold must not be empty"), conventionsFile: z.string().min(1, "conventions_file must not be empty"), - phases: z.string().min(1, "phases must not be empty"), + phases: phasesOrDefault, contextBudgetTokens: requiredPositiveInteger, traceRelatedFiles: z.boolean(), maxScanFiles: requiredPositiveInteger, diff --git a/src/orchestrate.ts b/src/orchestrate.ts index fb171a9..c227e37 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -41,7 +41,11 @@ import { type Finding, type FindingSeverity, } from "./review/finding.js" -import { resolvePhases, type ReviewPhase } from "./review/phases.js" +import { + resolveStages, + type ReviewPhase, + type ReviewStage, +} from "./review/phases.js" import { buildSystemPrompt, buildUserPrompt, @@ -354,12 +358,12 @@ const runReviewPipeline = async ( deps, prContext, severityThreshold, - phases, + stages, }: { deps: OrchestrateDeps prContext: PrContext severityThreshold: FindingSeverity - phases: ReviewPhase[] + stages: ReviewStage[] }, logger: Logger, ): Promise => { @@ -620,9 +624,9 @@ const runReviewPipeline = async ( .slice(-PRIOR_COMMENT_CAP) // Step 10–11: generate findings (V1: single combined phase) - const phase = phases[0] + const phase = stages[0]?.[0] if (!phase) { - throw new Error("resolvePhases returned no phases") + throw new Error("resolveStages returned no phases") } const structuredResult = await generateFindings({ prContext, @@ -828,7 +832,7 @@ export const orchestrate = async ( // Step 1: fail-fast validation — throws before any network call const severityThreshold = resolveSeverityThreshold(config.severityThreshold) - const phases = resolvePhases(config.phases) + const stages = resolveStages(config.phases) logger.info("review settings from action inputs", { model: config.model, @@ -882,7 +886,7 @@ export const orchestrate = async ( try { const result = await runReviewPipeline( - { deps, prContext, severityThreshold, phases }, + { deps, prContext, severityThreshold, stages }, logger, ) const completion = resolveCheckRunCompletion({ diff --git a/src/review/__tests__/phases.test.ts b/src/review/__tests__/phases.test.ts index 7fcaeea..9dc0391 100644 --- a/src/review/__tests__/phases.test.ts +++ b/src/review/__tests__/phases.test.ts @@ -1,36 +1,97 @@ import { describe, expect, it } from "vitest" import { + buildPassScope, CI_WORKFLOW_CHECKS, DIMENSION_CODE_QUALITY, DIMENSION_CORRECTNESS_SECURITY, DIMENSION_SUBTLE_BUGS, DIMENSION_TEST_QUALITY, REPORTING_RULES, - resolvePhases, + resolveStages, } from "../phases.js" -describe("resolvePhases", () => { - it("resolves combined to a single phase carrying all four dimensions plus CI checks and reporting rules", () => { - const phases = resolvePhases("combined") - - expect(phases).toEqual([ - { - id: "combined", - instructionSections: [ - DIMENSION_CORRECTNESS_SECURITY, - DIMENSION_CODE_QUALITY, - DIMENSION_TEST_QUALITY, - DIMENSION_SUBTLE_BUGS, - CI_WORKFLOW_CHECKS, - REPORTING_RULES, - ], - }, +// Test-owned copies of the phase shapes: drift in a phase's id, section +// order, or scope line fails here rather than passing through the resolver. +const expectedCombinedPhase = { + id: "combined", + instructionSections: [ + DIMENSION_CORRECTNESS_SECURITY, + DIMENSION_CODE_QUALITY, + DIMENSION_TEST_QUALITY, + DIMENSION_SUBTLE_BUGS, + CI_WORKFLOW_CHECKS, + REPORTING_RULES, + ], +} + +const expectedCorrectnessSecurityPhase = { + id: "correctness-security", + instructionSections: [ + buildPassScope(["correctness & security", "CI workflow checks"]), + DIMENSION_CORRECTNESS_SECURITY, + CI_WORKFLOW_CHECKS, + REPORTING_RULES, + ], +} + +const expectedConventionsTestsPhase = { + id: "conventions-tests", + instructionSections: [ + buildPassScope(["code quality & conventions", "test quality & coverage"]), + DIMENSION_CODE_QUALITY, + DIMENSION_TEST_QUALITY, + REPORTING_RULES, + ], +} + +const expectedSubtleBugsPhase = { + id: "subtle-bugs", + instructionSections: [ + buildPassScope(["subtle bug patterns"]), + DIMENSION_SUBTLE_BUGS, + REPORTING_RULES, + ], +} + +describe("resolveStages", () => { + it("resolves combined to one stage of one phase carrying all four dimensions plus CI checks and reporting rules", () => { + expect(resolveStages("combined")).toEqual([[expectedCombinedPhase]]) + }) + + it("resolves parallel to one stage of the three split phases", () => { + expect(resolveStages("parallel")).toEqual([ + [ + expectedCorrectnessSecurityPhase, + expectedConventionsTestsPhase, + expectedSubtleBugsPhase, + ], ]) }) - it("rejects an unknown phases value with remediation", () => { - expect(() => resolvePhases("correctness,tests")).toThrow( - 'unknown phases value "correctness,tests" — V1 supports only "combined"', + it("resolves sequential to three single-phase stages in dimension order", () => { + expect(resolveStages("sequential")).toEqual([ + [expectedCorrectnessSecurityPhase], + [expectedConventionsTestsPhase], + [expectedSubtleBugsPhase], + ]) + }) + + it("rejects an unknown phases value naming the valid modes", () => { + expect(() => resolveStages("correctness,tests")).toThrow( + 'unknown phases value "correctness,tests" — valid: combined | parallel | sequential', + ) + }) +}) + +describe("buildPassScope", () => { + it("names the covered dimensions and keeps the cross-pass bug boundary", () => { + expect(buildPassScope(["subtle bug patterns"])).toBe( + [ + "PASS SCOPE: this pass covers subtle bug patterns. Other passes", + "cover the remaining dimensions; spend your analysis on these. Boundary: a", + "concrete bug you notice while tracing is still reported with its real", + "category, never dropped because it belongs to another pass.", + ].join("\n"), ) }) }) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 7429e36..00b3309 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest" import type { PrContext } from "../../github/event.js" -import { resolvePhases } from "../phases.js" +import { COMBINED_PHASE as combinedPhase } from "../phases.js" import { buildSystemPrompt, buildUserPrompt, @@ -10,11 +10,6 @@ import { } from "../prompt.js" import { makeFinding } from "./make-finding.js" -const resolvedPhases = resolvePhases("combined") -const combinedPhase = resolvedPhases[0] -if (combinedPhase === undefined) - throw new Error("combined phase missing from resolvePhases") - const prContext: PrContext = { prNumber: 7, title: "feat: trim names before greeting", diff --git a/src/review/phases.ts b/src/review/phases.ts index 24d5fa4..d963497 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -1,8 +1,10 @@ /** - * Review phase definitions. V1 runs a single "combined" phase carrying all - * four dimensions; V2 splits them into sequential phases that receive prior - * findings. Each dimension is its own constant so V2 reuses the blocks - * unchanged. + * Review phase definitions and the stage resolver. A phase is one model call + * carrying a set of instruction sections. A stage is the phases that run + * concurrently; stages run in order, and each later stage sees the earlier + * stages' findings. `combined` is one stage of one phase carrying every + * dimension; `parallel` and `sequential` split the dimensions into the same + * three phases and differ only in how those phases are laid out in stages. * * Dimension content is written for a single-call reviewer: every check must * be resolvable by reasoning over the provided files — no assumption of @@ -13,6 +15,8 @@ export type ReviewPhase = { instructionSections: string[] } +export type ReviewStage = ReviewPhase[] + export const DIMENSION_CORRECTNESS_SECURITY = `DIMENSION 1 — CORRECTNESS & SECURITY. Logic errors, incorrect conditions, off-by-one errors, null/undefined access, race conditions, unhandled error paths, missing edge cases. Error paths get @@ -294,7 +298,16 @@ export const REPORTING_RULES = `REPORTING RULES — these override intuition: same pattern, including pre-existing ones. Boundary: sweep the specific pattern that fired, not all dimensions.` -const combinedPhase: ReviewPhase = { +/** Opens every split phase's instructions. The boundary keeps a phase from + * dropping a real bug because another phase "owns" that dimension. */ +export const buildPassScope = (dimensionTitles: string[]): string => { + return `PASS SCOPE: this pass covers ${dimensionTitles.join(", ")}. Other passes +cover the remaining dimensions; spend your analysis on these. Boundary: a +concrete bug you notice while tracing is still reported with its real +category, never dropped because it belongs to another pass.` +} + +export const COMBINED_PHASE: ReviewPhase = { id: "combined", instructionSections: [ DIMENSION_CORRECTNESS_SECURITY, @@ -306,9 +319,55 @@ const combinedPhase: ReviewPhase = { ], } -export const resolvePhases = (phasesInput: string): ReviewPhase[] => { - if (phasesInput === "combined") return [combinedPhase] +export const CORRECTNESS_SECURITY_PHASE: ReviewPhase = { + id: "correctness-security", + instructionSections: [ + buildPassScope(["correctness & security", "CI workflow checks"]), + DIMENSION_CORRECTNESS_SECURITY, + CI_WORKFLOW_CHECKS, + REPORTING_RULES, + ], +} + +export const CONVENTIONS_TESTS_PHASE: ReviewPhase = { + id: "conventions-tests", + instructionSections: [ + buildPassScope(["code quality & conventions", "test quality & coverage"]), + DIMENSION_CODE_QUALITY, + DIMENSION_TEST_QUALITY, + REPORTING_RULES, + ], +} + +export const SUBTLE_BUGS_PHASE: ReviewPhase = { + id: "subtle-bugs", + instructionSections: [ + buildPassScope(["subtle bug patterns"]), + DIMENSION_SUBTLE_BUGS, + REPORTING_RULES, + ], +} + +/** + * Validates the phases action input against the modes this module owns — + * config validates shape only (same split as resolveSeverityThreshold). + * Called at startup so a bad value crashes before any OpenRouter call. + */ +export const resolveStages = (phasesInput: string): ReviewStage[] => { + if (phasesInput === "combined") return [[COMBINED_PHASE]] + if (phasesInput === "parallel") { + return [ + [CORRECTNESS_SECURITY_PHASE, CONVENTIONS_TESTS_PHASE, SUBTLE_BUGS_PHASE], + ] + } + if (phasesInput === "sequential") { + return [ + [CORRECTNESS_SECURITY_PHASE], + [CONVENTIONS_TESTS_PHASE], + [SUBTLE_BUGS_PHASE], + ] + } throw new Error( - `unknown phases value "${phasesInput}" — V1 supports only "combined"`, + `unknown phases value "${phasesInput}" — valid: combined | parallel | sequential`, ) } From 66e6e86283ec1bd108d19726ef9de216b1e68547 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:04:53 -0400 Subject: [PATCH 02/20] refactor(prompt): move the it() enumeration into the test dimension Co-Authored-By: Claude Fable 5.1 --- src/review/__tests__/phases.test.ts | 9 +++++++++ src/review/__tests__/prompt.test.ts | 10 +++++++++- src/review/phases.ts | 7 ++++++- src/review/prompt.ts | 9 +++------ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/review/__tests__/phases.test.ts b/src/review/__tests__/phases.test.ts index 9dc0391..e596697 100644 --- a/src/review/__tests__/phases.test.ts +++ b/src/review/__tests__/phases.test.ts @@ -212,6 +212,15 @@ describe("DIMENSION_TEST_QUALITY", () => { expect(normalized).toContain("wrong-item") }) + it("requires every changed it() to be enumerated in the analysis field", () => { + // Lives in the test dimension, not the shared proof-of-work section, so + // phases without the test dimension do not enumerate tests they are not + // reviewing + expect(DIMENSION_TEST_QUALITY.replace(/\s+/g, " ")).toContain( + 'Proof of work: for each new or changed it() block in a test file, add one line to the "analysis" field naming the test, what the exact expected value would be, and whether the test asserts that exact value — a test you did not enumerate is a test you did not check.', + ) + }) + it("guards against false positives on optional chaining and loop-bounds continue", () => { expect(DIMENSION_TEST_QUALITY.replace(/\s+/g, " ")).toContain( 'Do NOT flag these as violations: "?." array access and "?? fallback"', diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 00b3309..24ac7bd 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -66,7 +66,15 @@ describe("buildSystemPrompt", () => { expect(systemPrompt).toContain( "REPORTING RULES — these override intuition:", ) - expect(systemPrompt).toContain('fill the "analysis" field') + expect(systemPrompt).toContain( + [ + 'Before reporting findings, fill the "analysis" field: for each changed file,', + "one line stating what you checked per dimension and which callers or related", + "files you traced. When verifying documentation or description claims, quote", + "the sentence you checked. Findings emitted without corresponding analysis are", + "not trustworthy.", + ].join("\n"), + ) expect(systemPrompt).toContain("Severity rubric:") expect(systemPrompt).toContain( [ diff --git a/src/review/phases.ts b/src/review/phases.ts index d963497..362cbd3 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -202,7 +202,12 @@ Report each gap with the specific untested scenario. In changed test files, flag coverage regressions: removed it() blocks, weakened assertions (toBe → toBeDefined, exact match → toContain), and skipped or commented-out tests. Filter and exclusion tests must seed data both inside AND outside the filter -— exclusion is half the behavior.` +— exclusion is half the behavior. + +Proof of work: for each new or changed it() block in a test file, add one +line to the "analysis" field naming the test, what the exact expected value +would be, and whether the test asserts that exact value — a test you did not +enumerate is a test you did not check.` export const DIMENSION_SUBTLE_BUGS = `DIMENSION 4 — SUBTLE BUG PATTERNS. Apply these checks systematically to every changed file: diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 3c08460..83b5137 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -40,12 +40,9 @@ a traced regression or a concrete bug.` const PROOF_OF_WORK = `Before reporting findings, fill the "analysis" field: for each changed file, one line stating what you checked per dimension and which callers or related -files you traced. For each new or changed it() block in a test file, add one -line naming the test, what the exact expected value would be, and whether the -test asserts that exact value — a test you did not enumerate is a test you -did not check. When verifying documentation or description claims, quote the -sentence you checked. Findings emitted without corresponding analysis are not -trustworthy.` +files you traced. When verifying documentation or description claims, quote +the sentence you checked. Findings emitted without corresponding analysis are +not trustworthy.` const SEVERITY_RUBRIC = `Severity rubric: - critical: exploitable security issue, data loss, or corruption From 741884e0efe28942647dc32593a18f64fbcde194 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:07:07 -0400 Subject: [PATCH 03/20] feat(review): cross-phase merge, phase column in the cost table, ReviewRequestError Co-Authored-By: Claude Fable 5.1 --- src/__tests__/orchestrate.test.ts | 2 +- src/openrouter/__tests__/client.test.ts | 74 ++++++++++++ src/openrouter/__tests__/cost-summary.test.ts | 89 +++++++++----- src/openrouter/client.ts | 39 +++++- src/openrouter/cost-summary.ts | 16 ++- src/orchestrate.ts | 5 +- .../__tests__/merge-phase-findings.test.ts | 114 ++++++++++++++++++ src/review/merge-phase-findings.ts | 57 +++++++++ src/review/select-findings.ts | 6 +- 9 files changed, 358 insertions(+), 44 deletions(-) create mode 100644 src/review/__tests__/merge-phase-findings.test.ts create mode 100644 src/review/merge-phase-findings.ts diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index f5edb30..1c17175 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -106,7 +106,7 @@ const expectedMapped = mapFindingsToReview({ model: "test/model", }) const expectedCostSummary = renderCostSummary({ - attempts: [fixtureAttempt], + attempts: [{ ...fixtureAttempt, phase: "combined" }], modelUsed: "test/model", }) diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts index 2d3922a..a98500a 100644 --- a/src/openrouter/__tests__/client.test.ts +++ b/src/openrouter/__tests__/client.test.ts @@ -4,6 +4,7 @@ import { createTestLogger } from "../../__tests__/test-logger.js" import { reviewResponseJsonSchema } from "../../review/finding.js" import { createOpenRouterClient, + ReviewRequestError, type ChatRequestSubset, type OpenRouterLike, } from "../client.js" @@ -120,6 +121,18 @@ const makeClient = (stub: { sdk: OpenRouterLike }) => { const makeStatusError = (statusCode: number): Error => Object.assign(new Error(`HTTP ${statusCode}`), { statusCode }) +/** The rejection of a request expected to fail, so its fields can be asserted. */ +const captureRejection = async ( + request: Promise, +): Promise => { + try { + await request + return undefined + } catch (error) { + return error + } +} + const requestParams = { systemPrompt: "system prompt", userPrompt: "user prompt", @@ -733,6 +746,67 @@ describe("requestReview", () => { expect(stub.sendCalls).toHaveLength(2) }) + it("carries every billed attempt on the error when the ladder is exhausted", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { error: makeStatusError(500) }, + { error: makeStatusError(503) }, + ], + }) + const { client } = makeClient(stub) + + const failure = await captureRejection( + client.requestReview({ ...requestParams, fallbackModel: null }), + ) + + if (!(failure instanceof ReviewRequestError)) { + throw new Error("expected a ReviewRequestError") + } + expect(failure.aborted).toBe(false) + expect(failure.attempts).toEqual([ + { + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 500: HTTP 500", + }, + { + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 503: HTTP 503", + }, + ]) + }) + + it("marks the error as aborted on an auth/credit failure", async () => { + const stub = makeSdkStub({ + sendResponses: [{ error: makeStatusError(402) }], + }) + const { client } = makeClient(stub) + + const failure = await captureRejection(client.requestReview(requestParams)) + + if (!(failure instanceof ReviewRequestError)) { + throw new Error("expected a ReviewRequestError") + } + expect(failure.aborted).toBe(true) + expect(failure.attempts).toEqual([ + { + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 402: HTTP 402", + }, + ]) + }) + it("makes at most four calls across both ladder models", async () => { const stub = makeSdkStub({ sendResponses: [ diff --git a/src/openrouter/__tests__/cost-summary.test.ts b/src/openrouter/__tests__/cost-summary.test.ts index fd9c63d..2b10e8e 100644 --- a/src/openrouter/__tests__/cost-summary.test.ts +++ b/src/openrouter/__tests__/cost-summary.test.ts @@ -1,8 +1,8 @@ import { describe, expect, it } from "vitest" -import type { ModelAttempt } from "../client.js" -import { renderCostSummary } from "../cost-summary.js" +import { renderCostSummary, type PhaseAttempt } from "../cost-summary.js" -const acceptedAttempt: ModelAttempt = { +const acceptedAttempt: PhaseAttempt = { + phase: "combined", model: "openai/gpt-5-mini", outcome: "accepted", promptTokens: 12000, @@ -11,6 +11,11 @@ const acceptedAttempt: ModelAttempt = { errorSummary: null, } +const tableHeader = [ + "| attempt | phase | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- | --- |", +] + describe("renderCostSummary", () => { it("renders a well-formed empty table for zero attempts", () => { const summary = renderCostSummary({ @@ -24,8 +29,7 @@ describe("renderCostSummary", () => { "", "Model used: openai/gpt-5-mini", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", + ...tableHeader, "", "Total cost: n/a", ].join("\n"), @@ -33,7 +37,8 @@ describe("renderCostSummary", () => { }) it("renders a timeout attempt row verbatim with n/a cells", () => { - const timeoutAttempt: ModelAttempt = { + const timeoutAttempt: PhaseAttempt = { + phase: "combined", model: "openai/gpt-5-mini", outcome: "timeout", promptTokens: null, @@ -52,10 +57,9 @@ describe("renderCostSummary", () => { "", "Model used: openai/gpt-5-mini", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", - "| 1 | openai/gpt-5-mini | timeout | n/a | n/a | n/a |", - "| 2 | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + ...tableHeader, + "| 1 | combined | openai/gpt-5-mini | timeout | n/a | n/a | n/a |", + "| 2 | combined | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", "", "Total cost: $0.042100 (some attempts unpriced)", ].join("\n"), @@ -74,9 +78,8 @@ describe("renderCostSummary", () => { "", "Model used: openai/gpt-5-mini", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", - "| 1 | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + ...tableHeader, + "| 1 | combined | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", "", "Total cost: $0.042100", ].join("\n"), @@ -84,7 +87,8 @@ describe("renderCostSummary", () => { }) it("renders n/a cells and flags a partially priced total", () => { - const failedAttempt: ModelAttempt = { + const failedAttempt: PhaseAttempt = { + phase: "combined", model: "openai/gpt-5-mini", outcome: "api_error", promptTokens: null, @@ -104,10 +108,9 @@ describe("renderCostSummary", () => { "", "Model used: openai/gpt-5-mini", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", - "| 1 | openai/gpt-5-mini | api_error | n/a | n/a | n/a |", - "| 2 | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + ...tableHeader, + "| 1 | combined | openai/gpt-5-mini | api_error | n/a | n/a | n/a |", + "| 2 | combined | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", "", "Total cost: $0.042100 (some attempts unpriced)", ].join("\n"), @@ -115,7 +118,8 @@ describe("renderCostSummary", () => { }) it("sums costs across attempts when every attempt is priced", () => { - const fallbackAttempt: ModelAttempt = { + const fallbackAttempt: PhaseAttempt = { + phase: "combined", model: "anthropic/claude-haiku-4.5", outcome: "accepted", promptTokens: 11000, @@ -138,18 +142,50 @@ describe("renderCostSummary", () => { "", "Model used: anthropic/claude-haiku-4.5", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", - "| 1 | openai/gpt-5-mini | schema_mismatch | 12000 | 800 | $0.042100 |", - "| 2 | anthropic/claude-haiku-4.5 | accepted | 11000 | 600 | $0.010000 |", + ...tableHeader, + "| 1 | combined | openai/gpt-5-mini | schema_mismatch | 12000 | 800 | $0.042100 |", + "| 2 | combined | anthropic/claude-haiku-4.5 | accepted | 11000 | 600 | $0.010000 |", "", "Total cost: $0.052100", ].join("\n"), ) }) + it("renders attempts from several phases in the given order under a joined model line", () => { + const summary = renderCostSummary({ + attempts: [ + { ...acceptedAttempt, phase: "correctness-security" }, + { + ...acceptedAttempt, + phase: "conventions-tests", + model: "anthropic/claude-haiku-4.5", + promptTokens: 11000, + completionTokens: 600, + costUsd: 0.01, + }, + { ...acceptedAttempt, phase: "subtle-bugs" }, + ], + modelUsed: "openai/gpt-5-mini, anthropic/claude-haiku-4.5", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini, anthropic/claude-haiku-4.5", + "", + ...tableHeader, + "| 1 | correctness-security | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + "| 2 | conventions-tests | anthropic/claude-haiku-4.5 | accepted | 11000 | 600 | $0.010000 |", + "| 3 | subtle-bugs | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + "", + "Total cost: $0.094200", + ].join("\n"), + ) + }) + it("renders an n/a total when no attempt carries a cost", () => { - const unpricedAttempt: ModelAttempt = { + const unpricedAttempt: PhaseAttempt = { ...acceptedAttempt, costUsd: null, } @@ -165,9 +201,8 @@ describe("renderCostSummary", () => { "", "Model used: openai/gpt-5-mini", "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", - "| 1 | openai/gpt-5-mini | accepted | 12000 | 800 | n/a |", + ...tableHeader, + "| 1 | combined | openai/gpt-5-mini | accepted | 12000 | 800 | n/a |", "", "Total cost: n/a", ].join("\n"), diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts index a6de06c..53701df 100644 --- a/src/openrouter/client.ts +++ b/src/openrouter/client.ts @@ -278,6 +278,29 @@ type SingleAttempt = abort: boolean } +/** Thrown when the ladder ends without an accepted response. Carries the + * billed attempts so the caller can still account for their cost, and + * whether the ladder stopped on an auth/credit error (no fallback tried). */ +export class ReviewRequestError extends Error { + readonly attempts: ModelAttempt[] + readonly aborted: boolean + + constructor({ + message, + attempts, + aborted, + }: { + message: string + attempts: ModelAttempt[] + aborted: boolean + }) { + super(message) + this.name = "ReviewRequestError" + this.attempts = attempts + this.aborted = aborted + } +} + const describeAttempt = (attempt: ModelAttempt): string => { const errorSuffix = attempt.errorSummary === null ? "" : ` (${attempt.errorSummary})` @@ -523,9 +546,11 @@ export const createOpenRouterClient = ( errorSummary: attemptResult.attempt.errorSummary, }) if (attemptResult.abort) { - throw new Error( - `OpenRouter auth/credit error — aborting without fallback: ${summarizeAttempts(attempts)}`, - ) + throw new ReviewRequestError({ + message: `OpenRouter auth/credit error — aborting without fallback: ${summarizeAttempts(attempts)}`, + attempts, + aborted: true, + }) } if (!attemptResult.retryable) break attemptNumber++ @@ -535,9 +560,11 @@ export const createOpenRouterClient = ( } } - throw new Error( - `review request failed after ${attempts.length} attempt(s): ${summarizeAttempts(attempts)}`, - ) + throw new ReviewRequestError({ + message: `review request failed after ${attempts.length} attempt(s): ${summarizeAttempts(attempts)}`, + attempts, + aborted: false, + }) } return { requestReview } diff --git a/src/openrouter/cost-summary.ts b/src/openrouter/cost-summary.ts index f80dcc5..58ba699 100644 --- a/src/openrouter/cost-summary.ts +++ b/src/openrouter/cost-summary.ts @@ -1,23 +1,27 @@ import type { ModelAttempt } from "./client.js" +/** An attempt tagged with the review phase that made it. */ +export type PhaseAttempt = ModelAttempt & { phase: string } + const formatCost = (costUsd: number | null): string => costUsd === null ? "n/a" : `$${costUsd.toFixed(6)}` const formatTokens = (tokens: number | null): string => tokens === null ? "n/a" : String(tokens) -/** Markdown table for the workflow job summary — one row per attempt, - * failed ones included (any attempt that reached the provider was billed). */ +/** Markdown table for the workflow job summary — one row per attempt across + * every phase, failed ones included (any attempt that reached the provider + * was billed). */ export const renderCostSummary = ({ attempts, modelUsed, }: { - attempts: ModelAttempt[] + attempts: PhaseAttempt[] modelUsed: string }): string => { const rows = attempts.map( (attempt, attemptIndex) => - `| ${attemptIndex + 1} | ${attempt.model} | ${attempt.outcome} | ${formatTokens(attempt.promptTokens)} | ${formatTokens(attempt.completionTokens)} | ${formatCost(attempt.costUsd)} |`, + `| ${attemptIndex + 1} | ${attempt.phase} | ${attempt.model} | ${attempt.outcome} | ${formatTokens(attempt.promptTokens)} | ${formatTokens(attempt.completionTokens)} | ${formatCost(attempt.costUsd)} |`, ) const knownCosts = attempts.flatMap((attempt) => @@ -36,8 +40,8 @@ export const renderCostSummary = ({ "", `Model used: ${modelUsed}`, "", - "| attempt | model | outcome | prompt tokens | completion tokens | cost |", - "| --- | --- | --- | --- | --- | --- |", + "| attempt | phase | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- | --- |", ...rows, "", totalLine, diff --git a/src/orchestrate.ts b/src/orchestrate.ts index c227e37..addb676 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -707,7 +707,10 @@ const runReviewPipeline = async ( // event, no prose); the rest post as individual issue comments so every // new finding is a visible event. All narration lives in the status // comment. Unposted findings carry no anchor and re-report next run. - const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) + const costSummaryMarkdown = renderCostSummary({ + attempts: attempts.map((attempt) => ({ ...attempt, phase: phase.id })), + modelUsed, + }) const { comments, bodyFindings } = mapFindingsToReview({ findings: selected, commentableByPath, diff --git a/src/review/__tests__/merge-phase-findings.test.ts b/src/review/__tests__/merge-phase-findings.test.ts new file mode 100644 index 0000000..e7e8585 --- /dev/null +++ b/src/review/__tests__/merge-phase-findings.test.ts @@ -0,0 +1,114 @@ +import { describe, expect, it } from "vitest" +import { mergePhaseFindings } from "../merge-phase-findings.js" +import { makeFinding } from "./make-finding.js" + +describe("mergePhaseFindings", () => { + it("collapses a cross-phase overlap across categories, keeping the higher severity in the earlier position", () => { + const correctnessFinding = makeFinding({ + line: 10, + category: "correctness", + severity: "medium", + title: "Correctness phase", + }) + const subtleBugsFinding = makeFinding({ + line: 10, + category: "subtle_bugs", + severity: "high", + title: "Subtle bugs phase", + }) + const unrelatedFinding = makeFinding({ line: 40, title: "Unrelated" }) + + const merged = mergePhaseFindings([ + [correctnessFinding, unrelatedFinding], + [subtleBugsFinding], + ]) + + expect(merged).toEqual({ + findings: [subtleBugsFinding, unrelatedFinding], + duplicatesAcrossPhases: 1, + }) + }) + + it("keeps the earlier phase's finding on an equal-severity overlap", () => { + const earlierFinding = makeFinding({ line: 10, title: "Earlier phase" }) + const laterFinding = makeFinding({ + line: 10, + category: "subtle_bugs", + title: "Later phase", + }) + + const merged = mergePhaseFindings([[earlierFinding], [laterFinding]]) + + expect(merged).toEqual({ + findings: [earlierFinding], + duplicatesAcrossPhases: 1, + }) + }) + + it("treats a range as overlapping when a later phase's line falls inside it", () => { + const rangeFinding = makeFinding({ line: 10, end_line: 15 }) + const insideRangeFinding = makeFinding({ + line: 12, + category: "tests", + severity: "critical", + }) + + const merged = mergePhaseFindings([[rangeFinding], [insideRangeFinding]]) + + expect(merged).toEqual({ + findings: [insideRangeFinding], + duplicatesAcrossPhases: 1, + }) + }) + + it("never compares findings from the same phase, so a one-phase run passes through unchanged", () => { + const correctnessFinding = makeFinding({ line: 10 }) + const subtleBugsFinding = makeFinding({ + line: 10, + category: "subtle_bugs", + severity: "high", + }) + + const merged = mergePhaseFindings([[correctnessFinding, subtleBugsFinding]]) + + expect(merged).toEqual({ + findings: [correctnessFinding, subtleBugsFinding], + duplicatesAcrossPhases: 0, + }) + }) + + it("keeps cross-phase findings on non-overlapping lines of the same file", () => { + const firstFinding = makeFinding({ line: 10, end_line: 12 }) + const secondFinding = makeFinding({ line: 13, category: "subtle_bugs" }) + + const merged = mergePhaseFindings([[firstFinding], [secondFinding]]) + + expect(merged).toEqual({ + findings: [firstFinding, secondFinding], + duplicatesAcrossPhases: 0, + }) + }) + + it("keeps cross-phase findings on the same lines of different files", () => { + const greeterFinding = makeFinding({ line: 10 }) + const registryFinding = makeFinding({ + line: 10, + file: "src/registry.ts", + category: "subtle_bugs", + }) + + const merged = mergePhaseFindings([[greeterFinding], [registryFinding]]) + + expect(merged).toEqual({ + findings: [greeterFinding, registryFinding], + duplicatesAcrossPhases: 0, + }) + }) + + it("returns nothing for no phases", () => { + expect(mergePhaseFindings([])).toEqual({ + findings: [], + duplicatesAcrossPhases: 0, + }) + }) +}) diff --git a/src/review/merge-phase-findings.ts b/src/review/merge-phase-findings.ts new file mode 100644 index 0000000..2a58cc1 --- /dev/null +++ b/src/review/merge-phase-findings.ts @@ -0,0 +1,57 @@ +import type { Finding } from "./finding.js" +import { SEVERITY_RANK } from "./finding.js" +import { rangesOverlap } from "./select-findings.js" + +export type MergedPhaseFindings = { + findings: Finding[] + duplicatesAcrossPhases: number +} + +type PhasedFinding = { finding: Finding; phaseIndex: number } + +/** Category is ignored on purpose: two phases reporting overlapping lines of + * one file are almost always one defect under two labels. Findings from the + * same phase never compare — the model deduplicates within its own call, and + * a one-phase run must pass through unchanged. */ +const isCrossPhaseDuplicate = ( + candidate: PhasedFinding, + kept: PhasedFinding, +): boolean => { + return ( + candidate.phaseIndex !== kept.phaseIndex && + candidate.finding.file === kept.finding.file && + rangesOverlap(candidate.finding, kept.finding) + ) +} + +const outranks = (candidate: Finding, kept: Finding): boolean => + SEVERITY_RANK[candidate.severity] > SEVERITY_RANK[kept.severity] + +/** + * Collapses findings that different phases reported on overlapping lines of + * the same file, keeping the higher severity; on a tie the earlier phase + * wins. Input is in phase order (stage order, then phase order within a + * stage); output preserves it, with a replacement taking the position of the + * finding it outranked. + */ +export const mergePhaseFindings = ( + findingsByPhase: Finding[][], +): MergedPhaseFindings => { + const phased = findingsByPhase.flatMap((phaseFindings, phaseIndex) => { + return phaseFindings.map((finding) => ({ finding, phaseIndex })) + }) + + const kept = phased.reduce((keptSoFar, candidate) => { + const duplicate = keptSoFar.find((entry) => + isCrossPhaseDuplicate(candidate, entry), + ) + if (!duplicate) return [...keptSoFar, candidate] + if (!outranks(candidate.finding, duplicate.finding)) return keptSoFar + return keptSoFar.map((entry) => (entry === duplicate ? candidate : entry)) + }, []) + + return { + findings: kept.map((entry) => entry.finding), + duplicatesAcrossPhases: phased.length - kept.length, + } +} diff --git a/src/review/select-findings.ts b/src/review/select-findings.ts index 9927c0c..40fb795 100644 --- a/src/review/select-findings.ts +++ b/src/review/select-findings.ts @@ -19,7 +19,7 @@ const lineRange = (finding: Finding): { start: number; end: number } => { } } -const rangesOverlap = (first: Finding, second: Finding): boolean => { +export const rangesOverlap = (first: Finding, second: Finding): boolean => { const firstRange = lineRange(first) const secondRange = lineRange(second) return ( @@ -38,8 +38,8 @@ const isDuplicate = (candidate: Finding, kept: Finding): boolean => { /** * Threshold-filter, dedupe, sort, and (only when a cap was provided) cap. * Dedupe keeps the higher-severity finding when two findings of the same - * category overlap in the same file — in V2 this is also where cross-phase - * duplicates collapse. + * category overlap in the same file. Duplicates between review phases are + * collapsed earlier, by merge-phase-findings.ts, without the category match. */ export const selectFindings = ({ findings, From 3d0a33108aca28666d747f5aaa188841d168b096 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:13:56 -0400 Subject: [PATCH 04/20] feat(orchestrate): dispatch review phases in stages and report partial completion Co-Authored-By: Claude Fable 5.1 --- src/__tests__/orchestrate.test.ts | 21 +- src/logger.ts | 8 + src/orchestrate.ts | 233 +++++++++++++----- src/review/__tests__/review-summary.test.ts | 13 + src/review/__tests__/run-stages.test.ts | 250 ++++++++++++++++++++ src/review/comment-mapping.ts | 12 + src/review/review-summary.ts | 12 + src/review/run-stages.ts | 127 ++++++++++ 8 files changed, 617 insertions(+), 59 deletions(-) create mode 100644 src/review/__tests__/run-stages.test.ts create mode 100644 src/review/run-stages.ts diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 1c17175..a819534 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -116,6 +116,8 @@ const expectedReviewSummary = ( renderReviewSummary({ prContext: fixturePrContext, conventionsFile: "AGENTS.md", + phasesCompleted: ["combined"], + phasesIncomplete: [], changedFilePaths: [fixtureChangedFile.path], relatedFilePaths: [], relatedFilesExcludedPaths: [], @@ -131,6 +133,7 @@ const expectedReviewSummary = ( totalFromModel: fixtureReviewResponse.findings.length, droppedAsNonFinding: 0, droppedAsUnknownFile: 0, + duplicatesAcrossPhases: 0, duplicatesRemoved: 0, droppedBelowThreshold: 0, droppedAsOverlapping: 0, @@ -492,6 +495,7 @@ describe("orchestrate", () => { reviewUrl: "", modelUsed: "", skippedReason: "unsupported event: push", + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -529,6 +533,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -557,6 +562,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -584,6 +590,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -624,6 +631,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary(), costSummaryMarkdown: expectedCostSummary, }) @@ -1666,6 +1674,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary({ totalFromModel: 2, droppedAsNonFinding: 1, @@ -1691,6 +1700,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 1, droppedAsUnknownFile: 0, + duplicatesAcrossPhases: 0, }, }) }) @@ -1727,6 +1737,7 @@ describe("orchestrate", () => { reviewUrl: "", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary({ relatedFilePaths: ["src/caller.ts"], tokenBudgetRemainingForDocs: @@ -1774,6 +1785,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary({ totalFromModel: 2, droppedAsUnknownFile: 1, @@ -1794,6 +1806,7 @@ describe("orchestrate", () => { level: "warn", message: "dropping finding: file not in prompt context", data: { + phase: "combined", file: "deploy/railway/README.md and the same issues...", line: 493, category: "subtle_bugs", @@ -1807,6 +1820,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 0, droppedAsUnknownFile: 1, + duplicatesAcrossPhases: 0, }, }) }) @@ -1875,7 +1889,12 @@ describe("orchestrate", () => { expect(missingLogger.messages).toContainEqual({ level: "warn", message: "dropping finding: file not in prompt context", - data: { file: "AGENTS.md", line: 1, category: "correctness" }, + data: { + phase: "combined", + file: "AGENTS.md", + line: 1, + category: "correctness", + }, }) }) }) diff --git a/src/logger.ts b/src/logger.ts index 601c55d..3f21fc9 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -98,6 +98,14 @@ const resolveLazyProps = ( return Object.fromEntries(resolvedEntries) } +/** `[ErrorName]: message` for log fields and user-facing summaries; a thrown + * non-Error value is stringified. */ +export const describeError = (error: unknown): string => { + return error instanceof Error + ? `[${error.name}]: ${error.message}` + : String(error) +} + export const createLogger = ( name: string, options?: { diff --git a/src/orchestrate.ts b/src/orchestrate.ts index addb676..9e71297 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -6,18 +6,22 @@ import { newFilePath, } from "./diff/commentable-lines.js" import { annotateDiff } from "./diff/annotate-diff.js" -import type { Logger } from "./logger.js" +import { describeError, type Logger } from "./logger.js" import type { CheckRunConclusion, CheckRunOutput, GithubClient, } from "./github/client.js" import { resolvePullRequestEvent, type PrContext } from "./github/event.js" -import type { - OpenRouterClient, - StructuredReviewResult, +import { + ReviewRequestError, + type OpenRouterClient, + type StructuredReviewResult, } from "./openrouter/client.js" -import { renderCostSummary } from "./openrouter/cost-summary.js" +import { + renderCostSummary, + type PhaseAttempt, +} from "./openrouter/cost-summary.js" import type { ContextReader } from "./context/workspace.js" import { buildStatusComment, @@ -57,7 +61,13 @@ import { } from "./review/prompt.js" import { filterNonFindings } from "./review/filter-non-findings.js" import { filterUnknownFileFindings } from "./review/filter-unknown-file-findings.js" +import { mergePhaseFindings } from "./review/merge-phase-findings.js" import { renderReviewSummary } from "./review/review-summary.js" +import { + runStages, + type PhaseOutcome, + type RunPhase, +} from "./review/run-stages.js" import { selectFindings } from "./review/select-findings.js" /** Fraction of contextBudgetTokens reserved for priority docs — related files @@ -80,11 +90,19 @@ export type GenerateFindings = ( reviewContext: ReviewContext, ) => Promise +/** How each review phase ended; a failed phase's findings are absent from + * the run and its reason is the error text, not a remediation. */ +export type PhaseStatus = + | { phase: string; status: "completed" } + | { phase: string; status: "failed"; reason: string } + export type OrchestrateResult = { findingsCount: number reviewUrl: string + /** Routed model slugs the completed phases used, comma-joined when they differ. */ modelUsed: string skippedReason: string + phases: PhaseStatus[] reviewSummaryMarkdown: string | null costSummaryMarkdown: string | null } @@ -109,12 +127,6 @@ const stripAnchorComment = (body: string): string => const buildSkipBody = (reason: string): string => `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` -const describeError = (error: unknown): string => { - return error instanceof Error - ? `[${error.name}]: ${error.message}` - : String(error) -} - type InlineCommentState = { anchors: AnchorEntry[] commentBodies: string[] @@ -242,6 +254,7 @@ const SKIPPED_RESULT_BASE: Omit< > = { findingsCount: 0, modelUsed: "", + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, } @@ -310,7 +323,9 @@ const completeCheckRunSafely = async ( * The conclusion grades the run, not the code: a completed review is * `success` whether or not it posted findings (the count lives in the * title), a skip is `neutral` (no review happened), and `failure` is - * reserved for the pipeline itself erroring. */ + * reserved for the pipeline itself erroring. A review that lost some of + * its phases still ran and posted, so it stays `success` and the title + * carries the gap. */ const resolveCheckRunCompletion = ({ result, costSummaryMarkdown, @@ -328,12 +343,27 @@ const resolveCheckRunCompletion = ({ }, } } + + const incompletePhases = result.phases.filter( + (phase) => phase.status === "failed", + ) + const incompleteSuffix = + incompletePhases.length === 0 + ? "" + : ` (${incompletePhases.length} of ${result.phases.length} phases incomplete)` + const incompleteSection = + incompletePhases.length === 0 + ? "" + : `\n\nIncomplete phases: ${incompletePhases + .map((phase) => `\`${phase.phase}\` (${phase.reason})`) + .join(", ")}` + if (result.findingsCount === 0) { return { conclusion: "success", output: { - title: "No findings above threshold", - summary: `Reviewed with \`${result.modelUsed}\` — no findings above threshold.${costSection}`, + title: `No findings above threshold${incompleteSuffix}`, + summary: `Reviewed with \`${result.modelUsed}\` — no findings above threshold.${incompleteSection}${costSection}`, }, } } @@ -344,12 +374,84 @@ const resolveCheckRunCompletion = ({ return { conclusion: "success", output: { - title: findingsLabel, - summary: `Reviewed with \`${result.modelUsed}\` — ${findingsLabel} posted.${costSection}`, + title: `${findingsLabel}${incompleteSuffix}`, + summary: `Reviewed with \`${result.modelUsed}\` — ${findingsLabel} posted.${incompleteSection}${costSection}`, }, } } +type CompletedPhase = Extract + +const describePhaseOutcome = (outcome: PhaseOutcome): PhaseStatus => { + if (outcome.status === "completed") { + return { phase: outcome.phase.id, status: "completed" } + } + return { + phase: outcome.phase.id, + status: "failed", + reason: describeError(outcome.error), + } +} + +/** A failed phase's billed attempts ride on the client's error; any other + * failure reached no provider and billed nothing. */ +const phaseAttempts = (outcome: PhaseOutcome): PhaseAttempt[] => { + const attempts = + outcome.status === "completed" + ? outcome.result.attempts + : outcome.error instanceof ReviewRequestError + ? outcome.error.attempts + : [] + return attempts.map((attempt) => ({ ...attempt, phase: outcome.phase.id })) +} + +type FilteredPhaseFindings = { + findings: Finding[] + droppedAsNonFinding: number + droppedAsUnknownFile: number +} + +/** Drops non-findings and findings on files the model never saw. */ +const filterPhaseFindings = ( + { outcome, knownPaths }: { outcome: CompletedPhase; knownPaths: string[] }, + logger: Logger, +): FilteredPhaseFindings => { + const { findings: nonFindingFiltered, droppedAsNonFinding } = + filterNonFindings(outcome.result.review.findings) + const { findings, droppedAsUnknownFile } = filterUnknownFileFindings({ + findings: nonFindingFiltered, + knownPaths, + }) + // Per-drop warn on purpose: each one is a model-quality event, not loop + // chatter, and the title is omitted because it may be garbage + for (const finding of droppedAsUnknownFile) { + logger.warn("dropping finding: file not in prompt context", { + phase: outcome.phase.id, + file: finding.file, + line: finding.line, + category: finding.category, + }) + } + return { + findings, + droppedAsNonFinding, + droppedAsUnknownFile: droppedAsUnknownFile.length, + } +} + +const sumBy = ( + items: Item[], + valueOf: (item: Item) => number, +): number => { + return items.reduce((sum, item) => sum + valueOf(item), 0) +} + +const incompletePhaseIds = (phases: PhaseStatus[]): string[] => { + return phases + .filter((phase) => phase.status === "failed") + .map((phase) => phase.phase) +} + /** Steps 4–14: diff fetch through status comment — everything downstream of * PR-context resolution, extracted so orchestrate can bracket it with the * check-run lifecycle. */ @@ -623,48 +725,61 @@ const runReviewPipeline = async ( .map(stripAnchorComment) .slice(-PRIOR_COMMENT_CAP) - // Step 10–11: generate findings (V1: single combined phase) - const phase = stages[0]?.[0] - if (!phase) { - throw new Error("resolveStages returned no phases") + // Step 10–11: generate findings — one model call per phase, stages in order + const runPhase: RunPhase = ({ phase, priorFindings }) => { + return generateFindings({ + prContext, + phase, + conventions: conventionsForPrompt, + changedFiles, + relatedFiles, + relatedDocs, + annotatedDiff, + priorFindings, + priorBotComments, + }) } - const structuredResult = await generateFindings({ - prContext, - phase, - conventions: conventionsForPrompt, - changedFiles, - relatedFiles, - relatedDocs, - annotatedDiff, - priorFindings: [], - priorBotComments, + const phaseOutcomes = await runStages({ stages, runPhase }, logger) + const completedPhases = phaseOutcomes.filter( + (outcome) => outcome.status === "completed", + ) + const phases = phaseOutcomes.map(describePhaseOutcome) + const modelUsed = [ + ...new Set(completedPhases.map((outcome) => outcome.result.modelUsed)), + ].join(", ") + const attempts = phaseOutcomes.flatMap(phaseAttempts) + logger.info("review phases finished", { + completed: completedPhases.map((outcome) => outcome.phase.id), + incomplete: incompletePhaseIds(phases), }) - const { modelUsed, attempts } = structuredResult - - // Step 12: filter non-findings and findings on files the model never saw - // before selection so cap slots aren't wasted - const { findings: nonFindingFiltered, droppedAsNonFinding } = - filterNonFindings(structuredResult.review.findings) - const { findings: realFindings, droppedAsUnknownFile } = - filterUnknownFileFindings({ - findings: nonFindingFiltered, - knownPaths: promptFilePaths, - }) - // Per-drop warn on purpose: each one is a model-quality event, not loop - // chatter, and the title is omitted because it may be garbage - for (const finding of droppedAsUnknownFile) { - logger.warn("dropping finding: file not in prompt context", { - file: finding.file, - line: finding.line, - category: finding.category, - }) - } + // Step 12: per phase, drop non-findings and findings on files the model + // never saw, then collapse cross-phase duplicates — all before selection so + // cap slots aren't wasted + const filteredPhases = completedPhases.map((outcome) => { + return filterPhaseFindings({ outcome, knownPaths: promptFilePaths }, logger) + }) + const totalFromModel = sumBy( + completedPhases, + (outcome) => outcome.result.review.findings.length, + ) + const droppedAsNonFinding = sumBy( + filteredPhases, + (filtered) => filtered.droppedAsNonFinding, + ) + const droppedAsUnknownFile = sumBy( + filteredPhases, + (filtered) => filtered.droppedAsUnknownFile, + ) + const { findings: realFindings, duplicatesAcrossPhases } = mergePhaseFindings( + filteredPhases.map((filtered) => filtered.findings), + ) logger.info("non-finding filter applied to model output", { - totalFromModel: structuredResult.review.findings.length, + totalFromModel, kept: realFindings.length, droppedAsNonFinding, - droppedAsUnknownFile: droppedAsUnknownFile.length, + droppedAsUnknownFile, + duplicatesAcrossPhases, }) // Step 12.5: cross-run dedup — every run walks the same path; a first run @@ -707,10 +822,7 @@ const runReviewPipeline = async ( // event, no prose); the rest post as individual issue comments so every // new finding is a visible event. All narration lives in the status // comment. Unposted findings carry no anchor and re-report next run. - const costSummaryMarkdown = renderCostSummary({ - attempts: attempts.map((attempt) => ({ ...attempt, phase: phase.id })), - modelUsed, - }) + const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) const { comments, bodyFindings } = mapFindingsToReview({ findings: selected, commentableByPath, @@ -772,6 +884,7 @@ const runReviewPipeline = async ( droppedByCap, model: modelUsed, contextNotes, + incompletePhases: incompletePhaseIds(phases), }) try { await githubClient.upsertSummaryComment({ @@ -788,6 +901,8 @@ const runReviewPipeline = async ( const reviewSummaryMarkdown = renderReviewSummary({ prContext, conventionsFile: conventions ? config.conventionsFile : null, + phasesCompleted: completedPhases.map((outcome) => outcome.phase.id), + phasesIncomplete: incompletePhaseIds(phases), changedFilePaths: changedFiles.map((file) => file.path), relatedFilePaths: relatedFiles.map((file) => file.path), relatedFilesExcludedPaths: relatedFilesResult.excludedByCapPaths, @@ -802,9 +917,10 @@ const runReviewPipeline = async ( tokenBudgetUsedByDiff: diffTokens, tokenBudgetPriorityDocFloor: priorityDocFloor, tokenBudgetRemainingForDocs: docRemainingTokens, - totalFromModel: structuredResult.review.findings.length, + totalFromModel, droppedAsNonFinding, - droppedAsUnknownFile: droppedAsUnknownFile.length, + droppedAsUnknownFile, + duplicatesAcrossPhases, duplicatesRemoved: realFindings.length - newFindings.length, droppedBelowThreshold, droppedAsOverlapping, @@ -817,6 +933,7 @@ const runReviewPipeline = async ( reviewUrl: inlineOutcome.url, modelUsed, skippedReason: "", + phases, reviewSummaryMarkdown, costSummaryMarkdown, } diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index cbb02c1..0d92d57 100644 --- a/src/review/__tests__/review-summary.test.ts +++ b/src/review/__tests__/review-summary.test.ts @@ -14,6 +14,8 @@ const baseStats: ReviewSummaryStats = { baseRef: "main", }, conventionsFile: "AGENTS.md", + phasesCompleted: ["combined"], + phasesIncomplete: [], changedFilePaths: ["src/greeter.ts"], relatedFilePaths: [], relatedFilesExcludedPaths: [], @@ -29,6 +31,7 @@ const baseStats: ReviewSummaryStats = { totalFromModel: 3, droppedAsNonFinding: 0, droppedAsUnknownFile: 0, + duplicatesAcrossPhases: 0, duplicatesRemoved: 0, droppedBelowThreshold: 0, droppedAsOverlapping: 0, @@ -48,6 +51,8 @@ describe("renderReviewSummary", () => { "", "**Instructions:** AGENTS.md", "", + "**Phases:** combined", + "", "#### Context", "", "| type | count | paths |", @@ -70,6 +75,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 3 |", "| Dropped as non-findings | 0 |", "| Dropped as unknown file | 0 |", + "| Duplicates (cross-phase) | 0 |", "| Duplicates (cross-run) | 0 |", "| Dropped below threshold | 0 |", "| Dropped as overlapping | 0 |", @@ -100,6 +106,8 @@ describe("renderReviewSummary", () => { "", "**Instructions:** AGENTS.md", "", + "**Phases:** combined", + "", "#### Context", "", "| type | count | paths |", @@ -122,6 +130,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 3 |", "| Dropped as non-findings | 0 |", "| Dropped as unknown file | 0 |", + "| Duplicates (cross-phase) | 0 |", "| Duplicates (cross-run) | 0 |", "| Dropped below threshold | 0 |", "| Dropped as overlapping | 0 |", @@ -137,6 +146,7 @@ describe("renderReviewSummary", () => { totalFromModel: 11, droppedAsNonFinding: 2, droppedAsUnknownFile: 1, + duplicatesAcrossPhases: 2, duplicatesRemoved: 3, droppedBelowThreshold: 1, droppedAsOverlapping: 0, @@ -152,6 +162,8 @@ describe("renderReviewSummary", () => { "", "**Instructions:** AGENTS.md", "", + "**Phases:** combined", + "", "#### Context", "", "| type | count | paths |", @@ -174,6 +186,7 @@ describe("renderReviewSummary", () => { "| Raw from model | 11 |", "| Dropped as non-findings | 2 |", "| Dropped as unknown file | 1 |", + "| Duplicates (cross-phase) | 2 |", "| Duplicates (cross-run) | 3 |", "| Dropped below threshold | 1 |", "| Dropped as overlapping | 0 |", diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts new file mode 100644 index 0000000..07e4e9c --- /dev/null +++ b/src/review/__tests__/run-stages.test.ts @@ -0,0 +1,250 @@ +import { describe, expect, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import type { StructuredReviewResult } from "../../openrouter/client.js" +import type { Finding } from "../finding.js" +import type { ReviewPhase } from "../phases.js" +import { runStages, type RunPhase } from "../run-stages.js" +import { makeFinding } from "./make-finding.js" + +const makePhase = (id: string): ReviewPhase => ({ + id, + instructionSections: [`instructions for ${id}`], +}) + +const makeResult = ( + overrides: Partial = {}, +): StructuredReviewResult => ({ + review: { analysis: "", findings: [] }, + modelUsed: "test/model", + attempts: [], + ...overrides, +}) + +type RecordedCall = { phase: string; priorFindings: Finding[] } + +/** A runPhase stub answering each phase id from a fixed table; records + * every call so dispatch order and prior findings can be asserted whole. */ +const makeRunPhase = ( + responses: Record< + string, + () => Promise | StructuredReviewResult + >, +) => { + const calls: RecordedCall[] = [] + const runPhase: RunPhase = async ({ phase, priorFindings }) => { + calls.push({ phase: phase.id, priorFindings }) + const respond = responses[phase.id] + if (!respond) throw new Error(`stub: no response for phase ${phase.id}`) + return respond() + } + return { runPhase, calls } +} + +const captureRejection = async ( + pending: Promise, +): Promise => { + try { + await pending + return undefined + } catch (error) { + return error + } +} + +const phaseA = makePhase("a") +const phaseB = makePhase("b") +const phaseC = makePhase("c") + +describe("runStages", () => { + it("dispatches a stage's phases together with empty prior findings and returns outcomes in phase order", async () => { + const resultA = makeResult({ modelUsed: "model/a" }) + const resultB = makeResult({ modelUsed: "model/b" }) + const deferredA = Promise.withResolvers() + const { runPhase, calls } = makeRunPhase({ + a: () => deferredA.promise, + b: () => resultB, + }) + + const pending = runStages( + { stages: [[phaseA, phaseB]], runPhase }, + createTestLogger(), + ) + // Both phases were called before the first one settled + expect(calls).toEqual([ + { phase: "a", priorFindings: [] }, + { phase: "b", priorFindings: [] }, + ]) + deferredA.resolve(resultA) + + expect(await pending).toEqual([ + { phase: phaseA, status: "completed", result: resultA }, + { phase: phaseB, status: "completed", result: resultB }, + ]) + }) + + it("passes every earlier stage's raw findings to the next stage", async () => { + const findingA = makeFinding({ title: "From a" }) + const findingB = makeFinding({ title: "From b", line: 20 }) + const { runPhase, calls } = makeRunPhase({ + a: () => makeResult({ review: { analysis: "", findings: [findingA] } }), + b: () => makeResult({ review: { analysis: "", findings: [findingB] } }), + c: () => makeResult(), + }) + + await runStages( + { stages: [[phaseA], [phaseB], [phaseC]], runPhase }, + createTestLogger(), + ) + + expect(calls).toEqual([ + { phase: "a", priorFindings: [] }, + { phase: "b", priorFindings: [findingA] }, + { phase: "c", priorFindings: [findingA, findingB] }, + ]) + }) + + it("keeps a sibling's result when one phase in the stage fails", async () => { + const resultA = makeResult() + const failure = new Error("model exploded") + const { runPhase } = makeRunPhase({ + a: () => resultA, + b: () => Promise.reject(failure), + }) + + const outcomes = await runStages( + { stages: [[phaseA, phaseB]], runPhase }, + createTestLogger(), + ) + + expect(outcomes).toEqual([ + { phase: phaseA, status: "completed", result: resultA }, + { phase: phaseB, status: "failed", error: failure }, + ]) + }) + + it("rethrows the only failure unchanged when nothing completed", async () => { + const failure = new Error("model exploded") + const { runPhase } = makeRunPhase({ a: () => Promise.reject(failure) }) + + const rejection = await captureRejection( + runStages({ stages: [[phaseA]], runPhase }, createTestLogger()), + ) + + expect(rejection).toBe(failure) + }) + + it("aggregates every failure into one message when nothing completed", async () => { + const { runPhase } = makeRunPhase({ + a: () => Promise.reject(new Error("boom a")), + b: () => Promise.reject(new Error("boom b")), + }) + + await expect( + runStages({ stages: [[phaseA], [phaseB]], runPhase }, createTestLogger()), + ).rejects.toThrow( + "all 2 review phases failed: a: [Error]: boom a; b: [Error]: boom b", + ) + }) + + it("skips later stages after an auth/credit abort and reports their phases as not attempted", async () => { + const resultA = makeResult() + const abort = Object.assign(new Error("HTTP 401"), { aborted: true }) + const { runPhase, calls } = makeRunPhase({ + a: () => resultA, + b: () => Promise.reject(abort), + c: () => makeResult(), + }) + + const outcomes = await runStages( + { stages: [[phaseA, phaseB], [phaseC]], runPhase }, + createTestLogger(), + ) + + expect(calls.map((call) => call.phase)).toEqual(["a", "b"]) + expect(outcomes).toEqual([ + { phase: phaseA, status: "completed", result: resultA }, + { phase: phaseB, status: "failed", error: abort }, + { + phase: phaseC, + status: "failed", + error: new Error( + "not attempted: an earlier phase aborted on an auth/credit error", + ), + }, + ]) + }) + + it("does not skip later stages after an ordinary failure", async () => { + const resultC = makeResult() + const { runPhase, calls } = makeRunPhase({ + a: () => Promise.reject(new Error("HTTP 500")), + c: () => resultC, + }) + + const outcomes = await runStages( + { stages: [[phaseA], [phaseC]], runPhase }, + createTestLogger(), + ) + + expect(calls.map((call) => call.phase)).toEqual(["a", "c"]) + expect(outcomes.map((outcome) => outcome.status)).toEqual([ + "failed", + "completed", + ]) + }) + + it.each([ + { label: "no stages", stages: [] }, + { label: "an empty stage", stages: [[phaseA], []] }, + ])("throws before any call for $label", async ({ stages }) => { + const { runPhase, calls } = makeRunPhase({ a: () => makeResult() }) + + await expect( + runStages({ stages, runPhase }, createTestLogger()), + ).rejects.toThrow("no review phases to run") + expect(calls).toEqual([]) + }) + + it("logs each phase's completion or failure under its id", async () => { + const logger = createTestLogger() + const findingA = makeFinding() + const { runPhase } = makeRunPhase({ + a: () => + makeResult({ + review: { analysis: "", findings: [findingA] }, + modelUsed: "model/a", + attempts: [ + { + model: "model/a", + outcome: "accepted", + promptTokens: 1, + completionTokens: 1, + costUsd: null, + errorSummary: null, + }, + ], + }), + b: () => Promise.reject(new Error("boom")), + }) + + await runStages({ stages: [[phaseA, phaseB]], runPhase }, logger) + + expect(logger.messages).toEqual([ + { + level: "info", + message: "review phase completed", + data: { + phase: "a", + modelUsed: "model/a", + attemptCount: 1, + findingsCount: 1, + }, + }, + { + level: "warn", + message: "review phase failed", + data: { phase: "b", error: "[Error]: boom" }, + }, + ]) + }) +}) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 739a3ff..452d3f8 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -338,6 +338,7 @@ export const buildStatusComment = ({ droppedByCap, model, contextNotes = [], + incompletePhases = [], }: { sha: string isFirstRun: boolean @@ -347,6 +348,8 @@ export const buildStatusComment = ({ droppedByCap: Finding[] model: string contextNotes?: string[] + /** Ids of review phases that ended without an accepted response. */ + incompletePhases?: string[] }): string => { const shaShort = sha.slice(0, 7) const verb = isFirstRun ? "reviewed" : "re-reviewed" @@ -367,6 +370,14 @@ export const buildStatusComment = ({ droppedByCap.length === 0 ? "" : `_${droppedByCap.length} lower-severity finding(s) omitted by the max_findings cap: ${droppedByCap.map((finding) => `\`${finding.file}:${finding.line}\``).join(", ")}_` + // The error text stays in the check run: it embeds model slugs and + // provider response bodies that do not belong on the PR timeline. + const incompleteNote = + incompletePhases.length === 0 + ? "" + : incompletePhases.length === 1 + ? `_Review phase \`${incompletePhases[0]}\` did not complete; its findings are missing from this run. See the check run for details._` + : `_Review phases ${incompletePhases.map((phaseId) => `\`${phaseId}\``).join(", ")} did not complete; their findings are missing from this run. See the check run for details._` const contextSection = contextNotes.length === 0 ? "" @@ -378,6 +389,7 @@ export const buildStatusComment = ({ findingsLine, unpostedNote, capNote, + incompleteNote, contextSection, attribution, ] diff --git a/src/review/review-summary.ts b/src/review/review-summary.ts index 76f3315..1c4e4a9 100644 --- a/src/review/review-summary.ts +++ b/src/review/review-summary.ts @@ -3,6 +3,9 @@ import type { PrContext } from "../github/event.js" export type ReviewSummaryStats = { prContext: PrContext conventionsFile: string | null + phasesCompleted: string[] + /** Phases that ended without an accepted response — their findings are absent. */ + phasesIncomplete: string[] changedFilePaths: string[] relatedFilePaths: string[] relatedFilesExcludedPaths: string[] @@ -21,6 +24,8 @@ export type ReviewSummaryStats = { droppedAsNonFinding: number /** Findings naming a file the model was never given. */ droppedAsUnknownFile: number + /** Findings two phases reported on overlapping lines of one file. */ + duplicatesAcrossPhases: number duplicatesRemoved: number droppedBelowThreshold: number droppedAsOverlapping: number @@ -41,6 +46,10 @@ const renderPaths = (paths: string[]): string => * each finding. */ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { const sha = stats.prContext.headSha.slice(0, 7) + const incompleteClause = + stats.phasesIncomplete.length === 0 + ? "" + : ` · incomplete: ${stats.phasesIncomplete.join(", ")}` return [ "### umm-actually review summary", @@ -49,6 +58,8 @@ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { "", `**Instructions:** ${stats.conventionsFile ?? "none"}`, "", + `**Phases:** ${renderPaths(stats.phasesCompleted)}${incompleteClause}`, + "", "#### Context", "", "| type | count | paths |", @@ -71,6 +82,7 @@ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { `| Raw from model | ${stats.totalFromModel} |`, `| Dropped as non-findings | ${stats.droppedAsNonFinding} |`, `| Dropped as unknown file | ${stats.droppedAsUnknownFile} |`, + `| Duplicates (cross-phase) | ${stats.duplicatesAcrossPhases} |`, `| Duplicates (cross-run) | ${stats.duplicatesRemoved} |`, `| Dropped below threshold | ${stats.droppedBelowThreshold} |`, `| Dropped as overlapping | ${stats.droppedAsOverlapping} |`, diff --git a/src/review/run-stages.ts b/src/review/run-stages.ts new file mode 100644 index 0000000..71dfa93 --- /dev/null +++ b/src/review/run-stages.ts @@ -0,0 +1,127 @@ +import { describeError, type Logger } from "../logger.js" +import type { StructuredReviewResult } from "../openrouter/client.js" +import type { Finding } from "./finding.js" +import type { ReviewPhase, ReviewStage } from "./phases.js" + +export type PhaseOutcome = + | { phase: ReviewPhase; status: "completed"; result: StructuredReviewResult } + | { phase: ReviewPhase; status: "failed"; error: unknown } + +export type RunPhase = (params: { + phase: ReviewPhase + priorFindings: Finding[] +}) => Promise + +/** The client marks an auth/credit failure as aborted: the key is bad for + * every model, so no later stage can succeed either. */ +const isAbortedRequest = (error: unknown): boolean => { + return ( + typeof error === "object" && + error !== null && + "aborted" in error && + error.aborted === true + ) +} + +const runStage = async ( + { + stage, + priorFindings, + runPhase, + }: { + stage: ReviewStage + priorFindings: Finding[] + runPhase: RunPhase + }, + logger: Logger, +): Promise => { + return Promise.all( + stage.map(async (phase): Promise => { + try { + const result = await runPhase({ phase, priorFindings }) + logger.info("review phase completed", { + phase: phase.id, + modelUsed: result.modelUsed, + attemptCount: result.attempts.length, + findingsCount: result.review.findings.length, + }) + return { phase, status: "completed", result } + } catch (error) { + logger.warn("review phase failed", { + phase: phase.id, + error: describeError(error), + }) + return { phase, status: "failed", error } + } + }), + ) +} + +const notAttempted = (phase: ReviewPhase): PhaseOutcome => { + return { + phase, + status: "failed", + error: new Error( + "not attempted: an earlier phase aborted on an auth/credit error", + ), + } +} + +const completedFindings = (outcomes: PhaseOutcome[]): Finding[] => { + return outcomes.flatMap((outcome) => { + return outcome.status === "completed" ? outcome.result.review.findings : [] + }) +} + +/** + * Runs each stage's phases concurrently and the stages in order, passing + * every earlier stage's raw findings to the next stage as prior findings. + * Outcomes come back in stage-then-phase order regardless of completion + * order. Throws only when no phase completed: one failure is rethrown as-is, + * several are aggregated into one message. + */ +export const runStages = async ( + { stages, runPhase }: { stages: ReviewStage[]; runPhase: RunPhase }, + logger: Logger, +): Promise => { + if (stages.length === 0 || stages.some((stage) => stage.length === 0)) { + throw new Error("no review phases to run") + } + + // Loop-threaded: each stage appends its outcomes, and the next stage's + // prior findings are read from everything completed so far. + let outcomes: PhaseOutcome[] = [] + for (const [stageIndex, stage] of stages.entries()) { + const stageOutcomes = await runStage( + { stage, priorFindings: completedFindings(outcomes), runPhase }, + logger, + ) + outcomes = [...outcomes, ...stageOutcomes] + + const aborted = stageOutcomes.some( + (outcome) => + outcome.status === "failed" && isAbortedRequest(outcome.error), + ) + const remainingStages = stages.slice(stageIndex + 1) + if (aborted && remainingStages.length > 0) { + const skippedPhases = remainingStages.flat() + logger.warn( + "skipping remaining review stages after an auth/credit abort", + { + skippedPhases: skippedPhases.map((phase) => phase.id), + }, + ) + outcomes = [...outcomes, ...skippedPhases.map(notAttempted)] + break + } + } + + const failures = outcomes.filter((outcome) => outcome.status === "failed") + if (failures.length < outcomes.length) return outcomes + if (failures.length === 1 && failures[0]) throw failures[0].error + throw new Error( + `all ${failures.length} review phases failed: ${failures + .map((failure) => `${failure.phase.id}: ${describeError(failure.error)}`) + .join("; ")}`, + ) +} From 27875c60acd6f1074dbfcf2dc740206108dbc7de Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:15:20 -0400 Subject: [PATCH 05/20] test(orchestrate): cover parallel, sequential, partial, and all-failed phase runs Co-Authored-By: Claude Fable 5.1 --- src/__tests__/orchestrate.test.ts | 263 ++++++++++++++++++- src/review/__tests__/comment-mapping.test.ts | 34 +++ src/review/__tests__/review-summary.test.ts | 49 ++++ 3 files changed, 341 insertions(+), 5 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index a819534..9aa7764 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -9,10 +9,11 @@ import type { } from "../github/client.js" import type { PrContext } from "../github/event.js" import type { ContextReader } from "../context/workspace.js" -import type { - ModelAttempt, - OpenRouterClient, - StructuredReviewResult, +import { + ReviewRequestError, + type ModelAttempt, + type OpenRouterClient, + type StructuredReviewResult, } from "../openrouter/client.js" import { estimateTokens, type PromptFile } from "../review/prompt.js" import { annotateDiff } from "../diff/annotate-diff.js" @@ -28,7 +29,12 @@ import { type ReviewComment, } from "../review/comment-mapping.js" import { filterNonFindings } from "../review/filter-non-findings.js" -import { COMBINED_PHASE } from "../review/phases.js" +import { + COMBINED_PHASE, + CONVENTIONS_TESTS_PHASE, + CORRECTNESS_SECURITY_PHASE, + SUBTLE_BUGS_PHASE, +} from "../review/phases.js" import { selectFindings } from "../review/select-findings.js" import { renderCostSummary } from "../openrouter/cost-summary.js" import { @@ -178,6 +184,7 @@ const expectedStatus = ({ totalCount, droppedByCap = [], contextNotes = [], + incompletePhases = [], }: { isFirstRun: boolean postedCount: number @@ -185,6 +192,7 @@ const expectedStatus = ({ totalCount: number droppedByCap?: Finding[] contextNotes?: string[] + incompletePhases?: string[] }) => ({ prNumber: 7, anchor: STATUS_ANCHOR, @@ -197,6 +205,7 @@ const expectedStatus = ({ droppedByCap, model: "test/model", contextNotes, + incompletePhases, }), }) @@ -2543,6 +2552,250 @@ describe("orchestrate", () => { }) }) +describe("staged phases", () => { + const splitPhaseIds = [ + "correctness-security", + "conventions-tests", + "subtle-bugs", + ] + const completedSplitPhases = splitPhaseIds.map((phase) => ({ + phase, + status: "completed" as const, + })) + const splitAttempts = splitPhaseIds.map((phase) => ({ + ...fixtureAttempt, + phase, + })) + + it("runs parallel as one stage: every split phase is called with no prior findings and identical findings collapse across phases", async () => { + const stubs = makeOrchestrateDeps({ config: { phases: "parallel" } }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect( + stubs.generateFindingsCalls.map((call) => [ + call.phase, + call.priorFindings, + ]), + ).toEqual([ + [CORRECTNESS_SECURITY_PHASE, []], + [CONVENTIONS_TESTS_PHASE, []], + [SUBTLE_BUGS_PHASE, []], + ]) + // Every phase returned the same fixture findings: the first phase's copy + // survives, the other two phases' copies are cross-phase duplicates + expect(result).toEqual({ + findingsCount: expectedSelection.selected.length, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "test/model", + skippedReason: "", + phases: completedSplitPhases, + reviewSummaryMarkdown: expectedReviewSummary({ + phasesCompleted: splitPhaseIds, + totalFromModel: fixtureReviewResponse.findings.length * 3, + duplicatesAcrossPhases: fixtureReviewResponse.findings.length * 2, + }), + costSummaryMarkdown: renderCostSummary({ + attempts: splitAttempts, + modelUsed: "test/model", + }), + }) + expect(stubs.postFindingsReviewCalls).toEqual([ + expectedFindingsReview(expectedSelection.selected), + ]) + }) + + it("runs sequential as three stages, passing every earlier phase's raw findings forward", async () => { + const correctnessFinding = makeFinding({ line: 145, title: "From cs" }) + const conventionsFinding = makeFinding({ + line: 3, + category: "conventions", + title: "From ct", + }) + const findingsByPhase: Record = { + "correctness-security": [correctnessFinding], + "conventions-tests": [conventionsFinding], + "subtle-bugs": [], + } + const stubs = makeOrchestrateDeps({ + config: { phases: "sequential" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + return { + review: { + analysis: "", + findings: findingsByPhase[reviewContext.phase.id] ?? [], + }, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect( + stubs.generateFindingsCalls.map((call) => [ + call.phase.id, + call.priorFindings, + ]), + ).toEqual([ + ["correctness-security", []], + ["conventions-tests", [correctnessFinding]], + ["subtle-bugs", [correctnessFinding, conventionsFinding]], + ]) + }) + + it("posts the surviving phases' findings when one phase fails, naming the gap on the status comment and the check run", async () => { + const timeoutAttempt: ModelAttempt = { + model: "test/model", + outcome: "timeout", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "no response within 900s", + } + const stubs = makeOrchestrateDeps({ + config: { phases: "parallel" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + if (reviewContext.phase.id === "subtle-bugs") { + throw new ReviewRequestError({ + message: "review request failed after 1 attempt(s)", + attempts: [timeoutAttempt], + aborted: false, + }) + } + return { + review: fixtureReviewResponse, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + const expectedCost = renderCostSummary({ + attempts: [ + { ...fixtureAttempt, phase: "correctness-security" }, + { ...fixtureAttempt, phase: "conventions-tests" }, + { ...timeoutAttempt, phase: "subtle-bugs" }, + ], + modelUsed: "test/model", + }) + expect(result).toEqual({ + findingsCount: expectedSelection.selected.length, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "test/model", + skippedReason: "", + phases: [ + { phase: "correctness-security", status: "completed" }, + { phase: "conventions-tests", status: "completed" }, + { + phase: "subtle-bugs", + status: "failed", + reason: + "[ReviewRequestError]: review request failed after 1 attempt(s)", + }, + ], + reviewSummaryMarkdown: expectedReviewSummary({ + phasesCompleted: ["correctness-security", "conventions-tests"], + phasesIncomplete: ["subtle-bugs"], + totalFromModel: fixtureReviewResponse.findings.length * 2, + duplicatesAcrossPhases: fixtureReviewResponse.findings.length, + }), + costSummaryMarkdown: expectedCost, + }) + expect(stubs.postFindingsReviewCalls).toEqual([ + expectedFindingsReview(expectedSelection.selected), + ]) + expect(stubs.upsertSummaryCommentCalls).toEqual([ + expectedStatus({ + isFirstRun: true, + postedCount: expectedSelection.selected.length, + totalCount: expectedSelection.selected.length, + incompletePhases: ["subtle-bugs"], + }), + ]) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "success", + output: { + title: `${expectedSelection.selected.length} findings (1 of 3 phases incomplete)`, + summary: `Reviewed with \`test/model\` — ${expectedSelection.selected.length} findings posted.\n\nIncomplete phases: \`subtle-bugs\` ([ReviewRequestError]: review request failed after 1 attempt(s))\n\n${expectedCost}`, + }, + }, + ]) + }) + + it("fails the run with every phase's error when no phase completes", async () => { + const stubs = makeOrchestrateDeps({ + config: { phases: "parallel" }, + generateFindings: async (reviewContext) => { + throw new Error(`${reviewContext.phase.id} exploded`) + }, + }) + const logger = createTestLogger() + + const expectedMessage = + "all 3 review phases failed: correctness-security: [Error]: correctness-security exploded; conventions-tests: [Error]: conventions-tests exploded; subtle-bugs: [Error]: subtle-bugs exploded" + await expect(orchestrate(stubs.deps, logger)).rejects.toThrow( + expectedMessage, + ) + expect(stubs.postFindingsReviewCalls).toEqual([]) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "failure", + output: { + title: "Error — review did not complete", + summary: `[Error]: ${expectedMessage}`, + }, + }, + ]) + }) + + it("joins the routed models when phases were served by different models", async () => { + const stubs = makeOrchestrateDeps({ + config: { phases: "parallel" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + return { + review: fixtureReviewResponse, + modelUsed: + reviewContext.phase.id === "subtle-bugs" + ? "fallback/model" + : "test/model", + attempts: [fixtureAttempt], + } + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.modelUsed).toBe("test/model, fallback/model") + const mapped = mapFindingsToReview({ + findings: expectedSelection.selected, + commentableByPath: fixtureCommentableByPath, + model: "test/model, fallback/model", + }) + expect(stubs.postFindingsReviewCalls).toEqual([ + { + prNumber: 7, + commitId: fixturePrContext.headSha, + body: REVIEW_MARKER, + comments: mapped.comments, + }, + ]) + }) +}) + describe("createPromptedGenerateFindings", () => { it("passes model and fallbackModel to openrouterClient.requestReview", async () => { const requestReviewCalls: RequestReviewParams[] = [] diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 4284ebb..2dd25fe 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -465,6 +465,40 @@ describe("buildStatusComment", () => { ) }) + it("names a single incomplete phase without its error text", () => { + const body = buildStatusComment({ + sha: "abc123def456abc123def456abc123def456abc1", + isFirstRun: true, + postedCount: 2, + unpostedCount: 0, + totalCount: 2, + droppedByCap: [], + model: "anthropic/claude-sonnet-4-6", + incompletePhases: ["subtle-bugs"], + }) + + expect(body).toBe( + `${STATUS_ANCHOR}\n\n**umm-actually** reviewed at \`abc123d\`\n\n2 new finding(s) posted (2 tracked finding(s) across all runs).\n\n_Review phase \`subtle-bugs\` did not complete; its findings are missing from this run. See the check run for details._\n\n---\n*umm-actually · anthropic/claude-sonnet-4-6*`, + ) + }) + + it("names several incomplete phases in the plural", () => { + const body = buildStatusComment({ + sha: "abc123def456abc123def456abc123def456abc1", + isFirstRun: true, + postedCount: 0, + unpostedCount: 0, + totalCount: 0, + droppedByCap: [], + model: "anthropic/claude-sonnet-4-6", + incompletePhases: ["conventions-tests", "subtle-bugs"], + }) + + expect(body).toBe( + `${STATUS_ANCHOR}\n\n**umm-actually** reviewed at \`abc123d\`\n\nNo findings above threshold.\n\n_Review phases \`conventions-tests\`, \`subtle-bugs\` did not complete; their findings are missing from this run. See the check run for details._\n\n---\n*umm-actually · anthropic/claude-sonnet-4-6*`, + ) + }) + it("omits the context notes section when contextNotes is empty", () => { const body = buildStatusComment({ sha: "abc123def456abc123def456abc123def456abc1", diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index 0d92d57..ee494e0 100644 --- a/src/review/__tests__/review-summary.test.ts +++ b/src/review/__tests__/review-summary.test.ts @@ -196,6 +196,55 @@ describe("renderReviewSummary", () => { ) }) + it("lists the completed phases and names the incomplete ones", () => { + const summary = renderReviewSummary({ + ...baseStats, + phasesCompleted: ["correctness-security", "conventions-tests"], + phasesIncomplete: ["subtle-bugs"], + }) + + expect(summary).toBe( + [ + "### umm-actually review summary", + "", + "PR #7 · `feat/trim-names` → `main` · `abc123d`", + "", + "**Instructions:** AGENTS.md", + "", + "**Phases:** correctness-security, conventions-tests · incomplete: subtle-bugs", + "", + "#### Context", + "", + "| type | count | paths |", + "| --- | --- | --- |", + "| Changed files | 1 | src/greeter.ts |", + "| Related files | 0 | — |", + "| Priority docs | 0 | — |", + "| Priority docs (already in context) | 0 | — |", + "| Priority docs (not included) | 0 | — |", + "| Mention-matched docs | 0 | — |", + "| Excluded (related files cap) | 0 | — |", + "| Excluded (docs cap) | 0 | — |", + "", + "**Token budget:** 300000 total · 12000 diff · 30000 priority-doc floor · 250000 left for docs", + "", + "#### Findings pipeline", + "", + "| stage | count |", + "| --- | --- |", + "| Raw from model | 3 |", + "| Dropped as non-findings | 0 |", + "| Dropped as unknown file | 0 |", + "| Duplicates (cross-phase) | 0 |", + "| Duplicates (cross-run) | 0 |", + "| Dropped below threshold | 0 |", + "| Dropped as overlapping | 0 |", + "| Dropped by cap | 0 |", + "| **Posted** | **3** |", + ].join("\n"), + ) + }) + it("renders the budget split when changed files consumed everything", () => { const summary = renderReviewSummary({ ...baseStats, From 18eb63751a31bef9b62a434881067371c9ddcff0 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:16:37 -0400 Subject: [PATCH 06/20] docs: describe the parallel and sequential phases modes Co-Authored-By: Claude Fable 5.1 --- .github/workflows/self_review.yml | 1 + AGENTS.md | 5 +++-- README.md | 21 +++++++++++---------- action.yml | 2 +- src/orchestrate.ts | 12 ++++++++---- 5 files changed, 24 insertions(+), 17 deletions(-) diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 460ce26..589a6b1 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -54,3 +54,4 @@ jobs: openrouter_api_key: ${{ secrets.OPENROUTER_KEY }} model: ${{ vars.OPENROUTER_MODEL || 'anthropic/claude-sonnet-4-6' }} request_timeout_seconds: ${{ vars.UMM_REQUEST_TIMEOUT_SECONDS }} + phases: ${{ vars.UMM_PHASES }} diff --git a/AGENTS.md b/AGENTS.md index 1f9eee7..10118be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ src/ openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, per-attempt deadline, structured-output retry ladder, cost summary diff/ # pure transforms over parse-diff output context/ # workspace I/O: conventions file, changed files, import-trace scan, doc-mention scan, priority docs - review/ # pure review logic: finding schema, phases, prompt, non-finding filter, unknown-file filter, path normalization, selection, comment mapping, context notes, summary + review/ # pure review logic: finding schema, phases + stage dispatch, prompt, non-finding filter, unknown-file filter, cross-phase merge, path normalization, selection, comment mapping, context notes, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` @@ -188,7 +188,8 @@ files. Prefer SDK-provided types over redefining shapes. ## Review instruction authoring The bot's system-prompt instructions live in `src/review/phases.ts` -(dimension constants + reporting rules) and `src/review/prompt.ts` +(dimension constants, the per-phase pass-scope line, reporting rules, and +the phase groups each `phases` mode dispatches) and `src/review/prompt.ts` (identity/scope, proof-of-work, severity rubric, output discipline). When writing or updating a review instruction, follow this formula — each element is here because its absence measurably cost findings in live runs: diff --git a/README.md b/README.md index 3500da5..b3ab3c3 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm | `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | | `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | | `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | -| `phases` | `combined` | Review phases to run. V1 supports: `combined` | +| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — faster, roughly 3x the prompt tokens), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | | `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | | `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | | `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | @@ -103,7 +103,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm | ---------------- | ------------------------------------------------------------------------------------------------------------------- | | `findings_count` | Number of new findings posted (after the non-finding and unknown-file filters, threshold, cap, and cross-run dedup) | | `review_url` | URL of the submitted review; empty when no review was posted | -| `model_used` | Model that produced the accepted response | +| `model_used` | Model(s) that produced the accepted responses, comma-separated when phases were routed to different models | | `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | ## How it works @@ -111,13 +111,13 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm 1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events); once the PR context is known, opens a check run under the token's identity (best-effort — skipped when the token lacks `checks: write`) 2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped 3. Reads the conventions file and changed source files (token-budgeted), traces imports to find related code files, and scans doc files (`.md`, `.json`) for mentions of changed paths -4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends it to OpenRouter -5. Validates the response against a strict Zod schema, retrying with a fallback model if the primary fails -6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), then on re-runs deduplicates against previously posted inline comments (by hidden HTML anchor) +4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter — `combined` is a single request, `parallel` runs three focused requests at once, `sequential` runs them in order with each phase seeing the earlier findings (see the `phases` input) +5. Validates each response against a strict Zod schema, retrying with a fallback model if the primary fails. A phase that fails after its retry ladder is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes +6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on the same lines, then on re-runs deduplicates against previously posted inline comments (by hidden HTML anchor) 7. Filters remaining findings by severity threshold, deduplicates overlapping findings within the run, and caps if configured 8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback 9. Posts one review with inline comments (invisible body); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals -10. Completes the check run with the outcome — the conclusion grades the run, not the code: `success` for any completed review (with or without findings — the count is in the check title), `neutral` for a skip, `failure` only when the pipeline itself errors +10. Completes the check run with the outcome — the conclusion grades the run, not the code: `success` for any completed review (with or without findings — the count is in the check title, and a review that lost a phase says so there too), `neutral` for a skip, `failure` only when the pipeline itself errors ## Non-finding filter @@ -139,15 +139,16 @@ umm-actually is in early development — the core review pipeline works but ther **Shipped (V1)** -- Single-pass review with inline findings anchored to diff lines -- Structured output with retry ladder and fallback model +- Inline findings anchored to diff lines +- Phased review — the `phases` input runs every dimension in one model call (`combined`, the default) or splits them into three focused calls, at once (`parallel`) or in order (`sequential`); findings two phases report on the same lines collapse to one +- Structured output with retry ladder and fallback model, per phase - Import-tracing: changed code is traced into callers via reverse-import scan - Doc-mention scan: unchanged docs (`.md`, `.json`) that reference changed code reach the prompt for staleness detection - Token-budgeted context (changed files + related files + related docs + conventions) - Prompt injection defense (randomized delimiter nonces) - Skip-path handling with posted reasons (oversized diff, empty diff, API limits) -- Cost transparency (per-run model/token/USD report in workflow summary) -- Every posted comment — inline findings, beyond-diff findings, and the status comment — ends with an `umm-actually · ` byline naming the model that produced it, so runs under different models or fallbacks stay distinguishable on the PR +- Cost transparency (per-attempt phase/model/token/USD report in workflow summary, failed attempts included) +- Every posted comment — inline findings, beyond-diff findings, and the status comment — ends with an `umm-actually · ` byline naming the model(s) the run used, so runs under different models or fallbacks stay distinguishable on the PR - `@umm review` comment trigger for on-demand re-reviews - Cross-run finding dedup — re-runs detect previously posted inline findings via hidden HTML anchors and post only new ones; prior bot comment bodies also feed into the prompt for conceptual dedup (the model self-suppresses even when positional anchors differ). An updatable summary comment tracks totals - Non-finding filter — deterministic drop of findings that amount to "no bug here" (`N/A` prefixes, "no action needed" suggestions, "…is correct" titles) before threshold and cap diff --git a/action.yml b/action.yml index ae0d6f9..081773a 100644 --- a/action.yml +++ b/action.yml @@ -93,7 +93,7 @@ outputs: review_url: description: URL of the submitted review; empty when no review was posted model_used: - description: Model that produced the accepted response + description: Model(s) that produced the accepted responses, comma-separated when phases were routed to different models skipped_reason: description: Non-empty when the review was skipped (e.g. diff too large) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 9e71297..eaf81c4 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -1037,9 +1037,9 @@ export const orchestrate = async ( } } -/** V1 one-shot strategy — builds a prompt from the review context and sends - * it to OpenRouter. V1.5/V2 will swap in different strategies behind the - * same GenerateFindings interface. */ +/** Prompted strategy — builds the prompt for one review phase and sends it + * to OpenRouter; the stage dispatcher calls it once per phase. V1.5/V2 + * tool-loop strategies swap in behind the same GenerateFindings interface. */ export const createPromptedGenerateFindings = ( { openrouterClient, @@ -1062,7 +1062,11 @@ export const createPromptedGenerateFindings = ( delimiterNonce, }) - log.info("requesting review", { model, fallbackModel }) + log.info("requesting review", { + phase: reviewContext.phase.id, + model, + fallbackModel, + }) return openrouterClient.requestReview({ systemPrompt, From e8c36b9861b79a0b7fc703ff4a5c6e2caa1ddcc7 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:25:07 -0400 Subject: [PATCH 07/20] fix(review): log phases input in review settings The phases input (combined/parallel/sequential) determines whether the review makes 1 or 3 model calls but was missing from the startup settings log that records every other action input. Co-Authored-By: Claude Fable 5.1 --- src/orchestrate.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index eaf81c4..bc32130 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -957,6 +957,7 @@ export const orchestrate = async ( logger.info("review settings from action inputs", { model: config.model, fallbackModel: config.fallbackModel || null, + phases: config.phases, severityThreshold: config.severityThreshold, maxFindings: config.maxFindings ?? "uncapped", traceRelatedFiles: config.traceRelatedFiles, From 443fd461925a63204b94ff1b1b373642b0072d7d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:34:41 -0400 Subject: [PATCH 08/20] style: simplify ternaries and trim repeated phases explanation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - config.ts: `value ? value : default` → `value || default` - comment-mapping.ts: extract chained ternaries (findingsLine, incompleteNote) into helpers with early returns - README.md: step 4 and roadmap reference the phases input table instead of re-explaining all three modes Co-Authored-By: Claude Fable 5.1 --- README.md | 4 +-- src/config.ts | 4 +-- src/review/comment-mapping.ts | 56 ++++++++++++++++++++++++----------- 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index b3ab3c3..6f64366 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm 1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events); once the PR context is known, opens a check run under the token's identity (best-effort — skipped when the token lacks `checks: write`) 2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped 3. Reads the conventions file and changed source files (token-budgeted), traces imports to find related code files, and scans doc files (`.md`, `.json`) for mentions of changed paths -4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter — `combined` is a single request, `parallel` runs three focused requests at once, `sequential` runs them in order with each phase seeing the earlier findings (see the `phases` input) +4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes) 5. Validates each response against a strict Zod schema, retrying with a fallback model if the primary fails. A phase that fails after its retry ladder is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes 6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on the same lines, then on re-runs deduplicates against previously posted inline comments (by hidden HTML anchor) 7. Filters remaining findings by severity threshold, deduplicates overlapping findings within the run, and caps if configured @@ -140,7 +140,7 @@ umm-actually is in early development — the core review pipeline works but ther **Shipped (V1)** - Inline findings anchored to diff lines -- Phased review — the `phases` input runs every dimension in one model call (`combined`, the default) or splits them into three focused calls, at once (`parallel`) or in order (`sequential`); findings two phases report on the same lines collapse to one +- Phased review — `combined`, `parallel`, and `sequential` dispatch modes (see the `phases` input); cross-phase findings on overlapping lines collapse to one - Structured output with retry ladder and fallback model, per phase - Import-tracing: changed code is traced into callers via reverse-import scan - Doc-mention scan: unchanged docs (`.md`, `.json`) that reference changed code reach the prompt for staleness detection diff --git a/src/config.ts b/src/config.ts index bc2cd9c..2bd90b0 100644 --- a/src/config.ts +++ b/src/config.ts @@ -51,9 +51,7 @@ const defaultPhases = "combined" /** Shape-only: the value is validated by its domain owner * (review/phases.ts resolveStages) at startup. Empty string means "not * provided" for the same reason as timerSafeSeconds. */ -const phasesOrDefault = z - .string() - .transform((value) => (value ? value : defaultPhases)) +const phasesOrDefault = z.string().transform((value) => value || defaultPhases) const configSchema = z.object({ githubToken: z.string().min(1, "github_token is required"), diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 452d3f8..51eacbf 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -324,6 +324,38 @@ ${attributionLine(model)} ` } +const buildFindingsLine = ({ + isFirstRun, + postedCount, + unpostedCount, + totalCount, +}: { + isFirstRun: boolean + postedCount: number + unpostedCount: number + totalCount: number +}): string => { + if (postedCount > 0) { + return `${postedCount} new finding(s) posted (${totalCount} tracked finding(s) across all runs).` + } + if (unpostedCount > 0) { + return `No new findings posted (${totalCount} tracked finding(s) across all runs).` + } + return isFirstRun + ? "No findings above threshold." + : `No new findings (${totalCount} tracked finding(s) across all runs).` +} + +const buildIncompleteNote = (incompletePhases: string[]): string => { + if (incompletePhases.length === 0) return "" + // The error text stays in the check run: it embeds model slugs and + // provider response bodies that do not belong on the PR timeline. + if (incompletePhases.length === 1) { + return `_Review phase \`${incompletePhases[0]}\` did not complete; its findings are missing from this run. See the check run for details._` + } + return `_Review phases ${incompletePhases.map((phaseId) => `\`${phaseId}\``).join(", ")} did not complete; their findings are missing from this run. See the check run for details._` +} + /** The single always-upserted status comment — the receipt that a run * happened and the running cross-run state. Never carries finding text; * findings are their own comments. Counts reflect what actually landed on @@ -353,15 +385,12 @@ export const buildStatusComment = ({ }): string => { const shaShort = sha.slice(0, 7) const verb = isFirstRun ? "reviewed" : "re-reviewed" - const zeroLine = isFirstRun - ? "No findings above threshold." - : `No new findings (${totalCount} tracked finding(s) across all runs).` - const findingsLine = - postedCount > 0 - ? `${postedCount} new finding(s) posted (${totalCount} tracked finding(s) across all runs).` - : unpostedCount > 0 - ? `No new findings posted (${totalCount} tracked finding(s) across all runs).` - : zeroLine + const findingsLine = buildFindingsLine({ + isFirstRun, + postedCount, + unpostedCount, + totalCount, + }) const unpostedNote = unpostedCount === 0 ? "" @@ -370,14 +399,7 @@ export const buildStatusComment = ({ droppedByCap.length === 0 ? "" : `_${droppedByCap.length} lower-severity finding(s) omitted by the max_findings cap: ${droppedByCap.map((finding) => `\`${finding.file}:${finding.line}\``).join(", ")}_` - // The error text stays in the check run: it embeds model slugs and - // provider response bodies that do not belong on the PR timeline. - const incompleteNote = - incompletePhases.length === 0 - ? "" - : incompletePhases.length === 1 - ? `_Review phase \`${incompletePhases[0]}\` did not complete; its findings are missing from this run. See the check run for details._` - : `_Review phases ${incompletePhases.map((phaseId) => `\`${phaseId}\``).join(", ")} did not complete; their findings are missing from this run. See the check run for details._` + const incompleteNote = buildIncompleteNote(incompletePhases) const contextSection = contextNotes.length === 0 ? "" From 40303c50c95156efc235266a21baddac7f5b0159 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:45:28 -0400 Subject: [PATCH 09/20] test: cover describeError branches and zero-findings-with-incomplete-phases check run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two coverage gaps found during test audit: - describeError (newly exported from logger.ts) had no direct tests for either branch — add tests for Error formatting and non-Error stringification - resolveCheckRunCompletion's zero-findings path with incomplete phases was untested — the check run title and summary carry the incomplete suffix even when no findings were posted Co-Authored-By: Claude Fable 5.1 --- src/__tests__/logger.test.ts | 16 +++++++- src/__tests__/orchestrate.test.ts | 61 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/src/__tests__/logger.test.ts b/src/__tests__/logger.test.ts index 90e8dce..0851d5a 100644 --- a/src/__tests__/logger.test.ts +++ b/src/__tests__/logger.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, onTestFinished, vi } from "vitest" -import { createLogger } from "../logger.js" +import { createLogger, describeError } from "../logger.js" type WrittenLine = Record @@ -250,3 +250,17 @@ describe("createLogger", () => { expect(lines[1]?.sessionId).toBe("generated-later") }) }) + +describe("describeError", () => { + it("formats an Error as [Name]: message", () => { + const error = new TypeError("value is not a function") + + expect(describeError(error)).toBe("[TypeError]: value is not a function") + }) + + it("stringifies a non-Error value", () => { + expect(describeError("plain string")).toBe("plain string") + expect(describeError(42)).toBe("42") + expect(describeError(null)).toBe("null") + }) +}) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 9aa7764..b3b30ba 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2794,6 +2794,67 @@ describe("staged phases", () => { }, ]) }) + + it("reports zero findings with the incomplete suffix when surviving phases find nothing", async () => { + const stubs = makeOrchestrateDeps({ + config: { phases: "parallel" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + if (reviewContext.phase.id === "subtle-bugs") { + throw new Error("model exploded") + } + return { + review: { analysis: "", findings: [] }, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + const expectedCost = renderCostSummary({ + attempts: [ + { ...fixtureAttempt, phase: "correctness-security" }, + { ...fixtureAttempt, phase: "conventions-tests" }, + ], + modelUsed: "test/model", + }) + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "", + modelUsed: "test/model", + skippedReason: "", + phases: [ + { phase: "correctness-security", status: "completed" }, + { phase: "conventions-tests", status: "completed" }, + { + phase: "subtle-bugs", + status: "failed", + reason: "[Error]: model exploded", + }, + ], + reviewSummaryMarkdown: expectedReviewSummary({ + phasesCompleted: ["correctness-security", "conventions-tests"], + phasesIncomplete: ["subtle-bugs"], + totalFromModel: 0, + duplicatesAcrossPhases: 0, + posted: 0, + }), + costSummaryMarkdown: expectedCost, + }) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "success", + output: { + title: "No findings above threshold (1 of 3 phases incomplete)", + summary: `Reviewed with \`test/model\` — no findings above threshold.\n\nIncomplete phases: \`subtle-bugs\` ([Error]: model exploded)\n\n${expectedCost}`, + }, + }, + ]) + }) }) describe("createPromptedGenerateFindings", () => { From cadb3dc866efba09597a415307d3105bfd68fe18 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:55:48 -0400 Subject: [PATCH 10/20] fix(review): evict every overlap a merged finding outranks; thread only real findings between stages Co-Authored-By: Claude Fable 5.1 --- .../__tests__/merge-phase-findings.test.ts | 55 +++++++++++++++++++ src/review/__tests__/run-stages.test.ts | 25 +++++++++ src/review/merge-phase-findings.ts | 18 ++++-- src/review/run-stages.ts | 10 +++- 4 files changed, 101 insertions(+), 7 deletions(-) diff --git a/src/review/__tests__/merge-phase-findings.test.ts b/src/review/__tests__/merge-phase-findings.test.ts index e7e8585..bf8591c 100644 --- a/src/review/__tests__/merge-phase-findings.test.ts +++ b/src/review/__tests__/merge-phase-findings.test.ts @@ -61,6 +61,61 @@ describe("mergePhaseFindings", () => { }) }) + it("evicts every overlapping finding a range candidate outranks, not only the first", () => { + const lineTenFinding = makeFinding({ line: 10, title: "Line 10" }) + const lineTwelveFinding = makeFinding({ + line: 12, + category: "conventions", + title: "Line 12", + }) + const rangeFinding = makeFinding({ + line: 10, + end_line: 12, + category: "subtle_bugs", + severity: "high", + title: "Range 10 to 12", + }) + + const merged = mergePhaseFindings([ + [lineTenFinding], + [lineTwelveFinding], + [rangeFinding], + ]) + + expect(merged).toEqual({ + findings: [rangeFinding], + duplicatesAcrossPhases: 2, + }) + }) + + it("drops a range candidate that fails to outrank one of the findings it overlaps", () => { + const lineTenFinding = makeFinding({ line: 10, title: "Line 10" }) + const lineTwelveFinding = makeFinding({ + line: 12, + category: "conventions", + severity: "high", + title: "Line 12", + }) + const rangeFinding = makeFinding({ + line: 10, + end_line: 12, + category: "subtle_bugs", + severity: "high", + title: "Range 10 to 12", + }) + + const merged = mergePhaseFindings([ + [lineTenFinding], + [lineTwelveFinding], + [rangeFinding], + ]) + + expect(merged).toEqual({ + findings: [lineTenFinding, lineTwelveFinding], + duplicatesAcrossPhases: 1, + }) + }) + it("never compares findings from the same phase, so a one-phase run passes through unchanged", () => { const correctnessFinding = makeFinding({ line: 10 }) const subtleBugsFinding = makeFinding({ diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts index 07e4e9c..b9e5f2b 100644 --- a/src/review/__tests__/run-stages.test.ts +++ b/src/review/__tests__/run-stages.test.ts @@ -103,6 +103,31 @@ describe("runStages", () => { ]) }) + it("drops non-findings before threading them to the next stage", async () => { + const realFinding = makeFinding({ title: "Real defect" }) + const nonFinding = makeFinding({ + line: 20, + title: "N/A — the guard is correct", + }) + const { runPhase, calls } = makeRunPhase({ + a: () => + makeResult({ + review: { analysis: "", findings: [realFinding, nonFinding] }, + }), + b: () => makeResult(), + }) + + await runStages( + { stages: [[phaseA], [phaseB]], runPhase }, + createTestLogger(), + ) + + expect(calls).toEqual([ + { phase: "a", priorFindings: [] }, + { phase: "b", priorFindings: [realFinding] }, + ]) + }) + it("keeps a sibling's result when one phase in the stage fails", async () => { const resultA = makeResult() const failure = new Error("model exploded") diff --git a/src/review/merge-phase-findings.ts b/src/review/merge-phase-findings.ts index 2a58cc1..12c2476 100644 --- a/src/review/merge-phase-findings.ts +++ b/src/review/merge-phase-findings.ts @@ -32,7 +32,9 @@ const outranks = (candidate: Finding, kept: Finding): boolean => * the same file, keeping the higher severity; on a tie the earlier phase * wins. Input is in phase order (stage order, then phase order within a * stage); output preserves it, with a replacement taking the position of the - * finding it outranked. + * first finding it outranked. A candidate may overlap several kept findings + * (a range spanning two earlier single-line findings): it must outrank all + * of them to be kept, and then every one of them goes. */ export const mergePhaseFindings = ( findingsByPhase: Finding[][], @@ -42,12 +44,18 @@ export const mergePhaseFindings = ( }) const kept = phased.reduce((keptSoFar, candidate) => { - const duplicate = keptSoFar.find((entry) => + const overlapping = keptSoFar.filter((entry) => isCrossPhaseDuplicate(candidate, entry), ) - if (!duplicate) return [...keptSoFar, candidate] - if (!outranks(candidate.finding, duplicate.finding)) return keptSoFar - return keptSoFar.map((entry) => (entry === duplicate ? candidate : entry)) + if (overlapping.length === 0) return [...keptSoFar, candidate] + const outranksAll = overlapping.every((entry) => + outranks(candidate.finding, entry.finding), + ) + if (!outranksAll) return keptSoFar + return keptSoFar.flatMap((entry) => { + if (entry === overlapping[0]) return [candidate] + return overlapping.includes(entry) ? [] : [entry] + }) }, []) return { diff --git a/src/review/run-stages.ts b/src/review/run-stages.ts index 71dfa93..d3d6975 100644 --- a/src/review/run-stages.ts +++ b/src/review/run-stages.ts @@ -1,5 +1,6 @@ import { describeError, type Logger } from "../logger.js" import type { StructuredReviewResult } from "../openrouter/client.js" +import { filterNonFindings } from "./filter-non-findings.js" import type { Finding } from "./finding.js" import type { ReviewPhase, ReviewStage } from "./phases.js" @@ -67,15 +68,20 @@ const notAttempted = (phase: ReviewPhase): PhaseOutcome => { } } +/** Non-findings are dropped before threading: a "no bug here" entry in the + * prior-findings list could otherwise stop a later phase from reporting the + * real defect at that location. */ const completedFindings = (outcomes: PhaseOutcome[]): Finding[] => { - return outcomes.flatMap((outcome) => { + const rawFindings = outcomes.flatMap((outcome) => { return outcome.status === "completed" ? outcome.result.review.findings : [] }) + return filterNonFindings(rawFindings).findings } /** * Runs each stage's phases concurrently and the stages in order, passing - * every earlier stage's raw findings to the next stage as prior findings. + * every earlier stage's findings (non-findings removed) to the next stage as + * prior findings. * Outcomes come back in stage-then-phase order regardless of completion * order. Throws only when no phase completed: one failure is rethrown as-is, * several are aggregated into one message. From 21346e4f99627071d197dd2c3b887924df414df4 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:09:47 -0400 Subject: [PATCH 11/20] fix(orchestrate): keep the cost table when every review phase fails Co-Authored-By: Claude Fable 5.1 --- src/__tests__/orchestrate.test.ts | 47 +++++++++++++++++++++++-- src/orchestrate.ts | 24 ++++++++++++- src/review/__tests__/run-stages.test.ts | 22 +++++++++--- src/review/run-stages.ts | 34 +++++++++++++----- 4 files changed, 109 insertions(+), 18 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index b3b30ba..e9706c9 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2500,7 +2500,8 @@ describe("orchestrate", () => { conclusion: "failure", output: { title: "Error — review did not complete", - summary: "[Error]: model exploded", + summary: + "[AllPhasesFailedError]: every review phase failed: combined: [Error]: model exploded", }, }, ]) @@ -2743,7 +2744,7 @@ describe("staged phases", () => { const logger = createTestLogger() const expectedMessage = - "all 3 review phases failed: correctness-security: [Error]: correctness-security exploded; conventions-tests: [Error]: conventions-tests exploded; subtle-bugs: [Error]: subtle-bugs exploded" + "every review phase failed: correctness-security: [Error]: correctness-security exploded; conventions-tests: [Error]: conventions-tests exploded; subtle-bugs: [Error]: subtle-bugs exploded" await expect(orchestrate(stubs.deps, logger)).rejects.toThrow( expectedMessage, ) @@ -2754,7 +2755,47 @@ describe("staged phases", () => { conclusion: "failure", output: { title: "Error — review did not complete", - summary: `[Error]: ${expectedMessage}`, + summary: `[AllPhasesFailedError]: ${expectedMessage}`, + }, + }, + ]) + }) + + it("carries the billed attempts into the failure summary when every phase fails", async () => { + const timeoutAttempt: ModelAttempt = { + model: "test/model", + outcome: "timeout", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "no response within 900s", + } + const stubs = makeOrchestrateDeps({ + generateFindings: async () => { + throw new ReviewRequestError({ + message: "review request failed after 1 attempt(s)", + attempts: [timeoutAttempt], + aborted: false, + }) + }, + }) + const logger = createTestLogger() + + await expect(orchestrate(stubs.deps, logger)).rejects.toThrow( + "every review phase failed: combined: [ReviewRequestError]: review request failed after 1 attempt(s)", + ) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "failure", + output: { + title: "Error — review did not complete", + summary: `[AllPhasesFailedError]: every review phase failed: combined: [ReviewRequestError]: review request failed after 1 attempt(s)\n\n${renderCostSummary( + { + attempts: [{ ...timeoutAttempt, phase: "combined" }], + modelUsed: "none", + }, + )}`, }, }, ]) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index bc32130..eed1830 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -64,6 +64,7 @@ import { filterUnknownFileFindings } from "./review/filter-unknown-file-findings import { mergePhaseFindings } from "./review/merge-phase-findings.js" import { renderReviewSummary } from "./review/review-summary.js" import { + AllPhasesFailedError, runStages, type PhaseOutcome, type RunPhase, @@ -439,6 +440,24 @@ const filterPhaseFindings = ( } } +/** A run where every phase failed still billed its attempts; the failure + * summary carries the cost table so they are not lost with the findings. */ +const describePipelineFailure = ({ + pipelineError, + costSummary, +}: { + pipelineError: unknown + costSummary: boolean +}): string => { + const description = describeError(pipelineError) + if (!costSummary || !(pipelineError instanceof AllPhasesFailedError)) { + return description + } + const attempts = pipelineError.outcomes.flatMap(phaseAttempts) + if (attempts.length === 0) return description + return `${description}\n\n${renderCostSummary({ attempts, modelUsed: "none" })}` +} + const sumBy = ( items: Item[], valueOf: (item: Item) => number, @@ -1029,7 +1048,10 @@ export const orchestrate = async ( conclusion: "failure", output: { title: "Error — review did not complete", - summary: describeError(pipelineError), + summary: describePipelineFailure({ + pipelineError, + costSummary: config.costSummary, + }), }, }, logger, diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts index b9e5f2b..e8b7813 100644 --- a/src/review/__tests__/run-stages.test.ts +++ b/src/review/__tests__/run-stages.test.ts @@ -3,7 +3,11 @@ import { createTestLogger } from "../../__tests__/test-logger.js" import type { StructuredReviewResult } from "../../openrouter/client.js" import type { Finding } from "../finding.js" import type { ReviewPhase } from "../phases.js" -import { runStages, type RunPhase } from "../run-stages.js" +import { + AllPhasesFailedError, + runStages, + type RunPhase, +} from "../run-stages.js" import { makeFinding } from "./make-finding.js" const makePhase = (id: string): ReviewPhase => ({ @@ -147,7 +151,7 @@ describe("runStages", () => { ]) }) - it("rethrows the only failure unchanged when nothing completed", async () => { + it("throws an error carrying the outcomes when the only phase fails", async () => { const failure = new Error("model exploded") const { runPhase } = makeRunPhase({ a: () => Promise.reject(failure) }) @@ -155,10 +159,18 @@ describe("runStages", () => { runStages({ stages: [[phaseA]], runPhase }, createTestLogger()), ) - expect(rejection).toBe(failure) + if (!(rejection instanceof AllPhasesFailedError)) { + throw new Error("expected an AllPhasesFailedError") + } + expect(rejection.message).toBe( + "every review phase failed: a: [Error]: model exploded", + ) + expect(rejection.outcomes).toEqual([ + { phase: phaseA, status: "failed", error: failure }, + ]) }) - it("aggregates every failure into one message when nothing completed", async () => { + it("names every failure in the message when nothing completed", async () => { const { runPhase } = makeRunPhase({ a: () => Promise.reject(new Error("boom a")), b: () => Promise.reject(new Error("boom b")), @@ -167,7 +179,7 @@ describe("runStages", () => { await expect( runStages({ stages: [[phaseA], [phaseB]], runPhase }, createTestLogger()), ).rejects.toThrow( - "all 2 review phases failed: a: [Error]: boom a; b: [Error]: boom b", + "every review phase failed: a: [Error]: boom a; b: [Error]: boom b", ) }) diff --git a/src/review/run-stages.ts b/src/review/run-stages.ts index d3d6975..ed699c3 100644 --- a/src/review/run-stages.ts +++ b/src/review/run-stages.ts @@ -13,6 +13,26 @@ export type RunPhase = (params: { priorFindings: Finding[] }) => Promise +const describeFailure = (outcome: PhaseOutcome): string => { + return outcome.status === "failed" + ? `${outcome.phase.id}: ${describeError(outcome.error)}` + : `${outcome.phase.id}: completed` +} + +/** Thrown when no phase completed. `outcomes` keeps every phase's error so + * the caller can still account for the attempts the failed phases billed. */ +export class AllPhasesFailedError extends Error { + readonly outcomes: PhaseOutcome[] + + constructor(outcomes: PhaseOutcome[]) { + super( + `every review phase failed: ${outcomes.map(describeFailure).join("; ")}`, + ) + this.name = "AllPhasesFailedError" + this.outcomes = outcomes + } +} + /** The client marks an auth/credit failure as aborted: the key is bad for * every model, so no later stage can succeed either. */ const isAbortedRequest = (error: unknown): boolean => { @@ -83,8 +103,7 @@ const completedFindings = (outcomes: PhaseOutcome[]): Finding[] => { * every earlier stage's findings (non-findings removed) to the next stage as * prior findings. * Outcomes come back in stage-then-phase order regardless of completion - * order. Throws only when no phase completed: one failure is rethrown as-is, - * several are aggregated into one message. + * order. Throws AllPhasesFailedError only when no phase completed. */ export const runStages = async ( { stages, runPhase }: { stages: ReviewStage[]; runPhase: RunPhase }, @@ -122,12 +141,9 @@ export const runStages = async ( } } - const failures = outcomes.filter((outcome) => outcome.status === "failed") - if (failures.length < outcomes.length) return outcomes - if (failures.length === 1 && failures[0]) throw failures[0].error - throw new Error( - `all ${failures.length} review phases failed: ${failures - .map((failure) => `${failure.phase.id}: ${describeError(failure.error)}`) - .join("; ")}`, + const anyCompleted = outcomes.some( + (outcome) => outcome.status === "completed", ) + if (anyCompleted) return outcomes + throw new AllPhasesFailedError(outcomes) } From 701a0fd447d8c34daf10a51a50cefdc514c77060 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:34:15 -0400 Subject: [PATCH 12/20] fix(prompt): scope the prior-findings note to the same issue Co-Authored-By: Claude Fable 5.1 --- src/review/__tests__/prompt.test.ts | 2 +- src/review/prompt.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 24ac7bd..6355898 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -343,7 +343,7 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + '', ) expect(userPrompt).toContain(priorFinding.title) }) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 83b5137..4dfbdc4 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -215,7 +215,7 @@ export const buildUserPrompt = ({ const priorFindingsSection = priorFindings.length === 0 ? "" - : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report">\n${JSON.stringify(priorFindings, null, 2)}\n` + : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report the same issue; a different defect at the same location is still a finding">\n${JSON.stringify(priorFindings, null, 2)}\n` const priorBotCommentsSection = priorBotComments.length === 0 From 27a100757e89dc4a6c92ee19eb36a62372ee5a8e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:01:24 -0400 Subject: [PATCH 13/20] fix(prompt): state the merge rule in the prior-findings note; test the sequential abort path end to end Co-Authored-By: Claude Fable 5.1 --- src/__tests__/orchestrate.test.ts | 48 +++++++++++++++++++++++++++++ src/review/__tests__/prompt.test.ts | 2 +- src/review/prompt.ts | 2 +- 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index e9706c9..69301d5 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2761,6 +2761,54 @@ describe("staged phases", () => { ]) }) + it("stops a sequential run after an auth/credit abort and reports the skipped phases", async () => { + const billedAttempt: ModelAttempt = { + model: "test/model", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 402: HTTP 402", + } + const stubs = makeOrchestrateDeps({ + config: { phases: "sequential" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + throw new ReviewRequestError({ + message: "OpenRouter auth/credit error — aborting without fallback", + attempts: [billedAttempt], + aborted: true, + }) + }, + }) + const logger = createTestLogger() + + const notAttempted = + "[Error]: not attempted: an earlier phase aborted on an auth/credit error" + const expectedMessage = `every review phase failed: correctness-security: [ReviewRequestError]: OpenRouter auth/credit error — aborting without fallback; conventions-tests: ${notAttempted}; subtle-bugs: ${notAttempted}` + await expect(orchestrate(stubs.deps, logger)).rejects.toThrow( + expectedMessage, + ) + expect(stubs.generateFindingsCalls.map((call) => call.phase.id)).toEqual([ + "correctness-security", + ]) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "failure", + output: { + title: "Error — review did not complete", + summary: `[AllPhasesFailedError]: ${expectedMessage}\n\n${renderCostSummary( + { + attempts: [{ ...billedAttempt, phase: "correctness-security" }], + modelUsed: "none", + }, + )}`, + }, + }, + ]) + }) + it("carries the billed attempts into the failure summary when every phase fails", async () => { const timeoutAttempt: ModelAttempt = { model: "test/model", diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 6355898..40b945e 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -343,7 +343,7 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + '', ) expect(userPrompt).toContain(priorFinding.title) }) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 4dfbdc4..8ec2fae 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -215,7 +215,7 @@ export const buildUserPrompt = ({ const priorFindingsSection = priorFindings.length === 0 ? "" - : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report the same issue; a different defect at the same location is still a finding">\n${JSON.stringify(priorFindings, null, 2)}\n` + : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report them; a later report on the same lines is merged into the earlier one">\n${JSON.stringify(priorFindings, null, 2)}\n` const priorBotCommentsSection = priorBotComments.length === 0 From 170b16f8f1738ee9e834bb4a1714297ed25a3d54 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:11:54 -0400 Subject: [PATCH 14/20] fix(prompt): describe the cross-phase collapse precisely in the prior-findings note Co-Authored-By: Claude Fable 5.1 --- src/review/__tests__/prompt.test.ts | 2 +- src/review/prompt.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 40b945e..50fc545 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -343,7 +343,7 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + '', ) expect(userPrompt).toContain(priorFinding.title) }) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 8ec2fae..1c85995 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -215,7 +215,7 @@ export const buildUserPrompt = ({ const priorFindingsSection = priorFindings.length === 0 ? "" - : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report them; a later report on the same lines is merged into the earlier one">\n${JSON.stringify(priorFindings, null, 2)}\n` + : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report them; when a later report overlaps one of these lines, only the higher-severity finding of the two is kept">\n${JSON.stringify(priorFindings, null, 2)}\n` const priorBotCommentsSection = priorBotComments.length === 0 From 18dee8b1c735907bfa657e873281523f55e46ca6 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:58:40 -0400 Subject: [PATCH 15/20] docs: drop the unqualified speed claim for the parallel phases mode Co-Authored-By: Claude Fable 5.1 --- README.md | 42 +++++++++++++++++++++--------------------- action.yml | 2 +- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 6f64366..cc0c715 100644 --- a/README.md +++ b/README.md @@ -75,27 +75,27 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Inputs -| Input | Default | Description | -| ------------------------- | ----------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | -| `openrouter_api_key` | _(required)_ | OpenRouter API key | -| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | -| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | -| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | -| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | -| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | -| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | -| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — faster, roughly 3x the prompt tokens), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | -| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | -| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | -| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | -| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | -| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | -| `max_related_files` | `8` | Maximum import-traced related files to include in review context | -| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | -| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | -| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | -| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | +| Input | Default | Description | +| ------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | +| `openrouter_api_key` | _(required)_ | OpenRouter API key | +| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | +| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | +| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | +| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | +| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | +| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | +| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | +| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | +| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | +| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | +| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | +| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | +| `max_related_files` | `8` | Maximum import-traced related files to include in review context | +| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | +| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | +| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | +| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | ## Outputs diff --git a/action.yml b/action.yml index 081773a..bd697d7 100644 --- a/action.yml +++ b/action.yml @@ -37,7 +37,7 @@ inputs: required: false default: AGENTS.md phases: - description: "How the review dimensions are dispatched: combined (one model call carrying every dimension) | parallel (three focused calls at once — faster, roughly 3x the prompt tokens) | sequential (the same three calls in order, each seeing the earlier findings). Empty = combined, so workflows can wire an unset repo variable directly" + description: "How the review dimensions are dispatched: combined (one model call carrying every dimension) | parallel (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple) | sequential (the same three calls in order, each seeing the earlier findings). Empty = combined, so workflows can wire an unset repo variable directly" required: false default: combined context_budget_tokens: From 12f39511c6966d4b25d7e68aec6bc62a60ab8a74 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:34:27 -0400 Subject: [PATCH 16/20] docs(review): add mode layout table to the phase/stage module comment --- src/review/phases.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/review/phases.ts b/src/review/phases.ts index 362cbd3..831b42a 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -1,10 +1,18 @@ /** - * Review phase definitions and the stage resolver. A phase is one model call - * carrying a set of instruction sections. A stage is the phases that run - * concurrently; stages run in order, and each later stage sees the earlier - * stages' findings. `combined` is one stage of one phase carrying every - * dimension; `parallel` and `sequential` split the dimensions into the same - * three phases and differ only in how those phases are laid out in stages. + * Review phase definitions and the stage resolver. + * + * - Phase: one model call carrying a set of review dimensions + * - Stage: which phases run concurrently; stages run in order, each + * later stage sees the earlier stages' findings + * + * | Mode | Layout | Behavior | + * |--------------|------------------------------|-----------------------------------| + * | `combined` | 1 stage, 1 phase | all dimensions in one call | + * | `parallel` | 1 stage, 3 phases | 3 calls fire at once (fast) | + * | `sequential` | 3 stages, 1 phase each | each call sees prior findings | + * + * All three modes use the same three phase definitions — they differ only + * in how those phases are grouped into stages. * * Dimension content is written for a single-call reviewer: every check must * be resolvable by reasoning over the provided files — no assumption of From 9ed1578116b69e983a130d23b61416e109358b75 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:35:28 -0400 Subject: [PATCH 17/20] docs(review): clarify sequential layout as 3 stages, 3 phases (1 each) --- src/review/phases.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/review/phases.ts b/src/review/phases.ts index 831b42a..0d53859 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -9,7 +9,7 @@ * |--------------|------------------------------|-----------------------------------| * | `combined` | 1 stage, 1 phase | all dimensions in one call | * | `parallel` | 1 stage, 3 phases | 3 calls fire at once (fast) | - * | `sequential` | 3 stages, 1 phase each | each call sees prior findings | + * | `sequential` | 3 stages, 3 phases (1 each) | each call sees prior findings | * * All three modes use the same three phase definitions — they differ only * in how those phases are grouped into stages. From 003e998d9f9a91ae7485db7f56b6dbb4afcb49d5 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:30:31 -0400 Subject: [PATCH 18/20] =?UTF-8?q?test(review):=20add=20sequential=20partia?= =?UTF-8?q?l-failure=20test=20=E2=80=94=20early=20phase=20fails,=20later?= =?UTF-8?q?=20phases=20complete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/__tests__/orchestrate.test.ts | 37 +++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index dd643bd..96610d7 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2761,6 +2761,43 @@ describe("staged phases", () => { ]) }) + it("continues a sequential run when an early phase fails non-fatally", async () => { + const stubs = makeOrchestrateDeps({ + config: { phases: "sequential" }, + generateFindings: async (reviewContext) => { + stubs.generateFindingsCalls.push(reviewContext) + if (reviewContext.phase.id === "correctness-security") { + throw new Error("model returned invalid JSON") + } + return { + review: fixtureReviewResponse, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(stubs.generateFindingsCalls.map((call) => call.phase.id)).toEqual([ + "correctness-security", + "conventions-tests", + "subtle-bugs", + ]) + expect(result.findingsCount).toBeGreaterThan(0) + expect( + result.phases.map((phaseStatus) => ({ + phase: phaseStatus.phase, + status: phaseStatus.status, + })), + ).toEqual([ + { phase: "correctness-security", status: "failed" }, + { phase: "conventions-tests", status: "completed" }, + { phase: "subtle-bugs", status: "completed" }, + ]) + }) + it("stops a sequential run after an auth/credit abort and reports the skipped phases", async () => { const billedAttempt: ModelAttempt = { model: "test/model", From 5454ccdbd2429948ad98d6be3a0b6b11726501b1 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:46:57 -0400 Subject: [PATCH 19/20] docs(review): add dispatch-stack diagram and doc comments for phased review functions --- src/orchestrate.ts | 2 ++ src/review/phases.ts | 2 ++ src/review/run-stages.ts | 14 ++++++++++++++ 3 files changed, 18 insertions(+) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 3a7840f..ce1f175 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -442,6 +442,8 @@ const filterPhaseFindings = ( /** A run where every phase failed still billed its attempts; the failure * summary carries the cost table so they are not lost with the findings. */ +/** Renders the check-run summary when the pipeline throws. Appends + * a cost table when every phase failed — the operator still pays. */ const describePipelineFailure = ({ pipelineError, costSummary, diff --git a/src/review/phases.ts b/src/review/phases.ts index 0d53859..a893c8d 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -313,6 +313,8 @@ export const REPORTING_RULES = `REPORTING RULES — these override intuition: /** Opens every split phase's instructions. The boundary keeps a phase from * dropping a real bug because another phase "owns" that dimension. */ +/** Tells the model which dimensions this phase covers, so it spends its + * analysis budget on them instead of duplicating another phase's work. */ export const buildPassScope = (dimensionTitles: string[]): string => { return `PASS SCOPE: this pass covers ${dimensionTitles.join(", ")}. Other passes cover the remaining dimensions; spend your analysis on these. Boundary: a diff --git a/src/review/run-stages.ts b/src/review/run-stages.ts index ed699c3..46bbb97 100644 --- a/src/review/run-stages.ts +++ b/src/review/run-stages.ts @@ -1,3 +1,15 @@ +/** + * Dispatch stack for phased reviews: + * + * runStages — loops over stages in order, threading prior findings + * └ runStage — fires one stage's phases concurrently via Promise.all + * └ runPhase — caller-provided: one model call for one phase + * + * A stage is a group of phases that run at the same time. In `parallel` + * mode there is one stage with all three phases; in `sequential` mode + * there are three stages with one phase each (see phases.ts for the + * layout table). + */ import { describeError, type Logger } from "../logger.js" import type { StructuredReviewResult } from "../openrouter/client.js" import { filterNonFindings } from "./filter-non-findings.js" @@ -44,6 +56,8 @@ const isAbortedRequest = (error: unknown): boolean => { ) } +/** Fires one stage's phases concurrently. Each phase is tried + * independently — a failed phase does not cancel its siblings. */ const runStage = async ( { stage, From 53b327ef2d2f2b46f476841238db34d173109323 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:02:40 -0400 Subject: [PATCH 20/20] docs: add phase/stage mechanics explanation to AGENTS.md review section --- AGENTS.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1b88a36..bf61866 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -190,8 +190,17 @@ files. Prefer SDK-provided types over redefining shapes. The bot's system-prompt instructions live in `src/review/phases.ts` (dimension constants, the per-phase pass-scope line, reporting rules, and the phase groups each `phases` mode dispatches) and `src/review/prompt.ts` -(identity/scope, proof-of-work, severity rubric, output discipline). When -writing or updating a review instruction, follow this formula — each +(identity/scope, proof-of-work, severity rubric, output discipline). + +**Phase/stage mechanics:** a phase is one model call carrying a set of +review dimensions. A stage groups the phases that run concurrently; stages +run in order, and each later stage sees the earlier stages' findings. +`combined` = 1 stage, 1 phase; `parallel` = 1 stage, 3 phases; +`sequential` = 3 stages, 3 phases (1 each). The dispatch stack is +`runStages` → `runStage` → `runPhase` in `src/review/run-stages.ts`; +cross-phase finding collapse lives in `src/review/merge-phase-findings.ts`. + +When writing or updating a review instruction, follow this formula — each element is here because its absence measurably cost findings in live runs: - **Trigger, not preference.** Action + condition + boundary: "when you see