Skip to content

Let Reviewer2 record a report the gate counts (#627) - #765

Merged
Birfy merged 1 commit into
mainfrom
chore/627-reviewer2-writer
Oct 22, 2026
Merged

Birfy merged 1 commit into
mainfrom
chore/627-reviewer2-writer

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Reproduce

Reviewer2 was added to the team today. Every conclusion it wrote was refused by the gate:

$ node .github/scripts/review-gate.mjs --repo ThinkFlowLab/FlyBot --pr 646
... Written-by names "Reviewer2", who is not in the writers list of .github/agent/review-policy.json

The list is read from the base commit (.github/agent/review-policy.json, writers), and Reviewer2 is not in it, so a Written-by: Reviewer2 PASS does not count — for every tip, not only that one. Nothing failed loudly: reviewed tip is report-only, so the gap only surfaces when a T2/T3 pull request waits for a conclusion that never arrives.

What changed

  • .github/agent/review-policy.json: Reviewer2 joins writers, next to Reviewer. One line; reviewers, selfReview and the note are untouched.
  • .github/scripts/review-gate.test.mjs: the real-policy test now walks a written-out roster (TEAM_WRITERS) and, for each name, asserts (a) it is in writers and (b) a PASS carrying Written-by: <name> is accepted by decide(). report() gained a writer option (default Reviewer, so the other cases are unchanged).

The roster is spelled out on purpose: when a Bot joins the team, this list and the policy have to gain the name together, and a name that loses its right is a T3 edit here.

No user-visible change (no changelog fragment): this is automation configuration, not a product behaviour. Docs are untouched — docs/autonomous-dev.md describes writers without listing its members.

What I ran

$ node --test .github/scripts/review-gate.test.mjs
# tests 39   # pass 39   # fail 0        (38 existing + 1 new)

$ node --test ".github/scripts/*.test.mjs"
# tests 221  # pass 217  # fail 4

$ node .github/scripts/changelog.mjs check
changelog: 29 fragment(s) in changelog.d/ are well formed

The four failures are all in ds-bot-patches.test.mjs and are environmental: this box checks out a tarball, so ds-bot/ has no working tree. The same file is 4/4 red on pristine main here, before this change, and CI's package job (which has the submodule) runs it green.

Red without the source change

  • Delete Reviewer2 from writers in .github/agent/review-policy.json, keep the test → 38/39, the only red is the policy lets every Bot on the team write a report, and its message is these Bots would have every PASS refused: Reviewer2. Restore → 39/39.
  • The other direction: add Reviewer3 to the roster without touching the policy → same test red, message would have every PASS refused: Reviewer3. Revert → 39/39.

Not run

  • No live gate run against a real pull request with the new list: review-gate.mjs needs gh and network, which this box does not have. The unit test drives the same loadPolicy() + decide() path on the real file.
  • The package job as a whole (patches, tsc, actionlint, interop): CI runs it on the tip; the four local ds-bot-patches.test.mjs failures above are the only gap and are identical on main here.

Risk

T3, as review-policy.json says of itself. The behaviour change is that a PASS whose Written-by: names Reviewer2 now counts; nothing else in the gate moves, and an unknown name is still refused. Reviewer2 is a Bot and a reviewer, so the change follows the same rule Reviewer already had. Rollback is deleting the line.

Fixes #627

The gate reads `writers` from .github/agent/review-policy.json and refuses a
`Written-by:` line that names something outside the list, so every PASS
Reviewer2 wrote today was refused. The team gained that Bot after the file was
last touched, and nothing noticed: the refusal only shows up when a T2/T3 pull
request waits for its conclusion.

The test now writes the roster out and checks each Bot in it: the name must be
in `writers`, and a PASS carrying it must count. A Bot added to the team later,
or a name dropped from the policy, turns the test red.
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Not a verdict — a triage note, so this does not stall at the queue.

The guard will refuse this pull request as written: it needs a line of its own matching Fixes #<n> (or Closes #<n>), and the body has none — the (#627) in the title is not read. chore/627-reviewer2-writer is a branch this repository owns, so the fork exemption does not apply. Today that means enqueue-guard exits 1 with "no issue reference in the pull request body: add "Fixes #"".

There is now a tracking issue for the gap this closes: #767 (a reviewer the gate does not count, silently). Adding Fixes #767 to the body is the author's edit; nothing else about the change needs to move.

@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at cec4515
Written-by: Reviewer

#627: Reviewer2 is in the policy's writers, so a report signed by it counts from the moment this lands; the roster test makes both halves of that (policy gains the name, team gains a Bot) fail loudly instead of silently refusing a PASS later.

Ran:

  • TZ=UTC node --test .github/scripts/review-gate.test.mjs → 39/39 pass (tip cec4515)
  • the same, with "Reviewer2" deleted from .github/agent/review-policy.json → 38/39, red only on the policy lets every Bot on the team write a report, which names it: these Bots would have every PASS refused: Reviewer2
  • the same, with 'Reviewer3' added to the test's team roster and not to the policy → 38/39, red only on that same test, naming Reviewer3
  • TZ=UTC node .github/scripts/changelog.mjs check → fragments ok

Restored after each cut → 39/39.

The enumerated roster is the right shape for this file: the gate fails closed on an unlisted name, so a Bot added to the team without a policy line has no symptom until its first real PASS is refused, and this test turns that into a red run at the PR that adds the Bot. Nothing outside the policy and its test needs touching — docs/autonomous-dev.md and AGENTS.md describe writers without listing names, so they stay correct.

One thing to keep in mind once this is on main: with both reviewers able to write a verdict, "a pull request belongs to one of them per round" is now the only thing preventing two conclusions at one tip; the gates accepted that before only because a second writer's report was refused, and they no longer will.

@github-actions github-actions Bot added the pass label Oct 22, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
@Birfy
Birfy removed this pull request from the merge queue due to a manual request Oct 22, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 22, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit f3ee2c8 Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the chore/627-reviewer2-writer branch October 22, 2026 11:52
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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