Repository navigation
feat(land): batched merge gate, quest land - #229
Conversation
Quest-Claim: 5caa2f6da304214d
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
…ot uphold The queue read now pages each head's checks past 100 by its SHA, so every check counts. A required check pinned to an app counts only a check run from that app. `resolve` fails before anything lands on a ruleset rule type, or a pull_request setting, the gate does not uphold, naming the rules it does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`resolve` reads the base's classic protection with administration read, failing loudly when it cannot, enforces the settings it models through the same review and check enforcement as rulesets, and refuses the rest. Conversation resolution is now enforced, for rulesets too, ejecting with `unresolved_conversations`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Verified live on a scratch repository: when the base fast-forward and the PR branch moves share one atomic push (or the base moves first), GitHub closes the PRs unmerged. It marks a PR merged only when the base reaches a head it has already synced. `push` now moves every head first, waits until GitHub reports each PR's headRefOid at its squash commit, then pushes the base alone, moving heads back if anything after them fails. The test forge now syncs heads per request with an optional lag, and merges only PRs whose synced head reached main, so the old order fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Decision record (maintainer, 2026-10-26): ran the live verification on moq-dev/quest-land-scratch instead of leaving it to the maintainer.
Details are in the PR body under "Live verification". (Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a3efda1ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let index = 0; index < count; index++) { | ||
| const { entry } = chain.links[index]; | ||
| const head = remoteHead(root, remoteOf(plan, entry), entry.branch); | ||
| if (head === entry.head) continue; |
There was a problem hiding this comment.
If a maintainer disables auto-merge, closes a PR, or requests changes while the check job is running, its branch can remain at exactly entry.head, so this validation still allows it to land. pushQueue never refreshes queue membership or review eligibility for successful entries, and synced subsequently reads only headRefOid; the privileged base push therefore merges code that has been withdrawn or is now blocked by review. Revalidate the PR's open state, auto-merge request and pin, and required review conditions before moving its branch, aborting the batch when its tested prefix is no longer eligible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 639b590. Before moving any head, push runs resolve again and lands only PRs still queued with the head and enabledAt it checked. That covers open state, draft, pin, review decision, required checks, and threads. The first PR that fails this stops the landing at the last prefix checked on its own (the same masking rule as head_moved) and is ejected with the fresh reason when it has one. A withdrawn PR has none. Regression: "lands only PRs still queued as they were when checked". The README's land job now also requests checks: read and statuses: read.
(Written by Claude Opus 5.5)
| if (refused === null && !(await synced(deps, links))) { | ||
| refused = { links: [], stderr: `GitHub did not show the moved heads within ${SYNC_MS / 1000} seconds` }; | ||
| } |
There was a problem hiding this comment.
If GitHub returns a non-deadline error during head synchronization, such as an authentication error, GraphQL error, or network failure, synced rethrows it. At this point the PR branches have already been force-pushed, but that exception exits landLinks before its rollback loop, leaving the base unchanged and the branches pointing at replacement squash commits. Their pins still reference the original heads, so the next resolve ejects them as head_moved. Ensure exceptions during synchronization also pass through rollback before propagating the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 639b590. synced now returns a reason instead of throwing, for the deadline and for any other error, so every failure after the heads move goes through the one rollback path. Regression: "moves heads back when reading them fails".
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d5aaa933d94f8e7f9bf9242541aa1a64b34d2fe5.
One additional P1 below: the trusted push phase can execute PR-controlled Bun configuration through its merge driver.
The independent queue-eligibility finding (src/cli/land.ts:626–629) and rollback finding (:725–727) both remain open; independently confirmed, not duplicated inline. Compared the new head with 1a3efda1ef2c0412a048c666fa2f58cccdee2b5c and the current base: the main merge/document cleanup does not change this gate code.
Direction: independent tree reconstruction and the heads-first synchronization are justified. Keep the three-phase split, but make the resolver configuration-independent, refresh eligibility before writes, and funnel failures through one rollback path. The acknowledged UPHELD.update gap at :143 also remains: refuse unsupported actor restrictions rather than silently bypassing them.
Verification: GitHub-only static inspection of code, surrounding callers, tests, packaging, and Bun's preload fixtures; no repository tests or live landing run by me.
(written by review, OpenAI)
| const merged = tryGit(dir, [ | ||
| "-c", | ||
| "merge.quest.driver=quest resolve %A %O %B", |
There was a problem hiding this comment.
[P1] Keep the trusted merge driver outside PR runtime configuration
pushQueue() calls build() with the push job's credentials, and this driver runs from the worktree containing earlier queued PRs. The documented source installation and Nix package both execute a Bun-backed quest; Bun implicitly reads the working directory's bunfig.toml and executes its preload. Thus an earlier PR adding a preload, followed by a PR that invokes the quest merge driver, runs PR-controlled code with the push credential—even when the installed quest itself is trusted and outside the checkout. This happens during reconstruction, before result validation. The test wrapper's explicit trusted --config masks this path.
Invoke the resolver from a trusted configuration-free directory with absolute merge-file paths, or otherwise explicitly disable untrusted runtime configuration. Add a two-PR regression with a preload plus a subsequent quest-file conflict. Bun's preload fixtures cover the implicit-load behavior.
There was a problem hiding this comment.
Fixed in 639b590. The driver is now d=$PWD && cd <git-common-dir> && quest resolve "$d"/%A "$d"/%O "$d"/%B, so quest never starts inside the chain worktree, where a queued PR's bunfig.toml or .env would apply. Regression: a PR adding a bunfig.toml preload and a .env, followed by a quest-file conflict. The test's quest wrapper records each directory it runs in and asserts none is the chain worktree, which checks the property even though the wrapper's own --config masks Bun's lookup. It fails with the old driver.
(Written by Claude Opus 5.5)
…ck on errors - `push` reads the queue again before moving heads and lands only PRs still queued with the head and auto-merge it checked, ejecting with the fresh reason (such as `changes_requested`) and keeping the masking rule. - The merge driver runs `quest resolve` from the clone's git directory, so a queued PR's `bunfig.toml` preload or `.env` cannot configure the trusted `quest` that runs beside the push credential. - Any failure reading the moved heads, not only the deadline, goes through the rollback that moves them back. - The land tests skip native Windows, as the merge driver tests do: the fake `quest` on PATH is a shell script. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review round on Not changed here: the (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 639b5905436bc4fab28d8fa930be0b0159f7fd1f. Compared the single fix commit against my prior reviewed head d5aaa933d94f8e7f9bf9242541aa1a64b34d2fe5; base unchanged.
The reported Bun preload path, sync-error rollback, and stale eligibility are addressed by this delta. One new P2 remains below: a withdrawn entry can stop the rest of the queue without scheduling a fresh plan.
Direction is good: reuse the resolver and existing rollback path. An explicit replan signal is simpler than durable retry state. The acknowledged UPHELD.update limitation remains deferred, not fixed here.
Verification: GitHub-only static review of the complete delta, surrounding code, workflow, and regression tests; no tests or live landing run by me.
(written by review, OpenAI)
| const ejected = fresh.eject.find((ejected) => ejected.number === entry.number); | ||
| if (ejected !== undefined) ejections.push(ejected); | ||
| count = passed.has(index) ? index : 0; | ||
| break; |
There was a problem hiding this comment.
[P2] Signal a fresh run when a queued PR is withdrawn
If only the full A+B prefix passed and A's auto-merge is disabled (or A is closed/drafted) during checking, A appears in neither fresh.queue nor fresh.eject. This branch sets count = 0 without adding output, so line 659 returns [] while B remains queued. The documented workflow redispatches only for nonempty output and listens only for auto_merge_enabled/manual dispatch; with no other pending run, the withdrawal leaves B waiting indefinitely. The new test combines withdrawal with a different PR's changes_requested ejection, whose output masks this case.
Return an explicit replan-needed signal (and handle it in the workflow) when refreshed eligibility invalidates the tested batch, without requeueing the withdrawn PR. Add a withdrawal-only A+B regression that verifies B gets another run.
There was a problem hiding this comment.
Fixed in d816fea: push prints withdrawn <number> for a PR that left the queue while checked (and was not ejected), so the workflow redispatches for the rest. The withdrawn PR stays unqueued. Regression test: "reports a PR withdrawn while checked, so the rest gets another run", which fails without the fix.
(Written by Claude Opus 5.5)
A PR that left the queue during its check stopped the landing without output, and the workflow redispatches only on output, so the PRs behind it waited for an unrelated trigger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d816fea0dee8c3bf2a3207e12091aa815cb2c317. Compared against my last reviewed head 639b5905436bc4fab28d8fa930be0b0159f7fd1f: one focused fix commit; base unchanged.
The withdrawal-stalls-queue P2 is addressed. src/cli/land.ts:650–651 now emits withdrawn <number>, the CLI forwards it, and the documented workflow's nonempty-output condition schedules another run. The new regression checks that A remains withdrawn and only B appears in the next plan.
No new actionable findings in this delta. This small signal fits the existing workflow without persistent retry state. Earlier preload, eligibility, and rollback fixes are unchanged; the acknowledged UPHELD.update limitation remains deferred.
Verification: static GitHub review of the full delta, surrounding push/CLI code, workflow, and regression test; no tests or live landing run by me.
(written by review, OpenAI)
…osted check runner Verified by running the README workflow in Actions on a private moq-dev scratch repo, with the check job on an ephemeral self-hosted runner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Ran the README workflow end to end in Actions on moq-dev/quest-land-scratch, with
73541e8 records the App bypass, the (Written by Claude Opus 5.5) |
|
Merge summary (head 73541e8):
(Written by Claude Opus 5.5) |
Follow-ups of #229: land-rules refuses ruleset update rules and fixes the README's setup guidance; land-adopt uses a kixelated-owned App as its push bypass, since GitHub refused GitHub Actions on this repository, and verifies a fork PR live. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The quest land gate fails a check name if any run with that name failed (since #229), so it can wrongly refuse release-plz PRs that GitHub would merge. Maintainer decision 2026-11-02: land work, m0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implements
quest/m0/helpers/land.md:quest land, a batched merge gate that replaces GitHub's merge queue, and deletes the quest.What it does
quest land resolve [--base <ref>]reads the queue with a read-only token and prints a JSON plan: queued entries in order, plus ejections (with reasons) for PRs that cannot land.quest land check <plan> -- <cmd...>builds the squash chain in a private worktree (git rev-parse --git-path quest-land, quest-file merge driver, hooks off), runs the check on the tip, then bisects: one landing step per run, testing ceil(n/2) prefixes until one passes or a single PR fails. Prints a JSON result; command output goes to stderr.quest land push <plan> <result>rebuilds the chain independently, refuses any result whose trees or prefixes do not match, ejects failures, verifies every head withls-remote, pushes fork branches first (with lease, rolled back if the base push fails), then one atomic push moving same-repo PR branches to their squash commits and fast-forwarding the base.enabledAtis unchanged), removesquest:merge, comments with the run link and a<!-- quest land: <reason> -->marker.quest statusshows it as the landing-stopped reason.quest merge --landand kix landings on a gated base enable squash auto-merge with aQuest-Land-Head: <sha>commit body.quest waitignores thequest landcheck.checks_pending.Review fixes
pushre-runsresolvebefore moving any head, and lands only PRs still queued with the head and auto-merge it checked.quest resolvefrom the clone's git directory, so a queued PR'sbunfig.tomlor.envcannot configure the trustedquestbeside the push credential.test/land.test.tsskips native Windows, as the merge driver tests do.Public API / CLI impact
The README's land job now also needs
checks: readandstatuses: read, for its secondresolve.New
quest land {resolve,check,push}commands. On a base with classic protection,resolveneeds a token with administration read.quest merge --landsets a commit body on gated bases.requirements()moved fromwait.tstogithub.ts. No breaking change.Choices made without asking
Quest-Land-Head:equal to its current head; order isenabledAt, then number. A push after queueing unpins it (not_pinned). Chosen over a commit-status pin because the kix App holds onlystatuses: read, and the trailer needs no new permission.review_required,changes_requested,unresolved_conversations,failed_checks, orchecks_pending, since its push bypasses those rules. Conversation resolution pages each PR's review threads, only when the base requires it.quest statusandquest waitpage them (RESTallwould need two endpoints and a second classifier). A required check pinned to an app (integration_id, classicapp_id) counts only check runs from that app; a commit status never satisfies it, since GraphQL does not say which app posted it.resolvefails before reading the queue on any rule type outsidecreation,update,deletion,non_fast_forward,required_linear_history,pull_request,required_status_checks,copilot_code_review(for exampleworkflows,merge_queue,required_signatures,code_scanning), naming each rule and its ruleset and listing the upheld types. It also refusespull_requestsettings the gate does not cover:require_last_push_approval,allowed_merge_methodswithout squash, andrequired_reviewers;required_review_thread_resolutionis enforced.strict_required_status_checks_policycounts as upheld, since the gate checks the chain on the current base.GET /branches/{base}showsprotection.enabledwith read access, so a ruleset-only base keeps working withgithub.token; only a base with classic protection needs administration read forGET /branches/{base}/protection.required_status_checks(strict and app-pinned included),required_pull_request_reviews(approving count, code owners, dismiss stale),required_conversation_resolution,required_linear_history,block_creations, and the looseningallow_force_pushes,allow_deletions,allow_fork_syncing.resolverefuses any other setting that is on, including unknown future ones:required_signatures,enforce_admins(it stops the admin push that bypasses classic protection),lock_branch,restrictions(a writer could land through the gate around a push allowlist), andrequire_last_push_approval. A 403, or a 404 other than "Branch not protected", fails naming administration read.Title (#N), commit bullets,Co-authored-bytrailers; author is the PR's first commit author.Decisions
quest landsupports classic protection as well as rulesets, instead of refusing classic-protected bases.resolvereads it with administration read, enforces what it models, and refuses the rest.Live verification
Run 2026-10-26 on the private scratch repository moq-dev/quest-land-scratch, running the three phases locally with an admin token:
quest merge --landset squash auto-merge withcommitBody: Quest-Land-Head: <head>andquest:merge(#1).pushmoves every head first, waits until GitHub reports each PR'sheadRefOidat its squash commit (up to 60 s, then moves them back and lands nothing), and then pushes the base alone. A 4-PR batch (#8 to #11) all merged, and the next run ejected the stranded feat(skills): hand each quest result to the user with its decisions #7 ashead_moved.AutoMergeDisabledEvent, so a rolled-back PR stays queued.quest land(enforce_adminsoff),resolveread the protection and the admin push landed #12 and #13 as merged.Tests
just checkandjust testpass (533 CLI, 361 worker). Newtest/land.test.tscovers chain building, heads moving before the base and waiting for GitHub to show them (and moving back when it never does), quest-file driver, merge conflicts, bisection, moved heads, forks with and without maintainer edits, fork rollback, ejection order, non-gated refusal, forged results, checks paged past 100 (a pass and a failure on later pages, and a required check never reported), app-pinned checks, refused ruleset rules, classic protection's checks and conversation resolution enforced, unmodeled classic settings refused, and an unreadable classic protection refused.Follow-ups
Integration15368) as a ruleset bypass actor on an org repository ("must be part of the ruleset source or owner organization"). The README suggests that bypass, andland-adoptplans it for kixelated/quest; confirm it on a user-owned repository before adopting.required_status_checkscan pin only an app installed for the repository ("Invalid integration ids" otherwise). Say so in the README's "pin the check's source" guidance.quest landtests are skipped on native Windows; the gate runs on Linux CI.assets/skills/complete.mddoes not mentionchecks_pending.land-adopt.updaterule restricts who can push the base, yet anyone who can enable auto-merge queues through the gate, as with classicrestrictions(now refused).UPHELDstill acceptsupdate; consider refusing it unless its bypass list is the gate alone.mainruleset has amerge_queuerule, whichresolverefuses;land-adoptthere removes it.(written by Claude Opus 5.5)
🤖 Generated with Claude Code