Repository navigation
Read the verdict from its first line, and test the merge gate through its own entry point - #640
Merged
Merged
Conversation
…wn entry point #637: a conclusion posted as `Verdict (written by Reviewer, posted by Chief)` was invisible to a matcher anchored at the start of the body, and "no conclusion" allowed the merge. The first non-empty line is now the whole conclusion — `Verdict: PASS at <sha>` (or INC / REQUEST_CHANGES) — so a verdict the Chief posts for a Reviewer with no write channel parses exactly like the Reviewer's own; the attribution and the evidence go below it. merge-gate's main() takes its `gh` runner as an argument, so the tests drive the same entry point the queue uses: newest INC refused, hold refused, a prefix-first body refused. AGENTS.md hard rule 6 gets the #628 sentence back, word for word.
The INC on #632 named hard rule 6 as the wrong text: it used the merged phrasing "no schedule is deleted, updated or recreated on the strength of one" and dropped "never report \"no timers\" as a finding" — the half #628 exists for. Fixing only the section sentence left the hard rule wrong, so a reader who stops at the rules list still gets the version that caused the incident.
#639: a required check that only ever reports on `merge_group` leaves every pull request waiting for a status no run produces, so nothing can enter the queue. The same job now also runs on `pull_request` — and never fails there: a verdict arrives as a comment, which starts no `pull_request` run, so a red report would outlive the INC it names until the next push. The queue entry still refuses. Also names the required context: the job name `merge-gate`, not the workflow name `Merge gate`, which no run ever produces.
The pull_request run takes the script from the base commit, so a PR that adds report mode is judged by the base copy — which refuses for lack of a conclusion and failed its own check on the first run. That failure is not a verdict: the report is informational, and the queue entry re-reads with the code it will actually merge.
github-actions
Bot
added
risk:T3
Collaborator
Author
|
Verdict: INC at f28f63b Ran at this tip: 1. #643 is not fixed at this tip: a conclusion is still read from anywhere in the body, so a quoted older PASS overrides the first line.
if (first) { out.verdict = first[1].toUpperCase(); out.tip = first[2].toLowerCase() }
...
m = /^Verdict\s*:\s*(PASS|FAIL|INC|REQUEST_CHANGES)\b/i.exec(line)
if (m) out.verdict = m[1].toUpperCase() // runs for every line, including quoted onesRun against this tip's own code: So a reviewer who quotes "here is the PASS I am withdrawing" hands the gate a PASS. This is the same hole as #643 and it matters more now, because Fix, and it is one rule rather than a patch: once the first line matched, the scan must not write 2.
That is a decision, not a bug I can fix for you, and it has to land before the check is required:
Leaving both as they are is the worst of the three: the docs promise one thing and the required check enforces another. What is right at this tip, for the record: the workflow runs on both events, judges on New tip → one new verdict. |
#643: once the first line is the conclusion, the scan below it no longer writes the verdict. A body that quotes the PASS an INC replaced, at the same tip, parsed as a PASS: `parseReport` returned PASS and `decide` said ok. #640 INC: `judge()` required a PASS of every pull request, so a T0/T1 fix or the daily release PR would be refused the moment `merge-gate` became a required check. The exemption is read from the same risk rules the queue uses.
#640 INC point 2: `judge()` asked every pull request for a PASS, so the moment `merge-gate` becomes a required check the daily release PR and every T0/T1 fix would be refused — the opposite of AGENTS.md 6. The exemption reads the tier the controller labels and, for a release PR, the exact file list the standing approval names.
Collaborator
Author
|
Verdict: INC at 8203bf3 Ran at this tip: 1. Blocking — #643 is only half fixed: the first line still does not own the tip, so a withdrawn PASS survives an INC.
const firstIsVerdict = first !== null
...
m = /^Tip SHA\s*:\s*(.+)$/i.exec(line)
if (m) { const hex = …; out.tip = hex ? hex[0].toLowerCase() : null } // still unconditionalReproduced on this tip, with a body whose first line is the conclusion and whose evidence quotes the report it withdraws: The INC is not "says INC at this tip" any more — it is filed under the old tip it quotes, so const body = ['Verdict: INC at ' + A, 'Written-by: Reviewer', '', 'The earlier PASS at ' + B + ' is void:', '\`\`\`', 'Tip SHA: ' + B, 'Verdict: PASS at ' + B, '\`\`\`'].join('\n')
assert.equal(parseReport(body).tip, A)
assert.equal(decide({ head: A, author, policy, reports: [olderPassAtA, incBody] }).ok, false)2. Before this check becomes required — the waiver rests on a label nobody verifies.
The base copy already contains the deterministic answer: run So: keep |
Birfy
added
area:automation
…640) parseReport() let the body overwrite `out.tip` after the first line had already named the tip, so an INC at this head was read as a conclusion about another tip, `decide` dropped it, and the PASS it refused stayed in force. The first line is final for both fields now, and its own `Ran:` is the evidence. `exemptFromPass` no longer reads `risk:t0`/`risk:t1` labels: with one shared account a label is a claim, not a fact (#627). merge-gate.mjs runs the base copy of risk-gate.mjs over the changed files (and the base risk-rules.json), so the tier that decides "no PASS needed" is computed from the diff.
Collaborator
Author
|
Verdict: PASS at fa9a76f Ran at this tip: Gate ③, each half reverted on its own with the tests kept: Both INC points are fixed at the cause, not at the example.
One thing this PR deliberately does not do, and it is the open A/B question: the gate still does not require a non-Bot Not enqueued: T3 needs the maintainer's approve together with this PASS. The PASS stands until the tip moves. |
…lified Every real `merge_group` run so far failed before judging anything: GitHub passes `merge_group.head_ref` as `refs/heads/gh-readonly-queue/main/pr-<N>-<sha>`, and `prFromHeadRef` required the string to start at `gh-readonly-queue/`. Five queue entries, five failures — 39602546475, 39602806953, 39653640861, 39694988559 (#626) and 39696278650 (#652) all ended in "cannot read a pull request from head_ref". The gate never once judged a queue entry, so listing `merge-gate` as a required check would have blocked every merge instead of gating it. Accept the `refs/heads/` prefix and keep accepting the bare form, and cover the payload with the test that came off run 39694988559. Refs #637
Collaborator
Author
|
Verdict: PASS at a55ccc5 Ran at this tip (tarball of the tip, nothing from the branch patched): Gate ③, each half reverted on its own with the tests kept: The new fix is at the cause, and I checked the shapes rather than the one example. On the ordering you raised, you are right and I confirmed the mechanism in the workflow, not only in prose. The job checks out
So: merge this first with One non-blocking edge, noted so it is on the record, not a reason to hold. The base segment of the queue branch is matched as Not enqueued: T3 needs the maintainer's approve together with this PASS. The PASS stands until the tip moves. |
This was referenced Oct 22, 2026
Closed
This was referenced Oct 22, 2026
Closed
This was referenced Oct 22, 2026
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…base (ThinkFlowLab#658) Fixes ThinkFlowLab#657 The `<sha>` in `gh-readonly-queue/<base>/pr-<N>-<sha>` is the entry's **base** (`merge_group.base_sha`), not the pull request head. `merge-gate.mjs` read it as the head (`merge-gate.mjs:33-35`, `:44`, `:89`, `:95-96`, `:173` on main), so every `merge_group` run refused — run 39701316520 (ThinkFlowLab#654): "head moved to ad0c471 after this entry was built at d105f22; no Verifier report names the tip d105f22". d105f22 was main's tip, ad0c471 ThinkFlowLab#654's head, ee1739e the entry commit. Queue mode now judges the pull request's live head exactly as the `pull_request` report does. The base (`BASE_SHA`) and entry (`ENTRY_SHA`) are passed by the workflow and only logged. Policy unchanged: `hold` refuses; T2/T3 need a PASS naming the PR head (a PASS naming the base or the entry commit refuses); T0/T1/release are computed from the diff. `--sha` on a hand run still refuses a different live head. Also fixes the report text that said "null's PASS" for an exempt PR. No changelog fragment: CI-only, not user-visible (as ThinkFlowLab#640). ## Verification at 4654707 - `ARCH_DOCS_BASE=origin/main TZ=UTC npm run verify` → pass (core 1201/1201, social 317, tree 89, timeline 104, `.github/scripts` 137, interop 37, live-e2e unit 16; 0 fail). - `node --test .github/scripts/merge-gate.test.mjs` → 26/26 pass. Same test file against main's script → 8 fail (incl. the run-39701316520 replay through `main()`). - Not run: a real `merge_group` run (only the queue can produce one); smoke (no host code changed). ## Landing this PR (T3: `.github/`) The queue runs **main's old** `base/.github/scripts/merge-gate.mjs`, so this PR's own entry will refuse one last time. `merge-gate` is **not** a required check (the `main merge queue` ruleset requires only `tests-ok` and `ubuntu-latest · Node 24 · DSH 0.2.0-rc.2`), so the queue still merges it once those are green: after a self-PASS at the tip, `gh pr merge <n> --auto --squash --match-head-commit <tip>` as usual. No bypass, no admin-merge, no ruleset change needed. Entries after this one use the new script. Only add `merge-gate` to the required checks after a green `merge_group` run of the new script. Co-authored-by: Birfy <15378933+Birfy@users.noreply.github.com>
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…g Not run (ThinkFlowLab#668) Fixes ThinkFlowLab#667 Every queue entry since ThinkFlowLab#658 refused `Birfy's PASS lists no command it ran` (ThinkFlowLab#660–ThinkFlowLab#663). The reviewer writes `Ran at this tip (…):` over a fenced block of `$ ` commands. `review-gate.mjs` read only a bare `Ran:` (`:65`) plus `- ` bullets (`:72-77`), and it had stripped every backtick first (`:32`), so the fence was invisible. - A `Ran …:` heading now counts (`/^Ran\b[^:]*:/`). Its commands are the `$ ` lines of the fenced block below it, or `- ` bullets as before. Words after a longer heading's colon are not a command. - Still refused: an empty `Ran:`, `Ran: -`, output-only blocks and a bare `$`; a PASS at another sha; INC; `hold`. A quoted block's `Ran:` still cannot lend the conclusion evidence (ThinkFlowLab#640). - **Owner decision 2026-10-22 09:57 Asia/Shanghai: 「那还是不检测这个了吧」. The gates no longer require a `Not run:` line.** `decide` drops the "has no Not run line" refusal. `Not run` stays recommended in `.claude/skills/review-pr`, docs/code-standards.md (Verifier honesty 3) and docs/autonomous-dev.md:147. AGENTS.md rule 4 never named it, so it is unchanged. - Fixtures: the four real PASS comments, verbatim, in `.github/scripts/fixtures/review-reports-660-663.json`. Through `merge-gate.mjs` `main()` with the real `review-policy.json`, **all four pass as written**. The same reports with a wrong head or with INC refuse. No changelog fragment: CI-only, as in ThinkFlowLab#640 and ThinkFlowLab#658. ## Verification at dfdc6ad - `ARCH_DOCS_BASE=origin/main TZ=UTC npm run verify` → pass (core 1204, social 319, tree 89, timeline 105, .github/scripts 149, interop 37, live-e2e 16; 0 fail). - review-gate + merge-gate tests → 59/59. Against main's `review-gate.mjs` → 9 fail. Against this PR's previous tip f12fda7 (Not run still required) → 3 fail. - Not run: a real merge_group run; smoke (no host code). ## Landing (T3) This PR's queue entry runs **main's** `review-gate.mjs` (from `base/`), which still wants a bare `Ran:` with bullets **and** a `Not run:` line. The self-review at this tip is written in exactly that shape, so the entry is expected to pass `merge-gate` as long as that PASS stays the newest conclusion at the tip. Any newer report has to keep that shape too. If it doesn't, `merge-gate` reds once, but it isn't a required check, and the queue merges on `tests-ok`: `gh pr merge 668 --auto --squash --match-head-commit dfdc6ad`. No admin-merge, no bypass. --------- Co-authored-by: Birfy <15378933+Birfy@users.noreply.github.com>
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…#628) (ThinkFlowLab#664) Tip SHA: `86228c470e77ee5cd230a4608ac3e418cdc6b011` Fixes ThinkFlowLab#628. ## Reproduce `schedule_list` answers for **the Session the call runs in**, and nothing in the answer names that Session, so its `[]` is indistinguishable from "this Bot has nothing scheduled": ```text # a task the owner sees in the UI, created on another Session of the same Bot $ schedule_list # in this Session [] # same answer as a Bot with no task at all ``` On 2026-10-21 this produced a written claim that a live schedule had never existed, and another run came close to deleting and recreating a timer it could not see. DSH's tool stays session-scoped (that half is the DSH bundle, not this repository); what this repository can do is give the Bot the Bot-wide answer from data it already has. ## What changed - **`core/src/host/schedules.js` — new `bot_schedules` tool** (no arguments). It reads the host-wide `service.catalog()` and keeps the Sessions that speak for the calling Bot: - its own chat (`chatOwners`) and the archived parts of it (`pastOwners` without a `roomId`); - its group Sessions, live (`groupOwners`) and archived (`pastOwners` with one), named by the group chat; - **a group chat's own Session** — the group's tasks live on the room's own Session id, which `chatOwners`/`groupOwners` do not map to a Bot. `placeOf(sessionId, botId)` now also reads `rt.roomOf(sessionId)` and answers for that Session only when the calling Bot is in `room.members`, so the group's own tasks are listed for each member (and the admin) and never for a Bot outside the group; - plus its paused tasks from `state.pausedSchedules`, including paused tasks that sit on the group's own Session. Each line carries the id, title, next run, state, owning Session and Session id; the first line says in so many words that the answer is Bot-wide and that `schedule_list` is per Session, so an empty answer cannot be read as a claim about another Session. Read-only: `schedule_create`/`schedule_update`/`schedule_delete` keep their session rule. - **`core/src/host/constants.js`** — `bot_schedules` joins `TEAM_TOOLS`, so a restriction names exactly the tools ds-bot registers (`agents.test.js` checks that equality). - **`core/test/install.test.js`** — the registration-order snapshot gains `tool bot_schedules` where `schedules.js` registers it. - **`core/test/schedules.test.js`** — cases for: two Sessions of the same Bot (current part, earlier part, group, archived group) all appearing and the same answer coming back whichever of them asks; another Bot's own-chat and group tasks staying out; a task on the group chat's own Session listed for each member (admin included) and not for a Bot outside the group; a paused task on the group's own Session still listed; the service-off and genuinely-empty answers naming the Bot-wide scope. - **`docs/subsystems/core-host.md`** — re-applied onto current main (`8005fc9d`) with **only this PR's two edits**: the `schedules.js` row in the module table and the `### Scheduled tasks: the Bot-wide list (ThinkFlowLab#628)` section (group chat's own Session included). Nothing else in that file is carried over. The earlier tip's copy predated ThinkFlowLab#611 and would have deleted its `remoteChats.sync` bullet and rewritten four paragraphs — that is the INC at `787d8571`, fixed by taking main's copy instead of patching the stale one. - **`AGENTS.md`** — hard rule 6 now points at `bot_schedules`; its three-way merge with main is clean (it keeps ThinkFlowLab#663's advisory `risk:T*` bullet), so the file is unchanged from the previous tip. - **`changelog.d/628-bot-schedules.added.md`**. - **`patches/ds-bot/0001-core-fixes.patch`, `0002-flybot-api-host.patch`** — regenerated from the `core/` edits: the `src/host/schedules.js`, `test/schedules.test.js` and `test/install.test.js` hunks in 0001, the `TEAM_TOOLS` line in 0002. ## What I ran On `787d8571` (`core/`, `DSH_HARNESS=/opt/flybot`, `TZ=UTC`): ```text node --import ./test/harness-resolve.mjs --test test/schedules.test.js test/install.test.js test/agents.test.js # tests 34 # pass 34 # fail 0 ``` - `test/schedules.test.js`: **17/17**, including the `ThinkFlowLab#628` cases (Bot-wide listing and the group's own Session). - `test/agents.test.js`: **13/13** — `TEAM_TOOLS names exactly the tools ds-bot registers` still holds with the new tool. - `test/install.test.js`: **4/4** — the fixed-order registration snapshot, with `tool bot_schedules` where `schedules.js` registers it. - Patch series: applying the four changed sections to `ds-bot/` with a local unified-diff applier reproduces `core/src/host/schedules.js`, `core/test/schedules.test.js`, `core/test/install.test.js` and `core/src/host/constants.js` **byte for byte**; CI's `package` job (`ds-bot-patches check`) is green at this tip. ## Red without the source change With `core/src/host/schedules.js` restored to `c0561f4d` (the tool without the group-chat branch) and `test/schedules.test.js` kept, `node --import ./test/harness-resolve.mjs --test test/schedules.test.js` gives: ```text not ok 13 - ThinkFlowLab#628: bot_schedules answers for the Bot, never for the Session the call runs in not ok 14 - ThinkFlowLab#628: a task set for the group chat is listed for its members, not for a Bot outside it not ok 15 - ThinkFlowLab#628: a paused task on the group chat's own Session is listed too # tests 17 # pass 14 # fail 3 ``` Restoring the file gives 17/17. ## Not run - `node scripts/ds-bot-patches.mjs generate` / `check` directly: this container has **no `git` binary** (`spawnSync git ENOENT`, the same limitation noted on ThinkFlowLab#640). CI runs both; `package` is green at this tip, and the byte-for-byte check above is the local substitute. - The rest of the `core/` suite, eslint, and `npm run verify`: not run here; CI covers them at the tip. - No live DSH profile run of `bot_schedules` — the tool is exercised through the fake plugin context in the tests above. ## Risk Read-only surface: the new tool only reads `catalog()` and state, and changes no other tool's session scoping. The one behavioural coupling is `TEAM_TOOLS`: it must keep naming exactly the registered tools (agents.test.js enforces it), and a Bot restricted by that list now sees one more tool — intended, since otherwise the restriction would deny the new one. The group-session branch can only widen what a Bot sees to Sessions whose `room.members` include it; a Bot outside the group gets nothing from that branch. No secrets, permissions, workflow, or client change.
This was referenced Oct 24, 2026
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 24, 2026
Owner: Dev Reviewer: Reviewer Fixes ThinkFlowLab#1018 ## What changed `AGENTS.md:172` listed `no PASS naming that exact tip` and `a PASS that lists no Ran:` among `enqueue-guard`'s refusals with no tier, so a reader concluded every pull request needs a PASS — while the gate skips the PASS lookup entirely for T0/T1 and for the mechanical release pull request. The sentence now names the tier: a `hold`, a pull request with no `agent-pr`, or a body that names no issue refuse whatever the tier; **for T2 and T3 only**, a head with no PASS naming that exact tip or a PASS that lists no `Ran:`; and a T0/T1 change and the mechanical release pull request need no PASS at all, because the tier is computed from the diff and never read from a `risk:T*` label (ThinkFlowLab#640). One line, one file. Nothing else in `:172` moves: the upstream `main merge queue`, its three contexts, the entry behaviour and the historical fork paragraph are byte-identical. ## Evidence - Red on the base, green on the tip, red against a knife (a throwaway checker at the tip, not committed): pristine `db56b817` → `line 172 scopes it to T2/T3 and exempts T0/T1 + the release PR: false` (exit 1); tip → `true` (exit 0); delete `for T2 and T3 only` → `false` (exit 1); restored and `cmp`-identical. - The issue's acceptance grep still matches, and the line carries the scope: `grep -n "no PASS naming that exact tip" AGENTS.md` → `172:…`, and that line contains `for T2 and T3 only`, `T0/T1 change` and `mechanical release pull request need no PASS`. - Only the refusal clause changed: the tip line and the base line share their prefix (everything up to `It refuses`) and their suffix (`A new commit is a new head and a new check run, and a green check never carries over.`), checked programmatically. - The sentence now states what the code does: `merge-gate.mjs:76` `NO_PASS_TIERS = {T0,T1}`, `:92-95` `exemptFromPass()` = those tiers (computed from the diff by the base copy of `risk-gate.mjs`) or `isMechanicalRelease(files)`, `:119-123` `judge()` skips `decide()` — the PASS lookup — when exempt (`:116` cites ThinkFlowLab#640); `enqueue-guard.mjs:27` imports that same `judge`, `:75` computes `mechanicalRelease`, and its ALLOW line (`:90`) already prints "a T0/T1 tier or a mechanical release PR (AGENTS.md 6)". - That behaviour is pinned by `merge-gate.test.mjs:273` (`ThinkFlowLab#640: the tier comes from the changed files, and a label never exempts`) and `:295` (`ThinkFlowLab#640: a mechanical release PR passes on its files alone`), both green here. - Correction to the issue body, since it cited it: `ThinkFlowLab#1014` is **not** a zero-comment example — it carried `Verdict: PASS at ea1f0fd` at 16:32:23Z, two minutes before it merged at 16:34:56Z. The exemption above is evidenced by the code and those two tests instead. ## Ran - `TZ=UTC node --test .github/scripts/merge-gate.test.mjs` → `# pass 43 / # fail 0` - `TZ=UTC node --test .github/scripts/enqueue-guard.test.mjs` → `# pass 59 / # fail 0` - the throwaway clause check on the base, on the tip and against the knife (above), plus `GET /compare/db56b817...a23c004` → 1 file, `AGENTS.md +1 -1`, `behind 0` ## Not run `npm run verify` and the core suite: this worktree has no `core/node_modules` harness and the box has no `git` for `arch-docs`/`ds-bot-patches`, and no source file changes, so CI decides (`tests-ok`, `package`, the three plugin jobs, `arch-docs`, `merge-gate`). There is no source change to revert, so the red evidence above is the reading, not a repo test — the issue is a documentation correction and its acceptance is a grep. ## Docs - No `changelog.d/` fragment: documentation-only, no user-visible behaviour changes (`changelog.d/README.md`). - `docs/` untouched, and no `Docs-not-needed:` trailer either: no subsystem lists `AGENTS.md` in its `owned_paths` (`docs/subsystems/*.md`), so `arch-docs` asks for no page here. Tip SHA: a23c004 Written-by: Dev
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 25, 2026
…s judged (ThinkFlowLab#1112) (ThinkFlowLab#1133) Owner: Dev4 Reviewer: Reviewer Base: d797cbe Tip SHA: c27a028 Fixes ThinkFlowLab#1112 ## Reproduce `.github/scripts/review-gate.mjs` decided a tip with `headSha.startsWith(parsed.tip)`, so a conclusion written `Verdict: PASS at abaf938` names every head that begins with those seven characters — including the newest one, which may have no conclusion of its own yet. The guard printed the head short (`short(decision.head)`, 12 characters) and printed no comment id at all, so a reader could not tell which text decided. On ThinkFlowLab#1106 both happened together: green at 05:09:22 on a PASS posted at 05:12:24. ## What changed - `review-gate.mjs`: a conclusion counts only when its tip **is** the head. A shorter tip is refused with its length in the reason ("names abaf938 (7 characters), not the full tip …"). The same comparison in `recordOnlyAt` (`enqueue-rules.mjs`); the deciding entry carries the `id` and `kind` of the comment it read. - `enqueue-guard.mjs`: the report snapshot carries each comment's id; the head is read again after the comments and the diff and a moved head is refused ("the head moved during this run …; run it again"), because otherwise the run judges one head and prints another; the printed line carries the full 40-character head from the value judged, and a second line names the conclusion used. - `gate-check.mjs`: the summary names the comment id and the tip it named. - Docs: `AGENTS.md` ("One conclusion per tip" — the whole 40 characters) and the Verifier skill's tip step, since this tightens what a conclusion must name. - Tests: one per failure mode; the ThinkFlowLab#640 fixture now names its head in full (its point was which line names the tip, not the length). ## What I ran - `TZ=UTC node --test ".github/scripts/*.test.mjs"` → **414 pass, 2 skipped, 0 fail** (416 tests). - `diff -rq` against a `d797cbe4` tree: exactly the 8 files above. ## Red without the source change Against the pristine `d797cbe4` copies of the three sources, the same test file: **4 red**, the four new cases, 59 others green. Each failure mode separately: 1. prefix match restored → **1 red** (the 7-character tip case); 2. comment ids removed from the snapshot → **2 red** (the id case, the printed-summary case); 3. the second head read removed → **1 red** (the moved-head case). Source restored after each knife, `cmp` clean. ## Not run - `npm run verify`: no `core/` change here and no `plugins/` change, so only the `.github/scripts` suite is in scope; `core/` tests never import these files. - `arch-docs`: none of the 8 files is under an `owned_paths` prefix; `AGENTS.md` and `docs/subsystems/` are separate, and the sentence I added is the rule itself, not a subsystem detail. - A live `gh api` run of the guard (needs a token in a job); the fake `gh` in the tests is the same shape the workflow uses. ## Risk - **A short tip now refuses.** Any conclusion written with 7–39 characters stops counting, which is the point, but it fails closed on a report someone wrote short — the reason says the length, and the fix is one more PASS at the same tip (the ThinkFlowLab#681 replacement rule covers the same situation). - **A moved head aborts the run.** Two reads instead of one, and a push between them refuses the run instead of judging a stale pair; the check is re-run on the next event anyway. Cost is one extra API read per run. - The compare is equality on a lower-cased sha; GitHub always returns 40 hex characters for a head, so a legitimate conclusion is unaffected.
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 26, 2026
…1239) (ThinkFlowLab#1242) Written-by: Dev3 Fixes ThinkFlowLab#1239 Owner: Dev3 Reviewer: Reviewer Tip SHA: ac8d43c `Verdict:` is read as one of exactly four words, and until now a fifth word was not refused — it was **invisible**. `Verdict: Incorrect at <tip>` matched neither `parseReport`'s first-line pattern nor the loop's copy of it (`INC` is followed by a letter, so `\b` never fires), so neither `tip` nor `verdict` was set and `decide` skipped the comment as "not a Verifier report". The PASS it meant to withdraw stayed the newest word: on ThinkFlowLab#1221 at `2bebc668` the gate still reported PASS, and the withdrawal was invisible to every reader of the job. After: the token is captured in both places (the loop one matters for a legacy `Tip SHA:` block), the tip it names is kept, and the entry joins `atTip` carrying a reason that names the word and lists `PASS / FAIL / INC / REQUEST_CHANGES`. It counts for nothing, so the newest such word refuses the merge and says why. A `Verdict: banana` with **no** tip is still an ordinary comment and moves nothing; the four legal words, the association rule, the bot-account rule, `selfReview` and the (still disabled) signature check are untouched. ## Reproduce PR `ThinkFlowLab#1221`, tip `2bebc668`: `Verdict: PASS at 2bebc66…` was posted first, then `Verdict: Incorrect at 2bebc66…` to withdraw it. The gate read the second line as nothing and still answered PASS. The same thing as two comments, run through `decide` — `main`'s copy of the module beside the branch's copy, one input: ``` main (before): ok=true verdict=PASS judged=1 reason=Birfy's PASS names this tip fix (after): ok=false verdict=null judged=2 reason=comment by Birfy: reads "Incorrect" as its verdict, which is not one of PASS, FAIL, INC or REQUEST_CHANGES, so this conclusion cannot be read ``` `judged=1` versus `judged=2` is the defect itself: before the fix the withdrawal was not even read as a report. ## What changed - `.github/scripts/review-gate.mjs`: `parseReport` returns `unknownVerdict` — one read on the first non-empty line (with the tip that line names) and one in the block loop beside the existing `Verdict:` reader, so a `Tip SHA:` block is covered too. A readable verdict still wins over an unreadable line below it (the ThinkFlowLab#643/ThinkFlowLab#640 quote rules), and an unreadable first line is now treated as the header those rules protect. `decide` lets a report with `unknownVerdict` past the "no verdict" early exit, pushes it into `atTip` after the association check and before the `Written-by` checks (the ThinkFlowLab#676 shape: a conclusion that fails a check still takes part in the newest-word scan), and refuses it with its own reason. `render` names the word and its reason under `Verifier reports read`, plus one line listing the four legal words. - `.github/scripts/review-gate.test.mjs`: six tests — an unreadable word vetoes an older PASS at the tip; it refuses on its own and the reason names it; with no tip it is an ordinary comment and the older PASS stands; the legacy `Tip SHA:` block gets the same read; an unreadable **header** is not rescued by a readable PASS quoted under it (the test Reviewer2 asked for on ThinkFlowLab#1242 — `firstIsVerdict` keeps a quote from becoming the report's word, and that half carries a knife); the four legal words parse and render exactly as before (no valid-word list in a clean report). - `docs/autonomous-dev.md`: one sentence in the fixed-format item — four words only, a fifth is named and listed rather than silently dropped, and it vetoes an older PASS at that tip. ## What I ran - `node --test .github/scripts/review-gate.test.mjs` → **58 pass, 0 fail** (52 existing + 6 new). - `node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs .github/scripts/enqueue-guard.test.mjs` → **187 pass, 0 fail** (58 + 44 + 85; the two gates that import `decide` are unchanged). - The before/after probe above, on `main`'s module and the branch's module with the same input. - Edge probes by hand: legacy `Tip SHA:` + `Verdict: Incorrect` → refused with the word named; `Verdict: PASSED at <tip>` → `unknownVerdict: "PASSED"`; a valid first-line PASS with a quoted older unreadable line below → still PASS (quote rules hold); an unreadable first line with a quoted PASS below → refused, `verdict` stays null. ## Red without the source change Two knives, each applied alone to `review-gate.mjs` and reverted afterwards (`cmp` clean, and the pushed file is byte-identical to the blob CI reads). **A. This test's knife: remove `firstUnknown`'s half of `firstIsVerdict`.** The capture stays; only `const firstIsVerdict = first !== null || firstUnknown !== null` becomes `first !== null`: ``` not ok 16 - ThinkFlowLab#1239: a readable PASS quoted under an unreadable first line does not rescue it error: |- the header is the unreadable word + actual - expected + null - 'Incorrect' # tests 58 # pass 57 # fail 1 ``` One test goes red and it is the new one; the other 57 stay green, so the half is load-bearing and nothing else covers it. With the header no longer treated as the header, the quoted `Verdict: PASS` below it is read (`parsed → {verdict: "PASS", unknownVerdict: null}`) and `decide` answers `ok=true verdict=PASS reason=reviewer-agent's PASS names this tip` — the tip goes green on a quotation, which is the defect of ThinkFlowLab#1239 in its second shape. **B. Both reads removed** (`firstUnknown` forced to `null`, the loop's `unknownLine` read removed): ``` not ok 12 - ThinkFlowLab#1239: an unreadable verdict word at the tip vetoes the older PASS there not ok 13 - ThinkFlowLab#1239: an unreadable word alone at the tip still refuses, and the reason names the word not ok 15 - ThinkFlowLab#1239: the legacy Tip SHA block needs the same read as the first line not ok 16 - ThinkFlowLab#1239: a readable PASS quoted under an unreadable first line does not rescue it # tests 58 # pass 54 # fail 4 ``` Four reds under that knife, all of them ThinkFlowLab#1239 tests; the other 54 are unchanged. ## Not run - `npm run verify`: this box has no `npm ci` / `core/dev/harness` install for the core suite, and the change is under `.github/scripts/` only. The three gate suites above are what covers this file. - The live `review-gate.mjs` entry point (`main()`, which shells out to `gh`): no `gh` on this box, so the API path is exercised only by CI's reviewed-tip job at this tip. - A signed conclusion: the `signers` roster is empty (ThinkFlowLab#1111 closed as not planned), so `unknownVerdict` under signing is unreachable today and not tested — the signing block runs before `atTip`, so an unverified line stays unverified rather than becoming an unreadable one. ## Risk - **Fail-closed by construction, and it widens the veto slightly.** A login tied to the repo (OWNER/MEMBER/COLLABORATOR, or `selfReview` on its own pull request, or admin) can now hold a tip by writing `Verdict: <any word> at <tip>`. That is the same power a `Verdict: INC` already has under ThinkFlowLab#676, and it is the point of the issue: the alternative is the invisible withdrawal. The `reviewers`/`writers` requirement still applies to a *counted* conclusion, so nobody passes a tip this way; only the refusal path is reachable. - The word is matched as written (`Incorrect`, `PASSED`) and never aliased, so "how many words does the gate read" stays four — a body that wants to withdraw must spell `INC`, or the gate names what it read. - Nothing in `merge-gate.mjs`, `enqueue-guard.mjs` or `review-policy.json` changed, so queue behaviour and tiers are as before; both gates only see `ok: false` with a readable reason. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1242, "claim": "Dev3", "issue": 1239, "summary": "A Verdict: line is read as one of four words; a fifth word used to be invisible rather than refused. Verdict: Incorrect at <tip> matched neither pattern, so decide set no tip and no verdict, skipped the comment, and the PASS it withdrew stayed the newest word — on ThinkFlowLab#1221 the gate still reported PASS. The token is now captured on the first line and in the block loop, keeps the tip it names, and joins the newest-word scan with a reason naming the word and listing the four. A word with no tip stays an ordinary comment.", "files": [".github/scripts/review-gate.mjs", ".github/scripts/review-gate.test.mjs", "docs/autonomous-dev.md"], "checks": { "ran": "node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs .github/scripts/enqueue-guard.test.mjs", "result": "green", "at": "ac8d43cf931d577c0ea7e5dfaff1345b9d98b124" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #637, #643
Reproduce
The merge that closed #632 carried an INC at the same tip (
b99d6052, 12:49Z) and merged anyway. Two causes, both reproduced in tests here:So: the first line of the conclusion was not the verdict, and the mechanical gate was not standing in the way.
What changed
.github/scripts/review-gate.mjs— the conclusion is now the first non-empty line, exactlyVerdict: PASS|INC|REQUEST_CHANGES at <sha>. That line carries its own sha, so a verdict the Chief posts for a Reviewer with no write channel parses exactly like the Reviewer's own. The evidence (Ran:,Not run:) is read below it.REQUEST_CHANGESis accepted in both forms..github/scripts/merge-gate.mjs—main()now takes itsghrunner as an argument (default: the real one), so a test drives the same entry point the queue runs, not a helper beside it. No behaviour change on its own..github/workflows/merge-gate.yml— the job now runs on both events:merge_groupjudges and a refusal fails it,pull_requestonly reports what the queue entry would refuse and never fails. A required check that only ever reports onmerge_groupleaves every pull requestblockedon a status no run produces, so nothing can enter the queue (Merge gateis not a required check on the merge queue, so a red gate does not block a merge #639, the ruleset change that was rolled back). Thepull_requestrun cannot fail either: a verdict arrives as a comment, which starts nopull_requestrun, so a red report would outlive the INC it names until the next push. The file also records which context a ruleset must list — the job namemerge-gate, not the workflow nameMerge gate, which no run ever produces.merge-gate.mjsfrom the base commit (so a pull request cannot weaken the gate that judges it), and main still has the version without report mode: it refused for lack of a conclusion and the check went red on its own PR. The report step is nowcontinue-on-erroronpull_requestonly, so an older base copy cannot fail the check; the queue entry still fails when it refuses..github/scripts/review-gate.mjs— a first-line verdict is final (review-gate: a first-lineVerdict: INCis read back as PASS when the body quotes an older PASS at the same tip #643). The scan that collectsRan:/Tip SHA:also matched anyVerdict:line below the first one, so a body quoting the PASS an INC replaced — same tip — parsed as PASS anddecidesaid ok. One condition: the scan no longer writes the verdict once the first line is one..github/scripts/merge-gate.mjs— T0/T1 and the mechanical release PR need no PASS (Read the verdict from its first line, and test the merge gate through its own entry point #640 INC point 2). Without it the gate would refuse the daily release PR and every T0/T1 fix as soon as it became a required check, which is the opposite of AGENTS.md 6. The tier comes from the controller'srisk:T*label; a release PR is recognised by the exact file list the standing approval names.INCat the tip refuses the queue entry;holdrefuses; a body whose first line isVerdict (written by …)is no conclusion and refuses; a first-linePASSposted as a proxy passes; a review body is judged like a comment. Plus the parser tests inreview-gate.test.mjs.AGENTS.md— hard rule 6 gets the A schedule that exists in the UI is invisible to the Bot:schedule_listreturns[]for the session, not for the Bot #628 sentence back, word for word: an emptyschedule_listis not evidence that a schedule does not exist; never delete or recreate a schedule from it, and never report "no timers" as a finding. Merge-gate rule 4 states the fixed first-line format.docs/autonomous-dev.md,.claude/skills/review-pr/SKILL.md— the template and the rules say what the gates now read.What I ran
At tip
8203bf3d:.github/scripts/*.test.mjssuite: 121 pass, 4 fail — all four inds-bot-patches.test.mjs, which shells out togit; this container has nogit(spawnSync git ENOENT). They are unrelated to this diff and CI runs them.merge-gateand it now appears on a pull_request head — atf28f63bathe run is 9/9 (the eight usual ones plusmerge-gate), which is the first of the two conditions for listing it as required.Tip SHA:…Verdict: PASS) still parses, so nothing already recorded stops counting.Red without the source change
With
.github/scripts/review-gate.mjstaken fromfe79de66(no first-line support), the tests kept:(The "first line is not the verdict" refusal holds on both sides, as it must — that half only refuses.)
With
.github/workflows/merge-gate.ymland.github/scripts/merge-gate.mjstaken from8ffa52b9(nopull_requestrun, no report mode), the same command keeps:With
review-gate.mjsas it is onf28f63ba(first line set, scan still writing the verdict):Not run
npm run verifyend to end (patches check andarch-docsneedgit), the core/plugin suites (this diff touches.github/and docs only), smoke. CI covers them at the tip.Risk
One direction only: text that is not a first-line verdict is no longer a conclusion, so a PR whose only conclusion is in the old shape waits for a fresh one — the safe direction, and the old
Tip SHA:block still parses. The gate still only blocks oncemerge-gateis a required check of themain merge queueruleset — a ruleset change for the maintainer, never an agent (AGENTS.md → Merge gates 1), and it must name the jobmerge-gate; the workflow nameMerge gateis a context no run produces and would leave every pull requestblocked. The report onpull_requestcan name a refusal that has since been answered: that is deliberate, and why it never fails there — the queue entry re-reads live.Tier from the diff: T2 (CI gate, no secrets/permissions).
Revision after the 8203bf3 INC
review-gate.mjs).parseReport()now keeps the first line'stipas well as itsverdict: a quotedTip SHA: <other sha>block below the conclusion can no longer move the tip, soan INC at this head is no longer dropped as "about another tip" while the PASS it refused revives.
The first line's own
Ran:is the evidence, and a quoted block'sRan:does not replace it.merge-gate.mjs).exemptFromPassno longer readsrisk:t0/risk:t1labels. Theworkflow already checks the script out from the base commit, so it now loads the base copy of
risk-gate.mjsand.github/agent/risk-rules.jsonand computes the tier from the changed files.A label is a claim, not a fact: one shared account, no signature (An owner decision on a PR cannot be told apart from Agent-written text: no proxy signature, one shared identity #627).
holdstill refuses at anytier, and an empty file list is no exemption.
AGENTS.md→ Merge gates anddocs/autonomous-dev.mdsay the same thing in one paragraph each.Ran:
node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs .github/scripts/risk-gate.test.mjs-> 58 tests, 58 pass, 0 fail (review-gate 24, merge-gate 20, risk-gate 14).
Red without the source change:
review-gate.mjsback at8203bf3d-> 23/24, red#640: a quoted Tip SHA later in the body does not move the first line to another tip;merge-gate.mjsback at8203bf3d-> 19/20, red#640: the tier comes from the changed files, and a label never exempts.Not run: the whole
.github/scriptssuite beyond those three files, and any CI job locally (noghhere); CI covers them on the pushed tip.Revision after
fa9a76f4: the queue entry'shead_refarrives fully qualifiedprFromHeadRef()in.github/scripts/merge-gate.mjsnow accepts a head_ref thatcarries the
refs/heads/prefix, and still accepts the baregh-readonly-queue/...form.merge_grouprun so far failed before judging anything:GitHub passes
merge_group.head_refasrefs/heads/gh-readonly-queue/main/pr-<N>-<sha>, and theparser required the string to start at
gh-readonly-queue/. Five queue entries, five failures —39602546475,39602806953,39653640861,39694988559(fix(turn-origin): fail closed on a source kind it does not recognise (#620) #626) and39696278650(Say why a saved key is withheld from a shell (#651) #652), eachending in
cannot read a pull request from head_ref. So the job has never judged a queue entry;listing
merge-gateas a required check would have blocked every merge, not gated it.Ran:
node --test .github/scripts/merge-gate.test.mjs .github/scripts/review-gate.test.mjs .github/scripts/risk-gate.test.mjs-> 59 tests, 59 pass, 0 fail (merge-gate 21, review-gate 24, risk-gate 14).
Red without the source change: with
.github/scripts/merge-gate.mjsrestored tofa9a76f4(the one-line prefix strip removed) -> 20/21, red
#652 follow-up: a head_ref that carries refs/heads/ still names the queue entry.Sequencing, for whoever flips the ruleset: the job reads the script from the base commit, so a
queue entry for this very pull request still runs the broken parser on
main.merge-gatecan only belisted as a required check after this fix is on
main; listing it first would freeze the repository.