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 ab2dd9b..bf61866 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, title similarity, 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, title similarity, context notes, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` @@ -188,9 +188,19 @@ 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` -(identity/scope, proof-of-work, severity rubric, output discipline). When -writing or updating a review instruction, follow this formula — each +(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). + +**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 diff --git a/README.md b/README.md index 91ac386..a20ddcd 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` | Review phases to run. V1 supports: `combined` | -| `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 @@ -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 bot comments (two-tier: positional match by hidden HTML anchor, or content match by title similarity within 50 lines) +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 bot comments (two-tier: positional match by hidden HTML anchor, or content match by title similarity within 50 lines) 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 diff --git a/action.yml b/action.yml index 903ba46..bd697d7 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 — 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: @@ -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/__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__/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 5b717df..96610d7 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,6 +29,12 @@ import { type ReviewComment, } from "../review/comment-mapping.js" import { filterNonFindings } from "../review/filter-non-findings.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 { @@ -105,7 +112,7 @@ const expectedMapped = mapFindingsToReview({ model: "test/model", }) const expectedCostSummary = renderCostSummary({ - attempts: [fixtureAttempt], + attempts: [{ ...fixtureAttempt, phase: "combined" }], modelUsed: "test/model", }) @@ -115,6 +122,8 @@ const expectedReviewSummary = ( renderReviewSummary({ prContext: fixturePrContext, conventionsFile: "AGENTS.md", + phasesCompleted: ["combined"], + phasesIncomplete: [], changedFilePaths: [fixtureChangedFile.path], relatedFilePaths: [], relatedFilesExcludedPaths: [], @@ -130,6 +139,7 @@ const expectedReviewSummary = ( totalFromModel: fixtureReviewResponse.findings.length, droppedAsNonFinding: 0, droppedAsUnknownFile: 0, + duplicatesAcrossPhases: 0, duplicatesRemoved: 0, droppedBelowThreshold: 0, droppedAsOverlapping: 0, @@ -174,6 +184,7 @@ const expectedStatus = ({ totalCount, droppedByCap = [], contextNotes = [], + incompletePhases = [], }: { isFirstRun: boolean postedCount: number @@ -181,6 +192,7 @@ const expectedStatus = ({ totalCount: number droppedByCap?: Finding[] contextNotes?: string[] + incompletePhases?: string[] }) => ({ prNumber: 7, anchor: STATUS_ANCHOR, @@ -193,6 +205,7 @@ const expectedStatus = ({ droppedByCap, model: "test/model", contextNotes, + incompletePhases, }), }) @@ -491,6 +504,7 @@ describe("orchestrate", () => { reviewUrl: "", modelUsed: "", skippedReason: "unsupported event: push", + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -528,6 +542,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -556,6 +571,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -583,6 +599,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "", skippedReason: skipReason, + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, }) @@ -623,6 +640,7 @@ describe("orchestrate", () => { reviewUrl: "https://github.com/test/review/1", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary(), costSummaryMarkdown: expectedCostSummary, }) @@ -1665,6 +1683,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, @@ -1690,6 +1709,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 1, droppedAsUnknownFile: 0, + duplicatesAcrossPhases: 0, }, }) }) @@ -1726,6 +1746,7 @@ describe("orchestrate", () => { reviewUrl: "", modelUsed: "test/model", skippedReason: "", + phases: [{ phase: "combined", status: "completed" }], reviewSummaryMarkdown: expectedReviewSummary({ relatedFilePaths: ["src/caller.ts"], tokenBudgetRemainingForDocs: @@ -1773,6 +1794,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, @@ -1793,6 +1815,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", @@ -1806,6 +1829,7 @@ describe("orchestrate", () => { kept: 1, droppedAsNonFinding: 0, droppedAsUnknownFile: 1, + duplicatesAcrossPhases: 0, }, }) }) @@ -1874,7 +1898,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", + }, }) }) }) @@ -2471,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", }, }, ]) @@ -2523,6 +2553,436 @@ 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 = + "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, + ) + expect(stubs.postFindingsReviewCalls).toEqual([]) + expect(stubs.updateCheckRunCalls).toEqual([ + { + checkRunId: 555, + conclusion: "failure", + output: { + title: "Error — review did not complete", + summary: `[AllPhasesFailedError]: ${expectedMessage}`, + }, + }, + ]) + }) + + 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", + 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: "test/model", + }, + )}`, + }, + }, + ]) + }) + + 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: "test/model", + }, + )}`, + }, + }, + ]) + }) + + 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, + }, + ]) + }) + + 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", () => { it("passes model and fallbackModel to openrouterClient.requestReview", async () => { const requestReviewCalls: RequestReviewParams[] = [] @@ -2548,10 +3008,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 +3051,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 +3090,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..2bd90b0 100644 --- a/src/config.ts +++ b/src/config.ts @@ -45,6 +45,14 @@ 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 || 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 +64,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/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/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 fb171a9..ce1f175 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, @@ -41,7 +45,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, @@ -53,7 +61,14 @@ 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 { + AllPhasesFailedError, + 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 @@ -76,11 +91,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 } @@ -105,12 +128,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[] @@ -238,6 +255,7 @@ const SKIPPED_RESULT_BASE: Omit< > = { findingsCount: 0, modelUsed: "", + phases: [], reviewSummaryMarkdown: null, costSummaryMarkdown: null, } @@ -306,7 +324,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, @@ -324,12 +344,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}`, }, } } @@ -340,12 +375,106 @@ 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, + } +} + +/** 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, +}: { + 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 + const models = [...new Set(attempts.map((attempt) => attempt.model))] + const modelUsed = models.length > 0 ? models.join(", ") : "none" + return `${description}\n\n${renderCostSummary({ attempts, modelUsed })}` +} + +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. */ @@ -354,12 +483,12 @@ const runReviewPipeline = async ( deps, prContext, severityThreshold, - phases, + stages, }: { deps: OrchestrateDeps prContext: PrContext severityThreshold: FindingSeverity - phases: ReviewPhase[] + stages: ReviewStage[] }, logger: Logger, ): Promise => { @@ -619,48 +748,61 @@ const runReviewPipeline = async ( .map(stripAnchorComment) .slice(-PRIOR_COMMENT_CAP) - // Step 10–11: generate findings (V1: single combined phase) - const phase = phases[0] - if (!phase) { - throw new Error("resolvePhases 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 @@ -765,6 +907,7 @@ const runReviewPipeline = async ( droppedByCap, model: modelUsed, contextNotes, + incompletePhases: incompletePhaseIds(phases), }) try { await githubClient.upsertSummaryComment({ @@ -781,6 +924,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, @@ -795,9 +940,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, @@ -810,6 +956,7 @@ const runReviewPipeline = async ( reviewUrl: inlineOutcome.url, modelUsed, skippedReason: "", + phases, reviewSummaryMarkdown, costSummaryMarkdown, } @@ -828,11 +975,12 @@ 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, fallbackModel: config.fallbackModel || null, + phases: config.phases, severityThreshold: config.severityThreshold, maxFindings: config.maxFindings ?? "uncapped", traceRelatedFiles: config.traceRelatedFiles, @@ -882,7 +1030,7 @@ export const orchestrate = async ( try { const result = await runReviewPipeline( - { deps, prContext, severityThreshold, phases }, + { deps, prContext, severityThreshold, stages }, logger, ) const completion = resolveCheckRunCompletion({ @@ -904,7 +1052,10 @@ export const orchestrate = async ( conclusion: "failure", output: { title: "Error — review did not complete", - summary: describeError(pipelineError), + summary: describePipelineFailure({ + pipelineError, + costSummary: config.costSummary, + }), }, }, logger, @@ -913,9 +1064,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, @@ -938,7 +1089,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, diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 480939c..590c3af 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__/merge-phase-findings.test.ts b/src/review/__tests__/merge-phase-findings.test.ts new file mode 100644 index 0000000..bf8591c --- /dev/null +++ b/src/review/__tests__/merge-phase-findings.test.ts @@ -0,0 +1,169 @@ +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("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({ + 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/__tests__/phases.test.ts b/src/review/__tests__/phases.test.ts index 7fcaeea..e596697 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("resolves sequential to three single-phase stages in dimension order", () => { + expect(resolveStages("sequential")).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("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"), ) }) }) @@ -151,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 7429e36..50fc545 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", @@ -71,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( [ @@ -340,7 +343,7 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + '', ) expect(userPrompt).toContain(priorFinding.title) }) diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index cbb02c1..ee494e0 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 |", @@ -183,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, diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts new file mode 100644 index 0000000..e8b7813 --- /dev/null +++ b/src/review/__tests__/run-stages.test.ts @@ -0,0 +1,287 @@ +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 { + AllPhasesFailedError, + 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("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") + 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("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) }) + + const rejection = await captureRejection( + runStages({ stages: [[phaseA]], runPhase }, createTestLogger()), + ) + + 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("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")), + }) + + await expect( + runStages({ stages: [[phaseA], [phaseB]], runPhase }, createTestLogger()), + ).rejects.toThrow( + "every review phase 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 33894a1..5aef30b 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -375,6 +375,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 @@ -389,6 +421,7 @@ export const buildStatusComment = ({ droppedByCap, model, contextNotes = [], + incompletePhases = [], }: { sha: string isFirstRun: boolean @@ -398,18 +431,17 @@ 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" - 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 ? "" @@ -418,6 +450,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(", ")}_` + const incompleteNote = buildIncompleteNote(incompletePhases) const contextSection = contextNotes.length === 0 ? "" @@ -429,6 +462,7 @@ export const buildStatusComment = ({ findingsLine, unpostedNote, capNote, + incompleteNote, contextSection, attribution, ] diff --git a/src/review/merge-phase-findings.ts b/src/review/merge-phase-findings.ts new file mode 100644 index 0000000..12c2476 --- /dev/null +++ b/src/review/merge-phase-findings.ts @@ -0,0 +1,65 @@ +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 + * 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[][], +): MergedPhaseFindings => { + const phased = findingsByPhase.flatMap((phaseFindings, phaseIndex) => { + return phaseFindings.map((finding) => ({ finding, phaseIndex })) + }) + + const kept = phased.reduce((keptSoFar, candidate) => { + const overlapping = keptSoFar.filter((entry) => + isCrossPhaseDuplicate(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 { + findings: kept.map((entry) => entry.finding), + duplicatesAcrossPhases: phased.length - kept.length, + } +} diff --git a/src/review/phases.ts b/src/review/phases.ts index 24d5fa4..a893c8d 100644 --- a/src/review/phases.ts +++ b/src/review/phases.ts @@ -1,8 +1,18 @@ /** - * 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. + * + * - 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, 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. * * 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 +23,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 @@ -198,7 +210,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: @@ -294,7 +311,18 @@ 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. */ +/** 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 +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 +334,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`, ) } diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 3c08460..1c85995 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 @@ -218,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 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 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..46bbb97 --- /dev/null +++ b/src/review/run-stages.ts @@ -0,0 +1,163 @@ +/** + * 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" +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 + +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 => { + return ( + typeof error === "object" && + error !== null && + "aborted" in error && + error.aborted === true + ) +} + +/** Fires one stage's phases concurrently. Each phase is tried + * independently — a failed phase does not cancel its siblings. */ +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", + ), + } +} + +/** 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[] => { + 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 findings (non-findings removed) to the next stage as + * prior findings. + * Outcomes come back in stage-then-phase order regardless of completion + * order. Throws AllPhasesFailedError only when no phase completed. + */ +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 anyCompleted = outcomes.some( + (outcome) => outcome.status === "completed", + ) + if (anyCompleted) return outcomes + throw new AllPhasesFailedError(outcomes) +} 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,