From 902aa4ed4593fa52349c5e64ebea4a75e4c93ba7 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:11:54 -0400
Subject: [PATCH 1/9] refactor: clear readability pauses in the client,
 orchestrator, and entrypoint

- client.ts: define the ladder on ModelAttempt.model, rename the failed
  attempt's `abort` flag to `keyRejected` (and its status set to match),
  compute the call deadline and its timeout summary once, debug-log the two
  skipped cost-lookup paths, rename the per-model attempt counter to
  `modelAttemptNumber`, and state why a context overflow continues.
- orchestrate.ts: replace step-numbered comments with sentences, make the
  rename check an early return, use sumBy, name the cost-summary flag for
  what it gates, and state the identity check and the 422 logging owner.
- config.ts: an empty fallback_model parses to null once; main.ts and the
  settings log read it as is.
- main.ts: use describeError and name what keeps the cancellation cleanup
  from throwing.

Log output changes: "review attempt failed" reports `modelAttemptNumber`
instead of `attemptNumber`, and a skipped cost lookup logs at debug.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/config.test.ts            |  8 +-
 src/__tests__/orchestrate.test.ts       |  4 +-
 src/config.ts                           |  3 +-
 src/main.ts                             | 14 ++--
 src/openrouter/__tests__/client.test.ts | 28 ++++++-
 src/openrouter/client.ts                | 97 ++++++++++++++-----------
 src/orchestrate.ts                      | 87 +++++++++++-----------
 7 files changed, 140 insertions(+), 101 deletions(-)

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..f6ce5bf 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -216,7 +216,7 @@ const baseConfig: ActionConfig = {
   githubToken: "ghp_test",
   openrouterApiKey: "sk-test",
   model: "test/model",
-  fallbackModel: "",
+  fallbackModel: null,
   requestTimeoutSeconds: 600,
   reviewTimeoutSeconds: 1500,
   maxFindings: undefined,
@@ -4185,7 +4185,7 @@ describe("staged phases", () => {
         data: {
           phase: "correctness-security",
           model: "test/model",
-          attemptNumber: 1,
+          modelAttemptNumber: 1,
           outcome: "api_error",
           errorSummary: "HTTP 500: HTTP 500",
         },
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/main.ts b/src/main.ts
index 72b5b6e..7e45dc1 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,7 +14,7 @@ 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),
   })
 })
 
@@ -35,8 +35,8 @@ 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 only registered
+  // cleanup is completeCheckRunSafely, which catches its own errors
   void (async () => {
     logger.warn("cancellation signal received — closing the check run", {
       signal: signalName,
@@ -57,8 +57,8 @@ 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.
+ *   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;
  *   parseConfig applies it for an explicitly empty one.
  */
@@ -135,7 +135,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..54117c5 100644
--- a/src/openrouter/__tests__/client.test.ts
+++ b/src/openrouter/__tests__/client.test.ts
@@ -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",
           },
@@ -1223,7 +1234,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)
@@ -1724,5 +1735,16 @@ describe("requestReview", () => {
     const result = await client.requestReview(requestParams, logger)
 
     expect(result.attempts[0]?.costUsd).toBeNull()
+    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: "no generations client",
+        },
+      },
+    ])
   })
 })
diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts
index f4c4bed..85e4038 100644
--- a/src/openrouter/client.ts
+++ b/src/openrouter/client.ts
@@ -11,7 +11,8 @@ 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 modelUsed. */
   model: string
   outcome: AttemptOutcome
   promptTokens: number | null
