Skip to content

feat(land watch): only genuine flakes count toward disabling a test - #468

Merged
moq-bot[bot] merged 6 commits into
mainfrom
quest/m0/queue-watcher/flaky-verdicts
Nov 4, 2026
Merged

moq-bot[bot] merged 6 commits into
mainfrom
quest/m0/queue-watcher/flaky-verdicts

Conversation

@kixelated

@kixelated kixelated commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m0/queue-watcher/flaky-verdicts.md.

Problem

quest land watch counted every unchanged requeue of a failed build as a flaky verdict, and two verdicts take a disable. Failures that reproduce on the base or come from the runner were counted too: the EROFS cgroup.kill and missing flock/script rounds on moq-dev/quest#324, and an unused import already on moq's main, requeued twice on moq-dev/moq#7560 and moq-dev/moq#7221. Signatures that differed only by backticks also split one test's verdicts.

Changes

  • A requeue result may name its cause: flaky, base (the base fails it alone), or environment (the runner: a missing tool, a read-only path, a full disk). Only flaky is recorded in flaky.json, and disable is accepted only on a flaky requeue.
  • Watcher rounds record the cause as their outcome (flaky, base, environment). All three still requeue at the back and count toward the renamed REQUEUE_LIMIT streak (3 in a row hands back). The handback, now "stop rule (requeue limit)", names the last round's cause. The requeue comment says which cause the fixer found.
  • failure signatures are normalized at parse time (backticks and quotes stripped, whitespace collapsed). recordFlaky and the repeated-failure stop rule normalize both sides when they compare, so older records and fallback step names still match.
  • assets/land-fix.md tells the fixer how to choose each cause, that a base or runner failure is never disabled, and to hand back when it cannot tell why the failure happened.
  • README watcher and gate sections updated. Quest deleted; the epic's Plan and quest/m0/review-test-load.md now link this PR instead.

Public API / wire impact: the fixer result format gains an optional cause field. StopRule flaky is renamed requeued and FLAKY_LIMIT is renamed REQUEUE_LIMIT (internal; histories already on disk still read, since nothing reads rule back).

Choices made without asking

  • A watcher requeue of a failed build that names no cause hands the PR back (the unchanged stop rule), instead of defaulting to flaky. Defaulting to flaky would keep the bug for any fixer that forgets the field. With build concurrency 1, a failure that isn't the PR's is always one of the three causes.
  • The frozen host gate reads cause too, defaulting to flaky when none is named. That keeps its collateral-PR requeues unchanged. The gate still keeps no flaky record.
  • Base and environment requeues share the flaky streak rather than having a limit of their own, so a pick never cycles forever on a broken base or runner.
  • disable without failure is still accepted, as before. It falls back to the job's failing step.

Validation

  • quest gate -- just check: passed.
  • quest gate -- just test: passed (1046 + 531 tests).
  • New tests: a test failing on the base twice and then on the runner is requeued with no verdict and handed back by the requeue limit, so its first genuine flake is ignored as a first verdict. A requeue with no cause hands back. The disable test now names the test three ways (backticks, quotes with doubled spaces, plain) and still disables it. Parse tests cover cause, disable requiring flaky, and normalization. Stop-rule tests cover mixed causes.

Suggested follow-ups

None.

Review

mechanical fix by codex / Codex: Merged base and combined conflicting documentation examples, preserving failure causes and repository checks; ran bun install --frozen-lockfile, bun run build, bun run check, bun run --silent quest check, bun run test, and successful timeout retries with bun run test --maxWorkers=1 --testTimeout=30000 test/re·solve.test.ts test/agent.test.ts test/review.test.ts. (head 7cb87ec)

mechanical fix by codex / Codex: Merged base and retained deletion of the completed review-test-load plan, resolving its link-only modify/delete conflict; passed bun run build, bun run check, and bun run test (28 files, 1058 tests passed, 5 skipped). (head 491150a)

Codex gpt-6.1-sol, effort xhigh, via quest review --base origin/main.

  • Round 1 (head 88c19ef), 2 findings, both fixed:
    • [P2] The repeated-failure rule compared a newly normalized signature with an un-normalized stored one. Fixed: stopRule normalizes both operands, with a test.
    • [P3] A mixed-cause streak ending in flaky said "judged flaky 3 times". Fixed: every requeue-limit handback reads "requeued unchanged 3 times in a row, last ..." with the last cause.
  • Round 2 (head 7d98540): no findings.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 3 commits November 3, 2026 22:57
A requeue names its cause: flaky, the base, or the runner's environment.
Only flaky ones are recorded in flaky.json; the others still requeue and
count toward the requeue limit. Failure signatures are compared without
quoting or repeated whitespace.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Only genuine flakes count toward disabling a test feat(land watch): only genuine flakes count toward disabling a test Nov 4, 2026
…the requeue limit by cause

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 4, 2026 07:32
@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 159d3b7d-5ba5-4316-b136-bb398932e5df
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one docs gap worth closing.

Reviewed changes

