Skip to content

review: stage the review threads deterministically (orchestrator slice 1) #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 4 commits into from
Jul 31, 2026
Merged

review: stage the review threads deterministically (orchestrator slice 1) #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecb1b6a
[jwies/review-stage-threads] review: stage the review threads determi…
jwbron Jul 31, 2026
f52f1f0
[jwies/review-stage-threads] review: close four fail-open seams the s…
jwbron Jul 31, 2026
b5168cb
[jwies/review-stage-threads] review: say where REVIEW_BOT_LOGIN goes
jwbron Jul 31, 2026
64e0082
[jwies/review-stage-threads] review: re-emit the suppression tripwire…
jwbron Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/review-stage-threads-deterministically.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"review": patch
"autofix": patch
---

`threads.json` / `human-threads.json` are staged by code, completing deterministic-orchestrator slice 1. `lib/stage-pr.ts` deliberately deferred them ("Phase 2; a later slice"), so review.md Step 3 asked the ORCHESTRATOR to fetch the unresolved threads and write both files in a particular shape, and everything downstream then depended on a model-produced file: `dispatch.ts` reads it for `hasThreads` (which gates the thread-reconciler dispatch, so it changes the roster) and for open-thread suppression, and `lib/rereview.ts` reads it for the accountability section. That seam is the failure class Khan/actions#302 patched a symptom of: the prompt selected bot threads by one spelling of the bot's login while `openThreadsFromStaged` admitted another, so a *conforming* staging produced zero usable threads and suppression silently never ran for a whole release. The worse direction was never hit but was always available: `human-threads.json` was specified as "any author other than the bot", and a bot thread misfiled there lands in `skipLines`, which makes the submission DROP a fresh finding on that line rather than merely duplicate one.

The staging step now does one GraphQL fetch of every unresolved review thread and partitions it: threads this bot opened (full reply chain, bodies byte-for-byte, `resolved: false`, the opener's `html_url`) into `threads.json`, everyone else's `{path, line}` into `human-threads.json`, so a thread is in exactly one file and neither list is assembled by hand. GraphQL rather than REST because REST exposes neither a thread's resolution state nor the `PRRT_…` node id the resolve safe output takes. Producer and consumer now share one bot-identity predicate (`lib/threads.ts`'s `isReviewBotAuthor`, which compares suffix-stripped so REST's `github-actions[bot]` and GraphQL's bare `github-actions` are one account): the two layers can no longer spell the identity differently, which is the actual #302 defect rather than its symptom. The fetch, its paging, and its fail-closed guards live once and are shared with autofix's staging, which had the only copy and whose comments carry both prior postmortems (Khan/webapp#41140's `threadCount: 0`, and GitHub answering a rate limit with HTTP 200 plus an `errors` array); autofix's `collectThreads` is now that shared fetch plus its by-opener filter, with no behavior change.

The bot identity is deployment config, not a compiled-in constant: `REVIEW_BOT_LOGIN` (default `github-actions[bot]`, matching autofix's `AUTOFIX_BOT_LOGIN` and the thumbs sweep's `REVIEW_SWEEP_BOT_LOGIN`) moves the producer and the suppression guard together, so a consumer posting reviews under its own GitHub App no longer has every one of its bot threads misfiled as human, which would put them in `skipLines` and drop fresh findings on those lines. Login comparison also case-folds before stripping the `[bot]` suffix, so a `…[BOT]` spelling cannot read as a different account.

A failed thread fetch fails the staging step rather than degrading to `[]`, because an empty staging is not the conservative direction here: it drops the flip gate's `keptBlockingCount` to zero, and a reduced-depth re-review may then flip a prior REQUEST_CHANGES to APPROVE past still-open blocking threads nobody read. The step runs before any AI spend, GraphQL's HTTP-200 rate limit is retried, and the review re-runs on the next push. That retry now lives in `lib/threads.ts` beside the fetch rather than inline in the reviewer's CLI, so autofix's port inherits it (it had none, and died on its first throttle) and a unit test can reach both directions; a page that claims a successor without a cursor is likewise refused rather than returning the partial list, matching the guard on every other failure shape here. `dedup.ts`'s fail-closed guards (bot-authored opener, explicit `resolved: false`) stay rather than trusting the new producer, as does the `stagedThreadShapeFailure` tripwire: a conforming code staging can no longer trip it, which is the point, since a fire now means either that the producer and consumer have drifted inside one repo, or that the staging came from the eval's own producer or a hand-built reproduction. The new tests feed the staged bytes straight into `openThreadsFromStaged` and `computeRoster` rather than only asserting the file's shape, since "each layer looked right on its own" is how #302 shipped. review.md Step 3 keeps exactly one thread job: reading the reply chains for an author's factual dispute, which is a judgment.