@@ -103,8 +104,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 +253,9 @@ const withDeadline = async <T>(
   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<BoundedResult<T>>()
 
@@ -338,7 +341,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"
@@ -408,6 +412,17 @@ export const createOpenRouterClient = ({
 }): OpenRouterClient => {
   const requestTimeoutSummary = `no response within ${Math.round(requestTimeoutMs / 1000)}s`
 
+  /** How long the next bounded call may run, and the summary a timeout gets.
+   *  The review deadline takes the blame when it is the nearer of the two. */
+  const getCallDeadline = (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 +430,14 @@ export const createOpenRouterClient = ({
     logger: Logger,
   ): Promise<SingleAttempt> => {
     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
+    const callDeadline = getCallDeadline(remainingReviewMs())
 
     const sendResult = await withDeadline(
       {
         start: (signal) => {
           return sdk.chat.send({ chatRequest }, { retries: { strategy: "none" }, signal })
         },
-        timeoutMs: Math.min(requestTimeoutMs, remainingMs),
+        timeoutMs: callDeadline.timeoutMs,
       },
       logger.child({ operation: "chat request", model }),
     )
@@ -442,12 +453,10 @@ export const createOpenRouterClient = ({
           promptTokens: null,
           completionTokens: null,
           costUsd: null,
-          errorSummary: reviewDeadlineIsBinding
-            ? "review deadline exceeded"
-            : requestTimeoutSummary,
+          errorSummary: callDeadline.timeoutSummary,
         },
         retryable: true,
-        abort: false,
+        keyRejected: false,
       }
     }
     if (sendResult.status === "rejected") {
@@ -474,14 +483,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 +509,7 @@ export const createOpenRouterClient = ({
           errorSummary: "unexpected chat response shape",
         },
         retryable: true,
-        abort: false,
+        keyRejected: false,
       }
     }
 
@@ -516,7 +525,7 @@ export const createOpenRouterClient = ({
       kind: "failed",
       attempt: { model, outcome, ...usage, errorSummary },
       retryable: true,
-      abort: false,
+      keyRejected: false,
     })
 
     const content = chatResult.choices[0]?.message.content
@@ -561,15 +570,20 @@ export const createOpenRouterClient = ({
     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 (!generations) {
+      lookupLogger.debug("generation cost lookup skipped", { reason: "no generations client" })
+      return null
+    }
+    if (remainingMs <= 0) {
+      lookupLogger.debug("generation cost lookup skipped", { reason: "review deadline exceeded" })
+      return null
+    }
+
+    const callDeadline = getCallDeadline(remainingMs)
+
     const lookup = await withDeadline(
       {
         start: (signal) => {
@@ -578,15 +592,13 @@ export const createOpenRouterClient = ({
             { retries: { strategy: "none" }, signal },
           )
         },
-        timeoutMs: Math.min(requestTimeoutMs, remainingMs),
+        timeoutMs: callDeadline.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: callDeadline.timeoutSummary })
       return null
     }
     if (lookup.status === "rejected") {
@@ -626,12 +638,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 }),
@@ -660,12 +672,12 @@ 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,
@@ -690,10 +702,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
diff --git a/src/orchestrate.ts b/src/orchestrate.ts
index 894ba71..93ee512 100644
--- a/src/orchestrate.ts
+++ b/src/orchestrate.ts
@@ -251,6 +251,7 @@ 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 }
     }
@@ -466,14 +467,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 +514,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 +549,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 +557,14 @@ 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
+  // Generated files 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,8 +592,8 @@ 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}`
@@ -611,7 +608,6 @@ const runReviewPipeline = async (
     })
   }
 
-  // Step 7: commentable lines
   const commentableByPath = computeCommentableLines(reviewableFiles)
 
   // Diff-excluded files must stay out of every context channel: the trailer
@@ -625,21 +621,22 @@ 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)
+  // 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
+
       // 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]
+      if (toPath === null || !fromPath || fromPath === "/dev/null" || fromPath === file.to) {
+        return [toPath]
+      }
+      return [toPath, fromPath]
     })
     .filter((path) => path !== null)
 
-  // Step 9: context reads
   const conventions = await contextReader.readConventions({
     conventionsFile: config.conventionsFile,
   })
