[kixelated]

Finishes quest/m0/land-fixer/holds.md.

Problem

quest land watch never read a PR's labels, so it ran the fixer on, pushed to, and re-enqueued a PR another session held. On moq-dev/moq#6949, the host gate pushed a fixer merge and re-armed the PR while a /quest-iterate session held quest:iterate and was pushing its own fixes; the watcher, which replaced the gate there, had the same gap.

Change

A hold is any label isHoldLabel accepts (quest:active, or an older pin's quest:iterate; the leftover quest:merge holds nothing). Every step where the watcher can see a hold ends the held PR's pick in its state first and stops acting on the PR, then posts one comment best-effort: a comment GitHub refuses is warned about and never retried. The comment has one wording (HELD in src/cli/land-watch-fix.ts), whichever step found the hold and whether or not a fix was already pushed.

• Removal (handle in src/cli/land-watch.ts): the pass's open-PR snapshot now reads labels. A fixable removal of a held PR gets the new verdict held instead of fix, recorded before the removal's comment, which then says the watcher leaves the PR to its session. A removal recorded held stays held if a crash makes a pass handle it again after the hold clears.
• Before the fixer starts (take): the PR re-read now carries labels. A hold taken while the removal waits for a fixer slot marks the removal held and ends the pick.
• A finished round (collect): a held PR's round is dropped before its result is recorded, like a round whose pick ended: no fixer note, no flaky verdict, no push. The PR is read again right after the fix is validated and before publishHead, since validation takes a while; a head already pushed (after a crash) needs no push and skips that read.
• Before a re-enqueue (requeue): a held PR is not re-enqueued. A head this round already asked GitHub to queue, and GitHub took, stays, since the hold came after it.
• A flaky-test disable PR (openDisable): a held one is recorded done: "held" and left for a maintainer, as a refused one is.
• Before enqueuing an armed pick (enqueueArmed): the watcher does not enqueue an armed pick GitHub left out of the queue while the PR is held. The pick stays armed, and the first pass after the hold clears enqueues it if its head and request are unchanged.

The README's watcher section documents it.

Public API: none. quest land watch behavior changes as above. Its state files change: the removal record's verdict gains held, and a disable's done can be held. Nothing is in production, so no migration.

Choices made without asking

• A held round's fixer note is dropped, so the PR gets exactly one comment from the hold, as the Goal says.
• A hold taken and released between two of the watcher's reads, with the PR left unchanged, is not seen and does not end the pick. Re-reading labels right before the fixer launches would only move that window (the fixer's run is minutes long); closing it would mean reading the label timeline, which the decision does not ask for. A session that pushed ends the pick anyway.
• handle records every removal before its comment, not only a held one. A held removal's comment is best-effort; any other verdict's refused comment still fails the pass before the event is marked handled, so the next pass handles it again and posts it, as before.
• An armed pick (enqueueArmed) is a writer's request, not a watcher's pick, so a hold leaves it armed rather than ending it, as decided before the checkpoint.

Validation

