Skip to content

package_bin_boot: stop pinning the --help marketing banner (#758) - #762

Merged
philcunliffe merged 2 commits into
masterfrom
fix/issue-758
Aug 14, 2026
Merged

package_bin_boot: stop pinning the --help marketing banner (#758)#762
philcunliffe merged 2 commits into
masterfrom
fix/issue-758

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • package_bin_boot was red on master: it asserted hypaware --help
    prints the literal string hyp - HypAware kernel CLI, which b9e50ee
    (Onboarding copy overhaul: explain the product, lead with shared collection (LLP 0211) #711, LLP 0211) replaced with the current onboarding banner.
  • Checked LLP 0211 (the onboarding-copy decision that changed the banner)
    and the other CLI/help LLPs that touch dispatch.js's renderHelp
    (LLP 0009 #layered-help, LLP 0129, LLP 0214). None of them treat the
    banner's marketing sentence as a contract: LLP 0211 documents the fork
    wizard
    intro line and menu copy as decisions, and only mentions the
    --help banner in passing in its commit message ("README and hyp
    --help lead with the product, not the architecture"). LLP 0009's ref
    on renderHelp covers the layout contract (one row per top-level
    command, subcommands collapsed into group help), not the leading
    sentence's exact wording.
  • Went with option 2 from the issue: loosened the smoke to assert
    what the flow is actually named for (the packaged binary boots and --help works), not the marketing copy. It now asserts the usage: hyp <command> [args...] line and the presence of known command names
    (daemon, status), on top of the exit-0 check that was already
    there.
  • Nothing pins the exact banner text after this change. That is
    deliberate: no LLP claims it as a contract, so no unit test was added
    next to the copy either. If a future LLP wants the exact sentence
    pinned, that's a new decision, not a reason to keep the boot smoke
    pinned to marketing copy.

Change

hypaware-core/smoke/flows/package_bin_boot.js: replaced the
hyp - HypAware kernel CLI substring assertion with two assertions:
the usage: hyp <command> [args...] line, and that the output lists
known command names.

Verified the new assertion still catches a real break

Temporarily swapped the usage: hyp <command> [args...] write in
src/core/cli/dispatch.js's renderHelp for a sentinel string, ran
npm run smoke -- package_bin_boot, confirmed it failed on
hypaware --help prints the usage line, then restored the file and
confirmed the smoke passes again. git status is clean; only
package_bin_boot.js is changed.

Checks (fresh npm install)

  • npm test: 4029 pass, 1 skipped, 0 fail
  • npm run typecheck: clean
  • npm run smoke -- package_bin_boot: ok

Full release battery (CLAUDE.md)

Flow Result
package_bin_boot ok (fixed here)
cli_bundled_plugins_activated ok
daemon_foreground_start_stop ok
daemon_install_render ok
walkthrough_picker_to_first_query FAIL - known, being fixed in #757 (not merged), not addressed here
client_attach_idempotent ok
gateway_claude_capture ok
gateway_codex_capture ok
hypignore_capture_drop ok
local_only_export_withhold ok
otel_loopback_capture ok
local_parquet_export ok
status_diagnostics ok

12 of 13 green. The one red flow is walkthrough_picker_to_first_query,
already tracked and being fixed by #757; not expanded here per the
issue's instruction to keep this PR scoped to #758.

Deliberately not done

🤖 Generated with Claude Code

The smoke asserted `hyp - HypAware kernel CLI`, a string b9e50ee (#711,
LLP 0211) replaced with an onboarding banner. LLP 0211 and the other
CLI/onboarding LLPs (0009, 0129, 0214) treat that sentence as copy, not
a contract: nothing pins the exact banner text. Assert the stable
things the flow is actually named for instead: the `usage: hyp
<command> [args...]` line and the presence of known command names.
Exit status 0 was already asserted and stays.

Verified the new assertion still catches a real break: swapped the
usage-line write for a sentinel string, watched the smoke go red on
"hypaware --help prints the usage line", then restored it and confirmed
green.

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

Copy link
Copy Markdown
Contributor Author

neutral review - round 1

Head reviewed: 8121682ffca3ad102011afa7fd1a5be69d49bc00. MERGEABLE, all 9 checks
SUCCESS. Reviewed in a detached worktree; nothing was written to the branch.

Correction to something I asserted when dispatching this review. I told the
reviewer that usage: hyp is "already asserted in test/core/cli/style.test.js and
test/core/command-dispatch.test.js, so the new assertion leans on a string the
repo already treats as stable". That is too loose and I have re-checked it: the
strings those tests pin are all subcommand usage lines (usage: hyp query <subcommand> [args...], usage: hyp graph <subcommand> ..., usage: hyp daemon install), produced by a different renderer. The exact top-level string
usage: hyp <command> [args...] is asserted nowhere in test/, llp/, docs/
or any root markdown. I grepped a prefix and inferred the specific string from it.

That does not make the PR's choice wrong, but it inverts the risk I described: this
smoke is now the sole guard for top-level --help output, which raises the
stakes on finding 1 rather than lowering them.

Finding 1 is a real one and it is the load-bearing half of the PR. The
"lists known commands" assertion is a substring match over the whole --help blob,
and both words it looks for appear in fixed prose independent of the command table:
status in the epilogue line and in the query row's summary, daemon in the
join row's summary and its own. The reviewer proved it empirically rather than by
reading: dropping every daemon* and status row from the table while leaving
the other 21 intact left the smoke green. That is precisely the regression the
assertion is supposed to name.


Worktree clean, my scratch files removed (others' untouched).

VERDICT: findings


1. major — hypaware-core/smoke/flows/package_bin_boot.js:76-80 — the "lists known commands" assertion does not check that any command is listed.

Both new command-name checks are plain String.includes over the whole --help stdout, and renderHelp emits both words in fixed prose that is independent of the command table:

  • status is printed unconditionally in the epilogue: Start with 'hyp status' for whether this install is working. (src/core/cli/dispatch.js:783), and again inside the query row's summary (Query the local cache (sql, schema, status, ...)).
  • daemon is printed inside the join row's summary (Join a centrally-managed fleet (write seed config + install daemon)) and inside the daemon row's own summary text.

I verified this empirically. With a filter that removes every daemon* and status command from the help table while leaving the other 21 rows intact — the exact regression "the packaged binary boots but its command table lost commands" — the smoke stayed green (case B10 below). The assertion only fires when the table collapses so completely that even incidental prose mentions disappear (B4, empty table).

Why it matters: this assertion is the entire load-bearing half of the PR's thesis. The banner check it replaced was rot-prone but at least checked a string that existed exactly once. The replacement reads as "the registry-assembled command table rendered" and does not test that. It also means a real rename is detected by accident rather than by design: renaming the whole daemon command family to service also stayed green (B9b), because the word survives in two summaries.

This is not a regression against master (the old banner assertion caught B10 no better), and CI green is correct — hence major, not blocker. But the fix is small and makes the assertion mean what its message says.

Exact fix — parse the rendered rows instead of substring-matching the blob. renderHelp writes each row as ` ${name.padEnd(nameWidth)} ${summary}\n` (src/core/cli/dispatch.js:780), so rows are uniquely identified by a two-space indent, the name, then two-or-more spaces. Replace lines 76-80 with:

      const helpRows = String(helpResult.stdout ?? '')
        .split('\n')
        .flatMap((line) => {
          const m = /^ {2}(\S+) {2,}\S/.exec(line)
          return m ? [m[1]] : []
        })
      expect.that(
        `hypaware --help lists the core command rows (got rows=${helpRows.join(',') || '<none>'})`,
        helpRows,
        (v) => v.includes('daemon') && v.includes('status') && v.length >= 10
      )

This is anchored to what LLP 0009 #layered-help actually specifies (one row per top-level token), cannot be satisfied by prose, adds a floor so a partial table collapse is caught too, and names the parsed rows in the failure message. It goes red on B4, B10 and B9b, and green on the unmodified tree (23 rows). A minimal variant, if the author prefers a two-line change in the existing idiom, is to assert v.includes('\n daemon ') and v.includes('\n status ') — that also survives B10 and B9b, since padEnd guarantees at least two spaces after the name.

On whether daemon/status are the right pair to pin (they are, keep them): LLP 0009 names daemon explicitly as one of the core groups built by makeGroupCommand, and has a whole ## Core-rendered status section stating hypaware status is rendered by core. Both names are LLP-anchored, not arbitrary picks, so this is not a new rot source of the HypAware kernel CLI kind. They are also more durable than they look: renderHelp synthesizes a group row from groupChildren, so daemon survives removal of the bare daemon command as long as any daemon <sub> remains (verified, B9). I would not swap them for a pure count — the count alone would not notice the table being replaced by unrelated rows. Name-plus-floor, as above, is the right combination.


What this smoke can and cannot now catch

All cases run against this worktree; each was injected, npm run smoke -- package_bin_boot was run, then reverted.

# Induced breakage Result Which assertion fired
B1 --help exits status 3, stdout intact RED exits 0 (got status=3, stderr=<empty>)
B2 --help prints nothing at all, exit 0 RED prints the usage line
B3 --help prints only the banner (returns before the usage line) RED prints the usage line
B4 usage line prints, command table empty (names = []) RED lists known commands (via daemon only; status was still present from the epilogue)
B5 binary cannot resolve its entry point (dispatch.js import broken) RED exits 0 (got status=1, stderr=ERR_MODULE_NOT_FOUND ...)
B7 usage line reworded to a sentinel, banner + table intact RED prints the usage line (author's claim re-derived)
B8 banner sentence deleted entirely GREEN intended by this PR — banner is copy, not contract
B9 bare daemon command renamed to service, subcommands kept GREEN group row re-synthesized from daemon * children; correct behavior
B9b entire daemon command family renamed to service GREEN finding 1 — word survives in the service/join summaries
B10 every daemon* and status row dropped from the table, other 21 rows intact GREEN finding 1 — the core case the new assertion should catch
B11 a bundled plugin's activate() throws GREEN not detectable here by design: top-level help renders pre-bootKernel (LLP 0009 #top-level-help-lists-plugin-commands-without-booting), so activation cannot affect the table. The old banner assertion did not catch it either. Not a regression.

Two gaps that pre-date the PR and are not findings against it, but are worth recording: (a) plugin-contributed help rows are never exercised — I confirmed plugin_command_count: 0 in the flow's fresh HYP_HOME, so a silent failure of collectPluginHelpCommands (which swallows all errors by design) is invisible to this flow; (b) nothing asserts the rows are sorted, which LLP 0009 requires.

Bottom line for a release gate: after the finding-1 fix, the flow catches every breakage class in the "packaged binary boots and --help works" family that I could construct. As written it sleeps through a partially-emptied command table.

Also checked, clean

  • LLP claim — author is correct. LLP 0009 #layered-help scopes itself to layout only ("one row per top-level command token, sorted", subcommands collapsed into groups, where a group row's summary comes from); it says nothing about the leading sentence. LLP 0211 pins the wizard fork intro line and retires the pick lane Welcome to HypAware - the local logs+telemetry collector. banner, both wizard surfaces, not --help. LLP 0129, 0153 and 0214 touch help but only at the group/long-help level (0214 explicitly says "hyp --help stays one row per token"). Grepping llp/, docs/ and root *.md for the banner text finds only llp/0211:37 and README.md:4 quoting the product sentence as prose, never as a pinned string. No doc treats the banner as a contract, so declining to add a unit test pinning it is the right call and the in-code comment is accurate.
  • Correction to one premise. The exact top-level string usage: hyp <command> [args...] is not asserted anywhere in test/, nor in any LLP or doc — grep for hyp <command> across test/ llp/ docs/ *.md returns nothing. What test/ pins are subcommand usage lines (usage: hyp query <subcommand> [args...], usage: hyp graph <subcommand> ..., usage: hyp leave), produced by a different renderer. There is no unit-level coverage of renderHelp's top-level output at all. This does not make the new assertion wrong, but it means this smoke is now the sole guard for top-level --help, which raises the stakes on finding 1.
  • Battery — author's claim verified exactly. Ran all 13 flows from the CLAUDE.md release checklist individually: 12 green, only walkthrough_picker_to_first_query red (Phase 5 picker config matches expected shape, the failure walkthrough_picker_to_first_query golden predates compose_with riders (#750) #757 fixes). No third red flow. Also ran source_optout_export_withhold (added to the AGENTS.md list on this branch): green.
  • npm test: 4029 pass, 0 fail, 1 skipped. npm run typecheck: clean. Fresh npm install before all runs.
  • Conventions: no semicolons, no em dashes (U+2014) anywhere in the added lines, no @typedef, no inline import() types. Diff is genuinely one file, +12/-2.
  • Failure-message idiom: the two new assertions do not interpolate, matching the file's existing stdout-content assertion (hypaware smoke core_boot_noop reports ok); only the exit-status assertions interpolate. This is correct here because assertionError in hypaware-core/smoke/lib/expect.js:87 dumps the full value= — I confirmed in the B4 and B7 runs that the complete --help stdout is printed on failure, so both new assertions are diagnosable as written.
  • Comment accuracy: the added comment's "no LLP pins it" and its e.g. LLP 0211 citation both hold. Not marking it @ref is right, since it is a negative reference (0211 does not govern this code) and an @ref would falsely claim implementation.
  • Worktree left clean (git status empty); /work/hypaware untouched; no commits, pushes, or PR edits.

…ubstring (#758)

Both `daemon` and `status` also appear in incidental --help prose (the
epilogue and the `query`/`join` row summaries), so the previous
`String.includes` check stayed green even when every daemon*/status row
was dropped from the command table, or when the whole daemon family was
renamed. Parse rows by their `renderHelp` shape (two-space indent, name,
two-or-more spaces) instead, keep the daemon/status name checks (both
LLP 0009-anchored), and add a >=10 row floor as a partial-collapse net.

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

Copy link
Copy Markdown
Contributor Author

Worktree clean, scratch removed. Here is the review.


neutral review - round 2 (final)

Head reviewed: a92f3384f9a70d84e4bfe159011fe315e67452b6. One file, +25/-2. Reviewed in a detached worktree; nothing written to the branch, no commits, no pushes.

Round 1's finding is fixed, and I re-derived both proof cases independently rather than trusting the report. Nothing here is ship-blocking. The three items below are a diagnosability weakness and two nits; all are safe to land as follow-ups or as a trivial amendment if triage wants one.

VERDICT: findings


1. minor (NOT ship-blocking) - hypaware-core/smoke/flows/package_bin_boot.js:88-92 - the row assertion drops the help text from its failure output, and is undiagnosable in its most likely failure mode.

The file's idiom (confirmed again in round 2) is that stdout-content assertions pass helpResult.stdout as the value, so assertionError (hypaware-core/smoke/lib/expect.js:87) dumps the whole blob. The new assertion passes helpRows instead, so on failure you get the parsed names twice (comma-joined in the message, then again as a pretty-printed JSON array) and never see the actual --help output. Captured verbatim from an injected B10 run:

hypaware --help lists the core command rows (got rows=ask,attach,backfill,...,version)
  value=[
  "ask",
  ...21 more lines...
]

25 lines to convey one list, and no stdout. That is survivable for B10. It is not survivable for the failure mode a parse-based assertion is most likely to hit: the renderer's row formatting changing while the table is fine. I injected exactly that (case N1, row indent 2 spaces to 3) and the entire diagnostic is:

hypaware --help lists the core command rows (got rows=<none>)
  value=[]

Nothing there distinguishes "the command table is empty" from "the regex stopped matching a perfectly good table", and the run's stdout is gone. Same output for N2 (all summaries empty). This matters more than usual because, as round 1 established and I re-confirmed by grep, this smoke is the sole guard for top-level --help in the repo: usage: hyp <command> [args...] appears in test/, llp/, docs/ and hypaware-core/ exactly once, on line 73 of this file.

It also mitigates the one coupling this PR adds. LLP 0009 #layered-help pins "one row per top-level command token, sorted" and where a group row's summary comes from; it does not pin the two-space indent or the two-space gap. So the PR trades a pin on unpinned banner copy for a pin on unpinned row whitespace. That trade is clearly correct (mechanical padEnd formatting is far more stable than marketing copy, and any change to it is a deliberate edit of renderHelp), but it is only non-rotting if a formatting break announces itself. Right now it does not.

Exact fix, keeping the parsed rows in the message and restoring the file's idiom for the value:

      expect.that(
        `hypaware --help lists the core command rows (got rows=${helpRows.join(',') || '<none>'})`,
        { rows: helpRows, stdout: helpResult.stdout },
        (v) => v.rows.includes('daemon') && v.rows.includes('status') && v.rows.length >= 10
      )

(JSON.stringify renders the stdout as one escaped string; I confirmed from the B7 run above that that form is perfectly readable.) A simpler variant is to pass helpResult.stdout as the value and let the predicate close over helpRows, at the cost of a predicate that ignores its argument.


2. nit (NOT ship-blocking) - hypaware-core/smoke/flows/package_bin_boot.js:91 - the floor of 10 fires, but only after a 60% collapse; and the count it was calibrated against was measured in the wrong environment.

Settling the 23-versus-24 discrepancy: both numbers are right, and 23 is the one that governs. The row count is install-specific, because renderHelp merges plugin-contributed commands from config-active plugins (the epilogue says so: "This list reflects the plugins active in your config"). In the flow's actual environment - the harness's fresh temp HYP_HOME - there are 23 rows, all core, and plugin_command_count: 0 in the emitted log line. In a developer home with ai-gateway config-active there are 24, the extra one being the synthesized session group row from hypaware-core/plugins-workspace/ai-gateway/hypaware.plugin.json:21-23. The PR's "24 rows" was measured outside the flow's environment. This does not change the arithmetic conclusion (10 is under half either way) and the environment dependence is one-directional (plugins only add rows), so the floor can never false-fire on a plugin-rich install.

The floor is not decoration; I proved it fires (N7: table cut to 6 rows with daemon and status kept goes RED, and only the length clause could have caught that). But its blind band is wide: I dropped 10 of the 23 rows while keeping daemon and status (N10) and the smoke stayed GREEN. So a partial table collapse is invisible up to 13 lost commands. Against the legitimate-shrink risk this is very conservative: a release would have to remove 14 commands in one go to trip a floor of 10, and 6 to trip a floor of 18. Removing 6 top-level commands at once is not a thing this CLI has ever done.

If you want the floor to do real work: change v.length >= 10 to v.length >= 18. I am not asking for it. 10 is defensible, never false-fires, and the name checks carry the load. Recording it so the number is a decision rather than an accident.


3. nit (NOT ship-blocking) - hypaware-core/smoke/flows/package_bin_boot.js:75-92 - the row-parsing block has no @ref, though it is now the only code in the repo that tests LLP 0009 #layered-help.

The comment cites src/core/cli/dispatch.js (a source file) rather than the doc that makes the row shape a contract. 22 of 59 smoke flows already use the [tests] idiom for exactly this relation (e.g. hypaware-core/smoke/flows/daemon_foreground_start_stop.js:32, join_flow_remote_config.js:47). A ref here would tell a reader something the code and filename do not: that the row layout is specified, not incidental, so if 0009 changes this is the assertion to revisit. Suggested, on the line above const helpRows:

      // @ref LLP 0009#layered-help [tests]: one row per top-level token is the rendered contract this parses

Round-1 finding, re-derived

Fixed. Not fixed-but-with-a-new-problem; the new assertion means what its message says.

I re-injected both proof cases myself against this head, ran the real smoke each time, and reverted:

  • B10 (drop every daemon* and status row from merged, leave the other 21 intact): RED, and the message names the surviving rows, so the failure is self-explaining: got rows=ask,attach,backfill,config,detach,ignore,init,join,leave,mcp,plugin,policy,purge,query,remote,report,sink,skills,sync,unignore,version. On master's substring assertion this was green.
  • B9b (rename the whole daemon family to service): RED, got rows=...,report,service,sink,.... The word still survives in two summaries, but it is no longer a row name, which is the point.
  • Unmodified tree: GREEN, repeatedly (baseline plus a final re-verify after all injections were reverted).

I also independently re-derived the two premises the fix rests on. The regex handles the tightest real case: nameWidth = Math.max(...names.map(n => n.length), 8), so the longest names (backfill, unignore) render with a gap of exactly two spaces, and both appear in the parsed set on the live output. Widening the column does not break it either (N4, injecting a 28-character command name: GREEN).

What this smoke can and cannot now catch

Every case injected into src/core/cli/dispatch.js in this worktree, run through npm run smoke -- package_bin_boot, then reverted. B-cases are round 1's numbering; N-cases are new in round 2.

# Induced breakage Result Which assertion fired
B1 --help exits status 3, stdout intact RED exits 0 (got status=3, stderr=<empty>)
B2 --help prints nothing, exit 0 RED prints the usage line, value=""
B3 banner only, returns before the usage line RED prints the usage line
B7 usage line reworded to Usage: hyp COMMAND [ARGS] RED prints the usage line (full stdout dumped)
B8 banner sentence deleted entirely GREEN intended by this PR; banner is copy, not contract
B9b whole daemon family renamed to service RED lists the core command rows (round-1 finding, now fixed)
B10 every daemon* and status row dropped, 21 rows intact RED lists the core command rows (round-1 finding, now fixed)
N1 row indent changed 2 spaces to 3, table otherwise perfect RED lists the core command rows, rows=<none> - correct verdict, undiagnosable message (finding 1)
N2 every summary rendered empty RED same, rows=<none> (finding 1)
N4 a 28-character command name widens nameWidth for every row GREEN correct; padding growth does not break the parse
N5 one command's summary rendered empty GREEN that row silently drops from helpRows; count 22, names intact. Acceptable
N6 rows rendered in reverse order (unsorted) GREEN gap: LLP 0009 requires sorted, nothing asserts it (see below)
N7 table cut to 6 rows, daemon + status kept RED the length >= 10 floor - it is not decoration
N10 10 of 23 rows dropped, daemon + status kept GREEN floor's blind band: up to 13 lost commands are invisible (finding 2)
N8 every command except daemon/status renamed with a zz prefix GREEN inherent to name-plus-floor; not a realistic silent regression
N3 command table emptied, a two-space-indented Examples: block added RED regex picked up status,init from the examples - it does match non-rows
N3b same, with 11 single-token example lines including daemon and status GREEN the assertion is fully satisfiable by non-rows (see below)
N9 table emptied, one row whose summary embeds newlines forming 9 fake rows GREEN same class, reachable through a summary string alone
B11 a bundled plugin's activate() throws GREEN not detectable by design; top-level help renders pre-bootKernel (LLP 0009 #top-level-help-lists-plugin-commands-without-booting). Not a regression

On N3b and N9, the answers to the questions posed for this round: yes, /^ {2}(\S+) {2,}\S/ can match something that is not a command row, and yes, helpRows.length >= 10 can be satisfied entirely by non-rows - but only by lines whose first token is followed by two or more spaces. Multi-token example lines do not match ( daemon install run it parses no name, because a single space follows daemon). Both demonstrations required simultaneously emptying the table and adding a new two-space-indented section to renderHelp, i.e. one change that breaks the contract plus a second, unrelated one that happens to mask it. I do not think that is worth defending against; anchoring the parse to the region after the Commands: header would harden it, at the cost of pinning another unpinned string. Recording it so the limit is known rather than assumed. A blank-name row and a row whose name contains whitespace are both unreachable: renderHelp derives each row name from cmd.name.split(' ')[0], so a top-level row name is always a single non-empty token.

The two out-of-scope gaps round 1 recorded both still stand.

  • Plugin-contributed help rows are never exercised. Re-confirmed directly: with a fresh HYP_HOME the emitted log line is "plugin_command_count":0 and the rendered table has 23 rows with no session and no graph. This PR does not move it closer to closable, because the gap is environmental (the flow has no active plugins), not parsing. Closing it means giving the flow a config with an active plugin, which is a different smoke. Worth filing, low priority.
  • Nothing asserts the rows are sorted, which LLP 0009 #layered-help states outright. Confirmed still open by N6 (reversed rows: GREEN). This one is now essentially free, exactly as suspected: the rows are already parsed, they are all lowercase ASCII, and the renderer uses a plain .sort(), so a mirrored check is one clause. I would rather see it added here than filed:
      expect.that(
        `hypaware --help lists the command rows sorted (got rows=${helpRows.join(',') || '<none>'})`,
        helpRows,
        (v) => v.join('\u0000') === [...v].sort().join('\u0000')
      )

That would make this flow cover both halves of what LLP 0009 actually specifies, instead of one. Not ship-blocking; if triage ships as-is, file it together with the plugin-rows gap.

Also checked, clean

  • Regressions from round 1's table: all as required. B1, B2, B3, B7 still RED; B8 still GREEN (the point of the PR). Verified by injection, not by reading.
  • npm test: 4030 tests, 4029 pass, 0 fail, 1 skipped. npm run typecheck: clean (tsc --noEmit, no output). Fresh npm install before everything.
  • Blast radius: the changed file is referenced nowhere else in the repo (flows are discovered by filename), and the PR touches no runtime code, so no other flow's behavior can move. I did not re-run the full 13-flow battery; round 1 verified it at 12/13 with only walkthrough_picker_to_first_query red, fixed by the unmerged walkthrough_picker_to_first_query golden predates compose_with riders (#750) #757, and nothing in this diff can affect that. package_bin_boot itself I ran green at baseline and again after all injections were reverted.
  • Conventions: no semicolons in the added lines, zero U+2014 in the two commits, no @typedef, no inline import() types, JSDoc untouched. truncate is still used consistently by the exit-status assertions.
  • Comment accuracy: the new comment's claims all hold. status does appear in the epilogue (dispatch.js:791) and in the query row summary; daemon does appear in the join row summary and its own. The renderHelp row shape it describes (dispatch.js:781) is quoted correctly.
  • LLP 0009 re-read: #layered-help specifies one row per top-level token, sorted, subcommands collapsed into groups, and where a group row's summary comes from. It says nothing about the leading banner (so round 1's conclusion that dropping the banner pin is correct stands) and nothing about the row whitespace (which is finding 1's context). daemon and status remain LLP-anchored names, not arbitrary picks.
  • Worktree left clean (git status --porcelain empty, still detached at a92f338), my scratch directory removed, other agents' scratch untouched, /work/hypaware untouched.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - ship

Review budget exhausted with three residual findings. All three are preferences.
This PR can merge.
Deferred to #765.

The one that needed arguing rather than labelling

Finding 1 is that the row assertion's failure output never shows the actual --help
stdout, so in its most probable failure mode (the renderer's row whitespace drifts
while the table is fine) the entire diagnostic is got rows=<none> / value=[].
Triage reproduced that by injection rather than taking it on report: indent 2 to 3
spaces, and the smoke goes red with exactly that and nothing else.

The case for treating it as more than a nit is genuine: this is the sole guard
for top-level --help anywhere in the repo, the battery is hand-run rather than
CI-run, and the PR knowingly trades a pin on unpinned banner copy for a pin on
unpinned row whitespace, so that failure mode is the one this assertion was born to
hit.

What settled it against blocking: in every failure mode either reviewer or triage
constructed, the smoke's verdict is correct and it goes red. Nothing ships
wrong, no operator experiences anything, and the cost is confined to the releaser's
diagnosis loop, where the repro is one command they already have against the tree
that just failed. A diagnosability tax on a correctly-failing test is a test nicety.
Blocking a strict guard-strength improvement over the quality of its failure text
would invert the priorities, especially against the baseline: before this PR the
smoke was red on master across at least two PRs, and its predecessor assertion was
provably satisfiable by prose.

The sorted check: deferred deliberately, not overlooked

LLP 0009 #layered-help specifies rows are sorted, reversing them stays green
today, and the rows are already parsed, so the check is one clause. Round 2 said it
would rather see it added here than filed. Triage deferred it anyway, and gave the
reason: the budget is exhausted and the call is all-or-nothing, so the only way to
get it "here" is to block a PR that fixes a smoke currently red on master, over a
missing assertion for an invariant with no observed violation. It goes on #765 with
exact code so it lands as one trivial PR alongside finding 1.

Also settled

The 23-versus-24 row-count discrepancy between the two rounds: both are right and
23 governs
. renderHelp merges plugin-contributed rows, and this flow's fresh temp
HYP_HOME has plugin_command_count: 0; the 24 was measured in a developer home
with ai-gateway active. Plugin-active homes only ever add rows, so the floor can
never false-fire.

Verified at head a92f338 after a fresh install: npm test 4029 pass / 0 fail / 1
skipped, npm run typecheck clean, npm run smoke -- package_bin_boot green.

@philcunliffe
philcunliffe marked this pull request as ready for review August 14, 2026 01:22
@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 14, 2026
@philcunliffe
philcunliffe merged commit 3e2c4a6 into master Aug 14, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-758 branch August 14, 2026 17:50
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.

1 participant