The LLP number gate refuses an unverifiable branch instead of passing it - #943
The LLP number gate refuses an unverifiable branch instead of passing it#943philcunliffe wants to merge 2 commits into
Conversation
|
Merge-order note for whoever lands this. This PR is stacked on The caveat is the squash. This repo squashes at the final PR, so #916's squash commit will not have PR #944 is stacked the same way on #915 and carries the same caveat. |
`check` gained a third exit path in this PR: exit 2, the scan could not work out its scope, distinct from the exit 1 that says a number collides. The file's own usage banner still described only exit 1, so a wrapper or agent reading it sees a nonzero exit on an orphan or grafted checkout and reports "a number collides, renumber it" for a run where nothing needs renumbering. `.claude/skills/ref-check/SKILL.md` described the same contract the same incomplete way, so it says the distinction out loud too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round:
|
Issue #937 carries three findings deferred out of PR #916. One of them is a
defect with a behavioural surface and is fixed here. The other two are not
defects and cannot be fixed from a pull request at all; they are answered at
the bottom rather than papered over.
Finding 2 (fixed):
checkexited 0 when the branch had no common ancestor with the baseRoot cause
scripts/llp-numbers.jshad already settled the doctrine for this class offailure, one block earlier, at the partial-scan refusal:
Two blocks down,
runbroke its own rule. Whengit merge-base HEAD <base>failed,
mintedNumberscorrectly reported the answer as unknown (a nullmergeBase), andcheckthen printed that and returned 0. An empty set ofminted numbers reads identically whether the branch minted nothing or the gate
could not work out what it minted, and
runacted on the set without readingmergeBase. So a branch of unrelated history cleared a gate that had scopeditself to nothing and checked nothing.
The neighbouring
minted.base === nullarm returned 0 for the same reason. Itis unreachable from
checktoday (partialScanrefusesNO_BASE_REFbeforerungets there), but the two predicates are written out separately and wouldreopen the hole if they ever drifted.
The fix
Both arms now refuse, on the same terms as the partial-scan refusal:
Exit 2, not 1: this script already distinguishes "cannot answer" (2, used by
the usage error, the non-checkout, and the partial scan) from "answered, and a
number collides" (1). The unknown merge base is the first kind. Review round 2
endorsed the message as the honest answer, and the message is kept verbatim in
substance; what changes is that the gate no longer goes green after saying it.
The
mintedNumbersJSDoc now says out loud that callers gating on the resulthave to read
mergeBase, not just the number set.Blast radius
None in CI. On a pull request
actions/checkoutbuildsrefs/pull/N/merge,which has
origin/masteras an ancestor by construction; on a push tomaster,HEADis the base. The refusal fires where it should: a local run on an orphanor grafted history, or a job whose checkout lost the base branch.
Regression test
test/core/llp-number-minting.test.js, new testthe check refuses a branch with no common ancestor instead of passing it. Itbuilds a corpus at LLP 0100 on
master, mirrors it torefs/remotes/origin/master,then cuts an orphan branch that mints a second
0100-*. The collision is realand
collisions()sees it across the refs; the point is whatcheckdoes aboutit when it cannot compute the scope.
Before (at
f403dfd, the head #937 was filed against):After: 21 pass, 0 fail.
The pre-existing test
a merge base that cannot be found is unknown, not an empty basekept an assertion onrun(['check']) === 0. That assertion was thedefect written down, so it is removed; the test keeps what it was actually for,
that
mintedNumbersreportsmergeBase: nullwith an empty set rather thantreating the missing base as an empty one.
Full suite: 4504 pass, 0 fail, 1 skipped.
npm run typecheckclean.node scripts/llp-numbers.js checkon this branch:0 LLP numbers minted against refs/remotes/origin/master, no collision, exit 0.Finding 1 (not fixed here, and not fixable here): the live cross-branch collisions
Renumbering them means editing documents that live on other branches. A
master-based pull request cannot touch another branch's tree, and whichdocument keeps a contested number depends on intended merge order, which is a
call for the branch owners. This is not a defect in the gate; it is the backlog
the gate creates, and it needs one issue per owning branch.
The ground truth has moved since triage.
surveyon this head reports the nextfree number as 0289 (0288 at triage), and 0266 now has eight claimants,
not six:
0175, 0200 and 0265 have three claimants each; 0246, 0248, 0249, 0267 and 0268
have two. The list grew between triage and now, which is the argument for
landing #916 sooner rather than later: every day it stays open is another
branch minting from a stale denominator.
Finding 3 (not fixed here, by design): settled collisions on stale branches
Both review rounds endorsed scoping
checkto the numbers a branch mints, andthis PR does not change that scoping. The deferred work named in the issue is
corpus hygiene, not code: the pre-repair filenames for 0026, 0032, 0046, 0049
and the rest survive only because the branches carrying them were never deleted.
Deleting dead branches removes those claimants from
surveywith no codechange at all. There is no defect here to write a failing test against.
Fixes #937