Skip to content

The LLP number gate refuses an unverifiable branch instead of passing it - #943

Closed
philcunliffe wants to merge 2 commits into
fix/issue-907from
fix/issue-937
Closed

The LLP number gate refuses an unverifiable branch instead of passing it#943
philcunliffe wants to merge 2 commits into
fix/issue-907from
fix/issue-937

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

Stacked on #916. scripts/llp-numbers.js does not exist on master yet:
PR #916 is still open, so this PR is based on fix/issue-907 and its diff is
only the change below. If GitHub retargets it to master when #916 merges,
nothing else moves.

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): check exited 0 when the branch had no common ancestor with the base

Root cause

scripts/llp-numbers.js had already settled the doctrine for this class of
failure, one block earlier, at the partial-scan refusal:

// `check` refuses rather than warns: a gate whose failure mode is a silent
// pass is the defect of issue #907 again, one layer up.

Two blocks down, run broke its own rule. When git merge-base HEAD <base>
failed, mintedNumbers correctly reported the answer as unknown (a null
mergeBase), and check then printed that and returned 0. An empty set of
minted numbers reads identically whether the branch minted nothing or the gate
could not work out what it minted, and run acted on the set without reading
mergeBase. So a branch of unrelated history cleared a gate that had scoped
itself to nothing and checked nothing.

The neighbouring minted.base === null arm returned 0 for the same reason. It
is unreachable from check today (partialScan refuses NO_BASE_REF before
run gets there), but the two predicates are written out separately and would
reopen the hole if they ever drifted.

The fix

Both arms now refuse, on the same terms as the partial-scan refusal:

if (minted.base === null || minted.mergeBase === null) {
  ...
  writeError(`${unknown}, so what this branch mints cannot be told from what it
    inherited. The check would pass without looking, so it refuses instead. ...`)
  return 2
}

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 mintedNumbers JSDoc now says out loud that callers gating on the result
have to read mergeBase, not just the number set.

Blast radius

None in CI. On a pull request actions/checkout builds refs/pull/N/merge,
which has origin/master as an ancestor by construction; on a push to master,
HEAD is the base. The refusal fires where it should: a local run on an orphan
or grafted history, or a job whose checkout lost the base branch.

Regression test

test/core/llp-number-minting.test.js, new test
the check refuses a branch with no common ancestor instead of passing it. It
builds a corpus at LLP 0100 on master, mirrors it to refs/remotes/origin/master,
then cuts an orphan branch that mints a second 0100-*. The collision is real
and collisions() sees it across the refs; the point is what check does about
it when it cannot compute the scope.

