From 15e50d4b1cab933b9830b11d4a1b2e03be4bb99d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 18:18:20 -0800 Subject: [PATCH 1/8] chore(quest): claim quest/m0/benchmark/breakage.md Quest-Claim: 428fc370b86bb9b7 From 160469b9f9f661e559aa42ade69031fa00a880bb Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 18:24:24 -0800 Subject: [PATCH 2/8] feat(records)!: record what each merged PR broke Send each breakage a merged fix names under ## Caused by, with its culprit's tier, size, writers, and reviewers, beside the session records; report an incident with quest records incident; list breakages in quest usage. Co-Authored-By: Claude Opus 5.5 --- README.md | 29 ++++++- quest/m0/benchmark/breakage.md | 53 ------------- quest/m0/benchmark/guardrails.md | 2 - quest/m0/benchmark/reviewer-eval.md | 5 -- quest/m0/benchmark/risk-tier.md | 2 - src/cli/breakage.ts | 77 ++++++++++++++++++ src/cli/main.ts | 42 +++++++++- src/cli/send.ts | 118 ++++++++++++++++++++++++---- src/cli/usage.ts | 85 ++++++++++++++++---- src/core/breakage.ts | 102 ++++++++++++++++++++++++ src/core/index.ts | 9 +++ src/core/parse.ts | 33 ++++++++ src/core/record.ts | 56 ++++++------- test/records.test.ts | 70 +++++++++++++++-- test/send.test.ts | 115 +++++++++++++++++++++++++-- test/usage.test.ts | 34 +++++++- 16 files changed, 695 insertions(+), 137 deletions(-) delete mode 100644 quest/m0/benchmark/breakage.md create mode 100644 src/cli/breakage.ts create mode 100644 src/core/breakage.ts create mode 100644 src/core/parse.ts diff --git a/README.md b/README.md index c7b5939b..c23e7f58 100644 --- a/README.md +++ b/README.md @@ -1650,7 +1650,12 @@ fitting several, such as a routed and a shadow reviewer at one effort whose sessions no record names, is split evenly among them and reported as `split`. Each risk tier's PRs, how many merged, how many a reviewer raised, and how many a later PR's `## Caused by` names -show whether the tiers predict what breaks. Over the merged +show whether the tiers predict what breaks. Each breakage a merged PR in the +window names under `## Caused by` follows on its own line, by culprit, even one +older than the window, which is read from GitHub: the culprit's tier, size, +writers, and local reviewers, who found it when the bullet ends with +`(found by a check)`, or a reviewer, the queue, a canary, a person, or a +customer, and how long after the culprit merged the fix opened. Over the merged PRs come each online reviewer's coverage, final heads reviewed, findings per PR reviewed, and answers. A hand-back counts only while its draft is open, so a merged PR @@ -1757,7 +1762,9 @@ removes it. While no sender runs, stops still queue, and the next stop says so once per outage. Each pass runs token-max on this host from the earliest queued session's first -instant, reads the PRs updated since then, and posts `{"records": [...]}` with +instant, reads the PRs updated since then, and posts +`{"records": [...], "breakages": [...]}`, the breakages being those the merged +PRs among them name under `## Caused by`, as `quest usage` reports them, with `Authorization: Bearer `, one upload per endpoint and repository, each queued session with its sub-agents, which stop without the hook naming them. Each upload is checked with `parseUpload` before it is posted. A 2xx drops the @@ -1768,6 +1775,24 @@ endpoint and repository back off, waiting a minute and doubling up to six hours, and sit out the passes meanwhile, so they never widen the others' window. token-max missing is retried each pass. +A deploy or alert reports what it found with `quest records incident`, which +posts one breakage at once to the checkout's endpoint and fails loud on any +answer but a 2xx, since nothing retries it: + +```console +$ quest records incident https://alerts.example/1 --found canary +reported https://alerts.example/1 open, its culprit unknown +$ quest records incident https://alerts.example/1 --found canary --pr 42 +reported https://alerts.example/1 against #42 +``` + +The ID is the reporter's, and a report with the same ID replaces the last, so +an incident stays open until it is reported again with `--pr`, and the store +counts open ones apart. `--found` is one of `check`, `reviewer`, `queue`, +`canary`, `person`, or `customer`, and `--at` sets when it was found, by +default now. Reverts and red default-branch runs are not reported here: the +store reads them from GitHub itself. + ### Typed messages `quest messages` prints the messages a person typed to agents on this host diff --git a/quest/m0/benchmark/breakage.md b/quest/m0/benchmark/breakage.md deleted file mode 100644 index e7602646..00000000 --- a/quest/m0/benchmark/breakage.md +++ /dev/null @@ -1,53 +0,0 @@ -# [M] Record what each merged PR broke - -## Goal - -Every breakage is recorded against the PR that caused it, with that PR's -tier, size, models, and the guardrails it passed, so the report answers -what broke, which layer should have caught it, and whether more or less -should have been spent on the culprit. A breakage is any of: - -- a later PR naming it under `## Caused by`, which `quest usage` counts - today; -- a revert of it; -- a merge queue entry or default-branch run it turned red; -- an incident a repository reports against it, such as a deploy rolled - back or a page, through a command a deploy or alert can call. - -Each breakage records how it was found (a check, a reviewer, the queue, a -canary, a person, a customer) and how long after the merge, since a -breakage a person finds costs more than one a check finds. - -## Plan - -Decided 2026-11-13 (user, /quest-plan): "keep stats about what we're -shipping to recalibrate. Like attributing risk/cost/effort/model and -identifying what broke. Then we can audit if more or fewer guardrails are -needed." - -[Routing evidence](/quest/m0/routing-evidence/README.md) left runtime -escapes to each repository's gate quests. That stands for fixing them; -this quest only records them. - -Agent's choices, for the implementer to revisit: - -- Reverts and red runs come from the forge, which Kixel Code's index - already reads; send what only `quest` knows, the `## Caused by` links - and reported incidents, as records. -- An incident whose culprit is unknown is recorded open, and the fix's - `## Caused by` closes it. Count open ones separately so they are never - read as zero. -- Fix-forward counts the same as a revert. Augment's write-up warns that - revert rate alone undercounts: "many defects are fixed forward or - discovered later." -- Report a rate per class of change and per tier with its sample size, and - no threshold: the maintainer decides what a rise means. -- Keep names of customers and private repositories out of anything this - public repository's records or PRs show. - -## Related - -- [Mistake audit](/quest/m0/mistake-audit.md) - classifies each escape as - catchable or runtime every week -- [Reviewer test set](/quest/m0/benchmark/reviewer-eval.md) - replays these - culprits against the reviewer diff --git a/quest/m0/benchmark/guardrails.md b/quest/m0/benchmark/guardrails.md index b6844bb2..f344f686 100644 --- a/quest/m0/benchmark/guardrails.md +++ b/quest/m0/benchmark/guardrails.md @@ -79,7 +79,5 @@ automatic merging. ## Related -- [Breakage](/quest/m0/benchmark/breakage.md) - the culprit counts each - class is judged on - [Routing evidence](/quest/m0/routing-evidence/README.md) - its shadow reviewer measures what a deeper review buys diff --git a/quest/m0/benchmark/reviewer-eval.md b/quest/m0/benchmark/reviewer-eval.md index 13f8e177..d0905e9a 100644 --- a/quest/m0/benchmark/reviewer-eval.md +++ b/quest/m0/benchmark/reviewer-eval.md @@ -34,11 +34,6 @@ Agent's choices, for the implementer to revisit: - Report recall by tier and size, so a reviewer effort can be cut where it finds nothing more. -## Required - -- [Record what each merged PR broke](/quest/m0/benchmark/breakage.md) - the - culprits with their cause and how they were found - ## Related - [Model check](/quest/m1/model-check.md) - retunes the reviewer table diff --git a/quest/m0/benchmark/risk-tier.md b/quest/m0/benchmark/risk-tier.md index 8fdd33fc..a278fc8c 100644 --- a/quest/m0/benchmark/risk-tier.md +++ b/quest/m0/benchmark/risk-tier.md @@ -33,5 +33,3 @@ covers only the repositories that have one. - [Development as a benchmark](/quest/m0/benchmark/README.md) - the decisions this builds on -- [Breakage](/quest/m0/benchmark/breakage.md) - records what each tier's - PRs later broke diff --git a/src/cli/breakage.ts b/src/cli/breakage.ts new file mode 100644 index 00000000..3c3ec380 --- /dev/null +++ b/src/cli/breakage.ts @@ -0,0 +1,77 @@ +// What a PR body says about breakage: the culprits its `## Caused by` names, and, for a culprit, how it was made and +// checked, as its own body records it. +import { type Culprit, FINDERS, type Finder, SIZES, type Seat, type Size, recordedRisk } from "../core"; +import type { Repository } from "../github/client"; +import { recorded } from "./models-record"; +import { parseReview } from "./review-record"; +import type { Pull } from "./usage"; + +/** One culprit a `## Caused by` section names, and who or what found the breakage when its line says. */ +export interface Cause { + number: number; + found: Finder | null; +} + +/** + * The PRs of `repository` that `body`'s `## Caused by` section names as culprits: each bullet that opens with + * `[owner/repo#N](url) - why`. A link anywhere else on a line, as context or a "fixed by" aside, names no culprit. + * A line ending its why with `(found by a customer)`, or any finder, says who found it; a culprit named twice keeps + * its first line. + */ +export function culprits(body: string, repository: Repository): Cause[] { + const heading = /^## Caused by[ \t]*\r?$/m.exec(body); + if (!heading) return []; + const rest = body.slice(heading.index + heading[0].length); + const section = rest.slice(0, /^## /m.exec(rest)?.index ?? rest.length); + const escape = (text: string) => text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const name = `${escape(repository.owner)}/${escape(repository.repo)}`; + const culprit = new RegExp( + `^- \\[${name}#(\\d+)\\]\\(https://github\\.com/${name}/pull/(\\d+)\\) - \\S[^\\r\\n]*`, + "gim", + ); + const finder = new RegExp(`\\(found by (?:an? |the )?(${FINDERS.join("|")})\\)`, "i"); + const found = new Map(); + for (const match of section.matchAll(culprit)) { + if (match[1] !== match[2] || found.has(Number(match[1]))) continue; + found.set(Number(match[1]), (finder.exec(match[0])?.[1]?.toLowerCase() as Finder | undefined) ?? null); + } + return [...found].map(([number, by]) => ({ number, found: by })); +} + +/** + * `pull` as a culprit. Its body is its author's text, so a record this version cannot read leaves the field empty + * rather than stopping the records of every other PR. + */ +export function culpritOf(pull: Pull): Culprit { + const own = recorded(pull.body); + let tier: Culprit["tier"] = null; + let reviewers: Seat[] = []; + try { + tier = recordedRisk(pull.body)?.tier ?? null; + } catch { + // Malformed: no tier. + } + try { + reviewers = parseReview(pull.body); + } catch { + // Malformed: no reviewers. + } + return { + number: pull.number, + merged: pull.state === "merged" && pull.closed !== null ? new Date(pull.closed).toISOString() : null, + tier, + size: (own.sizes.find((s) => (SIZES as readonly string[]).includes(s)) as Size | undefined) ?? null, + writers: distinct(own.implementers.flatMap(({ fields }) => (fields ? [fields] : []))), + reviewers: distinct(reviewers), + }; +} + +/** Each seat once, in first-named order, as several Implementer lines or a shadow may repeat one. */ +function distinct(seats: Seat[]): Seat[] { + const seen = new Map(); + for (const { harness, model, effort } of seats) { + const id = JSON.stringify([harness, model, effort]); + if (!seen.has(id)) seen.set(id, { harness, model, effort }); + } + return [...seen.values()]; +} diff --git a/src/cli/main.ts b/src/cli/main.ts index a15bda78..c7a8ee05 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -21,6 +21,8 @@ import { questBranch, RISK_FILE, renderBlocker, + FINDERS, + type Finder, SIZES, type Size, unblocked, @@ -84,7 +86,7 @@ import { import { CHECKED_CONTEXT, USER_ASKED, questRoot } from "./rules"; import { messageLines, messages } from "./messages"; import { logWait, pullWaitKind, readWaits } from "./waits"; -import { CHECKOUT_CONFIG, SEND_INTERVAL, background, runSender, tokensPath } from "./send"; +import { CHECKOUT_CONFIG, SEND_INTERVAL, background, reportIncident, runSender, tokensPath } from "./send"; import { installSender, uninstallSender } from "./service"; import { heal, init, stubFindings, uninstall } from "./setup"; import { Refusal, describe, notUpstream, releaseQuest, startQuest } from "./start"; @@ -1195,7 +1197,7 @@ export function main(argv: string[], out: Output): number | Promise { "declines it. Rollups follow: spend by size, role, harness, model, and " + "effort, accuracy by size and implementer, summing each branch's tallies but the shadow's once, where an Implementer line's own size, as (iterate, XS), wins " + "over the PR's, with its writing cost over the merged PRs it was found on, the writer A/B by size and arm, counting a sized PR whose every Implementer line names the same arm, as (arm B), under its first line's writer with the spend of every session on it, local reviewers by size, harness, model, effort, and shadow, with their " + - "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, and each online reviewer's coverage and findings over the merged PRs. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + + "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, each online reviewer's coverage and findings over the merged PRs, and each breakage a merged PR in the window names under ## Caused by: its culprit's tier, size, writers, and local reviewers, read from GitHub when older than the window, who found it when the line ends with (found by a check), or a reviewer, the queue, a canary, a person, or a customer, and how long after the culprit merged the fix opened. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + "--json prints the same report as one object.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 0 when done, 2 for invalid arguments, and 1 for " + "other failures.", @@ -1234,7 +1236,7 @@ export function main(argv: string[], out: Output): number | Promise { .summary("Run the user's record sender until stopped, as its service does") .description( "Send each session record the stop hook queued, with the records of its sub-agents on this host, to the " + - `endpoint its checkout's ${CHECKOUT_CONFIG} names as records, posting {"records": [...]} with the token ` + + `endpoint its checkout's ${CHECKOUT_CONFIG} names as records, posting {"records": [...], "breakages": [...]}, the breakages the merged PRs among them name under ## Caused by, with the token ` + `${tokensPath()} pairs with that endpoint as a bearer token, one upload per endpoint and repository, ` + `then again every ${SEND_INTERVAL / 1000} seconds until stopped, at low priority. A record is built as quest usage ` + "builds it, from token-max's sessions on this host since the earliest queued session's first instant and " + @@ -1263,6 +1265,40 @@ export function main(argv: string[], out: Output): number | Promise { () => 1, ); }); + recordsCommand + .command("incident") + .summary("Report a breakage a deploy or alert found, open until its culprit is named") + .argument( + "", + "A stable ID for the incident, such as the alert's URL; a later report with it replaces this one", + ) + .description( + "Post one breakage to the endpoint this checkout's " + + `${CHECKOUT_CONFIG} names, with the token ${tokensPath()} pairs with it, as {"records": [], "breakages": [...]}. ` + + "Without --pr the incident is open, its culprit unknown; report the same ID again with --pr to close it on " + + "its culprit, whose tier, size, writers, and local reviewers are read from its PR body. It posts at once " + + "and retries nothing, for a deploy or alert to call.\n\nUses GH_TOKEN, GITHUB_TOKEN, or gh auth token. " + + "Exits 0 when the endpoint took it, 2 for invalid arguments, and 1 for other failures, refusal included.", + ) + .addOption(new Option("--found ", "Who or what found it").choices(FINDERS).makeOptionMandatory()) + .option("--pr ", "The PR that caused it", positiveInteger("--pr")) + .option("--at ", "When it was found, as an ISO 8601 instant (default: now)") + .action((id: string, options: { found: Finder; pr?: number; at?: string }) => { + if (/[\r\n]/.test(id) || id.trim() === "") + throw new UsageError("the incident ID must be one non-empty line"); + const at = options.at === undefined ? Date.now() : Date.parse(options.at); + if (Number.isNaN(at)) throw new UsageError(`--at ${options.at} is not an ISO 8601 instant`); + settle( + reportIncident( + { cwd: root(), id, found: options.found, pull: options.pr ?? null, at }, + { env: process.env, github: () => gitHubDependencies(warn), fetch: globalThis.fetch }, + ), + (breakage) => + print( + `reported ${breakage.incident} ${breakage.culprit ? `against #${breakage.culprit.number}` : "open, its culprit unknown"}`, + ), + ); + }); const serviceDeps = () => ({ platform: process.platform, env: process.env, diff --git a/src/cli/send.ts b/src/cli/send.ts index f65fdfff..06471dff 100644 --- a/src/cli/send.ts +++ b/src/cli/send.ts @@ -1,4 +1,4 @@ -// Sending each session's record to the Kixel Code endpoint its checkout names. The stop hook only queues the session; +// Sending each session's record, and the breakages its PRs name, to the Kixel Code endpoint its checkout names. The stop hook only queues the session; // one sender per user, run by its service manager, rebuilds each queued session's record, with its sub-agents', and // posts them, keeping what failed for the next pass. A record replaces its earlier copy, so a resend never doubles. import { spawnSync } from "node:child_process"; @@ -7,14 +7,15 @@ import { homedir, setPriority } from "node:os"; import { dirname, join } from "node:path"; import { createHash, randomUUID } from "node:crypto"; -import { type SessionRecord, parseUpload } from "../core"; +import { type BreakageRecord, type Finder, type RecordUpload, parseUpload } from "../core"; +import { culpritOf } from "./breakage"; import { type GitHubDependencies, GitHub } from "../github/client"; import { baseRepository } from "./claims"; import { discoveryEnv } from "./git"; import { readRemote, remoteRepository } from "./github"; import { eachLine } from "./messages"; import { questRoot } from "./rules"; -import { type Pull, type Session, fold, key, sessionRecords, updatedPulls } from "./usage"; +import { type Pull, type Session, fixBreakages, fold, key, readPull, sessionRecords, updatedPulls } from "./usage"; import { type LoggedWait } from "./waits"; /** The checkout's file naming its record endpoint, as `{"records": "https://..."}`. */ @@ -251,6 +252,7 @@ export interface Sent { endpoint: string; repository: string; records: number; + breakages: number; /** The endpoint refused the token, so the records were dropped. */ refused: boolean; } @@ -293,7 +295,7 @@ export async function runSender(deps: SendDependencies, log: (line: string) => v log( each.refused ? `${each.endpoint} refused the token for ${each.repository}; ${each.records} records dropped` - : `sent ${each.records} records of ${each.repository} to ${each.endpoint}`, + : `sent ${each.records} records and ${each.breakages} breakages of ${each.repository} to ${each.endpoint}`, ); } catch (error) { log(error instanceof Error ? error.message : String(error)); @@ -431,17 +433,11 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { emit, since, }); - const upload = [...records.values()]; - try { - parseUpload({ records: upload }); - } catch (error) { - throw new Error(`this quest built an upload its own parseUpload refuses: ${(error as Error).message}`); - } - const response = await deps.fetch(endpoint, { - method: "POST", - headers: { authorization: `Bearer ${token}`, "content-type": "application/json" }, - body: JSON.stringify({ records: upload } satisfies { records: SessionRecord[] }), - }); + const upload: RecordUpload = { + records: [...records.values()], + breakages: await fixBreakages(client, repository, pulls), + }; + const response = await post(deps.fetch, endpoint, token, upload); if (response.status === 401 || response.status === 403) { refuse(deps.env, endpoint, token); clearBackoff(deps.env, id); @@ -449,14 +445,26 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { notes.push( `${endpoint} refused this host's token (${response.status}), so it is sent no more session records until the token changes`, ); - sent.push({ endpoint, repository: key(repository), records: upload.length, refused: true }); + sent.push({ + endpoint, + repository: key(repository), + records: upload.records.length, + breakages: upload.breakages.length, + refused: true, + }); continue; } if (!response.ok) throw new Error(`${endpoint} answered ${response.status}: ${(await response.text()).slice(0, 500)}`); clearBackoff(deps.env, id); for (const q of group) settle(q); - sent.push({ endpoint, repository: key(repository), records: upload.length, refused: false }); + sent.push({ + endpoint, + repository: key(repository), + records: upload.records.length, + breakages: upload.breakages.length, + refused: false, + }); } catch (error) { const wait = backOff(deps, id); failures.push( @@ -470,6 +478,82 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { return sent; } +/** An incident to report, as a deploy or alert hands it to `quest records incident`. */ +export interface Incident { + /** A directory in the checkout whose endpoint and repository it is reported for. */ + cwd: string; + /** The reporter's stable ID for it, such as the alert's URL; reporting the same ID again replaces it. */ + id: string; + found: Finder; + /** The PR that caused it, or null while unknown, which leaves it open. */ + pull: number | null; + /** When it was found, epoch ms. */ + at: number; +} + +/** + * Post `incident` as a breakage to the endpoint the checkout's `${CHECKOUT_CONFIG}` names, with the token the user + * paired with it, and return it. Unlike the sender, it posts at once and fails loud on anything the endpoint does not + * take, refusal included, since a deploy or alert calls it once and nothing retries. + */ +export async function reportIncident( + incident: Incident, + deps: Pick, +): Promise { + const root = questRoot(incident.cwd); + if (root === null) throw new Error(`${incident.cwd} is not in a checkout that uses Quest`); + const endpoint = checkoutEndpoint(root); + if (endpoint === null) throw new Error(`${join(root, CHECKOUT_CONFIG)} names no record endpoint`); + const token = tokenFor(endpoint, deps.env); + if (token === null) throw new Error(`${tokensPath(deps.env)} holds no token for ${endpoint}`); + const remote = readRemote(root, "origin"); + const origin = remote === null ? null : remoteRepository(remote).repository; + if (origin === null) throw new Error("origin is not on GitHub"); + const client = new GitHub(deps.github()); + const prefix = spawnSync("git", ["-C", root, "rev-parse", "--show-prefix"], { + encoding: "utf8", + env: discoveryEnv(), + }).stdout.trim(); + const repository = await baseRepository(client, origin, prefix); + let culprit: BreakageRecord["culprit"] = null; + if (incident.pull !== null) { + const found = await readPull(client, repository, incident.pull); + if (found === null) throw new Error(`${key(repository)} has no PR #${incident.pull}`); + culprit = culpritOf(found); + } + const breakage: BreakageRecord = { + repository: key(repository), + culprit, + fix: null, + incident: incident.id, + found: incident.found, + at: new Date(incident.at).toISOString(), + }; + const response = await post(deps.fetch, endpoint, token, { records: [], breakages: [breakage] }); + if (!response.ok) + throw new Error(`${endpoint} answered ${response.status}: ${(await response.text()).slice(0, 500)}`); + return breakage; +} + +/** POST `upload` to `endpoint` with `token`, after checking it is one the store's own `parseUpload` takes. */ +async function post( + fetch: typeof globalThis.fetch, + endpoint: string, + token: string, + upload: RecordUpload, +): Promise { + try { + parseUpload(upload); + } catch (error) { + throw new Error(`this quest built an upload its own parseUpload refuses: ${(error as Error).message}`); + } + return fetch(endpoint, { + method: "POST", + headers: { authorization: `Bearer ${token}`, "content-type": "application/json" }, + body: JSON.stringify(upload), + }); +} + /** The longest a failing group waits between tries. */ const MAX_BACKOFF = 6 * 3_600_000; diff --git a/src/cli/usage.ts b/src/cli/usage.ts index 3c433b92..d78864fe 100644 --- a/src/cli/usage.ts +++ b/src/cli/usage.ts @@ -8,10 +8,11 @@ import { recorded } from "./models-record"; import { type ClaimDependencies, upstream } from "./claims"; import { repositoryPath } from "../github/client"; import { git } from "./git"; +import { culpritOf, culprits } from "./breakage"; import { fixes } from "./land-fixer"; import { MERGER } from "./land-watch-fix"; import { githubRepository, requireRemote } from "./github"; -import { type GitHub, type ObjectValue, type Repository, link } from "../github/client"; +import { type GitHub, type ObjectValue, type Repository, link, object } from "../github/client"; import { type OnlineReview, readOnlineReviews } from "../github/online-review"; import { type Counts, @@ -23,7 +24,7 @@ import { severityText, } from "./review-record"; import { SEVERITIES } from "./review"; -import { ROLES, type Role, type SessionRecord, WAIT_KINDS } from "../core"; +import { type BreakageRecord, ROLES, type Role, type Seat, type SessionRecord, WAIT_KINDS } from "../core"; import { buildRecords } from "./records"; import { type LoggedWait, readWaits } from "./waits"; @@ -210,6 +211,11 @@ export interface Usage { arms: ArmRow[]; tiers: TierRow[]; online: OnlineRow[]; + /** + * What the merged PRs in the window name under `## Caused by`, with each culprit as its body records it, wherever + * it falls; narrowed, those whose culprit or fix is reported. Incidents reach only the store. + */ + breakages: BreakageRecord[]; /** The sessions on no PR reported here, from every repository and agent token-max reads; null when narrowed. */ unattributed: ({ sessions: number } & Tokens) | null; /** Copies of a session found on several hosts, left out for its costliest copy, and the hosts they came from. */ @@ -402,20 +408,39 @@ export async function updatedPulls(client: GitHub, repository: Repository, since return pulls; } +/** The PR numbers of `repository` that `body`'s `## Caused by` section names as culprits, as {@link culprits} reads them. */ +export const causes = (body: string, repository: Repository): number[] => + culprits(body, repository).map((cause) => cause.number); + +/** PR `number` of `repository`, or null when GitHub has none, as for a mistyped `## Caused by` link. */ +export async function readPull(client: GitHub, repository: Repository, number: number): Promise { + const response = await client.request(`${repositoryPath(repository)}/pulls/${number}`, { allow: [404] }); + return response.status === 404 ? null : pull(object(await response.json())); +} + /** - * The PR numbers of `repository` that `body`'s `## Caused by` section names as culprits: each bullet that opens with - * `[owner/repo#N](url) - why`. A link anywhere else on a line, as context or a "fixed by" aside, names no culprit. + * A breakage for each culprit the merged PRs among `pulls` name under `## Caused by`, found when the fix opened. A + * culprit outside `pulls` is read from GitHub, once each; a link to no PR names no culprit. */ -export function causes(body: string, repository: Repository): number[] { - const heading = /^## Caused by[ \t]*\r?$/m.exec(body); - if (!heading) return []; - const rest = body.slice(heading.index + heading[0].length); - const section = rest.slice(0, /^## /m.exec(rest)?.index ?? rest.length); - const escape = (text: string) => text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); - const name = `${escape(repository.owner)}/${escape(repository.repo)}`; - const culprit = new RegExp(`^- \\[${name}#(\\d+)\\]\\(https://github\\.com/${name}/pull/(\\d+)\\) - \\S`, "gim"); - const found = [...section.matchAll(culprit)].filter((match) => match[1] === match[2]); - return [...new Set(found.map((match) => Number(match[1])))]; +export async function fixBreakages(client: GitHub, repository: Repository, pulls: Pull[]): Promise { + const known = new Map(pulls.map((each) => [each.number, each])); + const breakages: BreakageRecord[] = []; + for (const fix of pulls.filter((each) => each.state === "merged")) + for (const cause of culprits(fix.body, repository)) { + if (cause.number === fix.number) continue; + if (!known.has(cause.number)) known.set(cause.number, await readPull(client, repository, cause.number)); + const culprit = known.get(cause.number); + if (!culprit) continue; + breakages.push({ + repository: key(repository), + culprit: culpritOf(culprit), + fix: fix.number, + incident: null, + found: cause.found, + at: new Date(fix.created).toISOString(), + }); + } + return breakages; } const sized = (value: string | null | undefined): Size | null => @@ -1041,6 +1066,10 @@ export async function usage(root: string, options: UsageOptions, deps: UsageDepe // `quest ready` and hand edits drop the section before merge, so an earlier body counts too. if (read?.bodies.some(handedBack)) row.handedBack = true; } + const reported = new Set(rows.map((row) => row.number)); + const breakages = (await fixBreakages(client, repository, all)).filter( + (each) => !narrowed || reported.has(each.fix!) || reported.has(each.culprit!.number), + ); return { repository: key(repository), since: options.since, @@ -1052,6 +1081,7 @@ export async function usage(root: string, options: UsageOptions, deps: UsageDepe arms: armRows(rows), tiers: tierRows(rows), online: onlineRows(rows), + breakages, unattributed, folded: { copies: folded.length, hosts: [...new Set(folded.map((session) => session.host))] }, incomplete: incomplete.map((session) => ({ @@ -1083,6 +1113,27 @@ const percent = (part: number, whole: number) => (whole ? `${Math.round((100 * p const answers = (r: Omit) => `${r.findings} finding${r.findings === 1 ? "" : "s"}: ${r.accepted} accepted, ${r.declined} declined, ${r.unclassified} unclassified, ${r.unanswered} unanswered`; +const seat = ({ model, effort }: Seat) => `${model} ${effort}`; + +/** A fix's breakage: its culprit's tier, size, writers, and reviewers, then who found it, and when after the merge. */ +function breakageText(b: BreakageRecord): string { + const c = b.culprit!; + const made = [ + c.tier ?? "no tier", + c.size ?? "unsized", + c.writers.length ? `written by ${c.writers.map(seat).join(", ")}` : "no writer recorded", + c.reviewers.length ? `reviewed by ${c.reviewers.map(seat).join(", ")}` : "no local review", + ].join(", "); + const ms = c.merged === null ? null : Date.parse(b.at) - Date.parse(c.merged); + const after = + ms === null + ? "never merged" + : ms < 0 + ? "opened before it merged" + : `${ms >= 172_800_000 ? `${(ms / 86_400_000).toFixed(1)}d` : duration(ms)} after merge`; + return `${made}; fixed by #${b.fix}, ${b.found ? `found by ${b.found}` : "finder unrecorded"}, ${after}`; +} + /** Seconds under a minute, hours past two, else minutes. */ const duration = (ms: number) => ms < 60_000 @@ -1249,6 +1300,12 @@ export function usageLines(report: Usage): string[] { ]), ), ); + if (report.breakages.length) + lines.push( + "", + "Breakages, by the merged fix naming its culprit under ## Caused by:", + ...table(report.breakages.map((b) => [" ", `#${b.culprit!.number}`, breakageText(b)])), + ); const u = report.unattributed; if (u) lines.push("", `Unattributed: ${u.sessions} sessions on no PR here, any repository or agent: ${tokens(u)}`); const host = (name: string | null) => name ?? "local"; diff --git a/src/core/breakage.ts b/src/core/breakage.ts new file mode 100644 index 00000000..919f27d4 --- /dev/null +++ b/src/core/breakage.ts @@ -0,0 +1,102 @@ +// A breakage: what a merged PR broke, as `quest` sends it to Kixel Code beside the session records. It comes from a +// later PR naming the culprit under `## Caused by`, or from an incident a deploy or alert reports. Reverts and red +// default-branch runs are left to the store, which reads them from the forge itself. +import { SIZES, type Size } from "./doc"; +import { parser } from "./parse"; +import { TIERS, type Tier } from "./risk"; + +/** Who or what found a breakage, cheapest first: a person or customer finding it costs more than a check. */ +export const FINDERS = ["check", "reviewer", "queue", "canary", "person", "customer"] as const; +export type Finder = (typeof FINDERS)[number]; + +/** One writer or reviewer seat as the culprit's PR body records it. */ +export interface Seat { + harness: string; + model: string; + effort: string; +} + +/** The PR that caused a breakage, with what its body records of how it was made and checked. */ +export interface Culprit { + number: number; + /** When it merged, ISO 8601 UTC; null when it never did, as a closed PR whose commits landed elsewhere. */ + merged: string | null; + /** Its risk tier, from its `## Review` record; null when it records none or one this version cannot read. */ + tier: Tier | null; + /** Its size, from its `## Models` record. */ + size: Size | null; + /** The writers its `Implementer:` lines name. */ + writers: Seat[]; + /** The local reviewers its `## Review` record names, the guardrail it passed. */ + reviewers: Seat[]; +} + +/** + * One breakage. A fix's is keyed by its repository, culprit, and fix, and an incident's by its repository and ID, so a + * resend replaces it: reporting an open incident again with its culprit closes it. + */ +export interface BreakageRecord { + /** The culprit's repository as lowercase `owner/repo`. */ + repository: string; + /** Null only for an incident whose culprit is not known yet. */ + culprit: Culprit | null; + /** The PR naming the culprit under `## Caused by`, or null for an incident. */ + fix: number | null; + /** The ID its reporter gave an incident, such as the alert's URL, or null for a fix. */ + incident: string | null; + /** Null when the fix's `## Caused by` line does not say. */ + found: Finder | null; + /** When it was found, ISO 8601 UTC: the fix PR's opening, or the incident's report. */ + at: string; +} + +const { fail, isObject, text, maybe, instant, oneOf, exact, pull, repository } = parser("breakage record"); + +const seat = (value: unknown): Seat => { + if (!isObject(value)) return fail("a seat"); + exact(value, ["harness", "model", "effort"], "a seat"); + return { + harness: text(value.harness, "a seat's harness"), + model: text(value.model, "a seat's model"), + effort: text(value.effort, "a seat's effort"), + }; +}; + +const seats = (value: unknown, what: string): Seat[] => (Array.isArray(value) ? value.map(seat) : fail(what)); + +function culprit(value: unknown): Culprit { + if (!isObject(value)) return fail("the culprit"); + exact(value, ["number", "merged", "tier", "size", "writers", "reviewers"], "the culprit"); + return { + number: pull(value.number, "the culprit's number"), + merged: value.merged === null ? null : instant(value.merged, "the culprit's merge"), + tier: value.tier === null ? null : oneOf(value.tier, TIERS, "the culprit's tier"), + size: value.size === null ? null : oneOf(value.size, SIZES, "the culprit's size"), + writers: seats(value.writers, "the culprit's writers"), + reviewers: seats(value.reviewers, "the culprit's reviewers"), + }; +} + +/** `value` as a breakage, failing on any missing, extra, or ill-typed field, or one that is neither fix nor incident. */ +export function parseBreakage(value: unknown): BreakageRecord { + if (!isObject(value)) return fail("not an object"); + exact(value, ["repository", "culprit", "fix", "incident", "found", "at"], "the breakage"); + const incident = maybe(value.incident, "the incident"); + if (incident !== null && /[\r\n]/.test(incident)) fail("the incident spans lines"); + const record: BreakageRecord = { + repository: repository(value.repository, "repository"), + culprit: value.culprit === null ? null : culprit(value.culprit), + fix: value.fix === null ? null : pull(value.fix, "the fix"), + incident, + found: value.found === null ? null : oneOf(value.found, FINDERS, "how it was found"), + at: instant(value.at, "when it was found"), + }; + if ((record.fix === null) === (record.incident === null)) fail("it names neither or both of a fix and an incident"); + if (record.fix !== null && record.culprit === null) fail("a fix names no culprit"); + if (record.fix !== null && record.fix === record.culprit?.number) fail("a PR fixes itself"); + return record; +} + +/** The identity a store replaces a breakage by. */ +export const breakageKey = (b: BreakageRecord): string => + JSON.stringify(b.incident === null ? [b.repository, b.culprit?.number, b.fix] : [b.repository, b.incident]); diff --git a/src/core/index.ts b/src/core/index.ts index cd18aee5..7b519732 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -94,6 +94,15 @@ export { latestHold, heldToken, } from "./hold"; +export { + type BreakageRecord, + type Culprit, + FINDERS, + type Finder, + type Seat, + breakageKey, + parseBreakage, +} from "./breakage"; export { type ModelSpend, type RecordUpload, diff --git a/src/core/parse.ts b/src/core/parse.ts new file mode 100644 index 00000000..585efad6 --- /dev/null +++ b/src/core/parse.ts @@ -0,0 +1,33 @@ +// Strict field checks for the records `quest` sends, so a store never keeps one in part. + +/** Checks that throw `malformed : ` on any missing, extra, or ill-typed field. */ +export function parser(what: string) { + const fail = (field: string): never => { + throw new Error(`malformed ${what}: ${field}`); + }; + const isObject = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + const text = (value: unknown, field: string): string => + typeof value === "string" && value !== "" ? value : fail(field); + const maybe = (value: unknown, field: string): string | null => (value === null ? null : text(value, field)); + const count = (value: unknown, field: string): number => + typeof value === "number" && Number.isFinite(value) && value >= 0 ? value : fail(field); + const flag = (value: unknown, field: string): boolean => (typeof value === "boolean" ? value : fail(field)); + const instant = (value: unknown, field: string): string => { + const at = text(value, field); + return /^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(\.\d+)?Z$/.test(at) && !Number.isNaN(Date.parse(at)) ? at : fail(field); + }; + const oneOf = (value: unknown, options: readonly T[], field: string): T => + options.includes(value as T) ? (value as T) : fail(field); + const exact = (value: Record, keys: string[], field: string) => { + const extra = Object.keys(value).find((key) => !keys.includes(key)); + if (extra !== undefined) fail(`${field} has an unknown field ${extra}`); + }; + const pull = (value: unknown, field: string): number => + Number.isSafeInteger(value) && (value as number) > 0 ? (value as number) : fail(field); + const repository = (value: unknown, field: string): string => { + const name = text(value, field); + return /^[a-z\d._-]+\/[a-z\d._-]+$/.test(name) ? name : fail(`${field} is not a lowercase owner/repo`); + }; + return { fail, isObject, text, maybe, count, flag, instant, oneOf, exact, pull, repository }; +} diff --git a/src/core/record.ts b/src/core/record.ts index 73523210..6a56e421 100644 --- a/src/core/record.ts +++ b/src/core/record.ts @@ -1,3 +1,6 @@ +import { type BreakageRecord, breakageKey, parseBreakage } from "./breakage"; +import { parser } from "./parse"; + // The record of one agent session that `quest` sends to Kixel Code, which stores exactly this shape, and that // `quest usage` prints. One session is one record: a resend under the same harness and session ID replaces it. A // host's one sender uploads a session's copies in order, so the latest upload is the newest copy. @@ -77,31 +80,25 @@ export interface SessionRecord { waits: SessionWait[] | null; } -/** The body of a record upload. */ +/** The body of a record upload: sessions' records, and the breakages the uploading host knows of. */ export interface RecordUpload { records: SessionRecord[]; + breakages: BreakageRecord[]; } -const fail = (what: string): never => { - throw new Error(`malformed session record: ${what}`); -}; -const isObject = (value: unknown): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); -const text = (value: unknown, what: string): string => (typeof value === "string" && value !== "" ? value : fail(what)); -const maybe = (value: unknown, what: string): string | null => (value === null ? null : text(value, what)); -const count = (value: unknown, what: string): number => - typeof value === "number" && Number.isFinite(value) && value >= 0 ? value : fail(what); -const flag = (value: unknown, what: string): boolean => (typeof value === "boolean" ? value : fail(what)); -const instant = (value: unknown, what: string): string => { - const at = text(value, what); - return /^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(\.\d+)?Z$/.test(at) && !Number.isNaN(Date.parse(at)) ? at : fail(what); -}; -const oneOf = (value: unknown, options: readonly T[], what: string): T => - options.includes(value as T) ? (value as T) : fail(what); -const exact = (value: Record, keys: string[], what: string) => { - const extra = Object.keys(value).find((key) => !keys.includes(key)); - if (extra !== undefined) fail(`${what} has an unknown field ${extra}`); -}; +const { + fail, + isObject, + text, + maybe, + count, + flag, + instant, + oneOf, + exact, + pull, + repository: ownerRepo, +} = parser("session record"); /** `value` as a record, failing on any missing, extra, or ill-typed field, so a store never keeps one in part. */ export function parseRecord(value: unknown): SessionRecord { @@ -126,8 +123,7 @@ export function parseRecord(value: unknown): SessionRecord { const pulls = Array.isArray(value.pulls) ? value.pulls : fail("pulls"); const models = Array.isArray(value.models) ? value.models : fail("models"); const waits = value.waits === null ? null : Array.isArray(value.waits) ? value.waits : fail("waits"); - const repository = text(value.repository, "repository"); - if (!/^[a-z\d._-]+\/[a-z\d._-]+$/.test(repository)) fail("repository is not a lowercase owner/repo"); + const repository = ownerRepo(value.repository, "repository"); const quest = maybe(value.quest, "quest"); if (quest !== null && !/^quest\/.+\.md$/.test(quest)) fail("quest is not a quest path"); const record: SessionRecord = { @@ -137,7 +133,7 @@ export function parseRecord(value: unknown): SessionRecord { repository, branch: maybe(value.branch, "branch"), quest, - pulls: pulls.map((n) => (Number.isSafeInteger(n) && (n as number) > 0 ? (n as number) : fail("a PR number"))), + pulls: pulls.map((n) => pull(n, "a PR number")), role: oneOf(value.role, ROLES, "role"), models: models.map((m: unknown) => { if (!isObject(m)) return fail("a model"); @@ -178,14 +174,20 @@ export function parseRecord(value: unknown): SessionRecord { return record; } -/** An upload's body, failing whole on any malformed record, one naming no session, or a session sent twice. */ +/** + * An upload's body, failing whole on any malformed record, one naming no session, or a session or breakage sent + * twice. + */ export function parseUpload(value: unknown): RecordUpload { if (!isObject(value)) return fail("the upload is not an object"); - exact(value, ["records"], "the upload"); + exact(value, ["records", "breakages"], "the upload"); if (!Array.isArray(value.records)) return fail("the upload has no record list"); + if (!Array.isArray(value.breakages)) return fail("the upload has no breakage list"); + const breakages = value.breakages.map(parseBreakage); + if (new Set(breakages.map(breakageKey)).size !== breakages.length) fail("the upload holds a breakage twice"); const records = value.records.map(parseRecord); if (records.some((r) => r.session === null)) fail("an uploaded record names no session"); const ids = new Set(records.map((r) => `${r.harness}\0${r.session}`)); if (ids.size !== records.length) fail("the upload holds a session twice"); - return { records }; + return { records, breakages }; } diff --git a/test/records.test.ts b/test/records.test.ts index a419bcc2..c887e062 100644 --- a/test/records.test.ts +++ b/test/records.test.ts @@ -3,7 +3,7 @@ import { join } from "node:path"; import { describe, expect, test } from "vitest"; -import { type SessionRecord, parseRecord, parseUpload } from "../src/core"; +import { type BreakageRecord, type SessionRecord, parseBreakage, parseRecord, parseUpload } from "../src/core"; import { buildRecords } from "../src/cli/records"; import { transcriptFacts } from "../src/cli/transcript"; import type { Pull, Session } from "../src/cli/usage"; @@ -97,13 +97,71 @@ describe("parseRecord", () => { }); test("an upload fails whole on one bad record or a session sent twice", () => { - expect(parseUpload({ records: [record(), record({ session: "s2" })] }).records).toHaveLength(2); - expect(() => parseUpload({ records: [record(), record({ session: "s2", cost: Number.NaN })] })).toThrow("cost"); - expect(() => parseUpload({ records: [record(), record()] })).toThrow("twice"); - expect(() => parseUpload({ records: [], more: true })).toThrow("unknown field"); + const upload = (records: unknown[]) => ({ records, breakages: [] }); + expect(parseUpload(upload([record(), record({ session: "s2" })])).records).toHaveLength(2); + expect(() => parseUpload(upload([record(), record({ session: "s2", cost: Number.NaN })]))).toThrow("cost"); + expect(() => parseUpload(upload([record(), record()]))).toThrow("twice"); + expect(() => parseUpload({ ...upload([]), more: true })).toThrow("unknown field"); + expect(() => parseUpload({ records: [] })).toThrow("no breakage list"); // A record may name no session, but no upload carries one, since the store replaces records by session. expect(parseRecord(record({ session: null })).session).toBeNull(); - expect(() => parseUpload({ records: [record({ session: null })] })).toThrow("names no session"); + expect(() => parseUpload(upload([record({ session: null })]))).toThrow("names no session"); + }); +}); + +const breakage = (extra: Partial = {}): BreakageRecord => ({ + repository: "acme/proj", + culprit: { + number: 5, + merged: "2026-11-01T10:00:00.000Z", + tier: "low", + size: "S", + writers: [{ harness: "Claude", model: "claude-opus-5-5", effort: "medium" }], + reviewers: [{ harness: "Codex", model: "gpt-6.1-sol", effort: "high" }], + }, + fix: 9, + incident: null, + found: "customer", + at: "2026-11-03T10:00:00.000Z", + ...extra, +}); + +describe("parseBreakage", () => { + test("keeps a fix's breakage and an open or closed incident exactly", () => { + for (const sent of [ + breakage(), + breakage({ fix: null, incident: "https://alerts.example/1", culprit: null, found: "canary" }), + breakage({ fix: null, incident: "deploy 42", found: null }), + ]) + expect(parseBreakage(JSON.parse(JSON.stringify(sent)))).toEqual(sent); + }); + + test.each([ + ["neither fix nor incident", breakage({ fix: null }), "neither or both"], + ["both fix and incident", breakage({ incident: "x" }), "neither or both"], + ["a fix with no culprit", breakage({ culprit: null }), "names no culprit"], + ["a PR fixing itself", breakage({ fix: 5 }), "fixes itself"], + ["an unknown finder", breakage({ found: "luck" as never }), "how it was found"], + ["an incident spanning lines", breakage({ fix: null, incident: "a\nb" }), "spans lines"], + ["an unknown tier", breakage({ culprit: { ...breakage().culprit!, tier: "severe" as never } }), "tier"], + [ + "an extra culprit field", + breakage({ culprit: { ...breakage().culprit!, cost: 1 } as never }), + "unknown field cost", + ], + ["a mixed-case repository", breakage({ repository: "Acme/proj" }), "lowercase"], + ])("refuses %s", (_, value, message) => { + expect(() => parseBreakage(value)).toThrow(message); + }); + + test("an upload holds each breakage once, a fix's by culprit and fix and an incident's by ID", () => { + const closed = breakage({ fix: null, incident: "i1" }); + const open = breakage({ fix: null, incident: "i1", culprit: null }); + expect( + parseUpload({ records: [], breakages: [breakage(), breakage({ fix: 10 }), closed] }).breakages, + ).toHaveLength(3); + expect(() => parseUpload({ records: [], breakages: [breakage(), breakage()] })).toThrow("breakage twice"); + expect(() => parseUpload({ records: [], breakages: [closed, open] })).toThrow("breakage twice"); }); }); diff --git a/test/send.test.ts b/test/send.test.ts index 7e7b3029..2871ce48 100644 --- a/test/send.test.ts +++ b/test/send.test.ts @@ -6,7 +6,7 @@ import { expect, test } from "vitest"; import { REFERENCE_LINE, parseUpload } from "../src/core"; import { hook } from "../src/cli/hooks"; -import { type SendDependencies, queue, runSender, sendPass, tokensPath } from "../src/cli/send"; +import { type SendDependencies, queue, reportIncident, runSender, sendPass, tokensPath } from "../src/cli/send"; import type { Session } from "../src/cli/usage"; import { tempDir } from "./fixture"; @@ -101,6 +101,7 @@ function sender( status: number | (() => number), sessions: Session[] | null, during: () => void = () => {}, + pulls: object[] = [], ) { const uploads: { token: string; body: unknown }[] = []; const windows: Date[] = []; @@ -119,8 +120,10 @@ function sender( ssh_url: "git@github.com:acme/proj.git", fork: false, }); + if (url.pathname === "/repos/acme/proj/pulls/3") return json(CULPRIT); if (url.pathname === "/repos/acme/proj/pulls") return json([ + ...pulls, { number: 7, html_url: "https://github.com/acme/proj/pull/7", @@ -170,6 +173,21 @@ function sender( return { deps, uploads, windows, slept, clock }; } +/** A merged PR older than any window, which later PRs name as a culprit. */ +const CULPRIT = { + number: 3, + html_url: "https://github.com/acme/proj/pull/3", + title: "C", + head: { ref: "quest/m0/c", repo: { full_name: "acme/proj" } }, + state: "closed", + draft: false, + merged_at: "2026-10-01T00:00:00Z", + created_at: "2026-09-30T00:00:00Z", + updated_at: "2026-10-01T00:00:00Z", + closed_at: "2026-10-01T00:00:00Z", + body: "## Models\n\n- Size: M\n- Implementer: Claude Code, claude-opus-5-5, medium effort\n", +}; + const pending = (env: NodeJS.ProcessEnv) => readdirSync(join(env.XDG_STATE_HOME!, "quest", "records", "pending")); test("the sender posts each queued session with its sub-agents, from its transcript's start", async () => { @@ -182,7 +200,9 @@ test("the sender posts each queued session with its sub-agents, from its transcr session("other", "quest/m0/a"), ]; const { deps, uploads, windows } = sender(env, 200, sessions); - expect(await sendPass(deps)).toEqual([{ endpoint: ENDPOINT, repository: "acme/proj", records: 2, refused: false }]); + expect(await sendPass(deps)).toEqual([ + { endpoint: ENDPOINT, repository: "acme/proj", records: 2, breakages: 0, refused: false }, + ]); expect(windows).toEqual([new Date("2026-11-06T10:00:00Z")]); expect(uploads).toHaveLength(1); expect(uploads[0]!.token).toBe("Bearer secret"); @@ -208,7 +228,7 @@ test("a failed upload stays queued and is said once; a refused token stops sendi const refusing = sender(env, 403, [session("s1", "quest/m0/a")]); expect(await sendPass(refusing.deps)).toEqual([ - { endpoint: ENDPOINT, repository: "acme/proj", records: 1, refused: true }, + { endpoint: ENDPOINT, repository: "acme/proj", records: 1, breakages: 0, refused: true }, ]); expect(pending(env)).toEqual([]); expect(queue(stopped(root), env)).toEqual({ @@ -288,7 +308,9 @@ test("a session token-max no longer lists is dropped and said, without holding b queue(stopped(root), env); queue({ ...stopped(root), session: "gone" }, env); const { deps, uploads } = sender(env, 200, [session("s1", "quest/m0/a")]); - expect(await sendPass(deps)).toEqual([{ endpoint: ENDPOINT, repository: "acme/proj", records: 1, refused: false }]); + expect(await sendPass(deps)).toEqual([ + { endpoint: ENDPOINT, repository: "acme/proj", records: 1, breakages: 0, refused: false }, + ]); expect(parseUpload(uploads[0]!.body).records.map((r) => r.session)).toEqual(["s1"]); expect(pending(env)).toEqual([]); expect(queue(stopped(root), env).notices).toEqual([ @@ -386,7 +408,10 @@ test("the sender passes every interval, records its PID, and outlives a failed p expect(slept).toEqual([60_000, 60_000, 60_000]); // The failed pass keeps the session; the next sends it, and the last finds the queue empty. expect(uploads).toHaveLength(2); - expect(log).toEqual([expect.stringContaining("answered 500"), `sent 1 records of acme/proj to ${ENDPOINT}`]); + expect(log).toEqual([ + expect.stringContaining("answered 500"), + `sent 1 records and 0 breakages of acme/proj to ${ENDPOINT}`, + ]); expect(pending(env)).toEqual([]); }); @@ -440,3 +465,83 @@ test("a queued session for an endpoint that refused its token is dropped unsent" expect(later.uploads).toEqual([]); expect(pending(env)).toEqual([]); }); + +test("the sender sends the breakages a merged fix in its window names, reading an older culprit", async () => { + const env = home(); + queue(stopped(checkout(ENDPOINT)), env); + const fix = { + ...CULPRIT, + number: 8, + head: { ref: "fix/c", repo: { full_name: "acme/proj" } }, + merged_at: "2026-11-06T10:50:00Z", + created_at: "2026-11-06T10:20:00Z", + updated_at: "2026-11-06T10:50:00Z", + closed_at: "2026-11-06T10:50:00Z", + body: "## Caused by\n\n- [acme/proj#3](https://github.com/acme/proj/pull/3) - lost a frame (found by the queue)\n", + }; + const { deps, uploads } = sender(env, 200, [session("s1", "quest/m0/a")], () => {}, [fix]); + expect(await sendPass(deps)).toEqual([ + { endpoint: ENDPOINT, repository: "acme/proj", records: 1, breakages: 1, refused: false }, + ]); + expect(parseUpload(uploads[0]!.body).breakages).toEqual([ + { + repository: "acme/proj", + culprit: { + number: 3, + merged: "2026-10-01T00:00:00.000Z", + tier: null, + size: "M", + writers: [{ harness: "Claude Code", model: "claude-opus-5-5", effort: "medium" }], + reviewers: [], + }, + fix: 8, + incident: null, + found: "queue", + at: "2026-11-06T10:20:00.000Z", + }, + ]); +}); + +test("an incident is posted at once, open until reported again with its culprit, and fails loud", async () => { + const env = home(); + const root = checkout(ENDPOINT); + const { deps, uploads } = sender(env, 200, []); + const report = (pull: number | null) => + reportIncident( + { + cwd: root, + id: "https://alerts.example/1", + found: "canary", + pull, + at: Date.parse("2026-11-06T12:00:00Z"), + }, + deps, + ); + expect((await report(null)).culprit).toBeNull(); + expect((await report(3)).culprit).toMatchObject({ number: 3, size: "M" }); + expect(uploads.map((u) => parseUpload(u.body))).toEqual([ + { + records: [], + breakages: [ + expect.objectContaining({ + incident: "https://alerts.example/1", + culprit: null, + fix: null, + found: "canary", + at: "2026-11-06T12:00:00.000Z", + }), + ], + }, + { records: [], breakages: [expect.objectContaining({ culprit: expect.objectContaining({ number: 3 }) })] }, + ]); + await expect(report(4)).rejects.toThrow("acme/proj has no PR #4"); + await expect( + reportIncident({ cwd: root, id: "x", found: "person", pull: null, at: 0 }, sender(env, 403, []).deps), + ).rejects.toThrow("answered 403"); + await expect( + reportIncident({ cwd: root, id: "x", found: "person", pull: null, at: 0 }, sender(home(null), 200, []).deps), + ).rejects.toThrow("holds no token"); + await expect( + reportIncident({ cwd: checkout(), id: "x", found: "person", pull: null, at: 0 }, deps), + ).rejects.toThrow("names no record endpoint"); +}); diff --git a/test/usage.test.ts b/test/usage.test.ts index db5493e0..aca42f9d 100644 --- a/test/usage.test.ts +++ b/test/usage.test.ts @@ -605,8 +605,13 @@ describe("usage", () => { const unrated = body10.slice(body10.indexOf("## Review")).replace(`${RISK}\n`, ""); const body9 = `${unrated}\n\n## Models\n\n- Size: L\n- Implementer: Claude Code, claude-opus-5-5, high effort\n`; const fork13 = unrated.replaceAll(THREAD, "thread-fork"); + // #11 also names #2, older than the window and read on its own, which a customer found, and #99, which is no PR. const body11 = - "Fix.\n\n## Caused by\n\n- [acme/proj#10](https://github.com/acme/proj/pull/10) - missed a case\n"; + "Fix.\n\n## Caused by\n\n- [acme/proj#10](https://github.com/acme/proj/pull/10) - missed a case\n" + + "- [acme/proj#2](https://github.com/acme/proj/pull/2) - dropped a header (found by a customer)\n" + + "- [acme/proj#99](https://github.com/acme/proj/pull/99) - mistyped\n"; + const body2 = + "Old.\n\n## Models\n\n- Size: XS\n- Implementer: Codex, gpt-6.1-sol, low effort\n\n## Review\n\nRewritten by `quest review` after each round.\n\n- Risk: garbled\n"; // A closed duplicate of #11's fix: it never landed, so it fixed nothing. // Newest updated first, as GitHub lists them, so the report stops at #8. const pulls = [ @@ -757,6 +762,14 @@ describe("usage", () => { fork: false, }); if (url.pathname === "/repos/acme/proj/pulls") return json(pulls); + if (url.pathname === "/repos/acme/proj/pulls/2") + return json( + pr(2, "old/b", body2, "2026-10-19T00:00:00Z", { + merged_at: "2026-10-20T00:00:00Z", + closed_at: "2026-10-20T00:00:00Z", + updated_at: "2026-10-20T00:00:00Z", + }), + ); return new Response("not found", { status: 404 }); }; const calls: [Date, string[]][] = []; @@ -940,6 +953,25 @@ describe("usage", () => { "#10 L high merged: PR 10 (quest/m0/a)\n risk: high, It rewrites the wire format.; raised from medium by a reviewer\n", ); expect(text).toContain("#11 - - merged: PR 11 (fix/a)\n"); + // #11 breaks down by culprit, #2 read from GitHub; #12 never merged, and #99 is no PR. + expect(report.breakages.map((b) => [b.culprit?.number, b.fix, b.found, b.at])).toEqual([ + [10, 11, null, "2026-11-06T20:00:00.000Z"], + [2, 11, "customer", "2026-11-06T20:00:00.000Z"], + ]); + expect(report.breakages[1]!.culprit).toEqual({ + number: 2, + merged: "2026-10-20T00:00:00.000Z", + tier: null, + size: "XS", + writers: [{ harness: "Codex", model: "gpt-6.1-sol", effort: "low" }], + reviewers: [], + }); + expect(report.breakages[0]!.culprit).toMatchObject({ tier: "high", size: "L" }); + expect(text).toContain( + "Breakages, by the merged fix naming its culprit under ## Caused by:\n" + + " #10 high, L, written by claude-opus-5-5 high, gpt-6.1-sol medium, reviewed by gpt-6.1-sol high, gemini-3-pro high, claude-opus-5-5 high, gpt-6.1-sol medium; fixed by #11, finder unrecorded, opened before it merged\n" + + " #2 no tier, XS, written by gpt-6.1-sol low, no local review; fixed by #11, found by customer, 17.8d after merge\n", + ); // #10 is the one PR with a tier, and a later PR fixed it; #11, #5, and #6 of the rest merged. expect(report.tiers).toEqual([ { tier: "high", pulls: 1, merged: 1, raised: 1, culprits: 1 }, From ef532fb94ed25c27615ab8e8a8badfba3cba0110 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 18:32:07 -0800 Subject: [PATCH 3/8] feat(context): ask fixes to name who found the bug under ## Caused by The maintainer approved the finder clause on #726. Tighten nearby wording so the hook context stays under 10,000 characters. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- assets/CONTEXT.md | 6 +++--- docs/context.md | 6 +++--- src/cli/breakage.ts | 2 +- src/cli/main.ts | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index c23e7f58..c54a1eb5 100644 --- a/README.md +++ b/README.md @@ -1654,8 +1654,8 @@ show whether the tiers predict what breaks. Each breakage a merged PR in the window names under `## Caused by` follows on its own line, by culprit, even one older than the window, which is read from GitHub: the culprit's tier, size, writers, and local reviewers, who found it when the bullet ends with -`(found by a check)`, or a reviewer, the queue, a canary, a person, or a -customer, and how long after the culprit merged the fix opened. Over the merged +`(found by check|reviewer|queue|canary|person|customer)` as the rules ask, +and how long after the culprit merged the fix opened. Over the merged PRs come each online reviewer's coverage, final heads reviewed, findings per PR reviewed, and answers. A hand-back counts only while its draft is open, so a merged PR diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index f1e8c09b..8d0cb291 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -65,7 +65,7 @@ A decision about an open PR never authorizes closing it. Confirm a close as its ## Pull requests Merge the base only when the branch conflicts with it or a landing step, such as `quest merge` or a merge queue, refuses the head, and resolve the conflicts in that merge; a branch only behind needs none: landing tests it on the current base. -Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. +Before each push to a PR's branch, confirm it is still open: a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. @@ -74,9 +74,9 @@ Once you fix or refute an automated reviewer's finding, say so in its thread and The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. -Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments limited to protected branches): quest branches are pushed to the repository, so their workflows get its secrets. Flag a workflow that breaks this. +Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments for protected branches): quest branches live in the repository, so their workflows get its secrets. Flag a workflow that breaks this. -A PR that fixes a bug, or redoes work an earlier PR did badly, lists that PR under `## Caused by` as `- [owner/repo#N](url) - what it got wrong`, found by `git blame` on the lines the fix changes or named in its quest's Plan, and confirmed in that PR's diff to introduce the defect, not merely touch the lines, defer the work, or keep older behavior, else traced further back with blame, or says in one line that no single PR caused it, such as an upstream change or a bug as old as the code. +A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer)`, the finder when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. Review findings fixed in the same PR are not culprits. ## Quests diff --git a/docs/context.md b/docs/context.md index 31a6a257..aab4f5cf 100644 --- a/docs/context.md +++ b/docs/context.md @@ -65,7 +65,7 @@ A decision about an open PR never authorizes closing it. Confirm a close as its ## Pull requests Merge the base only when the branch conflicts with it or a landing step, such as `quest merge` or a merge queue, refuses the head, and resolve the conflicts in that merge; a branch only behind needs none: landing tests it on the current base. -Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. +Before each push to a PR's branch, confirm it is still open: a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. @@ -74,9 +74,9 @@ Once you fix or refute an automated reviewer's finding, say so in its thread and The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. -Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments limited to protected branches): quest branches are pushed to the repository, so their workflows get its secrets. Flag a workflow that breaks this. +Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments for protected branches): quest branches live in the repository, so their workflows get its secrets. Flag a workflow that breaks this. -A PR that fixes a bug, or redoes work an earlier PR did badly, lists that PR under `## Caused by` as `- [owner/repo#N](url) - what it got wrong`, found by `git blame` on the lines the fix changes or named in its quest's Plan, and confirmed in that PR's diff to introduce the defect, not merely touch the lines, defer the work, or keep older behavior, else traced further back with blame, or says in one line that no single PR caused it, such as an upstream change or a bug as old as the code. +A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer)`, the finder when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. Review findings fixed in the same PR are not culprits. ## Quests diff --git a/src/cli/breakage.ts b/src/cli/breakage.ts index 3c3ec380..f4c6efd2 100644 --- a/src/cli/breakage.ts +++ b/src/cli/breakage.ts @@ -15,7 +15,7 @@ export interface Cause { /** * The PRs of `repository` that `body`'s `## Caused by` section names as culprits: each bullet that opens with * `[owner/repo#N](url) - why`. A link anywhere else on a line, as context or a "fixed by" aside, names no culprit. - * A line ending its why with `(found by a customer)`, or any finder, says who found it; a culprit named twice keeps + * A line ending its why with `(found by customer)`, or any finder, an article allowed, says who found it; a culprit named twice keeps * its first line. */ export function culprits(body: string, repository: Repository): Cause[] { diff --git a/src/cli/main.ts b/src/cli/main.ts index c7a8ee05..6d8ff7ff 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1197,7 +1197,7 @@ export function main(argv: string[], out: Output): number | Promise { "declines it. Rollups follow: spend by size, role, harness, model, and " + "effort, accuracy by size and implementer, summing each branch's tallies but the shadow's once, where an Implementer line's own size, as (iterate, XS), wins " + "over the PR's, with its writing cost over the merged PRs it was found on, the writer A/B by size and arm, counting a sized PR whose every Implementer line names the same arm, as (arm B), under its first line's writer with the spend of every session on it, local reviewers by size, harness, model, effort, and shadow, with their " + - "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, each online reviewer's coverage and findings over the merged PRs, and each breakage a merged PR in the window names under ## Caused by: its culprit's tier, size, writers, and local reviewers, read from GitHub when older than the window, who found it when the line ends with (found by a check), or a reviewer, the queue, a canary, a person, or a customer, and how long after the culprit merged the fix opened. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + + "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, each online reviewer's coverage and findings over the merged PRs, and each breakage a merged PR in the window names under ## Caused by: its culprit's tier, size, writers, and local reviewers, read from GitHub when older than the window, who found it when the line ends with (found by check|reviewer|queue|canary|person|customer), and how long after the culprit merged the fix opened. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + "--json prints the same report as one object.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 0 when done, 2 for invalid arguments, and 1 for " + "other failures.", From 36b047bd915449e34ffddbd9ef8189adb553018d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 18:36:49 -0800 Subject: [PATCH 4/8] fix(records): send a fix that merges after its sessions, and resolve an incident's directory Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +++- src/cli/send.ts | 35 ++++++++++++++++++++++++++++---- src/github/client.ts | 4 ++-- test/send.test.ts | 48 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index c54a1eb5..cfc02933 100644 --- a/README.md +++ b/README.md @@ -1762,7 +1762,9 @@ removes it. While no sender runs, stops still queue, and the next stop says so once per outage. Each pass runs token-max on this host from the earliest queued session's first -instant, reads the PRs updated since then, and posts +instant, reads the PRs updated since then, or since its last upload for that +endpoint and repository read them, if earlier, so a fix merged after its +sessions were sent still goes with the next one, and posts `{"records": [...], "breakages": [...]}`, the breakages being those the merged PRs among them name under `## Caused by`, as `quest usage` reports them, with `Authorization: Bearer `, one upload per endpoint and repository, each diff --git a/src/cli/send.ts b/src/cli/send.ts index 06471dff..fec19386 100644 --- a/src/cli/send.ts +++ b/src/cli/send.ts @@ -4,12 +4,12 @@ import { spawnSync } from "node:child_process"; import { existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { homedir, setPriority } from "node:os"; -import { dirname, join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { createHash, randomUUID } from "node:crypto"; import { type BreakageRecord, type Finder, type RecordUpload, parseUpload } from "../core"; import { culpritOf } from "./breakage"; -import { type GitHubDependencies, GitHub } from "../github/client"; +import { CLOCK_SKEW, type GitHubDependencies, GitHub } from "../github/client"; import { baseRepository } from "./claims"; import { discoveryEnv } from "./git"; import { readRemote, remoteRepository } from "./github"; @@ -364,7 +364,14 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { } const [owner, repo] = origin.split("/") as [string, string]; const repository = await baseRepository(client, { owner, repo }, prefix); - const pulls: Pull[] = await updatedPulls(client, repository, since); + // PRs are read from the last upload's read, if earlier, so a fix that merged after its sessions were sent, + // as through the merge queue, still reaches the store with the next session. + const read = deps.now() - CLOCK_SKEW; + const pulls: Pull[] = await updatedPulls( + client, + repository, + Math.min(since, readCursor(deps.env, id) ?? since), + ); const emit = new Set(); const missing: typeof group = []; for (const q of group) { @@ -457,6 +464,7 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { if (!response.ok) throw new Error(`${endpoint} answered ${response.status}: ${(await response.text()).slice(0, 500)}`); clearBackoff(deps.env, id); + writeCursor(deps.env, id, read); for (const q of group) settle(q); sent.push({ endpoint, @@ -500,7 +508,7 @@ export async function reportIncident( incident: Incident, deps: Pick, ): Promise { - const root = questRoot(incident.cwd); + const root = questRoot(resolve(incident.cwd)); if (root === null) throw new Error(`${incident.cwd} is not in a checkout that uses Quest`); const endpoint = checkoutEndpoint(root); if (endpoint === null) throw new Error(`${join(root, CHECKOUT_CONFIG)} names no record endpoint`); @@ -563,6 +571,25 @@ interface Backoff { next: number; } +const cursorPath = (env: NodeJS.ProcessEnv, id: string) => join(stateDirectory(env), "read", digest(id)); + +/** When group `id`'s last successful upload read its PRs, epoch ms, or null; an unreadable file reads as none. */ +function readCursor(env: NodeJS.ProcessEnv, id: string): number | null { + try { + const at = Number(readFileSync(cursorPath(env, id), "utf8")); + return Number.isFinite(at) && at > 0 ? at : null; + } catch { + return null; + } +} + +function writeCursor(env: NodeJS.ProcessEnv, id: string, at: number): void { + mkdirSync(join(stateDirectory(env), "read"), { recursive: true, mode: 0o700 }); + const path = cursorPath(env, id); + writeFileSync(`${path}.new`, String(at)); + renameSync(`${path}.new`, path); +} + const backoffPath = (env: NodeJS.ProcessEnv, id: string) => join(stateDirectory(env), "backoff", digest(id)); /** A group's backoff, or null without one; an unreadable file reads as none, so it can never block every group. */ diff --git a/src/github/client.ts b/src/github/client.ts index 4b5cfe80..cf80a1f1 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -48,8 +48,8 @@ export type ObjectValue = Record; /** Retries of a 5xx or a dropped connection, waiting 1, 2, 4, then 8 seconds unless GitHub says how long. */ const TRANSIENT_RETRIES = 4; const backoff = (failures: number) => 1000 * 2 ** failures; -/** How far GitHub's clock may run behind ours when looking for what a lost request created. */ -const CLOCK_SKEW = 60_000; +/** How far GitHub's clock may run behind ours, as when looking for what a lost request created. */ +export const CLOCK_SKEW = 60_000; /** Statuses whose response has no body. */ const NULL_BODY = [204, 205, 304]; diff --git a/test/send.test.ts b/test/send.test.ts index 2871ce48..7d932358 100644 --- a/test/send.test.ts +++ b/test/send.test.ts @@ -545,3 +545,51 @@ test("an incident is posted at once, open until reported again with its culprit, reportIncident({ cwd: checkout(), id: "x", found: "person", pull: null, at: 0 }, deps), ).rejects.toThrow("names no record endpoint"); }); + +test("a fix that merges after its sessions were sent reaches the store with the next session", async () => { + const env = home(); + const root = checkout(ENDPOINT); + queue(stopped(root), env); + const fix = (merged: boolean) => ({ + ...CULPRIT, + number: 8, + head: { ref: "fix/c", repo: { full_name: "acme/proj" } }, + state: merged ? "closed" : "open", + merged_at: merged ? "2026-11-06T12:30:00Z" : null, + closed_at: merged ? "2026-11-06T12:30:00Z" : null, + created_at: "2026-11-06T10:20:00Z", + updated_at: merged ? "2026-11-06T12:30:00Z" : "2026-11-06T10:20:00Z", + body: "## Caused by\n\n- [acme/proj#3](https://github.com/acme/proj/pull/3) - lost a frame\n", + }); + const open = sender(env, 200, [session("s1", "quest/m0/a")], () => {}, [fix(false)]); + expect((await sendPass(open.deps))[0]).toMatchObject({ records: 1, breakages: 0 }); + // The next session starts after the fix merged, so its own window alone would miss the fix. + writeFileSync(join(root, "later.jsonl"), `{"type":"user","timestamp":"2026-11-06T13:00:00Z"}\n`); + queue({ ...stopped(root), session: "s2", transcript: join(root, "later.jsonl") }, env); + const later = session("s2", "main", { + start: Date.parse("2026-11-06T13:00:00Z"), + end: Date.parse("2026-11-06T13:30:00Z"), + branches: [ + { name: "main", first: Date.parse("2026-11-06T13:00:00Z"), last: Date.parse("2026-11-06T13:30:00Z") }, + ], + }); + const merged = sender(env, 200, [later], () => {}, [fix(true)]); + merged.clock.now = Date.parse("2026-11-06T14:00:00Z"); + await sendPass(merged.deps); + expect(parseUpload(merged.uploads[0]!.body).breakages.map((b) => [b.culprit?.number, b.fix])).toEqual([[3, 8]]); +}); + +test("an incident reported from a subdirectory as `.`, as with no --root, finds its checkout", async () => { + const env = home(); + const root = checkout(ENDPOINT); + mkdirSync(join(root, "sub")); + const { deps } = sender(env, 200, []); + const before = process.cwd(); + process.chdir(join(root, "sub")); + try { + const reported = await reportIncident({ cwd: ".", id: "x", found: "person", pull: null, at: 0 }, deps); + expect(reported.repository).toBe("acme/proj"); + } finally { + process.chdir(before); + } +}); From 6d1c41f967b07d37368346ed9cd02db6813e28ee Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 18:46:08 -0800 Subject: [PATCH 5/8] feat(records): a fix's ## Caused by line closes the incident it names The maintainer picked option 1 on #726: a culprit line may end with (found by ...; incident ), and the fix's breakage replaces the open incident as one record. Co-Authored-By: Claude Opus 5.5 --- README.md | 17 +++++++++++------ assets/CONTEXT.md | 2 +- docs/context.md | 2 +- src/cli/breakage.ts | 27 +++++++++++++++++++-------- src/cli/main.ts | 5 +++-- src/cli/usage.ts | 19 ++++++++++++------- src/core/breakage.ts | 16 +++++++++------- test/records.test.ts | 9 +++++++-- test/send.test.ts | 21 +++++++++++++++++++++ test/usage.test.ts | 20 ++++++++++++++++++-- 10 files changed, 102 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index cfc02933..6acef492 100644 --- a/README.md +++ b/README.md @@ -1653,9 +1653,10 @@ how many a reviewer raised, and how many a later PR's `## Caused by` names show whether the tiers predict what breaks. Each breakage a merged PR in the window names under `## Caused by` follows on its own line, by culprit, even one older than the window, which is read from GitHub: the culprit's tier, size, -writers, and local reviewers, who found it when the bullet ends with -`(found by check|reviewer|queue|canary|person|customer)` as the rules ask, -and how long after the culprit merged the fix opened. Over the merged +writers, and local reviewers, who found it and the incident it closes when +the bullet ends with +`(found by check|reviewer|queue|canary|person|customer; incident )` as the +rules ask, either part alone allowed, and how long after the culprit merged the fix opened. Over the merged PRs come each online reviewer's coverage, final heads reviewed, findings per PR reviewed, and answers. A hand-back counts only while its draft is open, so a merged PR @@ -1788,9 +1789,13 @@ $ quest records incident https://alerts.example/1 --found canary --pr 42 reported https://alerts.example/1 against #42 ``` -The ID is the reporter's, and a report with the same ID replaces the last, so -an incident stays open until it is reported again with `--pr`, and the store -counts open ones apart. `--found` is one of `check`, `reviewer`, `queue`, +The ID is the reporter's, and a report with the same ID replaces the last. An +incident stays open, and the store counts open ones apart, until it is +reported again with `--pr`, or a merged fix names it on its culprit's +`## Caused by` line, as `(incident https://alerts.example/1)`: the sender then +sends the fix's breakage under the incident's ID, so it replaces the open one +as one record holding the culprit, the fix, and the incident. Where several +fixes name one incident, the one updated last wins. `--found` is one of `check`, `reviewer`, `queue`, `canary`, `person`, or `customer`, and `--at` sets when it was found, by default now. Reverts and red default-branch runs are not reported here: the store reads them from GitHub itself. diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index 8d0cb291..a24d7574 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -76,7 +76,7 @@ The PR body's `## Models` section records its size as `Size: M` (its quest's, or Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments for protected branches): quest branches live in the repository, so their workflows get its secrets. Flag a workflow that breaks this. -A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer)`, the finder when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. +A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer; incident )`, each part when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. Review findings fixed in the same PR are not culprits. ## Quests diff --git a/docs/context.md b/docs/context.md index aab4f5cf..2f015659 100644 --- a/docs/context.md +++ b/docs/context.md @@ -76,7 +76,7 @@ The PR body's `## Models` section records its size as `Size: M` (its quest's, or Keep pull request CI read-only (a read-only `GITHUB_TOKEN`, write credentials only in environments for protected branches): quest branches live in the repository, so their workflows get its secrets. Flag a workflow that breaks this. -A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer)`, the finder when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. +A PR fixing a bug, or redoing an earlier PR's bad work, lists it under `## Caused by` as `- [owner/repo#N](url) - what it got wrong (found by check|reviewer|queue|canary|person|customer; incident )`, each part when known. Find it by blaming the fixed lines, or in its quest's Plan, and confirm its diff introduced the defect, not merely touched those lines, deferred work, or kept older behavior, else trace further back; or say in one line why no single PR did, such as an upstream change or a bug as old as the code. Review findings fixed in the same PR are not culprits. ## Quests diff --git a/src/cli/breakage.ts b/src/cli/breakage.ts index f4c6efd2..b7de509c 100644 --- a/src/cli/breakage.ts +++ b/src/cli/breakage.ts @@ -6,17 +6,19 @@ import { recorded } from "./models-record"; import { parseReview } from "./review-record"; import type { Pull } from "./usage"; -/** One culprit a `## Caused by` section names, and who or what found the breakage when its line says. */ +/** One culprit a `## Caused by` section names, and who or what found the breakage and its incident, when its line says. */ export interface Cause { number: number; found: Finder | null; + /** The ID of the incident the fix closes, as `quest records incident` reported it. */ + incident: string | null; } /** * The PRs of `repository` that `body`'s `## Caused by` section names as culprits: each bullet that opens with * `[owner/repo#N](url) - why`. A link anywhere else on a line, as context or a "fixed by" aside, names no culprit. - * A line ending its why with `(found by customer)`, or any finder, an article allowed, says who found it; a culprit named twice keeps - * its first line. + * A line may end with a parenthesis of either part or both, as `(found by customer; incident )`: who found it, + * an article allowed, and the incident the fix closes. A culprit named twice keeps its first line. */ export function culprits(body: string, repository: Repository): Cause[] { const heading = /^## Caused by[ \t]*\r?$/m.exec(body); @@ -29,13 +31,22 @@ export function culprits(body: string, repository: Repository): Cause[] { `^- \\[${name}#(\\d+)\\]\\(https://github\\.com/${name}/pull/(\\d+)\\) - \\S[^\\r\\n]*`, "gim", ); - const finder = new RegExp(`\\(found by (?:an? |the )?(${FINDERS.join("|")})\\)`, "i"); - const found = new Map(); + const tail = new RegExp( + `\\((?:found by (?:an? |the )?(${FINDERS.join("|")})(?:; |(?=\\))))?(?:incident ([^\\r\\n]+?))?\\)\\s*$`, + "i", + ); + const found = new Map(); for (const match of section.matchAll(culprit)) { - if (match[1] !== match[2] || found.has(Number(match[1]))) continue; - found.set(Number(match[1]), (finder.exec(match[0])?.[1]?.toLowerCase() as Finder | undefined) ?? null); + const number = Number(match[1]); + if (match[1] !== match[2] || found.has(number)) continue; + const [, by, incident] = tail.exec(match[0]) ?? []; + found.set(number, { + number, + found: (by?.toLowerCase() as Finder | undefined) ?? null, + incident: incident?.trim() || null, + }); } - return [...found].map(([number, by]) => ({ number, found: by })); + return [...found.values()]; } /** diff --git a/src/cli/main.ts b/src/cli/main.ts index 6d8ff7ff..3cba7318 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -1197,7 +1197,7 @@ export function main(argv: string[], out: Output): number | Promise { "declines it. Rollups follow: spend by size, role, harness, model, and " + "effort, accuracy by size and implementer, summing each branch's tallies but the shadow's once, where an Implementer line's own size, as (iterate, XS), wins " + "over the PR's, with its writing cost over the merged PRs it was found on, the writer A/B by size and arm, counting a sized PR whose every Implementer line names the same arm, as (arm B), under its first line's writer with the spend of every session on it, local reviewers by size, harness, model, effort, and shadow, with their " + - "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, each online reviewer's coverage and findings over the merged PRs, and each breakage a merged PR in the window names under ## Caused by: its culprit's tier, size, writers, and local reviewers, read from GitHub when older than the window, who found it when the line ends with (found by check|reviewer|queue|canary|person|customer), and how long after the culprit merged the fix opened. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + + "tallies and reviewing cost per PR, each risk tier's PRs, merged PRs, tiers a reviewer raised, and PRs a later one fixed, each online reviewer's coverage and findings over the merged PRs, and each breakage a merged PR in the window names under ## Caused by: its culprit's tier, size, writers, and local reviewers, read from GitHub when older than the window, who found it and the incident it closes when the line ends with (found by check|reviewer|queue|canary|person|customer; incident ), either part alone allowed, and how long after the culprit merged the fix opened. Without token-max on PATH, it says how to install it and reports the PRs' records alone. " + "--json prints the same report as one object.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 0 when done, 2 for invalid arguments, and 1 for " + "other failures.", @@ -1276,7 +1276,8 @@ export function main(argv: string[], out: Output): number | Promise { "Post one breakage to the endpoint this checkout's " + `${CHECKOUT_CONFIG} names, with the token ${tokensPath()} pairs with it, as {"records": [], "breakages": [...]}. ` + "Without --pr the incident is open, its culprit unknown; report the same ID again with --pr to close it on " + - "its culprit, whose tier, size, writers, and local reviewers are read from its PR body. It posts at once " + + "its culprit, whose tier, size, writers, and local reviewers are read from its PR body, or name it on the " + + "culprit's ## Caused by line of the fix, as (incident ), whose record then replaces it. It posts at once " + "and retries nothing, for a deploy or alert to call.\n\nUses GH_TOKEN, GITHUB_TOKEN, or gh auth token. " + "Exits 0 when the endpoint took it, 2 for invalid arguments, and 1 for other failures, refusal included.", ) diff --git a/src/cli/usage.ts b/src/cli/usage.ts index d78864fe..69794f53 100644 --- a/src/cli/usage.ts +++ b/src/cli/usage.ts @@ -24,7 +24,7 @@ import { severityText, } from "./review-record"; import { SEVERITIES } from "./review"; -import { type BreakageRecord, ROLES, type Role, type Seat, type SessionRecord, WAIT_KINDS } from "../core"; +import { type BreakageRecord, ROLES, breakageKey, type Role, type Seat, type SessionRecord, WAIT_KINDS } from "../core"; import { buildRecords } from "./records"; import { type LoggedWait, readWaits } from "./waits"; @@ -419,26 +419,31 @@ export async function readPull(client: GitHub, repository: Repository, number: n } /** - * A breakage for each culprit the merged PRs among `pulls` name under `## Caused by`, found when the fix opened. A - * culprit outside `pulls` is read from GitHub, once each; a link to no PR names no culprit. + * A breakage for each culprit the merged PRs among `pulls` name under `## Caused by`, found when the fix opened, with + * the incident its line closes. A culprit outside `pulls` is read from GitHub, once each; a link to no PR names no + * culprit. An incident several lines close keeps the line of the fix updated last, so no body can send one twice. */ export async function fixBreakages(client: GitHub, repository: Repository, pulls: Pull[]): Promise { const known = new Map(pulls.map((each) => [each.number, each])); const breakages: BreakageRecord[] = []; + const seen = new Set(); + // GitHub lists the newest update first. for (const fix of pulls.filter((each) => each.state === "merged")) for (const cause of culprits(fix.body, repository)) { if (cause.number === fix.number) continue; if (!known.has(cause.number)) known.set(cause.number, await readPull(client, repository, cause.number)); const culprit = known.get(cause.number); if (!culprit) continue; - breakages.push({ + const breakage: BreakageRecord = { repository: key(repository), culprit: culpritOf(culprit), fix: fix.number, - incident: null, + incident: cause.incident, found: cause.found, at: new Date(fix.created).toISOString(), - }); + }; + if (!seen.has(breakageKey(breakage))) breakages.push(breakage); + seen.add(breakageKey(breakage)); } return breakages; } @@ -1131,7 +1136,7 @@ function breakageText(b: BreakageRecord): string { : ms < 0 ? "opened before it merged" : `${ms >= 172_800_000 ? `${(ms / 86_400_000).toFixed(1)}d` : duration(ms)} after merge`; - return `${made}; fixed by #${b.fix}, ${b.found ? `found by ${b.found}` : "finder unrecorded"}, ${after}`; + return `${made}; fixed by #${b.fix}${b.incident ? `, closing incident ${b.incident}` : ""}, ${b.found ? `found by ${b.found}` : "finder unrecorded"}, ${after}`; } /** Seconds under a minute, hours past two, else minutes. */ diff --git a/src/core/breakage.ts b/src/core/breakage.ts index 919f27d4..5162fa91 100644 --- a/src/core/breakage.ts +++ b/src/core/breakage.ts @@ -32,21 +32,23 @@ export interface Culprit { } /** - * One breakage. A fix's is keyed by its repository, culprit, and fix, and an incident's by its repository and ID, so a - * resend replaces it: reporting an open incident again with its culprit closes it. + * One breakage. One naming an incident is keyed by its repository and that ID, else by its repository, culprit, and + * fix, so a resend replaces it. An open incident closes when it is reported again with its culprit, or when a merged + * fix's `## Caused by` names it: the fix's record, holding culprit, fix, and incident, then replaces it. A store + * replacing an incident keeps its earlier `at`, and its finder when the fix names none, since the fix opened later. */ export interface BreakageRecord { /** The culprit's repository as lowercase `owner/repo`. */ repository: string; /** Null only for an incident whose culprit is not known yet. */ culprit: Culprit | null; - /** The PR naming the culprit under `## Caused by`, or null for an incident. */ + /** The PR naming the culprit under `## Caused by`, or null for an incident a fix has not closed. */ fix: number | null; - /** The ID its reporter gave an incident, such as the alert's URL, or null for a fix. */ + /** The ID its reporter gave an incident, such as the alert's URL, or null for a fix naming no incident. */ incident: string | null; /** Null when the fix's `## Caused by` line does not say. */ found: Finder | null; - /** When it was found, ISO 8601 UTC: the fix PR's opening, or the incident's report. */ + /** When it was found, ISO 8601 UTC: the incident's report, else the fix PR's opening. */ at: string; } @@ -77,7 +79,7 @@ function culprit(value: unknown): Culprit { }; } -/** `value` as a breakage, failing on any missing, extra, or ill-typed field, or one that is neither fix nor incident. */ +/** `value` as a breakage, failing on any missing, extra, or ill-typed field, or one naming neither fix nor incident. */ export function parseBreakage(value: unknown): BreakageRecord { if (!isObject(value)) return fail("not an object"); exact(value, ["repository", "culprit", "fix", "incident", "found", "at"], "the breakage"); @@ -91,7 +93,7 @@ export function parseBreakage(value: unknown): BreakageRecord { found: value.found === null ? null : oneOf(value.found, FINDERS, "how it was found"), at: instant(value.at, "when it was found"), }; - if ((record.fix === null) === (record.incident === null)) fail("it names neither or both of a fix and an incident"); + if (record.fix === null && record.incident === null) fail("it names neither a fix nor an incident"); if (record.fix !== null && record.culprit === null) fail("a fix names no culprit"); if (record.fix !== null && record.fix === record.culprit?.number) fail("a PR fixes itself"); return record; diff --git a/test/records.test.ts b/test/records.test.ts index c887e062..48df9297 100644 --- a/test/records.test.ts +++ b/test/records.test.ts @@ -132,13 +132,14 @@ describe("parseBreakage", () => { breakage(), breakage({ fix: null, incident: "https://alerts.example/1", culprit: null, found: "canary" }), breakage({ fix: null, incident: "deploy 42", found: null }), + // A fix closing an incident. + breakage({ incident: "deploy 42" }), ]) expect(parseBreakage(JSON.parse(JSON.stringify(sent)))).toEqual(sent); }); test.each([ - ["neither fix nor incident", breakage({ fix: null }), "neither or both"], - ["both fix and incident", breakage({ incident: "x" }), "neither or both"], + ["neither fix nor incident", breakage({ fix: null }), "neither a fix nor an incident"], ["a fix with no culprit", breakage({ culprit: null }), "names no culprit"], ["a PR fixing itself", breakage({ fix: 5 }), "fixes itself"], ["an unknown finder", breakage({ found: "luck" as never }), "how it was found"], @@ -162,6 +163,10 @@ describe("parseBreakage", () => { ).toHaveLength(3); expect(() => parseUpload({ records: [], breakages: [breakage(), breakage()] })).toThrow("breakage twice"); expect(() => parseUpload({ records: [], breakages: [closed, open] })).toThrow("breakage twice"); + // A fix closing the incident replaces it, so the two never travel together. + expect(() => parseUpload({ records: [], breakages: [open, breakage({ incident: "i1" })] })).toThrow( + "breakage twice", + ); }); }); diff --git a/test/send.test.ts b/test/send.test.ts index 7d932358..5021de43 100644 --- a/test/send.test.ts +++ b/test/send.test.ts @@ -593,3 +593,24 @@ test("an incident reported from a subdirectory as `.`, as with no --root, finds process.chdir(before); } }); + +test("a merged fix naming an open incident sends one record under the incident, once however many name it", async () => { + const env = home(); + queue(stopped(checkout(ENDPOINT)), env); + const fix = (number: number, updated: string) => ({ + ...CULPRIT, + number, + head: { ref: `fix/${number}`, repo: { full_name: "acme/proj" } }, + merged_at: updated, + created_at: "2026-11-06T10:20:00Z", + updated_at: updated, + closed_at: updated, + body: "## Caused by\n\n- [acme/proj#3](https://github.com/acme/proj/pull/3) - lost a frame (found by canary; incident deploy 42)\n", + }); + const pulls = [fix(9, "2026-11-06T10:55:00Z"), fix(8, "2026-11-06T10:50:00Z")]; + const { deps, uploads } = sender(env, 200, [session("s1", "quest/m0/a")], () => {}, pulls); + await sendPass(deps); + expect(parseUpload(uploads[0]!.body).breakages.map((b) => [b.culprit?.number, b.fix, b.incident, b.found])).toEqual( + [[3, 9, "deploy 42", "canary"]], + ); +}); diff --git a/test/usage.test.ts b/test/usage.test.ts index aca42f9d..a6e5af7f 100644 --- a/test/usage.test.ts +++ b/test/usage.test.ts @@ -8,6 +8,7 @@ import { implementer } from "../src/cli/models-record"; import { fixes } from "../src/cli/land-fixer"; import type { Risk, Round, Tally } from "../src/cli/review"; import { classifierSessions, parseReview, reviewRecord, reviewSessions } from "../src/cli/review-record"; +import { culprits } from "../src/cli/breakage"; import { type Session, causes, fold, parseSessions, share, usage, usageLines } from "../src/cli/usage"; import type { LoggedWait } from "../src/cli/waits"; import { tempDir } from "./fixture"; @@ -166,6 +167,20 @@ describe("causes", () => { ]); expect(causes(`- ${link(7)} - outside the section`, repository)).toEqual([]); }); + + test("reads who found it and the incident it closes from the parenthesis ending the line", () => { + const read = (line: string) => culprits(body(`- ${link(7)} - ${line}`), repository)[0]; + expect(read("lost a frame (found by canary; incident https://alerts.example/1)")).toEqual({ + number: 7, + found: "canary", + incident: "https://alerts.example/1", + }); + expect(read("lost a frame (incident deploy 42)\r")).toMatchObject({ found: null, incident: "deploy 42" }); + expect(read("lost a frame (found by the Queue)")).toMatchObject({ found: "queue", incident: null }); + // Only a parenthesis ending the line counts, and only a known finder. + expect(read("lost a frame (found by canary) on deploy")).toMatchObject({ found: null, incident: null }); + expect(read("lost a frame (found by luck)")).toMatchObject({ found: null, incident: null }); + }); }); describe("PR records", () => { @@ -608,7 +623,7 @@ describe("usage", () => { // #11 also names #2, older than the window and read on its own, which a customer found, and #99, which is no PR. const body11 = "Fix.\n\n## Caused by\n\n- [acme/proj#10](https://github.com/acme/proj/pull/10) - missed a case\n" + - "- [acme/proj#2](https://github.com/acme/proj/pull/2) - dropped a header (found by a customer)\n" + + "- [acme/proj#2](https://github.com/acme/proj/pull/2) - dropped a header (found by a customer; incident page 7)\n" + "- [acme/proj#99](https://github.com/acme/proj/pull/99) - mistyped\n"; const body2 = "Old.\n\n## Models\n\n- Size: XS\n- Implementer: Codex, gpt-6.1-sol, low effort\n\n## Review\n\nRewritten by `quest review` after each round.\n\n- Risk: garbled\n"; @@ -958,6 +973,7 @@ describe("usage", () => { [10, 11, null, "2026-11-06T20:00:00.000Z"], [2, 11, "customer", "2026-11-06T20:00:00.000Z"], ]); + expect(report.breakages.map((b) => b.incident)).toEqual([null, "page 7"]); expect(report.breakages[1]!.culprit).toEqual({ number: 2, merged: "2026-10-20T00:00:00.000Z", @@ -970,7 +986,7 @@ describe("usage", () => { expect(text).toContain( "Breakages, by the merged fix naming its culprit under ## Caused by:\n" + " #10 high, L, written by claude-opus-5-5 high, gpt-6.1-sol medium, reviewed by gpt-6.1-sol high, gemini-3-pro high, claude-opus-5-5 high, gpt-6.1-sol medium; fixed by #11, finder unrecorded, opened before it merged\n" + - " #2 no tier, XS, written by gpt-6.1-sol low, no local review; fixed by #11, found by customer, 17.8d after merge\n", + " #2 no tier, XS, written by gpt-6.1-sol low, no local review; fixed by #11, closing incident page 7, found by customer, 17.8d after merge\n", ); // #10 is the one PR with a tier, and a later PR fixed it; #11, #5, and #6 of the rest merged. expect(report.tiers).toEqual([ From 6a04878aa05cb2d0ecc7a7926dda7ced071994f1 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 19:22:57 -0800 Subject: [PATCH 6/8] fix(records): read each Caused by line by culprit and incident together Co-Authored-By: Claude Opus 5.5 --- README.md | 3 ++- src/cli/breakage.ts | 24 ++++++++++++++---------- src/cli/usage.ts | 5 +++-- test/usage.test.ts | 10 ++++++++++ 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 6acef492..6716afcc 100644 --- a/README.md +++ b/README.md @@ -1795,7 +1795,8 @@ reported again with `--pr`, or a merged fix names it on its culprit's `## Caused by` line, as `(incident https://alerts.example/1)`: the sender then sends the fix's breakage under the incident's ID, so it replaces the open one as one record holding the culprit, the fix, and the incident. Where several -fixes name one incident, the one updated last wins. `--found` is one of `check`, `reviewer`, `queue`, +fixes name one incident, the one updated last wins. One fix closes several incidents of a +culprit with a line for each. `--found` is one of `check`, `reviewer`, `queue`, `canary`, `person`, or `customer`, and `--at` sets when it was found, by default now. Reverts and red default-branch runs are not reported here: the store reads them from GitHub itself. diff --git a/src/cli/breakage.ts b/src/cli/breakage.ts index b7de509c..42c9002e 100644 --- a/src/cli/breakage.ts +++ b/src/cli/breakage.ts @@ -18,7 +18,8 @@ export interface Cause { * The PRs of `repository` that `body`'s `## Caused by` section names as culprits: each bullet that opens with * `[owner/repo#N](url) - why`. A link anywhere else on a line, as context or a "fixed by" aside, names no culprit. * A line may end with a parenthesis of either part or both, as `(found by customer; incident )`: who found it, - * an article allowed, and the incident the fix closes. A culprit named twice keeps its first line. + * an article allowed, and the incident the fix closes. Each culprit and incident keeps its first line, and a + * culprit whose lines name incidents drops its line naming none. */ export function culprits(body: string, repository: Repository): Cause[] { const heading = /^## Caused by[ \t]*\r?$/m.exec(body); @@ -35,18 +36,21 @@ export function culprits(body: string, repository: Repository): Cause[] { `\\((?:found by (?:an? |the )?(${FINDERS.join("|")})(?:; |(?=\\))))?(?:incident ([^\\r\\n]+?))?\\)\\s*$`, "i", ); - const found = new Map(); + const found = new Map(); for (const match of section.matchAll(culprit)) { - const number = Number(match[1]); - if (match[1] !== match[2] || found.has(number)) continue; - const [, by, incident] = tail.exec(match[0]) ?? []; - found.set(number, { - number, + if (match[1] !== match[2]) continue; + const [, by, named] = tail.exec(match[0]) ?? []; + const cause: Cause = { + number: Number(match[1]), found: (by?.toLowerCase() as Finder | undefined) ?? null, - incident: incident?.trim() || null, - }); + incident: named?.trim() || null, + }; + const id = JSON.stringify([cause.number, cause.incident]); + if (!found.has(id)) found.set(id, cause); } - return [...found.values()]; + // A line naming an incident records its culprit's breakage, so the culprit's line naming none would count it twice. + const named = new Set([...found.values()].filter((c) => c.incident !== null).map((c) => c.number)); + return [...found.values()].filter((c) => c.incident !== null || !named.has(c.number)); } /** diff --git a/src/cli/usage.ts b/src/cli/usage.ts index 69794f53..1c164f65 100644 --- a/src/cli/usage.ts +++ b/src/cli/usage.ts @@ -409,8 +409,9 @@ export async function updatedPulls(client: GitHub, repository: Repository, since } /** The PR numbers of `repository` that `body`'s `## Caused by` section names as culprits, as {@link culprits} reads them. */ -export const causes = (body: string, repository: Repository): number[] => - culprits(body, repository).map((cause) => cause.number); +export const causes = (body: string, repository: Repository): number[] => [ + ...new Set(culprits(body, repository).map((cause) => cause.number)), +]; /** PR `number` of `repository`, or null when GitHub has none, as for a mistyped `## Caused by` link. */ export async function readPull(client: GitHub, repository: Repository, number: number): Promise { diff --git a/test/usage.test.ts b/test/usage.test.ts index a6e5af7f..4d860ca1 100644 --- a/test/usage.test.ts +++ b/test/usage.test.ts @@ -180,6 +180,16 @@ describe("causes", () => { // Only a parenthesis ending the line counts, and only a known finder. expect(read("lost a frame (found by canary) on deploy")).toMatchObject({ found: null, incident: null }); expect(read("lost a frame (found by luck)")).toMatchObject({ found: null, incident: null }); + // One fix closing two incidents of one culprit closes both, its line naming none dropped; the culprit counts once. + const lines = body( + `- ${link(7)} - lost a frame (incident a)\n- ${link(7)} - again (found by person; incident b)\n- ${link(7)} - plain\n- ${link(7)} - dup (incident a)\n- ${link(8)} - other`, + ); + expect(culprits(lines, repository)).toEqual([ + { number: 7, found: null, incident: "a" }, + { number: 7, found: "person", incident: "b" }, + { number: 8, found: null, incident: null }, + ]); + expect(causes(lines, repository)).toEqual([7, 8]); }); }); From fd27182a27c706bc12245c740fdb60e37944e822 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 19:24:17 -0800 Subject: [PATCH 7/8] fix(records): refuse impossible calendar dates in a record's instants Applies CodeRabbit's suggested patch unchanged. Co-Authored-By: Claude Opus 5.5 --- src/core/parse.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/core/parse.ts b/src/core/parse.ts index 585efad6..04abed05 100644 --- a/src/core/parse.ts +++ b/src/core/parse.ts @@ -15,7 +15,12 @@ export function parser(what: string) { const flag = (value: unknown, field: string): boolean => (typeof value === "boolean" ? value : fail(field)); const instant = (value: unknown, field: string): string => { const at = text(value, field); - return /^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(\.\d+)?Z$/.test(at) && !Number.isNaN(Date.parse(at)) ? at : fail(field); + const match = /^(\d{4})-(\d\d)-(\d\d)T\d\d:\d\d:\d\d(\.\d+)?Z$/.exec(at); + if (!match || Number.isNaN(Date.parse(at))) return fail(field); + const date = new Date(0); + date.setUTCFullYear(Number(match[1]), Number(match[2]) - 1, Number(match[3])); + const valid = date.getUTCMonth() === Number(match[2]) - 1 && date.getUTCDate() === Number(match[3]); + return valid ? at : fail(field); }; const oneOf = (value: unknown, options: readonly T[], field: string): T => options.includes(value as T) ? (value as T) : fail(field); From 03b356b074bb8a8a332850552adb3be2c6b0b942 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 19:25:51 -0800 Subject: [PATCH 8/8] fix(records): require a Z or offset in --at, and read no culprit a narrowed report leaves out Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- src/cli/main.ts | 19 +++++++++++++++---- src/cli/send.ts | 19 ++++++++++++++++++- src/cli/usage.ts | 24 +++++++++++++++++++----- test/records.test.ts | 3 +++ test/send.test.ts | 25 ++++++++++++++++++++++++- test/usage.test.ts | 8 +++++++- 7 files changed, 88 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 6716afcc..8d4ef483 100644 --- a/README.md +++ b/README.md @@ -1797,8 +1797,8 @@ sends the fix's breakage under the incident's ID, so it replaces the open one as one record holding the culprit, the fix, and the incident. Where several fixes name one incident, the one updated last wins. One fix closes several incidents of a culprit with a line for each. `--found` is one of `check`, `reviewer`, `queue`, -`canary`, `person`, or `customer`, and `--at` sets when it was found, by -default now. Reverts and red default-branch runs are not reported here: the +`canary`, `person`, or `customer`, and `--at` sets when it was found, as +an ISO 8601 date and time with a `Z` or offset, by default now. Reverts and red default-branch runs are not reported here: the store reads them from GitHub itself. ### Typed messages diff --git a/src/cli/main.ts b/src/cli/main.ts index 3cba7318..1f409028 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -86,7 +86,15 @@ import { import { CHECKED_CONTEXT, USER_ASKED, questRoot } from "./rules"; import { messageLines, messages } from "./messages"; import { logWait, pullWaitKind, readWaits } from "./waits"; -import { CHECKOUT_CONFIG, SEND_INTERVAL, background, reportIncident, runSender, tokensPath } from "./send"; +import { + CHECKOUT_CONFIG, + SEND_INTERVAL, + background, + parseInstant, + reportIncident, + runSender, + tokensPath, +} from "./send"; import { installSender, uninstallSender } from "./service"; import { heal, init, stubFindings, uninstall } from "./setup"; import { Refusal, describe, notUpstream, releaseQuest, startQuest } from "./start"; @@ -1283,12 +1291,15 @@ export function main(argv: string[], out: Output): number | Promise { ) .addOption(new Option("--found ", "Who or what found it").choices(FINDERS).makeOptionMandatory()) .option("--pr ", "The PR that caused it", positiveInteger("--pr")) - .option("--at ", "When it was found, as an ISO 8601 instant (default: now)") + .option("--at ", "When it was found, as an ISO 8601 date and time with a Z or offset (default: now)") .action((id: string, options: { found: Finder; pr?: number; at?: string }) => { if (/[\r\n]/.test(id) || id.trim() === "") throw new UsageError("the incident ID must be one non-empty line"); - const at = options.at === undefined ? Date.now() : Date.parse(options.at); - if (Number.isNaN(at)) throw new UsageError(`--at ${options.at} is not an ISO 8601 instant`); + const at = options.at === undefined ? Date.now() : parseInstant(options.at); + if (at === null) + throw new UsageError( + `--at ${options.at} is not an ISO 8601 date and time with a Z or offset, on a real calendar day`, + ); settle( reportIncident( { cwd: root(), id, found: options.found, pull: options.pr ?? null, at }, diff --git a/src/cli/send.ts b/src/cli/send.ts index fec19386..bfd358d6 100644 --- a/src/cli/send.ts +++ b/src/cli/send.ts @@ -442,7 +442,7 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { }); const upload: RecordUpload = { records: [...records.values()], - breakages: await fixBreakages(client, repository, pulls), + breakages: await fixBreakages({ client, repository, pulls }), }; const response = await post(deps.fetch, endpoint, token, upload); if (response.status === 401 || response.status === 403) { @@ -486,6 +486,23 @@ async function sendOnce(deps: SendDependencies, dir: string): Promise { return sent; } +/** + * `text` as epoch ms when it is an ISO 8601 date and time with an explicit `Z` or offset on a real calendar day, else + * null: `Date.parse` alone takes a bare date as UTC midnight, a time without an offset as this host's, and rolls an + * impossible day such as February 30 into the next month. + */ +export function parseInstant(text: string): number | null { + const match = + /^(\d{4})-(\d\d)-(\d\d)T([01]\d|2[0-3]):[0-5]\d(:[0-5]\d(\.\d+)?)?(Z|[+-]([01]\d|2[0-3]):[0-5]\d)$/.exec(text); + if (!match) return null; + const [year, month, day] = [Number(match[1]), Number(match[2]), Number(match[3])]; + const date = new Date(0); + date.setUTCFullYear(year, month - 1, day); + if (date.getUTCMonth() !== month - 1 || date.getUTCDate() !== day) return null; + const at = Date.parse(text); + return Number.isNaN(at) ? null : at; +} + /** An incident to report, as a deploy or alert hands it to `quest records incident`. */ export interface Incident { /** A directory in the checkout whose endpoint and repository it is reported for. */ diff --git a/src/cli/usage.ts b/src/cli/usage.ts index 1c164f65..8fa66f5b 100644 --- a/src/cli/usage.ts +++ b/src/cli/usage.ts @@ -419,12 +419,22 @@ export async function readPull(client: GitHub, repository: Repository, number: n return response.status === 404 ? null : pull(object(await response.json())); } +/** What {@link fixBreakages} reads. */ +export interface FixSources { + client: GitHub; + repository: Repository; + pulls: Pull[]; + /** The PRs a narrowed report shows; unset, every breakage. */ + only?: Set; +} + /** * A breakage for each culprit the merged PRs among `pulls` name under `## Caused by`, found when the fix opened, with * the incident its line closes. A culprit outside `pulls` is read from GitHub, once each; a link to no PR names no * culprit. An incident several lines close keeps the line of the fix updated last, so no body can send one twice. + * Given `only`, it keeps only the breakages whose fix or culprit `only` names. */ -export async function fixBreakages(client: GitHub, repository: Repository, pulls: Pull[]): Promise { +export async function fixBreakages({ client, repository, pulls, only }: FixSources): Promise { const known = new Map(pulls.map((each) => [each.number, each])); const breakages: BreakageRecord[] = []; const seen = new Set(); @@ -432,6 +442,8 @@ export async function fixBreakages(client: GitHub, repository: Repository, pulls for (const fix of pulls.filter((each) => each.state === "merged")) for (const cause of culprits(fix.body, repository)) { if (cause.number === fix.number) continue; + // A narrowed report reads no culprit of a breakage it leaves out. + if (only && !only.has(fix.number) && !only.has(cause.number)) continue; if (!known.has(cause.number)) known.set(cause.number, await readPull(client, repository, cause.number)); const culprit = known.get(cause.number); if (!culprit) continue; @@ -1072,10 +1084,12 @@ export async function usage(root: string, options: UsageOptions, deps: UsageDepe // `quest ready` and hand edits drop the section before merge, so an earlier body counts too. if (read?.bodies.some(handedBack)) row.handedBack = true; } - const reported = new Set(rows.map((row) => row.number)); - const breakages = (await fixBreakages(client, repository, all)).filter( - (each) => !narrowed || reported.has(each.fix!) || reported.has(each.culprit!.number), - ); + const breakages = await fixBreakages({ + client, + repository, + pulls: all, + only: narrowed ? new Set(rows.map((row) => row.number)) : undefined, + }); return { repository: key(repository), since: options.since, diff --git a/test/records.test.ts b/test/records.test.ts index 48df9297..fe4cde97 100644 --- a/test/records.test.ts +++ b/test/records.test.ts @@ -134,6 +134,8 @@ describe("parseBreakage", () => { breakage({ fix: null, incident: "deploy 42", found: null }), // A fix closing an incident. breakage({ incident: "deploy 42" }), + // A leap day is a real one. + breakage({ at: "2024-02-29T10:00:00Z" }), ]) expect(parseBreakage(JSON.parse(JSON.stringify(sent)))).toEqual(sent); }); @@ -151,6 +153,7 @@ describe("parseBreakage", () => { "unknown field cost", ], ["a mixed-case repository", breakage({ repository: "Acme/proj" }), "lowercase"], + ["an impossible calendar day", breakage({ at: "2026-02-30T10:00:00Z" }), "when it was found"], ])("refuses %s", (_, value, message) => { expect(() => parseBreakage(value)).toThrow(message); }); diff --git a/test/send.test.ts b/test/send.test.ts index 5021de43..ef3ec316 100644 --- a/test/send.test.ts +++ b/test/send.test.ts @@ -6,7 +6,15 @@ import { expect, test } from "vitest"; import { REFERENCE_LINE, parseUpload } from "../src/core"; import { hook } from "../src/cli/hooks"; -import { type SendDependencies, queue, reportIncident, runSender, sendPass, tokensPath } from "../src/cli/send"; +import { + type SendDependencies, + parseInstant, + queue, + reportIncident, + runSender, + sendPass, + tokensPath, +} from "../src/cli/send"; import type { Session } from "../src/cli/usage"; import { tempDir } from "./fixture"; @@ -614,3 +622,18 @@ test("a merged fix naming an open incident sends one record under the incident, [[3, 9, "deploy 42", "canary"]], ); }); + +test("an incident's --at must be a date and time with a Z or offset, on a real calendar day", () => { + expect(parseInstant("2026-11-06T10:00:00Z")).toBe(Date.parse("2026-11-06T10:00:00Z")); + expect(parseInstant("2026-11-06T10:00+02:00")).toBe(Date.parse("2026-11-06T08:00:00Z")); + expect(parseInstant("2024-02-29T10:00:00.5Z")).toBe(Date.parse("2024-02-29T10:00:00.500Z")); + for (const text of [ + "2026-11-06", + "2026-11-06T10:00:00", + "2026-02-30T10:00:00Z", + "2026-11-06T24:00:00Z", + "2026-11-06T10:00:00+25:00", + "Nov 6 2026 10:00 UTC", + ]) + expect(parseInstant(text), text).toBeNull(); +}); diff --git a/test/usage.test.ts b/test/usage.test.ts index 4d860ca1..5eff8e83 100644 --- a/test/usage.test.ts +++ b/test/usage.test.ts @@ -1174,10 +1174,16 @@ describe("usage", () => { }); test("narrows to one PR, and reports the records alone without token-max", async () => { - const { root, deps, sessions } = setup(); + const { root, deps, sessions, requests } = setup(); const one = await usage(root, { since: "2026-11-05", pr: 10 }, deps(sessions)); expect(one.pulls.map((p) => p.number)).toEqual([10]); expect(one.pulls[0]!.fixedBy).toEqual([11]); + // Only #10's breakage is kept, and #11's other culprits, #2 and #99, are never read. + expect(one.breakages.map((b) => [b.culprit?.number, b.fix])).toEqual([[10, 11]]); + expect(requests.filter((path) => /\/pulls\/\d+$/.test(path))).toEqual([]); + // Narrowed to the fix, every culprit it names is kept. + const fix = await usage(root, { since: "2026-11-05", pr: 11 }, deps(sessions)); + expect(fix.breakages.map((b) => b.culprit?.number)).toEqual([10, 2]); expect(one.unattributed).toBeNull(); // The unreadable lines were an unattributed session's, which a narrowed report leaves out. expect(one.incomplete).toEqual([]);