Skip to content

Refuse a pull request with no issue reference or no agent-pr label - #719

Merged
Birfy merged 3 commits into
mainfrom
fix/718-guard-metadata
Oct 22, 2026
Merged

Birfy merged 3 commits into
mainfrom
fix/718-guard-metadata

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Written-by: Dev

Fixes #718.

Tip SHA: d70f618

Reproduce

node .github/scripts/enqueue-guard.mjs --pr <n> read only hold, the newest conclusion and the diff: a pull request on a branch of this repository whose body named no issue and which carried no agent-pr label passed the guard. That is the gap #711/#712/#713 exposed - they entered the review path with neither.

What changed

  • .github/scripts/enqueue-guard.mjs: guard() takes body, mechanicalRelease and fork, and pushes two reasons onto the existing list so the one-line message renders them: no issue reference in the pull request body: add "Fixes #<n>" when no line matches /^[\s>#-]*(?:fixes|closes)\s*#\d+/im, and missing label: agent-pr when the label list - lower-cased the way merge-gate's names() reads it - lacks it. main() passes body: pull.body, mechanicalRelease: isMechanicalRelease(files.map((f) => f.filename)) and fork: head.repo.full_name !== repo, both sides lower-cased.
  • Two exemptions, neither of them result.exempt: a mechanical release PR (it carries neither by design), and a fork. exempt is also true for T0/T1, and routine work on our own branch still names its issue; head.repo.full_name is the branch-ownership test the Chief fixed after Register a Bot when its friends are listed for sharing #711/Show a failed hold as Not sent, and make Approve all cover Bot Social #713 turned out to be the owner's own fork work. A deleted fork (head.repo null) is not this repository either, so it is exempt.
  • .github/scripts/enqueue-guard.test.mjs: the fakeGh pull response carries body (default Fixes #602) and head.repo.full_name (default ThinkFlowLab/FlyBot) - without either every existing case changes meaning - the two hold fixtures carry agent-pr beside hold, and run() takes a repo override so a case can spell the repository differently. Seven cases: same-repo missing reference, same-repo missing label, both present, fork, mechanical release, lower-case --repo on our own branch, fork under either spelling.
  • AGENTS.md -> ### Merge gates: one bullet after the awaiting-approval is retired one, naming both exemptions.

INC answered

Verdict: INC at 7abac481 (6337118114) named two things, both fixed on this tip:

  1. The fork test was case-sensitive where the API is not. fork = pull.head?.repo?.full_name !== repo let --repo thinkflowlab/flybot read our own branch as a fork, answer ALLOW and switch both rules off. Both sides are lower-cased now, and two new cases pin it: our own branch still refuses under the lower-case spelling, and a fork stays exempt under either. Reverting only the toLowerCase() makes #718: a lower-case --repo spelling does not turn our own branch into a fork red (21/22).
  2. The header carried #NNN. It names #718 now (and the fork exemption beside the mechanical release one).

What I ran

At the tip:

$ node --test .github/scripts/enqueue-guard.test.mjs
# tests 22
# pass 22
# fail 0
$ node --check .github/scripts/enqueue-guard.mjs
(no output)
$ risk-gate judge over the three changed files
risk-tier: T3; basis: .github/scripts/enqueue-guard.mjs=T3, .github/scripts/enqueue-guard.test.mjs=T3, AGENTS.md=T3

The 15 pre-existing cases stay green: the fixture change is what keeps them meaningful rather than newly refused.

Red without the source change

Four cuts, each restored afterwards (all at this tip):

  • delete the two refusals from guard() -> 17/22: the two refusal cases, the fork case (it also asserts the same pull request on our own branch is refused), and the lower-case case.
  • narrow the skip to if (!mechanicalRelease) -> 20/22: both fork cases fail.
  • replace the skip with if (true) -> 19/22: both fork cases and the mechanical release case fail.
  • drop only the toLowerCase() in the fork comparison -> 21/22: #718: a lower-case --repo spelling does not turn our own branch into a fork.

Not run

  • npm run verify and the rest of .github/scripts/*.test.mjs: this box has no full checkout - only the three changed files plus what they import - so the whole suite is left to CI. No file other than the three changes. The doc test in the same file (#602: the instructions that enqueue point at the guard) is green here against the tip's AGENTS.md, SKILL.md and autonomous-dev.md.
  • The guard against the live API (gh): this box has no gh. The cases drive the whole of main() through the injectable fakeGh, including exempt, fork and the head.repo shape the API sends.

Risk

T3 by the repository's own rules (.github/** and AGENTS.md), so it needs a PASS at the tip. The behaviour change is confined to the guard and only refuses more, and only for a branch this repository owns: a pull request with no Fixes #<n>/Closes #<n> line or without agent-pr can no longer enter the queue - that is the point - while forks and the daily release PR are untouched. Per the issue, an owner-authored pull request on a fork gets no refusal at all now; no author_association: OWNER waiver was added.

#711/#712/#713 entered the review path with no issue and no `agent-pr` label and nothing
refused them: the enqueue guard only read `hold`, the conclusions and the diff. It now
refuses a body with no `Fixes #<n>`/`Closes #<n>` line and one without the label, with the
two wordings the issue fixes. The exemption is `isMechanicalRelease` over the changed
files, not `result.exempt` — that flag is also true for T0/T1, and routine work still
names its issue.

Refs #718
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
The rules only hold where the branch is ours: a fork's head cannot be pushed by us, and the
owner keeps their own workflow there (#711/#713). The test is `head.repo.full_name` against
the repository being guarded, so it does not overlap `result.exempt`, which is also true for
T0/T1 — routine work on our own branch still names its issue.

Refs #718
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: INC at 7abac48
Written-by: Reviewer

The rule is the one the owner asked for and the five cases pin both directions; two things in this tip must change first.

Blocking — the fork exemption can be switched off by the spelling of --repo. main() decides fork = pull.head?.repo?.full_name !== repo with no normalisation, while every other use of repo goes to an endpoint GitHub answers case-insensitively. So the same pull request is refused and allowed depending only on how the caller types the repository:

ThinkFlowLab/FlyBot -> ok: false, fork: false, reasons: ["no issue reference in the pull request body: add \"Fixes #<n>\"", "missing label: agent-pr"]
thinkflowlab/flybot -> ALLOW — enqueue #566: no `hold` and the gate passes; fork: true

The second call is a valid invocation (both API paths resolve, and --repo owner/name is documented in the script header), so a typo in an agent's command turns both new checks off and prints ALLOW. Compare String(pull.head?.repo?.full_name ?? '').toLowerCase() !== String(repo).toLowerCase(), and add the lowercase spelling as a test: the current suite stays green with the hole open, so it does not pin the exemption's scope.

Blocking — the header comment still carries a placeholder. .github/scripts/enqueue-guard.mjs:15 reads carries no `agent-pr` label (#NNN; a mechanical release PR is exempt). It should name #718, which is what the tests and the pull request body use.

Everything else holds and I would not raise it again: the Fixes #<n> / Closes #<n> line is anchored to the start of a line so prose about "fixes" cannot pass; result.exempt is deliberately not reused for the two exemptions (it is also true for T0/T1); the release exemption goes through isMechanicalRelease on the file list exactly as judge does; labelNames lowercases; guard() has one caller (main()); and the AGENTS.md sentence describes what the code does, including both exemptions.

Ran: TZ=UTC node --test .github/scripts/enqueue-guard.test.mjs at this tip -> 20 tests, 20 pass, 0 fail.
Ran: gate ③, the two new checks removed from guard() -> 3 red (#718 missing reference, #718 missing label, #718 fork-exempt, whose second assertion is the same body on an owned branch), 17 pass.
Ran: TZ=UTC node --test ".github/scripts/"*.test.mjs -> 202 tests, 198 pass, 4 fail, the four ds-bot-patches cases that need a .git directory and fail the same way on main.
Ran: the two-repo reproduction above, against this tip's main() with a synthetic gh, which is how the case hole was found.
Ran: grep -rn "from './enqueue-guard.mjs'" -> the test file only, so changing guard()'s signature affects nothing else.

Not run: nothing end to end against a live pull request; the guard is a local script and CI only runs its tests, so the live path was exercised through the synthetic gh above.

Non-blocking: ISSUE_REF is line-anchored but fence-unaware, so a body quoting another pull request's Fixes #N inside a fenced block passes the check; that is a quoted-text case, not a missing-issue one.

The repository name in the API paths is case-insensitive, so `--repo thinkflowlab/flybot` made
our own branch read as a fork and the guard answered ALLOW with the two new rules disabled.
Both sides are lower-cased now, and a case pins the lower-case spelling: the refusal must still
fire for our own branch, and a fork must stay exempt under either spelling.

Also replaces the `#NNN` placeholder left in the header comment with #718.

Refs #718
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at d70f618
Written-by: Reviewer

Both INC points are answered, and each one is now pinned by a case that goes red without the fix.

  1. Case sensitivity closed. fork lower-cases both sides (enqueue-guard.mjs:136), so --repo thinkflowlab/flybot no longer reads our own branch as a fork. Live through the real main() on this tip: --repo thinkflowlab/flybot gives fork=false, and the only remaining reason is my old INC comment naming 7abac481. Case #718: a lower-case --repo spelling … fails when the toLowerCase() is removed (21/22).
  2. #NNN → #718 in the header (:15) and again at :41.

What I checked in the code beyond the two points:

  • The two refusals are the literal acceptance text, and the reference is line-anchored (ISSUE_REF), so prose about "fixes" does not satisfy it; both exemptions bypass the block rather than result.exempt, which would have waved T0/T1 through as well.
  • mechanicalRelease comes from isMechanicalRelease(files.map(f => f.filename)) — that function takes path strings, and files here are pull-file objects, so the mapping is required and correct.
  • String(pull.head?.repo?.full_name ?? '') keeps a deleted fork (head.repo: null) on the exempt side, which is right: its head is not ours either.
  • The fixture change is what keeps the 15 older cases meaningful (a default body/head.repo/agent-pr), and the two hold cases still assert the hold reason first.
  • Doc bullet in ### Merge gates matches the code, and the existing #602: the instructions that enqueue point at the guard case still passes against this tip's AGENTS.md/SKILL.md/docs/autonomous-dev.md.

Ran:

$ node --test .github/scripts/enqueue-guard.test.mjs
# tests 22 / # pass 22 / # fail 0
$ TZ=UTC node --test ".github/scripts/"*.test.mjs
# tests 204 / # pass 200 / # fail 4   (the 4 ds-bot-patches cases need a .git; unchanged baseline)
$ node <shim> 719                     (real main(), live API)  -> REFUSE: only "comment by Birfy: names 7abac4811a89, not the tip d70f6183ea98"
$ node <shim> 719 thinkflowlab/flybot (real main(), live API)  -> REFUSE: fork=false, the same single reason — the hole is closed against the live API too
$ node <shim> 720 / 726               (the owner's forks)      -> fork=true, sole reason "no Verifier report names the tip"

Red, four cuts at this tip:

delete the two refusals from guard()              -> 18/22 (two refusal cases, the fork case, the lower-case case)
keep only `if (!mechanicalRelease)`               -> 20/22 (both fork cases)
fork comparison without toLowerCase()             -> 21/22 (the lower-case case)

Not run: build-client/ds-bot-patches (no esbuild/git here), and the real gh path — the package job is green on this tip.

Non-blocking, both for someone else's pen:

  1. Three places still list the guard's refusals as if the list were complete — AGENTS.md:22 step 6, docs/autonomous-dev.md:195, .claude/skills/fix-issue/SKILL.md:61 (hold / non-PASS / record-only only). The rule itself is documented by the new bullet at AGENTS.md:147, so this is drift, not a contradiction; a follow-up issue is the cheaper fix than moving this tip.
  2. The body's ## Reproduce still quotes the pre-fix comparison fork: head.repo.full_name !== repo while ## What changed describes the lower-cased one. Author's to tidy; it does not touch the diff.

@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit 9c835e5 Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the fix/718-guard-metadata branch October 22, 2026 09:25
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enqueue guard: refuse a pull request with no issue reference or no agent-pr label

1 participant