diff --git a/README.md b/README.md index 067227ad..2166eab1 100644 --- a/README.md +++ b/README.md @@ -579,9 +579,15 @@ runs `quest land apply `, which refuses a moved head, rebuilds and checks th tree, and prints an unpushed commit and its push command. A resolution too large for the comment hands back plainly. A handback disables auto-merge and carries the reason marker `quest status` -reads. Each maintainer pick gets one attempt, spent only when a fixer job blames -the PR: a PR that only shared a broken batch keeps it. A blamed PR whose pick already -spent its attempt hands back. A new maintainer pick resets the attempt. Host provisioning and isolation +reads. Each failure a fixer job blames a PR for is a round of its maintainer pick; a PR +that only shared a broken batch has none. The fixer keeps going while each round +changes something, and a handback names the stop rule that ended it: a round with no +new commit for a failure that isn't flaky, such as a conflict requeued unchanged; the +failure a fix addressed recurring right after it; or three flaky rounds in a row, +where the fixer requeued a PR unchanged and the base passed alone. The fixer names +each failure's test or step, else the round counts the gate's check as one; a +conflict names the base it conflicts with, so one after the base moved is new. A new +maintainer pick starts a fresh history. Host provisioning and isolation belong to the host; these commands are executable paths, not shell expressions. Wrappers must expose only the supplied tree and log, never the state directory or its private bare repository. diff --git a/assets/land-fix.md b/assets/land-fix.md index 3c230ed3..60ca5033 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -25,7 +25,7 @@ PR's. You start on `base`, the batch's base. The repository also has `batch`, the failed merge, and one `pr-` branch per supplied PR at its pinned head. For each PR, requeue it unchanged if healthy (if the failure was flaky, name the -flaky test in the reason so it can be followed up), commit a +flaky test in its `failure` so it can be followed up), commit a mechanical fix on its own branch, or hand it back with diagnosis and recommendation. Allowed fixes: preserve both sides' intent in conflicts, format/lint, regenerate lockfiles/generated files, or follow a base rename. Merge base where practical. @@ -54,6 +54,13 @@ one entry per supplied PR, no extra fields. Use one of: {"number":123,"action":"fixed","head":"full 40-character SHA","reason":"one-line mechanical reason"} {"number":123,"action":"handback","diagnosis":"one line","recommendation":"one line"} +On each entry, add `"failure"`: the test or step the failure log shows +failing, on one line of at most 200 characters, such as +`"failure":"cargo test -p moq-relay: interop::render_timeout"`. Name it the +same way each time the same test or step fails, from the log rather than from +your fix: quest land keeps fixing a PR while each round makes progress, and +hands it back when the failure a fix addressed recurs right after it. + You have no GitHub credential and must not seek one. To tell a PR's maintainer something, such as what a fix changed and why, a flaky test, or the decision a handback needs, add an optional `"comment"` field to that PR's entry: Markdown of diff --git a/quest/m0/host-land/fixer.md b/quest/m0/host-land/fixer.md index 4ba4fbd5..02081866 100644 --- a/quest/m0/host-land/fixer.md +++ b/quest/m0/host-land/fixer.md @@ -24,8 +24,8 @@ back for them. - ✅ Keep as decided The quest CLI implements `quest land fix`, durable collection, validation, -lease-pushes, Review records, and one attempt per maintainer pick (the -watcher's [stop rules](/quest/m0/queue-watcher/stop.md) replace that limit). Host work +lease-pushes, Review records, and the stop rules that end a pick's fixer +rounds (`stopRule` in `src/cli/land-fixer.ts`). Host work remains in kixelated/nixos: the ephemeral fixer container (`qland-fix`) reaches CLIProxyAPI and, like the check containers, the public internet without private ranges (`modules/ci-egress.nix`), so its builds fetch dependencies; diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index ece4a497..0b6481f0 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -36,7 +36,7 @@ moq-dev/rust2, moq-dev/rind and moq-dev/quest. | 16 | fixer disappeared before recording an exit status | fixed on the host; none after 2026-10-31 04:00Z | | 7 | fixer repository exceeds its size cap | fixed by #332 | | 6 | AGENTS.md "pull the latest origin changes" read as an injection | fixed by [#325](https://github.com/moq-dev/quest/pull/325) and [#328](https://github.com/moq-dev/quest/pull/328); none after 2026-10-31 18:46Z | -| 98 | "already used its mechanical fix attempt" after a collateral requeue or an earlier conflict fix; each needed a manual re-pick | [Attempt budget](/quest/m0/queue-watcher/stop.md), batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) | +| 98 | "already used its mechanical fix attempt" after a collateral requeue or an earlier conflict fix; each needed a manual re-pick | stop rules ([moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)), batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) | | ~75 | "requeued unchanged": collateral of a batch another PR broke | batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) | | 36 | picked again while fixing; the newer pick is kept, so nothing is lost | not planned | | 17 | conflicts in protected paths (`flake.nix`, `.github/`), mostly mechanical; resolved by hand on moq-dev/moq#7344, #7315, #7318 | blocked by design; fixer-written protected edits become a ready commit ([Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md)) | @@ -53,7 +53,8 @@ Decision prompts, selected options marked ✅: - ✅ Broader: also fold batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) and [Local mergeability](/quest/m0/queue-watcher/bad-picks.md) into this epic -- How attempts count: see [Attempt budget](/quest/m0/queue-watcher/stop.md). +- How attempts count: unlimited while progressing, with three stop rules + ([moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)). - How the fixer gets dependencies: - Host prefetches the job's lockfile dependencies and mounts them read-only - ✅ Allow registry network. The host already gave the fixer container @@ -116,8 +117,8 @@ Decision prompts, selected options marked ✅: the daemon's markers, `quest status`) · keep the token - Flaky tests: ✅ disable only a rerun-proven flaky test, in a separate PR · keep the fixer's judgment and the base PR - - New work, all picked: ship the attempt budget (now - [Stop rules](/quest/m0/queue-watcher/stop.md)) right after triage; the + - New work, all picked: ship the attempt budget (now the stop rules, + [moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)) right after triage; the batch check ([moq-dev/quest#379](https://github.com/moq-dev/quest/pull/379)); pre-checks ([#377](https://github.com/moq-dev/quest/pull/377)); and tuning ([#376](https://github.com/moq-dev/quest/pull/376)). All three were @@ -132,7 +133,8 @@ Decision prompts, selected options marked ✅: through GitHub's merge queue one PR at a time, and a host watcher runs this fixer on each PR the queue drops and re-enqueues it. Batching is gone, so this epic keeps only the fixer's own quality work. Moved to the watcher: - the attempt budget (now [Stop rules](/quest/m0/queue-watcher/stop.md)), + the attempt budget (now the stop rules, + [moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)), local mergeability and CI-failure parking (now [Bad picks](/quest/m0/queue-watcher/bad-picks.md)), and flaky tests (now [Flaky failures](/quest/m0/queue-watcher/flaky.md), without the rerun). diff --git a/quest/m0/queue-watcher/README.md b/quest/m0/queue-watcher/README.md index 39aed80e..9f75c951 100644 --- a/quest/m0/queue-watcher/README.md +++ b/quest/m0/queue-watcher/README.md @@ -69,10 +69,12 @@ Decision prompts, selected options marked ✅: passed - The front - Attempts: - - ✅ Unlimited while progressing; see - [Stop rules](/quest/m0/queue-watcher/stop.md). This replaces batch - triage's ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) - one attempt per pick. + - ✅ Unlimited while progressing, handing back on a round with no new + commit for a failure that isn't flaky, the failure a fix addressed + recurring right after it, or three flaky rounds in a row. This replaces + batch triage's ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) + one attempt per pick; the host gate applies the same rules + ([moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)). - Flaky failures: - ✅ The fixer re-enqueues them unchanged, and a test seen flaking twice is disabled in a separate PR with a follow-up quest; the rerun before blame @@ -110,7 +112,6 @@ Shared shape, for the children to follow: - [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) - `quest merge --land` enqueues the pinned head into GitHub's merge queue and records the pick where the watcher reads it - [Queue removals](/quest/m0/queue-watcher/removals.md) - the watcher sees each PR the queue drops, why, and the failing build's log - [Fix and re-enqueue](/quest/m0/queue-watcher/fix.md) - the fixer runs on each dropped PR in the background, and the watcher re-enqueues it at the back -- [Stop rules](/quest/m0/queue-watcher/stop.md) - the fixer keeps going while each round changes something, and hands back on a no-op fix, a repeated failure, or three flaky re-enqueues - [Bad picks](/quest/m0/queue-watcher/bad-picks.md) - a pick GitHub refuses to enqueue is merged locally with the quest driver or fixed, then pushed and enqueued - [Flaky failures](/quest/m0/queue-watcher/flaky.md) - a flaky failure is re-enqueued unchanged, and a test seen flaking twice is disabled in a separate PR with a follow-up quest - [Watcher on the host](/quest/m0/queue-watcher/host.md) - kixelated/nixos runs the watcher for moq-dev/moq beside its fixer container diff --git a/quest/m0/queue-watcher/bad-picks.md b/quest/m0/queue-watcher/bad-picks.md index 207114e8..e5f5f8c4 100644 --- a/quest/m0/queue-watcher/bad-picks.md +++ b/quest/m0/queue-watcher/bad-picks.md @@ -41,7 +41,7 @@ since on a merge-queue base such a PR is a pick GitHub won't enqueue. push. A clean driver merge needs no review: it is a conflict-free base merge. - Fetch failing required checks' logs through the App's `checks: read` and `actions: read`, trimmed like a queue failure's. -- The same [stop rules](/quest/m0/queue-watcher/stop.md) apply. +- The same stop rules apply (`stopRule` in `src/cli/land-fixer.ts`). ## Required diff --git a/quest/m0/queue-watcher/fix.md b/quest/m0/queue-watcher/fix.md index 846c616e..3387aa83 100644 --- a/quest/m0/queue-watcher/fix.md +++ b/quest/m0/queue-watcher/fix.md @@ -32,8 +32,17 @@ going while each round makes progress. queue's builds in `quest gate`, and pass an explicit `model_reasoning_effort` (default `medium`, with a daemon option), since fixer logs show `reasoning effort: none`. -- When to stop is [Stop rules](/quest/m0/queue-watcher/stop.md); flaky - handling is [Flaky failures](/quest/m0/queue-watcher/flaky.md). +- Apply the stop rules the host gate already applies + ([moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)): record + each round in the pick's history with `saveRound` and ask `stopRule` + (`src/cli/land-fixer.ts`) whether it hands back, as `collectJob` does. A + round is `fixed` for a fixer commit, `merged` for a clean quest-driver merge + of a conflict, `flaky` for the fixer's flaky verdict, and `unchanged` for + any other round with no new commit. Its failure signature is the test or + step the fixer names in its result's `failure`, falling back to the failing + job and step of the `merge_group` run; a conflict's is `conflictFailure` + of the base it conflicted with. A rule's handback posts `stopReason`. + Flaky handling is [Flaky failures](/quest/m0/queue-watcher/flaky.md). ## Required diff --git a/quest/m0/queue-watcher/flaky.md b/quest/m0/queue-watcher/flaky.md index cd067c33..2c480836 100644 --- a/quest/m0/queue-watcher/flaky.md +++ b/quest/m0/queue-watcher/flaky.md @@ -38,8 +38,9 @@ apply (2026-11-01, user and maintainer): What counts: the second flaky verdict for the same test, across any PRs, in the watcher's state. A single verdict only re-enqueues, since disabling on one judgment alone risks hiding a real bug. Each verdict names the test in the -same signature [Stop rules](/quest/m0/queue-watcher/stop.md) uses, and three -flaky re-enqueues of one PR in a row hand it back there. +same signature the stop rules use (the fixer result's `failure`), and the +third flaky round of one PR in a row hands it back there (`stopRule` in +`src/cli/land-fixer.ts`). The fixer container holds no GitHub credentials, so the fixer returns the disabling commit in its result and the watcher pushes it to a new branch and diff --git a/quest/m0/queue-watcher/stop.md b/quest/m0/queue-watcher/stop.md deleted file mode 100644 index 24f6ce02..00000000 --- a/quest/m0/queue-watcher/stop.md +++ /dev/null @@ -1,41 +0,0 @@ -# [S] The fixer keeps going while it makes progress - -## Goal - -The watcher keeps fixing and re-enqueuing a dropped PR, with no attempt -limit, for as long as each round changes something. It hands the PR back to -a person when: - -- a fix attempt produces no new commit and the failure isn't flaky; -- the same failure (the same failing test or step) recurs right after a fix; -- or the PR has been re-enqueued unchanged as flaky 3 times in a row. - -The fixer handing back on its own (a design decision, a protected edit) still -hands back at once. A handback comment names the rule that fired. - -## Plan - -Decided 2026-11-01 (maintainer, /quest-plan; see the -[epic](/quest/m0/queue-watcher/README.md)): - -- How attempts count: - - ✅ Unlimited while progressing, with the three stop rules above. This - replaces batch triage's ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) - one attempt per pick, and the earlier plan to spend that attempt only on - a failed fix (2026-11-01, from the audit of 98 "already used its - mechanical fix attempt" handbacks), which this quest held before the - switch. - -Extend the attempt record `fix-attempt-.json` (`src/cli/land-fixer.ts`, -read by `spent` and `handbackRepeated`) into a per-pick history: each round's -outcome (`fixed`, `merged`, `unchanged`, `flaky`, or a handback), the head -before and after, and the failure's signature. The signature is the failing -test or step the fixer names, falling back to the failing job and step from -the `merge_group` run. A record written by the host gate, without these -fields, starts a fresh history. - -A conflict after the base moved is a new failure, not a repeat, so a PR that -keeps conflicting with a busy base keeps being fixed while each fix lands a -new merge. A flaky round resets neither of the other rules' state; three -flaky rounds in a row hand back so a real nondeterministic bug in the PR -reaches a person. diff --git a/src/cli/land-fix.ts b/src/cli/land-fix.ts index fd14c82b..ed278397 100644 --- a/src/cli/land-fix.ts +++ b/src/cli/land-fix.ts @@ -25,13 +25,16 @@ export function readFixResult(root: string): string { } /** The longest Markdown comment a result may ask the daemon to post on its PR. */ export const COMMENT_LIMIT = 4000; +/** The longest failure signature a result may name, short enough to quote in a handback. */ +const FAILURE_LIMIT = 200; /** * The fixer's verdict on one PR. `blame` names a PR whose own change caused the failure; a blamed PR * is fixed or handed back, never requeued. A handback's `log` cites the failure log's lines, first - * to last, that show the failure, which its comment quotes. + * to last, that show the failure, which its comment quotes. `failure` names the failing test or step, + * the signature the stop rules compare across a pick's rounds. */ -export type FixResult = { agent?: string; model?: string; blame?: boolean; comment?: string } & ( +export type FixResult = { agent?: string; model?: string; blame?: boolean; comment?: string; failure?: string } & ( | { number: number; action: "requeue"; reason: string } | { number: number; action: "fixed"; head: string; reason: string } | { number: number; action: "handback"; diagnosis: string; recommendation: string; log?: [number, number] } @@ -86,6 +89,16 @@ function validate(item: any, pr: boolean): void { throw new Error("invalid fixer log lines"); metadata.push("log"); } + if (pr && "failure" in item) { + if ( + typeof item.failure !== "string" || + !item.failure.trim() || + item.failure.length > FAILURE_LIMIT || + /[\u0000-\u001f\u007f]/.test(item.failure) + ) + throw new Error("invalid fixer failure"); + metadata.push("failure"); + } // One optional Markdown comment per PR: multiline, but no other control characters. if (pr && "comment" in item) { if ( diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 5630834e..1ffe31f1 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -21,13 +21,10 @@ import { baseRemote, commandEnvironment, contains, - eject, - ejection, git, tryGit, enqueue, type Entry, - type Ejection, type Plan, type Reason, } from "./land"; @@ -137,35 +134,105 @@ interface Job extends Process { // Persist publication before moving a remote head; retries recognize only this exact head. published: number[]; } -/** A maintainer pick's fixer attempt; a fixed head's requeue keeps it as `requeued`. */ -export interface Attempt { +/** + * How a round ended: a fixer commit (`fixed`), a clean base merge with no fixer edit (`merged`), no new + * commit for a failure the fixer did not judge flaky (`unchanged`), an unchanged re-enqueue of a failure + * it judged flaky (`flaky`), or a handback. + */ +export type Outcome = "fixed" | "merged" | "unchanged" | "flaky" | "handback"; + +/** One failure a pick was blamed for, and what came of it. */ +export interface Round { + /** The fixer job that ran it: collecting the same job again replaces its round rather than adding one. */ + job: string; + outcome: Outcome; + /** The head that failed. */ + before: string; + /** The head the PR went back to the queue at, which a fix or merge moves; a handback has none. */ + after?: string; + /** The failure's signature: what failed, so a recurrence after a fix can be told from a new failure. */ + failure: string; + /** The stop rule that handed the PR back. */ + rule?: StopRule; +} + +/** + * A maintainer pick's history of fixer rounds; a fixed head's requeue keeps the pick as `requeued`. + * A record without `rounds`, written before rounds were kept, starts a fresh history. + */ +export interface History { agent?: string; model?: string; pick: string; requeued?: string; head?: string; - /** - * The job that blamed this pick, which spends its attempt, or false while it was only collateral of - * other PRs' failures. A record from before blame was tracked has none, and counts as spent. - */ - blamed?: string | false; + rounds: Round[]; } function save(path: string, data: unknown): void { writeFileSync(`${path}.new`, JSON.stringify(data), { mode: 0o600 }); renameSync(`${path}.new`, path); } -const attemptPath = (state: string, number: number) => join(state, `fix-attempt-${number}.json`); -export function attempt(state: string, number: number): Attempt | undefined { - const path = attemptPath(state, number); - return existsSync(path) ? (JSON.parse(readFileSync(path, "utf8")) as Attempt) : undefined; +const historyPath = (state: string, number: number) => join(state, `fix-attempt-${number}.json`); +export function history(state: string, number: number): History | undefined { + const path = historyPath(state, number); + if (!existsSync(path)) return undefined; + const record = JSON.parse(readFileSync(path, "utf8")) as Omit & { rounds?: Round[] }; + return { ...record, rounds: record.rounds ?? [] }; +} +/** Record `round` in `record`, replacing the same job's earlier one, and save it as PR `number`'s history. */ +export function saveRound(state: string, number: number, record: History, round: Round): History { + const next = { ...record, rounds: [...record.rounds.filter((item) => item.job !== round.job), round] }; + save(historyPath(state, number), next); + return next; } /** Whether `record` belongs to the pick `entry` is queued under: the pick itself, or the fixer's requeue of it. */ -function samePick(record: Attempt | undefined, entry: Pick): record is Attempt { +function samePick(record: History | undefined, entry: Pick): record is History { return record !== undefined && (record.pick === entry.enabledAt || record.requeued === entry.enabledAt); } -/** Whether the pick already spent its attempt on a job other than `job`: only being blamed spends it. */ -function spent(record: Attempt | undefined, entry: Pick, job?: string): boolean { - return samePick(record, entry) && record.blamed !== false && (job === undefined || record.blamed !== job); + +/** + * Why a pick stops being fixed while each round still changes something: a round with no new commit + * for a failure that isn't flaky (`unchanged`), the failure a fix addressed recurring right after it + * (`repeated`), or `FLAKY_LIMIT` flaky rounds in a row (`flaky`). + */ +export type StopRule = "unchanged" | "repeated" | "flaky"; +/** The flaky rounds in a row that hand a PR back, so a real nondeterministic bug in it reaches a person. */ +export const FLAKY_LIMIT = 3; + +/** + * The stop rule `round` fires after `rounds`, the pick's earlier rounds, if any. A flaky round resets + * neither of the other rules: the failure a fix addressed recurring after flaky rounds is still a + * repeat. A fixer's own handback needs no rule. + */ +export function stopRule(rounds: Round[], round: Pick): StopRule | undefined { + if (round.outcome === "handback") return undefined; + if (round.outcome === "unchanged") return "unchanged"; + const last = rounds.filter((item) => item.outcome !== "flaky").at(-1); + if ((last?.outcome === "fixed" || last?.outcome === "merged") && last.failure === round.failure) return "repeated"; + if (round.outcome !== "flaky") return undefined; + let streak = 1; + for (let index = rounds.length - 1; index >= 0 && rounds[index].outcome === "flaky"; index--) streak++; + return streak >= FLAKY_LIMIT ? "flaky" : undefined; +} + +/** A handback's reason, naming the stop rule that fired; worded so `fixerText` quotes it unchanged. */ +export function stopReason(rule: StopRule, failure: string): string { + switch (rule) { + case "unchanged": + return "stop rule (no new commit): the fixer's round produced no new commit and the failure isn't flaky"; + case "repeated": + return `stop rule (repeated failure): ${failure} failed again right after the round that addressed it`; + case "flaky": + return `stop rule (flaky limit): the failure was judged flaky ${FLAKY_LIMIT} times in a row; it may be a real nondeterministic bug`; + } +} + +/** + * A conflict's signature names the base it conflicts with, so a conflict after the base moved is a new + * failure rather than a repeat, and a PR that keeps conflicting with a busy base keeps being fixed. + */ +export function conflictFailure(base: string): string { + return `merge conflict with ${base}`; } function identity(pid: number): string | undefined { try { @@ -213,32 +280,12 @@ async function comment( await post(github, plan, entry, content, suffix, attribution, reason, section); } -/** Queue eligibility can eject a previously requeued PR before building another batch. */ /** How many one-second pauses to wait for GitHub to show a pushed head. */ const SYNC_PAUSES = 60; -export async function handbackRepeated( - state: string, - plan: Plan, - entry: Ejection | Entry, - deps: GitHubDependencies, -): Promise { - const previous = attempt(state, entry.number); - if (!spent(previous, entry)) return false; - await comment( - new GitHub(deps), - plan, - entry, - "This maintainer pick already used its mechanical fix attempt. Run /quest-iterate to diagnose the repeated failure, then pick it again.", - "handback", - author(previous!), - ); - return true; -} - /** - * Park the failure's PRs and start one fixer job on them. A PR the gate already blames, as for a - * conflict, whose pick spent its attempt is handed back instead. Returns what it did. + * Park the failure's PRs and start one fixer job on them; the stop rules are applied as it is + * collected, once its outcome and the failure's signature are known. Returns what it did. */ export async function startFixer( root: string, @@ -248,27 +295,18 @@ export async function startFixer( state: string, deps: GitHubDependencies, ): Promise { - const lines: string[] = []; const entries: Entry[] = []; const held = parked(state); for (const entry of failure.plan.queue.filter((item) => failure.numbers.includes(item.number))) { // failure.json may replay after launching this same job. It is not a second pick. if (isParked(held, entry)) continue; - const previous = attempt(state, entry.number); - if (failure.blame?.includes(entry.number) && spent(previous, entry)) { - const detail = - "this maintainer pick already used its mechanical fix attempt. Run /quest-iterate to diagnose the repeated failure, then pick it again"; - if (await eject(new GitHub(deps), failure.plan, ejection(entry, failure.reason ?? "failed_checks", detail))) - lines.push(`handback ${entry.number}`); - continue; - } - if (!samePick(previous, entry)) - save(attemptPath(state, entry.number), { pick: entry.enabledAt, blamed: false }); + if (!samePick(history(state, entry.number), entry)) + save(historyPath(state, entry.number), { pick: entry.enabledAt, rounds: [] } satisfies History); entries.push(entry); } if (!entries.length) { rmSync(checkout, { recursive: true, force: true }); - return lines; + return []; } const packs = prepare( root, @@ -297,8 +335,7 @@ export async function startFixer( published: [], }; await launch(path, job, fixer, failure.log, job.numbers); - for (const number of job.numbers) lines.push(`parked ${number}`); - return lines; + return job.numbers.map((number) => `parked ${number}`); } /** @@ -755,7 +792,7 @@ async function collectJob( { id: entry.id }, ); const auto = object(queued.node).autoMergeRequest; - let previous = attempt(state, number)!; + let previous = history(state, number)!; // The head the PR stands at: its pinned head, or the gate's own base merge on it for this pick, as // when the gate pushed that merge and then failed to land it (moq-dev/quest#342). The gate's merge is // known only from its private record, never from its forgeable author or message. @@ -783,17 +820,37 @@ async function collectJob( error ??= "PR left the queue while fixing; its result was not applied"; else if (queuedAs !== null && queuedAs !== entry.enabledAt && !requeued) error = "PR was picked again while fixing; keeping the newer pick"; - // Only a blamed PR spends the pick's attempt; collateral of another PR's failure never does. - if (blamed && spent(previous, entry, name)) - error ??= - "This maintainer pick already used its mechanical fix attempt. Run /quest-iterate to diagnose the repeated failure, then pick it again"; + // Only a blamed PR's failure is a round of its pick; collateral of another PR's failure never is. A + // conflict names its base; otherwise the fixer names the failure, or the gate's one check step does. + let round: Round | undefined; + if (blamed && !error) { + const failure = + job.reason === "merge_conflict" + ? conflictFailure(job.plan.base.sha) + : (result?.failure ?? job.reason ?? "failed_checks"); + // A PR requeued unchanged is flaky by the fixer's judgment, unless the gate itself blamed it, as for a + // conflict that is still there: a blamed PR the fixer named is never requeued. + const outcome: Outcome = + result?.action === "fixed" + ? "fixed" + : result?.action === "requeue" + ? job.blame + ? "unchanged" + : "flaky" + : "handback"; + const rule = stopRule( + previous.rounds.filter((item) => item.job !== name), + { outcome, failure }, + ); + if (rule) error = stopReason(rule, failure); + round = rule + ? { job: name, outcome: "handback", before: standing, failure, rule } + : { job: name, outcome, before: standing, failure, ...(outcome === "handback" ? {} : { after: head }) }; + previous = saveRound(state, number, previous, round); + } if (requeued) { - save(attemptPath(state, number), { ...previous, requeued: queuedAs }); + save(historyPath(state, number), { ...previous, requeued: queuedAs }); } else if (!error && result?.action !== "handback") { - if (blamed && previous.blamed !== name) { - previous = { ...previous, blamed: name }; - save(attemptPath(state, number), previous); - } try { if (result?.action === "fixed") { if (entry.repository !== job.plan.repository && !entry.maintainerCanModify) @@ -870,14 +927,14 @@ async function collectJob( // A parked PR requeued unchanged is still queued as it was; anything else is pinned anew. if (queuedAs !== entry.enabledAt || head !== entry.head) { // Save intent before enabling, so a lost response can be reconciled after restart. - save(attemptPath(state, number), { ...previous, ...actor, head }); + save(historyPath(state, number), { ...previous, ...actor, head }); await enqueue(github, entry.id, head); const enabled = await github.graphql( "query($id: ID!) { node(id: $id) { ... on PullRequest { autoMergeRequest { enabledAt } } } }", { id: entry.id }, ); const enabledAt = string(object(object(enabled.node).autoMergeRequest).enabledAt); - save(attemptPath(state, number), { ...previous, ...actor, head, requeued: enabledAt }); + save(historyPath(state, number), { ...previous, ...actor, head, requeued: enabledAt }); } } catch (failure) { if (!(failure instanceof FixerRefusal)) throw failure; @@ -896,8 +953,11 @@ async function collectJob( ); const handback = error !== undefined || result?.action === "handback"; - if (handback && blamed && !error && previous.blamed !== name) - save(attemptPath(state, number), { ...previous, blamed: name }); + // A refused fix or requeue ends the round in a handback after all. + if (handback && round && round.outcome !== "handback") { + const { after: _, ...rest } = round; + saveRound(state, number, history(state, number)!, { ...rest, outcome: "handback" }); + } if (error) { // Before leaving the queue, so a restart keeps this reason rather than finding it left. job.handbacks ??= {}; diff --git a/src/cli/land-park.ts b/src/cli/land-park.ts index 10c2479c..ab4a746a 100644 --- a/src/cli/land-park.ts +++ b/src/cli/land-park.ts @@ -1,7 +1,7 @@ // Parking: a PR the fixer is working on stays queued but sits out batches. import { readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import { attempt } from "./land-fixer"; +import { history } from "./land-fixer"; import type { Entry, Plan } from "./land"; /** @@ -39,12 +39,12 @@ export function isParked(held: Map>, entry: Pick { - const record = attempt(state, entry.number); + const record = history(state, entry.number); return record?.requeued === entry.enabledAt ? record.pick : entry.enabledAt; }; const queue = plan.queue diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 63d3233d..3fc6d747 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -1,6 +1,6 @@ // Host orchestration: PR code runs only through the configured sandbox. import { authenticatedRead } from "./land-git"; -import { startFixer, startBaseFixer, collectFixers, handbackRepeated } from "./land-fixer"; +import { startFixer, startBaseFixer, collectFixers } from "./land-fixer"; import { closeFix, fixing, haltNotice, readHalt, resume, saveHalt } from "./land-halt"; import { batchable } from "./land-park"; import { spawn } from "node:child_process"; @@ -533,10 +533,7 @@ export async function serveBatch( if (await eject(github, plan, item)) output.push(`ejected ${item.number} ${item.reason}`); forgetHeads(options.state, item.number); }; - for (const item of plan.eject) { - await report(item); - await handbackRepeated(options.state, plan, item, deps); - } + for (const item of plan.eject) await report(item); output.push(...(await reportStalled(github, plan, options.state, deps.now()))); // A halted queue waits for its base to move, then resumes once the new base passes alone, even // with no PR ready to batch, so its issue and any fix it no longer needs are closed at once. diff --git a/test/land.test.ts b/test/land.test.ts index 79feb75c..46a2c5e7 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -29,8 +29,11 @@ import { validateFix, fixerMarkdown, fixerText, - handbackRepeated, + FLAKY_LIMIT, + history, redactTokens, + type Round, + stopRule, } from "../src/cli/land-fixer"; import { importFixer } from "../src/cli/land-fixer-repository"; import { apply, parseResolution, pushCommand, resolutionSection } from "../src/cli/land-apply"; @@ -2530,7 +2533,7 @@ describe.skipIf(process.platform === "win32")("asynchronous land fixer", () => { expect(JSON.parse(readFileSync(join(fixture.state.state, "halt.json"), "utf8"))).toEqual(halt); expect(fixture.jobs()).toEqual([]); }); - test("requeues a parked PR unchanged, and hands back a second failure it is blamed for in the same pick", async () => { + test("requeues a PR judged flaky unchanged, and hands it back after three flaky rounds in a row", async () => { const fixture = await fixerFixture(requeueScript); await fixture.finished; expect(await fixture.collect()).toEqual(["requeued 1"]); @@ -2556,14 +2559,28 @@ describe.skipIf(process.platform === "win32")("asynchronous land fixer", () => { ).toEqual(["parked 1"]); await finished; }; - // The fixer blamed no PR and the base passed alone, so the pick spent its attempt. + // The fixer blamed no PR and the base passed alone each time: flaky rounds, re-enqueued unchanged. + for (let round = 2; round < FLAKY_LIMIT; round++) { + await fail(); + expect(await fixture.collect()).toEqual(["requeued 1"]); + } await fail(); expect(await fixture.collect()).toEqual(["handback 1"]); expect(fixture.jobs()).toEqual([]); expect(fixture.pull.landing).toBe(false); - expect(fixture.forge.comment(1)).toContain("already used its mechanical"); + expect(fixture.forge.comment(1)).toContain("stop rule (flaky limit)"); expect(fixture.forge.comment(1)).toContain(""); - // A new maintainer pick grants a new attempt. + const rounds = history(fixture.state.state, 1)!.rounds; + expect(rounds.map(({ outcome, rule }) => [outcome, rule])).toEqual([ + ...Array.from({ length: FLAKY_LIMIT - 1 }, () => ["flaky", undefined]), + ["handback", "flaky"], + ]); + expect(rounds[0]).toMatchObject({ + before: fixture.pull.head, + after: fixture.pull.head, + failure: "failed_checks", + }); + // A new maintainer pick starts a fresh history. fixture.pull.landing = true; fixture.pull.autoMerge = { enabledAt: "2099-01-01T00:00:00Z", commitBody: pin(fixture.pull.head!) }; await fail(); @@ -2661,14 +2678,16 @@ printf '{"action":"fixed","head":"%s","reason":"edit the flake"}' "$(git rev-par ); expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); }); - test("hands back a conflict at once when its pick already spent its attempt on a fix", async () => { + test("keeps fixing a PR that conflicts again after the base moved, as a new failure", async () => { const fixture = await fixerFixture(fixedScript(), { conflict: true }); await fixture.finished; expect(await fixture.collect(await fixture.forge.deps())).toEqual(["requeued 1"]); - // The fixed head conflicts again as the base moves; the gate blames it without asking the fixer. + const first = fixture.pull.head!; + // The fixed head conflicts again as the base moves: a new failure, so the fixer runs again. fixture.forge.advance({ "code.txt": "one\nthree\n" }); const deps = await fixture.forge.deps(); const next = await resolveQueue(REPOSITORY, undefined, deps); + const finished = eventFile(fixture.state.state, (name) => name.endsWith(".exit")); expect( await serveBatch( fixture.root, @@ -2677,11 +2696,66 @@ printf '{"action":"fixed","head":"%s","reason":"edit the flake"}' "$(git rev-par deps, async () => deps.token, ), - ).toEqual(["handback 1"]); - expect(fixture.jobs()).toEqual([]); + ).toEqual(["parked 1"]); + await finished; + expect(await fixture.collect(deps)).toEqual(["requeued 1"]); + expect(fixture.pull.landing).toBe(true); + expect(git(fixture.root, "show", "-s", "--format=%P", fixture.pull.head!)).toContain(first); + const rounds = history(fixture.state.state, 1)!.rounds; + expect(rounds.map(({ outcome, before, after }) => [outcome, before, after])).toEqual([ + ["fixed", fixture.plan.queue[0].head, first], + ["fixed", first, fixture.pull.head], + ]); + expect(rounds.map(({ failure }) => failure)).toEqual([ + `merge conflict with ${fixture.plan.base.sha}`, + `merge conflict with ${next.base.sha}`, + ]); + }); + test("hands back a conflict the fixer requeued unchanged: a round with no new commit", async () => { + const fixture = await fixerFixture(requeueScript, { conflict: true }); + await fixture.finished; + expect(await fixture.collect()).toEqual(["handback 1"]); expect(fixture.pull.landing).toBe(false); - expect(fixture.forge.comment(1)).toContain("already used its mechanical fix attempt"); + expect(fixture.forge.comment(1)).toContain("stop rule (no new commit)"); expect(fixture.forge.comment(1)).toContain(""); + expect(history(fixture.state.state, 1)!.rounds).toMatchObject([{ outcome: "handback", rule: "unchanged" }]); + }); + test("hands back a failure the fixer names again right after its fix", async () => { + const marker = join(tempDir(), "fixed"); + const fixture = await fixerFixture(`if [ -e '${marker}' ]; then +printf '[{"number":1,"action":"requeue","reason":"looks flaky","failure":"cargo test: render"}]' > quest-land-result.json +else +touch '${marker}' +git checkout -q pr-1 +printf 'one\nII fixed\nthree\n' > code.txt +git commit -qam 'mechanical fix' +printf '[{"number":1,"action":"fixed","head":"%s","reason":"fix render","failure":"cargo test: render"}]' "$(git rev-parse HEAD)" > quest-land-result.json +fi`); + await fixture.finished; + expect(await fixture.collect()).toEqual(["requeued 1"]); + const fixed = fixture.pull.head!; + const next = await resolveQueue(REPOSITORY, undefined, fixture.deps); + const finished = eventFile(fixture.state.state, (name) => name.endsWith(".exit")); + expect( + await serveBatch( + fixture.root, + next, + { ...fixture.state, fixer: fixture.wrapper }, + fixture.deps, + async () => fixture.deps.token, + ), + ).toEqual(["parked 1"]); + await finished; + // The same failure recurs right after its fix: judging it flaky now does not keep it going. + expect(await fixture.collect()).toEqual(["handback 1"]); + expect(fixture.pull.head).toBe(fixed); + expect(fixture.pull.landing).toBe(false); + expect(fixture.forge.comment(1)).toContain("stop rule (repeated failure)"); + expect(fixture.forge.comment(1)).toContain("cargo test: render"); + expect(history(fixture.state.state, 1)!.rounds).toMatchObject([ + { outcome: "fixed", failure: "cargo test: render" }, + { outcome: "handback", rule: "repeated", before: fixed }, + ]); }); test("accepts a fix whose base gained protected changes before collection", async () => { const fixture = await fixerFixture(fixedScript(), { conflict: true, protectedBase: true }); @@ -2894,10 +2968,8 @@ printf '{"action":"fixed","head":"%s","reason":"edit the flake"}' "$(git rev-par expect(collateral.labels).toBeUndefined(); expect(collateral.autoMerge?.commitBody).toBe(pin(collateral.head!)); expect(fixture.forge.comment(2)).toBeUndefined(); - // Collateral never spends an attempt, however often it shares a broken batch. - expect(await handbackRepeated(fixture.state.state, fixture.plan, fixture.plan.queue[1], fixture.deps)).toBe( - false, - ); + // Collateral is never a round of its pick, however often it shares a broken batch. + expect(history(fixture.state.state, 2)!.rounds).toEqual([]); }); const editJob = (state: string, change: (job: Record) => void) => { const path = join( @@ -3224,17 +3296,11 @@ ${requeueScript}`); await fixture.finished; expect(await fixture.collect({ ...fixture.deps, now: () => 120 * 60_000 })).toEqual(["requeued 1"]); }); - test("counts an attempt record from before blame was tracked as spent", async () => { - const forge = new Forge(); - forge.pull("One", { "one.txt": "yes" }); - const deps = await forge.deps(); - const plan = await resolveQueue(REPOSITORY, undefined, deps); + test("starts a fresh history from a record written before rounds were kept", () => { const state = tempDir(); - const [entry] = plan.queue; - writeFileSync(join(state, "fix-attempt-1.json"), JSON.stringify({ pick: entry.enabledAt })); - expect(await handbackRepeated(state, plan, entry, deps)).toBe(true); - writeFileSync(join(state, "fix-attempt-1.json"), JSON.stringify({ pick: entry.enabledAt, blamed: false })); - expect(await handbackRepeated(state, plan, entry, deps)).toBe(false); + const pick = "2026-10-26T00:00:01Z"; + writeFileSync(join(state, "fix-attempt-1.json"), JSON.stringify({ pick, blamed: "fix-job-x.json" })); + expect(history(state, 1)).toMatchObject({ pick, rounds: [] }); }); test("quotes contributor text without markup, mentions, or closing instructions", () => { const text = fixerText( @@ -3292,10 +3358,14 @@ printf '[{"number":1,"action":"handback","diagnosis":"saw %s","recommendation":" await fixture.finished; const marker = ``; (fixture.pull.comments ??= []).push({ author: "outsider", body: marker, at: "2026-10-30T00:00:00Z" }); + const [name] = fixture.jobs(); + const job = readFileSync(join(fixture.state.state, name), "utf8"); expect(await fixture.collect()).toEqual(["handback 1"]); expect(fixture.pull.comments!.at(-1)?.author).toBe("gate[bot]"); const count = fixture.pull.comments!.length; - await handbackRepeated(fixture.state.state, fixture.plan, fixture.plan.queue[0], fixture.deps); + // A retried handback of the same pick finds the bot's own marker and posts nothing. + writeFileSync(join(fixture.state.state, name), JSON.stringify({ ...JSON.parse(job), error: "retried" })); + expect(await fixture.collect()).toEqual(["handback 1"]); expect(fixture.pull.comments).toHaveLength(count); }); test("rereads the author's body immediately before recording the fix", async () => { @@ -3435,6 +3505,18 @@ printf '[{"number":1,"action":"handback","diagnosis":"saw %s","recommendation":" ), ).toHaveLength(2); }); + test("accepts a one-line failure signature on any PR result", () => { + const accepted = { number: 1, action: "requeue", reason: "fine", failure: "cargo test: interop::render" }; + expect(parseFixResult(JSON.stringify([accepted]), [1])).toEqual([accepted]); + for (const failure of [7, "", " ", "x".repeat(201), "a\nb", "tab\t"]) + expect(() => + parseFixResult(JSON.stringify([{ number: 1, action: "requeue", reason: "fine", failure }]), [1]), + ).toThrow("invalid fixer failure"); + // The base fix has no signature: nothing compares its rounds. + expect(() => + parseBaseFix(JSON.stringify({ action: "fixed", head: "a".repeat(40), reason: "fine", failure: "x" })), + ).toThrow(); + }); test("strictly accepts one fixed or handback object for the base alone", () => { const head = "a".repeat(40); for (const value of [ @@ -3699,3 +3781,41 @@ describe.skipIf(process.platform === "win32")("quest land apply", () => { expect(plain).toContain("too large for a comment"); }); }); + +describe("land fixer stop rules", () => { + const round = (outcome: Round["outcome"], failure = "test a"): Round => ({ + job: `job-${outcome}-${failure}`, + outcome, + before: "h", + failure, + }); + test("keeps going while each round changes something", () => { + expect(stopRule([], round("fixed"))).toBeUndefined(); + // A conflict on a moved base is a new failure; so is another test failing after a fix. + expect(stopRule([round("merged", "conflict with b1")], round("fixed", "conflict with b2"))).toBeUndefined(); + expect(stopRule([round("fixed", "test a")], round("fixed", "test b"))).toBeUndefined(); + // Only right after a fix: a failure fixed two rounds back may recur. + expect( + stopRule([round("fixed", "test a"), round("fixed", "test b")], round("fixed", "test a")), + ).toBeUndefined(); + // A fixer's own handback needs no rule. + expect(stopRule([round("fixed")], round("handback"))).toBeUndefined(); + }); + test("hands back a round with no new commit for a failure that isn't flaky", () => { + expect(stopRule([], round("unchanged"))).toBe("unchanged"); + }); + test("hands back the failure a fix addressed recurring right after it, through flaky rounds", () => { + expect(stopRule([round("fixed")], round("fixed"))).toBe("repeated"); + expect(stopRule([round("merged", "conflict with b1")], round("fixed", "conflict with b1"))).toBe("repeated"); + expect(stopRule([round("fixed"), round("flaky", "test b")], round("fixed"))).toBe("repeated"); + expect(stopRule([round("fixed")], round("flaky"))).toBe("repeated"); + }); + test("hands back the third flaky round in a row, which a fix in between resets", () => { + const flaky = (count: number) => Array.from({ length: count }, (_, index) => round("flaky", `test ${index}`)); + expect(stopRule(flaky(FLAKY_LIMIT - 2), round("flaky", "test x"))).toBeUndefined(); + expect(stopRule(flaky(FLAKY_LIMIT - 1), round("flaky", "test x"))).toBe("flaky"); + expect( + stopRule([...flaky(FLAKY_LIMIT - 1), round("fixed", "test y")], round("flaky", "test x")), + ).toBeUndefined(); + }); +});