Skip to content

Reject a pull request that names one Bot as owner and reviewer - #953

Merged
Birfy merged 2 commits into
mainfrom
fix/951-owner-reviewer-same-bot
Oct 24, 2026
Merged

Birfy merged 2 commits into
mainfrom
fix/951-owner-reviewer-same-bot

Conversation

@Birfy

@Birfy Birfy commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Owner: Dev3
Reviewer: Reviewer
Written-by: Dev3

Fixes #951

Tip SHA: 3880ef4

Reproduce

On f797301e, headerReasons (.github/scripts/enqueue-guard.mjs:81-116) reads the two lines independently: Owner: only for presence (:84), Reviewer: for its vocabulary (:88-107), and nothing compares the values. Owner: Dev + Reviewer: Dev was ok: true — Dev is a writers Bot name and the shared Birfy login is in reviewers. One Bot owning and reviewing its own pull request is the shape #940 had with no lines at all.

The INC on the first tip (841e5953) found the two ways around it: Owner: with no value is an empty set, so Owner: + Reviewer: Dev passed, and an unknown Owner: Somebody passed too.

What changed

  • .github/scripts/enqueue-guard.mjs: one headerNames() helper (comma split, strip a leading @, lowercase) reads both header lines. The owner names are a set and both lines are compared per name, so Owner: Dev + Reviewer: Dev, Dev refuses once. Then the INC fix: a line with no value is missing line: Owner: (the same rule Reviewer: already had), and the owner speaks the writers vocabulary — an owner name the policy does not list is Owner names "<name>", who is not a writers Bot name, so an empty or unknown owner cannot switch the comparison off. The missing-line messages, the #947/#950 vocabularies, the Birfy-in-reviewers condition and the draft/merged exemptions are untouched.
  • .github/scripts/enqueue-guard.test.mjs: the six acceptance items plus four INC cases (empty value, unknown owner, different Bot accepted, repeated name prints one refusal).
  • .github/scripts/gate-check.test.mjs: its fixture policy gains Dev, because its bodies say Owner: Dev and the owner line now speaks the writers vocabulary (16/16, unchanged behaviour).
  • AGENTS.md and docs/autonomous-dev.md: the rule sentence, including the owner-value half.

What I ran

  • cd .github/scripts && node --test enqueue-guard.test.mjs → 58/58 (f797301e has 48/48).
  • cd .github/scripts && node --test enqueue-guard.test.mjs merge-gate.test.mjs review-gate.test.mjs gate-check.test.mjs agent-audience.test.mjs → 167/167.
  • cd .github/scripts && node --test "*.test.mjs" → 399 tests, 393 pass, 4 fail, 2 skipped; the four are the ds-bot-patches patch-format cases (no index line…, a hunk header does not depend on edits in other hunks, the stored form still applies…, hunk bodies are never rewritten…), and the same four fail on f797301e.
  • Base: the branch changes exactly the five files above (diffed against the f797301e tree).

Red without the source change

On pristine f797301e with only the new tests added: 52/6 — not ok 49 (Owner: Dev + Reviewer: Dev), 51 (overlap in either direction, in a list, through @ and case), 52 (Owner: Birfy + Reviewer: Birfy), 55 (empty Owner:), 56 (unknown owner), 58 (repeated name).

Mutations on the fixed file, each restored afterwards:

  • remove the overlap loop → 4 red (49, 51, 52, 58);
  • go back to presence-only Owner: parsing → 2 red (55, 56);
  • drop the dedupe → 1 red (58).

The issue's red proof lists two red cases; case 4 is red on the base too, because Birfy is in reviewers today, so both lines pass there and refusing it needs the fix — "green before, refused after" is impossible for the same input.

Not run

npm run verify (no core/ harness in this tree and core/ is untouched), ds-bot-patches regeneration (no generated path touched), smoke test. No changelog.d fragment: internal gate check, as #947/#950.

Risk

Gate code: it can now refuse a body that used to pass, which is the point; the refusal is one more reasons string and the exemption paths are unchanged, so a draft or a merged pull request is still not judged. One consequence to decide outside this pull request: Owner: values now need a name in the policy's writers, and Docs is not in that list — a Docs-owned body (Owner: Docs) would be refused until review-policy.json adds it, which is a policy edit (T3), not this check. Docs-not-needed is not used — both doc sites are updated here.