@@ -740,10 +737,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 +919,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 +933,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 +968,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 +988,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 runs on every run, and a first run is the case where no bot
+  // comments exist yet. It compares against the bot's inline comments (live
+  // positions) and its beyond-diff issue comments (anchor lines), 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 +1039,20 @@ 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.
+  // 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 costSummaryMarkdown = renderCostSummary({ attempts, modelUsed })
   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(
@@ -1092,7 +1089,7 @@ const runReviewPipeline = async (
     logger.info("beyond-diff findings posted", { count: postedStandalone })
   }
 
-  // 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
@@ -1176,13 +1173,13 @@ export const orchestrate = async (
 ): Promise<OrchestrateResult> => {
   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 +1198,6 @@ export const orchestrate = async (
     costSummary: config.costSummary,
   })
 
-  // Step 2: event resolution
   const resolvedEvent = resolvePullRequestEvent(
     {
       eventName: deps.eventName,
@@ -1220,7 +1216,6 @@ export const orchestrate = async (
     }
   }
 
-  // Step 3: PR context
   const prContext: PrContext =
     resolvedEvent.kind === "complete"
       ? resolvedEvent.context
@@ -1228,7 +1223,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 +1272,7 @@ export const orchestrate = async (
           title: "Error — review did not complete",
           summary: describePipelineFailure({
             pipelineError,
-            costSummary: config.costSummary,
+            includeCostSummary: config.costSummary,
           }),
         },
       },

From 959e75f3a520391b5bdf7078bc72e58d7da69b4e Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:23:49 -0400
Subject: [PATCH 2/9] style: name the key-rejected fact once and make comments
 and log lines match the code
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- ReviewRequestError.aborted becomes keyRejected, matching SingleAttempt; run-stages reads it through isKeyRejected
- getCallDeadline becomes getCallTimeout, and its doc states the tie
- The success log's attemptCount becomes totalAttemptCount beside the failure log's modelAttemptNumber, in the client and the stage runner
- Issue-comment posting names its group for both members (beyond-diff and rerouted findings), and its log lines say issue comment
- The diff-budget, dedup, deleted-path, cancellation, and empty-input comments state what the code does
- The cancellation warning reports whether a check run was open

Ship-Check: code-quality · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts       | 19 ++++---
 src/main.ts                             | 24 +++++---
 src/openrouter/__tests__/client.test.ts | 14 ++---
 src/openrouter/client.ts                | 49 ++++++++--------
 src/orchestrate.ts                      | 75 ++++++++++++++-----------
 src/review/__tests__/run-stages.test.ts |  4 +-
 src/review/run-stages.ts                | 19 ++++---
 7 files changed, 115 insertions(+), 89 deletions(-)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index f6ce5bf..bb545e9 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -2684,11 +2684,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",
@@ -4160,7 +4163,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 +4171,7 @@ describe("staged phases", () => {
         model: "test/model",
         routedModel: "test/model",
         generationId: "completed",
-        attemptCount,
+        totalAttemptCount,
       },
     })
 
@@ -4215,7 +4218,7 @@ describe("staged phases", () => {
           throw new ReviewRequestError({
             message: "review request failed after 1 attempt(s)",
             attempts: [timeoutAttempt],
-            aborted: false,
+            keyRejected: false,
           })
         }
         return {
@@ -4361,7 +4364,7 @@ describe("staged phases", () => {
         throw new ReviewRequestError({
           message: "OpenRouter auth/credit error — aborting without fallback",
           attempts: [billedAttempt],
-          aborted: true,
+          keyRejected: true,
         })
       },
     })
@@ -4402,7 +4405,7 @@ describe("staged phases", () => {
         throw new ReviewRequestError({
           message: "review request failed after 1 attempt(s)",
           attempts: [timeoutAttempt],
-          aborted: false,
+          keyRejected: false,
         })
       },
     })
@@ -4444,7 +4447,7 @@ describe("staged phases", () => {
         throw new ReviewRequestError({
           message: "review request failed after 3 attempt(s)",
           attempts,
-          aborted: false,
+          keyRejected: false,
         })
       },
     })
diff --git a/src/main.ts b/src/main.ts
index 7e45dc1..8cdb469 100644
--- a/src/main.ts
+++ b/src/main.ts
@@ -18,8 +18,8 @@ process.on("unhandledRejection", (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<void>>()
 
@@ -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. The only registered
-  // cleanup is completeCheckRunSafely, which catches its own errors
+  // 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()
     }
@@ -59,8 +64,9 @@ process.on("SIGTERM", exitOnCancellationSignal)
  *   1.2 core-schema values (true|True|TRUE / false|False|FALSE) and throws on
  *   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;
- *   parseConfig applies it for an explicitly empty one.
+ * - 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 }),
diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts
index 54117c5..93adf1c 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(
@@ -1528,7 +1528,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",
@@ -1549,7 +1549,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) }],
     })
@@ -1560,7 +1560,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",
diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts
index 85e4038..36fd2f6 100644
--- a/src/openrouter/client.ts
+++ b/src/openrouter/client.ts
@@ -61,6 +61,8 @@ export type OpenRouterLike = {
       },
     ): Promise<unknown>
   }
