From 1a40fabea06196154b2e3204dff9be1e7e56f873 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 18:54:24 -0800 Subject: [PATCH 1/3] chore(quest): claim quest/m0/queue-watcher/flaky.md Quest-Claim: a32acf64ebfdf217 From 966206112a1f510648e2ff66ceb2ced649dd2cb8 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:05:36 -0800 Subject: [PATCH 2/3] feat(land): the watcher disables a test seen flaking twice in its own PR The watcher keeps the repository's flaky verdicts by test in flaky.json and tells each failed build's fixer round which tests flaked before. When a known test flakes again, the fixer commits the skip and its follow-up quest in quest/m0/flaky/ on a `disable` branch from the base; the watcher validates that commit, pushes it to quest-land/flaky-, opens it as a PR with its Review recorded, and enqueues it like any other. Co-Authored-By: Claude Opus 5.5 --- README.md | 16 ++++ assets/land-fix.md | 31 +++++++ quest/m0/land-fixer/README.md | 8 +- quest/m0/queue-watcher/README.md | 3 +- quest/m0/queue-watcher/flaky.md | 53 ------------ src/cli/land-fix.ts | 58 +++++++++++-- src/cli/land-fixer.ts | 2 +- src/cli/land-watch-fix.ts | 140 +++++++++++++++++++++++++++++- src/cli/land-watch-flaky.ts | 142 +++++++++++++++++++++++++++++++ test/land-watch.test.ts | 126 ++++++++++++++++++++++++++- test/land.test.ts | 30 ++++++- 11 files changed, 536 insertions(+), 73 deletions(-) delete mode 100644 quest/m0/queue-watcher/flaky.md create mode 100644 src/cli/land-watch-flaky.ts diff --git a/README.md b/README.md index 31c9c53c..df0204f9 100644 --- a/README.md +++ b/README.md @@ -829,6 +829,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`, and 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 b539ce2e..285c50af 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 @@ -137,8 +137,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](/quest/m0/queue-watcher/bad-picks.md), dropped 2026-11-02: a - pick GitHub refuses fails `quest merge` instead), and flaky tests (now - [Flaky failures](/quest/m0/queue-watcher/flaky.md), without the rerun). + pick GitHub refuses fails `quest merge` instead), 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 722adc41..47c302ee 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 @@ -131,7 +131,6 @@ Shared shape, for the children to follow: ## Required - [Bad picks](/quest/m0/queue-watcher/bad-picks.md) - a pick whose auto-merge is armed on a ready PR GitHub never queued is enqueued at the head it was armed on -- [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 ea28f0a5..5325ff30 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -27,14 +27,25 @@ 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, + 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 +77,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 +188,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 +344,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 +357,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 +482,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 +496,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 +553,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 +583,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 +614,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, ""); + if (pushed.status !== 0) { + disable.done = `push refused: ${pushed.stderr.trim()}`; + record(); + return `disable refused ${name}: ${disable.done}`; + } + } + const head = `${repository.split("/")[0]}:${branch}`; + const [open] = await github.all(`/repos/${repository}/pulls?head=${encodeURIComponent(head)}&state=open`); + const seen = test.verdicts.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, 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..c740deab --- /dev/null +++ b/src/cli/land-watch-flaky.ts @@ -0,0 +1,142 @@ +// 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 known flaky tests a round is told about, most recently seen first. */ +const KNOWN = 50; +/** 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[] = readFlaky(state) + .filter((test) => test.disable === undefined && test.verdicts.length > 0) + .sort((a, b) => latest(b).localeCompare(latest(a))) + .slice(0, KNOWN) + .map((test) => ({ failure: test.failure, jobs: test.verdicts.flatMap((verdict) => verdict.jobs) })); + 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 3cb0c5ec..01cfaeb0 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -14,6 +14,7 @@ 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 { readFlaky } from "../src/cli/land-watch-flaky"; 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 +58,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; } interface Job { id: number; @@ -102,9 +106,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(); } @@ -234,7 +240,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" }, removals: removalPage(pull, variables.since, null), @@ -277,6 +283,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]))!; @@ -303,7 +333,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"))!; return Response.json(world.unlisted?.includes(pull.number) ? [] : pushes(world, pull)); } throw new Error(`unexpected request: ${path}`); @@ -1028,6 +1058,96 @@ 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"); + 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 [ From 73b45f1783e8a52a34d0b5ad5ac677576445af5b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:11:03 -0800 Subject: [PATCH 3/3] fix(land): bound the known flaky input, and retry a failed disable push Co-Authored-By: Claude Opus 5.5 --- src/cli/land-watch-fix.ts | 15 ++++++++------- src/cli/land-watch-flaky.ts | 22 ++++++++++++++++------ test/land-watch.test.ts | 37 +++++++++++++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 5325ff30..4286e73f 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -42,6 +42,7 @@ import { type FlakyTest, questName, readFlaky, + RECENT, recordFlaky, saveFlaky, writeKnownFlaky, @@ -670,15 +671,15 @@ async function openDisable( // 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, ""); - if (pushed.status !== 0) { - disable.done = `push refused: ${pushed.stderr.trim()}`; - record(); - return `disable refused ${name}: ${disable.done}`; - } + // 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.map((verdict) => `#${verdict.number} (${jobLinks(verdict.jobs)})`).join(", "); + 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 ?? @@ -689,7 +690,7 @@ async function openDisable( 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, 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})`, + 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(), diff --git a/src/cli/land-watch-flaky.ts b/src/cli/land-watch-flaky.ts index c740deab..01de9b31 100644 --- a/src/cli/land-watch-flaky.ts +++ b/src/cli/land-watch-flaky.ts @@ -44,8 +44,13 @@ export interface FlakyTest { /** 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 known flaky tests a round is told about, most recently seen first. */ -const KNOWN = 50; +/** 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$/; @@ -67,11 +72,16 @@ export function saveFlaky(state: string, tests: FlakyTest[]): void { */ 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[] = readFlaky(state) + 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))) - .slice(0, KNOWN) - .map((test) => ({ failure: test.failure, jobs: test.verdicts.flatMap((verdict) => verdict.jobs) })); + .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 }); } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 01cfaeb0..6cfdae96 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -14,7 +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 { readFlaky } from "../src/cli/land-watch-flaky"; +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. @@ -655,6 +656,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; @@ -1128,7 +1159,9 @@ git commit -qm 'disable render_timeout'`; ]); 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"); + 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))", );