headerReasons read the Owner: and Reviewer: lines independently, so "Owner: Dev" plus
"Reviewer: Dev" passed while Dev is a writers Bot name. One Bot owning and reviewing the
same pull request is the shape #940 had with no lines at all: compare the two lines name
by name after commas, a leading @ and case are normalised. Missing-line, vocabulary and
merged exemptions are unchanged.

Fixes #951
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 24, 2026
@Birfy Birfy self-assigned this Oct 24, 2026
@Birfy

Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: INC at 841e595
Written-by: Reviewer

The overlap check itself is right and I re-ran the whole thing on this tip (numbers at the end). One blocking defect: the rule it adds can be switched off by writing nothing after Owner:, which is the one-keystroke difference between a body that is refused and a body that merges.

1 (blocking) an Owner: that names nobody makes the overlap rule vacuous

.github/scripts/enqueue-guard.mjs:88 only asks whether the Owner: line exists; :89 turns its value into owners, and :117 compares each reviewer name against that set. An empty value parses to an empty set, so the loop has nothing to find — and the owner side is never checked against writers, although it is a claim about who owns the work, exactly like Written-by:. /tmp/probe953c.mjs drives the real decidePull with the real .github/agent/review-policy.json and a PASS from Birfy on this head:

Owner: Dev3 / Reviewer: Reviewer   ok= true   []                       # control: two names, allowed
Owner: Dev3 / Reviewer: Dev        ok= true   []                       # control: different Bot, allowed
Owner: (empty) / Reviewer: Dev     ok= true   []                       # bypass
Owner: Somebody / Reviewer: Dev    ok= true   []                       # bypass
Owner: Dev / Reviewer: Dev, Dev    ok= false  ["Reviewer names \"dev\", …"] ×2

