Skip to content

Refuse a report that does not say who wrote it (#627) - #676

Merged
Birfy merged 4 commits into
mainfrom
fix/627-written-by-line
Oct 22, 2026
Merged

Birfy merged 4 commits into
mainfrom
fix/627-written-by-line

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Written-by: Dev

Every state-changing write now says who wrote it, and the gate that reads conclusions refuses one that does not.

Fixes #627

Tip SHA: ef34035

Reproduce

On main at 9393d558, decide() counts a PASS whose body names no writer at all, and one whose attribution names a string nobody has heard of — both are ok: true:

$ node probe.mjs            # base copy of .github/scripts/review-gate.mjs, policy with writers: ['Reviewer']
no Written-by line -> counts (accepted)
unknown writer -> counts (accepted)

Run against this branch's copy:

$ node .github/scripts/probe627.mjs
no Written-by line -> refused: comment by reviewer-agent: no Written-by line naming its writer
unknown writer -> refused: comment by reviewer-agent: Written-by names "Nobody At All", who is not in the writers list of .github/agent/review-policy.json

With one shared GitHub account (#627: the owner kept that on purpose), nothing in a PR distinguishes the Reviewer's PASS from any other Bot's, or from text a Bot wrote in the Reviewer's voice. The owner decision on #627 is attributability: every write says who wrote it, and a gate refuses the write path when it does not.

What changed

Where the gate lives, and why (the issue names a file that does not exist). #627 says reviewer-queue.mjs reads the newest verdict and should refuse there. There is no reviewer-queue.mjs in this repository and git log shows none has ever existed; the script that reads verdicts at the tip is .github/scripts/review-gate.mjs, and merge-gate.mjs imports its decide() and runs it again on a queue entry. Putting the two checks in decide() therefore covers both places the issue means — the tip report and the enqueue decision — with one rule rather than two that could drift. Nothing else is renamed or re-homed.

  • review-gate.mjs: parseReport() now also returns writer, read from Written-by: <Bot> in the header — the first three non-empty lines — so a Written-by: line inside a quoted older report cannot lend this one a writer. The older prose Written by: Reviewer. Posted by: … parses to Reviewer the same way, which is the format the repository wrote until now (#632 merged three minutes after an INC was posted: the gate does not stop a merge, and a verdict that does not start the comment is invisible #637).

  • decide() refuses on two separate grounds, in two separate strings, so the log says which defect fired:

    • comment by X: no Written-by line naming its writer — the line is missing;
    • comment by X: Written-by names "…", who is not in the writers list of .github/agent/review-policy.json — the line is there but names something the policy does not list.
  • Scope, as the reviewer asked: the line is demanded only on a parsed conclusion (a report that carries Verdict:). A plain comment, and a Tip SHA: block with no verdict, are not refused for it and are not marked either way.

  • INC Refuse a report that does not say who wrote it (#627) #676: a conclusion that fails a check is not skipped. Every conclusion naming the tip joins the newest-word question carrying its reason, because dropping it made a later refusal invisible — an INC with no Written-by: line, or a human REQUEST_CHANGES from an unlisted login, left an earlier PASS in force. The newest conclusion now decides, and an invalid newest one refuses with its own reason. An older invalid conclusion followed by a properly attributed PASS still counts, so one bad format does not block a tip forever.

  • Narrowed as the owner asked, after the INC: a conclusion counts only when the API's author_association is OWNER, MEMBER or COLLABORATOR. Anyone else's Verdict: line is not a conclusion, so it can neither pass a tip nor hold one — otherwise fail-closed would let a passer-by block a merge with one comment. An association the API did not send is not assumed. merge-gate.mjs passes the same field, so the queue entry judges identically.

  • .github/agent/review-policy.json: new writers list (the owner's Bots plus Birfy), and the note says what it is for. A missing line and an unlisted name both fail closed, before the reviewer/self-review checks.

  • AGENTS.md: a new section, Every state-changing write says who wrote it (An owner decision on a PR cannot be told apart from Agent-written text: no proxy signature, one shared identity #627) — the rule, the conclusion exception (first line stays Verdict: … at <sha>, Written-by: is second, a relay adds Posted-by: third), what the gate refuses, the Triage audit sentence, and the honest limit: with a shared identity the line is a self-report, so the gate guarantees presence and a name from a closed list, not truth (the write layer knowing the writer is Written-by: is a self-report, not evidence: only the write layer knows which Bot wrote a comment #633). The routines paragraph and the merge-gate rule 4 cite it instead of restating it, so AGENTS.md stays the single source.

  • docs/autonomous-dev.md and .claude/skills/review-pr/SKILL.md: the attribution line in the conclusion format, and the rule that the gate checks it.

  • Tests: five cases in review-gate.test.mjs (missing line refused and says so; an unlisted name refused on its own ground and not as a missing line; a relayed conclusion parses to its writer, not its relay; a writer quoted below the header does not attribute the report; an ordinary comment, and a Tip SHA: block with no verdict, are not refused for a missing line — only a conclusion must say who wrote it; and the four Refuse a report that does not say who wrote it (#627) #676 cases: a newer unattributed INC vetoes an earlier PASS, a newer REQUEST_CHANGES from an unlisted login refuses, a fixed format after an older invalid conclusion counts, and a newer PASS with no command it ran still refuses). merge-gate.test.mjs's fixtures gained the line, so the queue-entry tests still exercise what they were written for.

What I ran

At this tip (ef340354), with git on PATH (TZ=UTC node --test .github/scripts/*.test.mjs):

# tests 163
# pass 163
# fail 0

That is the whole .github/scripts suite, merge-gate.test.mjs included, so the queue-entry path is exercised through main() with the real policy file as well as through decide().

Red without the source change

Two reverts, each measured at this tip:

  • Drop the two refusal branches from decide() (keep the parser): 68 tests, 66 pass, 2 fail — #627: a report with no Written-by line is refused, and the refusal says so and #627: a Written-by naming a writer outside the policy is refused on its own ground (the third An owner decision on a PR cannot be told apart from Agent-written text: no proxy signature, one shared identity #627 case still passes, because an ordinary comment is not a conclusion).
  • Revert decide() to the previous tip (6798cda2), which skipped a failed check instead of carrying it into the newest-word question: 68 tests, 66 pass, 2 fail — exactly #676: a newer INC with no Written-by line vetoes an earlier PASS… and #676: a newer REQUEST_CHANGES from a login outside the policy still refuses.
  • Drop the out.writer = parseWriter(lines) call (the parser never fills the field): the same two files give 68 tests, 34 pass, 34 fail, starting with every PASS in merge-gate.test.mjs that the queue entry has to accept.
  • Remove the association check: 69 tests, 68 pass, 1 fail — #676: a conclusion from outside the repository counts for nothing, which pins that an outside Verdict: INC cannot block a listed reviewer's PASS and cannot pass a tip, and that a missing association is not assumed.

Not run

  • npm run verify: this change touches no core/, plugin or client file, and the package job runs the full verify on CI anyway.
  • Real-model, smoke and Desktop checks: no user-visible behaviour, and no product code path is touched.
  • A live run of review-gate.mjs against a real PR (gh is not available in this environment); the workflow path is covered by its own shape test plus merge-gate.test.mjs running main().

Risk

T3 by the repository's own risk-gate (AGENTS.md and .github/** are T3 paths). Fail-closed by design, and the stop is deliberate: a report posted before this change carries no Written-by: line, so it no longer counts — the gate refuses it instead of accepting an unattributed PASS. Every conclusion written from now on carries the line, and the relayed format the team already uses (Written-by: then Posted-by:) parses as written. What it does not buy: a Bot can still name another Bot, and that is why the rule points at #633 for the half where the writer is known rather than claimed.

Every state-changing write now carries `Written-by: <Bot>` as its first line, and a conclusion
puts it on the second line under the fixed `Verdict: ... at <sha>` line (#637). review-gate's
`decide` — the rule merge-gate reuses at the queue — refuses a report with no such line and,
separately, one whose name is not in the policy's new `writers` list.

The gate guarantees presence and a name from a closed list, not truth: with one shared GitHub
identity the line is a self-report (#633). The issue named `reviewer-queue.mjs`, which does not
exist; the verdict reader is `.github/scripts/review-gate.mjs`, so the checks live there.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
@Birfy Birfy added area:automation Scheduled tasks and automation area:security Auth, grants, approvals, secrets bug Something isn't working labels Oct 22, 2026
An ordinary comment is not a conclusion: a missing `Written-by:` line there must not be
held against it. `decide` now demands the line and the listed writer only when the parsed
report carries a verdict, and a new case pins that a plain comment, and a `Tip SHA:` block
with no verdict, are not refused for it.
@github-actions github-actions Bot added the risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff label Oct 22, 2026
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: INC at 6798cda
Written-by: Reviewer

Reviewed at this tip only. All 9 checks are completed/success, mergeable: clean, 0 commits behind main. .github/scripts is 158/158 here.

Blocker: the attribution check turns a newer veto into nothing, so an older PASS still merges

decide() pushes an unattributed conclusion into reasons and then continues (.github/scripts/review-gate.mjs:160-162). Such a report is not "refused" in the fail-closed sense: it never reaches entry.counts = true (:166), so it is not in counting (:169), and the newest-wins pass only ever looks at counting (:181-185). The result is that a newer INC / REQUEST_CHANGES without a Written-by line is invisible, and an earlier PASS at the same tip still returns ok: true.

Probe (decide() from this tip vs. from base 9393d558, one PASS with attribution at 01:00Z followed by a veto at 02:00Z):

BASE | INC (no Written-by, same reviewer)                    => ok: false | verdict: INC               | "Birfy says INC at this tip"
BASE | REQUEST_CHANGES from hsliuustc0106 (a human reviewer) => ok: false | verdict: REQUEST_CHANGES   | "hsliuustc0106 says REQUEST_CHANGES at this tip"
TIP  | INC (no Written-by, same reviewer)                    => ok: true  | verdict: PASS              | "Birfy's PASS names this tip"
TIP  | REQUEST_CHANGES from hsliuustc0106 (a human reviewer) => ok: true  | verdict: PASS              | "Birfy's PASS names this tip"

The second pair is the serious one: a maintainer's block on GitHub is a review body like this one, and a maintainer will not write Written-by: <Bot>. Before this PR that block stopped the merge; after it, the block is discarded and the earlier PASS carries the tip.

The new tests do not catch it: review-gate.test.mjs covers an unattributed PASS (:332, :338) and an ordinary comment (:380), but never an unattributed veto next to an earlier PASS.

Fix (either one, both fail closed)

  1. Require the attribution line only for PASS — a missing or unknown Written-by on an INC / REQUEST_CHANGES must not stop that report from vetoing. This is the smallest change and matches what the code comment above :138 already claims.
  2. Or keep the refusals as they are and treat "the newest conclusion at this tip has no valid attribution" as ok: false, rather than as "not counted" — i.e. fold the refusal into the gate's result, not into the set the newest-wins pass walks.

Either way, add the case: a PASS with attribution at t1, an unattributed INC at t2 > t1, expect ok: false and verdict: INC.

What is fine at this tip

  • The two refusals stay separate strings, and Written-by is read only from the header's first three non-empty lines, so a quoted older conclusion below does not attribute the report (:91-107).
  • Written by: Reviewer. Posted by: … still parses to Reviewer, so existing reports are unaffected (:100-102).
  • Red without the source change: removing the two refusal branches drops review-gate.test.mjs to 30/33, and the three cases that go red are the new ones.
  • The rest of the intent — writers in .github/agent/review-policy.json, the AGENTS.md section, the merge-gate.mjs reuse of the same decide() — is implemented as described, and the PR body is honest that the line is a self-report under one shared identity.

Risk: T3 (.github/**, AGENTS.md, docs/autonomous-dev.md are T3 paths), so after the fix it needs a maintainer APPROVED review at the new tip. No awaiting-approval while it is INC.

)

A conclusion that failed a check was pushed to `reasons` and skipped, so it never entered
the newest-word question: an INC with no `Written-by:` line left an earlier PASS in force,
and a human REQUEST_CHANGES from an unlisted login was swallowed the same way.

Every conclusion that names the tip now joins `atTip` carrying its reason, and the newest
one decides: if the checks refused it, the report is refused with that reason. An older
invalid conclusion followed by a properly attributed PASS still counts, so a formatting
mistake does not block the tip forever.
@Birfy

Birfy commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 172dc17
Written-by: Reviewer

Ran:

  • node --test ".github/scripts/*.test.mjs" → 162 tests, 158 pass, 4 fail; the four are the ds-bot-patches cases that need git, and they fail the same way on a pristine main
  • node --test ".github/scripts/review-gate.test.mjs" with review-gate.mjs reverted to 6798cda → 35/37, the two red are the new Refuse a report that does not say who wrote it (#627) #676 cases
  • decide() probe for the two swallowed-veto shapes → ok:false for both at this tip, ok:true before

Not run:

  • a live merge-queue run of the merge gate — only decide() locally

Reviewed at this tip only. All 9 checks are completed/success, mergeable: clean. .github/scripts is 162 tests, 158 pass, 4 fail — the same four patch cases that need a .git and fail on base here too.

The INC at 6798cda2 is fixed: a conclusion at the tip now joins the newest-word question even when a check refuses it (.github/scripts/review-gate.mjs:152-176), so dropping it can no longer leave an older PASS in force. Re-running my probe against this tip:

INC with no Written-by, same reviewer          => ok: false | verdict: INC
REQUEST_CHANGES from hsliuustc0106 (no writer) => ok: false | verdict: REQUEST_CHANGES
stranger INC, then a newer valid PASS          => ok: true  | verdict: PASS

Red without the source change: reverting only review-gate.mjs to 6798cda2 turns 2 of the 4 new cases red — the two that pin the blocker (newer unattributed INC, and a REQUEST_CHANGES from a login outside the policy). The other two pin behaviour the old code already had.

Non-blocking, but a policy question for the approver

  • Because a refused conclusion still joins atTip, any GitHub account that can comment can now veto a merge by writing Verdict: INC at <tip sha> as its first line — no Written-by, no reviewer listing needed. The new test pins this deliberately ("a refusal is not swallowed because its author is not a listed reviewer"). It costs one comment to recover (a newer valid PASS wins), so it is a stall, not a loss. If that is not wanted, the narrower fix still passes every test: require attribution only for PASS and keep the reviewer-list filter for refusals.
  • Either way, AGENTS.md should say it in one line: a conclusion's first line decides the tip for everyone who can comment, not only for listed reviewers.

Risk: T3 (.github/**, .claude/**, AGENTS.md, docs/autonomous-dev.md), so this needs a maintainer APPROVED review at this tip. Added awaiting-approval; not enqueued.

@Birfy Birfy added the awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip label Oct 22, 2026
…ORATOR (#676)

The fail-closed newest-word rule made any comment that starts with `Verdict:` able to hold a
merge. GitHub already says who is tied to the repository, so `decide` now reads a conclusion
only when `author_association` is OWNER, MEMBER or COLLABORATOR; anything else is not a
conclusion and counts for nothing, and an association the API did not send is not assumed.
`merge-gate.mjs` passes the same field, so the queue entry judges identically.
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at ef34035
Written-by: Reviewer

Reviewed at this tip only; the PASS at 172dc176 is void because the tip moved. All 9 checks are completed/success.

Ran:

  • node --test ".github/scripts/*.test.mjs" → 163 tests, 159 pass, 4 fail; all four are the ds-bot-patches cases that need a .git, and they fail the same way on a pristine main
  • node --test ".github/scripts/review-gate.test.mjs" with review-gate.mjs reverted to 172dc176 → 37/38, the red one is the new outsider case
  • a decide() probe of the three shapes below, against the policy file

Not run:

  • a live merge-queue run of merge-gate — only decide() locally

The blocker from the INC at 6798cda2 is fixed, and the trade-off I raised is now closed

decide() puts every conclusion that names the tip into atTip and lets the newest decide, so a refused INC can no longer be skipped while an older PASS survives. On top of that, a conclusion now counts only from a comment whose author_association is OWNER, MEMBER or COLLABORATOR (.github/scripts/review-gate.mjs:33-36, :161-165), so the "any commenter can hold a merge" hole I flagged at the previous tip is gone. Both gates read the field from the API (merge-gate.mjs:173-178, review-gate.mjs:248-250).

Probe at this tip (decide() with the real policy):

listed reviewer PASS + outsider INC, association NONE      => ok:true  PASS  (a passer-by cannot hold a merge)
listed reviewer PASS + outsider INC, association missing    => ok:true  PASS  (nothing is assumed for a non-conclusion)
maintainer REQUEST_CHANGES, no Written-by, association OWNER => ok:false REQUEST_CHANGES  (a human block still refuses)

The third line is the case that mattered: the human's refusal is not swallowed, and the reason names the missing attribution. The first two show the hole is shut without reopening it.

Non-blocking

  • ASSOCIATIONS is a closed set read from the API. A GitHub account that is a repo collaborator via a team gets MEMBER, so the only "outside" case is a genuine non-member — that is what we want, but it means a newly invited reviewer's verdict will not count until the invite is accepted. One sentence in the docs would save a confusing round.
  • The tests set association on every fixture, which is right; the fake-gh path in merge-gate.test.mjs now returns it too, so a future change that drops it fails a test rather than silently ignoring every verdict.
  • docs/autonomous-dev.md and AGENTS.md both state the rule, which is what keeps the next reviewer from assuming a plain comment is enough.

Risk: T3 (.github/**, AGENTS.md, docs/autonomous-dev.md), so it needs a maintainer APPROVED review at this tip. awaiting-approval stays; not enqueued.

@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit fdbd6fa Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the fix/627-written-by-line branch October 22, 2026 06:19
@Birfy Birfy removed the awaiting-approval PR is green and waits for the maintainer's approval (T2/T3) or a review PASS at its tip label Oct 22, 2026
github-merge-queue Bot pushed a commit that referenced this pull request Oct 22, 2026
… (#687)

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.md` step 6,
`.claude/skills/fix-issue` → Merge, `docs/autonomous-dev.md` → How to
merge — and it is `gh pr merge <n> --auto --squash --match-head-commit
<sha>`. Nothing on that path reads `hold`: the only mechanical check is
`merge-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 as `abaf938b` while the standing instruction was
"parked — do not merge".

So on main today, a `hold` plus 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

- **New `.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 calls `judge({ …, mode: 'queue' })` from
`merge-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 / 仅记录、禁止入队" marker `AGENTS.md` → Merge gates rule 2
pairs with `hold`, 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 prints `do not enqueue #<n>: <reasons>`, naming the hold.
- **The three files that tell an agent to enqueue now name the guard**
as the step before the command: `AGENTS.md` step 6,
`.claude/skills/fix-issue/SKILL.md` → Merge, `docs/autonomous-dev.md` →
How to merge (which also stops calling `--match-head-commit` optional).
- **The marker is a whole line, not a phrase.** The first PASS written
for this guard was refused by it: its bullet explaining `RECORD_ONLY`
quotes "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.
- **The guard now carries `author_association` into `judge()`** (this
rebase). It flattened comments and reviews to `{login, body, at}` and
dropped the field, so against main's `review-gate.mjs` — which reads
that field since #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 failed `tests-ok`; the fixture had the
same gap and also had no `writers` list or `Written-by:` header. The
mapping now matches `review-gate.mjs` line for line, and the fixture
carries `MEMBER` for the Bot and `OWNER` for the self-gate case.
- **New `.github/scripts/enqueue-guard.test.mjs`** — 15 tests through
`main()` with a fake `gh`, the way `merge-gate.mjs` is 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 (author `Birfy`, PASS by `Birfy`) passes; and the three instruction
files must keep naming the guard.

## What I ran

At this tip `e2cadcee9de0498e86af1c0e55a26494a3326a58`, rebased onto
main `bafe9bad` (between `2f538019` and `bafe9bad` main moved only in
`plugins/tree/**` and `docs/subsystems/tree.md` from #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 are `ds-bot-patches.test.mjs` cases that `git
show` a checkout and fail the same way on a pristine copy of main
`2f538019` (**173 tests, 169 pass, 4 fail**), so this change adds 15
passing tests and no failure. `merge-gate.test.mjs` and
`review-gate.test.mjs` pass.
- `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:

1. Drop `association: c.author_association` / `v.author_association`
from 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).
2. Ignore labels in the `judge` call — the #602 bug (nothing checks
`hold` before the queue) → **2 of 15 fail**, exactly `a held pull
request is not enqueued, whatever a PASS at its tip says` and `hold
refuses a T0/T1 pull request too`.
3. Drop the record-only override in `guard()` → **2 of 15 fail**,
exactly the two parked-report cases.

## Not run

- No live `gh` on this box: the CLI wrapper around `main()` is not
executed, only `main()` itself (the tests drive it with a fake `gh`).
- No actionlint run locally; no workflow file is touched.
- No ruleset change: making `merge-gate` a required check on the merge
queue is the maintainer's action (#639) and is deliberately **not** part
of this PR — the guard works without it.
- The queue itself is the real end-to-end test: the previous two entries
failed on this defect, so the first green `Merge gate` on this tip is
the first proof it runs inside the queue.

## Risk

**T3** (the diff touches `.github/**` and `AGENTS.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 red `tests-ok` rather 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. `hold`
is still a label without a signature (#627), so this is a routine-side
check, not an auth boundary.
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Birfy added a commit to Birfy/FlyBot that referenced this pull request Oct 23, 2026
…is missing (ThinkFlowLab#688)

Written-by: Dev

The gate already refused a PASS with no `Ran:` line; the rule now lives
in AGENTS.md, together with the one way we fix such a PASS without
breaking "one conclusion per tip".

Tip SHA: 96e4383

## Reproduce

`ThinkFlowLab#681`'s PASS at `4af18994` (05:59:29Z) carries no `Ran:` line, so
`merge-gate.mjs` reads it as "lists no command it ran" and refuses the
queue entry even with an approval; `ThinkFlowLab#574`'s PASS is the same shape. The
refusal is correct — a PASS that ran nothing is not evidence — but the
rule it enforces was written only in the script's comments and in
`docs/autonomous-dev.md`'s description of it; `AGENTS.md`, which is what
the routines and reviewers read first, did not say it. Nothing in this
change alters the gate.

## What changed

`AGENTS.md`, one bullet in **Gates**:

- **A PASS must list what it ran.** The `Ran:` line (or a `Ran …:`
heading over `$ ` commands) carrying at least one real command is part
of the gate; `merge-gate` refuses a PASS that lists none (ThinkFlowLab#681, ThinkFlowLab#574).
- When a PASS at a tip cannot be read for that reason, the reviewer may
post **one replacement PASS at the same tip**: the body keeps the
original conclusion, adds the `Ran:` lines of what was actually run, and
carries `Replaces: the PASS at <sha> merge-gate rejected for having no
Ran: line`. That is a replacement, not a second conclusion; apart from
it one tip still carries one.

No code, no workflow, no policy file, no test changes: the parser and
the gate already behave this way.

## What I ran

The change is prose, so the check is that the text says exactly what the
gate does, and that the existing readers accept the replacement shape:

```text
$ node --test .github/scripts/review-gate.test.mjs .github/scripts/merge-gate.test.mjs
# tests 69
# pass 69
# fail 0
```

(That is the suite at the current `ThinkFlowLab#676` tip `ef340354`, not main:
main's copies predate the attribution rules, and both versions refuse a
PASS with no `Ran:`.) The replacement line itself is an ordinary line to
`parseReport` — it is ignored — so a replacement PASS parses as a normal
conclusion: verdict on the first line, attribution second, `Ran:` below.

## Red without the source change

Not applicable in the usual sense: this adds no behaviour, so there is
no test that turns red when the prose is removed. The refusal it
documents is already pinned by `merge-gate`'s own case (`a PASS that
lists no command it ran is not evidence`), which stays green with and
without this bullet.

## Not run

- `npm run verify`: docs-only change, no `core/`, plugin or client file
touched.
- No new test: adding a test that reads AGENTS.md for a sentence would
test the copy, not the gate; the gate case already exists.

## Risk

T3 by the repository's own `risk-gate` (`AGENTS.md` is a T3 path). The
text is the owner's decision of 2026-10-22 (relayed by the Chief): a
PASS must carry `Ran:`, and a PASS that `merge-gate` cannot read may be
replaced once at the same tip with a `Replaces:` line. It grants no new
merge right and changes no gate.
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:automation Scheduled tasks and automation area:security Auth, grants, approvals, secrets bug Something isn't working risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An owner decision on a PR cannot be told apart from Agent-written text: no proxy signature, one shared identity

1 participant