Skip to content

Every [tests] @ref in the wizard pick suite carries a gloss (#613) - #624

Merged
philcunliffe merged 3 commits into
masterfrom
fix/issue-613
Aug 7, 2026
Merged

Every [tests] @ref in the wizard pick suite carries a gloss (#613)#624
philcunliffe merged 3 commits into
masterfrom
fix/issue-613

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Deferred, non-blocking finding from neutral triage of PR #609: an @ref in
test/core/cli/wizard/pick.test.js opens a gloss with a colon and then says
nothing. Triage's own note said the honest fix is a house-style sweep of the
whole file's [tests]: refs rather than a one-line patch on #609, because the
file already had the same shape in three other places.

What was wrong

Four of the file's seven annotations carried an empty gloss:

line annotation
24 @ref LLP 0129#join-before-picker [tests]:
25 @ref LLP 0031#status-provenance [tests]:
133 @ref LLP 0137#pathway-defaults [tests]:
270 @ref LLP 0132#never-silent [tests]:

LLP 0001 (Conventions adopted) spells the syntax // @ref LLP NNNN#anchor: gloss and says "a colon opens the gloss"; CLAUDE.md adds that a ref must tell
you something the code and the filename do not. A bare [tests]: tells you only
that a section exists, which the anchor already did.

What changed

All four glosses filled in, in the file's own wrapped two-line style, each
saying why the block below it is tested rather than restating the test names:

  • 0129#join-before-picker: locked is the org config the join already
    waited for, which is what makes a locked row truthful and not a guess. That is
    the LLP's own consequence line, and it is the reason this suite can feed
    locked in directly.
  • 0031#status-provenance: a locked row's "managed by your fleet" label is
    status's central/local provenance split in picker form. LLP 0129 cites
    0031#status-provenance for exactly this vocabulary, so the ref is honest about
    what it names.
  • 0137#pathway-defaults: the pick phase never prompts for a window;
    retentionDefault is the pathway's number, 90 the fall-through.
  • 0132#never-silent: on a managed machine no local addition is left
    unmarked, and the suffix appears nowhere else.

Nothing else in the file moved: no test body, assertion, import, or source
change. The three refs that already had glosses (0135#finale, both 0180#decision
refs) were left alone; they were already honest and specific.

Verification: the regression-test gate does not apply

This is a non-behavioural, comment-only change. There is no behaviour to
break and therefore no test that can fail before the change and pass after it,
so no regression test was written and none was invented. What was used instead:

/ref-check test/core/cli/wizard/pick.test.js, before and after (LLP index
built over llp/ with all three anchor forms: heading slugs, {#slug} markers,
inline <a id>):

before: 7 references. Broken 0. Warnings 0. Empty glosses 4. Clean 3.
after:  7 references. Broken 0. Warnings 0. Empty glosses 0. Clean 7.

Every anchor resolved before and after; the delta is exactly the four glosses.

Full suite: npm test - 3377 tests, 3376 pass, 0 fail, 1 skipped. That
includes test/core/llp-ref-hygiene.test.js, which mechanically resolves every
@ref in the repo against a live LLP anchor and rejects an em dash anywhere in
a gloss, including on continuation lines. Both new continuation lines are inside
its scan.

Typecheck: npm run typecheck clean.

Two notes for the reviewer

  • LLP 0001 suggests a gloss of 80 characters or less. These four wrap to two
    lines and land at 92-113 characters, matching the file's pre-existing glosses
    (83, 105, 105) and the corpus norm: 804 of the 1229 non-empty glosses in
    tracked .js files (65%) are over 80. Each individual line stays under 80.
    Cutting these to fit would have cost the part that explains anything.
  • The annotation triage actually flagged, @ref LLP 0183#seed-from-config [tests]:, arrives with hyp init reconfigure starts from the config on disk (#603) #609 and is not on master, so this branch cannot
    touch it. The diffs do not collide (hyp init reconfigure starts from the config on disk (#603) #609 only appends past the end of the
    file). Whichever lands second should give that one a gloss in the same style;
    it is a one-line follow-up, not a re-run of this sweep.

Fixes #613

Four of the file's seven `@ref` annotations opened a gloss with a colon
and then said nothing. LLP 0001 makes the gloss the point of the syntax
("a colon opens the gloss"), and CLAUDE.md asks a ref to tell you
something the code and the filename do not; a bare
`@ref LLP NNNN#anchor [tests]:` tells you only that a section exists.

Filled in all four, in the file's own wrapped two-line style, saying why
the block below each one is tested rather than restating the test names:

- 0129#join-before-picker: `locked` is the org config the join already
  waited for, which is what makes a locked row truthful.
- 0031#status-provenance: the "managed by your fleet" label is status's
  central/local provenance split in picker form (0129 cites 0031 for
  exactly this vocabulary).
- 0137#pathway-defaults: the pick phase never prompts for a window;
  `retentionDefault` is the pathway's number, 90 the fall-through.
- 0132#never-silent: on a managed machine no local addition is left
  unmarked, and the suffix appears nowhere else.

Annotation-only: no test, assertion, or source behaviour changes.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Neutral review, round 1/2: approve

Reviewed at head 1beb698 in a detached worktree off origin/fix/issue-613.
Nothing actionable found; no commits pushed.

I accept the author's verification argument. A comment-only edit has no
behaviour that can fail before and pass after, so there is no regression test to
write and none was invented. The right gate here is "does each gloss say
something true, and something the code does not already say", and that is what I
checked.

Scope

git diff origin/master...HEAD is a single file, +8 / -4, all inside //
comments: test/core/cli/wizard/pick.test.js lines 24-27, 135-136, 273-274. No
test body, assertion, import, helper, or source line moved. Confirmed
non-behavioural.

The four glosses, checked against the sections they name

Each claim was read against the LLP prose and the code below it, not just
spot-checked for non-emptiness.

  • 0129#join-before-picker (test/core/cli/wizard/pick.test.js:24) - "locked
    is the org config the join already waited for, which is what makes a locked row
    truthful and not a guess." True to the section (llp/0129-init-wizard-fork.decision.md:22,
    "enrolls, then waits for the org's config, then shows the picker") and to its
    Consequences line ("it is what makes locked-row rendering truthful rather than
    aspirational"). It earns its place: it explains why this suite may feed
    locked: [...] straight in without modelling the wait at all.
  • 0031#status-provenance (:26) - the "managed by your fleet" label as
    status's central/local provenance split in picker form. Matches
    llp/0031-layered-config.decision.md:240 (per-entry [central · locked] /
    [local] tags), and LLP 0129 itself cites this anchor for exactly that
    vocabulary. The label is real: src/core/cli/wizard/pick.js:36
    (LOCKED_LABEL_SUFFIX), asserted at test/core/cli/wizard/pick.test.js:290.
  • 0137#pathway-defaults (:135) - "never prompts for a window;
    retentionDefault is the pathway's number, 90 the fall-through." Matches
    llp/0137-onboarding-retention-defaults.decision.md:25 ("the orchestrator
    passes the local default in as retentionDefault; team and scoped runs fall
    through to DEFAULT_RETENTION_DAYS"), and the fall-through number is right:
    src/core/cli/walkthrough.js:55 is 90, consumed at
    src/core/cli/wizard/pick.js:163 as opts.retentionDefault ?? DEFAULT_RETENTION_DAYS.
  • 0132#never-silent (:273) - "no local addition is left unmarked, and the
    suffix appears nowhere else." Matches
    llp/0132-managed-local-additions-local-only.decision.md:48, and the second
    half is the honest part: it is the negative the two tests below actually prove
    (the locked row asserts doesNotMatch(/stays on this machine/); the
    solo-machine test asserts no row carries the suffix at all). That is not in the
    section banner and not in the [implements] gloss on
    src/core/cli/wizard/pick.js:73.

None of the four is a restatement of its test name. Two of them (0129, 0132)
state something I could not have recovered from the file alone.

Anchors

All four resolve. #join-before-picker, #pathway-defaults and #never-silent
are explicit <a id> markers; #status-provenance resolves via the ## Status provenance heading slug at llp/0031-layered-config.decision.md:240. This is
also verified mechanically rather than by eye: test/core/llp-ref-hygiene.test.js:339
("every @ref resolves to a live LLP document and one of its anchors") is green in
the full run below.

Conventions

  • No U+2014 anywhere in the diff or the file. Independently enforced for these
    lines by test/core/llp-ref-hygiene.test.js:389, which reads the whole gloss
    including continuation lines, so both new continuation lines are inside its
    scan.
  • No semicolons introduced; no trailing whitespace; every new line is at most 80
    characters.
  • The continuation lines attach correctly: glossLines
    (test/core/llp-ref-hygiene.test.js:238) breaks on a following @ref, so the
    0129 gloss stops at line 25 and does not swallow the 0031 annotation.
  • All 7 annotations in the file now carry a gloss (lines 24, 26, 135, 251, 273,
    374, 396). The three pre-existing ones were correctly left alone.

The #609 overlap claim

Verified. @ref LLP 0183#seed-from-config [tests]: is at line 406 of
test/core/cli/wizard/pick.test.js on origin/fix/issue-603 (PR #609) and does
not exist on master; llp/0183-* does not exist on master either. So this
branch genuinely cannot annotate it. The two diffs do not collide: #609 touches
@@ -5,9 +5,10 @@ and @@ -400,3 +401,354 @@, this PR touches lines 21-30,
130-139 and 267-277. Whichever lands second should give 0183 a gloss in the same
style. One correction to the PR body's framing, informational only: #609 is not
purely an append, it also adds an import near line 5, still disjoint from every
hunk here.

Verification run

Worktree with node_modules symlinked (a bare worktree silently runs a truncated
subset, so this matters).

  • npm test - 3377 tests, 3376 pass, 0 fail, 1 skipped, 0 todo, 15.8s.
  • npm run typecheck - clean, no output.

Both reproduce the numbers in the PR body exactly.

Residual findings

None blocking. One observation, explicitly not a change request:

  • Nit, informational, test/core/cli/wizard/pick.test.js:135. Of the four,
    the 0137 gloss carries the least that is new: "never prompts for a window /
    retentionDefault is the pathway's number" largely re-says the banner comment
    one line above it (// --- retention defaults (LLP 0137): never asked, pathway-supplied ---). The clause that earns it is "90 the fall-through",
    which is a real fact a reader would otherwise have to chase into
    src/core/cli/walkthrough.js. It clears the CLAUDE.md bar and I am not asking
    for a reword; churning a comment-only PR over it would cost more than it buys.

No follow-up issue needed.

@philcunliffe philcunliffe added neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) and removed neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) labels Aug 5, 2026
test added 2 commits August 5, 2026 21:35
# Conflicts:
#	test/core/cli/wizard/pick.test.js
…jacent prose

The gloss now states the genuinely new fact: pinning both machine kinds
matters because the suffix's absence on a solo machine is what proves
it was retired outright, not merely re-scoped to unmanaged rows.
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Neutral review round 1 - 87b786b (post-merge head)

Reviewed the merge head after neutral resolved the conflict PR #629's merge created. Verdict: clean apart from one gloss nit, which is fixed. Local: node --test test/core/cli/wizard/pick.test.js 38/38 pass, npx tsc --noEmit exit 0.

Merge integrity - confirmed independently

origin/master (7116f95) is a verified ancestor. The diff vs master touches one file, +20/-7, and filtering it for any changed line that is not a // comment returns zero results. No test body, assertion, helper, import, or fixture differs from master, so nothing #629 added or changed was dropped.

Worth calling out, because it was the one place the conflict could have produced a confidently wrong gloss: the pre-merge branch tip carried a gloss for LLP 0132#never-silent written against the old test, which asserted the · stays on this machine suffix did appear. #629 inverted that test and retargeted the ref to LLP 0188#never-silent. The resolution correctly discarded the now-false gloss and wrote a new one for the inverted semantics rather than carrying it forward.

Anchors - 10/10 resolve

0129#join-before-picker, 0031#status-provenance (heading slug), 0190#pick-gate, 0190#commit-point, 0137#pathway-defaults, 0135#finale, 0188#never-silent, 0180#decision x2, 0183#seed-from-config. All cited docs exist; none is Superseded, and 0188 correctly replaces the old 0132 ref (0132 is itself superseded by 0188).

Gloss quality

10 of 10 refs carry a gloss (7 added here, 3 pre-existing). I verified all 7 new ones against both the test body and the cited LLP section: all 7 accurate, none overclaiming. The strongest is 0190#commit-point, which names the actual failure mode ("a cancel at the sync lane cannot strand a machine whose config was already replaced") and is backed by assertions on byte-identical config content plus overwriteAsked === false.

Finding, fixed

LOW - test/core/cli/wizard/pick.test.js:399: the 0188#never-silent gloss restated the prose comment three lines above it. Lines 396-398 already said the suffix is retired and that the sync-scope step now offers local-only; the gloss repeated that in different words, adding only that both machine kinds are covered.

Fixed in 152fdca:

- the retired suffix must survive on no row of either machine kind, because a row
- still promising "stays on this machine" would contradict the default-sync the
- sync-scope step now states
+ both machine kinds are pinned because the suffix's absence on a solo machine is
+ what proves it was retired outright, not just re-scoped to unmanaged rows

Verified against both annotated tests (the managed-machine case at 403-424 and the solo case at 426-438): each asserts doesNotMatch(row.label, /stays on this machine/), so pinning the pair is what shows the suffix is gone everywhere rather than merely re-scoped. Diff is comment-only.

Not fixed - out of scope

LOW, pre-existing: 6 of the 10 refs sit above a blank line before their test(...) call (lines 25-28, 154-157, 255-256, 399-401, 484-486, 610-613). CLAUDE.md says a blank line breaks attachment, so a strict /ref-check reads these as file-scoped rather than attached. This structure is inherited verbatim from master and the PR neither introduced nor worsened it, so fixing it here would be scope creep on a fix PR. Flagged because this PR is specifically about @ref hygiene and is the natural place to consider it.

Conventions

No em dashes, no semicolons introduced, no @typedef, no inline import('...') types. The five await import(...) calls are runtime dynamic imports in test bodies, which the rule does not cover.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Triaged at 152fdca - all residuals non-blocking, PR can ship

The review fix-loop hit its 2-round cap with one finding still open. Triaged independently of the rounds themselves, since neutral authored both the review and the fix on this PR.

Scope makes this near-trivial: the diff against master is one file, +20/-7, and every changed line is a // comment. No test body, assertion, import, helper, or fixture differs. A comment-only edit to a test file produces zero bytes of difference in what node --test or the shipped package executes, so there is no production risk to weigh.

Residual, deferred to #642: six @ref [tests]: annotations sit above a blank line before their construct, which per CLAUDE.md breaks attachment, so a strict /ref-check reads them as file-scoped. Preference, not a blocker. Two things confirmed while triaging:

  • It is genuinely pre-existing. All six blank lines are present verbatim on master; this PR only appended gloss prose to the existing @ref lines.
  • The impact is narrower than the review round suggested. test/core/llp-ref-hygiene.test.js is the gate npm test actually enforces, and it checks anchor resolution, gloss non-triviality and em dashes, but not construct attachment. So nothing in CI or at runtime is affected, only interactive /ref-check output.

The fix in 152fdca re-checked and confirmed accurate. Both annotated tests (the managed-machine case at :403 and the solo case at :426) assert doesNotMatch(row.label, /stays on this machine/), so the gloss's causal claim holds: without the solo-machine test, the managed result alone would be consistent with the suffix having merely moved to unmanaged rows rather than being retired outright.

No further unresolved findings. Next tick this PR is terminal and will be held for a human to merge.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 5, 2026
@philcunliffe
philcunliffe merged commit 5a64cc9 into master Aug 7, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-613 branch August 7, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #609

1 participant