diff --git a/README.md b/README.md index b7d838ec..9a41a418 100644 --- a/README.md +++ b/README.md @@ -842,6 +842,22 @@ posted with its reason and a recommendation, and the PR leaves the watcher's care until a maintainer enqueues it again. A round whose pick ended while it ran, by a writer's push or enqueue, is not applied, and its comment says so. +The watcher also keeps the repository's flaky tests in `flaky.json` in its +state directory: each flaky verdict, across every PR, by the failure signature +the fixer named, with the `merge_group` jobs that showed it. Each failed +build's round is told the tests that flaked before and are not yet disabled, in +`.git/quest-land-flaky.json` in its checkout, which `quest land fix` passes to +the agent. One verdict alone only re-enqueues the PR, since one judgment could +hide a real bug. When a listed test flakes again, the fixer also commits its +disabling on a `disable` branch from the base: the skip in the test +framework's own form, naming the follow-up quest it adds to the repository's +m0 `flaky` epic, `quest/m0/flaky/.md`, to fix or delete the test. The +watcher validates that commit as it does a base fix, requiring exactly one new +follow-up quest, pushes it to `quest-land/flaky-`, opens it as a pull +request with its Review recorded, and enqueues it like any other, with no +maintainer's pick. A refused disable waits for the test's next flake, and a +pull request GitHub refuses to enqueue says so and waits for a maintainer. + It shares `quest land serve`'s App key, state directory, private repository, and locks, so the two never run for the same repository at once. Without a fixer its token asks for `contents: read`, `pull_requests: write`, diff --git a/assets/land-fix.md b/assets/land-fix.md index 60ca5033..26b1489f 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -74,6 +74,37 @@ A fixed head must be the tip of pr-, descend from its pinned head, and contain only that PR's fix and base history. Leave all other PR branches intact. Keep the result file uncommitted. +## Flaky tests + +A failure is flaky when it comes and goes with no cause in the PR or the base, +such as a timeout under load: requeue the PR unchanged and name the test in its +`failure`. The input may list the repository's known flaky tests, each by the +`failure` an earlier round named and the `merge_group` jobs that showed it. +When the test you judge flaky is one of them, name it exactly as listed, and +disable it in a commit of its own: create a `disable` branch from `base`, and +commit on it only + +- the skip, in the test framework's own form, whose reason names the follow-up + quest, such as `#[ignore = "flaky: /quest/m0/flaky/.md"]` in Rust, + `test.skip` with that path in a comment in JavaScript, or + `pytest.mark.skip(reason="flaky: /quest/m0/flaky/.md")` in Python; +- that follow-up quest, `quest/m0/flaky/.md`, with `` the test's + name in lowercase kebab-case, whose Goal says to fix the test or delete it + and links each listed job and this failure's, which the failure log names; +- its link in the `Required` list of `quest/m0/flaky/README.md`, the epic + holding one quest per disabled test, created with a link in + `quest/m0/README.md`'s `Required` if it is missing. + +Follow the repository's quest format, and run `quest check` when the +repository has it. A disabled test that is another quest's acceptance test +leaves that quest as it is: the test still counts as its work. Build the +package the skip is in, and add the branch's full SHA to the PR's entry: +{"number":123,"action":"requeue","reason":"healthy; flaky test","failure":"cargo test -p moq-relay: interop::render_timeout","disable":"full 40-character SHA"} +quest land opens the commit as a pull request of its own and enqueues it. Never +disable a test that is not listed. When the repository keeps no `quest/m0/`, or +the skip needs a protected path or more than a mechanical edit, requeue +without `disable`. + ## Blame The supplied PRs stay queued while you work. First decide which of them caused diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index d7e72469..c4709c9d 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -42,7 +42,7 @@ moq-dev/rust2, moq-dev/rind and moq-dev/quest. | 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)) | | 17 of the last 40 jobs | `cargo` fails offline ("failed to get `uniffi`"), so Rust fixes were requeued uncompiled: moq-dev/moq#7357 sank three batches, and #6985's lockfile could not be regenerated | fixed by [moq-dev/quest#356](https://github.com/moq-dev/quest/pull/356): the prompt allows dependency fetches and requires a build and test | | 2 | GitHub refused the push to a fork because the merge touches workflows: moq-dev/moq#7439, #7467 | handed back with a ready commit for `quest land apply` by [moq-dev/quest#358](https://github.com/moq-dev/quest/pull/358) | -| several batches | a flaky ffmpeg timeout ([moq-dev/moq#7480](https://github.com/moq-dev/moq/pull/7480)) ejected whole batches | [Flaky failures](/quest/m0/queue-watcher/flaky.md) | +| several batches | a flaky ffmpeg timeout ([moq-dev/moq#7480](https://github.com/moq-dev/moq/pull/7480)) ejected whole batches | the watcher re-enqueues flaky failures and disables a test seen flaking twice ([moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406)) | | ~15 | design conflicts, correctly handed back | none | Decision prompts, selected options marked ✅: @@ -66,7 +66,7 @@ Decision prompts, selected options marked ✅: - Protected paths: the fixer first could edit them (moq-dev/quest#357), then not; see the 2026-11-01 audit below. - Fork pushes GitHub refuses: ✅ hand back a ready commit, which `quest land apply` rebuilds for a maintainer to push (Grant moq-bot the `workflows` permission · Leave it manual). -- Flaky tests: see [Flaky failures](/quest/m0/queue-watcher/flaky.md). +- Flaky tests: the watcher re-enqueues them, and disables a test seen flaking twice in a separate PR ([moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406)). - Milestone for the new quests: ✅ m0 · m1 - Restated 2026-11-01 (user, a second /quest-plan): "the merge queue can autonomously fix most issues. The fixer inspects the batch and tries to @@ -136,8 +136,8 @@ Decision prompts, selected options marked ✅: [moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)), local mergeability and CI-failure parking (then Bad picks, dropped 2026-11-02: a pick GitHub refuses fails `quest merge` - instead, and the watcher only enqueues armed picks GitHub never queued), and flaky tests (now - [Flaky failures](/quest/m0/queue-watcher/flaky.md), without the rerun). + instead, and the watcher only enqueues armed picks GitHub never queued), and flaky tests (done + without the rerun in [moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406)). Deleted: the batch quest check, pre-checks, and throughput tuning, which only served batches (the fixer cap and reasoning effort moved to the watcher, as `quest land watch --fixer-jobs` and `--fixer-effort`); and queue status in diff --git a/quest/m0/queue-watcher/README.md b/quest/m0/queue-watcher/README.md index 087aa3d7..5657097c 100644 --- a/quest/m0/queue-watcher/README.md +++ b/quest/m0/queue-watcher/README.md @@ -78,7 +78,7 @@ Decision prompts, selected options marked ✅: - 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 - (#375) is dropped. See [Flaky failures](/quest/m0/queue-watcher/flaky.md). + (#375) is dropped. Done in [moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406). - Pick record (decided 2026-11-02, replacing "Picks GitHub refuses to enqueue: ✅ The watcher fixes them"): - ✅ GitHub's records only. Drop the comment and `pick.ts`. The watcher @@ -129,7 +129,6 @@ Shared shape, for the children to follow: ## Required -- [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 - [The other repositories follow](/quest/m0/queue-watcher/follow.md) - condition: quest, rind and rust2 are cut over; rind and rust2 each land a PR, and a fork PR lands - [Harden the queue runners](/quest/m0/queue-watcher/runners.md) - each queue runner runs only its repository's queue workflow, and every outside fork PR needs approval diff --git a/quest/m0/queue-watcher/flaky.md b/quest/m0/queue-watcher/flaky.md deleted file mode 100644 index f481c64a..00000000 --- a/quest/m0/queue-watcher/flaky.md +++ /dev/null @@ -1,53 +0,0 @@ -# [M] A flaky failure is re-enqueued, and a twice-flaky test is disabled - -## Goal - -When the fixer judges a dropped PR's failure flaky, the watcher re-enqueues -the PR unchanged. The watcher keeps a per-repository record of flaky -verdicts by test. A test seen flaking twice is disabled in a separate PR that -also adds a follow-up quest to fix or delete it in the repository's m0 -`flaky` epic; the watcher enqueues that PR like any other, with no human pick -needed. - -## Plan - -Decided 2026-11-01 (maintainer, /quest-plan; see the -[epic](/quest/m0/queue-watcher/README.md)): - -- 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 host gate's rerun - before blame ([moq-dev/quest#375](https://github.com/moq-dev/quest/pull/375), - and moq's module naming for it, - [moq-dev/moq#7588](https://github.com/moq-dev/moq/pull/7588)) is dropped; - both PRs are closed. - -Carried over from the host gate's flaky-test quest, whose decisions still -apply (2026-11-01, user and maintainer): - -- A flaky ffmpeg render timeout in moq's interop tests ejected whole batches - on 2026-11-01, and its fix - ([moq-dev/moq#7480](https://github.com/moq-dev/moq/pull/7480)) was written - by hand. The user asked for "a rule to immediately disable any flakey tests - and create a follow-up to fix/delete them". -- The disable goes in its own PR, never inside another PR's commit, since PRs - land as squashes and an unrelated PR's commit would carry it. -- The follow-up goes in a standing `flaky` epic in the repository's m0, one - quest per disabled test. - -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 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 -opens the PR as moq-bot, the way the host gate opened its -`quest-land/fix-` base fixes. The fixer writes the skip in the test framework's own form, with the reason -naming the follow-up quest: `#[ignore = "flaky: /quest/m0/flaky/.md"]`, -`test.skip`, `pytest.mark.skip`, and so on. The follow-up quest's Goal says to -fix the test or delete it, and links the `merge_group` runs that showed the -flake. A disabled test that is a quest's acceptance test still counts as its -quest's work. diff --git a/src/cli/land-fix.ts b/src/cli/land-fix.ts index 9883c034..c16bfb2f 100644 --- a/src/cli/land-fix.ts +++ b/src/cli/land-fix.ts @@ -1,7 +1,7 @@ // Untrusted agent side. The host wrapper supplies only a model endpoint and its client key; the agent holds no // GitHub credential, and the daemon posts the comments it writes into its result. import { spawn, spawnSync } from "node:child_process"; -import { constants, closeSync, fstatSync, openSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { constants, closeSync, existsSync, fstatSync, openSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { basename, join, resolve } from "node:path"; import prompt from "../../assets/land-fix.md"; import { programEnvironment, requireProgram } from "./program"; @@ -13,16 +13,52 @@ export class FixerAgentError extends Error { } export const FIX_RESULT = "quest-land-result.json"; -export function readFixResult(root: string): string { - const fd = openSync(join(root, FIX_RESULT), constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); +/** + * The repository's known flaky tests, which a daemon may write in the checkout's `.git` before a round, + * where no commit can carry it: a JSON array of `KnownFlaky`. + */ +export const FLAKY_INPUT = "quest-land-flaky.json"; + +/** A test the fixer judged flaky before, by the `failure` it named, with the `merge_group` jobs that showed it. */ +export interface KnownFlaky { + failure: string; + jobs: string[]; +} + +/** A regular file of at most 100 KB, never through a link. */ +function readBounded(path: string, what: string): string { + const fd = openSync(path, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); try { const stat = fstatSync(fd); - if (!stat.isFile() || stat.size > 100_000) throw new Error("fixer result must be a regular file under 100 KB"); + if (!stat.isFile() || stat.size > 100_000) throw new Error(`${what} must be a regular file under 100 KB`); return readFileSync(fd, "utf8"); } finally { closeSync(fd); } } + +export function readFixResult(root: string): string { + return readBounded(join(root, FIX_RESULT), "fixer result"); +} + +/** The known flaky tests the daemon left in `root`'s `.git`, if any, as the agent's input lists them. */ +function knownFlaky(root: string): KnownFlaky[] { + const path = join(root, ".git", FLAKY_INPUT); + if (!existsSync(path)) return []; + const value = json(readBounded(path, "known flaky tests")); + if ( + !Array.isArray(value) || + !value.every( + (item) => + item && + typeof item.failure === "string" && + Array.isArray(item.jobs) && + item.jobs.every((job: unknown) => typeof job === "string"), + ) + ) + throw new Error("invalid known flaky tests"); + return value as KnownFlaky[]; +} /** 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. */ @@ -32,10 +68,12 @@ 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. `failure` names the failing test or step, - * the signature the stop rules compare across a pick's rounds. + * the signature the stop rules compare across a pick's rounds. A requeue of a known flaky test's + * failure may carry `disable`: the tip of the fixer's `disable` branch, a commit on the base that skips + * the test and adds its follow-up quest, which the daemon opens as a PR of its own. */ export type FixResult = { agent?: string; model?: string; blame?: boolean; comment?: string; failure?: string } & ( - | { number: number; action: "requeue"; reason: string } + | { number: number; action: "requeue"; reason: string; disable?: string } | { number: number; action: "fixed"; head: string; reason: string } | { number: number; action: "handback"; diagnosis: string; recommendation: string; log?: [number, number] } ); @@ -99,6 +137,11 @@ function validate(item: any, pr: boolean): void { throw new Error("invalid fixer failure"); metadata.push("failure"); } + if (pr && "disable" in item) { + if (item.action !== "requeue" || typeof item.disable !== "string" || !/^[a-f0-9]{40}$/.test(item.disable)) + throw new Error("invalid fixer disable"); + metadata.push("disable"); + } // One optional Markdown comment per PR: multiline, but no other control characters. if (pr && "comment" in item) { if ( @@ -179,6 +222,7 @@ export async function fix( if (!args && (!process.env.OPENAI_BASE_URL || !process.env.OPENAI_API_KEY)) throw new Error("fix needs OPENAI_BASE_URL and OPENAI_API_KEY from the container wrapper"); readFileSync(log); // Fail before starting an agent when the wrapper did not expose the log. + const flaky = numbers.length ? knownFlaky(root) : []; // The daemon pinned each PR's head on its pr- branch; name it so the agent need not look it up. const prs = numbers.map((number) => { const ref = `pr-${number}`; @@ -240,7 +284,7 @@ export async function fix( child.once("error", reject); child.stdin.on("error", () => {}); child.stdin.end( - `${prompt}\n\nFailure log: ${JSON.stringify(log)}\nPRs:${prs.length ? `\n${prs.join("\n")}` : " none; the base fails alone"}\n`, + `${prompt}\n\nFailure log: ${JSON.stringify(log)}\nPRs:${prs.length ? `\n${prs.join("\n")}` : " none; the base fails alone"}\n${flaky.length ? `Known flaky tests: ${JSON.stringify(flaky)}\n` : ""}`, ); child.once("close", (code) => (code === 0 ? done() : reject(new FixerAgentError(code ?? 1)))); }); diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 75ec65ee..7327365f 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -692,7 +692,7 @@ export function author(result?: Pick): string { ); } /** The PR body with the fix recorded in its `## Review` section; the line names the agent, and the author's text stays as written. */ -function review(body: string, head: string, reason: string, attribution: string): string { +export function review(body: string, head: string, reason: string, attribution: string): string { const line = `mechanical fix by ${attribution}: ${fixerText(reason)} (head ${head})`; if (body.includes(line)) return body; const section = /^## Review[ \t]*\r?$/m.exec(body); diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 0761ebf9..9d70add0 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -27,14 +27,26 @@ import { prepare, type Process, publishHead, + review, type Round, saveRound, settle, stopReason, stopRule, } from "./land-fixer"; +import { push, remoteHead } from "./land-head"; import { save } from "./land-serve"; import type { Removal } from "./land-watch"; +import { + type Disable, + type FlakyTest, + questName, + readFlaky, + RECENT, + recordFlaky, + saveFlaky, + writeKnownFlaky, +} from "./land-watch-flaky"; /** What the watcher needs to run the fixer on the PRs the queue drops. */ export interface FixContext { @@ -66,6 +78,8 @@ interface WatchJob extends Process { log: string; /** The failure's signature when the fixer names none: the failing job and step, or the conflict's base. */ failure: string; + /** A failed build's failing `merge_group` jobs, by URL, which a flaky verdict records. */ + runs?: string[]; /** A clean merge of the base with the quest merge driver, which needs no fixer; `result` holds its head. */ merged?: boolean; result?: FixResult; @@ -175,6 +189,7 @@ export async function advanceFixes( if (line) output.push(line); } output.push(...(await collectAll(github, state, context, deps, true))); + output.push(...(await advanceDisables(github, repository, state, context, deps))); return output; } @@ -330,6 +345,7 @@ async function take( // Start at the base: the agent loads its instructions and tooling only from code already landed. base.sha, ); + if (!conflict) writeKnownFlaky(state, checkout); const job: WatchJob = { checkout, started: deps.now(), @@ -342,6 +358,7 @@ async function take( reason: conflict ? "merge_conflict" : "failed_checks", log, failure: conflict ? conflictFailure(base.sha) : jobFailure(removal), + ...(conflict ? {} : { runs: removal.jobs.map((item) => item.url) }), }; const name = `job-${basename(checkout)}`; await launch(join(state, `watch-${name}.json`), job, context.fixer, log, [number], { @@ -466,6 +483,7 @@ async function collect( let handback = job.handback ?? job.error; let round: Round | undefined; + const said: string[] = []; const conflict = job.reason === "merge_conflict"; const failure = conflict || !result ? job.failure : (result.failure ?? job.failure); if (!handback && result) { @@ -479,6 +497,22 @@ async function collect( ? "unchanged" : "flaky" : "handback"; + // A flaky verdict counts for its test across every PR, whatever becomes of this pick. + if (outcome === "flaky" && result.action === "requeue") + said.push( + ...recordFlaky(context.root, state, { + job: name, + number, + dropped: job.dropped, + jobs: job.runs ?? [], + base: job.base, + checkout: job.checkout, + packs: job.packs, + failure, + result, + attribution, + }), + ); const rule = stopRule( record!.rounds.filter((item) => item.job !== name), { outcome, failure }, @@ -520,7 +554,7 @@ async function collect( } } - if (job.requeued && result) return announce(job.requeued, result); + if (job.requeued && result) return [...said, ...(await announce(job.requeued, result))]; // Anything else is a handback. await note(); // Every handback ends the pick with a round of its own, a crash's or a timeout's too, so the App's @@ -550,7 +584,7 @@ async function collect( job.resolution ? (room) => resolutionSection(number, job.resolution!, room) : undefined, ); finish(path, job); - return [`handback ${number}`]; + return [...said, `handback ${number}`]; } /** How `head` went back: enqueued, or armed to join the queue once its required checks pass. */ @@ -581,3 +615,106 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d } return how(pull); } + +/** + * Open each validated disable as a PR of its own and enqueue it like any other, with no maintainer's + * pick: its Review records the fixer's change. A disable that fails to advance is retried on the next + * pass; one GitHub refuses is left for a maintainer. + */ +async function advanceDisables( + github: GitHub, + repository: string, + state: string, + context: FixContext, + deps: GitHubDependencies, +): Promise { + const output: string[] = []; + const tests = readFlaky(state); + for (const test of tests) { + if (!test.disable || test.disable.done !== undefined) continue; + try { + output.push( + await openDisable(github, repository, context, test, test.disable, () => saveFlaky(state, tests)), + ); + } catch (error) { + deps.warn(`quest land watch: disable of ${test.disable.quest} pending: ${String(error)}`); + } + } + return output; +} + +/** A failing job's link, for a disable's PR body; only GitHub's own URLs. */ +const jobLinks = (jobs: string[]) => + jobs + .filter((url) => /^https:\/\/github\.com\/[\w./-]+$/.test(url)) + .map((url, index) => `[job ${index + 1}](${url})`) + .join(", ") || "no failing job found"; + +/** + * Push `disable` to its own `quest-land/flaky-` branch, open it on the base it was committed on, + * and put it in the merge queue, recording each step with `record`, so a restart repeats none. + * Returns what it did. + */ +async function openDisable( + github: GitHub, + repository: string, + context: FixContext, + test: FlakyTest, + disable: Disable, + record: () => void, +): Promise { + const name = questName(disable.quest); + if (disable.id === undefined) { + const branch = `quest-land/flaky-${disable.head.slice(0, 12)}`; + const ref = `refs/heads/${branch}`; + const remote = `https://github.com/${repository}.git`; + // A retry after a restart finds its own push already there. + if ((await remoteHead(context.root, remote, ref, context.read)) !== disable.head) { + const pushed = await push(context.root, remote, disable.base.sha, disable.head, ref, context.token, ""); + // Retried on the next pass: a push fails as often for an outage as for a rule. + if (pushed.status !== 0) throw new Error(`push failed: ${pushed.stderr.trim()}`); + } + const head = `${repository.split("/")[0]}:${branch}`; + const [open] = await github.all(`/repos/${repository}/pulls?head=${encodeURIComponent(head)}&state=open`); + const seen = test.verdicts + .slice(-RECENT) + .map((verdict) => `#${verdict.number} (${jobLinks(verdict.jobs)})`) + .join(", "); + const reason = `skip the flaky ${test.failure}, and add ${disable.quest} to fix or delete it`; + const pull = + open ?? + object( + await ( + await github.request(`/repos/${repository}/pulls`, { + body: { + title: `quest land: disable the flaky test ${name}`, + head, + base: disable.base.ref, + body: `The land fixer judged ${fixerText(test.failure)} flaky ${test.verdicts.length} times, most recently in the \`merge_group\` builds of ${seen}. This pull request skips it and adds its follow-up quest, \`${disable.quest}\`, to fix or delete it. quest land watch enqueues it like any other pull request, and fixes it if the queue drops it.${review("", disable.head, reason, disable.attribution)}\n\n(written by ${disable.attribution})`, + }, + }) + ).json(), + ); + disable.number = Number(pull.number); + disable.id = string(pull.node_id); + record(); + } + try { + disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); + } catch (error) { + if (!(error instanceof FixerRefusal)) throw error; + await post( + github, + { repository }, + { number: disable.number!, enabledAt: disable.head }, + `quest land watch: ${fixerText(error.message)}, so this pull request waits for a maintainer to enqueue it.`, + "refused", + MERGER, + ); + disable.done = error.message; + record(); + return `disable refused ${name}: ${error.message}`; + } + record(); + return `disabled ${name} ${disable.number}`; +} diff --git a/src/cli/land-watch-flaky.ts b/src/cli/land-watch-flaky.ts new file mode 100644 index 00000000..01de9b31 --- /dev/null +++ b/src/cli/land-watch-flaky.ts @@ -0,0 +1,152 @@ +// The merge queue watcher's record of flaky tests: each flaky verdict the fixer gives, by the test it +// names, and the commit that disables a test seen flaking twice, which the watcher opens as a PR. +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { git } from "./land"; +import { FLAKY_INPUT, type FixResult, type KnownFlaky } from "./land-fix"; +import { validateFix } from "./land-fixer"; +import { importFixer } from "./land-fixer-repository"; +import { save } from "./land-serve"; + +/** One round's flaky verdict on a test. */ +export interface FlakyVerdict { + /** The fixer job that gave it: collecting the same job again replaces its verdict rather than adding one. */ + job: string; + number: number; + /** When the queue dropped the PR. */ + at: string; + /** The failing `merge_group` jobs, by URL. */ + jobs: string[]; +} + +/** The commit that disables a test, validated, and how far its PR has come. */ +export interface Disable { + head: string; + /** The base the fixer committed it on. */ + base: { ref: string; sha: string }; + /** The follow-up quest it adds, `quest/m0/flaky/.md`. */ + quest: string; + /** Who wrote it, for the PR's attribution and its Review record. */ + attribution: string; + /** Its PR, once opened. */ + number?: number; + id?: string; + /** How it ended: queued (`enqueued` or `armed`), or why it could not be. */ + done?: string; +} + +/** A test the fixer judged flaky, by the `failure` it named. */ +export interface FlakyTest { + failure: string; + verdicts: FlakyVerdict[]; + disable?: Disable; +} + +/** The flaky verdicts that disable a test: one alone only re-enqueues, since it might hide a real bug. */ +export const FLAKY_VERDICTS = 2; +/** The most recent verdicts of a test that a round's input or a disable's PR lists. */ +export const RECENT = 10; +/** + * The bytes of known flaky tests a round is told about, most recently seen first, well within the + * 100 KB `quest land fix` reads. + */ +const KNOWN_BYTES = 50_000; +/** The follow-up quest a disable adds, one per test, in the repository's m0 `flaky` epic. */ +const QUEST = /^quest\/m0\/flaky\/([a-z0-9][a-z0-9._-]*)\.md$/; + +const flakyPath = (state: string) => join(state, "flaky.json"); + +/** The repository's flaky tests, in the order first seen. */ +export function readFlaky(state: string): FlakyTest[] { + const path = flakyPath(state); + return existsSync(path) ? (JSON.parse(readFileSync(path, "utf8")) as FlakyTest[]) : []; +} + +export function saveFlaky(state: string, tests: FlakyTest[]): void { + save(flakyPath(state), tests); +} + +/** + * Tell a round's fixer, in its `checkout`'s `.git`, which tests flaked before and are not yet disabled, + * by the signature it should reuse, so a second flake of one is recognized and disabled. + */ +export function writeKnownFlaky(state: string, checkout: string): void { + const latest = (test: FlakyTest) => test.verdicts.reduce((at, verdict) => (verdict.at > at ? verdict.at : at), ""); + const known: KnownFlaky[] = []; + let bytes = 2; + for (const test of readFlaky(state) + .filter((test) => test.disable === undefined && test.verdicts.length > 0) + .sort((a, b) => latest(b).localeCompare(latest(a)))) { + const item = { failure: test.failure, jobs: test.verdicts.slice(-RECENT).flatMap((verdict) => verdict.jobs) }; + bytes += Buffer.byteLength(JSON.stringify(item)) + 1; + if (bytes > KNOWN_BYTES) break; + known.push(item); + } + if (known.length) writeFileSync(join(checkout, ".git", FLAKY_INPUT), JSON.stringify(known), { mode: 0o600 }); +} + +/** A collected round that requeued a failed build unchanged, judging it flaky. */ +export interface FlakyRound { + /** The fixer job's file name. */ + job: string; + number: number; + dropped: string; + jobs: string[]; + base: { ref: string; sha: string }; + checkout: string; + packs?: string[]; + failure: string; + result: Extract; + attribution: string; +} + +/** + * Record a flaky verdict on `round.failure`, and, once the test has `FLAKY_VERDICTS` of them, take the + * round's disabling commit for its own PR: imported into the daemon's `root` and validated as a fix of + * the base is, adding one follow-up quest. Returns what it did, if anything worth saying. + */ +export function recordFlaky(root: string, state: string, round: FlakyRound): string[] { + const tests = readFlaky(state); + let test = tests.find((item) => item.failure === round.failure); + if (!test) { + test = { failure: round.failure, verdicts: [] }; + tests.push(test); + } + test.verdicts = [ + ...test.verdicts.filter((verdict) => verdict.job !== round.job), + { job: round.job, number: round.number, at: round.dropped, jobs: round.jobs }, + ]; + const output: string[] = []; + const head = round.result.disable; + if (head !== undefined && test.disable === undefined) { + if (test.verdicts.length < FLAKY_VERDICTS) output.push(`disable ignored ${round.number}: first flaky verdict`); + else + try { + const quest = disabling(root, state, round, head); + test.disable = { head, base: round.base, quest, attribution: round.attribution }; + output.push(`disable ${round.number} ${head}`); + } catch (error) { + // A later flake of the same test tries again. + output.push(`disable refused ${round.number}: ${String(error)}`); + } + } + saveFlaky(state, tests); + return output; +} + +/** + * Import the fixer's `disable` branch at `head` and check it: new commits on the base it was given, + * leaving every protected path as the base has it, that add exactly one follow-up quest. Returns its path. + */ +function disabling(root: string, state: string, round: FlakyRound, head: string): string { + importFixer(root, state, round.checkout, "disable", head, round.packs); + validateFix(root, { head: round.base.sha }, head, round.base.sha); + const added = git(root, ["diff", "--name-only", "--no-renames", "--diff-filter=A", round.base.sha, head, "--"]) + .split("\n") + .filter((path) => QUEST.test(path) && path !== "quest/m0/flaky/README.md"); + if (added.length !== 1) throw new Error("a disable must add exactly one follow-up quest in quest/m0/flaky/"); + return added[0]; +} + +/** The follow-up quest's name, as a disable's branch and title name it. */ +export const questName = (quest: string) => QUEST.exec(quest)?.[1] ?? quest; diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 980f4cd3..f76eda44 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -14,6 +14,8 @@ import { FLAKY_LIMIT, history } from "../src/cli/land-fixer"; import { privateRepository } from "../src/cli/land-serve"; import { jobsSection, readRemoval, SETTLE, watchPass, watchPermissions } from "../src/cli/land-watch"; import type { FixContext } from "../src/cli/land-watch-fix"; +import { type FlakyTest, RECENT, readFlaky, saveFlaky, writeKnownFlaky } from "../src/cli/land-watch-flaky"; +import { FLAKY_INPUT, type KnownFlaky } from "../src/cli/land-fix"; import { configure, GIT_SETTINGS, initRepository, tempDir } from "./fixture"; // The fixer tests merge with the quest driver and run fixer wrappers, which take longer than vitest's default. @@ -57,6 +59,9 @@ interface Pull { /** When auto-merge was armed, while it is. */ armed?: string; body?: string; + title?: string; + /** Its head branch, `branch-` by default. */ + branch?: string; /** Its head branch's pushes, oldest first, in place of those its removals imply. */ pushes?: { sha: string; at: string }[]; } @@ -108,9 +113,11 @@ function world(pulls: Pull[], runs: Run[] = []): World { /** A PR's head: its branch in the upstream repository, when the world has one. */ function headOf(world: World, pull: Pull): string { - return world.origin ? git(world.origin, "rev-parse", `refs/heads/branch-${pull.number}`) : pull.head; + return world.origin ? git(world.origin, "rev-parse", `refs/heads/${branchOf(pull)}`) : pull.head; } +const branchOf = (pull: Pull) => pull.branch ?? `branch-${pull.number}`; + function git(cwd: string, ...args: string[]): string { return execFileSync("git", args, { cwd, encoding: "utf8" }).trim(); } @@ -244,7 +251,7 @@ function deps(world: World): GitHubDependencies { url: `https://github.com/upstream/project/pull/${pull.number}`, title: `PR ${pull.number}`, headRefOid: headOf(world, pull), - headRefName: `branch-${pull.number}`, + headRefName: branchOf(pull), maintainerCanModify: false, headRepository: { nameWithOwner: "upstream/project" }, mergeStateStatus: pull.mergeState ?? "CLEAN", @@ -290,6 +297,30 @@ function deps(world: World): GitHubDependencies { const job = world.runs.flatMap((run) => run.jobs).find((item) => item.id === Number(logs[1])); return job?.log === undefined ? new Response("missing", { status: 404 }) : new Response(job.log); } + if (path === "/repos/upstream/project/pulls") { + if (init?.method === "POST") { + const { title, head, base, body } = JSON.parse(String(init.body)); + expect(base).toBe("main"); + const number = Math.max(...world.pulls.map((item) => item.number)) + 1; + const branch = head.replace(/^upstream:/, ""); + world.pulls.push({ + number, + head: headOf(world, { number, branch } as Pull), + branch, + title, + body, + removals: [], + enqueues: [], + }); + return Response.json({ number, node_id: `PR_${number}` }, { status: 201 }); + } + const head = url.searchParams.get("head"); + return Response.json( + world.pulls + .filter((item) => `upstream:${branchOf(item)}` === head && (item.state ?? "OPEN") === "OPEN") + .map((item) => ({ number: item.number, node_id: `PR_${item.number}` })), + ); + } const pullPath = /^\/repos\/upstream\/project\/pulls\/(\d+)$/.exec(path); if (pullPath) { const pull = world.pulls.find((item) => item.number === Number(pullPath[1]))!; @@ -316,7 +347,7 @@ function deps(world: World): GitHubDependencies { } const activity = /^\/repos\/upstream\/project\/activity$/.exec(path); if (activity) { - const pull = world.pulls.find((item) => `branch-${item.number}` === url.searchParams.get("ref"))!; + const pull = world.pulls.find((item) => branchOf(item) === url.searchParams.get("ref"))!; world.onActivity?.(pull); return Response.json(world.unlisted?.includes(pull.number) ? [] : pushes(world, pull)); } @@ -738,6 +769,36 @@ test("failing jobs that do not fit the comment are counted instead", () => { expect(cut.length).toBeLessThanOrEqual(800); }); +test("a round is told the most recently seen flaky tests not yet disabled, within what the fixer reads", () => { + const state = tempDir(); + const checkout = tempDir(); + mkdirSync(join(checkout, ".git")); + // Long-lived flakes that never got a valid disable keep collecting verdicts. + const tests: FlakyTest[] = Array.from({ length: 200 }, (_, index) => ({ + failure: `just test: flaky_${index}`, + verdicts: Array.from({ length: 30 }, (_, round) => ({ + job: `watch-job-${index}-${round}.json`, + number: round + 1, + at: at(index), + jobs: [`https://github.com/upstream/project/actions/runs/${index}/job/${round}`], + })), + })); + tests[0].disable = { + head: HEAD, + base: { ref: "main", sha: BASE }, + quest: "quest/m0/flaky/x.md", + attribution: "Codex", + }; + saveFlaky(state, tests); + writeKnownFlaky(state, checkout); + const text = readFileSync(join(checkout, ".git", FLAKY_INPUT), "utf8"); + expect(text.length).toBeLessThan(100_000); + const known = JSON.parse(text) as KnownFlaky[]; + expect(known[0].failure).toBe("just test: flaky_199"); + expect(known.every((item) => item.jobs.length === RECENT)).toBe(true); + expect(known.some((item) => item.failure === "just test: flaky_0")).toBe(false); +}); + const bin = fileURLToPath(new URL("../src/cli/bin.ts", import.meta.url)); const bunfig = fileURLToPath(new URL("../bunfig.toml", import.meta.url)); const PATH = process.env.PATH; @@ -1146,6 +1207,98 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl await exited(state); }); + test("a test seen flaking twice is disabled in its own pull request, which the watcher enqueues", async () => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const heads = [5, 6, 7].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); + const github = { ...world([], []), origin: repo.origin }; + /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ + const flaky = (script: string) => `${script} +printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","failure":"just test: render_timeout","disable":"%s"}]' "$2" "$(git rev-parse HEAD)" > quest-land-result.json +`; + /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ + const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json +git checkout -q -b disable base +mkdir -p quest/m0/flaky +printf '# [S] Fix or delete render_timeout\\n\\n## Goal\\n\\nFix the flaky test or delete it.\\n' > quest/m0/flaky/render-timeout.md +printf '# Flaky tests\\n\\n## Goal\\n\\nFix or delete each disabled test.\\n\\n## Required\\n\\n- [Render timeout](/quest/m0/flaky/render-timeout.md)\\n' > quest/m0/flaky/README.md +printf 'skip render_timeout\\n' > skip.txt +${extra} +git add quest skip.txt ${extra ? "flake.nix" : ""} +git commit -qm 'disable render_timeout'`; + // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a + // protected path and is refused, and the third's is opened as its own pull request. + fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]")); + fix.round(6, flaky(disabling("printf 'changed\\n' > flake.nix"))); + fix.round(7, flaky(disabling(""))); + const said: string[][] = []; + for (const [index, number] of [5, 6, 7].entries()) { + const minute = (index + 1) * 10; + github.pulls.push({ + number, + head: heads[index], + enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(minute), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads[index], + }, + ], + }); + github.runs.push({ ...failedRun(number, at(minute - 2), base), id: number * 10 }); + github.now = Date.parse(at(minute + 1)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + `removed ${number} failed_checks fix`, + `fixing ${number}`, + ]); + await exited(state); + github.now = Date.parse(at(minute + 2)); + said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); + } + const branch = github.pulls.find((pull) => pull.number === 8)?.branch; + expect(branch).toMatch(/^quest-land\/flaky-[0-9a-f]{12}$/); + const head = git(repo.origin, "rev-parse", `refs/heads/${branch}`); + expect(said).toEqual([ + ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], + [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], + [`disable 7 ${head}`, `requeued 7 ${heads[2]}`, "disabled render-timeout 8"], + ]); + // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. + expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); + expect(git(repo.origin, "show", `${head}:quest/m0/flaky/render-timeout.md`)).toContain("Fix the flaky test"); + expect(github.mutations).toEqual([ + `enqueuePullRequest 5 ${heads[0]}`, + `enqueuePullRequest 6 ${heads[1]}`, + `enqueuePullRequest 7 ${heads[2]}`, + `enqueuePullRequest 8 ${head}`, + ]); + const opened = github.pulls.find((pull) => pull.number === 8)!; + expect(opened.title).toBe("quest land: disable the flaky test render-timeout"); + expect(opened.body).toContain( + "The land fixer judged `just test: render_timeout` flaky 3 times, most recently in", + ); + expect(opened.body).toContain( + "#5 ([job 1](https://github.com/upstream/project/actions/runs/50/job/71)), #6 ([job 1](https://github.com/upstream/project/actions/runs/60/job/71))", + ); + expect(opened.body).toContain("adds its follow-up quest, `quest/m0/flaky/render-timeout.md`"); + expect(opened.body).toContain(`## Review\n\nmechanical fix by Codex: \``); + expect(opened.body).toContain(`(head ${head})`); + expect(readFlaky(state)).toMatchObject([ + { + failure: "just test: render_timeout", + verdicts: [{ number: 5 }, { number: 6 }, { number: 7 }], + disable: { head, quest: "quest/m0/flaky/render-timeout.md", number: 8, done: "enqueued" }, + }, + ]); + + // A restart neither opens nor enqueues it again, and the disabled test is no longer offered as known. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + expect(github.mutations).toHaveLength(4); + }); + test("a pick that ends while the fixer runs is left alone, and every handback ends the pick", async () => { const { repo, state, fix, context } = setup(5); const base = repo.main(); diff --git a/test/land.test.ts b/test/land.test.ts index 6cde2067..466150f7 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -38,7 +38,7 @@ import { import { importFixer } from "../src/cli/land-fixer-repository"; import { apply, parseResolution, pushCommand, resolutionSection } from "../src/cli/land-apply"; import fixerPrompt from "../assets/land-fix.md"; -import { COMMENT_LIMIT, fix, parseBaseFix, parseFixResult } from "../src/cli/land-fix"; +import { COMMENT_LIMIT, FLAKY_INPUT, fix, parseBaseFix, parseFixResult } from "../src/cli/land-fix"; import { resolvePreparedQueue, prepareHead, finishFallback } from "../src/cli/land-head"; import type { GitHubDependencies } from "../src/cli/github"; import { @@ -3103,6 +3103,19 @@ ${requeueScript} ); await fix(dir, log, [1], agent, ["-"]); expect(readFileSync(join(dir, "prompt.txt"), "utf8")).toContain(`PRs:\n- #1: branch pr-1 at ${head}\n`); + expect(readFileSync(join(dir, "prompt.txt"), "utf8")).not.toContain("Known flaky tests"); + + // The daemon's known flaky tests, in the checkout's .git, follow the PRs as data. + const known = [ + { failure: "cargo test: render_timeout", jobs: ["https://github.com/o/r/actions/runs/1/job/2"] }, + ]; + writeFileSync(join(dir, ".git", FLAKY_INPUT), JSON.stringify(known)); + await fix(dir, log, [1], agent, ["-"]); + expect(readFileSync(join(dir, "prompt.txt"), "utf8")).toContain( + `PRs:\n- #1: branch pr-1 at ${head}\nKnown flaky tests: ${JSON.stringify(known)}\n`, + ); + writeFileSync(join(dir, ".git", FLAKY_INPUT), JSON.stringify([{ failure: 7, jobs: [] }])); + await expect(fix(dir, log, [1], agent, ["-"])).rejects.toThrow("invalid known flaky tests"); }); test("finds the agent on PATH, never in the checkout it fixes", async () => { const dir = fixRepository(); @@ -3544,6 +3557,21 @@ printf '[{"number":1,"action":"handback","diagnosis":"saw %s","recommendation":" parseBaseFix(JSON.stringify({ action: "fixed", head: "a".repeat(40), reason: "fine", failure: "x" })), ).toThrow(); }); + test("accepts a disabling commit only on a requeue", () => { + const disable = "d".repeat(40); + const accepted = { number: 1, action: "requeue", reason: "flaky", failure: "cargo test: render", disable }; + expect(parseFixResult(JSON.stringify([accepted]), [1])).toEqual([accepted]); + for (const value of [ + { number: 1, action: "requeue", reason: "flaky", disable: "short" }, + { number: 1, action: "requeue", reason: "flaky", disable: 7 }, + { number: 1, action: "fixed", head: disable, reason: "fine", disable }, + { number: 1, action: "handback", diagnosis: "d", recommendation: "r", disable }, + ]) + expect(() => parseFixResult(JSON.stringify([value]), [1])).toThrow("invalid fixer disable"); + expect(() => + parseBaseFix(JSON.stringify({ action: "fixed", head: disable, reason: "fine", disable })), + ).toThrow(); + }); test("strictly accepts one fixed or handback object for the base alone", () => { const head = "a".repeat(40); for (const value of [