diff --git a/.changeset/review-adjudicated-thread-suppression.md b/.changeset/review-adjudicated-thread-suppression.md new file mode 100644 index 00000000..ae0d23eb --- /dev/null +++ b/.changeset/review-adjudicated-thread-suppression.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +Resolving a bot thread now means "settled", not "open season for a rephrase". The staging collects the bot's threads a HUMAN resolved into a new adjudicated corpus (`adjudicated-threads.json`), and the dispatcher suppresses any non-blocking candidate that re-derives a defect that corpus already settled (same defect-identity match as open-thread suppression). Previously, resolution removed the thread from the only suppression corpus, so the next run could re-post the same concern with fresh wording as a brand-new thread, which every later accountability recap then reported as "still unaddressed" (webapp#41290: six resolved variants of one concern, then a seventh). Two safety asymmetries: a thread the BOT resolved (the reconciler, after a fix) never joins the corpus, and a BLOCKING candidate is never suppressed by it, so a fixed-then-regressed defect worth stopping the PR for always posts. diff --git a/.changeset/review-downvote-adjudication.md b/.changeset/review-downvote-adjudication.md new file mode 100644 index 00000000..ad7014ce --- /dev/null +++ b/.changeset/review-downvote-adjudication.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +A 👎 on a bot finding now adjudicates it, exactly like resolving its thread: the staging reads each thread opener's THUMBS_DOWN reaction count, and a bot-opened thread with a downvoted opener joins the adjudicated suppression corpus whatever its resolution state, so the settled defect cannot re-post under fresh wording (blocking re-flags still always post, and a still-open downvoted thread stays in the open corpus, which keeps the verdict-floor bookkeeping). Previously the downvote channel the bot advertises (the thumbs sweep asks "why?" on exactly this signal) dead-ended in counters and changed nothing about what posts. Also documents the full feedback signal contract (reply / resolve / 👎 / hide) in the README. diff --git a/workflows/review/README.md b/workflows/review/README.md index b650557a..84c0c438 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -104,7 +104,11 @@ CLI invocation (`lib/dispatch.ts`) that runs triage, the reviewer fan-out (roster, budget cap, and planned sheds computed from `routing.json`), the provenance gate, the scope filter, cross-source dedup, open-thread suppression (a candidate describing a defect an open bot thread already tracks posts no -duplicate; a suppressed blocking candidate still floors the verdict), and +duplicate; a suppressed blocking candidate still floors the verdict), +adjudicated-thread suppression (a non-blocking candidate re-deriving a defect +a human settled by resolving the bot's thread or downvoting its opener posts +no new thread; a blocking candidate is never suppressed this way, so a +regression re-flag stays visible), and claim validation, inside the same firewall sandbox (the api-proxy meters and caps script-spawned sub-agents exactly like Task-spawned ones). Each sub-agent delivers its result through an in-process `submit_result` MCP tool whose input @@ -125,6 +129,42 @@ run-local JSONL append that needs no credentials, but the agent sandbox mounts write it. Removing the seam wants a writable path into the queue (an upstream mount change, or a post-agent step on the host); neither is tested yet. +## What your feedback does + +The reviewer reads four signals off its own threads. What each one means, so +you can pick the one that says what you mean: + +- **Reply to the thread.** Read in full: the reconciler sees every reply + chain verbatim, and the orchestrator surfaces replies that factually + dispute a finding to claim validation. But a reply alone does not close + anything: the reconciler resolves a thread when the CODE changes to address + it, so arguing a finding down in prose and pushing nothing leaves it open + (and blocking, if it was blocking). +- **Resolve the thread.** "This is settled." The thread leaves the + accountability recap, and the defect joins the adjudicated corpus: a later + run that re-derives the same defect (any wording, any nearby line) posts + nothing, unless it comes back at BLOCKING severity, which always posts (a + regression worth stopping the PR for must never be silenced by an old + resolution). Threads the bot resolved itself (because a push fixed them) + do not join the corpus; a fixed defect that reappears is a fresh finding. +- **👎 the finding's comment.** Same adjudication as resolving, through the + reaction channel: a 👎 on a thread's OPENING comment puts its defect in the + adjudicated corpus whether or not you also resolve. The feedback sweep may + additionally ask one follow-up ("why?"), which calibrates the eval suite; + answering it is welcome but the 👎 alone is what suppresses. Reactions on + replies are conversation, not adjudication. 👎 is the ONLY adjudicating + reaction: a 😕 triggers the sweep's follow-up question like a 👎 does, but + it does not suppress (😕 reads as "unclear", not "wrong", and ambiguity is + worth a question, not a standing suppression). The bot's own seeded nudge + reactions never count as adjudication either. +- **Hide the comment.** Reads as nothing. The reviewer does not see hidden + state; resolve or 👎 instead. + +Per-PR opt-out and re-runs are consumer-trigger concerns: repos using the +stock push trigger skip any PR carrying the `skip-ai-review` label, and +consumers with comment triggers or shims should honor the same label (see +Khan/webapp's kore shim). + ## Install ```sh diff --git a/workflows/review/lib/dedup-adjudicated.test.ts b/workflows/review/lib/dedup-adjudicated.test.ts new file mode 100644 index 00000000..8ecbcbf6 --- /dev/null +++ b/workflows/review/lib/dedup-adjudicated.test.ts @@ -0,0 +1,240 @@ +import {describe, it, expect} from "vitest"; + +import { + adjudicatedThreadsFromStaged, + suppressAdjudicatedDuplicates, + suppressTrackedDuplicates, +} from "./dedup-adjudicated"; +import type {Claim} from "./dispatch-contracts"; + +/** + * Adjudicated-thread suppression tests, split from dedup.test.ts for its + * max-lines budget (the dedup-cluster.test.ts precedent); the `claim` factory + * mirrors that file's. The scenario throughout is webapp#41290's: a human + * resolved the bot's thread, and a later run re-derived the same defect with + * fresh wording at a nearby line, which the adjudicated corpus must absorb + * without ever absorbing a blocking regression re-flag. + */ + +const claim = (over: Partial & {id: string; source: string}): Claim => ({ + path: "services/ai-guide/memory/expiration.go", + line: 38, + label: "issue (blocking)", + subject: "s", + discussion: "d", + failure_scenario: "f", + confidence: 0.7, + ...over, +}); + +describe("adjudicatedThreadsFromStaged", () => { + const adjudicated = (over: Record = {}) => ({ + thread_id: "T1", + path: "a.ts", + resolved: true, + resolvedBy: "sxkosone", + comments: [ + { + author: "github-actions", + body: "**suggestion (non-blocking):** opener", + }, + ], + ...over, + }); + + it("admits only bot-opened threads a human resolved", () => { + expect(adjudicatedThreadsFromStaged([adjudicated()])).toEqual([ + { + thread_id: "T1", + path: "a.ts", + body: "**suggestion (non-blocking):** opener", + }, + ]); + }); + + it("admits a bot thread whose opener a reviewer downvoted, whatever its resolution state", () => { + // The 👎 channel: the same judgment as resolving, delivered through + // the reaction the thumbs sweep advertises. Resolution state does not + // gate it; a still-open downvoted thread is also in the open corpus, + // and the composed pass attributes a double match to the open thread. + for (const state of [ + {resolved: false, resolvedBy: ""}, + {resolved: true, resolvedBy: "github-actions"}, + {resolved: undefined, resolvedBy: undefined}, + ]) { + expect( + adjudicatedThreadsFromStaged([ + adjudicated({...state, openerDownvotes: 1}), + ]), + ).toEqual([ + { + thread_id: "T1", + path: "a.ts", + body: "**suggestion (non-blocking):** opener", + }, + ]); + } + }); + + it("fails closed on every guard: unresolved, bot-resolved, unattributable resolver, human opener, malformed staging", () => { + // Each rejected shape degrades to a duplicate comment, never to a + // suppression the staging cannot justify: this corpus grants the + // strongest suppression in the pipeline (a human's explicit "settled" + // outlives rephrasings), so membership must be unmanufacturable. + const rejected: unknown[] = [ + adjudicated({resolved: false}), + adjudicated({resolved: undefined}), + adjudicated({resolved: "true"}), + // A downvote count must be an explicit positive number: absent, + // zero, or malformed reads as no downvote, and a downvote alone + // never launders a thread that fails the bot-opener guard. + adjudicated({resolved: false, openerDownvotes: 0}), + adjudicated({resolved: false, openerDownvotes: "1"}), + adjudicated({ + resolved: false, + openerDownvotes: 1, + comments: [{author: "jwbron", body: "human opener"}], + }), + // The bot resolving its own thread is the reconciler marking a + // defect FIXED; a fixed defect that reappears is a fresh finding. + adjudicated({resolvedBy: "github-actions"}), + adjudicated({resolvedBy: "github-actions[bot]"}), + adjudicated({resolvedBy: ""}), + adjudicated({resolvedBy: undefined}), + adjudicated({ + comments: [{author: "jwbron", body: "human opener"}], + }), + adjudicated({comments: []}), + adjudicated({thread_id: undefined}), + "not a record", + ]; + for (const thread of rejected) { + expect(adjudicatedThreadsFromStaged([thread])).toEqual([]); + } + expect(adjudicatedThreadsFromStaged(undefined)).toEqual([]); + expect(adjudicatedThreadsFromStaged({not: "an array"})).toEqual([]); + }); +}); + +describe("suppressAdjudicatedDuplicates", () => { + const adjudicatedThread = (over: Record = {}) => ({ + thread_id: "T-adj", + path: "services/ai-guide/memory/expiration.go", + body: "**suggestion (non-blocking):** No test exercises the deletion path: TestExpiration only asserts that expired keys are identified, so a regression that identifies but never deletes expired memories stays green.", + ...over, + }); + const rederivation = (over: Partial = {}) => + claim({ + id: "correctness-reviewer-2", + source: "correctness-reviewer", + line: 42, + label: "suggestion (non-blocking)", + subject: + "Missing deletion test: the expiration path has no test covering the delete.", + discussion: + "No test exercises the deletion path; TestExpiration asserts expired keys are identified but a regression that never deletes expired memories stays green.", + failure_scenario: + "A regression that identifies expired memories but skips the deletion is not caught by TestExpiration and ships green.", + ...over, + }); + + it("suppresses a non-blocking re-derivation of an adjudicated defect, marked as adjudicated", () => { + const {kept, suppressed} = suppressAdjudicatedDuplicates( + [rederivation()], + [adjudicatedThread()], + ); + expect(kept).toEqual([]); + expect(suppressed).toEqual([ + { + id: "correctness-reviewer-2", + source: "correctness-reviewer", + label: "suggestion (non-blocking)", + path: "services/ai-guide/memory/expiration.go", + line: 42, + thread_id: "T-adj", + threadBlocking: false, + adjudicated: true, + }, + ]); + }); + + it("never suppresses a blocking candidate: a regression re-flag must stay visible", () => { + // The adjudicated thread is closed and floors nothing, so suppressing + // a blocker on it would let a re-confirmed blocking defect vanish + // without a trace. This asymmetry is also the regression escape + // hatch: a fixed-then-regressed defect worth stopping the PR for + // re-presents at blocking severity and posts. + const blocking = rederivation({label: "issue (blocking)"}); + const {kept, suppressed} = suppressAdjudicatedDuplicates( + [blocking], + [adjudicatedThread()], + ); + expect(kept).toEqual([blocking]); + expect(suppressed).toEqual([]); + }); + + it("keeps unrelated and pathless claims, and everything when the corpus is empty", () => { + const unrelated = rederivation({ + subject: "Retention window subtracts months, not days.", + discussion: + "AddDate(0, -MemoryTTLDays, 0) subtracts 180 months so the window never expires anything.", + failure_scenario: + "Memories never expire because the cutoff is 15 years in the past.", + }); + const pathless = rederivation({path: undefined, line: undefined}); + const empty = suppressAdjudicatedDuplicates([rederivation()], []); + expect(empty.kept).toHaveLength(1); + expect(empty.suppressed).toEqual([]); + const {kept, suppressed} = suppressAdjudicatedDuplicates( + [unrelated, pathless], + [adjudicatedThread()], + ); + expect(kept).toEqual([unrelated, pathless]); + expect(suppressed).toEqual([]); + }); + + it("attributes a candidate matching BOTH corpora to the OPEN thread (the verdict floor reads its blocking state)", () => { + // The composed pass order is the guarantee dispatch.ts relies on: the + // open corpus runs first, so a defect that is simultaneously tracked + // by an open thread and settled on an older resolved one suppresses + // against the OPEN thread, whose blocking state floors the verdict. + const openStaged = [ + { + thread_id: "T-open", + path: "services/ai-guide/memory/expiration.go", + resolved: false, + comments: [ + { + author: "github-actions", + body: adjudicatedThread().body, + }, + ], + }, + ]; + const adjudicatedStaged = [ + { + thread_id: "T-adj", + path: "services/ai-guide/memory/expiration.go", + resolved: true, + resolvedBy: "octo", + comments: [ + { + author: "github-actions", + body: adjudicatedThread().body, + }, + ], + }, + ]; + const result = suppressTrackedDuplicates( + [rederivation()], + openStaged, + adjudicatedStaged, + new Set(), + ); + expect(result.kept).toEqual([]); + expect(result.suppressed).toHaveLength(1); + expect(result.suppressed[0].thread_id).toBe("T-open"); + expect(result.suppressed[0].adjudicated).toBeUndefined(); + expect(result.shapeFailure).toBeUndefined(); + }); +}); diff --git a/workflows/review/lib/dedup-adjudicated.ts b/workflows/review/lib/dedup-adjudicated.ts new file mode 100644 index 00000000..34d6ef3f --- /dev/null +++ b/workflows/review/lib/dedup-adjudicated.ts @@ -0,0 +1,199 @@ +/** + * Adjudicated-thread suppression: the human-resolution memory the open-thread + * corpus cannot carry. Split from `dedup.ts` for its max-lines budget + * (following the precedent dedup-cluster.ts set) and because it is one + * self-contained concern: what a HUMAN resolving a bot thread means for later + * runs. + * + * Before this module, resolving a bot thread was quietly the opposite of + * settling it. threads.json stages only UNRESOLVED threads, and the + * open-thread suppression corpus is built from exactly that file, so a + * resolution removed the defect from the suppression shield and the next run + * was free to re-derive the same concern with fresh wording as a brand-new + * thread, which every later accountability recap then enumerated as "still + * unaddressed". Observed on webapp#41290: the author replied to and resolved + * SIX variants of one concern at moderation_helpers.go:135 over two days, and + * a seventh rephrasing posted anyway. From the author's side that is + * indistinguishable from the bot ignoring every resolution. + * + * The corpus this module consumes is staged by stage-pr.ts (step 5b', + * adjudicated-threads.json): bot-opened threads whose `resolvedBy` is a + * human, plus bot-opened threads whose OPENING comment carries a 👎 (the + * same judgment through the other feedback channel the bot advertises; the + * thumbs sweep asks "why?" on exactly that signal, and until this module the + * answer dead-ended in counters). The resolver identity is the resolution + * membership rule, not resolution alone; a thread the BOT resolved is the + * reconciler marking a defect FIXED, and a fixed defect that reappears is a + * fresh finding that must post. + */ + +import { + bestOpenThreadMatch, + openThreadsFromStaged, + stagedResolvedState, + stagedThreadShapeFailure, + suppressOpenThreadDuplicates, + threadOpenerIsBlocking, + type OpenThread, + type ThreadSuppression, +} from "./dedup"; +import {isRecord, type Claim} from "./dispatch-contracts"; +import {isBlockingLabel} from "./render-comment"; +import {isReviewBotAuthor} from "./threads"; + +/** + * Build the adjudicated-corpus suppression inputs from staged + * adjudicated-threads.json. Same {@link OpenThread} shape as the open corpus, + * because the matcher ({@link bestOpenThreadMatch}) is shared. + * + * The guards mirror `openThreadsFromStaged`'s, with the resolution checks + * INVERTED and strengthened: membership requires an explicit `resolved: true` + * AND a non-empty human `resolvedBy`, OR an explicit positive + * `openerDownvotes` count, because this corpus grants the strongest + * suppression in the pipeline (a defect a human marked settled stays settled + * across rephrasings) and must never be manufacturable from a malformed + * staging. Each guard fails closed toward NOT suppressing: a thread without + * a bot opener, without either signal, or whose resolver is absent, + * unattributable (""), or the bot itself contributes nothing, and the worst + * case is a duplicate comment. + */ +export const adjudicatedThreadsFromStaged = (threads: unknown): OpenThread[] => + (Array.isArray(threads) ? threads : []) + .filter(isRecord) + .flatMap((thread) => { + const comments = thread["comments"]; + const opener = + Array.isArray(comments) && isRecord(comments[0]) + ? comments[0] + : undefined; + const author = opener?.["author"]; + const resolvedBy = thread["resolvedBy"]; + const humanResolved = + stagedResolvedState(thread) === true && + typeof resolvedBy === "string" && + resolvedBy !== "" && + !isReviewBotAuthor(resolvedBy); + const downvotes = thread["openerDownvotes"]; + const downvoted = typeof downvotes === "number" && downvotes > 0; + if ( + typeof thread["thread_id"] !== "string" || + (!humanResolved && !downvoted) || + typeof author !== "string" || + !isReviewBotAuthor(author) + ) { + return []; + } + const path = + typeof thread["path"] === "string" + ? thread["path"] + : typeof opener?.["path"] === "string" + ? (opener["path"] as string) + : undefined; + return [ + { + thread_id: thread["thread_id"], + ...(path !== undefined ? {path} : {}), + body: + typeof opener["body"] === "string" + ? opener["body"] + : "", + }, + ]; + }); + +/** + * Drop candidate claims that re-derive a defect a human already ADJUDICATED. + * + * Two deliberate asymmetries against `suppressOpenThreadDuplicates`: + * + * - A BLOCKING candidate is never suppressed here. The open corpus can + * suppress blockers because the matched thread is still open and floors the + * verdict in the candidate's stead; an adjudicated thread is closed and + * floors nothing, so suppressing a blocker on it would let a re-confirmed + * blocking defect vanish without a trace. This is also the regression + * escape hatch the old "resolved threads never suppress" rule protected: a + * fixed-then-regressed defect worth stopping the PR for re-presents at + * blocking severity and posts. + * - There is no verdict-floor bookkeeping. The suppression record carries + * `adjudicated: true` and the matched thread's opener severity for the + * audit trail, but submission.ts's floor requires the CANDIDATE to be + * blocking, which no claim suppressed here is. + * + * The match itself is the shared one (same path, no line window, the #245 + * similarity floors via {@link bestOpenThreadMatch}): an adjudicated defect's + * rephrasing lands on nearby lines with new wording exactly the way a + * persisting open defect's re-flag does. + */ +export const suppressAdjudicatedDuplicates = ( + claims: Claim[], + threads: OpenThread[], +): {kept: Claim[]; suppressed: ThreadSuppression[]} => { + if (threads.length === 0) { + return {kept: claims, suppressed: []}; + } + const kept: Claim[] = []; + const suppressed: ThreadSuppression[] = []; + for (const claim of claims) { + const match = + claim.path === undefined || isBlockingLabel(claim.label) + ? undefined + : bestOpenThreadMatch(claim, threads); + if (match === undefined) { + kept.push(claim); + continue; + } + suppressed.push({ + id: claim.id, + source: claim.source, + label: claim.label, + path: claim.path as string, + ...(claim.line !== undefined ? {line: claim.line} : {}), + thread_id: match.thread_id, + threadBlocking: threadOpenerIsBlocking(match.body), + adjudicated: true, + }); + } + return {kept, suppressed}; +}; + +/** + * Both thread-suppression passes in dispatch order, as ONE call: the open + * corpus first (its matches carry the verdict-floor bookkeeping), then the + * adjudicated corpus over what survived. Extracted here rather than inlined + * in dispatch.ts for that file's max-lines budget, and so the pass ORDER is a + * property of this module rather than of the call site: a candidate matching + * BOTH corpora must be attributed to the OPEN thread, whose blocking state + * the verdict floor reads. + * + * Inputs are the raw staged values (threads.json, adjudicated-threads.json + * parsed or undefined when absent); every shape guard lives in the two corpus + * builders, so an older staging without the adjudicated file degrades to an + * empty corpus and suppresses nothing. `shapeFailure` is the open corpus's + * total-failure tripwire, surfaced unchanged (see `stagedThreadShapeFailure`). + */ +export const suppressTrackedDuplicates = ( + claims: Claim[], + stagedOpen: unknown, + stagedAdjudicated: unknown, + resolvedIds: ReadonlySet, +): { + kept: Claim[]; + suppressed: ThreadSuppression[]; + shapeFailure: ReturnType; +} => { + const openThreads = openThreadsFromStaged(stagedOpen, resolvedIds); + const open = suppressOpenThreadDuplicates(claims, openThreads); + const adjudicated = suppressAdjudicatedDuplicates( + open.kept, + adjudicatedThreadsFromStaged(stagedAdjudicated), + ); + return { + kept: adjudicated.kept, + suppressed: [...open.suppressed, ...adjudicated.suppressed], + shapeFailure: stagedThreadShapeFailure( + stagedOpen, + openThreads, + resolvedIds, + ), + }; +}; diff --git a/workflows/review/lib/dedup-text.ts b/workflows/review/lib/dedup-text.ts new file mode 100644 index 00000000..58bb39ee --- /dev/null +++ b/workflows/review/lib/dedup-text.ts @@ -0,0 +1,43 @@ +/** + * The text-similarity primitives every dedup tier scores with: content + * tokenization (lowercased alphanumerics, stopwords and short words + * dropped), token bigrams, and set intersection. Split from `dedup.ts` for + * its max-lines budget (the dedup-cluster.ts precedent); they are + * dependency-free and every floor in dedup.ts (same-line, other-line, + * pr-level) is calibrated against exactly these definitions, so a change + * here re-opens every calibration note there. + */ + +const STOPWORDS = new Set( + "the a an and or of to in is are was be for on with that this it as not no by at from so its their they".split( + " ", + ), +); + +export const contentTokens = (text: string): string[] => { + const tokens: string[] = []; + for (const word of text.toLowerCase().match(/[a-z0-9]+/g) ?? []) { + if (word.length >= 3 && !STOPWORDS.has(word)) { + tokens.push(word); + } + } + return tokens; +}; + +export const bigrams = (tokens: string[]): Set => { + const set = new Set(); + for (let i = 0; i + 1 < tokens.length; i += 1) { + set.add(`${tokens[i]} ${tokens[i + 1]}`); + } + return set; +}; + +export const intersectionSize = (a: Set, b: Set): number => { + let count = 0; + for (const item of a) { + if (b.has(item)) { + count += 1; + } + } + return count; +}; diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 96f3d056..0ed92e47 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -90,6 +90,7 @@ * can cost is bounded by code even where its judgment cannot be checked. */ +import {bigrams, contentTokens, intersectionSize} from "./dedup-text"; import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; import {isBlockingLabel} from "./render-comment"; import { @@ -166,40 +167,6 @@ const OTHER_LINE_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 6}; */ const PR_LEVEL_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 8}; -const STOPWORDS = new Set( - "the a an and or of to in is are was be for on with that this it as not no by at from so its their they".split( - " ", - ), -); - -const contentTokens = (text: string): string[] => { - const tokens: string[] = []; - for (const word of text.toLowerCase().match(/[a-z0-9]+/g) ?? []) { - if (word.length >= 3 && !STOPWORDS.has(word)) { - tokens.push(word); - } - } - return tokens; -}; - -const bigrams = (tokens: string[]): Set => { - const set = new Set(); - for (let i = 0; i + 1 < tokens.length; i += 1) { - set.add(`${tokens[i]} ${tokens[i + 1]}`); - } - return set; -}; - -const intersectionSize = (a: Set, b: Set): number => { - let count = 0; - for (const item of a) { - if (b.has(item)) { - count += 1; - } - } - return count; -}; - const comparisonKey = (text: string): string => text .toLowerCase() @@ -285,6 +252,15 @@ export type ThreadSuppression = { * threads that clear the floor their labels can differ. */ threadBlocking: boolean; + /** + * Present (and true) when the matched thread is from the ADJUDICATED + * corpus (a bot thread a human resolved) rather than an open one; see + * dedup-adjudicated.ts. Audit-trail only: an adjudicated suppression can + * never floor the verdict, because that pass never suppresses a blocking + * candidate and submission.ts's floor requires the CANDIDATE's label to + * be blocking too. + */ + adjudicated?: true; }; /** @@ -298,7 +274,7 @@ export type ThreadSuppression = { * recognizable label reads as non-blocking, since an unvalidated floor is the * failure mode this guards. */ -const threadOpenerIsBlocking = (body: string): boolean => { +export const threadOpenerIsBlocking = (body: string): boolean => { const label = /^\s*\*{0,2}([a-z]+ \([^)]*\))\*{0,2}:?/i.exec(body)?.[1]; return ( label !== undefined && @@ -335,7 +311,7 @@ const threadProse = (body: string): string => * hand-built staging used to reproduce a run) inherits its shape. Absent * reads as unknown, not as open. */ -const stagedResolvedState = (thread: Record): unknown => +export const stagedResolvedState = (thread: Record): unknown => thread["resolved"] ?? thread["is_resolved"] ?? thread["isResolved"]; /** @@ -478,7 +454,7 @@ export const describesOpenThreadDefect = ( * comparisons keep staging order as the final tiebreak, so an exact scoring * tie behaves as it did before. */ -const bestOpenThreadMatch = ( +export const bestOpenThreadMatch = ( claim: Claim, threads: readonly OpenThread[], ): OpenThread | undefined => { diff --git a/workflows/review/lib/dispatch-adjudicated.test.ts b/workflows/review/lib/dispatch-adjudicated.test.ts new file mode 100644 index 00000000..28c59721 --- /dev/null +++ b/workflows/review/lib/dispatch-adjudicated.test.ts @@ -0,0 +1,225 @@ +import {describe, it, expect} from "vitest"; + +import {runDispatch, type AgentRunner, type DispatchFs} from "./dispatch"; +import {computeDiffProvenance} from "./provenance"; + +/** + * Adjudicated-thread suppression, end to end through `runDispatch`: the + * staged adjudicated-threads.json corpus, the pre-validation drop, and the + * blocking exemption. Split from dispatch.test.ts for its max-lines budget + * (the dispatch-cluster.test.ts precedent); the fixtures mirror that file's. + * + * The scenario is webapp#41290's loop: the author resolves the bot's thread, + * the next run re-derives the same defect with fresh wording, and the + * accountability recap then reports it "still unaddressed". + */ + +const REVIEW = "/tmp/gh-aw/review"; +const AGENTS = "/work/.claude/agents"; + +const makeFakeFs = ( + files: Record = {}, +): DispatchFs & {files: Record} => { + const state = {...files}; + return { + files: state, + readFileSync: (p: string) => { + if (!(p in state)) { + throw new Error(`ENOENT: ${p}`); + } + return state[p]; + }, + writeFileSync: (p: string, data: string) => { + state[p] = data; + }, + existsSync: (p: string) => + p in state || Object.keys(state).some((f) => f.startsWith(`${p}/`)), + mkdirSync: () => {}, + readdirSync: (p: string) => { + const prefix = `${p}/`; + return [ + ...new Set( + Object.keys(state) + .filter((f) => f.startsWith(prefix)) + .map((f) => f.slice(prefix.length).split("/")[0]), + ), + ]; + }, + }; +}; + +const agentFile = (name: string): string => + `---\nname: ${name}\ndescription: d\nmodel: claude-opus-4-8\n---\nYou are ${name}. Read from disk and return JSON.`; + +const agentFiles = (...names: string[]): Record => + Object.fromEntries( + names.map((name) => [`${AGENTS}/${name}.md`, agentFile(name)]), + ); + +/** A runner stub: canned final text per agent, throwing for names in fail. */ +const stubRunner = ( + outputs: Record, + fail: string[] = [], +): AgentRunner & {calls: string[]} => { + const calls: string[] = []; + const runner = (async (request) => { + calls.push(request.name); + if (fail.includes(request.name)) { + throw new Error("boom"); + } + const output = outputs[request.name]; + if (output === undefined) { + throw new Error(`no canned output for ${request.name}`); + } + return {output, usd: 0.5, turns: 3, wallMs: 100}; + }) as AgentRunner & {calls: string[]}; + runner.calls = calls; + return runner; +}; + +const DIFF = [ + "diff --git a/a.ts b/a.ts", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1,2 +1,3 @@", + " ctx", + "+added line", + " ctx", + "", +].join("\n"); + +const baseStaging = (): Record => ({ + [`${REVIEW}/routing.json`]: JSON.stringify({ + enabledReviewers: [], + lensesToSpawn: [], + runBudget: {maxReviewerInvocations: 6, tier: "High"}, + }), + [`${REVIEW}/rereview-plan.json`]: JSON.stringify({depth: "full"}), + [`${REVIEW}/full.diff`]: DIFF, + [`${REVIEW}/files.json`]: JSON.stringify([ + {path: "a.ts", status: "modified", hasPatch: true}, + ]), + [`${REVIEW}/provenance.json`]: JSON.stringify(computeDiffProvenance(DIFF)), +}); + +const EMPTY_FINDINGS = JSON.stringify({findings: []}); + +const TRIAGE_OK = JSON.stringify({patterns: [], reviewFiles: ["a.ts"]}); + +const VALIDATOR_CONFIRM = JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.9, + }, + ], +}); + +/** The re-derivation finding, at the label the case under test wants. */ +const rederivationOut = (label: string): string => + JSON.stringify({ + findings: [ + { + path: "a.ts", + line: 2, + label, + subject: + "Missing deletion test: the expiration path has no test covering the delete.", + discussion: + "No test exercises the deletion path; TestExpiration asserts expired keys are identified but a regression that never deletes expired memories stays green.", + failure_scenario: + "A regression that identifies expired memories but skips the deletion is not caught by TestExpiration and ships green.", + }, + ], + files: [], + }); + +const ADJUDICATED_STAGING = JSON.stringify([ + { + thread_id: "T-adj", + path: "a.ts", + resolved: true, + resolvedBy: "octo", + comments: [ + { + author: "github-actions", + body: "**suggestion (non-blocking):** No test exercises the deletion path: TestExpiration only asserts that expired keys are identified, so a regression that identifies but never deletes expired memories stays green.", + }, + ], + }, +]); + +describe("runDispatch adjudicated-thread suppression", () => { + const options = (fs: DispatchFs, runner: AgentRunner) => ({ + fs, + runner, + repoRoot: "/work", + }); + + it("suppresses a non-blocking re-derivation of an adjudicated defect before validation", () => { + const fs = makeFakeFs({ + ...baseStaging(), + [`${REVIEW}/adjudicated-threads.json`]: ADJUDICATED_STAGING, + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": rederivationOut( + "suggestion (non-blocking)", + ), + "skill-auditor": EMPTY_FINDINGS, + }); + return runDispatch(options(fs, runner)).then((result) => { + expect(result.claims).toEqual([]); + expect(result.threadSuppressions).toEqual([ + { + id: "correctness-reviewer-1", + source: "correctness-reviewer", + label: "suggestion (non-blocking)", + path: "a.ts", + line: 2, + thread_id: "T-adj", + threadBlocking: false, + adjudicated: true, + }, + ]); + // Suppressed pre-validation: with nothing left to validate, the + // validator is never dispatched. + expect(runner.calls).not.toContain("claim-validator"); + }); + }); + + it("never adjudicates away a blocking candidate: a regression re-flag posts", () => { + // The adjudicated thread is closed and floors nothing, so a blocking + // re-flag suppressed against it would vanish without a trace. It must + // reach validation and post. + const fs = makeFakeFs({ + ...baseStaging(), + [`${REVIEW}/adjudicated-threads.json`]: ADJUDICATED_STAGING, + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": rederivationOut("issue (blocking)"), + "skill-auditor": EMPTY_FINDINGS, + "claim-validator": VALIDATOR_CONFIRM, + }); + return runDispatch(options(fs, runner)).then((result) => { + expect(result.claims).toHaveLength(1); + expect(result.claims[0].label).toBe("issue (blocking)"); + expect(result.threadSuppressions).toEqual([]); + expect(runner.calls).toContain("claim-validator"); + }); + }); +}); diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index 0d70494b..cda59bcc 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -39,14 +39,8 @@ * note lines) is pure code. No prose about the code under review. */ -import { - dedupeClaims, - openThreadsFromStaged, - stagedThreadShapeFailure, - suppressOpenThreadDuplicates, - type ClaimMerge, - type ThreadSuppression, -} from "./dedup"; +import {dedupeClaims, type ClaimMerge, type ThreadSuppression} from "./dedup"; +import {suppressTrackedDuplicates} from "./dedup-adjudicated"; import { clusteringRecord, runClusterStep, @@ -830,25 +824,25 @@ export const runDispatch = async ( ); let claims = deduped.claims; - // Open-thread suppression (trial suggestion g), also before validation: - // a defect an open bot thread already tracks is not re-validated or - // re-posted at a new anchor. Threads the reconciler resolves this run - // are exempt; when the reconciler was unavailable, nothing resolves, so - // every staged bot thread suppresses (fail toward fewer duplicate - // threads). The bot-opener filter, and the check for a staging whose shape - // defeats it and so suppresses nothing, both live in dedup.ts beside the - // rules they enforce; the staging is code now (stage-pr.ts), and the guards - // stay so a producer bug degrades to a duplicate, never to a dropped - // finding. - const resolvedIds = new Set(reconciliation?.resolve ?? []); - const openThreads = openThreadsFromStaged(threads, resolvedIds); - const suppression = suppressOpenThreadDuplicates(claims, openThreads); - claims = suppression.kept; - const threadSuppressionUnavailable = stagedThreadShapeFailure( + // Thread suppression (trial suggestion g), also before validation: a + // defect an OPEN bot thread already tracks, or one a human ADJUDICATED by + // resolving its thread (webapp#41290: six resolved variants of one + // concern, then a seventh posted anyway), is not re-validated or + // re-posted at a new anchor. Threads the reconciler resolves this run are + // exempt from the open corpus; blocking candidates are exempt from the + // adjudicated one (a closed thread floors nothing, so a regression + // re-flag at blocking severity must stay visible). Every filter and guard + // lives in dedup.ts / dedup-adjudicated.ts beside the rules it enforces; + // a producer bug or an older staging without adjudicated-threads.json + // degrades to a duplicate comment, never to a dropped finding. + const suppression = suppressTrackedDuplicates( + claims, threads, - openThreads, - resolvedIds, + readJson(fs, `${REVIEW_DIR}/adjudicated-threads.json`), + new Set(reconciliation?.resolve ?? []), ); + claims = suppression.kept; + const threadSuppressionUnavailable = suppression.shapeFailure; if (threadSuppressionUnavailable !== undefined) { // eslint-disable-next-line no-console console.error(threadSuppressionUnavailable.warning); diff --git a/workflows/review/lib/stage-pr.ts b/workflows/review/lib/stage-pr.ts index e3892514..c37dc997 100644 --- a/workflows/review/lib/stage-pr.ts +++ b/workflows/review/lib/stage-pr.ts @@ -29,6 +29,10 @@ * and the opener's html_url * human-threads.json the `{path, line}` of every unresolved thread someone * ELSE opened, which the dispatcher defers to + * adjudicated-threads.json the bot's threads a HUMAN resolved or + * downvoted, which the dispatcher's adjudicated + * suppression reads so a settled defect is not + * re-derived under fresh wording * disciplines.md the marker-delimited shared-disciplines section, cut * out of the rendered prompt (slice 3, #247) * routing.json the router's deterministic first pass (a non-empty @@ -87,7 +91,7 @@ import type {StagedThread} from "./rereview"; import {runRereviewPlanCli} from "./rereview-mode"; import {runCli as runRouterCli} from "./router"; import { - collectUnresolvedThreads, + collectReviewThreads, isReviewBotAuthor, withGraphqlRateLimitRetry, type GhGraphql, @@ -114,6 +118,7 @@ const NEW_SCOPE_OUT = `${REVIEW_DIR}/new-scope.json`; const PRIOR_REVIEWS_OUT = `${REVIEW_DIR}/prior-reviews.json`; const THREADS_OUT = `${REVIEW_DIR}/threads.json`; const HUMAN_THREADS_OUT = `${REVIEW_DIR}/human-threads.json`; +const ADJUDICATED_THREADS_OUT = `${REVIEW_DIR}/adjudicated-threads.json`; const ROUTING_OUT = `${REVIEW_DIR}/routing.json`; const PROVENANCE_OUT = `${REVIEW_DIR}/provenance.json`; const STRIPPED_DIFF_OUT = `${REVIEW_DIR}/full-stripped.diff`; @@ -541,12 +546,26 @@ export const runStagePrCli = async ( // duplicating one. Hence one fetch, one partition: a thread is in exactly // one file, and neither list is assembled by hand. const [owner = "", repoName = ""] = repo.split("/"); - const allThreads = await collectUnresolvedThreads( + const fetchedThreads = await collectReviewThreads( ghGraphql, owner, repoName, prNumber, ); + // The unresolved partition, in the exact `StagedThread` shape every + // downstream reader of threads.json / human-threads.json already parses: + // the resolution and reaction fields are stripped, not carried, because + // both files serialize these objects verbatim. + const allThreads = fetchedThreads + .filter((thread) => !thread.resolved) + .map( + ({ + resolved: _resolved, + resolvedBy: _resolvedBy, + openerDownvotes: _openerDownvotes, + ...thread + }) => thread, + ); // The OPENER decides which file a thread lands in (its opening comment is // the finding), so a thread with no opener at all is staged in NEITHER. A // real review thread always has one and a partial GraphQL response throws @@ -578,16 +597,51 @@ export const runStagePrCli = async ( JSON.stringify( botThreads.map((thread) => ({ ...thread, - // Unresolved by construction (the fetch drops resolved - // threads), but written anyway: dedup.ts requires an explicit - // `resolved: false` and fails closed without one. That guard - // stays deliberately, rather than trusting this producer. + // Unresolved by construction (the partition above drops + // resolved threads), but written anyway: dedup.ts requires an + // explicit `resolved: false` and fails closed without one. + // That guard stays deliberately, rather than trusting this + // producer. resolved: false, })), null, 2, ), ); + // 5b'. The adjudicated corpus: bot-opened threads a HUMAN resolved, or + // whose opening comment a reviewer downvoted. A human resolving a bot + // thread is the strongest "this is settled" signal the PR surface + // carries, and before this file existed it was also an anti-signal: + // resolution removed the thread from threads.json, so the suppression + // corpus, so the next run was free to re-derive the same defect with + // fresh wording as a brand-new thread (webapp#41290: six resolved + // variants of one concern at moderation_helpers.go:135, then a seventh + // posted anyway). A 👎 on the opener is the same judgment delivered + // through the OTHER feedback channel the bot advertises (the thumbs + // sweep asks "why?" on exactly this signal), and before this it + // dead-ended in counters. dedup-adjudicated.ts's suppression reads this + // file; only non-blocking candidates are suppressed by it, so a genuine + // regression re-flag at blocking severity always posts. + // + // The resolver identity decides resolution membership, not resolution + // alone: a thread the BOT resolved (the reconciler, after a code change + // addressed it) is a fixed defect, and a fixed defect that reappears is + // a fresh finding that must post. `resolvedBy` is "" for an + // unattributable resolver (a deleted account), which fails toward + // posting a duplicate, never toward suppression on unverifiable + // authority. A downvoted thread joins whatever its resolution state: + // still-open downvoted threads are also in threads.json, and the + // composed suppression attributes a candidate matching both corpora to + // the OPEN thread, whose blocking state floors the verdict. + const adjudicatedThreads = fetchedThreads.filter( + (thread) => + openedByBot(thread) && + ((thread.resolved && + thread.resolvedBy !== "" && + !isReviewBotAuthor(thread.resolvedBy)) || + thread.openerDownvotes > 0), + ); + write(ADJUDICATED_THREADS_OUT, JSON.stringify(adjudicatedThreads, null, 2)); // The reconciler echoes these into `skipLines`, so a thread with no // RIGHT-side line (outdated, or file-level) has nothing to contribute and // is dropped rather than staged as a line the submission cannot match. diff --git a/workflows/review/lib/stage-threads.test.ts b/workflows/review/lib/stage-threads.test.ts index 4a4a06ce..9e5b1240 100644 --- a/workflows/review/lib/stage-threads.test.ts +++ b/workflows/review/lib/stage-threads.test.ts @@ -1,6 +1,7 @@ import {describe, it, expect} from "vitest"; import {openThreadsFromStaged, stagedThreadShapeFailure} from "./dedup"; +import {adjudicatedThreadsFromStaged} from "./dedup-adjudicated"; import {computeRoster} from "./dispatch-roster"; import {runStagePrCli, type GhGet, type StagePrFs} from "./stage-pr"; import {withGraphqlRateLimitRetry, type GhGraphql} from "./threads"; @@ -124,6 +125,9 @@ describe("review-thread staging (slice 1)", () => { result, threads: JSON.parse(fs.files[`${REVIEW}/threads.json`]), humanThreads: JSON.parse(fs.files[`${REVIEW}/human-threads.json`]), + adjudicated: JSON.parse( + fs.files[`${REVIEW}/adjudicated-threads.json`], + ), }; }; @@ -232,7 +236,7 @@ describe("review-thread staging (slice 1)", () => { expect(humanThreads).toEqual([]); }); - it("drops resolved threads and omits an absent opener url", async () => { + it("drops resolved threads from the unresolved partition and omits an absent opener url", async () => { const {threads, humanThreads} = await stage([ threadPage([ threadNode({id: "PRRT_done", isResolved: true}), @@ -251,9 +255,212 @@ describe("review-thread staging (slice 1)", () => { ]); expect(threads).toHaveLength(1); expect("url" in threads[0]).toBe(false); + // The resolution fields serve the adjudicated partition only; leaking + // them into threads.json would be a shape change to every exact-match + // reader of the unresolved staging. + expect("resolvedBy" in threads[0]).toBe(false); expect(humanThreads).toEqual([]); }); + it("stages a human-resolved bot thread as adjudicated, and the corpus filter accepts the staged bytes", async () => { + // The webapp#41290 shape: the author resolved the bot's thread, and + // before this file existed that resolution REMOVED the defect from + // the suppression corpus, so the next run could re-derive it with + // fresh wording. Same producer-to-consumer bind as the open-corpus + // case above: the staged bytes go straight into + // `adjudicatedThreadsFromStaged`, so a shape drift fails HERE. + const {threads, adjudicated} = await stage([ + threadPage([ + threadNode({ + id: "PRRT_adjudicated", + isResolved: true, + resolvedBy: {login: "octo"}, + }), + threadNode({id: "PRRT_open2"}), + ]), + ]); + expect(threads.map((t: {thread_id: string}) => t.thread_id)).toEqual([ + "PRRT_open2", + ]); + expect(adjudicated).toEqual([ + { + thread_id: "PRRT_adjudicated", + path: "a.ts", + line: 2, + url: "https://github.com/o/r/pull/7#discussion_r1", + comments: [ + { + author: "github-actions", + body: "**issue (blocking):** opener", + }, + ], + resolved: true, + resolvedBy: "octo", + openerDownvotes: 0, + }, + ]); + expect(adjudicatedThreadsFromStaged(adjudicated)).toEqual([ + { + thread_id: "PRRT_adjudicated", + path: "a.ts", + body: "**issue (blocking):** opener", + }, + ]); + }); + + it("stages a downvoted OPEN bot thread in BOTH files: open for the verdict floor, adjudicated for re-derivation", async () => { + // The 👎 arrives over the opener's reactions connection; the thread is + // still open, so it stays in threads.json (the open corpus carries + // the verdict-floor bookkeeping) AND joins the adjudicated corpus + // (so the settled defect cannot re-post under fresh wording later). + const {threads, adjudicated} = await stage([ + threadPage([ + threadNode({ + id: "PRRT_downvoted", + comments: { + nodes: [ + { + author: {login: "github-actions"}, + body: "**note (non-blocking):** opener", + url: "https://github.com/o/r/pull/7#discussion_r9", + reactions: { + nodes: [ + {user: {login: "octo"}}, + {user: {login: "hubot"}}, + ], + }, + }, + ], + }, + }), + ]), + ]); + expect(threads.map((t: {thread_id: string}) => t.thread_id)).toEqual([ + "PRRT_downvoted", + ]); + // No reaction leak into the open staging's exact shape. + expect("openerDownvotes" in threads[0]).toBe(false); + expect(adjudicated).toEqual([ + { + thread_id: "PRRT_downvoted", + path: "a.ts", + line: 2, + url: "https://github.com/o/r/pull/7#discussion_r9", + comments: [ + { + author: "github-actions", + body: "**note (non-blocking):** opener", + }, + ], + resolved: false, + resolvedBy: "", + openerDownvotes: 2, + }, + ]); + expect(adjudicatedThreadsFromStaged(adjudicated)).toHaveLength(1); + }); + + it("counts only the opener's 👎, never a reply's", async () => { + // The opener is the finding; a reply's reactions are conversation. + // Structurally guaranteed today by the `rawComments[0]` indexing, and + // pinned here so a refactor that sums the chain cannot land quietly: + // a 👎 on a bot reply would otherwise adjudicate a finding its + // opener never received judgment on. + const {adjudicated} = await stage([ + threadPage([ + threadNode({ + id: "PRRT_reply_down", + comments: { + nodes: [ + { + author: {login: "github-actions"}, + body: "**note (non-blocking):** opener", + }, + { + author: {login: "octo"}, + body: "disagree", + reactions: { + nodes: [{user: {login: "octo"}}], + }, + }, + ], + }, + }), + ]), + ]); + expect(adjudicated).toEqual([]); + }); + + it("ignores the bot's own seeded 👎 and an unattributable reactor", async () => { + // The workflow plans to seed the 👍/👎 nudge pair on its own comments + // at post time (README, "Nudge seeding"); a seeded 👎 is the feedback + // widget, not a judgment, so it must not put the finding in the + // adjudicated corpus. The sweep's countDownvotes filters the same + // identity. GraphQL reports the bot bare, REST bracketed, so both + // spellings are pinned; a `user: null` reactor (deleted account) + // never reads as human adjudication, matching resolvedBy's rule. + const {adjudicated} = await stage([ + threadPage([ + threadNode({ + id: "PRRT_seeded", + comments: { + nodes: [ + { + author: {login: "github-actions"}, + body: "**note (non-blocking):** opener", + reactions: { + nodes: [ + {user: {login: "github-actions"}}, + { + user: { + login: "github-actions[bot]", + }, + }, + {user: null}, + {}, + ], + }, + }, + ], + }, + }), + ]), + ]); + // Only bot and unattributable reactors: no adjudication at all. + expect(adjudicated).toEqual([]); + }); + + it("keeps bot-resolved and human-opened resolved threads out of the adjudicated corpus", async () => { + // Bot-resolved = the reconciler marking a defect FIXED (its regression + // must re-post); a resolved HUMAN thread is not the bot's finding and + // adjudicates nothing. An unattributable resolver (deleted account, + // GraphQL null) fails toward posting a duplicate, never toward + // suppression on unverifiable authority. + const {adjudicated} = await stage([ + threadPage([ + threadNode({ + id: "PRRT_fixed", + isResolved: true, + resolvedBy: {login: "github-actions"}, + }), + threadNode({ + id: "PRRT_human_resolved", + isResolved: true, + resolvedBy: {login: "octo"}, + comments: { + nodes: [{author: {login: "octo"}, body: "human"}], + }, + }), + threadNode({ + id: "PRRT_ghost", + isResolved: true, + resolvedBy: null, + }), + ]), + ]); + expect(adjudicated).toEqual([]); + }); + it("skips human threads with no RIGHT-side line and collapses duplicates", async () => { // The reconciler echoes these into `skipLines`, which is matched on // `{path, line}`: an outdated thread has no line to skip, and two diff --git a/workflows/review/lib/threads.ts b/workflows/review/lib/threads.ts index f731c03c..5b9db653 100644 --- a/workflows/review/lib/threads.ts +++ b/workflows/review/lib/threads.ts @@ -117,10 +117,18 @@ query ($owner: String!, $repo: String!, $number: Int!, $cursor: String) { nodes { id isResolved + resolvedBy { login } path line comments(first: 100) { - nodes { author { login } body url } + nodes { + author { login } + body + url + reactions(first: 100, content: THUMBS_DOWN) { + nodes { user { login } } + } + } } } } @@ -227,23 +235,70 @@ const threadsConnectionOf = ( }; /** - * Every UNRESOLVED review thread on the PR, in API order, whoever opened it. - * Callers partition by opener ({@link isReviewBotAuthor}); nothing here is - * filtered by author, so the reviewer can stage the human threads it defers to - * from the same fetch. + * A review thread as fetched, resolution state included. The `StagedThread` + * fields carry the shape every downstream reader already consumes; + * `resolved`/`resolvedBy` exist so ONE fetch can serve both the unresolved + * partition (threads.json / human-threads.json) and the adjudicated corpus + * (adjudicated-threads.json: bot threads a HUMAN resolved, which suppress + * re-derivation of the defect they adjudicated — see dedup.ts's + * `adjudicatedThreadsFromStaged`). + */ +export type FetchedThread = StagedThread & { + resolved: boolean; + /** + * Who resolved the thread, suffix-stripped like every login comparison in + * this module (`resolvedBy` arrives over GraphQL, so the bot appears as + * bare `github-actions`; see {@link sameLogin}). Empty when the thread is + * unresolved or the resolver is unattributable (a deleted account), and + * an empty resolver never reads as human adjudication downstream. + */ + resolvedBy: string; + /** + * How many 👎 reactions from ATTRIBUTABLE NON-BOT reactors the thread's + * OPENING comment carries. The opener is the finding, so a downvote on it + * is a reviewer's judgment on the finding itself (the thumbs sweep reacts + * to exactly this signal); later comments' reactions are conversation, + * not adjudication, and are not counted. 0 when there is no opener or the + * API returned no connection. + * + * Reactor identity is filtered, not merely counted, for the same reason + * the sweep's `countDownvotes` filters `r.user !== botLogin`: the review + * workflow plans to seed the 👍/👎 nudge pair on its own comments at post + * time (README, "Nudge seeding"), and a seeded 👎 is the presence of the + * feedback widget, not a judgment on the finding. A raw `totalCount` + * cannot exclude the bot, so it would put every nudge-seeded finding in + * the adjudicated corpus the moment seeding ships. + * + * An unattributable reactor (a deleted account, GraphQL `user: null`) is + * excluded too, matching {@link FetchedThread.resolvedBy}'s rule that an + * empty identity never reads as human adjudication. This is deliberately + * STRICTER than the sweep, whose `Reaction` doc treats a login-less + * reaction as a real user's: the sweep's worst case is one wasted "why?" + * question, while this count suppresses re-derivation of the defect, and + * suppression on unverifiable authority is the expensive direction. + */ + openerDownvotes: number; +}; + +/** + * Every review thread on the PR, in API order, whoever opened it and whatever + * its resolution state. Callers partition by opener + * ({@link isReviewBotAuthor}) and by `resolved`; nothing here is filtered, so + * the reviewer can stage the human threads it defers to, its own open + * threads, and the human-adjudicated corpus from the same fetch. * * Fails closed on both shapes a failed query takes (an `errors` array, and a * body with no `reviewThreads` connection), because neither means "this PR has * no threads", and reading them that way is the one mistake this module cannot * afford (see {@link assertNoGraphqlErrors}). */ -export const collectUnresolvedThreads = async ( +export const collectReviewThreads = async ( graphql: GhGraphql, owner: string, repo: string, number: number, -): Promise => { - const out: StagedThread[] = []; +): Promise => { + const out: FetchedThread[] = []; let cursor: string | null = null; for (;;) { @@ -264,7 +319,7 @@ export const collectUnresolvedThreads = async ( const nodes = Array.isArray(conn["nodes"]) ? conn["nodes"] : []; for (const node of nodes) { - if (!isRecord(node) || node["isResolved"] === true) { + if (!isRecord(node)) { continue; } const commentsConn = isRecord(node["comments"]) @@ -286,12 +341,37 @@ export const collectUnresolvedThreads = async ( const firstUrl = isRecord(rawComments[0]) ? str(rawComments[0]["url"]) : ""; + // `resolved` is strict on `=== true`: an absent or malformed + // `isResolved` must not manufacture an adjudicated thread, and + // reading it as unresolved only risks a duplicate comment. + const resolved = node["isResolved"] === true; + const openerReactions = isRecord(rawComments[0]) + ? rawComments[0]["reactions"] + : undefined; + const reactionNodes = + isRecord(openerReactions) && + Array.isArray(openerReactions["nodes"]) + ? openerReactions["nodes"] + : []; + const openerDownvotes = reactionNodes.filter((reaction) => { + if (!isRecord(reaction) || !isRecord(reaction["user"])) { + return false; + } + const login = str(reaction["user"]["login"]); + return login !== "" && !isReviewBotAuthor(login); + }).length; out.push({ thread_id: str(node["id"]), path: str(node["path"]), line: typeof node["line"] === "number" ? node["line"] : null, ...(firstUrl === "" ? {} : {url: firstUrl}), comments, + resolved, + openerDownvotes, + resolvedBy: + resolved && isRecord(node["resolvedBy"]) + ? str(node["resolvedBy"]["login"]) + : "", }); } @@ -317,3 +397,29 @@ export const collectUnresolvedThreads = async ( cursor = next; } }; + +/** + * Every UNRESOLVED review thread on the PR, in the exact `StagedThread` shape + * the pre-`resolvedBy` collector returned. Kept as the narrow surface for the + * consumers that only ever want open threads (autofix's staging), so adding + * the adjudicated corpus could not silently change what they stage: the + * resolution fields are STRIPPED here, not merely defaulted, because both + * stagings serialize these objects verbatim and an extra field is a shape + * change to every exact-match reader downstream. + */ +export const collectUnresolvedThreads = async ( + graphql: GhGraphql, + owner: string, + repo: string, + number: number, +): Promise => + (await collectReviewThreads(graphql, owner, repo, number)) + .filter((thread) => !thread.resolved) + .map( + ({ + resolved: _resolved, + resolvedBy: _resolvedBy, + openerDownvotes: _openerDownvotes, + ...thread + }) => thread, + ); diff --git a/workflows/review/review.md b/workflows/review/review.md index 655ead44..26c7dedf 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -365,6 +365,11 @@ budget on content you never act on. threads, split by who opened them; the ones this bot opened (with their full reply chains) and the `{path, line}` of everyone else's. Step 3 says what each one feeds and the one judgment it still wants from you. +- `adjudicated-threads.json`: this bot's threads a HUMAN resolved or + downvoted. Entirely + the dispatcher's input (its suppression drops a non-blocking candidate that + re-derives a defect a human already settled); nothing in it is yours to act + on. - `routing.json`, `provenance.json`, `full-stripped.diff`, `full-stripped-annotated.diff`, `rereview-plan.json` (also copied to `out/rereview-plan.json` for the run artifact), and, on a reduced-depth @@ -591,6 +596,14 @@ misfiling one bot thread as human costs a dropped finding, per conversation is already open, so the dispatcher defers there and posts no bot comment on them. +The same fetch also staged `/tmp/gh-aw/review/adjudicated-threads.json`: this +bot's threads a HUMAN resolved or whose opener a reviewer downvoted (same +shape as `threads.json`, plus `resolved`, `resolvedBy`, and +`openerDownvotes`). It is entirely the dispatcher's input; its +suppression drops a non-blocking candidate that re-derives a defect a human +already settled, so do not read it, re-litigate it, or treat a resolved thread +as open. + **The pipeline.** Step 3 runs as ONE deterministic program; your part is exactly this sequence: 1. Read `threads.json`. If any staged bot thread's reply chain shows the author