The tripwire this PR keeps is also made visible. `stagedThreadShapeFailure` reported `threadSuppressionUnavailable` on `dispatch-result.json` and printed a `::warning`, but the dispatcher runs inside the agent's Bash tool, where a workflow command is only text: measured on webapp#41204 run 30654454047, a deliberately mis-staged `threads.json` produced `unusableThreads: 9`, the line reached the run log and the step summary, and zero annotations across the run's six jobs mentioned suppression, while the pre-agent staging step's own `::warning` in that same run did annotate. Since a fail-open guard nobody can see failing is exactly how #302 survived a release, the dispatch-conformance gate now re-emits it: the gate is a `post-steps` step that runs `if: always()`, already reads every `out/` file, and its own workflow commands do annotate. The line is REBUILT from the numeric `unusableThreads` rather than forwarded as stored text, because the gate step is trusted while `out/` is a directory the agent can write, and a stored string could carry newlines that inject further commands (`::error`, `::add-mask`) into it; anything absent, non-numeric or non-positive forwards nothing. The formatter is shared with `dedup.ts` so the two cannot drift, and the forwarding lives in `lib/forwarded-warnings.ts` because `dispatch-gate.ts` sits at its 1000-line lint ceiling. Post-#308 this matters more, not less: a conforming code staging can no longer trip the tripwire, so a fire now means producer/consumer drift inside one repo.
24 changes: 15 additions & 9 deletions workflows/autofix/lib/stage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,12 @@ describe("collectThreads", () => {
expect(threads.map((t) => t.thread_id)).toEqual(["A", "B"]);
});