+  /** The real SDK always has this. It is optional so a test stub can leave it
+   *  out, and the cost lookup then returns null. */
   generations?: {
     getGeneration(
       request: { id: string },
@@ -354,29 +356,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
   }
 }
@@ -412,9 +413,10 @@ export const createOpenRouterClient = ({
 }): OpenRouterClient => {
   const requestTimeoutSummary = `no response within ${Math.round(requestTimeoutMs / 1000)}s`
 
-  /** How long the next bounded call may run, and the summary a timeout gets.
-   *  The review deadline takes the blame when it is the nearer of the two. */
-  const getCallDeadline = (remainingMs: number): { timeoutMs: number; timeoutSummary: string } => {
+  /** 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 {
@@ -430,14 +432,17 @@ export const createOpenRouterClient = ({
     logger: Logger,
   ): Promise<SingleAttempt> => {
     const { model } = chatRequest
-    const callDeadline = getCallDeadline(remainingReviewMs())
+
+    // 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: callDeadline.timeoutMs,
+        timeoutMs: callTimeout.timeoutMs,
       },
       logger.child({ operation: "chat request", model }),
     )
@@ -453,7 +458,7 @@ export const createOpenRouterClient = ({
           promptTokens: null,
           completionTokens: null,
           costUsd: null,
-          errorSummary: callDeadline.timeoutSummary,
+          errorSummary: callTimeout.timeoutSummary,
         },
         retryable: true,
         keyRejected: false,
@@ -582,7 +587,7 @@ export const createOpenRouterClient = ({
       return null
     }
 
-    const callDeadline = getCallDeadline(remainingMs)
+    const callTimeout = getCallTimeout(remainingMs)
 
     const lookup = await withDeadline(
       {
@@ -592,13 +597,13 @@ export const createOpenRouterClient = ({
             { retries: { strategy: "none" }, signal },
           )
         },
-        timeoutMs: callDeadline.timeoutMs,
+        timeoutMs: callTimeout.timeoutMs,
       },
       lookupLogger,
     )
 
     if (lookup.status === "timed_out") {
-      lookupLogger.warn("generation cost lookup failed", { error: callDeadline.timeoutSummary })
+      lookupLogger.warn("generation cost lookup failed", { error: callTimeout.timeoutSummary })
       return null
     }
     if (lookup.status === "rejected") {
@@ -630,7 +635,7 @@ export const createOpenRouterClient = ({
       throw new ReviewRequestError({
         message: "review deadline exceeded",
         attempts,
-        aborted: false,
+        keyRejected: false,
         deadlineExceeded: true,
       })
     }
@@ -660,7 +665,7 @@ export const createOpenRouterClient = ({
             model: ladderModel,
             routedModel: attemptResult.routedModel,
             generationId: attemptResult.generationId,
-            attemptCount: attempts.length,
+            totalAttemptCount: attempts.length,
           })
           return {
             review: attemptResult.review,
@@ -681,7 +686,7 @@ export const createOpenRouterClient = ({
           throw new ReviewRequestError({
             message: `OpenRouter auth/credit error — aborting without fallback: ${summarizeAttempts(attempts)}`,
             attempts,
-            aborted: true,
+            keyRejected: true,
           })
         }
 
@@ -734,7 +739,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 93ee512..f4ec9b0 100644
--- a/src/orchestrate.ts
+++ b/src/orchestrate.ts
@@ -177,8 +177,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,
@@ -599,12 +599,14 @@ const runReviewPipeline = async (
     ? `${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})`,
     })
   }
 
@@ -623,19 +625,20 @@ const runReviewPipeline = async (
 
   // 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
-
-      // 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 (toPath === null || !fromPath || fromPath === "/dev/null" || fromPath === file.to) {
-        return [toPath]
-      }
-      return [toPath, fromPath]
-    })
-    .filter((path) => path !== null)
+  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]
+  })
 
   const conventions = await contextReader.readConventions({
     conventionsFile: config.conventionsFile,
@@ -988,10 +991,10 @@ const runReviewPipeline = async (
     duplicatesAcrossPhases,
   })
 
-  // Cross-run dedup runs on every run, and a first run is the case where no bot
-  // comments exist yet. It compares against the bot's inline comments (live
-  // positions) and its beyond-diff issue comments (anchor lines), 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 }
@@ -1045,7 +1048,6 @@ const runReviewPipeline = async (
   // - 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 costSummaryMarkdown = renderCostSummary({ attempts, modelUsed })
   const { comments, standaloneFindings: unanchoredFindings } = mapFindingsToReview({
     findings: selected,
     commentableByPath,
@@ -1066,33 +1068,36 @@ 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 in-diff findings whose anchors GitHub rejected
+  const issueCommentFindings = [...unanchoredFindings, ...inlineOutcome.rerouted]
+
+  // Each finding posts as its own issue comment. A failed post is logged and
+  // skipped without stopping the loop, and the let counts only the posts that landed
+  let postedAsIssueComments = 0
+  for (const finding of issueCommentFindings) {
     try {
       await githubClient.postIssueComment({
         prNumber: prContext.prNumber,
         body: renderStandaloneFinding(finding),
       })
-      postedStandalone += 1
+      postedAsIssueComments += 1
     } 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 > 0) {
+    logger.info("findings posted as issue comments", { count: postedAsIssueComments })
   }
 
   // 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
   const statusBody = buildStatusComment({
     sha: prContext.headSha,
     isFirstRun: !issueState.statusCommentExists,
@@ -1150,6 +1155,8 @@ const runReviewPipeline = async (
     posted: postedCount,
   })
 
+  const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed })
+
   return {
     findingsCount: postedCount,
     reviewUrl: inlineOutcome.url,
diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts
index b474302..eec66c2 100644
--- a/src/review/__tests__/run-stages.test.ts
+++ b/src/review/__tests__/run-stages.test.ts
@@ -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),
@@ -321,7 +321,7 @@ describe("runStages", () => {
         data: {
           phase: "a",
           modelUsed: "model/a",
-          attemptCount: 1,
+          totalAttemptCount: 1,
           findingsCount: 1,
         },
       },
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),

From 091c913da9c9308da8a9808d29f35f23d69dff88 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:25:09 -0400
Subject: [PATCH 3/9] docs: describe the issue-comment finding group by both of
 its members

The fetchBotIssueComments and renderStandaloneFinding docs called every issue-comment finding beyond-diff, but in-diff findings whose inline anchors GitHub rejected post the same way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/github/client.ts          | 4 ++--
 src/review/comment-mapping.ts | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

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/review/comment-mapping.ts b/src/review/comment-mapping.ts
index c3652b2..57fd13c 100644
--- a/src/review/comment-mapping.ts
+++ b/src/review/comment-mapping.ts
@@ -383,9 +383,10 @@ export const REVIEW_MARKER = "<!-- umm-actually-review -->"
 /** Identifies the single updatable status comment. */
 export const STATUS_ANCHOR = "<!-- umm-actually-status -->"
 
-/** 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
- *  its dedup anchor like any inline comment. */
+/** A finding posted as its own issue comment: a beyond-diff finding, or an
+ *  in-diff one whose inline anchor GitHub rejected. 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 => {
   return `${findingHeader(finding)}
 

From 3b70329312e781cbb113ac27c02a5c8afc63f5c9 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:28:08 -0400
Subject: [PATCH 4/9] refactor(openrouter): require the SDK's generations
 client

The real @openrouter/sdk always exposes generations, so the optional member and the no-client skip existed only for test stubs. Stubs now provide it, the skip branch and its debug line are gone, and the test that covered only that branch is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts       |  4 ++--
 src/openrouter/__tests__/client.test.ts | 23 -----------------------
 src/openrouter/client.ts                | 11 ++---------
 3 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index bb545e9..ef66b14 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -4035,7 +4035,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,
       })
@@ -4142,7 +4142,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,
diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts
index 93adf1c..54923dd 100644
--- a/src/openrouter/__tests__/client.test.ts
+++ b/src/openrouter/__tests__/client.test.ts
@@ -1202,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")
@@ -1725,26 +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()
-    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: "no generations client",
-        },
-      },
-    ])
-  })
 })
diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts
index 36fd2f6..c571af5 100644
--- a/src/openrouter/client.ts
+++ b/src/openrouter/client.ts
@@ -61,9 +61,7 @@ export type OpenRouterLike = {
       },
     ): Promise<unknown>
   }
-  /** The real SDK always has this. It is optional so a test stub can leave it
-   *  out, and the cost lookup then returns null. */
-  generations?: {
+  generations: {
     getGeneration(
       request: { id: string },
       options?: {
@@ -572,16 +570,11 @@ export const createOpenRouterClient = ({
     generationId: string,
     logger: Logger,
   ): Promise<number | null> => {
-    const generations = sdk.generations
     const remainingMs = remainingReviewMs()
 
     // Every lookup line names the generation, so a failure ties back to it
     const lookupLogger = logger.child({ operation: "generation cost lookup", generationId })
 
-    if (!generations) {
-      lookupLogger.debug("generation cost lookup skipped", { reason: "no generations client" })
-      return null
-    }
     if (remainingMs <= 0) {
       lookupLogger.debug("generation cost lookup skipped", { reason: "review deadline exceeded" })
       return null
@@ -592,7 +585,7 @@ export const createOpenRouterClient = ({
     const lookup = await withDeadline(
       {
         start: (signal) => {
-          return generations.getGeneration(
+          return sdk.generations.getGeneration(
             { id: generationId },
             { retries: { strategy: "none" }, signal },
           )

From 9a7935dab2514c019284a1f0fdc65e55a19f24e3 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:34:35 -0400
Subject: [PATCH 5/9] test: pin changed paths, the issue-comment post count,
 and the key-rejected skip
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- readChangedFiles gets its whole changedPaths list, so a dropped rename path, an added /dev/null, or a deleted file's path fails a dedicated test.
- The "findings posted as issue comments" info line is asserted with a count that leaves out a failed post, and it stays silent when no post lands.
- runStages skips later stages only when keyRejected is true. A failure carrying keyRejected: false runs them, and the skip warning names the skipped phases.

Ship-Check: test-audit · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts       | 83 +++++++++++++++++++++----
 src/review/__tests__/run-stages.test.ts | 39 ++++++++++--
 2 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index ef66b14..86e6375 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -1355,22 +1355,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 () => {
@@ -2699,6 +2701,65 @@ 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 = renderStandaloneFinding(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 } },
+      ])
     })
 
     it("uses complete PrContext from pull_request event without fetching", async () => {
diff --git a/src/review/__tests__/run-stages.test.ts b/src/review/__tests__/run-stages.test.ts
index eec66c2..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"
@@ -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([

From 0f6805589800cbbe652c562cc51d994df824437f Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:38:42 -0400
Subject: [PATCH 6/9] fix: state that a rejected inline review re-routes every
 in-diff finding
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GitHub rejects the inline review whole on a 422, so every in-diff finding in
it posts as an issue comment, not only the one with the bad anchor. The
renderStandaloneFinding doc, the rerouted field, and the issue-comment group
comment now say so. The diff-exclusion comment names every excluded file, not
only generated ones, and ModelAttempt.model points at where the routed model
is actually reported.

Ship-Check: bug-check · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/openrouter/client.ts      |  3 ++-
 src/orchestrate.ts            | 11 +++++++----
 src/review/comment-mapping.ts |  8 ++++----
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts
index c571af5..0b95185 100644
--- a/src/openrouter/client.ts
+++ b/src/openrouter/client.ts
@@ -12,7 +12,8 @@ export type AttemptOutcome =
 
 export type ModelAttempt = {
   /** The model this attempt requested. The ladder is the primary model, then
-   *  the fallback when one is set. OpenRouter's routed model is modelUsed. */
+   *  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
diff --git a/src/orchestrate.ts b/src/orchestrate.ts
index f4ec9b0..a8f91e1 100644
--- a/src/orchestrate.ts
+++ b/src/orchestrate.ts
@@ -216,7 +216,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
@@ -563,8 +564,9 @@ const runReviewPipeline = async (
     return postSkipReview({ reason: "empty diff" })
   }
 
-  // Generated files leave the review 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
@@ -1068,7 +1070,8 @@ const runReviewPipeline = async (
     logger,
   )
 
-  // Beyond-diff findings, plus in-diff findings whose anchors GitHub rejected
+  // Beyond-diff findings, plus every in-diff finding when GitHub rejected the
+  // inline review, since one bad anchor fails the whole review
   const issueCommentFindings = [...unanchoredFindings, ...inlineOutcome.rerouted]
 
   // Each finding posts as its own issue comment. A failed post is logged and
diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts
index 57fd13c..7094228 100644
--- a/src/review/comment-mapping.ts
+++ b/src/review/comment-mapping.ts
@@ -383,10 +383,10 @@ export const REVIEW_MARKER = "<!-- umm-actually-review -->"
 /** Identifies the single updatable status comment. */
 export const STATUS_ANCHOR = "<!-- umm-actually-status -->"
 
-/** A finding posted as its own issue comment: a beyond-diff finding, or an
- *  in-diff one whose inline anchor GitHub rejected. 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. */
+/** Renders a finding posted as its own issue comment. Beyond-diff findings
+ *  post this way, and so do in-diff findings when GitHub rejects their inline
+ *  review. 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 => {
   return `${findingHeader(finding)}
 

From 47ab100a6277b135901f69119d1e232aecf29c7b Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 14:54:40 -0400
Subject: [PATCH 7/9] test: pin that a disabled cost summary stays out of the
 failure summary

Every failure-path test ran with cost_summary on, so dropping describePipelineFailure's includeCostSummary guard left the suite green.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts | 37 +++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index 86e6375..e4f7516 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -4492,6 +4492,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",

From fb632df4be2d0e06959cdb650eee3c6ae34cbb08 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 15:00:42 -0400
Subject: [PATCH 8/9] fix(orchestrate): name the locations each findings post
 landed on

The inline-review and issue-comment summary lines logged only counts, so an operator could not tell which findings went where without diffing the PR against the model output. Both now carry file:line locations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts | 21 ++++++++++++++++++---
 src/orchestrate.ts                | 17 +++++++++++------
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index e4f7516..0218a99 100644
--- a/src/__tests__/orchestrate.test.ts
+++ b/src/__tests__/orchestrate.test.ts
@@ -910,9 +910,20 @@ 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) => ({
@@ -2758,7 +2769,11 @@ describe("orchestrate", () => {
         },
       ])
       expect(logsWithMessage(logger, "findings posted as issue comments")).toEqual([
-        { level: "info", message: "findings posted as issue comments", data: { count: 1 } },
+        {
+          level: "info",
+          message: "findings posted as issue comments",
+          data: { count: 1, locations: ["src/untouched.ts:420"] },
+        },
       ])
     })
 
diff --git a/src/orchestrate.ts b/src/orchestrate.ts
index a8f91e1..4e8262e 100644
--- a/src/orchestrate.ts
+++ b/src/orchestrate.ts
@@ -259,6 +259,7 @@ const postInlineFindings = async (
     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) {
@@ -1075,15 +1076,16 @@ const runReviewPipeline = async (
   const issueCommentFindings = [...unanchoredFindings, ...inlineOutcome.rerouted]
 
   // Each finding posts as its own issue comment. A failed post is logged and
-  // skipped without stopping the loop, and the let counts only the posts that landed
-  let postedAsIssueComments = 0
+  // 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 of issueCommentFindings) {
     try {
       await githubClient.postIssueComment({
         prNumber: prContext.prNumber,
         body: renderStandaloneFinding(finding),
       })
-      postedAsIssueComments += 1
+      postedAsIssueComments.push(finding)
     } catch (postError) {
       logger.warn("failed to post finding as an issue comment — it will re-report next run", {
         error: describeError(postError),
@@ -1093,14 +1095,17 @@ const runReviewPipeline = async (
     }
   }
 
-  if (postedAsIssueComments > 0) {
-    logger.info("findings posted as issue comments", { count: postedAsIssueComments })
+  if (postedAsIssueComments.length > 0) {
+    logger.info("findings posted as issue comments", {
+      count: postedAsIssueComments.length,
+      locations: postedAsIssueComments.map((finding) => `${finding.file}:${finding.line}`),
+    })
   }
 
   // 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 + postedAsIssueComments
+  const postedCount = inlineOutcome.postedCount + postedAsIssueComments.length
   const statusBody = buildStatusComment({
     sha: prContext.headSha,
     isFirstRun: !issueState.statusCommentExists,

From 47f5106e4934decaaccca94885ff97116b440f42 Mon Sep 17 00:00:00 2001
From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com>
Date: Wed, 30 Sep 2026 15:28:58 -0400
Subject: [PATCH 9/9] fix(review): label re-routed in-diff findings by where
 they sit

A finding re-posted as an issue comment after GitHub rejects the inline
review rendered the beyond-diff location note, which misstated a finding
on a changed line. Beyond-diff and re-routed findings now render through
separate functions that share one body, and the orchestrator pairs each
finding with its renderer where it builds the post list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 src/__tests__/orchestrate.test.ts            | 59 +++++++++++++++++---
 src/orchestrate.ts                           | 18 +++---
 src/review/__tests__/comment-mapping.test.ts | 37 +++++++++---
 src/review/comment-mapping.ts                | 34 +++++++++--
 4 files changed, 118 insertions(+), 30 deletions(-)

diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts
index 0218a99..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,
@@ -928,7 +929,7 @@ describe("orchestrate", () => {
       expect(stubs.postIssueCommentCalls).toEqual(
         expectedMapped.standaloneFindings.map((finding) => ({
           prNumber: fixturePrContext.prNumber,
-          body: renderStandaloneFinding(finding),
+          body: renderBeyondDiffFinding(finding),
         })),
       )
       expect(stubs.upsertSummaryCommentCalls).toEqual([
@@ -2604,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: {
@@ -2629,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([
@@ -2726,7 +2767,7 @@ describe("orchestrate", () => {
         line: 420,
         title: "Caller ignores the empty-key throw",
       })
-      const failedBody = renderStandaloneFinding(withRoutedModel(failedFinding, "test/model"))
+      const failedBody = renderBeyondDiffFinding(withRoutedModel(failedFinding, "test/model"))
       const stubs = makeOrchestrateDeps({
         fixtureResult: {
           review: { analysis: "checked", findings: [failedFinding, postedFinding] },
@@ -2902,7 +2943,7 @@ describe("orchestrate", () => {
       expect(stubs.postIssueCommentCalls).toEqual([
         {
           prNumber: 7,
-          body: renderStandaloneFinding(withRoutedModel(relatedFileFinding, "test/model")),
+          body: renderBeyondDiffFinding(withRoutedModel(relatedFileFinding, "test/model")),
         },
       ])
     })
@@ -3006,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")),
         },
       ])
     })
@@ -3037,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)
diff --git a/src/orchestrate.ts b/src/orchestrate.ts
index 4e8262e..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,
@@ -1072,19 +1073,20 @@ const runReviewPipeline = async (
   )
 
   // Beyond-diff findings, plus every in-diff finding when GitHub rejected the
-  // inline review, since one bad anchor fails the whole review
-  const issueCommentFindings = [...unanchoredFindings, ...inlineOutcome.rerouted]
+  // 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 of issueCommentFindings) {
+  for (const { finding, body } of issueCommentPosts) {
     try {
-      await githubClient.postIssueComment({
-        prNumber: prContext.prNumber,
-        body: renderStandaloneFinding(finding),
-      })
+      await githubClient.postIssueComment({ prNumber: prContext.prNumber, body })
       postedAsIssueComments.push(finding)
     } catch (postError) {
       logger.warn("failed to post finding as an issue comment — it will re-report next run", {
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("<!-- umm-actually:src/untouched.ts:subtle_bugs:30 -->")
@@ -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*
+
+<!-- umm-actually:src/greeter.ts:correctness:145 -->`)
+  })
+})
+
 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/comment-mapping.ts b/src/review/comment-mapping.ts
index 7094228..3af9c53 100644
--- a/src/review/comment-mapping.ts
+++ b/src/review/comment-mapping.ts
@@ -383,14 +383,19 @@ export const REVIEW_MARKER = "<!-- umm-actually-review -->"
 /** Identifies the single updatable status comment. */
 export const STATUS_ANCHOR = "<!-- umm-actually-status -->"
 
-/** Renders a finding posted as its own issue comment. Beyond-diff findings
- *  post this way, and so do in-diff findings when GitHub rejects their inline
- *  review. 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 => {
+/** 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. */
+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}
 
@@ -401,6 +406,23 @@ ${attributionLine(finding.modelUsed)}
 <!-- umm-actually:${computeAnchorKey(finding)} -->`
 }
 
+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,