From 7e77794deca7a0e0305991e9550e5878a2d0fbd3 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 10:41:14 -0800 Subject: [PATCH 1/3] quest: plan the merge-queue audit's levers and reverse three fixer decisions From an independent audit and the maintainer's decisions on 2026-11-01: drop protected.md (validateFix keeps the block, ready commits instead), disable only rerun-proven flaky tests in a separate PR, and add the batch quest-check, queued-PR pre-check, and throughput tuning quests. Co-Authored-By: Claude Opus 5.5 --- quest/m0/host-land/fixer.md | 6 +- quest/m0/land-fixer/README.md | 71 ++++++++++++++------- quest/m0/land-fixer/batch-check.md | 25 ++++++++ quest/m0/land-fixer/flaky.md | 18 ++++-- quest/m0/land-fixer/fork-commit.md | 4 -- quest/m0/land-fixer/precheck.md | 33 ++++++++++ quest/m0/land-fixer/protected.md | 44 ------------- quest/m0/land-fixer/tuning.md | 38 +++++++++++ quest/m0/land-fixer/workflows-permission.md | 4 -- 9 files changed, 162 insertions(+), 81 deletions(-) create mode 100644 quest/m0/land-fixer/batch-check.md create mode 100644 quest/m0/land-fixer/precheck.md delete mode 100644 quest/m0/land-fixer/protected.md create mode 100644 quest/m0/land-fixer/tuning.md diff --git a/quest/m0/host-land/fixer.md b/quest/m0/host-land/fixer.md index acdaabcb..007dbe32 100644 --- a/quest/m0/host-land/fixer.md +++ b/quest/m0/host-land/fixer.md @@ -10,8 +10,10 @@ mechanical fix and unchanged requeue through the host daemon. Maintainer decision, 2026-10-30: a `fixed` head lands without further review. "Mechanical" stays prompt-enforced, protected paths are blocked, and the residual -prompt-injection risk is accepted. Superseded 2026-11-01 (user): the fixer edits -protected paths it judges mechanical; see [Protected edits](/quest/m0/land-fixer/protected.md). The built-in prompt treats contributor content +prompt-injection risk is accepted. Protected paths stay blocked: on 2026-11-01 the +maintainer briefly let the fixer edit them (moq-dev/quest#357), then reverted that +after an audit, routing fixer-written protected edits through a ready commit +instead ([Fork commits](/quest/m0/land-fixer/fork-commit.md)). The built-in prompt treats contributor content as untrusted and ignores instructions in the PRs' diffs, without handing the PR back for them. diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index 8f8e96a7..dbf8511d 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -2,22 +2,29 @@ ## Goal -The land gate dequeues a PR only when a decision is required. When a batch -fails or a queued PR conflicts, the gate and its fixer resolve every -mechanical cause themselves and land the result, so a maintainer sees only -behavior and design decisions. The fixer starts from the base with internet -access and the batch's PRs and head SHAs. It ignores instructions in their -diffs, pushes its fixes onto the PRs' branches, disables a flaky test on the -batch's base PR with an investigation quest, and comments on GitHub. Nothing leaves the queue -for a conflict or a failure: a PR the fixer is working on keeps its -auto-merge and its place, sits out batches until the fix is in, and leaves -the queue only when the fixer hands it back. The fixer blames the failing PR -before anything else happens. The gate merges locally instead of -trusting GitHub's mergeability, reruns a failed module before blaming, and -disables a flaky test with a follow-up quest. The fixer builds and tests -with access to package registries, edits protected paths when the edit is -mechanical, and spends a pick's single attempt only on a fix that failed. -When GitHub refuses its push to a fork, it hands back a ready commit. +The land gate dequeues a PR only when a decision is required, and lands as +many PRs as it safely can. When a batch fails or a queued PR conflicts, the +gate and its fixer resolve every mechanical cause themselves and land the +result, so a maintainer sees only behavior and design decisions. + +- Nothing leaves the queue for a conflict or a failure. A PR the fixer is + working on keeps its auto-merge and its place, sits out batches until the + fix is in, and leaves the queue only when the fixer hands it back. The fixer + blames the failing PR before anything else happens, and a pick's single + attempt is spent only on a fix that failed. +- Broken PRs stay out of batches. The daemon runs `quest check` on a batch's + chain before the sandbox, and pre-checks queued PRs against the base while + a batch runs. Batch size follows the measured failure rate. +- The fixer starts from the base with internet access and the batch's PRs and + head SHAs. It ignores instructions in their diffs, builds and tests its + fixes, and pushes them onto the PRs' branches. The daemon posts the + comments the fixer writes into its result. +- The gate merges locally instead of trusting GitHub's mergeability, and + reruns a failed module before blaming. A rerun-proven flaky test is + disabled in a separate PR with a follow-up quest. +- Protected paths stay blocked for the fixer's own edits. A fixer-written + protected edit, or a push GitHub refuses, comes back as a ready commit the + maintainer applies with one command. Non-goals: design conflicts. A PR whose merge needs a behavior decision is still handed back to its author. @@ -37,7 +44,7 @@ moq-dev/rust2, moq-dev/rind and moq-dev/quest. | 98 | "already used its mechanical fix attempt" after a collateral requeue or an earlier conflict fix; each needed a manual re-pick | [Attempt budget](/quest/m0/land-fixer/attempts.md), [Batch triage](/quest/m0/land-fixer/triage.md) | | ~75 | "requeued unchanged": collateral of a batch another PR broke | [Batch triage](/quest/m0/land-fixer/triage.md) | | 36 | picked again while fixing; the newer pick is kept, so nothing is lost | not planned | -| 17 | conflicts in protected paths (`flake.nix`, `.github/`), mostly mechanical; resolved by hand on moq-dev/moq#7344, #7315, #7318 | [Protected edits](/quest/m0/land-fixer/protected.md) | +| 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 ([Fork commits](/quest/m0/land-fixer/fork-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 | [Fork commits](/quest/m0/land-fixer/fork-commit.md) | | several batches | a flaky ffmpeg timeout ([moq-dev/moq#7480](https://github.com/moq-dev/moq/pull/7480)) ejected whole batches | [Flaky tests](/quest/m0/land-fixer/flaky.md) | @@ -60,7 +67,7 @@ Decision prompts, selected options marked ✅: allows fetches from package registries and `github.com`/`codeload.github.com` and makes the fixer build and test what it reports. - Leave it -- Protected paths: see [Protected edits](/quest/m0/land-fixer/protected.md). +- 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: see [Fork commits](/quest/m0/land-fixer/fork-commit.md). - Flaky tests: see [Flaky tests](/quest/m0/land-fixer/flaky.md). - Milestone for the new quests: ✅ m0 · m1 @@ -96,15 +103,35 @@ Decision prompts, selected options marked ✅: The README's landing section and `CONTRIBUTING.md` list what the fixer resolves itself and what it hands back; that docs update finishes the epic. +- Audit of the merge-queue plan, 2026-11-01 (an independent read-only + agent; the maintainer then decided each point). On moq over 24 hours: 128 + landed and 390 ejected; 66 of 138 sandbox checks failed; batches stayed at + 2-3 PRs. The binding bottleneck is the batch failure rate, roughly 20-25% of + PRs broken against the current base, and not fixer capability. Decisions: + - Protected paths: ✅ keep `validateFix`'s block and route fixer-written + protected edits through [Fork commits](/quest/m0/land-fixer/fork-commit.md)'s + ready commit; moq-dev/quest#357 closed · keep #357 · #357 minus workflows + - Fixer comments: ✅ the daemon posts them from the fixer's result, no + token in the container (moq-bot comments carry trust: `quest land apply`, + the daemon's markers, `quest status`) · keep the token + - Flaky tests: ✅ disable only a rerun-proven flaky test, in a separate PR + · keep the fixer's judgment and the base PR + - New work, all picked: ship [Attempt budget](/quest/m0/land-fixer/attempts.md) + right after triage; [the batch check](/quest/m0/land-fixer/batch-check.md); + [pre-checks](/quest/m0/land-fixer/precheck.md); and + [tuning](/quest/m0/land-fixer/tuning.md) + ## Required +- [Batch triage](/quest/m0/land-fixer/triage.md) - the fixer blames the failing PR before ejection, and a red base halts the queue +- [Attempt budget](/quest/m0/land-fixer/attempts.md) - only a fix that fails again spends a pick's single attempt +- [Batch quest check](/quest/m0/land-fixer/batch-check.md) - the daemon runs `quest check` on the batch's chain before spending a sandbox check +- [Pre-check queued PRs](/quest/m0/land-fixer/precheck.md) - compile and lint the next PRs against the base while a batch runs, parking broken ones before they batch +- [Throughput tuning](/quest/m0/land-fixer/tuning.md) - batch size from the failure rate, no wasted base-conflict rounds, rerere, a fixer cap, and an explicit reasoning effort - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted - [Fixer comments](/quest/m0/land-fixer/comments.md) - a per-job, comment-only GitHub token lets the fixer explain its work on the PRs -- [Attempt budget](/quest/m0/land-fixer/attempts.md) - only a fix that fails again spends a pick's single attempt -- [Batch triage](/quest/m0/land-fixer/triage.md) - the fixer blames the failing PR before ejection, and a red base halts the queue -- [Flaky tests](/quest/m0/land-fixer/flaky.md) - the gate reruns a failed module, disables a flaky test on the batch's base PR, and files a follow-up +- [Flaky tests](/quest/m0/land-fixer/flaky.md) - the gate reruns a failed module, and a rerun-proven flaky test is disabled in a separate PR with a follow-up quest - [Local mergeability](/quest/m0/land-fixer/mergeability.md) - `quest land` merges with the quest driver instead of ejecting on GitHub's conflict flag -- [Protected edits](/quest/m0/land-fixer/protected.md) - the fixer edits protected paths when it judges the edit mechanical, and the fix lands - [Fork commits](/quest/m0/land-fixer/fork-commit.md) - a fork push GitHub refuses hands back a commit a maintainer session applies as is ## Related diff --git a/quest/m0/land-fixer/batch-check.md b/quest/m0/land-fixer/batch-check.md new file mode 100644 index 00000000..353130cb --- /dev/null +++ b/quest/m0/land-fixer/batch-check.md @@ -0,0 +1,25 @@ +# [XS] The daemon runs quest check on the batch before the sandbox + +## Goal + +Before it spends a sandbox check on a batch, `quest land` runs `quest check` +on the batch's combined squash chain inside the daemon. A failure there is +blamed on the link that introduced it, the same way +[Batch triage](/quest/m0/land-fixer/triage.md) blames a failing check: that +PR is parked for the fixer, and the batch is rebuilt without it. No sandbox +time is spent. + +## Plan + +Decided 2026-11-01 (maintainer, from the audit of the merge-queue plan; see +the [epic](/quest/m0/land-fixer/README.md)). In the audit's 24 hours on moq, +`check quest` was the first failing module in 9 of 66 failed batches (about +14%), even though each PR passed `quest check` alone. Two PRs can each pass +alone yet together leave a dangling link or a `Required` cycle. The daemon +already screens each candidate alone (`screener()` in `src/cli/land.ts`), and +`quest check` on the chain tip costs well under a second, so check the chain +too. + +## Required + +- [Batch triage](/quest/m0/land-fixer/triage.md) - provides parking and blame diff --git a/quest/m0/land-fixer/flaky.md b/quest/m0/land-fixer/flaky.md index 8db0b0d7..2b4545d5 100644 --- a/quest/m0/land-fixer/flaky.md +++ b/quest/m0/land-fixer/flaky.md @@ -4,11 +4,11 @@ When a batch's check fails, `quest land` reruns the failing check modules once on the same tip before blaming or ejecting anything. A test that failed -and then passed, or whose failure the fixer judges nondeterministic (for -example a timeout under load, or a race), is flaky. The fixer pushes one -commit onto the batch's base PR that disables the test with a reason and adds -a follow-up quest to fix or delete it in the repository's m0 `flaky` epic. -The batch then lands with that commit. +and then passed on that rerun is flaky. The batch lands, and the fixer opens a +separate PR that disables the test with a reason and adds a follow-up quest +to fix or delete it in the repository's m0 `flaky` epic. The gate queues that +PR like any other, with no human pick needed. A failure the fixer only +suspects is nondeterministic is not disabled. ## Plan @@ -42,6 +42,14 @@ was written and landed by hand. - The earliest milestone, ungrouped - m1 +- Superseded again 2026-11-01 (maintainer, from the audit of the merge-queue + plan): PRs land as squashes, so a disable commit on the batch's base PR + would land inside that unrelated PR's commit, under its title; and + disabling on the fixer's judgment alone risks hiding real bugs. + - ✅ Only a rerun-proven flaky test, disabled in a separate PR the gate + queues + - Keep: rerun or the fixer's judgment, with the commit on the base PR + How the gate reruns one module is up to the repository. `quest land` passes the failed module names to the check command (for example through `QUEST_LAND_RERUN`). A check command that ignores them reruns whole. moq's diff --git a/quest/m0/land-fixer/fork-commit.md b/quest/m0/land-fixer/fork-commit.md index accf2d79..b90cc934 100644 --- a/quest/m0/land-fixer/fork-commit.md +++ b/quest/m0/land-fixer/fork-commit.md @@ -32,7 +32,3 @@ against the merge's conflicted result. A command such as `quest land apply base, applies the patch, checks the tree hash, and leaves a commit for the maintainer to push. Size the comment to GitHub's limit and hand back plainly when the patch does not fit. - -## Related - -- [Protected edits](/quest/m0/land-fixer/protected.md) - the fixer's own workflow edits diff --git a/quest/m0/land-fixer/precheck.md b/quest/m0/land-fixer/precheck.md new file mode 100644 index 00000000..440a9e7c --- /dev/null +++ b/quest/m0/land-fixer/precheck.md @@ -0,0 +1,33 @@ +# [M] Queued PRs are pre-checked while a batch runs + +## Goal + +While a batch's check runs, the daemon pre-checks the next queued PRs, each +alone against the current base, with the repository's quick command (compile +and lint, not tests), at a lower priority than the batch check. A PR that +fails is parked for the fixer with the output before it enters a batch, so a +broken PR stops costing a whole batch. + +## Plan + +Decided 2026-11-01 (maintainer, from the audit of the merge-queue plan; see +the [epic](/quest/m0/land-fixer/README.md)). The audit found the batch +failure rate is the binding bottleneck: about half of moq's sandbox checks +failed, which means roughly 20-25% of PRs were broken against the current +base. A batch of k PRs lands about k·(1−p)^k PRs per check, so cutting p from +0.2 to 0.1 roughly doubles throughput. moq's `check rs` fails in about 26 s, +so a compile and lint pre-check catches most of these cheaply. + +- The repository declares the pre-check command beside its check command + (moq: something like `just check-changed `). Run it in the same kind + of sandbox as the batch check, never on the host. +- Run below the batch check's priority so it cannot slow the batch; the + `land-priority` work already provides the priority classes. +- Pre-check a PR at most once per (head, base) pair; a moved base re-queues + it for pre-check only when the PR is next in line. +- A pre-check failure parks the PR through [Batch triage](/quest/m0/land-fixer/triage.md)'s + mechanism, and the fixer gets the pre-check log as its failure. + +## Required + +- [Batch triage](/quest/m0/land-fixer/triage.md) - provides parking diff --git a/quest/m0/land-fixer/protected.md b/quest/m0/land-fixer/protected.md deleted file mode 100644 index dcfb5162..00000000 --- a/quest/m0/land-fixer/protected.md +++ /dev/null @@ -1,44 +0,0 @@ -# [S] The fixer edits protected paths when the edit is mechanical - -## Goal - -The fixer may resolve conflicts and failures in protected paths (`.github/`, -`flake.nix`, `nix/`, justfiles, and the rest of the protected set) when it -judges the edit mechanical. A head with such an edit lands like any other -fix. - -## Plan - -Decided 2026-11-01 (user, /quest-plan of the land fixer audit; see the -[epic](/quest/m0/land-fixer/README.md)). This reverses -[Fixer proxy access](/quest/m0/host-land/fixer.md)'s 2026-10-30 decision -that protected paths are blocked. 17 handbacks were protected-path -conflicts, most of them mechanical: keep both sides, or drop a tool the base -removed. Maintainers resolved them by hand, for example on -[moq-dev/moq#7344](https://github.com/moq-dev/moq/pull/7344), -[#7315](https://github.com/moq-dev/moq/pull/7315), and -[#7318](https://github.com/moq-dev/moq/pull/7318). - -- First round: - - Resolve verbatim hunks only - - Hand back a ready commit - - Keep blocking - - User's answer: "Up to the fixer to determine what is mechanical" -- Second round, challenging the earlier decision, since a fixer head lands - with no further review: - - The fixer decides, but a protected edit needs a counted review - - ✅ The fixer decides, and the fix lands. User: "Any prompt injections - should be caught before enabling auto merge"; the maintainer's pick is - the injection check - - Workflows stay blocked - -Remove the protected-set check from `validateFix` (`src/cli/land-fixer.ts`, -"protected set must equal pinned head or base merge result"). Replace the -protected-path rule in `assets/land-fix.md` with the same mechanical bar -every other path has. Instructions in the PRs' diffs stay ignored on these -paths as on any other. Update the decision line in -[Fixer proxy access](/quest/m0/host-land/fixer.md). - -## Related - -- [Fork commits](/quest/m0/land-fixer/fork-commit.md) - workflow edits GitHub refuses to take from the App on forks diff --git a/quest/m0/land-fixer/tuning.md b/quest/m0/land-fixer/tuning.md new file mode 100644 index 00000000..fe18501b --- /dev/null +++ b/quest/m0/land-fixer/tuning.md @@ -0,0 +1,38 @@ +# [S] Tune batching and the fixer for throughput + +## Goal + +Once failing PRs are parked instead of ejected, the gate stops paying for +choices made while a failure ejected a whole batch: + +- Batch size follows the measured per-PR failure rate, not halving on each + failure. +- A conflict on the batch's base PR parks that PR and fills the batch in the + same round, without spending a round on it. +- The gate's repository enables `git rerere`, so it replays conflict + resolutions it has already made. +- Concurrent fixer jobs are capped and run below the batch check's priority. +- The fixer runs with an explicit reasoning effort; today Codex reports + `reasoning effort: none`. + +## Plan + +Decided 2026-11-01 (maintainer, from the audit of the merge-queue plan; see +the [epic](/quest/m0/land-fixer/README.md)). Audit findings on moq's 24 hours: + +- Halving after each failure kept batches at 2-3 PRs. With failing PRs + parked, size from the recent failure rate instead (about 4-5 at today's + rate), keeping `batch-policy.json` as the state. +- 156 rounds were a base-PR conflict that ran no check, about 1.5-2.5 hours + of lane time a day. +- 44 of 79 conflicting PRs conflicted 2-7 times; `rerere` lets the gate's + own base merges replay resolutions. +- Seven fixer jobs started within three minutes on a host already at load + 18-25 of 32 cores, and timeouts under load caused several flaky failures. +- Fixer job logs show `reasoning effort: none`. `land-fix.ts` passes a model + but no `model_reasoning_effort`; set one, defaulting to `medium`, with a + daemon option to override it. + +## Required + +- [Batch triage](/quest/m0/land-fixer/triage.md) - the sizing change assumes failures park diff --git a/quest/m0/land-fixer/workflows-permission.md b/quest/m0/land-fixer/workflows-permission.md index 84345a4f..48759c06 100644 --- a/quest/m0/land-fixer/workflows-permission.md +++ b/quest/m0/land-fixer/workflows-permission.md @@ -29,7 +29,3 @@ happening; whether to keep that quest and review. Delete this quest once the permission is granted and accepted. - -## Related - -- [Protected edits](/quest/m0/land-fixer/protected.md) - the fixer edits workflows when the change is mechanical From 3651ea9762c7959604959f95857182eac06ffe20 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 10:42:26 -0800 Subject: [PATCH 2/3] quest: protected edits come back as ready commits; comments stay in the daemon Co-Authored-By: Claude Opus 5.5 --- quest/m0/land-fixer/README.md | 3 ++- quest/m0/land-fixer/protected-commit.md | 31 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 quest/m0/land-fixer/protected-commit.md diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index dbf8511d..85d6d0bf 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -129,10 +129,11 @@ resolves itself and what it hands back; that docs update finishes the epic. - [Pre-check queued PRs](/quest/m0/land-fixer/precheck.md) - compile and lint the next PRs against the base while a batch runs, parking broken ones before they batch - [Throughput tuning](/quest/m0/land-fixer/tuning.md) - batch size from the failure rate, no wasted base-conflict rounds, rerere, a fixer cap, and an explicit reasoning effort - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted -- [Fixer comments](/quest/m0/land-fixer/comments.md) - a per-job, comment-only GitHub token lets the fixer explain its work on the PRs +- [Fixer comments](/quest/m0/land-fixer/comments.md) - the fixer writes comments into its result and the daemon posts them; no token enters the container - [Flaky tests](/quest/m0/land-fixer/flaky.md) - the gate reruns a failed module, and a rerun-proven flaky test is disabled in a separate PR with a follow-up quest - [Local mergeability](/quest/m0/land-fixer/mergeability.md) - `quest land` merges with the quest driver instead of ejecting on GitHub's conflict flag - [Fork commits](/quest/m0/land-fixer/fork-commit.md) - a fork push GitHub refuses hands back a commit a maintainer session applies as is +- [Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md) - a fixer edit `validateFix` rejects for touching protected paths comes back as a commit the maintainer applies ## Related diff --git a/quest/m0/land-fixer/protected-commit.md b/quest/m0/land-fixer/protected-commit.md new file mode 100644 index 00000000..3cef4a30 --- /dev/null +++ b/quest/m0/land-fixer/protected-commit.md @@ -0,0 +1,31 @@ +# [S] A protected edit the fixer writes comes back as a ready commit + +## Goal + +When `validateFix` rejects a fixer result because it edits a protected path +(`.github/`, `flake.nix`, `nix/`, justfiles, gitlinks) beyond what the base +merge carries, the handback carries that result as a ready commit, the same +form as [Fork commits](/quest/m0/land-fixer/fork-commit.md)' refused pushes. +The maintainer reviews it and applies it with `quest land apply `, then +pushes and re-picks. Protected edits never land automatically. + +## Plan + +Decided 2026-11-01 (maintainer, from the audit of the merge-queue plan; see +the [epic](/quest/m0/land-fixer/README.md)). The maintainer kept +`validateFix`'s protected-path block and closed +[moq-dev/quest#357](https://github.com/moq-dev/quest/pull/357), which would +have let these edits land unreviewed. moq-dev/moq has about 25 release +workflows with publishing secrets, and a merge pick covers the PR's diff, +not the fixer's later resolution. About 17 such handbacks happened in 3 days, +most of them mechanical, so a one-command ready commit keeps the human review +cheap. + +Reuse [moq-dev/quest#358](https://github.com/moq-dev/quest/pull/358)'s +handback format and `quest land apply`. Record the PR head, base, expected +tree hash, and the patch against the merge's conflicted result. Size it to +GitHub's comment limit, and hand back plainly when it does not fit. + +## Required + +- [Fork commits](/quest/m0/land-fixer/fork-commit.md) - provides the ready-commit format and `quest land apply` From ab82fdeddd23941ee6852dba20473c9ce5a36003 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 10:42:54 -0800 Subject: [PATCH 3/3] quest: rewrite the comments quest to the daemon-posted design Co-Authored-By: Claude Opus 5.5 --- quest/m0/land-fixer/comments.md | 56 ++++++++++++++++----------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/quest/m0/land-fixer/comments.md b/quest/m0/land-fixer/comments.md index 23740fc5..8e4159f0 100644 --- a/quest/m0/land-fixer/comments.md +++ b/quest/m0/land-fixer/comments.md @@ -1,40 +1,38 @@ -# [S] The fixer comments on GitHub +# [S] The fixer comments on GitHub through the daemon ## Goal -The fixer can post and edit comments on the batch's PRs, for example to -explain a fix, a disabled flaky test, or the decision it needs. It does this -with a GitHub installation token for that one repository with only -`issues: write`, which the daemon mints for each job and which expires with -it. The token cannot push code, merge, or reach any other repository. As the -maintainer accepted, it can also edit or close issues and change labels in -that repository. +The fixer can explain its work on the batch's PRs (a fix, a rerun-proven +flaky test, or the decision it needs) by writing comments into its result: +each has a target PR number from its job and a Markdown body, at most one per +PR, with a size cap. When it collects the job, the daemon validates and posts +them. It redacts tokens, strips hidden markers so the fixer's text cannot +pose as the daemon's or the fixer's own, and drops any comment aimed at a PR +outside the job. No GitHub credential enters the fixer container. ## Plan Decided 2026-11-01 (user, /quest-plan): "It's also allowed to comment on Github." See the [epic](/quest/m0/land-fixer/README.md). -- How the fixer gets to comment: - - The daemon posts the comments the fixer writes into its result, so no - GitHub credential enters the container (recommended, because PR code the - fixer builds can read the container's environment, as - [moq-dev/quest#356](https://github.com/moq-dev/quest/pull/356) found for - the proxy key) - - ✅ A comment-only token in the container +- How the fixer gets to comment, first round: + - The daemon posts the comments the fixer writes into its result + - A comment-only token in the container (picked) - Both +- GitHub has no comment-only permission, so the token was `issues: write` + (picked: accept it). +- Reversed the same day after an independent audit, because PR code the fixer + builds can read the container's environment and post as moq-bot[bot]. Three + things trust moq-bot comments: `quest land apply` reads a bot comment that + carries the fixer's marker, the daemon skips a post when its own marker is + already there, and `quest status` reads the gate's hidden ejection reason. + - ✅ The daemon posts them from the fixer's result + - Keep the token in the container -- GitHub has no comment-only permission; `issues: write` also allows editing - and closing issues and changing labels, and PR code the fixer builds could - read the token (Codex review of this plan): - - A local comment command: the daemon keeps the credential and exposes - only comment operations inside the container (recommended) - - ✅ Accept `issues: write` in the container - - The daemon posts comments from the result - -Mint an installation token for the job's repository with -`permissions: { issues: "write" }`, which covers PR conversation comments, and -`repositories: []`. Pass it to the agent as `GH_TOKEN` only; the wrapper -keeps every other credential out, as today. GitHub caps a token at one hour, -so a longer job gets a fresh one or stops commenting. The daemon keeps posting -its own requeue and handback comments. +Implemented in [moq-dev/quest#367](https://github.com/moq-dev/quest/pull/367), +which is being reworked to this design. The daemon keeps posting its own +requeue and handback comments. + +## Related + +- [Fork commits](/quest/m0/land-fixer/fork-commit.md) - trusts the fixer's bot comment marker