Before (at f403dfd, the head #937 was filed against):

not ok 14 - the check refuses a branch with no common ancestor instead of passing it
  error: |-
    Expected values to be strictly equal:

    0 !== 2
  expected: 2
  actual: 0
# tests 21
# pass 20
# fail 1

After: 21 pass, 0 fail.

The pre-existing test a merge base that cannot be found is unknown, not an empty base kept an assertion on run(['check']) === 0. That assertion was the
defect written down, so it is removed; the test keeps what it was actually for,
that mintedNumbers reports mergeBase: null with an empty set rather than
treating the missing base as an empty one.

Full suite: 4504 pass, 0 fail, 1 skipped. npm run typecheck clean.
node scripts/llp-numbers.js check on 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 which
document 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. survey on this head reports the next
free number as 0289 (0288 at triage), and 0266 now has eight claimants,
not six:

LLP 0266
  0266-behavioral-rows-carry-cwd.decision.md               fix/issue-878
  0266-cli-compatibility-rollover.plan.md                  codex/cli-reorg
  0266-core-command-argument-validation.decision.md        fix/issue-836, fix/issue-903
  0266-hidden-rows-stay-off-the-sync-gate.decision.md      wizard-sync-gate-and-answerless-config
  0266-native-prepared-batches-through-query-sources.md    update/icebird-squirreling-native-batches
  0266-prune-asks-every-client-not-the-run.decision.md     fix/issue-884
  0266-rearm-fires-at-both-attach-success-exits.decision.md fix/issue-887
  0266-x509-and-local-ca-residuals.decision.md             fix/issue-886

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 check to the numbers a branch mints, and
this 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 survey with no code
change at all. There is no defect here to write a failing test against.

Fixes #937

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Merge-order note for whoever lands this.

This PR is stacked on fix/issue-907 (PR #916), which is the right base while #916 is open: against master the diff would re-propose #916's work.

The caveat is the squash. This repo squashes at the final PR, so #916's squash commit will not have fix/issue-907's commits as ancestors. When #916 merges, GitHub retargets this PR to master, and at that point the branch needs a rebase onto master before it is mergeable, otherwise the diff reopens #916's changes. The reconciler's merge-base rung will surface this on a later tick; flagging it here so it is visible at the merge button.

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>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round: 05b72f5, verdict findings (2 low, 1 fixed, 1 reported as pre-existing)

Reviewed the diff of this PR against its own base fix/issue-907, not the stacked
parent's changes: two files, scripts/llp-numbers.js (the check refusal) and
test/core/llp-number-minting.test.js (the regression test).

The change itself is correct. No high or medium finding. What I verified:

  • node --test test/core/llp-number-minting.test.js: 21 pass, 0 fail, including the
    new orphan-history test. Full npm test: 4504 pass, 0 fail, 1 skipped.
  • Every caller of the script on this branch is .github/workflows/llp-check.yml:42,
    which runs check as a bare run: step. Exit 2 fails that step exactly as exit 1
    does, and nothing branches on the specific code, so the 0 to 2 change breaks no
    consumer.
  • The PR's reachability claim holds: partialScan and mintedNumbers resolve the
    base with the identical BASE_CANDIDATES + rev-parse --verify --quiet predicate,
    so the minted.base === null arm really is unreachable from check today. Keeping
    it refusing anyway is correct belt-and-braces against the two predicates drifting.
  • Neither CI path trips the new refusal: refs/pull/N/merge has the base as an
    ancestor by construction, and the workflow's explicit
    git fetch '+refs/heads/*:refs/remotes/origin/*' guarantees the base ref exists.
  • node scripts/llp-numbers.js check from the head worktree still exits 0 with a real
    answer: 0 LLP numbers minted against refs/remotes/origin/master, no collision.

Finding 1 (low, fixed in 10dbf07): the usage banner still described only exit 1

scripts/llp-numbers.js:12. This PR gives check a third exit path, exit 2 with
nothing minted and no collision, but the file's own usage banner one screen above the
code still read check exit 1 if this branch mints a taken number. A wrapper or
agent reading that banner sees a nonzero exit on an orphan or grafted checkout and
reports "a number collides, renumber it" for a run where the gate could not compute
its scope and nothing needs renumbering. That is the honest-refusal message being
undone one layer up by a stale contract statement in the same file the PR edits.

.claude/skills/ref-check/SKILL.md:116 described the same contract the same
incomplete way (check fails when the branch you are on mints a number claimed
elsewhere), so it now names the distinction too: exit 1 collides, exit 2 is a scan
that could not run.

Fixed in 10dbf07, no behaviour change, comment and prose only. Suite re-run green.

Finding 2 (low, not fixed, pre-existing): the same hole survives one layer down, via a partial clone

scripts/llp-numbers.js:449. The doctrine this hunk installs, "no scope is not an
empty scope", is not applied to its sibling case. partialScan detects a truncated
clone with rev-parse --is-shallow-repository, which is false for a partial
clone (git clone --filter=tree:0), so it returns null and check proceeds. In
such a clone without network (offline CI, or a pruned promisor remote) every
git ls-tree fails, refFilesFromGit skips each unreadable ref by design,
scanRefFiles collapses to the working tree alone, and rivals is effectively empty.
check then prints N LLP numbers minted against refs/remotes/origin/master, no collision and exits 0 having compared the branch against nothing: the same silent
pass this PR closes for a null merge base, reached by a different route.

Not introduced here, and not in this PR's diff. It predates the change and the fix
belongs in partialScan's detection (a promisor-remote or unreadable-ref probe),
which is a different defect with its own behavioural surface and its own regression
test. Left for a follow-up issue rather than widened into this PR, but it is the one
remaining hole in the invariant the new hunk's comment asserts, so it should not be
lost.

Not re-litigated

Findings 1 and 3 of issue #937, which the PR body answers rather than fixes (live
cross-branch collisions needing one issue per owning branch; settled collisions on
stale branches, which are corpus hygiene, not code). Both reasons hold, and neither
is fixable from a pull request.

Head after this round: 10dbf07285c1abc8382d670f0005bb2d8fc6bac7.

@philcunliffe
philcunliffe deleted the branch fix/issue-907 August 19, 2026 18:55
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.

1 participant