Smoke JSDoc names disjointness, not coverage, as what the loaded-only cut rests on (#761)

[philcunliffe]

Finding

Issue #761 had two halves. The second (the missing disjointness test) landed on
master in PR #767; that PR carried no closing trailer, so the issue stayed
open. The first half was never fixed and is still on master: the JSDoc on
composedRiders in the walkthrough smoke justifies its loaded-only read with a
premise that does no work.

The stale text claimed the read agrees with ridersInDefaultSet "because the
allowlist and the excluded set together cover every bundled plugin". Coverage is
not what the agreement rests on, in either direction:

• Coverage can fail without breaking agreement. A bundled plugin in neither list
  lands in unknownDirs (src/core/runtime/bundled.js:151-157), so it is
  invisible to both composedRiders and buildPluginCatalog.
• Coverage can hold while agreement breaks. A name in both sets keeps
  coverage true and breaks agreement: discoverBundledPlugins tests the
  allowlist first, so such a plugin stays in loaded and the smoke composes it,
  while ridersInDefaultSet (src/core/cli/walkthrough.js:2212-2220) drops it
  and the install never writes it. The smoke goes red on a plugin the install
  never receives.

The load-bearing property is that the two sets are disjoint.

Fix

• hypaware-core/smoke/flows/walkthrough_picker_to_first_query.js: the JSDoc now
  states disjointness as the property the cut rests on, spells out why (the
  allowlist is checked before the exclude set, so a name in both would survive
  into loaded), keeps the separate observation about a plugin in neither list,
  and points at the test that guards it.
• test/core/bundled-sets.test.js: added the reciprocal pointer, so the
  invariant's consumer is discoverable from the guard as well as the other way
  round. This is the recurrence the issue flagged: a true conclusion resting on
  an unstated invariant, twice in this one file.

No behavior change; both edits are comments.

Proof the guard actually guards

The test asserted by #767 was verified to be a real guard rather than a
tautology, by breaking the invariant and observing the failure.

Perturbation: added '@hypaware/context-graph' (already in
V1_BUNDLED_PLUGIN_ALLOWLIST) to V1_EXCLUDED_FROM_DEFAULT, making the two sets
overlap while leaving coverage intact - exactly the case the old JSDoc's premise
could not detect.

not ok 1 - the default-activation allowlist and the excluded set are disjoint
  error: plugin(s) @hypaware/context-graph appear in both
  V1_BUNDLED_PLUGIN_ALLOWLIST and V1_EXCLUDED_FROM_DEFAULT: discoverBundledPlugins
  checks the allowlist first, so these land in its `loaded` bucket while
  ridersInDefaultSet treats them as excluded.
  + [ '@hypaware/context-graph' ]
  - []

The perturbation was reverted (git checkout -- src/core/runtime/bundled.js) and
the test passes again on the tree in this PR. The failure names the cause
directly, which was the point: without it the same break surfaces only as the
walkthrough smoke disagreeing with its golden.

Gates

• npm test: 4216 pass, 0 fail, 1 skipped.
• npm run typecheck: exit 0.
• npm run smoke -- walkthrough_picker_to_first_query: ok.

Note for anyone reproducing: the shared node_modules in this environment was
missing marked (a declared dependency), which produced 22 test failures and 1
TS2307 on unmodified origin/master. After installing it, both gates are
clean. The failure set was byte-identical with and without this change before
that install.

Fixes #761

[philcunliffe]

Verdict

Approve with fixes applied. The change is correct and does what #761 asked: it replaces a
premise that does no work (coverage) with the one that does (disjointness). Two accuracy
problems remained in the new prose, both of the same shape the issue was about (a claim
stated wider than its evidence supports). Both are fixed and pushed to this branch; nothing
is left outstanding.

Reviewed at head b0404be5. Comments-only diff, no executable code changed, so there is no
runtime-behavior surface. Every factual claim the new comments make was checked against
src/core/runtime/bundled.js, src/core/cli/walkthrough.js, src/core/plugin_catalog.js,
src/core/runtime/boot.js, and the smoke's own composedRiders.

What checks out

• discoverBundledPlugins does test the allowlist before the exclude set
  (src/core/runtime/bundled.js:150-157), so an overlapping name really would stay in loaded.
• ridersInDefaultSet (src/core/cli/walkthrough.js:2212-2220) drops on
  V1_EXCLUDED_FROM_DEFAULT membership alone, so disjointness really is what keeps the two
  reads in agreement.
• loadPickerCatalog (src/core/cli/walkthrough.js:2170-2179) really builds from
  [...bundled.loaded, ...bundled.excluded], so a plugin in neither list (which lands in
  unknownDirs) is invisible to both sides.
• The guard test named by the JSDoc exists and is a real assertion, not a tautology. I
  re-ran the PR body's perturbation independently: adding @hypaware/context-graph to
  V1_EXCLUDED_FROM_DEFAULT turns test/core/bundled-sets.test.js red with the cause named
  in the message, and reverting turns it green.
• Repo conventions: no em dashes introduced, no semicolon violations, the
  @ref LLP 0213#d1 [tests] annotation is still attached to composedRiders and #d1 still
  exists in llp/0213-graph-plugin-always-active.decision.md. No LLP text was edited.
• Gates on the PR head, with a complete node_modules: npm test 4216 pass / 0 fail / 1
  skipped, npm run typecheck exit 0, npm run smoke -- walkthrough_picker_to_first_query ok.

Findings

1. Low - the JSDoc claims exclusivity its own paragraph contradicts

hypaware-core/smoke/flows/walkthrough_picker_to_first_query.js:586-587 (at b0404be5):

Disjointness is guarded by test/core/bundled-sets.test.js; it is the whole of what this
agreement rests on, because discoverBundledPlugins checks the allowlist before the
exclude set [...]

The sentence immediately before it states a second load-bearing premise: that
loadPickerCatalog reads only the loaded and excluded buckets. That premise is doing real
work. If loadPickerCatalog were changed to build from [...loaded, ...excluded, ...unknownDirs], a bundled plugin in neither list that declares compose_with would compose
into the written config while composedRiders (which reads loaded only) omits it from the
golden: the smoke goes red with disjointness fully intact.

The because clause only establishes that disjointness is necessary; it does not establish
the exclusivity the sentence asserts. That is the same shape of defect #761 opened over, so
it seemed worth not shipping in the fix for it.

Fixed in 3910b1f0: the clause now reads "It is disjointness, not coverage, that does the
work here", which keeps the contrast the issue asked for without the unsupported absolute, and
the pointer to the guard test moved to its own sentence.

2. Low - the test comment's failure mode holds only for riders

test/core/bundled-sets.test.js:18-20 (at b0404be5):

Break disjointness and that smoke goes red on a plugin the install never receives [...]

True only when the overlapping name is a rider. Only two bundled manifests declare
compose_with (context-graph, ai-gateway-graph), which is presumably why the PR's own
perturbation picked one of them. For any other name nothing in the smoke's path ever consults
the exclude set: picked rows are not filtered by it, and ridersInDefaultSet only sees riders.

Verified empirically rather than by reading:

• @hypaware/local-fs (allowlisted, no compose_with) added to V1_EXCLUDED_FROM_DEFAULT
  -> walkthrough_picker_to_first_query stays green, with the invariant broken.
• @hypaware/context-graph (a rider) added to the same set -> the smoke goes red on the
  golden, exactly as the comment describes.

The non-rider break is not silent everywhere, though: computeSelectedPlugins
(src/core/runtime/boot.js:588-595) filters all-bundled by allowlist.has(name) && !V1_EXCLUDED_FROM_DEFAULT.has(name), so an overlapping name silently loses default activation
there. That strengthens the case for this test rather than weakening it, since it is the only
guard that catches the non-rider half at all.

Fixed in f0fe6765: the comment now scopes the smoke's red to riders, says plainly that
any other name leaves the smoke green, names the boot-profile consequence, and ends with
"This test names the cause in both cases".

Deliberately left alone

• The mechanism is now restated in four places (the smoke JSDoc, the test's header comment,
  the assertion message, and the PR body). Comment redundancy across a guard and its consumer
  is the point of the reciprocal pointer this PR adds, so it is not churn worth taking.
• The Fixes #761 trailer. The PR body has it, which is what the issue's own triage note
  said was missing from Pin the bundled allowlist/exclude-set disjointness invariant with a test (#761) #767. Nothing to do.
• No LLP change. Comments-only, realizing no new design decision, so there is nothing for
  a new doc to settle and nothing in an Accepted doc to extend.

Gates after the fixes

npm test 4216 pass / 0 fail / 1 skipped, npm run typecheck exit 0,
npm run smoke -- walkthrough_picker_to_first_query ok.
New head: f0fe676547120c3c59b032df05334e966770afd7.

[philcunliffe]

Verdict

Approve with fixes applied. Round 2 of 2, reviewed at head f0fe6765. Round 1's two fixes
are correct and landed as described; the residue is that round 1 fixed the rider-scoping problem
in only one of the two comments this PR touches, and left the same over-broad claim standing in
the other. That plus one profile-naming inaccuracy are fixed and pushed. Nothing outstanding.

Round 1's fixes verified

• 3910b1f0 (finding 1): the smoke JSDoc no longer asserts disjointness is "the whole of what
  this agreement rests on"; it now reads "It is disjointness, not coverage, that does the work
  here", and the pointer to the guard test is its own sentence. The second premise
  (loadPickerCatalog reading only the two buckets) is stated without being contradicted.
• f0fe6765 (finding 2): test/core/bundled-sets.test.js:18-25 now scopes the smoke's red to
  plugins declaring compose_with, says plainly that any other name leaves the smoke green,
  and names the computeSelectedPlugins consequence.

Re-verified against the tree (not taken on round 1's word)

• discoverBundledPlugins tests the allowlist before the exclude set
  (src/core/runtime/bundled.js:149-157), so an overlapping name lands in loaded.
• ridersInDefaultSet (src/core/cli/walkthrough.js:2212-2220) iterates the composeWith map,
  so it only ever sees riders.
• loadPickerCatalog (src/core/cli/walkthrough.js:2170-2179) builds from
  [...loaded, ...excluded]; a plugin in neither list goes to unknownDirs and is invisible
  to both sides.
• computeSelectedPlugins (src/core/runtime/boot.js:588-607) applies
  allowlist.has(name) && !V1_EXCLUDED_FROM_DEFAULT.has(name).
• Conventions: no em dashes introduced, no semicolons, no .d.ts/@typedef surface touched,
  the @ref LLP 0213#d1 [tests] annotation is still attached to composedRiders and #d1
  still exists. No LLP text edited; comments-only diff settles no new design, so no new doc is
  owed.
• Gates at f0fe6765: npm test 4216 pass / 0 fail / 1 skipped, npm run typecheck exit 0,
  npm run smoke -- walkthrough_picker_to_first_query ok.

Findings

1. Low - the smoke JSDoc still makes the unscoped claim round 1 removed from the test

hypaware-core/smoke/flows/walkthrough_picker_to_first_query.js:586-589 (at f0fe6765):

...discoverBundledPlugins checks the allowlist before the exclude set, so that name would
stay in loaded while ridersInDefaultSet drops it.

ridersInDefaultSet walks the compose_with map, so a name that declares no compose_with is
never presented to that filter and is not "dropped" by it. For such a name the two reads this
paragraph is reconciling do not disagree and this smoke stays green - which is exactly what
round 1 established, and exactly what round 1's own fix to test/core/bundled-sets.test.js now
says. Leaving the unqualified version in the smoke means the PR's two comments describe the same
mechanism at two different strengths, and the stronger one is the one a reader of
composedRiders hits first. Same defect shape as #761: a conclusion stated wider than its
evidence.

Fixed in e9f452ee: the sentence now separates the rider case (a live disagreement - this
function composes it into the golden, ridersInDefaultSet drops it from what the install
writes) from the non-rider case (never reaches that filter, the two reads still agree, this
smoke stays green), and points at computeSelectedPlugins as where a non-rider overlap actually
bites.

2. Low - the test comment names only the boot profile the product does not run

test/core/bundled-sets.test.js:23-24 (at f0fe6765):

computeSelectedPlugins (src/core/runtime/boot.js) still drops that name from the
all-bundled boot profile.

The identical allowlist && !excluded filter is applied to all-available as well
(src/core/runtime/boot.js:598-607), and all-available is the profile the shipped entrypoints
boot: decideBootProfile returns it for bare hyp and for hyp init
(src/core/cli/dispatch.js:624-625). Nothing under src/ or bin/ ever requests
'all-bundled'. As written the comment invites the reading that the silent consequence lives in
a profile users never hit, when it lands on the one hyp init runs under.

Fixed in e9f452ee: the comment now names both default profiles and marks all-available
as the one bare hyp and hyp init actually run.

Deliberately left alone

• "Break disjointness on a plugin that declares compose_with and that smoke goes red"
  (test/core/bundled-sets.test.js:18-21) is true for both riders that exist today
  (context-graph, ai-gateway-graph - both ride the gateway, which this smoke picks), but
  would not hold for a future rider whose compose_with this smoke never satisfies. Qualifying
  it further would trade a correct-today sentence for a longer hedge about a plugin that does
  not exist; not worth the prose.
• The mechanism now appears in four places (smoke JSDoc, test header, assertion message, PR
  body). The reciprocal pointer is the thing this PR set out to add, so the redundancy is
  intended, not churn.
• No LLP change and no @ref change. Comments-only, realizing no new decision; nothing in
  an Accepted doc to extend.

Gates after the fixes

npm test 4216 pass / 0 fail / 1 skipped, npm run typecheck exit 0,
npm run smoke -- walkthrough_picker_to_first_query ok.
New head: e9f452ee2d23d6280e05a6ff646f0b2fb7258c71.

[philcunliffe]

Triage at head e06c303e (only a green-master merge after round 2; no PR content changed): every residual finding from the two review rounds is non-blocking. The diff is comments-only and all factual claims in the final comment text were re-verified against the merged tree. The deferred items (a future-rider scoping hedge in the test/core/bundled-sets.test.js header comment, and the intended four-place restatement of the disjointness mechanism) are recorded in follow-up issue #828.