Reviewed the complete diff at head 7d98540 (11 files). Ran bun run check (tsc + prettier, clean) and the two affected test files with vitest (224 pass).

  • cause on requeues — a requeue result may name flaky, base, or environment; validation keeps cause on requeues only and makes disable require cause: "flaky", which transitively keeps it on requeues. failure signatures are normalized at parse time, so quoting variants of one test are stored as one signature.
  • Watcher and gate outcomes — rounds record the named cause as their outcome; only flaky reaches recordFlaky, so only genuine flakes count toward a disable. A watcher requeue naming no cause hands back via the unchanged rule, while the gate defaults it to flaky; both choices are documented in the PR body and tested.
  • Stop rules — FLAKY_LIMIT/"flaky" become REQUEUE_LIMIT/"requeued", counting every unchanged re-enqueue whatever its cause, with the handback naming the last round's cause. Old histories on disk still parse, since old "flaky" outcomes are a valid cause and nothing reads rule back.
  • Fixer prompt and docs — assets/land-fix.md tells the fixer how to pick a cause, that a base or runner failure is never disabled, and to hand back when it cannot tell; README and quest docs updated, quest deleted.

ℹ️ Stale --help and comment text the behavior change left behind

The PR updated README.md, assets/land-fix.md, and the quest docs for the new requeue semantics, but two user-facing strings that describe the old rules were missed. Both files are outside this PR's diff, so they cannot carry inline comments.

Technical details
# Stale user-facing text about the requeue and flaky rules

## Affected sites
- src/cli/main.ts:665 — the `quest land watch` command help still says a pick is handed back by "a round with no new commit for a failure that isn't flaky ... or three flaky rounds in a row", and that "each fixed, merged, or flaky head is re-enqueued at the back of the queue". After this PR the requeue limit counts three unchanged re-enqueues in a row whatever their causes, a no-cause requeue hands back on the first round, and base- or environment-caused requeues also re-enqueue.
- src/cli/land-watch.ts:383 — the comment posted on every dropped PR still says the watcher re-enqueues it "once it is fixed or judged flaky"; base- and environment-caused requeues re-enqueue too, and a no-cause requeue is now a handback.

## Required outcome
- Both strings describe the new rules rather than the old ones: the requeue limit is cause-agnostic, the disable verdict is what stays flaky-only, and the handback wording matches `stopReason` in `src/cli/land-fixer.ts`.

## Suggested approach (optional)
- Fold into this PR (it already owns the docs sweep) or a small follow-up; `AGENTS.md` asks for README/help consistency.

Pullfrog  | Fix it ➔ | View workflow run | Using glm-5.3-flash | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 4, 2026
@moq-bot

moq-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 7d98540 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 4, 2026 16:44
@moq-bot

moq-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 491150a, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex / Codex): Merged base and retained deletion of the completed review-test-load plan, resolving its link-only modify/delete conflict; passed bun run build, bun run check, and bun run test (28 files, 1058 tests passed, 5 skipped)..

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior review at 7d98540: a single head commit, 491150a, merging main into the branch.

  • Merged main (e102ec7) into the branch — the merge's combined diff is empty, so it introduced no novel content: the land-watch code, the fixer prompt, and the land tests are unchanged from the previously reviewed state.
  • Resolved the modify/delete conflict by keeping main's deletion of quest/m0/review-test-load.md (the branch had only added PR-#468 links; main deleted the completed plan), and it picked up main's own completions (gate-load-wording.md, kix/runner-review.md, m1/windows-suite.md plus their index entries).
  • Swept for dangling references — no file at the head references any deleted quest plan, including the ones this PR deleted itself.
  • Verified on CI — check (all four targets) and build are green on 491150a.

Pullfrog  | View workflow run | Using glm-5.3-flash | 𝕏

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 4, 2026
@moq-bot

moq-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 491150a because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 4, 2026 17:30
@moq-bot

moq-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

codex / Codex land fixer note:

Merged base and resolved both documentation example conflicts by preserving the PR's cause fields and base's repository-check instructions. No behavior changes were introduced by the resolution. Build, type/format checks, and quest validation passed. The full CLI suite reported 1053 passed, 5 skipped, and five 5000ms timeouts in agent, resolve, and review tests; all three affected files passed on retry with one worker and a 30000ms timeout (115 passed, 1 skipped). Landing tests passed in the full suite.

(written by codex / Codex)

@moq-bot

moq-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 7cb87ec, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex / Codex): Merged base and combined conflicting documentation examples, preserving failure causes and repository checks; ran bun install --frozen-lockfile, bun run build, bun run check, bun run --silent quest check, bun run test, and successful timeout retries with bun run test --maxWorkers=1 --testTimeout=30000 test/re·solve.test.ts test/agent.test.ts test/review.test.ts..

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior review at 491150a: a single head commit, 7cb87ec, merging main (de0e7e4, "The land fixer runs the repository's own checks" #472) into the branch again.

  • Reconciled the fixer prompt's requeue examples — the merge resolution folded this PR's cause semantics into #472's new ## Repository checks section in assets/land-fix.md: the requeue and disable examples now read {"cause":"flaky",...} first, with a reason that names the commands run ("ran just check and just test on a scratch merge"), and the base-alone fixed example names them too. The reordering of JSON keys in a prompt is cosmetic and the validation code is order-insensitive.
  • Kept both sides' prose — the branch's cause guidance (never disable on base or runner failures, hand back when the cause is unknown) coexists untouched with #472's checks-run-before-requeue requirement; ## Repository checks is main's text verbatim and nothing of the branch's was dropped in the merged hunks.
  • Verified the merged state locally — the fixer-prompt assertion test in test/land.test.ts (which now carries #472's contains/not-contains checks alongside this PR's) passes, and bun run check is clean.

Pullfrog  | View workflow run | Using glm-5.3-flash | 𝕏

Merged via the queue into main with commit 4b3faf1 Nov 4, 2026
18 checks passed
@moq-bot
moq-bot Bot deleted the quest/m0/queue-watcher/flaky-verdicts branch November 4, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant