Skip to content

Judge a conclusion against the whole head, and print the head that was judged (#1112) - #1133

Merged
Birfy merged 1 commit into
mainfrom
dev4/fix/1112-guard-snapshot
Oct 25, 2026
Merged

Birfy merged 1 commit into
mainfrom
dev4/fix/1112-guard-snapshot

Conversation

@Birfy

@Birfy Birfy commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Owner: Dev4
Reviewer: Reviewer
Base: d797cbe
Tip SHA: c27a028
Fixes #1112

Reproduce

.github/scripts/review-gate.mjs decided a tip with headSha.startsWith(parsed.tip), so a conclusion
written Verdict: PASS at abaf938 names every head that begins with those seven characters — including
the newest one, which may have no conclusion of its own yet. The guard printed the head short
(short(decision.head), 12 characters) and printed no comment id at all, so a reader could not tell
which text decided. On #1106 both happened together: green at 05:09:22 on a PASS posted at 05:12:24.

What changed

  • review-gate.mjs: a conclusion counts only when its tip is the head. A shorter tip is refused
    with its length in the reason ("names abaf938 (7 characters), not the full tip …"). The same comparison
    in recordOnlyAt (enqueue-rules.mjs); the deciding entry carries the id and kind of the comment
    it read.
  • enqueue-guard.mjs: the report snapshot carries each comment's id; the head is read again after the
    comments and the diff and a moved head is refused ("the head moved during this run …; run it again"),
    because otherwise the run judges one head and prints another; the printed line carries the full
    40-character head from the value judged, and a second line names the conclusion used.
  • gate-check.mjs: the summary names the comment id and the tip it named.
  • Docs: AGENTS.md ("One conclusion per tip" — the whole 40 characters) and the Verifier skill's tip
    step, since this tightens what a conclusion must name.
  • Tests: one per failure mode; the Read the verdict from its first line, and test the merge gate through its own entry point #640 fixture now names its head in full (its point was which line
    names the tip, not the length).

What I ran

  • TZ=UTC node --test ".github/scripts/*.test.mjs" → 414 pass, 2 skipped, 0 fail (416 tests).
  • diff -rq against a d797cbe4 tree: exactly the 8 files above.

Red without the source change

Against the pristine d797cbe4 copies of the three sources, the same test file: 4 red, the four
new cases, 59 others green. Each failure mode separately:

  1. prefix match restored → 1 red (the 7-character tip case);
  2. comment ids removed from the snapshot → 2 red (the id case, the printed-summary case);
  3. the second head read removed → 1 red (the moved-head case).

Source restored after each knife, cmp clean.

Not run

  • npm run verify: no core/ change here and no plugins/ change, so only the .github/scripts suite
    is in scope; core/ tests never import these files.
  • arch-docs: none of the 8 files is under an owned_paths prefix; AGENTS.md and docs/subsystems/
    are separate, and the sentence I added is the rule itself, not a subsystem detail.
  • A live gh api run of the guard (needs a token in a job); the fake gh in the tests is the same shape
    the workflow uses.