• bunx vitest run test/land-watch.test.ts: 25 passed.
  • "a held PR is neither fixed nor re-enqueued, whenever the hold is taken" covers a hold at the removal (5), one taken while the fix is validated, before the push (6), one taken right after the push, before the re-enqueue (7), one taken while the fixer runs (8), and one taken while the removal waits for a slot (9); quest:merge alone does not hold. GitHub refuses the hold comments of 5 (at the removal) and 7, and neither is retried once the holds clear. A crash that loses 5's handled mark still leaves it held after its hold clears.
  • "an armed pick withdrawn, retargeted, or held while the pass reads it is not enqueued" covers a hold present when the pass reads the PR and one taken during the pass, and that both are enqueued once the holds clear.
  • "a failed build is fixed and re-enqueued, ..." adds a hold after GitHub took the fixed head back and the read that followed failed: the round still reports requeued 5 <fixed>.
  • "a test seen flaking twice is disabled in its own pull request, which the watcher enqueues unless it is held" runs held too: the disable PR is held as it opens, gets the hold comment, and is not enqueued, even once the hold clears.
  • Each hold check was removed in turn (ten mutations: handle's recorded and live hold, take, collect's finished-round check, its pre-push read and that read's skip for a pushed head, the kept queued head, requeue, enqueueArmed, and ending the pick in collect); each fails the suite.
• just check and just test (through quest gate) on 2ccd142c: both pass.
• Merged origin/main (at 844c9934) cleanly.

Decisions

Settled by the maintainer in /quest-complete on 2026-11-04:

• What should a hold do to the held PR's merge-queue pick?
  • ✅ The hold ends the pick (as implemented): like a person removing it (endPick), the watcher records it, comments once, and forgets the pick. A maintainer enqueues the PR again once the session finishes.
  • Rejected: park the pick or the finished round and resume it when the hold clears.

Settled by the maintainer in /quest-complete on 2026-11-04, at the design checkpoint (Design change). Seven local review rounds plus Pullfrog kept finding gaps in the held comment's bookkeeping (persisting it before posting, retrying refused comments, "pushed as X" vs "not applied" wording), not in the rule itself:

• How should this PR continue after the design checkpoint?
  • ✅ Simplify, comment best-effort: keep the rule (the hold ends the pick). At every step where the watcher can see a hold, a held PR ends its pick, saved to state first, and the watcher stops acting on it. It then posts one hold comment best-effort: no retry, no persisted "comment pending" state, and one wording that doesn't depend on whether a fix was pushed. job.held, Disable.refused, Removal.hold, the comment-retry paths, and their tests are gone; the tests for the rule at each step stay.
  • Rejected: applying Pullfrog's one-line patch and merging.
  • Rejected: never touching or commenting on a held PR at all.

Review

Codex (quest review, run from this branch's source), gpt-6.1-sol, xhigh effort, in a new session after the design checkpoint (quest review --new).

• Round 1 on 2ccd142c, against origin/main at 05aebae9: no findings.

The earlier sessions' rounds (seven, plus Pullfrog) reviewed the held comment's bookkeeping this change removes; see the Design change.

Pullfrog's threads: discussion_r4470352858 (the held comment said "was not applied" after a push) and discussion_r4472838625 (a recovered retry re-derived that wording) are both moot: the comment has one wording that claims nothing about the fix, and the retry path is gone. The second is answered and resolved.

Suggested follow-ups

None.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The watcher now checks pull-request hold labels before starting fixer work, publishing results, or re-enqueueing. It records held removal outcomes before posting a best-effort comment. The fixer and flaky-test disable flows leave held pull requests for a maintainer to enqueue after the hold clears. Tests cover holds observed at multiple stages.

Merge Risk: Minimal · up to 2ccd1

No actionable issue remains from this review. The change is mergeable after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title "fix(land-watch): leave a PR another session holds to that session" matches the main change. The watcher now detects hold labels and ends a held PR's pick, stops acting on it, and leaves it …
Description check		The description explains the problem (quest land watch never read PR labels and acted on PRs held by another session), the change (hold detection at each step, held verdict, HELD comment, state change…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "fix(land-watch): leave a PR another session holds to that session" matches the main change. The watcher now detects hold labels and ends a held PR's pick, stops acting on it, and leaves it to the holding session. The title is specific, concise, and uses the Conventional Commits style.

Full details: Description check

Explanation

The description explains the problem (quest land watch never read PR labels and acted on PRs held by another session), the change (hold detection at each step, held verdict, HELD comment, state changes), validation steps, and design decisions. It is directly related to the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.)

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• 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

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

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: eec75c8b-d54f-4361-9f50-41b9fdee875f

Commits

Reviewing files that changed from the base of the PR and between 844c993 and 2ccd142.

Files selected for processing: 6

• README.md
• quest/m0/land-fixer/README.md
• quest/m0/land-fixer/holds.md
• src/cli/land-watch-fix.ts
• src/cli/land-watch.ts
• test/land-watch.test.ts

Files with no reviewable changes: 2

• quest/m0/land-fixer/README.md
• quest/m0/land-fixer/holds.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[pullfrog]

ℹ️ One minor wording accuracy point inline — nothing blocking.

Reviewed changes — reviewed head bd6deef against main (full diff, all 6 files), traced every push and enqueue site in the watcher path against the hold checks, and ran the suite locally (test/land-watch.test.ts: 23 passed; bun run check: tsc + prettier pass).

• Hold detection plumbing: labels join the open-PR snapshot and the PULL query; holdOf reads any quest:* label but the legacy quest:merge, reusing isHoldLabel from src/core/hold.ts, so the semantics match the hold machinery the other commands use.
• Removal handling: a fixable-reason removal of a held PR gets verdict held instead of fix — no fixer plan is recorded, the pick is ended, and the removal comment says the watcher leaves it to the holding session.
• Fixer gates: the hold is re-checked at each action point — after the already-started-job check in take and before the conflict merge and the slot wait, on a fresh read in collect before anything is pushed, with the pick upheld when the round's own re-enqueue was already accepted by GitHub, again right before publishHead to close the validation-window race, and in requeue before an enqueue of a head GitHub has not taken. Held thrown from any of these is folded into a single held comment per pick, with the fixer's note dropped.
• Armed picks and disable PRs: enqueueArmed leaves a held armed pick armed for the pass after the hold clears, and openDisable treats a held disable PR like a refused one — commented once and left for a maintainer, not retried each pass.
• Tests and docs: the new fixer test covers a hold at the removal, one taken while the fixer runs, one taken during fix validation (verifying the origin branch is untouched), and one taken while a removal waits for a fixer slot, plus quest:merge holding nothing; the armed-pick test covers both hold timings and the enqueue once the holds clear. The README's watcher section documents the behavior and quest records were cleaned up consistently.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using glm-5.3-flash ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues — the new persistence-ordering commits check out; one minor wording suggestion inline, of the same class as the comment from the previous review.

Reviewed changes — reviewed the delta since pullfrog's prior review of bd6deef: commits 411c13b, 58823be, fa70c6b, bb74740, and 1ec89e3 plus the merges of main. Traced every hold-recording site end to end and ran the suite locally (test/land-watch.test.ts: 25 passed; bun run check: tsc + prettier pass).

• Hold recorded before the comment (411c13b, 58823be, fa70c6b, bb74740): each site that posts a held or refused comment now persists the hold first — leave() writes job.held and short-circuits a retry into comment-only, take saves removal.hold before its comment, handle saves the removal record (with hold) and ends the pick before its comment, and openDisable records Disable.refused before its comment — so a hold seen once ends the pick even if GitHub refuses every comment and the label clears before the retry.
• Says the fix was pushed when it was (411c13b): collect tracks whether the PR's head is the round's fix, and the held comment picks between "was pushed as <head>, but not re-enqueued" and "was not applied"; the held test asserts both comment texts.
• Recovery keeps a taken requeue (1ec89e3): the pre-push hold read runs only when a push is still needed, so a crash that lost a round's requeue result no longer drops a head GitHub already queued; requeue still checks the hold before any enqueue, and the pick ends when the hold is recorded, so a round set aside for refused comments leaves no pick open.
• Tests and docs: the flaky-disable test is parametrized over a hold on the disable PR as it opens (comment refused once, left for a maintainer after the hold clears), the held test grows a scenario for a hold while the removal waits with refused comments retried on the next pass, World.refuse becomes a list, and World.onPull fires on each single-PR read. The README's watcher section documents the behavior and the quest records were cleaned up consistently.

Only one minor finding inline — see the suggested one-line derivation of pushed.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using glm-5.3-flash ｜ 𝕏}

[kixelated]

Design change: simplify the held PR's bookkeeping, and comment on it best-effort.

Settled by the maintainer in /quest-complete on 2026-11-04, after seven local review rounds and Pullfrog kept finding gaps in the held comment's bookkeeping (persisting it before posting, retrying refused comments, "pushed as X" vs "not applied" wording), not in the rule itself.

• How should this PR continue after the design checkpoint?
  • ✅ Simplify, comment best-effort: keep the rule (the hold ends the pick, as decided earlier). At every step where the watcher can see a hold (removal, fixer start, finished round, requeue, flaky-test disable PR), a held PR ends its pick, saved to state first, and the watcher stops acting on it. It then posts one hold comment best-effort: no retry, no persisted "comment pending" state, and one wording that doesn't depend on whether a fix was pushed. job.held, Disable.refused, the comment-retry paths, and their tests go; the tests for the rule at each step stay.
  • Rejected: applying Pullfrog's one-line patch and merging.
  • Rejected: never touching or commenting on a held PR at all.

The next local review starts a new session with quest review --new.

(Written by Claude Opus 5.5)

[pullfrog]

✅ No new issues found.

Reviewed changes — reviewed the delta since pullfrog's prior review of b46de7c0: commits a2b69f7 (clean merge of main) and 2ccd142c, the simplification settled at the design checkpoint. Traced every site that can see a hold end to end against its pick's state writes, and ran test/land-watch.test.ts (25 passed) and bun run check (tsc + prettier) locally.

• One wording, best-effort comments (2ccd142c): holdOf/heldText are replaced by isHeld and a single HELD constant, and every hold comment goes through the new sayHeld, which warns about a GitHub refusal and never retries.
• State before comment, everywhere: collect's leave() now runs endPick → finish → sayHeld, take runs mark("held") → endPick → sayHeld, openDisable records done: "held" before its comment, and handle saves the removal record before its comment with only a held verdict's comment best-effort — a refusal on any other verdict still fails the pass, as before. The persisted-hold bookkeeping is gone (Removal.hold, job.held, Disable.refused, and their retry tests), so no site can claim a pushed/worded state anymore.
• Pre-push hold read keyed off ours: the fresh label read before publishHead runs unless the round's fix is already the PR's head, and requeue still checks the hold before enqueuing a head GitHub has not taken; a head this round already had queued stays queued.
• A held round is dropped cleanly: its fixer note and flaky verdict are not recorded, so the hold's comment is the only one after the removal's; a removal the fixer took while held keeps taken: "held", which advanceFixes' taken === undefined filter never re-takes, and handle's prior.verdict === "held" covers a crash before its comment.
• Tests and docs: the held test asserts exactly one held comment per PR once the holds clear ([1, 1, 0, 1, 1] for 5–9), simulates the crash losing a handled mark, and keeps the timings at the removal, during validation, after the push, while the fixer runs, and while a slot is waited on; the flaky-disable test records done: "held" with the comment refused; the armed-pick checks stand. The README's watcher section documents the best-effort comment and the held disable PR.

  ^{｜ View workflow run ｜ Using glm-5.3-flash ｜ 𝕏}