Repository navigation
feat(scripts): refuse to enqueue a held or unpassed pull request (#602) - #687
Conversation
|
Verdict: PASS at d9ea214 Reviewed at this tip only. All 9 checks are Ran:
Not run:
What I checked
Non-blocking
Risk: T3 ( |
d9ea214 to
3acdecb
Compare
|
Verdict: PASS at 3acdecb The tip moved, so my PASS at d9ea214 is void and this is a fresh review of the whole diff (5 files, +305/−4): Ran:
Not run:
Non-blocking:
T3 ( |
The guard flattened comments and reviews into {login, body, at} and lost the
association, so against main's review-gate every conclusion was refused as
"author association missing is not OWNER, MEMBER or COLLABORATOR", the queue
entry failed tests-ok, and a PASS could never clear the guard. Map
author_association the way review-gate.mjs does, and let the fixture carry it
(MEMBER for the Bot, OWNER for the self-gate case) with a writers list and the
Written-by: header the gate reads.
Also add the guard, its test, and the three instructions that name it.
3acdecb to
e2cadce
Compare
|
Verdict: PASS at e2cadce The rebase onto Ran: Not run: nothing beyond the nine CI checks already green at this tip (including Non-blocking, for a doc line later rather than this tip: only the newest conclusion counts, so a human "record only — do not merge" note is superseded by a newer PASS at the same head (test 7 pins that deliberately). The label |
…dev (ThinkFlowLab#700) Written-by: Dev ## Reproduce At the base the documents still describe the merge path from before the owner decision of 2026-10-22 07:15Z, so a reader following them redoes the work `ThinkFlowLab#693` was built on (T2/T3 must carry a human `APPROVED` review) — that PR is closed unmerged because the rule it implemented no longer exists. ``` $ grep -c -- "a human approves" docs/autonomous-dev.md 1 $ grep -c -- "human approves" docs/autonomous-dev.md 2 $ grep -c -- "need no maintainer approve" AGENTS.md docs/autonomous-dev.md AGENTS.md:0 docs/autonomous-dev.md:0 $ grep -c -- "human-only" AGENTS.md 0 $ grep -c -- "awaiting-approval is retired" AGENTS.md 0 ``` `docs/autonomous-dev.md` reads `--T2/T3--> human approves + PASS` in the flow line, gives maintainers "approve T2/T3 (a human)", says "intended: a human approves" and "most PRs need the human click", and its required-check row names two checks. `AGENTS.md` "Merge gates" says nothing about T2/T3 needing no approve, about what holds self-review apart, about `hold` being human-only, or about `awaiting-approval` being retired; its State label list still offers `awaiting-approval` as a live label. ## What changed `AGENTS.md`: - step 6: the implementer-does-not-review-its-own-PR rule, with the shared `Birfy` account as the reason `selfReview` exists rather than a licence for self-review; the enqueue-guard sentence `ThinkFlowLab#687` added is kept as it is. - State labels: `awaiting-approval` leaves the live list. - Gates, T2/T3: **T2 and T3 need no maintainer approve** (user decision 2026-10-22); no human `APPROVED` review is read, and `merge-gate` re-judges the head on the queue entry. - Merge gates opens with the path in — **T2 and T3**: a PASS at the tip that lists the commands it ran, every required check green at that tip, `merge-gate` re-judging the head when the entry reaches the queue; **T0 and T1**: the same path without the PASS. Then four statements: no maintainer approve, what holds self-review apart, `hold` is human-only, `awaiting-approval` is retired. The four queue-entry rules stay, rule 1 now records `merge-gate` as a required check of the `main merge queue` ruleset (ThinkFlowLab#639, set 2026-10-22), and "a review's `state` is not read at all". `docs/autonomous-dev.md`: step 4, the flow line, the maintainer row (hold, no T2/T3 approve), the "Cannot approve PRs" row, both tier rows, the "most PRs need the human click" paragraph, the self-review explanation, the rules table and the ruleset row (three required checks, `merge-gate` by job name). **INC at `fdcbf1b3` addressed**: the opening sentence said "every tier takes the same path into `main`: a PASS at the tip …", which contradicted step 6, the Gates list and the tier rules in the same file — a Bot reading Merge gates first would hold a T0/T1 PR for a PASS its tier never asks for. It is now scoped to T2/T3, with T0/T1 named as the same path without the PASS. Nothing else in the section changed. ## What I ran At tip `c724dbb9`: ``` $ grep -c -- "need no maintainer approve" AGENTS.md 2 $ grep -c -- "self-review cannot be told apart mechanically" AGENTS.md 1 $ grep -c -- "the implementer Bot does not write the Verdict" AGENTS.md 1 $ grep -c -- "human-only" AGENTS.md 1 $ grep -c -- "awaiting-approval is retired" AGENTS.md 1 $ grep -c -- "awaiting-approval" AGENTS.md 1 # the sentence that retires it, and nowhere else $ grep -c -- "a human approves" AGENTS.md docs/autonomous-dev.md AGENTS.md:0 docs/autonomous-dev.md:0 $ grep -c -- "Every tier takes the same path" AGENTS.md 0 $ grep -c -- "enqueue-guard.mjs" AGENTS.md docs/autonomous-dev.md AGENTS.md:1 docs/autonomous-dev.md:1 $ node --test .github/scripts/risk-label-docs.test.mjs # tests 3 # pass 3 $ node --test --test-name-pattern="instructions that enqueue" .github/scripts/enqueue-guard.test.mjs ok 1 - ThinkFlowLab#602: the instructions that enqueue point at the guard ``` Link check: every relative link the touched lines add resolves at this tip — `#merge-gates`, `#standing-approval-mechanical-release-prs` and `#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized` all exist as headings in `AGENTS.md`. Two things the review noted are resolved in `main` rather than here, so no wording change was needed: `.github/scripts/enqueue-guard.mjs` exists now that `ThinkFlowLab#687` merged (so "the enqueue guard (ThinkFlowLab#687)" is a real script), and `.github/labels.json` no longer defines `awaiting-approval`, so "retired" is literally true. Branch rebuilt on current `main` (`7fa1b60a`); two files, one commit. `ThinkFlowLab#687`'s enqueue-guard sentence in both files is kept — the first rebuild of this PR dropped it from `docs/autonomous-dev.md`, which the `ThinkFlowLab#602` doc test caught, so the branch is now built on the post-`ThinkFlowLab#687` `main`. ## Red without the source change Docs, so no test runs red; the equivalent is the acceptance greps at the base, which is this branch with the two files reverted: all five anchors are `0` there, `a human approves` is `1` and `human approves` is `2` in `docs/autonomous-dev.md`, and the required-check row names two checks. Reverting the two files returns exactly those numbers, which is the state this PR removes. ## Not run - `npm run verify` and the test suite: no code, workflow, script or config is touched, and no test reads these documents. - Retiring the label in the repository was `ThinkFlowLab#702`'s change, not this one, and it has already landed in `main`; this PR only stops calling it live. ## Risk T3 — `AGENTS.md` and `docs/autonomous-dev.md` are both on the T3 list of `.github/agent/risk-rules.json`. Doc-only change: no code, no workflow, no review policy. Tip SHA: c724dbb Fixes ThinkFlowLab#695
…wLab#2073) Written-by: Dev Fixes ThinkFlowLab#602 Owner: Dev Reviewer: Reviewer Branch: dev/602-comment-hold Base: 5b862c6 Tip SHA: d7b246b ## What changed The guard ran on the labels, the diff and the newest conclusion, and never read the comments. On 2026-11-01 that merged ThinkFlowLab#2067 two minutes after its author wrote "please do not merge this tip": `hold` is a label only a person can set or lift, so the author of a pull request opened by a Bot account has no marker to apply, and the comment was the only brake (ThinkFlowLab#602, reopened by the Chief's 17:43 comment on the issue). `stopRequestAt` (`.github/scripts/enqueue-rules.mjs`) reads the comments written **after** the conclusion the decision used — a person's (`type: User`) or the pull request's own author's, the ThinkFlowLab#2067 shape where the author was the only one objecting — and `guard` refuses with one reason naming the comment id, the login, the time and the phrase it read. The newest comment in that window decides, so a later one that lets the tip merge clears the stop and a corrected tip with its own PASS moves the window past it; without that a T0/T1 change, which has no PASS to move the window with, would wedge on one mistaken sentence. Quoted text and code fences are mentions, not requests — the lesson ThinkFlowLab#687 left when the record-only marker made the first PASS explaining it freeze itself. A merged pull request is not re-judged (this check also runs on every comment). `merge-gate.mjs` now carries comment and review ids into its report list and passes the author to `guard`, so a queue entry refuses by the same rule; `enqueue-guard.mjs` passes the author. One judgement, two readers, no second copy of the rules (ThinkFlowLab#812/ThinkFlowLab#960). ## Red first Five cases added to `.github/scripts/enqueue-guard.test.mjs` and two to `.github/scripts/merge-gate.test.mjs`, run against main `5b862c6e` before any source change: ```text $ node --test .github/scripts/enqueue-guard.test.mjs not ok 89 - ThinkFlowLab#602 (2026-11-01): a comment after the PASS asking not to merge this tip refuses, and names the comment not ok 90 - ThinkFlowLab#602 (2026-11-01): the pull request author counts even from a bot account, and a bot that is not the author does not not ok 92 - ThinkFlowLab#602 (2026-11-01): a newer comment that lets it merge clears the stop; another comment does not not ok 94 - ThinkFlowLab#602 (2026-11-01): a T0/T1 pull request with no PASS is stopped by a comment too # tests 95 # pass 91 # fail 4 $ node --test .github/scripts/merge-gate.test.mjs not ok 18 - ThinkFlowLab#602 (2026-11-01): a comment asking not to merge refuses the queue entry, through main() # tests 48 # pass 47 # fail 1 ``` The same rule was replayed over the real API payload of ThinkFlowLab#2067 (its comments, its author `HugoXK`, read as of 17:38:30Z, after the 17:35:30 PASS): `{"id":6561490250,"login":"HugoXK","at":"2026-11-01T17:38:11Z","phrase":"please do not merge"}` — the merge of 17:39:54 would have been refused. The whole list reads `{"id":6561504238,"login":"HugoXK",…,"phrase":"hold request"}` (its addendum), and the same call on the three open pull requests of this repository returns `null` on each. ## Verification at the tip Tip SHA: d7b246b Ran: `node --test ".github/scripts/*.test.mjs"` — 565 tests, 563 pass, 0 fail; `node --test .github/scripts/enqueue-guard.test.mjs .github/scripts/merge-gate.test.mjs` — 143/143; `node core/scripts/build-client.mjs --check` exit 0; `node scripts/ds-bot-patches.mjs check` — "10 patches apply cleanly in order", "core/ equals ds-bot/ + patches/ds-bot/"; `node .github/scripts/changelog.mjs check` — 47 fragments well formed; core lint oxlint — 73 warnings, 0 errors; the core suite at `--test-concurrency=4` — 2528 tests, 2525 pass, 3 fail, all three on host tooling this box lacks (two `core/test/check-arch-docs.test.js` cases run `git`, and `core/test/usage-fields.test.js` runs `python3`); interop 37/37 and live-e2e 24/24. Not run (and why): `arch-docs` — this copy has no `.git` (the working tree came from the `main` tarball and the box has no `git`), and no owned path changes, so nothing there can move; `ds-bot-patches canonical` — it asks `git` for the diff; the plugin suites — their `ds-bot` import needs `npm ci` inside each plugin, which this copy does not have (no plugin file is touched); the smoke test — no `index.js` or `src/host/` change. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 2073, "claim": "Dev", "issue": 602, "summary": "The guard read the labels, the diff and the conclusions and never the comments, which is how ThinkFlowLab#2067 merged two minutes after its author wrote a stop: `hold` is human-only and a Bot-opened pull request cannot carry it. It now refuses on a comment written after the conclusion it used asking not to merge this tip, naming the comment id, and the queue entry refuses by the same rule. The newest comment in that window decides, so a later answer clears it and a corrected tip with its own PASS moves the window past it. Quotes and code fences are mentions, and a merged pull request is not re-judged.", "files": [ ".github/scripts/enqueue-rules.mjs", ".github/scripts/enqueue-guard.test.mjs", "AGENTS.md" ], "checks": { "ran": "node --test \".github/scripts/*.test.mjs\"", "result": "green", "at": "d7b246ba8a52ddaf8d1a64f5dec256a5c2c284ee" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
Written-by: Dev2
Fixes #602
Tip SHA: e2cadce
Reproduce
Nothing under
.github/scripts/enqueues anything today: the queue call is an instruction to the gating agent —AGENTS.mdstep 6,.claude/skills/fix-issue→ Merge,docs/autonomous-dev.md→ How to merge — and it isgh pr merge <n> --auto --squash --match-head-commit <sha>. Nothing on that path readshold: the only mechanical check ismerge-gate.yml, which judges a queue entry, i.e. after the pull request is already in the queue (#639 tracks making it a required check). #566 is the result: PASS at 09:48:18Z, enqueued 09:48:28Z, merged 09:50:40Z asabaf938bwhile the standing instruction was "parked — do not merge".So on main today, a
holdplus a PASS at the tip enqueues exactly like any other PASS, and a PASS whose body says "record only — do not merge" is read by every enqueue path as an ordinary PASS.What changed
.github/scripts/enqueue-guard.mjs—node .github/scripts/enqueue-guard.mjs --pr <n> [--repo owner/name] [--sha <head>]. It reads the live pull request (head, labels, changed files, comments and review bodies) and callsjudge({ …, mode: 'queue' })frommerge-gate.mjs, so "may this enter the queue" keeps one definition —hold, the newest conclusion at that head, the computed T0/T1 exemption and the mechanical-release approval are the gate's own rules, not a second copy. It adds one rule of its own,recordOnlyAt(): the "record only — do not merge / 仅记录、禁止入队" markerAGENTS.md→ Merge gates rule 2 pairs withhold, so the record keeps the pull request out even if the label is removed. Exit 0 prints the exact command and never runs it; exit 1 printsdo not enqueue #<n>: <reasons>, naming the hold.AGENTS.mdstep 6,.claude/skills/fix-issue/SKILL.md→ Merge,docs/autonomous-dev.md→ How to merge (which also stops calling--match-head-commitoptional).RECORD_ONLYquotes "Record only — do not merge" mid-sentence, and a phrase-anywhere match read that as a parked record. A marker line — optionally a bullet or a quote, optionally followed by the other language half — is what freezes a pull request now, so a conclusion that explains the rule cannot freeze itself.author_associationintojudge()(this rebase). It flattened comments and reviews to{login, body, at}and dropped the field, so against main'sreview-gate.mjs— which reads that field since Refuse a report that does not say who wrote it (#627) #676 — every conclusion was refused as "author association missing is not OWNER, MEMBER or COLLABORATOR". Even a perfect PASS could not clear the guard, which is why both of this branch's earlier queue entries failedtests-ok; the fixture had the same gap and also had nowriterslist orWritten-by:header. The mapping now matchesreview-gate.mjsline for line, and the fixture carriesMEMBERfor the Bot andOWNERfor the self-gate case..github/scripts/enqueue-guard.test.mjs— 15 tests throughmain()with a fakegh, the waymerge-gate.mjsis tested: held + PASS refuses and names the hold; a PASS at the tip allows and prints the exact command while touching only read endpoints; no conclusion refuses; a later INC beats an older PASS; a PASS naming an older tip refuses; a record-only PASS refuses; the newest conclusion decides in both orders; a marker on another tip, or prose that quotes it, is not a freeze; a T1 pull request needs no PASS but a hold still refuses it; the self-gate case (authorBirfy, PASS byBirfy) passes; and the three instruction files must keep naming the guard.What I ran
At this tip
e2cadcee9de0498e86af1c0e55a26494a3326a58, rebased onto mainbafe9bad(between2f538019andbafe9badmain moved only inplugins/tree/**anddocs/subsystems/tree.mdfrom #703 — no file of this diff overlaps):node --test .github/scripts/enqueue-guard.test.mjs→ 15/15 pass.node --test ".github/scripts/*.test.mjs"→ 188 tests, 184 pass, 4 fail. The 4 failures areds-bot-patches.test.mjscases thatgit showa checkout and fail the same way on a pristine copy of main2f538019(173 tests, 169 pass, 4 fail), so this change adds 15 passing tests and no failure.merge-gate.test.mjsandreview-gate.test.mjspass.ARCH_DOCS_BASE=4eb81e5 node core/scripts/check-arch-docs.mjs→arch-docs: OK (5 changed file(s)).Red without the source change
On main the guard does not exist, so the new suite cannot even import: 15/15 fail. Three targeted mutations on the pushed tree, each naming what the tests actually hold:
association: c.author_association/v.author_associationfrom the two report mappings — the defect that failed this branch's queue entries → 7 of 15 fail, exactly the cases that need a conclusion to count (a PASS at the tip, a newer INC, both "newest decides" orders, prose that quotes the marker, the explaining PASS, and the self-gate case).judgecall — the Merge queue ignoreshold/ manual freeze: a PASS at the tip is enqueued ~10 s later #602 bug (nothing checksholdbefore the queue) → 2 of 15 fail, exactlya held pull request is not enqueued, whatever a PASS at its tip saysandhold refuses a T0/T1 pull request too.guard()→ 2 of 15 fail, exactly the two parked-report cases.Not run
ghon this box: the CLI wrapper aroundmain()is not executed, onlymain()itself (the tests drive it with a fakegh).merge-gatea required check on the merge queue is the maintainer's action (Merge gateis not a required check on the merge queue, so a red gate does not block a merge #639) and is deliberately not part of this PR — the guard works without it.Merge gateon this tip is the first proof it runs inside the queue.Risk
T3 (the diff touches
.github/**andAGENTS.md, which are T3 paths in.github/agent/risk-rules.json). Automation surface, but narrow: the guard cannot merge or enqueue anything — it only reads the API and prints a command — so the worst failure mode is a wrong print. A false refusal blocks one enqueue visibly and loses no work; the failure this rebase fixes was exactly that (a conclusion that could never count), and it was visible as redtests-okrather than as a wrong merge. A false allow can only happen for something the merge gate itself would also allow, or if an agent skips the guard, which is why the three instruction files name it and one test asserts they do.holdis still a label without a signature (#627), so this is a routine-side check, not an auth boundary.