From d8a0f60875c83087dd9f87109161f5420d182819 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 9 Nov 2026 22:24:04 -0800 Subject: [PATCH 1/4] chore(quest): claim quest/m0/token-waste/review-carry.md Quest-Claim: cd682a2aa810aad3 From 4a012bd5918c17ad772190e7b57e8ab5386a5fd9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 10 Nov 2026 08:31:59 -0800 Subject: [PATCH 2/4] feat(status): report whether the ## Review record covers the head quest status prints a review-record line: the last full SHA the PR body's ## Review section names, and whether it is current, diff-identical (the base-to-head diff matches), stale, or unknown. /quest-merge, /quest-review, and /quest-complete read it instead of comparing diffs. Co-Authored-By: Claude Opus 5.5 --- README.md | 12 +++--- assets/skills/complete.md | 2 +- assets/skills/merge.md | 3 +- assets/skills/review.md | 2 +- quest/m0/token-waste/review-carry.md | 24 ------------ src/cli/github.ts | 4 +- src/cli/main.ts | 6 ++- src/cli/review-record.ts | 6 +-- src/cli/status.ts | 55 +++++++++++++++++++++++++++- src/core/review.ts | 2 +- test/status.test.ts | 47 +++++++++++++++++++++++- 11 files changed, 122 insertions(+), 41 deletions(-) delete mode 100644 quest/m0/token-waste/review-carry.md diff --git a/README.md b/README.md index 1ef9d14c..13bd44a6 100644 --- a/README.md +++ b/README.md @@ -455,6 +455,7 @@ head 0ade9d977d54c6f48152410532c58e9ea78db3d0 mergeable conflicting merge-state dirty review-decision - +review-record diff-identical 5f1c2e9a8b7d6c5e4f3a2b1c0d9e8f7a6b5c4d3e auto-merge - merge-queue - landing-stopped - @@ -472,6 +473,7 @@ comes last, and a missing value prints as `-`: | --- | --- | | `url`, `state`, `draft`, `head` | The PR URL; `open`, `closed`, or `merged`; `true` or `false`; the head SHA. | | `mergeable`, `merge-state`, `review-decision` | GitHub's values, lowercased. | +| `review-record` | Whether the PR body's [`## Review` record](#review-locally) still covers the head, and the head it names last: `current` when it is the head, `diff-identical` when `git diff ...` prints the same as for it, `stale` otherwise, or `unknown` when GitHub cannot compare them, such as a diff too large; `-` when the record names no full SHA. Whose record counts is the reader's call. | | `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | | `landing-stopped` | The reason the latest merge-queue entry, auto-merge, or [`quest land`](#land-a-batch-with-quest-land) queue entry ended (such as `merge_conflict`, `failed_checks`, `manual`, `manually_disabled`, or `merged`), who ended it, and when; `-` once the PR has entered the merge queue or auto-merge again since. | @@ -1385,18 +1387,18 @@ from the branch into the base, found among the upstream's PRs from the repository the base's remote pushes to, and prints `record `. The record names the reviewer harness, model, and effort, the session and round, the reviewed head, the base's short SHA, the score and rationale, each finding with -its location, and the last round's findings with their dispositions. The land -gate counts every full SHA the section names as a review of that head, so the +its location, and the last round's findings with their dispositions. `quest +status` takes the last full SHA the section names as the reviewed head, so the head appears in full only once a round reports no findings, and model text appears as inline code, which mentions or links nothing and never holds a full SHA. The section runs to the next level-1 or level-2 heading, both as Markdown parses them, so an example in a code fence is -text to the land gate and the record alike; the rest of the body stays as written, and a body without one +text to `quest status` and the record alike; the rest of the body stays as written, and a body without one gains it at the end, unless it ends inside an unclosed code fence or HTML comment, which fails the record. With no such PR, or a remote off GitHub, it changes nothing and prints `record skipped: `. It edits as the user the GitHub token names -(`GH_TOKEN`, `GITHUB_TOKEN`, or `gh auth token`), and the land gate counts the -record only while the PR's author made the body's latest edit, so review your +(`GH_TOKEN`, `GITHUB_TOKEN`, or `gh auth token`), and `/quest-merge` counts the +record only on a PR whose author has write access, so review your own PRs with your own token. A record GitHub refuses fails the command after the round's report, with exit status 1. It exits 0 once the round finishes, whatever it found, 2 for invalid arguments or a checkout or CLI that diff --git a/assets/skills/complete.md b/assets/skills/complete.md index 53daf118..c61c02c6 100644 --- a/assets/skills/complete.md +++ b/assets/skills/complete.md @@ -13,7 +13,7 @@ The skill argument can be used to filter the PRs in scope. Run each `/quest-merge` inline in this session; it queues a landable head, or hands the PR to `/quest-iterate`. Run each `/quest-review` and `/quest-iterate` in parallel in a sub-agent. -`/quest-iterate` is the default for a PR that needs work: dispatch it without asking when the PR has no decision for the user, such as for a merge conflict, failing checks, unaddressed review findings, or no counted review of the head. +`/quest-iterate` is the default for a PR that needs work: dispatch it without asking when the PR has no decision for the user, such as for a merge conflict, failing checks, unaddressed review findings, or no counted review of the head, which a `review-record` line of `current` or `diff-identical` in `quest status ` already is. It fixes the PR, reviews it, and marks it ready; ask about the PR again when it reports. A PR with open decisions, such as those its `## Waiting` section lists or a significant issue you would solve differently, goes to the user instead: settle its decisions with the grill skill's rounds, then dispatch `/quest-iterate` with the answers in its brief. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index 82ef5951..4cd94222 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -27,7 +27,8 @@ A PR that adds follow-ups also needs the answers of the round that picked it rec While its required checks are only pending, report that the pick stands until they pass; do not wait for them. A review counts for the head whose SHA it names: a `## Review` record of a local `quest review` round, or a recorded skip. -Changes since that head need no new review only when they are conflict-free merges from the base branch, a reviewer's suggested patch applied unchanged, the fixes of a round whose findings were all low severity, a land fixer's recorded mechanical fix that changes no behavior, the pick round's answers recorded in the PR's follow-ups, or changes, such as a hand-resolved merge, that leave the head's diff identical to the reviewed head's: `git diff /...` prints the same as `git diff /...`. +`quest status `'s `review-record` line names that head; `current` or `diff-identical` (the head's diff from the base is unchanged since it) counts, so read it rather than comparing diffs yourself. +Otherwise, changes since that head need no new review only when they are conflict-free merges from the base branch, a reviewer's suggested patch applied unchanged, the fixes of a round whose findings were all low severity, a land fixer's recorded mechanical fix that changes no behavior, or the pick round's answers recorded in the PR's follow-ups. The repository's review policy decides whose record counts and may require more, such as a maintainer's review. Where it says nothing, only a record on a PR whose author has write access counts: anyone else writes their own PR body, so review their head locally and rewrite the record before landing it. diff --git a/assets/skills/review.md b/assets/skills/review.md index 6473a48b..dea81905 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -18,7 +18,7 @@ On the head of a PR whose author lacks write access to the repository, run no co A check failure that reproduces unchanged in a checkout of the base branch is pre-existing rather than the PR's: record the command and both failures in the PR body's validation, go on to review and readiness, and add a follow-up for it unless an open PR already fixes it. Never alter the repository's harness to get a pass, such as by disabling a tool, unsetting its environment, or skipping a check, nor report an altered run as its checks. -If a `## Review` record that `/quest-merge` counts names the committed head, or its review exemptions cover the changes since the head it names, and it leaves no finding unrefuted, run no new round and go on to pushing and marking it ready below. +If a `## Review` record that `/quest-merge` counts names the committed head, `quest status ` reports its `review-record` `diff-identical` for the pushed head, or its other review exemptions cover the changes since the head it names, and it leaves no finding unrefuted, run no new round and go on to pushing and marking it ready below. If you judge the change too trivial to review, skip the review instead and record the skip and its reason in `## Review`. Otherwise, review the committed branch with `quest review --base /`, which picks a reviewer other than the model that changed it last, and its effort, by the PR's size; `quest review --help` explains the round. diff --git a/quest/m0/token-waste/review-carry.md b/quest/m0/token-waste/review-carry.md deleted file mode 100644 index b1b6d5e4..00000000 --- a/quest/m0/token-waste/review-carry.md +++ /dev/null @@ -1,24 +0,0 @@ -# [S] A review record carries across a diff-identical head - -## Goal - -`quest merge --land ` counts a `## Review` record that names an -earlier head when the branch's own diff is unchanged since it: -`git diff /...` prints the same as for the reviewed -head. A conflict-free merge of the base then lands without an agent -rewriting the record. - -## Plan - -Found 2026-11-09 by a workflow audit: over four days, 10 of 107 coordinator -follow-ups sent an agent back to a PR only because "the `## Review` record -still names" the head before a base merge, "so the new head can't be -queued". `/quest-merge` already exempts changes that leave the head's diff -identical (`assets/skills/merge.md`), but the command counts a record only -for the exact SHA it names, so the exemption costs a session each time. - -- Carry only this exemption, which a program can decide. The others, such - as a reviewer's suggested patch applied unchanged, stay the skill's - judgment. -- `/quest-merge` and `/quest-review` drop the steps that re-record a review - for such a head. diff --git a/src/cli/github.ts b/src/cli/github.ts index 32e39a36..81224465 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -65,6 +65,8 @@ export interface RequestOptions { body?: unknown; method?: "POST" | "PUT" | "PATCH" | "DELETE"; allow?: number[]; + /** A media type other than JSON, such as `application/vnd.github.diff`. */ + accept?: string; /** Overrides how the request repeats; the default follows its method and endpoint. */ repeat?: Repeat; } @@ -253,7 +255,7 @@ export class GitHub { ? options.method && { method: options.method } : { method: options.method ?? "POST", body: JSON.stringify(options.body) }), headers: { - Accept: "application/vnd.github+json", + Accept: options.accept ?? "application/vnd.github+json", Authorization: `Bearer ${this.deps.token}`, "X-GitHub-Api-Version": "2022-11-28", "User-Agent": "quest", diff --git a/src/cli/main.ts b/src/cli/main.ts index e1ca2355..6d37c006 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -532,8 +532,10 @@ export function main(argv: string[], out: Output): number | Promise { "Print a pull request's head, checks, merge state, reviews, reactions, and unresolved review threads " + "in one call. It reports facts; it does not decide whether the PR is ready or whose review counts.\n\n" + "Plain output prints one record per line, its type first and any free text (a check name or path) last: " + - "url, state, draft, head, mergeable, merge-state, review-decision, auto-merge, merge-queue, " + - "landing-stopped, and checks, whose state and counts follow each check's latest run; auto-merge gives its method, who enabled it and when, and the head it was " + + "url, state, draft, head, mergeable, merge-state, review-decision, review-record, auto-merge, merge-queue, " + + "landing-stopped, and checks, whose state and counts follow each check's latest run; review-record gives whether the PR body's ## Review record " + + "covers the head, as current, diff-identical (git diff ... prints the same as for the head it names), " + + "stale, or unknown when GitHub cannot compare them, then the head it names last; auto-merge gives its method, who enabled it and when, and the head it was " + "enabled on, which differs from head once a push follows, as GitHub keeps auto-merge across a writer's " + "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + "activity, which GitHub lists up to minutes late); landing-stopped is the latest exit from the merge queue, auto-merge, or " + diff --git a/src/cli/review-record.ts b/src/cli/review-record.ts index 88c9ae63..6a99060d 100644 --- a/src/cli/review-record.ts +++ b/src/cli/review-record.ts @@ -11,7 +11,7 @@ const HARNESSES: Record = { claude: "Claude Code", co /** * Model text as one inline code span, so a mention, issue reference, or Markdown in it renders as text. - * The land gate counts every full SHA the section names, so no full SHA appears in it. + * `quest status` takes the last full SHA the section names as its reviewed head, so no full SHA appears in it. */ function code(text: string): string { const flat = text @@ -32,7 +32,7 @@ export function reviewRecord(round: Round): string { "", `- Reviewer: ${HARNESSES[round.reviewer]}, ${round.model}, ${round.effort} effort`, `- Session: ${code(round.session)}, round ${round.round}`, - // The land gate counts a full SHA here as a review of that head, which an open finding has not passed. + // `quest status` counts a full SHA here as a review of that head, which an open finding has not passed. `- Head: ${round.findings.length === 0 ? round.head : `${round.head.slice(0, 12)}, named in full once a round reports no findings`}`, `- Base: ${round.base.ref} at ${round.base.sha.slice(0, 12)}`, `- Score: ${round.score}/10, ${code(round.rationale)}`, @@ -159,7 +159,7 @@ export async function pullModels( return recorded(pull.body); } -/** Record `round` in the `## Review` section of its branch's open PR. The edit is the token's user's, which the land gate counts only when that user wrote the PR. */ +/** Record `round` in the `## Review` section of its branch's open PR. The edit is the token's user's, which `/quest-merge` counts only when that user wrote the PR. */ export async function recordRound(root: string, round: Round, deps: ClaimDependencies): Promise { const pull = await openPull(root, round.base.ref, round.branch, deps); if ("skipped" in pull) return pull; diff --git a/src/cli/status.ts b/src/cli/status.ts index 2b85bc54..e74ae5e2 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -13,6 +13,7 @@ import { } from "./github"; import { checksState, contextOutcome, latestContexts } from "../github/checks"; import { EJECTED, pinned } from "./land"; +import { reviewSection } from "../core"; import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; /** @@ -24,7 +25,7 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, $moreChecks: Boolean!, $moreReviews: Boolean!, $moreThreads: Boolean!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - url state isDraft headRefOid headRefName mergeable mergeStateStatus reviewDecision + url body baseRefName state isDraft headRefOid headRefName mergeable mergeStateStatus reviewDecision headRepository { nameWithOwner } autoMergeRequest { enabledAt mergeMethod commitBody enabledBy { login } } mergeQueueEntry { state position enqueuedAt } @@ -102,6 +103,17 @@ export interface Thread { url: string; } +/** + * The head the PR body's `## Review` section names last, the latest reviewed one: a round's record, or a line after + * it for low-severity fixes or a skip. `current` is the PR's head; `diff-identical` an earlier head whose diff from + * the base (`git diff ...`) matches the head's; `unknown` one GitHub cannot compare, such as a diff too + * large to render. + */ +export interface ReviewRecord { + state: "current" | "diff-identical" | "stale" | "unknown"; + head: string; +} + export interface Status { url: string; state: "open" | "closed" | "merged"; @@ -121,6 +133,8 @@ export interface Status { checks: { state: string | null; passed: number; failing: Check[]; pending: Check[] }; /** Each reviewer's latest submitted review, oldest first. */ reviews: Review[]; + /** Null when the `## Review` section names no full SHA. */ + reviewRecord: ReviewRecord | null; reactions: Reaction[]; /** Unresolved review threads, oldest first. */ threads: Thread[]; @@ -184,10 +198,45 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie if (ownership.head !== first!.headRefOid) throw new Error("the PR head changed while reading its holds; rerun quest status"); const status = summarize(first!, rollup, contexts, reviewNodes, threadNodes); + status.reviewRecord = await reviewRecord(github, pr, first!, deps.warn); if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!, deps.warn); return { ...status, holds: ownership.holds, starterHold: ownership.starter }; } +async function reviewRecord( + github: GitHub, + pr: PullRequest, + pull: ObjectValue, + warn: (message: string) => void, +): Promise { + const body = typeof pull.body === "string" ? pull.body : ""; + const section = reviewSection(body); + // Only the last SHA, so a body naming many costs no more requests than one. + const reviewed = + section && + body + .slice(section.start, section.end) + .match(/\b[0-9a-f]{40}\b/g) + ?.at(-1); + if (!reviewed) return null; + const head = string(pull.headRefOid); + if (reviewed === head) return { state: "current", head: reviewed }; + const base = encodeURIComponent(string(pull.baseRefName)); + const diffs: string[] = []; + for (const sha of [head, reviewed]) { + const response = await github.request(`/repos/${pr.owner}/${pr.repo}/compare/${base}...${sha}`, { + accept: "application/vnd.github.diff", + allow: [404, 406, 422], + }); + if (!response.ok) { + warn(`cannot compare ${sha} with the base, so whether the review of ${reviewed} carries is unknown`); + return { state: "unknown", head: reviewed }; + } + diffs.push(await response.text()); + } + return { state: diffs[0] === diffs[1] ? "diff-identical" : "stale", head: reviewed }; +} + /** Repository activity types that move a branch to a new head, including merges into it. */ export const BRANCH_UPDATES = ["push", "force_push", "branch_creation", "pr_merge", "merge_queue_merge"]; @@ -363,6 +412,7 @@ function summarize( }, checks, reviews: [...latest.values()], + reviewRecord: null, reactions, threads, holds: [], @@ -413,6 +463,9 @@ export function statusLines(status: Status): string[] { `mergeable ${status.mergeable}`, `merge-state ${status.mergeState}`, `review-decision ${dash(status.reviewDecision)}`, + status.reviewRecord === null + ? "review-record -" + : `review-record ${status.reviewRecord.state} ${status.reviewRecord.head}`, status.autoMerge === null ? "auto-merge -" : `auto-merge ${status.autoMerge.method} ${status.autoMerge.enabledBy} ${status.autoMerge.enabledAt} ${dash(status.autoMerge.head)}`, diff --git a/src/core/review.ts b/src/core/review.ts index 44475f15..e87a0497 100644 --- a/src/core/review.ts +++ b/src/core/review.ts @@ -1,4 +1,4 @@ -// A PR body's `## Review` record, which `quest review` writes and the land gate reads. +// A PR body's `## Review` record, which `quest review` writes and `quest status` reads. import { markdown } from "./doc"; diff --git a/test/status.test.ts b/test/status.test.ts index 7630cb3f..132d11a7 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -112,6 +112,8 @@ function setup( /** Pages of the head repository's activity, newest first. */ activity?: { rows: Node[]; next?: string; status?: number }[]; warnings?: string[]; + /** The base-to-head diff per compared head, or the status GitHub refuses it with. */ + diffs?: Record; } = {}, ) { const queries: Record[] = []; @@ -137,6 +139,15 @@ function setup( return Response.json({ message: "Resource not accessible by integration" }, { status: 403 }); return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); } + const compare = /^\/repos\/example\/project\/compare\/main\.\.\.([0-9a-f]{40})$/.exec(path.pathname); + if (compare) { + expect(new Headers(init?.headers).get("Accept")).toBe("application/vnd.github.diff"); + const diff = options.diffs?.[compare[1]!]; + if (diff === undefined) throw new Error("unexpected compare request"); + return typeof diff === "number" + ? Response.json({ message: "No diff" }, { status: diff }) + : new Response(diff); + } if (path.pathname !== "/graphql") { expect(path.pathname).toBe("/repos/example/project/issues/1/events"); eventRequests.push(String(url)); @@ -216,6 +227,7 @@ test("reports checks by requiredness, each reviewer's latest review against the "mergeable mergeable", "merge-state blocked", "review-decision review_required", + "review-record -", "auto-merge -", "merge-queue -", "landing-stopped -", @@ -252,12 +264,13 @@ test("a queued PR with auto-merge and no checks reports both, and JSON is determ threads: page([]), }); const status = await pullRequestStatus(pr, setup([body]).deps); - expect(statusLines(status).slice(2, 11)).toEqual([ + expect(statusLines(status).slice(2, 12)).toEqual([ "draft true", `head ${A}`, "mergeable mergeable", "merge-state blocked", "review-decision -", + "review-record -", "auto-merge squash kixelated 2026-10-07T12:00:00Z -", "merge-queue awaiting_checks 2 2026-10-07T12:01:00Z", "landing-stopped -", @@ -644,3 +657,35 @@ test("refuses a head moved between the paged state and ownership read", async () }); await expect(pullRequestStatus(pr, fixture.deps)).rejects.toThrow("head changed while reading its holds"); }); + +describe("the review record", () => { + const C = "c".repeat(40); + const line = async (body: string, options: Parameters[1] = {}) => { + const fixture = setup([pull({ body, checks: null, reviews: page([]), threads: page([]) })], options); + return statusLines(await pullRequestStatus(pr, fixture.deps)).find((l) => l.startsWith("review-record ")); + }; + + test("is current when the section's last full SHA is the head, without comparing", async () => { + expect(await line(`## Review\n\n- Head: ${B}\n- Low findings fixed at ${A}`)).toBe( + `review-record current ${A}`, + ); + }); + + test("carries across a head whose diff from the base matches the reviewed head's, and not otherwise", async () => { + const body = `## Review\n\n- Head: ${B}`; + expect(await line(body, { diffs: { [A]: "same", [B]: "same" } })).toBe(`review-record diff-identical ${B}`); + expect(await line(body, { diffs: { [A]: "new", [B]: "old" } })).toBe(`review-record stale ${B}`); + }); + + test("is unknown, with a warning, when GitHub cannot compare a head", async () => { + const warnings: string[] = []; + expect(await line(`## Review\n\n- Head: ${B}`, { diffs: { [A]: 406 }, warnings })).toBe( + `review-record unknown ${B}`, + ); + expect(warnings).toHaveLength(1); + }); + + test("reads only the Review section, so a SHA elsewhere names no review", async () => { + expect(await line(`Fixes ${A}.\n\n## Review\n\nSkipped.\n\n## Models\n\n${C}`)).toBe("review-record -"); + }); +}); From a02715f8c063ea526403dbb90260c994e03b2120 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 10 Nov 2026 08:33:57 -0800 Subject: [PATCH 3/4] fix(review): carry a diff-identical record only for the pushed, counted head Co-Authored-By: Claude Opus 5.5 --- assets/skills/review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/skills/review.md b/assets/skills/review.md index dea81905..9ebf7ec0 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -18,7 +18,7 @@ On the head of a PR whose author lacks write access to the repository, run no co A check failure that reproduces unchanged in a checkout of the base branch is pre-existing rather than the PR's: record the command and both failures in the PR body's validation, go on to review and readiness, and add a follow-up for it unless an open PR already fixes it. Never alter the repository's harness to get a pass, such as by disabling a tool, unsetting its environment, or skipping a check, nor report an altered run as its checks. -If a `## Review` record that `/quest-merge` counts names the committed head, `quest status ` reports its `review-record` `diff-identical` for the pushed head, or its other review exemptions cover the changes since the head it names, and it leaves no finding unrefuted, run no new round and go on to pushing and marking it ready below. +If a `## Review` record that `/quest-merge` counts names the committed head, or `quest status ` reports it `diff-identical` while its `head` line is the committed head, or its other review exemptions cover the changes since the head it names, and it leaves no finding unrefuted, run no new round and go on to pushing and marking it ready below. If you judge the change too trivial to review, skip the review instead and record the skip and its reason in `## Review`. Otherwise, review the committed branch with `quest review --base /`, which picks a reviewer other than the model that changed it last, and its effort, by the PR's size; `quest review --help` explains the round. From 232a415c760cc31fe02fc1982964bda0f7471b35 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 10 Nov 2026 09:53:50 -0800 Subject: [PATCH 4/4] fix(status): report an unknown review record when a compare request fails Co-Authored-By: Claude Opus 5.5 --- src/cli/status.ts | 20 ++++++++++++++------ test/status.test.ts | 12 +++++++----- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/cli/status.ts b/src/cli/status.ts index e74ae5e2..d4c84dd0 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -224,14 +224,22 @@ async function reviewRecord( const base = encodeURIComponent(string(pull.baseRefName)); const diffs: string[] = []; for (const sha of [head, reviewed]) { - const response = await github.request(`/repos/${pr.owner}/${pr.repo}/compare/${base}...${sha}`, { - accept: "application/vnd.github.diff", - allow: [404, 406, 422], - }); - if (!response.ok) { - warn(`cannot compare ${sha} with the base, so whether the review of ${reviewed} carries is unknown`); + const unknown = (why: string): ReviewRecord => { + warn(`cannot compare ${sha} with the base, so whether the review of ${reviewed} carries is unknown${why}`); return { state: "unknown", head: reviewed }; + }; + let response: Response; + try { + response = await github.request(`/repos/${pr.owner}/${pr.repo}/compare/${base}...${sha}`, { + accept: "application/vnd.github.diff", + allow: [404, 406, 422], + }); + } catch (error) { + // A deadline is the caller's to handle; a forbidden or failing compare is not. + if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; + return unknown(`: ${error.message}`); } + if (!response.ok) return unknown(""); diffs.push(await response.text()); } return { state: diffs[0] === diffs[1] ? "diff-identical" : "stale", head: reviewed }; diff --git a/test/status.test.ts b/test/status.test.ts index 132d11a7..ba7a8f83 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -678,11 +678,13 @@ describe("the review record", () => { }); test("is unknown, with a warning, when GitHub cannot compare a head", async () => { - const warnings: string[] = []; - expect(await line(`## Review\n\n- Head: ${B}`, { diffs: { [A]: 406 }, warnings })).toBe( - `review-record unknown ${B}`, - ); - expect(warnings).toHaveLength(1); + for (const status of [406, 403]) { + const warnings: string[] = []; + expect(await line(`## Review\n\n- Head: ${B}`, { diffs: { [A]: status }, warnings })).toBe( + `review-record unknown ${B}`, + ); + expect(warnings).toHaveLength(1); + } }); test("reads only the Review section, so a SHA elsewhere names no review", async () => {