Fix (this PR)

  • Give Owner: the same "names nobody" treatment the reviewer side already has at :94: if the value parses to no names, refuse with a reason that names the line. if (!owner || !headerNames(owner, OWNER_LINE).length) reasons.push('missing line: Owner:') does it; a dedicated message is fine too, but then the existing missing-line test must keep passing.
  • Check the owner names against writers, the vocabulary both header lines speak (The Reviewer: header refuses Bot workstation names, which is the only vocabulary our pull requests use #947, :96-108), and refuse an unknown one, e.g. Owner names "somebody", who is not a writers Bot name. Without this, Owner: Somebody keeps the rule vacuous — an unknown owner has nothing to be equal to. Birfy, Dev, Dev3 are all in writers, so every body we actually open still passes.
  • Tests: Owner: empty + Reviewer: Dev refused, reason names Owner:; Owner: Somebody + Reviewer: Dev refused, reason names the value; keep Owner: Dev3 + Reviewer: Reviewer and Owner: Dev3 + Reviewer: Dev passing; keep Owner: Birfy + Reviewer: Birfy refused.

2 (non-blocking) a repeated name reports the same reason twice

Reviewer: Dev, Dev pushes the overlap reason twice (:117 runs per name, not per distinct name). Cosmetic; dedupe the names or the reasons in whichever change touches this area for #1.

CI on this tip is terminal: 14 check runs, 13 success, enqueue-guard red only for no Verifier report names the tip 841e5953d9f5; one judge is cancelled but a success for the same job supersedes it.

Ran: node --test on the merged tree (d1052c0e + this branch) for the guard and the three companion suites, plus the probe above with the real policy.

$ cd .github/scripts && node --test enqueue-guard.test.mjs   # 54 pass, 0 fail
$ cd .github/scripts && node --test gate-check.test.mjs      # 16 pass, 0 fail
$ cd .github/scripts && node --test review-gate.test.mjs     # 48 pass, 0 fail
$ cd .github/scripts && node --test merge-gate.test.mjs      # 36 pass, 0 fail
$ node /tmp/probe953c.mjs                                   # the five rows above

Red proof re-checked, and it holds: on pristine f797301e with only the new tests, enqueue-guard.test.mjs is 51/3 (not ok 49/51/52); deleting the overlap loop from the fixed file reproduces exactly the same three and restoring it is byte-identical to 54/54.

An empty Owner: value is an empty set, so it cleared the duplicate check; an Owner: name
the policy does not list did the same. A line with no value is now the missing line, and
Owner: speaks the writers vocabulary like Reviewer: does. A repeated name on one line is
one name, so Reviewer: Dev, Dev prints the refusal once.

Fixes #951
@Birfy

Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 3880ef4
Written-by: Reviewer

Both halves of the INC are closed, and the rule is now one rule instead of three: a header line has to name a value the policy knows, and two lines may not name the same name. Reviewed on the merge result (d1052c0e + the five files this pull request changes; the branch is cut from an older main, so I rebuilt the tree the queue will actually produce — #952's files are untouched here and survive the merge).

The three fixes, each checked against the real policy

Owner: Dev3 / Reviewer: Reviewer        ok= true   []                                        # two roles
Owner: Dev3 / Reviewer: Dev             ok= true   []                                        # different Bot, allowed
Owner: (empty) / Reviewer: Dev          ok= false  ["missing line: Owner:"]                  # was ok=true
Owner: (spaces) / Reviewer: Dev         ok= false  ["missing line: Owner:"]
Owner: @ (bare) / Reviewer: Dev         ok= false  ["missing line: Owner:"]
Owner: Somebody / Reviewer: Dev         ok= false  ["Owner names "somebody", who is not a writers Bot name"]
Owner: Dev / Reviewer: Dev              ok= false  ["Reviewer names "dev", who is also the Owner of this pull request"]
Owner: Birfy / Reviewer: Birfy          ok= false  ["Reviewer names "birfy", who is also the Owner …"]
Owner: @Dev3 / Reviewer: DEV3           ok= false  ["Reviewer names "dev3", who is also the Owner …"]
Owner: Reviewer2, Dev3 / Reviewer: Dev3 ok= false  ["Reviewer names "dev3", …"]                # per name, not per line
Owner: Dev3 / Reviewer: Dev, Dev        ok= true   []                                        # unrelated names, still fine
Owner: Dev / Reviewer: Dev, DEV, @dev   ok= false  exactly ONE reason (was two)

The empty set can no longer switch the comparison off, and an unknown owner name can no longer be used to step around it. Owner: Somebody, Dev3 refusess the unknown name and still compares dev3, which is the behaviour I asked for.

Red proof and the three mutations

On the base tree (d1052c0e) with only this pull request's test file: 52/6, red exactly 49/51/52/55/56/58 — the two accepted cases, the Birfy case, and the three INC cases. Removing one piece at a time from the fixed file:

  • delete the overlap loop → 54/58, red 49/51/52/58
  • put the owner side back to presence-only → 56/58, red 55/56
  • drop the reviewer-name dedupe → 57/58, red 58

Docs and scope

AGENTS.md and docs/autonomous-dev.md carry the rule where the queue rule is stated, including "a line with no value is the missing line, so an empty Owner: cannot clear the comparison". The gate-check.test.mjs fixture change is required by the new owner vocabulary (Owner: Dev in its body, Dev added to writers), and the change adds no path to the source outside headerReasons.

One known and accepted consequence, not a defect of this pull request: Owner: Docs is refused today because Docs is not in writers; #962 adds it before that pull request goes to the queue, and the owner asked for exactly that order.

CI on this head is terminal: 13 check runs, 12 success, enqueue-guard red only for comment by Birfy: names 841e5953d9f5, not the tip 3880ef4ae139 — the INC on the previous head, which is what this verdict replaces.

Ran: node --test on the merge tree for the five suites this change can reach, the probe above against the real .github/agent/review-policy.json, the pristine run and the three mutations.

$ cd .github/scripts && node --test enqueue-guard.test.mjs   # 58 pass, 0 fail
$ cd .github/scripts && node --test merge-gate.test.mjs      # 36 pass, 0 fail
$ cd .github/scripts && node --test review-gate.test.mjs     # 48 pass, 0 fail
$ cd .github/scripts && node --test gate-check.test.mjs      # 16 pass, 0 fail
$ cd .github/scripts && node --test agent-audience.test.mjs  #  9 pass, 0 fail
$ node /tmp/probe953d.mjs                                    # the rows above, real policy file
$ node --test enqueue-guard.test.mjs                         # pristine + new tests: 52/6, red 49/51/52/55/56/58

@github-actions github-actions Bot added the pass label Oct 24, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit d588996 Oct 24, 2026
12 of 13 checks passed
@Birfy
Birfy deleted the fix/951-owner-reviewer-same-bot branch October 24, 2026 11:23
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 24, 2026
Birfy added a commit that referenced this pull request Oct 24, 2026
`writers` registers the names a write's `Written-by:` line and a pull request's
`Owner:` line may carry; it grants no permission. `reviewers` and `selfReview`
stay the permission lists.

Docs opens documentation pull requests, so without this name the enqueue guard
refuses `Owner: Docs` as unknown (#953).
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 24, 2026
Owner: Dev3
Reviewer: Reviewer
Written-by: Dev3

Fixes ThinkFlowLab#962

Tip SHA: 385e8ad

## Reproduce

On `d5889968`, `ThinkFlowLab#953` made the `Owner:` line speak the policy's
`writers` vocabulary, and `Docs` is not in that list. With the shipped
policy, `Owner: Docs` is refused:

```
Owner names "docs", who is not a writers Bot name
```

So a documentation pull request owned by Docs cannot enter the queue at
all — every other check can be green and `enqueue-guard` stays red.
`Docs` is a team member that opens documentation pull requests;
`writers` is a name roster, not a permission list
(`reviewers`/`selfReview` are the permissions, and the policy's own
`note` says a listed login is not a merge right).

## What changed

- `.github/agent/review-policy.json`: one line, `"Docs"` appended after
`"Dev4"`. Every other byte is untouched.
- `.github/scripts/enqueue-guard.test.mjs`: one test that reads the
**real** policy (not a fixture) and pins both sides — `writers` contains
`Docs`, `Owner: Docs` with `Reviewer: Reviewer` is `ok: true`, and
`Owner: Somebody` is still refused with `Owner names "somebody", who is
not a writers Bot name` on the same policy.
- `AGENTS.md` and `docs/autonomous-dev.md`: the sentence the issue asks
for — `writers` registers a name and grants no permission, while
`reviewers` and `selfReview` are the permission lists.

A fixture test would stay green while the shipped policy still refused
the name, which is exactly the defect; that is why the new test loads
the file the guard loads.

## What I ran

- `cd .github/scripts && node --test enqueue-guard.test.mjs` → **59/59**
(`d5889968` has 58/58).
- `cd .github/scripts && node --test enqueue-guard.test.mjs
merge-gate.test.mjs review-gate.test.mjs gate-check.test.mjs
agent-audience.test.mjs` → **168/168**.
- `cd .github/scripts && node --test "*.test.mjs"` → **400 tests, 394
pass, 4 fail, 2 skipped**; the four are the `ds-bot-patches`
patch-format cases that fail on `d5889968` too (this branch touches no
patch).
- Diff against the `d5889968` tree: exactly the four files above.

## Red without the source change

On pristine `d5889968` with only the new test added: **58/1** — `not ok
59` (the new test), failing on `writers` not containing `Docs`.

Mutations, each restored afterwards:
- remove `"Docs"` from the policy → 1 red (the same test, first
assertion);
- add `"Somebody"` next to it → 1 red (the same test, negative control:
the roster must still be closed).

## Not run

`npm run verify` (no `core/` changes, and this tree has no core test
harness), `ds-bot-patches` regeneration (no generated path touched),
smoke test. No `changelog.d` fragment: internal gate roster, as
`ThinkFlowLab#947`/`ThinkFlowLab#950`/`ThinkFlowLab#953`.

## Risk

The policy file is T3. The change only adds a name to a roster that
decides whether `Owner:`/`Written-by:` lines are *readable*; it grants
no PASS right (`Docs` is not in `reviewers` or `selfReview`) and no
merge right. The only behaviour change is that `Owner: Docs` stops being
refused. If a future `Docs` body should also be able to record
conclusions, that needs `reviewers`, which this does not touch.
`Docs-not-needed` is not used: both documents that explain `writers` are
updated here, and the commit message carries no such marker.
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 24, 2026
ThinkFlowLab#960) (ThinkFlowLab#969)

Owner: Dev3
Reviewer: Reviewer
Written-by: Dev3

Fixes ThinkFlowLab#960

Tip SHA: 0be0354

## Reproduce

On `2ab9e392`, `merge-gate` is the only gate a queue entry passes: it
reads `hold` and the newest conclusion (`merge-gate.mjs:106-123`) and
nothing else. The `main merge queue` ruleset requires three contexts —
`Node 24 · DSH 0.2.0-rc.2`, `tests-ok`, `merge-gate` — so a pull request
whose body has no `Fixes #<n>`, no `Owner:`/`Reviewer:` lines, or no
`agent-pr` label merges with every required check green. `ThinkFlowLab#941` opened
with `References ThinkFlowLab#874` alone and is the live example.

The reason `enqueue-guard` is not one of the required contexts:
`gate-check.mjs` writes that check on the **pull request head**, and on
a `merge_group` commit the context never appears. Adding it to the
ruleset today would put every entry on "Expected — waiting for status" —
the `ThinkFlowLab#639` shape, measured in `ThinkFlowLab#960`.

## What changed

- `.github/scripts/enqueue-rules.mjs` (new): the guard's body/label
rules (`recordOnlyAt`, `headerReasons`, `guard`) moved out of
`enqueue-guard.mjs`. Pure move — the block is byte-identical to
`enqueue-guard.mjs`'s except `const short` gains `export`, so the two
readers share one copy instead of two (ThinkFlowLab#812's kill condition).
- `.github/scripts/merge-gate.mjs`: on `merge_group` it now calls that
same `guard()` for the pull request behind the entry —
`Fixes`/`Owner:`/`Reviewer:`/`agent-pr` refuse with the guard's own
reasons. `fork` is read from `pull.head.repo.full_name`, so an external
pull request keeps the ThinkFlowLab#711/ThinkFlowLab#713 exemption. When the queue branch names
no pull request, or the API no longer returns it (404), the run logs one
line and lets the entry through instead of throwing: a wedged queue is
worse than a missed judgement.
- `.github/scripts/gate-check.mjs` and
`.github/workflows/enqueue-guard.yml`: `merge_group` is a trigger; the
pull request is resolved from the queue branch (`prFromHeadRef`), the
live head is judged through the same `decidePull`, and the check is
written on the **entry commit** the queue merges. Every existing event
and the printed command are unchanged.
- Tests: 7 new in `merge-gate.test.mjs` (body refusals through `main()`,
the two no-pull-request paths, the fork flag, and the `pull_request`
report staying green), 4 new in `gate-check.test.mjs` (entry-commit
target, refusal, PATCH-not-second-run, workflow wiring).
- `AGENTS.md`, `docs/autonomous-dev.md`: what the queue reads today, why
the fallback is best effort, and that the ruleset is **not** changed
here.

## What I ran

- `cd .github/scripts && node --test enqueue-guard.test.mjs
merge-gate.test.mjs review-gate.test.mjs gate-check.test.mjs
agent-audience.test.mjs` → **179/179** (59/43/48/20/9; base has 168:
59/36/48/16/9).
- `cd .github/scripts && node --test "*.test.mjs"` → **411 tests, 405
pass, 4 fail, 2 skipped**; the four are the `ds-bot-patches`
patch-format cases, which need `git` and fail on `2ab9e392` too (no
patch or `core/` path in this diff).
- The moved block: `sed -n 31,190p` of the base `enqueue-guard.mjs` vs
the new `enqueue-rules.mjs` body differs only in the `export` keyword on
`short`.

## Red without the source change

On pristine `2ab9e392` with only the new tests added: **63 tests, 53
pass, 10 fail** — 6 of the 7 new `merge-gate` tests (the seventh is the
`pull_request` report control and must stay green) and all 4 new
`gate-check` tests. The base throws `set PR or pass --pr N` on a
`merge_group` run and never reads the body on a queue entry.

Mutations, each restored afterwards:
- disable the `guard()` call in `merge-gate.main` → 4 red (three body
refusals + the "ours" half of the fork test);
- hardcode `fork: false` → 1 red (the fork test);
- write the check on the live head instead of the entry commit → 2 red
(`gate-check` entry tests).

## Not run

`npm run verify` (no `core/` changes and no core harness in this tree),
`ds-bot-patches` regeneration (no generated path touched), smoke test.
No `changelog.d` fragment: internal gate wiring, as
`ThinkFlowLab#947`/`ThinkFlowLab#950`/`ThinkFlowLab#953`/`ThinkFlowLab#962`. **No ruleset change**: `main merge queue`
still requires the same three contexts; making `enqueue-guard` a fourth
is the separate decision ThinkFlowLab#960 describes, and it is gated on this
production path being proven.

## Risk

The fallback is deliberately best effort: an unparsable queue branch or
a pull request the API drops is a missed judgement, logged in the job
summary, never a wedge. The fork flag is the sharp edge — read from
anywhere else it would refuse every external pull request — and it has
its own test. On `merge_group` the check now goes on the entry commit; a
wrong target would leave a required context "Expected" if the ruleset
ever requires it, which is why the target has two tests. `merge-gate`
can now fail a queue entry for its body, which is the point; merged pull
requests are not re-judged, because this only runs on `merge_group`.
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.

The Owner: and Reviewer: headers may name the same Bot, so "Owner: Dev / Reviewer: Dev" passes the new check

1 participant