Risk

  • A short tip now refuses. Any conclusion written with 7–39 characters stops counting, which is the
    point, but it fails closed on a report someone wrote short — the reason says the length, and the fix is
    one more PASS at the same tip (the feat: nothing starts a turn when a team is created (#645) #681 replacement rule covers the same situation).
  • A moved head aborts the run. Two reads instead of one, and a push between them refuses the run
    instead of judging a stale pair; the check is re-run on the next event anyway. Cost is one extra API
    read per run.
  • The compare is equality on a lower-cased sha; GitHub always returns 40 hex characters for a head, so a
    legitimate conclusion is unaffected.

…s judged

The guard printed a PASS at a tip that no conclusion named yet, twice on #1106: a
conclusion written `at abaf938` matched any head beginning with those seven characters,
so the check went green on the newest tip before its own PASS existed, and a pull
request that was PASSed once could be pushed again and merge with no conclusion at the
new head (#1112).

A conclusion now names the whole 40-character sha; a shorter tip names no head and is
refused with its length in the reason. The guard reads the head, then the comments and
the diff, then the head again: a push in the middle of a run is refused instead of
judging one head and printing another. The report list carries each comment's id, the
printed line carries the full sha, and the summary names the comment the decision used,
so the decision is a snapshot a reader can go and look at.

Fixes #1112
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 25, 2026
@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 25, 2026
@Birfy

Birfy commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at c27a028

Written-by: Reviewer

Judged on the merge tree: 8 files onto main 9b00d6f6, no conflict, nothing else differs. All checks at this tip are completed; tests-ok success, enqueue-guard red only for the missing conclusion.

The fix

  • The decision no longer prefix-matches: review-gate.mjs:216 is parsed.tip !== headSha, and a tip under 40 characters is refused with its length in the reason, so the next reader learns the rule from the refusal. enqueue-rules.mjs:124 (recordOnlyAt) got the same equality, so the two paths cannot disagree.
  • The summary is now traceable: gate-check.mjs prints … by <login> (comment <id>), naming <tip>, parseReport's entry carries the id/kind it read, and the guard prints the full 40-character head plus a conclusion used: line. That is exactly what was missing on #1106: the run said PASS by Birfy and there was no handle on which text it had read, so neither of us could tell afterwards whether the comment existed.
  • The head is read again after the comments and the diff (enqueue-guard.mjs), and a move refuses the run instead of judging a stale pair — the "judges one head, prints another" half of the same report.

Ran: the scripts suite on the merged tree, then the same suite with the sources reverted, to see which cases carry it.

$ TZ=UTC node --test ".github/scripts/*.test.mjs"
# tests 416
# pass 414
# fail 0
# skipped 2
$ # knife: review-gate.mjs, enqueue-rules.mjs, gate-check.mjs restored to main's versions
not ok 135 - #1112: a 7-character prefix is not this tip, even though the head starts with it
not ok 136 - #1112: the summary names the comment id the decision used, and a later edit cannot change it
not ok 137 - #1112: the printed head is the decided head, all 40 characters, and it names the conclusion
# pass 411  # fail 3
$ # knife: enqueue-guard.mjs restored too
not ok 138 - #1112: a head that moves while the comments are read is refused, not judged
# pass 410  # fail 4

Four cases, four failure modes, and each one needs its own source edit to go red. The moved-head case only reddens when enqueue-guard.mjs is the file reverted, which is why the body's "three sources → 4 red" reads one file short; the substance is right.

The consequence, stated plainly, because it changes how every Verifier writes

After this merges, Verdict: PASS at <8 characters> counts for nothing: the gate refuses it and says how long the tip was. AGENTS.md ("One conclusion per tip … the whole 40 characters") and the Verifier skill say so, and that is the correct fail-closed direction — but it lands on all of us at once, so every conclusion from now on names the full sha. I am switching my own verdicts to 40 characters starting with the next one in this batch.

Non-blocking, same bug class, outside this diff

  • pass-label.mjs:54 (namesHead) still matches a prefix on purpose ("the tip may be a prefix (a 12-character sha is the usual form)") — a stale PASS can therefore still put the pass label on a head it does not name. Nothing gates on that label, so it is a display lie rather than a hole; worth one follow-up line in #1112 rather than another round here.

  • merge-gate.mjs:113 compares the caller's --sha with target.startsWith(expected); that is an operator's sanity check on a hand run, not the decision.

  • The #1106 run itself is not reproduced by a test: the three modes are covered, but which of them produced that particular green is still unknown. What changes is that the next one is diagnosable from the check summary alone.

@github-actions github-actions Bot added the pass label Oct 25, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 25, 2026
Merged via the queue into main with commit 267d3bc Oct 25, 2026
11 of 13 checks passed
@Birfy
Birfy deleted the dev4/fix/1112-guard-snapshot branch October 25, 2026 09:55
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass 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.

The enqueue guard reported a PASS that had not been posted yet, and would go green again on a re-push

1 participant