it("stops rather than looping when a page omits its cursor", async () => {
// A successor page that cannot be followed leaves two options, a partial
// list or a refusal, and partial is the shape this module refuses
// everywhere else: threads that never arrived would be neither fixed nor
// accounted for, while the run clears its arming label and reports clean.
// Refusing still terminates, which is all the infinite-loop guard wanted.
it("refuses rather than looping when a page omits its cursor", async () => {
const noCursor = {
data: {
repository: {
Expand All @@ -201,14 +206,15 @@ describe("collectThreads", () => {
},
},
};
const threads = await collectThreads(
portFor({threadPages: [noCursor, noCursor, noCursor]}),
"o",
"r",
1,
BOT,
);
expect(threads).toHaveLength(1);
await expect(
collectThreads(
portFor({threadPages: [noCursor, noCursor, noCursor]}),
"o",
"r",
1,
BOT,
),
).rejects.toThrow(/without an endCursor/);
});

// These four pin the fail-CLOSED direction. Staging zero threads on a PR
Expand Down
224 changes: 37 additions & 187 deletions workflows/autofix/lib/stage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@
import {buildUnifiedDiff} from "../../review/lib/stage-pr.ts";
import type {StagedThread} from "../../review/lib/rereview.ts";
import type {PriorReview} from "../../review/lib/rereview-mode.ts";
import {
assertNoGraphqlErrors,
collectUnresolvedThreads,
sameLogin,
withGraphqlRateLimitRetry,
} from "../../review/lib/threads.ts";

// Re-exported for the CLI transport below and for this module's tests: the
// guard moved to the shared module with the fetch it protects, and both callers
// still want it by this name.
export {assertNoGraphqlErrors};

/** The five files the plan CLI reads, plus the head SHA the prompt re-checks. */
export type StagedInputs = {
Expand Down Expand Up @@ -86,196 +97,32 @@ const isRecord = (v: unknown): v is Record<string, unknown> =>

const str = (v: unknown): string => (typeof v === "string" ? v : "");

/**
* Compare two GitHub logins across the REST/GraphQL bot-suffix split.
*
* REST reports an App's login as `github-actions[bot]`; GraphQL reports the
* same actor as `github-actions`. Staging reads threads over GraphQL and
* reviews over REST, so a single spelling cannot match both. Comparing on the
* suffix-stripped form does.
*
* This is not hypothetical: the first run with deterministic staging staged
* `threadCount: 0` on a PR carrying five reviewer threads, because every
* GraphQL author was `github-actions` and the configured login was
* `github-actions[bot]` (Khan/webapp#41140, run 30416237794). Unit tests could
* not have caught it; the fixtures were written in the REST spelling.
*/
const baseLogin = (login: string): string =>
login.endsWith("[bot]") ? login.slice(0, -"[bot]".length) : login;

const sameLogin = (a: string, b: string): boolean =>
baseLogin(a).toLowerCase() === baseLogin(b).toLowerCase();

/**
* Review threads with their full reply chain.
*
* `comments(first: 100)` rather than the sweep's `first: 1`: the reconciler
* contract wants the whole chain, because an author's reply is often what says
* a finding is already handled. `isResolved` is fetched so resolved threads can
* be dropped here rather than downstream.
*/
const THREADS_QUERY = `
query ($owner: String!, $repo: String!, $number: Int!, $cursor: String) {
repository(owner: $owner, name: $repo) {
pullRequest(number: $number) {
reviewThreads(first: 100, after: $cursor) {
pageInfo { hasNextPage endCursor }
nodes {
id
isResolved
path
line
comments(first: 100) {
nodes { author { login } body url }
}
}
}
}
}
}`;

/**
* Throw when a GraphQL body carries errors, mirroring the REST paths' `throw`.
*
* GraphQL does not use HTTP status to report failure. GitHub answers
* `RATE_LIMITED`, node-access failures, and partial field failures with HTTP
* **200** and an `errors` array, `data` absent or partial. A transport that
* only checks `res.ok` therefore reads a throttled response as a successful
* one, and every downstream reader sees a PR with no threads.
*
* Any `errors` entry is fatal here, including the partial-data case. Staging a
* subset of the reviewer's threads is worse than refusing: the plan would fix
* the threads that happened to arrive, clear the arming label, and report a
* clean run, leaving the rest silently unaddressed with nothing left to
* re-arm.
*/
export const assertNoGraphqlErrors = (body: unknown): void => {
if (!isRecord(body)) {
return;
}
const errors = body["errors"];
if (Array.isArray(errors) && errors.length > 0) {
throw new Error(`GraphQL errors: ${JSON.stringify(errors)}`);
}
};

const threadsConnectionOf = (
body: unknown,
): Record<string, unknown> | undefined => {
if (!isRecord(body)) {
return undefined;
}
const data = body["data"];
if (!isRecord(data)) {
return undefined;
}
const repository = data["repository"];
if (!isRecord(repository)) {
return undefined;
}
const pullRequest = repository["pullRequest"];
if (!isRecord(pullRequest)) {
return undefined;
}
const threads = pullRequest["reviewThreads"];
return isRecord(threads) ? threads : undefined;
};

/**
* Collect the bot's unresolved threads, newest page last.
*
* A thread is kept when it is unresolved and its FIRST comment is the bot's:
* that opener is the finding. A thread a human started is somebody else's
* conversation and autofix stays out of it, which is the same line the reviewer
* draws with its `human-threads.json`.
* draws with its `human-threads.json`; and, since that file became code-staged
* too, it is drawn from the very same fetch. The GraphQL query, its paging, and
* its fail-closed guards now live once in `review/lib/threads.ts`, along with
* the suffix-stripped login comparison that a REST/GraphQL split makes
* mandatory (Khan/webapp#41140 staged `threadCount: 0` on a PR carrying five
* threads because a configured `github-actions[bot]` never matched GraphQL's
* bare `github-actions`). All this function adds is the by-opener filter.
*/
export const collectThreads = async (
port: StagePort,
owner: string,
repo: string,
number: number,
botLogin: string,
): Promise<StagedThread[]> => {
const out: StagedThread[] = [];
let cursor: string | null = null;

for (;;) {
const body = await port.graphql(THREADS_QUERY, {
owner,
repo,
number,
cursor,
});
// Fail closed on both shapes a failed query can take. `errors` is the
// throttled/partial case; a missing connection is any other malformed
// body. Neither means "this PR has no threads", and reading them that
// way is the one mistake this module cannot afford: an empty
// threads.json makes the plan a no-op, the no-op populates
// `labelsToRemove`, and the run tells the author there is nothing to
// fix while the blocking findings it was armed for sit open. The label
// is gone, so nothing survives to re-arm from. The REST paths already
// throw on `!res.ok` for exactly this reason; this is that contract
// applied to the transport that does not signal failure by status.
assertNoGraphqlErrors(body);
const conn = threadsConnectionOf(body);
if (conn === undefined) {
throw new Error(
`GraphQL returned no reviewThreads connection for ` +
`${owner}/${repo}#${number}`,
);
}

const nodes = Array.isArray(conn["nodes"]) ? conn["nodes"] : [];
for (const node of nodes) {
if (!isRecord(node) || node["isResolved"] === true) {
continue;
}

const commentsConn = isRecord(node["comments"])
? node["comments"]
: {};
const rawComments = Array.isArray(commentsConn["nodes"])
? commentsConn["nodes"]
: [];
const comments = rawComments.filter(isRecord).map((c) => ({
author: isRecord(c["author"]) ? str(c["author"]["login"]) : "",
// Verbatim. The label parser reads the leading `**label:**` off
// this string; normalising it here is how a finding becomes
// unclassifiable.
body: str(c["body"]),
}));
if (
comments.length === 0 ||
!sameLogin(comments[0].author, botLogin)
) {
continue;
}

const firstUrl = isRecord(rawComments[0])
? str(rawComments[0]["url"])
: "";
out.push({
thread_id: str(node["id"]),
path: str(node["path"]),
line: typeof node["line"] === "number" ? node["line"] : null,
...(firstUrl === "" ? {} : {url: firstUrl}),
comments,
});
}

const pageInfo = isRecord(conn["pageInfo"]) ? conn["pageInfo"] : {};
if (pageInfo["hasNextPage"] !== true) {
break;
}
const next = pageInfo["endCursor"];
if (typeof next !== "string" || next === "") {
break;
}
cursor = next;
}

return out;
};
): Promise<StagedThread[]> =>
(await collectUnresolvedThreads(port.graphql, owner, repo, number)).filter(
(thread) =>
thread.comments.length > 0 &&
sameLogin(thread.comments[0].author, botLogin),
);

/** Fetch everything the plan needs. */
export const collectInputs = async (
Expand Down Expand Up @@ -399,6 +246,8 @@ if (typeof require !== "undefined" && require.main === module) {
const botLogin =
process.env.AUTOFIX_BOT_LOGIN?.trim() || "github-actions[bot]";
const api = process.env.GITHUB_API_URL?.trim() || "https://api.github.com";
const sleep = (ms: number): Promise<void> =>
new Promise((resolve) => setTimeout(resolve, ms));

const headers = {
authorization: `Bearer ${token}`,
Expand Down Expand Up @@ -451,7 +300,15 @@ if (typeof require !== "undefined" && require.main === module) {
}
return out;
},
graphql: async (query, variables) => {
// `withGraphqlRateLimitRetry` supplies the transport-level
// `assertNoGraphqlErrors` (duplicated in `collectThreads`
// deliberately: the guard belongs both at the transport, which any
// future GraphQL caller inherits, and at the reader, which is the one
// the unit tests can reach) and retries the one error that heals.
// GitHub answers a throttle with HTTP 200, so `res.ok` cannot see it,
// and without the retry the first throttle ends the run having cleared
// the arming label on a PR with open findings.
graphql: withGraphqlRateLimitRetry(async (query, variables) => {
const res = await fetch(`${api}/graphql`, {
method: "POST",
headers: {...headers, "content-type": "application/json"},
Expand All @@ -460,15 +317,8 @@ if (typeof require !== "undefined" && require.main === module) {
if (!res.ok) {
throw new Error(`GraphQL failed: ${res.status}`);
}
// Duplicated in `collectThreads`, deliberately. The guard is cheap
// and its absence clears the arming label on a PR with open
// findings, so it belongs both at the transport (any future
// GraphQL caller inherits it) and at the reader (which is the one
// the unit tests can reach).
const body = await res.json();
assertNoGraphqlErrors(body);
return body;
},
return res.json();
}, sleep),
};

collectInputs(port, owner, repo, number, botLogin)
Expand Down
Loading
Loading