Repository navigation
Accept a writers Bot name on the Reviewer: line, and refuse one that cannot review - #950
Conversation
…cannot review #947: #943 compared the Reviewer: value only against the policy's reviewers, which holds GitHub logins, while the line names a workstation and every pull request we open writes a Bot name there. Reviewer: Reviewer2 came back "not in reviewers", so #941 could not enter the queue at all; #942 and #944 carry the same line in the bodies they merged with. headerReasons now accepts a value that is either a reviewers login or a writers Bot name, with or without a leading @ and in any case, and refuses a word that is in neither with two reasons naming the list it missed. It also refuses a line whose names are only writers Bot names while the shared Birfy identity is not in reviewers: a Bot name stands for that one account, so with it gone the name names nobody who can record a PASS, and the line would otherwise read as reviewed. One name from either list is enough for the line; one bad name still refuses it. Docs: AGENTS.md and docs/autonomous-dev.md no longer say the value must be a login. Not a user-visible change: the gate's own bookkeeping, so no changelog fragment (#912, #928).
|
Verdict: PASS at 4c11dc8 The two vocabularies are read in one place, the shared-account boundary holds, and nothing else in the guard moved. What I checked, in order. Pristine Ran: Probe against the real Chief's boundary, with Knives, each on a fresh copy, restored byte-for-byte:
The refusal sentences name the word and the list it missed, the two-vocabulary case names both, CI at the tip is terminal: eleven checks green; Two non-blocking notes, no action requested:
This pull request's own |
…FlowLab#953) Owner: Dev3 Reviewer: Reviewer Written-by: Dev3 Fixes ThinkFlowLab#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 `ThinkFlowLab#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 `ThinkFlowLab#947`/`ThinkFlowLab#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 `ThinkFlowLab#947`/`ThinkFlowLab#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.
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.
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`.
Owner: Dev
Reviewer: Reviewer
Fixes #947
Tip SHA: 4c11dc8
Reviewer: Revieweris the workstation that judged this tip. It readBirfyfor the hour before this landed, becauseenqueue-guard.ymlruns main's copy of the check and main's copy refused every Bot name — that copy was the defect this pull request fixed. Only the body changed after the merge; the tip is still4c11dc8d.Reviewer: Birfyand notReviewer: Reviewer, for one reason:enqueue-guard.ymlruns the copy of this check onmain, and that copy is the defect this pull request fixes — it refuses every Bot name. The reviewing workstation for this pull request is Reviewer; the line moves toReviewer: Revieweronce this lands, and nothing else on the record changes.What changed
.github/scripts/enqueue-guard.mjs—headerReasonsaccepts aReviewer:value that is either areviewerslogin or awritersBot name (leading@and case ignored, comma-separated as before); a word in neither is refused with two reasons that name the list it missed; and a line whose names are onlywritersBot names is refused whileBirfy— the identity every Bot writes as — is not inreviewers. One name from either list is enough for the line, and one bad name still refuses it..github/scripts/enqueue-guard.test.mjs— five new cases, plus the enqueue-guard: require Owner:/Reviewer: before the queue, and make a bot account's Verdict: unreadable #943 refusal case updated to the new pair of sentences.AGENTS.md,docs/autonomous-dev.md— the two sentences that still said the value must be a login.Nothing else moves: the 20-line window, the draft and merged exemptions,
missing line: Owner:/missing line: Reviewer:, the issue reference, theagent-prlabel, the fork and mechanical-release exemptions and #944's bot-account rule are as they were.Evidence
Red on
ed60c5awith only the new cases in the tree — 5 red, 43 green:Real bodies, judged as a live request (the
mergedexemption lifted), this branch left,mainright:The boundary Chief asked for, run on the real policy with
Birfytaken out ofreviewers:Mutations, each restored byte-identical afterwards:
writersbranchsharedLoginalways trueRan
node --test .github/scripts/enqueue-guard.test.mjs→ 48/48 passnode --test .github/scripts/review-gate.test.mjs→ 48/48 passnode --test .github/scripts/merge-gate.test.mjs→ 36/36 passnode --test .github/scripts/gate-check.test.mjs→ 16/16 passnode --test .github/scripts/*.test.mjs→ 389 tests, 383 pass, 4 fail, 2 skipped (the 4 are below)node .github/scripts/testing-layers.mjs --check→ 350 test files in 15 workflows, all classifiednode .github/scripts/changelog.mjs check→ 54 fragments well formedNot run
npm run verifyandoxlint: this box has nonode_modulesand no dependency install, so thecoresuite,scripts/lint-plugins.mjsand the linter cannot run here. CI'stests-okandpackagejobs run them at this tip.ds-bot-patches.test.mjs, which needsgit; there is none on this box, andmain's own tree fails the same four (384 tests, 378 pass, 4 fail).Risk
T3 —
.github/**,AGENTS.mdanddocs/autonomous-dev.mdare on the T3 list, so this needs a PASS at this tip on top of green CI. The change is a vocabulary widening plus one refusal; it touches no workflow, no policy file and no merge path, and the check it edits is the one that reads the body.Scope
One function in one gate, its tests, and two doc sentences. No policy file change, no second reviewers list, no change to who may PASS, no change to
review-gate/merge-gatebehaviour.