diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 7435d12..da6d018 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -117,7 +117,7 @@ describe("parseConfig", () => { githubToken: "ghs_testtoken", openrouterApiKey: "sk-or-testkey", model: "anthropic/claude-sonnet-4-6", - fallbackModel: "", + fallbackModel: null, requestTimeoutSeconds: 600, reviewTimeoutSeconds: 1500, maxFindings: undefined, @@ -160,6 +160,12 @@ describe("parseConfig", () => { }) }) + it("keeps a configured fallback model", () => { + const config = parseConfig(makeRawInputs({ fallbackModel: "z-ai/glm-5.3-flash:online" })) + + expect(config.fallbackModel).toBe("z-ai/glm-5.3-flash:online") + }) + it.each(["1", "2400", "2147483"])("accepts review_timeout_seconds %s", (value) => { expect(parseConfig(makeRawInputs({ reviewTimeoutSeconds: value })).reviewTimeoutSeconds).toBe( Number(value), diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index e05a7e5..24a422c 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -21,7 +21,8 @@ import { buildStatusComment, computeAnchorKey, mapFindingsToReview, - renderStandaloneFinding, + renderBeyondDiffFinding, + renderReroutedFinding, REVIEW_MARKER, STATUS_ANCHOR, type ReviewComment, @@ -216,7 +217,7 @@ const baseConfig: ActionConfig = { githubToken: "ghp_test", openrouterApiKey: "sk-test", model: "test/model", - fallbackModel: "", + fallbackModel: null, requestTimeoutSeconds: 600, reviewTimeoutSeconds: 1500, maxFindings: undefined, @@ -910,14 +911,25 @@ describe("orchestrate", () => { conventionsNote: null, }) + const expectedReview = expectedFindingsReview(expectedSelection.selected) + expect(stubs.submitReviewCalls).toHaveLength(0) - expect(stubs.postFindingsReviewCalls).toEqual([ - expectedFindingsReview(expectedSelection.selected), + expect(stubs.postFindingsReviewCalls).toEqual([expectedReview]) + expect(logsWithMessage(logger, "findings review posted")).toEqual([ + { + level: "info", + message: "findings review posted", + data: { + reviewUrl: "https://github.com/test/review/1", + inlineCount: expectedReview.comments.length, + locations: expectedReview.comments.map((comment) => `${comment.path}:${comment.line}`), + }, + }, ]) expect(stubs.postIssueCommentCalls).toEqual( expectedMapped.standaloneFindings.map((finding) => ({ prNumber: fixturePrContext.prNumber, - body: renderStandaloneFinding(finding), + body: renderBeyondDiffFinding(finding), })), ) expect(stubs.upsertSummaryCommentCalls).toEqual([ @@ -1355,22 +1367,24 @@ describe("orchestrate", () => { expect(call.budgetTokens).toBe(expectedRemainingTokens) }) - it("passes paths extracted from the diff to readChangedFiles", async () => { + it("passes readChangedFiles each new path plus a rename's old path, and no deleted path", async () => { const stubs = makeOrchestrateDeps() const logger = createTestLogger() await orchestrate(stubs.deps, logger) - expect(stubs.readChangedFilesCalls).toHaveLength(1) - const changedPaths = first(stubs.readChangedFilesCalls).changedPaths - // The fixture diff modifies src/greeter.ts, adds src/added-file.ts, - // renames to src/new-name.ts, modifies src/no-trailing-newline.ts, and - // deletes src/removed-file.ts (null newFilePath) + binary logo.png - expect(changedPaths).toContain("src/greeter.ts") - expect(changedPaths).toContain("src/added-file.ts") - expect(changedPaths).toContain("src/new-name.ts") - // Deleted file has no newFilePath — should NOT appear - expect(changedPaths).not.toContain("src/removed-file.ts") + // The fixture diff deletes src/removed-file.ts, so that path is absent. + // It renames src/old-name.ts to src/new-name.ts, so both paths appear + expect(stubs.readChangedFilesCalls.map((call) => call.changedPaths)).toEqual([ + [ + "src/greeter.ts", + "src/added-file.ts", + "src/new-name.ts", + "src/old-name.ts", + "assets/logo.png", + "src/no-trailing-newline.ts", + ], + ]) }) it("passes remaining budget after related files to findRelatedDocs", async () => { @@ -2591,11 +2605,51 @@ describe("orchestrate", () => { expect(stubs.postIssueCommentCalls).toEqual( postedFindings.map((finding) => ({ prNumber: 7, - body: renderStandaloneFinding(finding), + body: renderReroutedFinding(finding), })), ) }) + it("keeps the beyond-diff note only on beyond-diff findings when GitHub rejects the inline review", async () => { + const beyondDiffFinding = makeFinding({ file: "src/untouched.ts", line: 400 }) + const inDiffFinding = makeFinding() + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "checked", findings: [beyondDiffFinding, inDiffFinding] }, + }, + contextReader: { + findRelatedFiles: async () => ({ + files: [ + { + path: "src/untouched.ts", + content: "import { greet } from './greeter.js'", + includedAs: "full", + reason: "imports src/greeter.ts", + }, + ], + excludedByCapPaths: [], + }), + }, + githubClient: { + postFindingsReview: async () => ({ kind: "rejected" as const }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.postIssueCommentCalls).toEqual([ + { + prNumber: 7, + body: renderBeyondDiffFinding(withRoutedModel(beyondDiffFinding, "test/model")), + }, + { + prNumber: 7, + body: renderReroutedFinding(withRoutedModel(inDiffFinding, "test/model")), + }, + ]) + }) + it("continues when the findings review post throws", async () => { const stubs = makeOrchestrateDeps({ githubClient: { @@ -2616,7 +2670,7 @@ describe("orchestrate", () => { expect(stubs.postIssueCommentCalls).toEqual( expectedMapped.standaloneFindings.map((finding) => ({ prNumber: 7, - body: renderStandaloneFinding(finding), + body: renderBeyondDiffFinding(finding), })), ) expect(stubs.upsertSummaryCommentCalls).toEqual([ @@ -2684,11 +2738,14 @@ describe("orchestrate", () => { }), ]) expect( - logsWithMessage(logger, "failed to post beyond-diff finding — it will re-report next run"), + logsWithMessage( + logger, + "failed to post finding as an issue comment — it will re-report next run", + ), ).toEqual([ { level: "warn", - message: "failed to post beyond-diff finding — it will re-report next run", + message: "failed to post finding as an issue comment — it will re-report next run", data: { error: "[Error]: boom", file: "src/untouched.ts", @@ -2696,6 +2753,69 @@ describe("orchestrate", () => { }, }, ]) + expect(logsWithMessage(logger, "findings posted as issue comments")).toEqual([]) + }) + + it("logs the count of findings posted as issue comments, leaving out a failed post", async () => { + const failedFinding = makeFinding({ + file: "src/untouched.ts", + line: 400, + title: "Unchecked greet result", + }) + const postedFinding = makeFinding({ + file: "src/untouched.ts", + line: 420, + title: "Caller ignores the empty-key throw", + }) + const failedBody = renderBeyondDiffFinding(withRoutedModel(failedFinding, "test/model")) + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "checked", findings: [failedFinding, postedFinding] }, + }, + contextReader: { + findRelatedFiles: async () => ({ + files: [ + { + path: "src/untouched.ts", + content: "import { greet } from './greeter.js'", + includedAs: "full", + reason: "imports src/greeter.ts", + }, + ], + excludedByCapPaths: [], + }), + }, + githubClient: { + postIssueComment: async ({ body }) => { + if (body === failedBody) throw new Error("boom") + return { url: "https://github.com/test/comment/2" } + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + // The failed post's warning proves both findings reached the posting loop + expect( + logsWithMessage( + logger, + "failed to post finding as an issue comment — it will re-report next run", + ), + ).toEqual([ + { + level: "warn", + message: "failed to post finding as an issue comment — it will re-report next run", + data: { error: "[Error]: boom", file: "src/untouched.ts", line: 400 }, + }, + ]) + expect(logsWithMessage(logger, "findings posted as issue comments")).toEqual([ + { + level: "info", + message: "findings posted as issue comments", + data: { count: 1, locations: ["src/untouched.ts:420"] }, + }, + ]) }) it("uses complete PrContext from pull_request event without fetching", async () => { @@ -2823,7 +2943,7 @@ describe("orchestrate", () => { expect(stubs.postIssueCommentCalls).toEqual([ { prNumber: 7, - body: renderStandaloneFinding(withRoutedModel(relatedFileFinding, "test/model")), + body: renderBeyondDiffFinding(withRoutedModel(relatedFileFinding, "test/model")), }, ]) }) @@ -2927,11 +3047,11 @@ describe("orchestrate", () => { expect(stubs.postIssueCommentCalls).toEqual([ { prNumber: 7, - body: renderStandaloneFinding(withRoutedModel(renamedFromFinding, "test/model")), + body: renderBeyondDiffFinding(withRoutedModel(renamedFromFinding, "test/model")), }, { prNumber: 7, - body: renderStandaloneFinding(withRoutedModel(deletedFileFinding, "test/model")), + body: renderBeyondDiffFinding(withRoutedModel(deletedFileFinding, "test/model")), }, ]) }) @@ -2958,7 +3078,7 @@ describe("orchestrate", () => { expect(foundStubs.postIssueCommentCalls).toEqual([ { prNumber: 7, - body: renderStandaloneFinding(withRoutedModel(conventionsFinding, "test/model")), + body: renderBeyondDiffFinding(withRoutedModel(conventionsFinding, "test/model")), }, ]) expect(missingResult.findingsCount).toBe(0) @@ -4032,7 +4152,7 @@ describe("staged phases", () => { .mockResolvedValueOnce(response) .mockImplementation(() => lateResponse.promise) const client = createOpenRouterClient({ - sdk: { chat: { send } }, + sdk: { chat: { send }, generations: { getGeneration: vi.fn() } }, requestTimeoutMs: 900_000, remainingReviewMs, }) @@ -4139,7 +4259,7 @@ describe("staged phases", () => { return response }) const client = createOpenRouterClient({ - sdk: { chat: { send } }, + sdk: { chat: { send }, generations: { getGeneration: vi.fn() } }, requestTimeoutMs: 900_000, remainingReviewMs: () => Infinity, retryDelayMs: 0, @@ -4160,7 +4280,7 @@ describe("staged phases", () => { const acceptedByPhase = logsWithMessage(generateLogger, "review response accepted").toSorted( (left, right) => String(left.data.phase).localeCompare(String(right.data.phase)), ) - const acceptedEntry = (phase: string, attemptCount: number) => ({ + const acceptedEntry = (phase: string, totalAttemptCount: number) => ({ level: "info", message: "review response accepted", data: { @@ -4168,7 +4288,7 @@ describe("staged phases", () => { model: "test/model", routedModel: "test/model", generationId: "completed", - attemptCount, + totalAttemptCount, }, }) @@ -4185,7 +4305,7 @@ describe("staged phases", () => { data: { phase: "correctness-security", model: "test/model", - attemptNumber: 1, + modelAttemptNumber: 1, outcome: "api_error", errorSummary: "HTTP 500: HTTP 500", }, @@ -4215,7 +4335,7 @@ describe("staged phases", () => { throw new ReviewRequestError({ message: "review request failed after 1 attempt(s)", attempts: [timeoutAttempt], - aborted: false, + keyRejected: false, }) } return { @@ -4361,7 +4481,7 @@ describe("staged phases", () => { throw new ReviewRequestError({ message: "OpenRouter auth/credit error — aborting without fallback", attempts: [billedAttempt], - aborted: true, + keyRejected: true, }) }, }) @@ -4402,7 +4522,7 @@ describe("staged phases", () => { throw new ReviewRequestError({ message: "review request failed after 1 attempt(s)", attempts: [timeoutAttempt], - aborted: false, + keyRejected: false, }) }, }) @@ -4428,6 +4548,43 @@ describe("staged phases", () => { ]) }) + it("leaves the cost table out of the failure summary when cost_summary is off", async () => { + const timeoutAttempt: ModelAttempt = { + model: "test/model", + outcome: "timeout", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "no response within 900s", + } + const stubs = makeOrchestrateDeps({ + config: { costSummary: false }, + generateFindings: async () => { + throw new ReviewRequestError({ + message: "review request failed after 1 attempt(s)", + attempts: [timeoutAttempt], + keyRejected: 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)", + }, + }, + ]) + }) + it("lists each attempted model once in the failure summary", async () => { const primaryTimeout: ModelAttempt = { model: "test/model", @@ -4444,7 +4601,7 @@ describe("staged phases", () => { throw new ReviewRequestError({ message: "review request failed after 3 attempt(s)", attempts, - aborted: false, + keyRejected: false, }) }, }) diff --git a/src/config.ts b/src/config.ts index cd9a909..ab92772 100644 --- a/src/config.ts +++ b/src/config.ts @@ -133,7 +133,8 @@ const configSchema = z.object({ githubToken: z.string().min(1, "github_token is required"), openrouterApiKey: z.string().min(1, "openrouter_api_key is required"), model: z.string().transform((value) => value || "anthropic/claude-sonnet-4-6"), - fallbackModel: z.string(), + // Empty means no fallback model + fallbackModel: z.string().transform((value) => value || null), requestTimeoutSeconds: timerSafeSeconds(900), reviewTimeoutSeconds: timerSafeSeconds(1500), maxFindings: optionalPositiveInteger, diff --git a/src/github/client.ts b/src/github/client.ts index b61854d..96b7640 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -390,8 +390,8 @@ export const createGithubClient = ( } /** The bot's own issue comments on the PR — the status comment plus any - * beyond-diff finding comments. One listing serves both the run signal - * and the beyond-diff dedup anchors. Author-filtered for the same reason + * finding issue comments. One listing serves both the run signal and the + * dedup anchors of findings posted as issue comments. Author-filtered for the same reason * as fetchBotReviewComments: anyone can paste an anchor marker. */ const fetchBotIssueComments = async ({ prNumber, diff --git a/src/main.ts b/src/main.ts index 72b5b6e..8cdb469 100644 --- a/src/main.ts +++ b/src/main.ts @@ -5,7 +5,7 @@ import envVar from "env-var" import { parseConfig, type RawInputs } from "./config.js" import { createContextReader } from "./context/workspace.js" import { createGithubClient } from "./github/client.js" -import { createLogger } from "./logger.js" +import { createLogger, describeError } from "./logger.js" import { createOpenRouterClient } from "./openrouter/client.js" import { createPromptedGenerateFindings, orchestrate } from "./orchestrate.js" @@ -14,12 +14,12 @@ const logger = createLogger("umm-actually") process.on("unhandledRejection", (error) => { logger.warn("unhandled promise rejection (likely SDK internal)", { - error: error instanceof Error ? `[${error.name}]: ${error.message}` : String(error), + error: describeError(error), }) }) -// Cleanups to run when the job is cancelled, registered while a check run -// is open — typically one entry, the open check run's completion call. +// Cleanups to run when the job is cancelled. It holds at most one entry, the +// open check run's completion call, registered only while that run is open. // Mutable on purpose — signal handlers can only reach shared state const cancellationCleanups = new Set<() => Promise>() @@ -35,14 +35,19 @@ let cancellationExitStarted = false const exitOnCancellationSignal = (signalName: NodeJS.Signals): void => { if (cancellationExitStarted) return cancellationExitStarted = true - // Observed, not awaited — a signal handler cannot await, and the cleanups - // never throw + // Observed, not awaited — a signal handler cannot await. The one cleanup that + // can be registered is completeCheckRunSafely, which catches its own errors void (async () => { - logger.warn("cancellation signal received — closing the check run", { - signal: signalName, - }) const pendingCleanups = [...cancellationCleanups] cancellationCleanups.clear() + + // A signal can arrive before the check run exists, so the line states + // whether there is one to close + logger.warn("cancellation signal received — closing any open check run before exit", { + signal: signalName, + checkRunOpen: pendingCleanups.length > 0, + }) + for (const pendingCleanup of pendingCleanups) { await pendingCleanup() } @@ -57,10 +62,11 @@ process.on("SIGTERM", exitOnCancellationSignal) * - Strings come from getInput. * - Booleans come pre-parsed from getBooleanInput, which accepts only the YAML * 1.2 core-schema values (true|True|TRUE / false|False|FALSE) and throws on - * anything else, an empty value included. An empty boolean input therefore - * skips it and passes undefined. - * - The runner fills in the action.yml default for an omitted input; - * parseConfig applies it for an explicitly empty one. + * anything else, an empty value included. So for an empty boolean input, + * collectRawInputs skips getBooleanInput and passes undefined. + * - The runner fills in the action.yml default for an omitted input. For an + * explicitly empty one, parseConfig applies the same default, except for + * priority_docs, where empty disables priority docs. */ const collectRawInputs = (): RawInputs => ({ githubToken: core.getInput("github_token", { required: true }), @@ -135,7 +141,7 @@ try { remainingReviewMs, }), model: config.model, - fallbackModel: config.fallbackModel === "" ? null : config.fallbackModel, + fallbackModel: config.fallbackModel, }, logger, ), diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts index fc51ffc..54923dd 100644 --- a/src/openrouter/__tests__/client.test.ts +++ b/src/openrouter/__tests__/client.test.ts @@ -176,12 +176,12 @@ describe("requestReview", () => { expect({ message: error.message, attempts: error.attempts, - aborted: error.aborted, + keyRejected: error.keyRejected, deadlineExceeded: error.deadlineExceeded, }).toEqual({ message: "review deadline exceeded", attempts: [], - aborted: false, + keyRejected: false, deadlineExceeded: true, }) expect(stub.sendCalls).toEqual([]) @@ -237,12 +237,12 @@ describe("requestReview", () => { expect({ message: error.message, attempts: error.attempts, - aborted: error.aborted, + keyRejected: error.keyRejected, deadlineExceeded: error.deadlineExceeded, }).toEqual({ message: "review deadline exceeded", attempts: expectedAttempts, - aborted: false, + keyRejected: false, deadlineExceeded: true, }) expect( @@ -358,6 +358,17 @@ describe("requestReview", () => { requestParams.model, ]) expect(stub.generationCalls).toEqual([]) + expect(logsWithMessage(logger, "generation cost lookup skipped")).toEqual([ + { + level: "debug", + message: "generation cost lookup skipped", + data: { + operation: "generation cost lookup", + generationId: "gen-no-cost", + reason: "review deadline exceeded", + }, + }, + ]) }) it("retains an accepted review when cost lookup reaches the review deadline and ignores its late response", async () => { @@ -649,7 +660,7 @@ describe("requestReview", () => { message: "review attempt failed", data: { model: "openai/gpt-5-mini", - attemptNumber: 1, + modelAttemptNumber: 1, outcome: "api_error", errorSummary: "Request timed out", }, @@ -812,7 +823,7 @@ describe("requestReview", () => { message: "review attempt failed", data: { model: "openai/gpt-5-mini", - attemptNumber: 1, + modelAttemptNumber: 1, outcome: "timeout", errorSummary: "no response within 45s", }, @@ -1191,7 +1202,6 @@ describe("requestReview", () => { const stub = makeSdkStub({ sendResponses: [{ value: acceptedChatResult }], }) - delete stub.sdk.generations const { client, logger } = makeClient(stub) const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout") @@ -1223,7 +1233,7 @@ describe("requestReview", () => { ).rejects.toThrow("review request failed") // retryDelayMs sleeps happen between retryable failures; the guard - // `attemptNumber <= MAX_ATTEMPTS_PER_MODEL` prevents an extra sleep + // `modelAttemptNumber <= MAX_ATTEMPTS_PER_MODEL` prevents an extra sleep // after the final attempt. Only one sleep (between attempts 1 and 2). const retrySleepCalls = setTimeoutSpy.mock.calls.filter((call) => call[1] === retryDelayMs) expect(retrySleepCalls).toHaveLength(1) @@ -1517,7 +1527,7 @@ describe("requestReview", () => { if (!(failure instanceof ReviewRequestError)) { throw new Error("expected a ReviewRequestError") } - expect(failure.aborted).toBe(false) + expect(failure.keyRejected).toBe(false) expect(failure.attempts).toEqual([ { model: "openai/gpt-5-mini", @@ -1538,7 +1548,7 @@ describe("requestReview", () => { ]) }) - it("marks the error as aborted on an auth/credit failure", async () => { + it("marks the error as key-rejected on an auth/credit failure", async () => { const stub = makeSdkStub({ sendResponses: [{ error: makeStatusError(402) }], }) @@ -1549,7 +1559,7 @@ describe("requestReview", () => { if (!(failure instanceof ReviewRequestError)) { throw new Error("expected a ReviewRequestError") } - expect(failure.aborted).toBe(true) + expect(failure.keyRejected).toBe(true) expect(failure.attempts).toEqual([ { model: "openai/gpt-5-mini", @@ -1714,15 +1724,4 @@ describe("requestReview", () => { }, ]) }) - - it("skips the cost lookup entirely when the sdk has no generations surface", async () => { - const sdkWithoutGenerations: OpenRouterLike = { - chat: { send: async () => makeNoCostChatResult() }, - } - const { client, logger } = makeClient({ sdk: sdkWithoutGenerations }) - - const result = await client.requestReview(requestParams, logger) - - expect(result.attempts[0]?.costUsd).toBeNull() - }) }) diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts index f4c4bed..0b95185 100644 --- a/src/openrouter/client.ts +++ b/src/openrouter/client.ts @@ -11,7 +11,9 @@ export type AttemptOutcome = "accepted" | "api_error" | "timeout" | "empty_content" | "invalid_json" | "schema_mismatch" export type ModelAttempt = { - /** The model slug requested for this attempt (ladder position, not routing). */ + /** The model this attempt requested. The ladder is the primary model, then + * the fallback when one is set. OpenRouter's routed model is reported only + * for the accepted attempt, as StructuredReviewResult.modelUsed. */ model: string outcome: AttemptOutcome promptTokens: number | null @@ -60,7 +62,7 @@ export type OpenRouterLike = { }, ): Promise } - generations?: { + generations: { getGeneration( request: { id: string }, options?: { @@ -103,8 +105,9 @@ const generationResponseSchema = z.object({ data: z.object({ totalCost: z.number() }), }) -/** Auth/credit failures abort the ladder — the fallback model shares the key. */ -const ABORT_STATUSES = new Set([401, 402, 403]) +/** Auth and credit statuses. The fallback model shares the key, so these end + * the ladder without trying it. */ +const KEY_REJECTED_STATUSES = new Set([401, 402, 403]) /** The transient 4xx statuses — HTTP request timeout (408) and rate limit * (429). attemptOnce also marks 5xx and status-less network errors @@ -251,8 +254,9 @@ const withDeadline = async ( const controller = new AbortController() const startedAt = DateTime.now() - // Promise.try settles a synchronous throw from start as a rejection, and - // toResult wraps it before the race so a late rejection never goes unhandled + // Promise.try passes controller.signal on to start and settles a synchronous + // throw from start as a rejection. toResult wraps it before the race so a + // late rejection never goes unhandled const settled = toResult(Promise.try(start, controller.signal)) const deadline = Promise.withResolvers>() @@ -338,7 +342,8 @@ type SingleAttempt = kind: "failed" attempt: ModelAttempt retryable: boolean - abort: boolean + /** An auth or credit status. The key fails for every model, so the ladder stops. */ + keyRejected: boolean } | { kind: "context_overflow" @@ -350,29 +355,28 @@ type SingleAttempt = * attempt, failed ones included. */ export class ReviewRequestError extends Error { readonly attempts: ModelAttempt[] - /** An auth or credit error (401/402/403) stopped the ladder. The key fails - * for every model, so callers skip the remaining work. Unrelated to the - * AbortSignal that cancels a single HTTP call. */ - readonly aborted: boolean - /** The review deadline ran out. That deadline is the whole run's time - * budget, which `remainingReviewMs` counts down. */ + /** An auth or credit status (401/402/403) stopped the ladder. The key fails + * for every model, so the stage runner skips the stages after this one. */ + readonly keyRejected: boolean + /** The whole review's time budget ran out, as reported by the + * `remainingReviewMs` that createOpenRouterClient takes. */ readonly deadlineExceeded: boolean constructor({ message, attempts, - aborted, + keyRejected, deadlineExceeded = false, }: { message: string attempts: ModelAttempt[] - aborted: boolean + keyRejected: boolean deadlineExceeded?: boolean }) { super(message) this.name = "ReviewRequestError" this.attempts = attempts - this.aborted = aborted + this.keyRejected = keyRejected this.deadlineExceeded = deadlineExceeded } } @@ -408,6 +412,18 @@ export const createOpenRouterClient = ({ }): OpenRouterClient => { const requestTimeoutSummary = `no response within ${Math.round(requestTimeoutMs / 1000)}s` + /** How long the next bounded call may run, and the error summary if it + * times out. When remainingMs is at or below requestTimeoutMs, the review + * deadline ends the call first, so the summary blames it. */ + const getCallTimeout = (remainingMs: number): { timeoutMs: number; timeoutSummary: string } => { + const reviewDeadlineIsBinding = remainingMs <= requestTimeoutMs + + return { + timeoutMs: Math.min(requestTimeoutMs, remainingMs), + timeoutSummary: reviewDeadlineIsBinding ? "review deadline exceeded" : requestTimeoutSummary, + } + } + /** Sends one chat request under the per-attempt deadline. Its only log lines * are withDeadline's; requestReview logs each attempt's outcome. */ const attemptOnce = async ( @@ -415,18 +431,17 @@ export const createOpenRouterClient = ({ logger: Logger, ): Promise => { const { model } = chatRequest - const remainingMs = remainingReviewMs() - // The review deadline, not the per-attempt timeout, ends this attempt - // if it runs long - const reviewDeadlineIsBinding = remainingMs <= requestTimeoutMs + // No remaining-time guard here. requestReview calls ensureReviewTimeRemaining + // before every attempt, so some review time is always left + const callTimeout = getCallTimeout(remainingReviewMs()) const sendResult = await withDeadline( { start: (signal) => { return sdk.chat.send({ chatRequest }, { retries: { strategy: "none" }, signal }) }, - timeoutMs: Math.min(requestTimeoutMs, remainingMs), + timeoutMs: callTimeout.timeoutMs, }, logger.child({ operation: "chat request", model }), ) @@ -442,12 +457,10 @@ export const createOpenRouterClient = ({ promptTokens: null, completionTokens: null, costUsd: null, - errorSummary: reviewDeadlineIsBinding - ? "review deadline exceeded" - : requestTimeoutSummary, + errorSummary: callTimeout.timeoutSummary, }, retryable: true, - abort: false, + keyRejected: false, } } if (sendResult.status === "rejected") { @@ -474,14 +487,14 @@ export const createOpenRouterClient = ({ return { kind: "context_overflow", attempt, fittedMaxCompletionTokens: fittedCeiling } } - const abort = statusCode ? ABORT_STATUSES.has(statusCode) : false + const keyRejected = statusCode ? KEY_REJECTED_STATUSES.has(statusCode) : false // An unknown status, a 5xx, a 408, or a 429 is transient and retries; - // any other 4xx, such as a 400 bad request or a 401/402/403 abort - // status, is structural and does not + // any other 4xx, such as a 400 bad request or a 401/402/403 auth or + // credit status, is structural and does not const retryable = !statusCode || statusCode >= 500 || RETRYABLE_4XX_STATUSES.has(statusCode) - return { kind: "failed", attempt, retryable, abort } + return { kind: "failed", attempt, retryable, keyRejected } } const parsedResult = chatResultSchema.safeParse(sendResult.value) @@ -500,7 +513,7 @@ export const createOpenRouterClient = ({ errorSummary: "unexpected chat response shape", }, retryable: true, - abort: false, + keyRejected: false, } } @@ -516,7 +529,7 @@ export const createOpenRouterClient = ({ kind: "failed", attempt: { model, outcome, ...usage, errorSummary }, retryable: true, - abort: false, + keyRejected: false, }) const content = chatResult.choices[0]?.message.content @@ -558,35 +571,33 @@ export const createOpenRouterClient = ({ generationId: string, logger: Logger, ): Promise => { - const generations = sdk.generations const remainingMs = remainingReviewMs() - if (!generations || remainingMs <= 0) return null - - // The review deadline, not the per-request timeout, ends the lookup if it - // runs long - const reviewDeadlineIsBinding = remainingMs <= requestTimeoutMs - // Every lookup line names the generation, so a failure ties back to it const lookupLogger = logger.child({ operation: "generation cost lookup", generationId }) + if (remainingMs <= 0) { + lookupLogger.debug("generation cost lookup skipped", { reason: "review deadline exceeded" }) + return null + } + + const callTimeout = getCallTimeout(remainingMs) + const lookup = await withDeadline( { start: (signal) => { - return generations.getGeneration( + return sdk.generations.getGeneration( { id: generationId }, { retries: { strategy: "none" }, signal }, ) }, - timeoutMs: Math.min(requestTimeoutMs, remainingMs), + timeoutMs: callTimeout.timeoutMs, }, lookupLogger, ) if (lookup.status === "timed_out") { - lookupLogger.warn("generation cost lookup failed", { - error: reviewDeadlineIsBinding ? "review deadline exceeded" : requestTimeoutSummary, - }) + lookupLogger.warn("generation cost lookup failed", { error: callTimeout.timeoutSummary }) return null } if (lookup.status === "rejected") { @@ -618,7 +629,7 @@ export const createOpenRouterClient = ({ throw new ReviewRequestError({ message: "review deadline exceeded", attempts, - aborted: false, + keyRejected: false, deadlineExceeded: true, }) } @@ -626,12 +637,12 @@ export const createOpenRouterClient = ({ for (const [ladderIndex, ladderModel] of modelLadder.entries()) { const nextLadderModel = modelLadder[ladderIndex + 1] - // Reassigned across attempts, like attemptNumber. Each model starts at - // the full ceiling because that model's endpoints decide what fits, so - // a context overflow lowers it only for the same model's retry + // Both are reassigned across this model's attempts. Each model starts at + // the full ceiling because its own endpoints decide what fits, so a + // context overflow lowers the ceiling only for the same model's retry let maxCompletionTokens = MAX_COMPLETION_TOKENS - let attemptNumber = 1 - while (attemptNumber <= MAX_ATTEMPTS_PER_MODEL) { + let modelAttemptNumber = 1 + while (modelAttemptNumber <= MAX_ATTEMPTS_PER_MODEL) { ensureReviewTimeRemaining() const attemptResult = await attemptOnce( buildChatRequest({ systemPrompt, userPrompt, model: ladderModel, maxCompletionTokens }), @@ -648,7 +659,7 @@ export const createOpenRouterClient = ({ model: ladderModel, routedModel: attemptResult.routedModel, generationId: attemptResult.generationId, - attemptCount: attempts.length, + totalAttemptCount: attempts.length, }) return { review: attemptResult.review, @@ -660,16 +671,16 @@ export const createOpenRouterClient = ({ attempts.push(attemptResult.attempt) logger.warn("review attempt failed", { model: ladderModel, - attemptNumber, + modelAttemptNumber, outcome: attemptResult.attempt.outcome, errorSummary: attemptResult.attempt.errorSummary, }) - if (attemptResult.kind === "failed" && attemptResult.abort) { + if (attemptResult.kind === "failed" && attemptResult.keyRejected) { throw new ReviewRequestError({ message: `OpenRouter auth/credit error — aborting without fallback: ${summarizeAttempts(attempts)}`, attempts, - aborted: true, + keyRejected: true, }) } @@ -690,10 +701,13 @@ export const createOpenRouterClient = ({ }) break } + + // A structural failure ends this model's attempts. A context overflow + // is not a `failed` result, so it goes on to the fitted-ceiling retry if (attemptResult.kind === "failed" && !attemptResult.retryable) break - attemptNumber++ - const retryRemains = attemptNumber <= MAX_ATTEMPTS_PER_MODEL + modelAttemptNumber++ + const retryRemains = modelAttemptNumber <= MAX_ATTEMPTS_PER_MODEL // An overflow on the model's last attempt has no retry left to use the // fitted ceiling, so the loop ends and any next model starts at the @@ -719,7 +733,7 @@ export const createOpenRouterClient = ({ throw new ReviewRequestError({ message: `review request failed after ${attempts.length} attempt(s): ${summarizeAttempts(attempts)}`, attempts, - aborted: false, + keyRejected: false, }) } diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 894ba71..bb78a6f 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -26,7 +26,8 @@ import { extractAnchors, classifyDuplicate, mapFindingsToReview, - renderStandaloneFinding, + renderBeyondDiffFinding, + renderReroutedFinding, REVIEW_MARKER, STATUS_ANCHOR, type AnchorEntry, @@ -177,8 +178,8 @@ type IssueCommentState = { /** The bot's issue comments carry the rest of the cross-run state: the * status comment's presence (the first-run signal) and dedup anchors from - * beyond-diff finding comments (anchor-line positions — issue comments - * aren't line-tracked). Fails open to a first run. */ + * finding issue comments (anchor-line positions — issue comments aren't + * line-tracked). Fails open to a first run. */ const fetchIssueCommentState = async ( { githubClient, prNumber }: { githubClient: GithubClient; prNumber: number }, logger: Logger, @@ -216,7 +217,8 @@ const fetchIssueCommentState = async ( type InlinePostOutcome = { url: string - /** Findings whose anchors GitHub rejected — re-routed to issue comments. */ + /** Every inline finding when GitHub rejected the review, re-routed to issue + * comments. One bad anchor fails the whole review. */ rerouted: AttributedFinding[] /** Inline comments that actually landed — zero when the post failed. */ postedCount: number @@ -251,12 +253,14 @@ const postInlineFindings = async ( comments, }) + // githubClient already warns about the 422 with the rejected comment count if (result.kind === "rejected") { return { url: "", rerouted: inlineFindings, postedCount: 0 } } logger.info("findings review posted", { reviewUrl: result.url, inlineCount: comments.length, + locations: comments.map((comment) => `${comment.path}:${comment.line}`), }) return { url: result.url, rerouted: [], postedCount: comments.length } } catch (postError) { @@ -466,14 +470,14 @@ const filterPhaseFindings = ( * a cost table when every phase failed — the operator still pays. */ const describePipelineFailure = ({ pipelineError, - costSummary, + includeCostSummary, }: { pipelineError: unknown - costSummary: boolean + includeCostSummary: boolean }): string => { const description = describeError(pipelineError) - if (!costSummary || !(pipelineError instanceof AllPhasesFailedError)) { + if (!includeCostSummary || !(pipelineError instanceof AllPhasesFailedError)) { return description } @@ -513,9 +517,8 @@ 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. */ +/** Runs everything after PR-context resolution, from the diff fetch through + * the status comment. orchestrate brackets it with the check-run lifecycle. */ const runReviewPipeline = async ( { deps, @@ -549,7 +552,6 @@ const runReviewPipeline = async ( return { ...SKIPPED_RESULT_BASE, reviewUrl: url, skippedReason: reason } } - // Step 4: diff fetch const diffResult = await githubClient.fetchDiff({ prNumber: prContext.prNumber, }) @@ -558,16 +560,15 @@ const runReviewPipeline = async ( return postSkipReview({ reason: "diff exceeds GitHub's diff API limits" }) } - // Step 5: parse diff const files = parseDiff(diffResult.diff) if (files.length === 0) { return postSkipReview({ reason: "empty diff" }) } - // Step 5.5: diff-level exclusion — generated files leave the review - // subject before the budget check so one oversized artifact cannot - // starve the reviewable rest of the PR + // Diff-excluded files, such as generated artifacts, leave the review before + // the budget check, so one oversized artifact cannot starve the reviewable + // rest of the PR const gitAttributesContent = config.respectLinguistGenerated ? await contextReader.readGitAttributes() : null @@ -595,23 +596,24 @@ const runReviewPipeline = async ( }) } - // Step 6: annotate + token check. The excluded-files trailer sits inside - // the annotated diff string, so its (few) tokens debit the diff budget. + // The excluded-files trailer sits inside the annotated diff string, so its + // few tokens count against the diff budget const excludedFilesNote = renderExcludedFilesNote(excludedDiffFiles) const annotatedDiff = excludedFilesNote ? `${annotateDiff(reviewableFiles)}\n\n${excludedFilesNote}` : annotateDiff(reviewableFiles) const diffTokens = estimateTokens(annotatedDiff) - // The diff gets half the budget; the other half is for context files. - const budgetHalf = Math.floor(config.contextBudgetTokens / 2) - if (diffTokens > budgetHalf) { + // The diff may use at most half the context budget. Context files get + // whatever the diff leaves (fileBudgetTokens below) + const diffTokenLimit = Math.floor(config.contextBudgetTokens / 2) + + if (diffTokens > diffTokenLimit) { return postSkipReview({ - reason: `diff too large for context budget (${diffTokens} tokens, limit ${budgetHalf} of ${config.contextBudgetTokens})`, + reason: `diff too large for context budget (${diffTokens} tokens, limit ${diffTokenLimit} of ${config.contextBudgetTokens})`, }) } - // Step 7: commentable lines const commentableByPath = computeCommentableLines(reviewableFiles) // Diff-excluded files must stay out of every context channel: the trailer @@ -625,21 +627,23 @@ const runReviewPipeline = async ( (docPath) => !diffExcludedPathSet.has(posix.normalize(docPath)), ) - // Step 8: extract changed paths (includes old path for renames so the - // import scanner finds callers that still reference the pre-rename path) - const changedPaths = reviewableFiles - .flatMap((file) => { - const toPath = newFilePath(file) - const fromPath = file.from - // parse-diff: from can be undefined for a binary file and is "/dev/null" - // for an added file (binary or not) — neither is a pre-rename path worth tracing - const isRename = - toPath !== null && fromPath && fromPath !== "/dev/null" && fromPath !== file.to - return isRename ? [toPath, fromPath] : [toPath] - }) - .filter((path) => path !== null) + // A rename contributes its old path too, so the import scanner finds + // callers that still reference the pre-rename path + const changedPaths = reviewableFiles.flatMap((file) => { + const toPath = newFilePath(file) + const fromPath = file.from + + // A deleted file has no new path. Its old path joins the prompt's file + // paths through deletedPaths below + if (toPath === null) return [] + + // parse-diff: from can be undefined for a binary file and is "/dev/null" + // for an added file (binary or not) — neither is a pre-rename path worth tracing + if (!fromPath || fromPath === "/dev/null" || fromPath === toPath) return [toPath] + + return [toPath, fromPath] + }) - // Step 9: context reads const conventions = await contextReader.readConventions({ conventionsFile: config.conventionsFile, }) @@ -740,10 +744,7 @@ const runReviewPipeline = async ( : { files: [], excludedByCapPaths: [] } const relatedFiles = relatedFilesResult.files - const relatedFilesTokens = relatedFiles.reduce( - (sum, file) => sum + estimateTokens(file.content), - 0, - ) + const relatedFilesTokens = sumBy(relatedFiles, (file) => estimateTokens(file.content)) const docBudgetTokens = Math.max(0, remainingTokens - relatedFilesTokens) // Every path whose full text a higher-priority channel already sent. @@ -925,9 +926,8 @@ const runReviewPipeline = async ( diffExcludedFiles: excludedDiffFiles, }) - // Step 9.5: fetch prior bot comments — needed both for the prompt (the - // model sees what's already posted and self-suppresses conceptual dupes) - // and for positional dedup after generation. + // Prior bot comments go into the prompt, so the model sees what is already + // posted and skips conceptual duplicates. Positional dedup uses them too const inlineState = await fetchInlineCommentState( { githubClient, prNumber: prContext.prNumber }, logger, @@ -940,7 +940,7 @@ const runReviewPipeline = async ( .map(stripAnchorComment) .slice(-PRIOR_COMMENT_CAP) - // Step 10–11: generate findings — one model call per phase, stages in order + // Each phase makes one model call, and the stages run in order const runPhase: RunPhase = ({ phase, priorFindings }) => { return generateFindings({ prContext, @@ -975,8 +975,8 @@ const runReviewPipeline = async ( incomplete: incompletePhaseIds(phases), }) - // Step 12: per phase, drop non-findings and findings on files the model - // never saw, then collapse cross-phase duplicates — all before selection so + // Each phase drops non-findings and findings on files the model never saw, + // then cross-phase duplicates collapse. All of it runs before selection so // cap slots aren't wasted const filteredPhases = completedPhases.map((outcome) => { return filterPhaseFindings({ outcome, knownPaths: promptFilePaths }, logger) @@ -995,10 +995,10 @@ const runReviewPipeline = async ( duplicatesAcrossPhases, }) - // Step 12.5: cross-run dedup — every run walks the same path; a first run - // is just the case where no bot comments exist yet. Sources: the bot's - // inline comments (live positions) and its beyond-diff issue comments - // (anchor lines). Runs before the cap so duplicates don't consume slots. + // Cross-run dedup drops findings the bot already posted. A first run needs no + // special case because it simply finds no anchors. The anchors come from the + // bot's inline comments (live positions) and its finding issue comments + // (anchor lines). Dedup runs before the cap so duplicates don't consume slots. const existingAnchors = [...inlineState.anchors, ...issueState.anchors] const newFindings: AttributedFinding[] = [] const dedupCounts = { positional: 0, content: 0, title: 0 } @@ -1046,16 +1046,19 @@ const runReviewPipeline = async ( droppedByCap: droppedByCap.length, }) - // Step 13: post findings — anchorable ones batch into a single review - // (invisible marker body: one notification, a bare "reviewed" timeline - // event, no prose); the rest post as individual issue comments so every - // new finding is a visible event. All narration lives in the status - // comment. Unposted findings carry no anchor and re-report next run. - const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) + // Findings post in two ways, and all narration lives in the status comment. + // - Anchorable findings batch into one review with an invisible marker body, + // which makes one notification and a bare "reviewed" timeline event. + // - The rest post as individual issue comments, so every new finding is a + // visible event. + // Unposted findings carry no anchor and re-report next run. const { comments, standaloneFindings: unanchoredFindings } = mapFindingsToReview({ findings: selected, commentableByPath, }) + + // mapFindingsToReview returns the same finding objects it was given, so an + // identity check separates the anchored findings from the unanchored ones const inlineFindings = selected.filter((finding) => !unanchoredFindings.includes(finding)) const inlineOutcome = await postInlineFindings( @@ -1069,33 +1072,42 @@ const runReviewPipeline = async ( logger, ) - const standaloneFindings = [...unanchoredFindings, ...inlineOutcome.rerouted] - // Sequential posting with per-comment fallback is inherently stateful — - // each failure drops only its own finding from the posted tally. - let postedStandalone = 0 - for (const finding of standaloneFindings) { + // Beyond-diff findings, plus every in-diff finding when GitHub rejected the + // inline review, since one bad anchor fails the whole review. Each keeps a + // location note that matches where it sits. + const issueCommentPosts = [ + ...unanchoredFindings.map((finding) => ({ finding, body: renderBeyondDiffFinding(finding) })), + ...inlineOutcome.rerouted.map((finding) => ({ finding, body: renderReroutedFinding(finding) })), + ] + + // Each finding posts as its own issue comment. A failed post is logged and + // skipped without stopping the loop, so the list is appended in place and + // holds only the posts that landed + const postedAsIssueComments: AttributedFinding[] = [] + for (const { finding, body } of issueCommentPosts) { try { - await githubClient.postIssueComment({ - prNumber: prContext.prNumber, - body: renderStandaloneFinding(finding), - }) - postedStandalone += 1 + await githubClient.postIssueComment({ prNumber: prContext.prNumber, body }) + postedAsIssueComments.push(finding) } catch (postError) { - logger.warn("failed to post beyond-diff finding — it will re-report next run", { + logger.warn("failed to post finding as an issue comment — it will re-report next run", { error: describeError(postError), file: finding.file, line: finding.line, }) } } - if (postedStandalone > 0) { - logger.info("beyond-diff findings posted", { count: postedStandalone }) + + if (postedAsIssueComments.length > 0) { + logger.info("findings posted as issue comments", { + count: postedAsIssueComments.length, + locations: postedAsIssueComments.map((finding) => `${finding.file}:${finding.line}`), + }) } - // Step 14: status comment — the always-updated run receipt. Counts report + // The status comment is the always-updated run receipt. Counts report // what actually landed; unposted findings self-heal next run and the // comment says so rather than claiming they were posted. - const postedCount = inlineOutcome.postedCount + postedStandalone + const postedCount = inlineOutcome.postedCount + postedAsIssueComments.length const statusBody = buildStatusComment({ sha: prContext.headSha, isFirstRun: !issueState.statusCommentExists, @@ -1153,6 +1165,8 @@ const runReviewPipeline = async ( posted: postedCount, }) + const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) + return { findingsCount: postedCount, reviewUrl: inlineOutcome.url, @@ -1176,13 +1190,13 @@ export const orchestrate = async ( ): Promise => { const { config, githubClient } = deps - // Step 1: fail-fast validation — throws before any network call + // Invalid settings throw here, before any network call const severityThreshold = resolveSeverityThreshold(config.severityThreshold) const stages = resolveStages(config.phases) logger.info("review settings from action inputs", { model: config.model, - fallbackModel: config.fallbackModel || null, + fallbackModel: config.fallbackModel, phases: config.phases, reviewTimeoutSeconds: config.reviewTimeoutSeconds, severityThreshold: config.severityThreshold, @@ -1201,7 +1215,6 @@ export const orchestrate = async ( costSummary: config.costSummary, }) - // Step 2: event resolution const resolvedEvent = resolvePullRequestEvent( { eventName: deps.eventName, @@ -1220,7 +1233,6 @@ export const orchestrate = async ( } } - // Step 3: PR context const prContext: PrContext = resolvedEvent.kind === "complete" ? resolvedEvent.context @@ -1228,7 +1240,7 @@ export const orchestrate = async ( prNumber: resolvedEvent.prNumber, }) - // Step 3.5: open the branded check run now that the head SHA is known + // The branded check run opens once the head SHA is known const checkRun = await createCheckRunSafely({ githubClient, headSha: prContext.headSha }, logger) // A cancelled job stops the container before the completions below run, @@ -1277,7 +1289,7 @@ export const orchestrate = async ( title: "Error — review did not complete", summary: describePipelineFailure({ pipelineError, - costSummary: config.costSummary, + includeCostSummary: config.costSummary, }), }, }, diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 5b277b5..ab7786d 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -8,7 +8,8 @@ import { extractAnchors, isDuplicateFinding, mapFindingsToReview, - renderStandaloneFinding, + renderBeyondDiffFinding, + renderReroutedFinding, STATUS_ANCHOR, type AnchorSource, } from "../comment-mapping.js" @@ -133,7 +134,7 @@ The guard rejects only the exact empty string. }) expect(mapped).toEqual({ comments: [], standaloneFindings: [finding] }) - expect(renderStandaloneFinding(finding)).toBe(`**Whitespace-only keys pass the empty-key guard** + expect(renderBeyondDiffFinding(finding)).toBe(`**Whitespace-only keys pass the empty-key guard** Medium severity · correctness · high confidence \`src/untouched.ts:30\` — beyond the diff's line ranges, in code the changes touch or depend on. @@ -365,11 +366,11 @@ _Reported at line 149 (outside the diff); anchored at nearby changed line 147._ }) }) -describe("renderStandaloneFinding", () => { +describe("renderBeyondDiffFinding", () => { it("renders the full finding block with location note and anchor", () => { const finding = makeFinding({ file: "src/untouched.ts", line: 30 }) - const body = renderStandaloneFinding(finding) + const body = renderBeyondDiffFinding(finding) expect(body).toBe(`**Whitespace-only keys pass the empty-key guard** Medium severity · correctness · high confidence @@ -393,7 +394,7 @@ The guard rejects only the exact empty string. category: "subtle_bugs", }) - const body = renderStandaloneFinding(finding) + const body = renderBeyondDiffFinding(finding) expect(body).toContain("Medium severity · subtle bugs · high confidence") expect(body).toContain("") @@ -406,7 +407,7 @@ The guard rejects only the exact empty string. suggestion: "-old line\n+new line", }) - const body = renderStandaloneFinding(finding) + const body = renderBeyondDiffFinding(finding) expect(body).toBe(`**Whitespace-only keys pass the empty-key guard** Medium severity · correctness · high confidence @@ -434,6 +435,28 @@ The guard rejects only the exact empty string. }) }) +describe("renderReroutedFinding", () => { + it("places the finding at or near a changed line and says why it posted as its own comment", () => { + const finding = makeFinding({ file: "src/greeter.ts", line: 145 }) + + const body = renderReroutedFinding(finding) + + expect(body).toBe(`**Whitespace-only keys pass the empty-key guard** +Medium severity · correctness · high confidence + +\`src/greeter.ts:145\` — at or near a changed line, posted here because GitHub rejected the inline review. + +The guard rejects only the exact empty string. + +**Failure scenario:** register(" ", "value") succeeds and the entry is orphaned. + +--- +*umm-actually · test/model* + +`) + }) +}) + describe("buildStatusComment", () => { it("renders a first run with findings", () => { const body = buildStatusComment({ @@ -723,7 +746,7 @@ describe("extractAnchors", () => { }) it("still finds the anchor behind the model attribution on a rendered comment", () => { - const body = renderStandaloneFinding(makeFinding({ file: "src/untouched.ts", line: 30 })) + const body = renderBeyondDiffFinding(makeFinding({ file: "src/untouched.ts", line: 30 })) const anchors = extractAnchors([anchorSource(body)]) diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts index b474302..b687d42 100644 --- a/src/review/__tests__/run-stages.test.ts +++ b/src/review/__tests__/run-stages.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest" -import { createTestLogger } from "../../__tests__/test-logger.js" +import { createTestLogger, logsWithMessage } from "../../__tests__/test-logger.js" import type { StructuredReviewResult } from "../../openrouter/client.js" import type { Finding } from "../finding.js" import type { ReviewPhase } from "../phases.js" @@ -237,7 +237,7 @@ describe("runStages", () => { 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 abort = Object.assign(new Error("HTTP 401"), { keyRejected: true }) const { runPhase, calls } = makeRunPhase({ a: () => resultA, b: () => Promise.reject(abort), @@ -261,17 +261,48 @@ describe("runStages", () => { ]) }) - it("does not skip later stages after an ordinary failure", async () => { + it("names the skipped phases in a warning after a key rejection", async () => { + const logger = createTestLogger() + const keyRejection = Object.assign(new Error("HTTP 402"), { keyRejected: true }) + const { runPhase } = makeRunPhase({ + a: () => Promise.reject(keyRejection), + }) + + await captureRejection(runStages({ stages: [[phaseA], [phaseB], [phaseC]], runPhase }, logger)) + + expect( + logsWithMessage(logger, "skipping remaining review stages after an auth/credit abort"), + ).toEqual([ + { + level: "warn", + message: "skipping remaining review stages after an auth/credit abort", + data: { skippedPhases: ["b", "c"] }, + }, + ]) + }) + + it.each([ + { label: "an ordinary failure", failure: new Error("HTTP 500") }, + // The client sets keyRejected on every error it throws, so only its value + // may decide the skip + { + label: "a failure with keyRejected: false", + failure: Object.assign(new Error("HTTP 500"), { keyRejected: false }), + }, + ])("does not skip later stages after $label", async ({ failure }) => { const resultC = makeResult() const { runPhase, calls } = makeRunPhase({ - a: () => Promise.reject(new Error("HTTP 500")), + a: () => Promise.reject(failure), 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"]) + expect(outcomes).toEqual([ + { phase: phaseA, status: "failed", error: failure }, + { phase: phaseC, status: "completed", result: resultC }, + ]) }) it.each([ @@ -321,7 +352,7 @@ describe("runStages", () => { data: { phase: "a", modelUsed: "model/a", - attemptCount: 1, + totalAttemptCount: 1, findingsCount: 1, }, }, diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index c3652b2..3af9c53 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -383,13 +383,19 @@ export const REVIEW_MARKER = "" /** Identifies the single updatable status comment. */ export const STATUS_ANCHOR = "" -/** A beyond-diff finding posted as its own issue comment — a new comment is - * a visible event to PR watchers, unlike an in-place status update. Carries +/** Renders a finding posted as its own issue comment. A new comment is a + * visible event to PR watchers, unlike an in-place status update. It carries * its dedup anchor like any inline comment. */ -export const renderStandaloneFinding = (finding: AttributedFinding): string => { +const renderIssueCommentFinding = ({ + finding, + locationNote, +}: { + finding: AttributedFinding + locationNote: string +}): string => { return `${findingHeader(finding)} -\`${finding.file}:${finding.line}\` — beyond the diff's line ranges, in code the changes touch or depend on. +\`${finding.file}:${finding.line}\` — ${locationNote} ${finding.description} @@ -400,6 +406,23 @@ ${attributionLine(finding.modelUsed)} ` } +export const renderBeyondDiffFinding = (finding: AttributedFinding): string => { + return renderIssueCommentFinding({ + finding, + locationNote: "beyond the diff's line ranges, in code the changes touch or depend on.", + }) +} + +/** Renders an in-diff finding posted after GitHub rejected the inline review. + * Its line is at or near a changed line, not beyond the diff. */ +export const renderReroutedFinding = (finding: AttributedFinding): string => { + return renderIssueCommentFinding({ + finding, + locationNote: + "at or near a changed line, posted here because GitHub rejected the inline review.", + }) +} + const buildFindingsLine = ({ isFirstRun, postedCount, diff --git a/src/review/run-stages.ts b/src/review/run-stages.ts index d807914..7fcd705 100644 --- a/src/review/run-stages.ts +++ b/src/review/run-stages.ts @@ -52,10 +52,15 @@ export class AllPhasesFailedError extends Error { // instanceof: review/ is a pure layer and may import openrouter/ only as types, // so the class itself is not available here at runtime. -/** The client marks an auth/credit failure as aborted. The key is bad for +/** The client marks an auth/credit failure as keyRejected. The key is bad for * every model, so no later stage can succeed either. */ -const isAbortedRequest = (error: unknown): boolean => { - return typeof error === "object" && error !== null && "aborted" in error && error.aborted === true +const isKeyRejected = (error: unknown): boolean => { + return ( + typeof error === "object" && + error !== null && + "keyRejected" in error && + error.keyRejected === true + ) } const isDeadlineExceeded = (error: unknown): boolean => { @@ -88,7 +93,7 @@ const runStage = async ( logger.info("review phase completed", { phase: phase.id, modelUsed: result.modelUsed, - attemptCount: result.attempts.length, + totalAttemptCount: result.attempts.length, findingsCount: result.review.findings.length, }) logger.debug("review phase analysis", { @@ -184,12 +189,12 @@ export const runStages = async ( ) outcomes = [...outcomes, ...stageOutcomes] - const aborted = stageOutcomes.some( - (outcome) => outcome.status === "failed" && isAbortedRequest(outcome.error), + const keyRejected = stageOutcomes.some( + (outcome) => outcome.status === "failed" && isKeyRejected(outcome.error), ) const remainingStages = stages.slice(stageIndex + 1) - if (aborted && remainingStages.length > 0) { + if (keyRejected && remainingStages.length > 0) { const skippedPhases = remainingStages.flat() logger.warn("skipping remaining review stages after an auth/credit abort", { skippedPhases: skippedPhases.map((phase) => phase.id),