walkthrough_picker_to_first_query golden predates compose_with riders (#750) - #757
Conversation
…#750) The Phase 5 golden config still lists the six plugins the picker rows compose, but `hyp init` has written eight since #720 landed LLP 0213 D1: `@hypaware/context-graph` and `@hypaware/ai-gateway-graph` declare `compose_with: ["@hypaware/ai-gateway"]` and ride the gateway pick. The picker is right and the golden is stale. #720 added the rider fold to `composePickerConfig` (+153 lines in `src/core/cli/walkthrough.js`) and touched no smoke flow, which is why neither `goldenPickerConfig` nor `bundled.js` shows the change in `git log`: the divergence arrived through the manifests. Rather than paste the two names in, the golden derives its rider tail from the same declarations the composer reads, so the next derived-data plugin is a manifest edit rather than a re-red smoke. The literal half is what the picker actually decides: which rows compose, the upstreams, the OTLP port, the sink shape, retention. The derivation reads manifests, not `ridersFor`, so a plugin that is neither picked nor declared a rider still fails the assertion; the rider mechanism itself keeps its unit coverage in `test/core/compose-picker-config.test.js`. Co-Authored-By: Claude <noreply@anthropic.com>
neutral review - round 1Head reviewed: The reviewer ran a mutation table rather than reading the new expectation and Cleanup done, worktree clean. Here is the review. VERDICT: findings 1. minor —
Verified: setting Why it matters: this is exactly the case the PR was built to avoid. The commit message says "the next derived-data plugin is a manifest edit rather than a re-red smoke", and the helper's own JSDoc at line 557 describes the rule as "a manifest whose Exact fix — make the helper transitive, matching async function composedRiders(picked) {
const { loaded } = await discoverBundledPlugins()
const present = new Set(picked)
/** @type {string[]} */
const riders = []
let grew = true
while (grew) {
grew = false
for (const { manifest } of loaded) {
const waitsFor = manifest.compose_with
if (!Array.isArray(waitsFor) || waitsFor.length === 0) continue
if (present.has(manifest.name)) continue
if (!waitsFor.every((name) => present.has(name))) continue
present.add(manifest.name)
riders.push(manifest.name)
grew = true
}
}
return riders
}(This is still not a call into 2. nit — " What the new expectation still catchesAll mutations applied to the head commit's worktree,
On M6, the one real blind spot. Because the golden reads the same declarations the composer does, a manifest-level change to The residual symmetric-degradation risk is narrow and I could not induce it: if Also checked, clean
|
…#750) composedRiders was single-pass while ridersFor runs to a fixpoint, so a rider whose compose_with names another rider (rather than a picked plugin) was composed by the implementation but missing from the golden, false-redding the smoke on a manifest-only edit. Run the same manifest set to a fixpoint, mirroring ridersFor, so second-order riders land in the golden too. Also corrects a comment that claimed discoverBundledPlugins's loaded bucket is "the same cut" ridersInDefaultSet makes on the composer. It is not the same cut (allowlist membership vs. exclude-set non-membership); they only agree because those two lists partition the whole bundled workspace and loadPickerCatalog reads only them.
neutral review - round 2 (final)Head reviewed: Both round-1 findings are genuinely fixed. The ordering argument, which is the one One nit, not ship-blocking, and it is the same defect class as the nit it replaced: Cleanup done (my three scratch files removed; the other scratchpad contents predate this session). Worktree clean at VERDICT: findings Nothing is ship-blocking. Both round-1 findings are genuinely fixed, I could not break the ordering argument in five constructed cases plus 8000 randomized graphs, and the ten mutations round 1 caught are still caught. The one finding below is a comment-accuracy nit. 1. nit — The replacement wording for round-1 finding 2 states a premise that is neither necessary nor sufficient for the conclusion it justifies: The two pools are: helper =
Why it matters: this is the same class of defect as round-1 nit 2 (a true conclusion resting on the wrong reason), and it survived because the round-1 reviewer supplied the wording verbatim. The stated invariant is also the one that is not guarded: Exact fix — replace lines 582-584 with: Direction of the failure this describes is fail-safe (false red, never false green), which is why it is a nit and not a correctness finding. Round-1 findings, re-derived
Ordering, attackedFirst the argument, traced independently. Cases constructed, each applied to the head worktree and reverted (
Then a randomized differential, since three hand cases prove little about a fixpoint: 8000 random On the two Also checked, clean
|
neutral triage - shipReview budget exhausted with one residual finding. It is a preference. This PR can The finding is a JSDoc comment justifying a correct mechanism with the wrong reason: Triage checked the thing that would have changed the answer rather than taking the The failure mechanism is real but fail-safe in both senses: it needs an inconsistent Worth recording, because it is a process signal rather than a code one: this is the Also checked, since master moved mid-review: PR #721 merged as Verified at head |
philcunliffe
added
the
neutral:approved
1 participant
Fixes #750.
Which side was wrong: the golden
The picker is correct.
goldenPickerConfigis stale.LLP 0213 #d1 (Accepted, 2026-08-12) decides it in as many words:
Both manifests carry exactly that:
So a Phase 5 config that picks the gateway is supposed to carry both. Option 2 in the issue (a graph plugin appearing unasked is the defect) is ruled out: appearing unasked is the decision. LLP 0213 names it a derived-data plugin, a fifth emergent category that "contributes no pick of its own, and rides the pick whose data it derives from", and LLP 0011 #no-architectural-names is cited as the reason it is deliberately not a picker row: the user says what to collect and HypAware picks the plugin set.
The commit that caused the divergence
5b13456"Reduce the client skill surface to three; always activate the graph (#720)" (Brendan McMullen, 2026-08-12).How I know:
compose_with(git log -S"compose_with" -- src/ hypaware-core/ test/returns exactly one commit), and the only commit touchingllp/0213-graph-plugin-always-active.decision.md.src/core/cli/walkthrough.js, which is whereridersForand thefor (const rider of ridersFor(plugins, args.composeWith)) plugins.push({ name: rider })line at the end ofcomposePickerConfignow live.git show --stat 5b13456 | grep -i "smoke\|walkthrough"matches onlysrc/core/cli/walkthrough.js. It changed no smoke flow.That is the indirection the issue predicted. Neither
goldenPickerConfignorbundled.jsshows the change ingit log, because the new plugins entered through manifest edits plus a new fold in the composer.V1_BUNDLED_PLUGIN_ALLOWLISTis a red herring: it never governed the picker, only theall-bundled/all-availableboot profiles. Membership in it is necessary (loadPickerCatalogfilters riders throughridersInDefaultSet) but not sufficient, which is why eleven other allowlisted plugins are still correctly absent from the written config.What changed
One file,
hypaware-core/smoke/flows/walkthrough_picker_to_first_query.js.Per the issue's warning against pasting in two lines: the golden now derives the rider tail and keeps the literal half for what the picker itself decides.
goldenPickerConfigstill lists the six picked plugins literally, with the upstreams, the OTLP host/port, the sink shape, and retention verbatim. That is the part the picker composes from its rows, and it stays a golden.composedRiders(picked)reads the bundled manifests and, run to a fixpoint mirroringridersFor, returns those whosecompose_withnames are, transitively, all in the picked set (a rider whosecompose_withnames another rider still lands). A new derived-data plugin, including one that rides another rider, is now a manifest edit, not a re-red smoke.On the "a derived expectation that mirrors the implementation stops testing anything" risk, three things keep it honest:
composePickerConfigorridersFor. It reads the manifest declarations and applies the rule to a literal picked set, so the composer is still the code under test.deepEqualover the whole config. A plugin that is neither picked nor a declared rider fails the assertion, which is the "graph plugin appears unasked" defect the issue worried about, still caught.test/core/compose-picker-config.test.js, which LLP 0213 #consequences designates as its home. The smoke does not need to re-derive policy that has unit tests.composedRidersreads onlydiscoverBundledPlugins().loaded, neverexcluded. That reproduces the default-activation boundaryridersInDefaultSetenforces (acompose_withmust not be a route around an explicit opt-in such as an API-backed embedder) without restatingV1_EXCLUDED_FROM_DEFAULTa second time.No LLP change: this implements a decision already Accepted in 0213 and changes no documented contract.
Full release battery
All 13 flows from
CLAUDE.md, on this branch, Linux, freshnpm install:package_bin_bootcli_bundled_plugins_activateddaemon_foreground_start_stopdaemon_install_renderwalkthrough_picker_to_first_queryclient_attach_idempotentgateway_claude_capturegateway_codex_capturehypignore_capture_droplocal_only_export_withholdotel_loopback_capturelocal_parquet_exportstatus_diagnosticsSo #750 was not the only red flow, which the issue explicitly left open.
package_bin_bootis a second, separate rot (not fixed here)Same shape as this issue, different commit, different smoke. Out of scope, deliberately not touched, worth its own issue:
hypaware-core/smoke/flows/package_bin_boot.js:68asserts the help output.includes('hyp - HypAware kernel CLI').src/core/cli/dispatch.js:773now writes:git log -S"HypAware kernel CLI" -- src/ bin/points atb9e50ee"Onboarding copy overhaul: explain the product, lead with shared collection (LLP 0211) (#711)", which rewrote the tagline and did not update the smoke that pins it. Confirmed pre-existing: it fails identically with this branch's change stashed. The fix is presumably to assert the stablehyp - HypAwareprefix rather than a marketing sentence that is expected to change.Checks
npm test- pass (4024 passing, 0 failing, 1 skipped)npm run typecheck- pass (clean)npm run smoke -- walkthrough_picker_to_first_query- okRecommendation on CI (not implemented, for a human to decide)
The issue's second half is the more valuable one, and I have deliberately not implemented a CI change. Here is the concrete tradeoff, with the cost measured rather than guessed.
Today
.github/workflows/runstest (22),test (24),typecheck (22),typecheck (24)andduplicate-numbers. No smoke runs anywhere.The cost argument is weaker than it looks. The entire 13-flow battery runs in about 4.9 seconds. Slowest flow 487 ms, whole battery under five seconds wall clock, against a job that already spends far longer on checkout and
npm ci. There is no per-flow triage to do: nothing here is slow.Nor is there an isolation problem. Every one of these is a hermetic smoke by construction: temp
HYP_HOME,HYP_DEV_TELEMETRY=1, loopback-only ports, no network egress, no real daemon install, no touching the runner's home. They are exactly the tierCLAUDE.mddescribes as "good for PR confidence and plugin/kernel wiring checks".Recommendation: run the full battery as one job on every PR, single Node version,
ubuntu-latest. Not a matrix: these exercise wiring, not language-version behaviour, and the existingtest/typecheckmatrix already covers 22 and 24.Two caveats a human should weigh:
package_bin_bootis red right now. Making the battery a required check either waits on that fix or lands with it. Otherwise the new job blocks every PR on day one.daemon_install_renderrenders systemd on Linux and launchd on macOS, so a Linux-only job leaves the launchd path unguarded. Amacos-latestleg on a nightly schedule (not per-PR) would close that at low cost. Everything else is platform-agnostic.What this does not change:
CLAUDE.mdis explicit that hermetic smokes are not the release gate, and acceptance smokes (docs/ACCEPTANCE.md) stay a human step. Putting the battery in CI does not weaken that; it just means the next rot is caught by a machine on the commit that causes it rather than by a passer-by two PRs later, which is what happened here twice.🤖 Generated with Claude Code