Skip to content

Onboarding wizard cleanup: defaults gate, back navigation, enrolled default sync, CLI severity colour - #629

Merged
bgmcmullen merged 14 commits into
masterfrom
onboarding-wizard
Aug 5, 2026
Merged

Onboarding wizard cleanup: defaults gate, back navigation, enrolled default sync, CLI severity colour#629
bgmcmullen merged 14 commits into
masterfrom
onboarding-wizard

Conversation

@bgmcmullen

@bgmcmullenbgmcmullen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Cleans up the onboarding wizard and lands the design work in LLP 0181-0186:

  • Enrolled default sync with per-client opt-out (LLP 0181, supersedes 0132): on an enrolled machine, sources sync by default; a per-client opt-out (client-sync.json, src/core/usage-policy/client_sync.js) plus export-seam enforcement (src/core/runtime/source_withhold.js, cache withhold) keeps opted-out sources local.
  • One Reconfigure for every returning machine (LLP 0182): the returning-machine fork collapses to a single reconfigure pathway; a managed machine choosing the local pathway is asked once whether to disconnect (hyp leave).
  • Wizard defaults gate (LLP 0185): the wizard states its defaults up front and confirms; the sync menu's checkboxes now mark what syncs (locked sources shown read-only) — src/core/cli/wizard/sync_scope.js, reshaped pick.js.
  • Wizard back navigation (LLP 0186): steps can go back; escape means back where back exists (src/core/cli/tui/keypress.js, wizard orchestration in wizard/index.js).
  • CLI severity colour at the stream (LLP 0183): severity words (error:, warning:, note:) are coloured centrally in src/core/cli/style.js instead of per write site.
  • Shortens picker labels/summaries in the plugin manifests to match the new terse pick lane, and files LLP 0184 (reconciler retries permanent failures) as a known issue.
  • New smoke source_optout_export_withhold; new tests for back navigation, sync scope, style, client-sync store, and withhold build/export-drop.

Test plan

  • npm test: 3491 pass, 2 pre-existing failures also present on master (attach-enable-resume, usage-policy-fold), not touched by this branch.
  • Manifest tests updated to the shortened picker copy.

🤖 Generated with Claude Code

bgmcmullenand others added 3 commits August 4, 2026 14:59
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g LLP docs
Conflict resolutions:
- fork.js: keep master's hoisted managed derivation (truthful on the
invalid-config first-run path, #607) plus the branch's single
Reconfigure menu (LLP 0182); drop the retired scoped re-entry.
- pick.js: master's reconfigure-from-disk seeding (LLP 0183) becomes a
tier of the branch's seed concept: back-navigation selection >
on-disk config > detection. Gate and menu read the merged seed;
export choice and retention read back from the existing config.
- walkthrough.js: keep master's regenerated-from-picks overwrite prompt.
- index.js: fold master's first-run managed lock into the branch's
unconditional managed lock (the else-if was unreachable).
- Master's new reconfigure tests answer the branch's defaults gate with
'customize'; the managed-first-run test stays connected ('stay').
LLP renumbering (master's numbers are settled):
- 0181-enrolled-default-sync-with-client-optout -> 0188
- 0183-cli-severity-colour -> 0189
- 0185-wizard-defaults-gate -> 0190
- 0186-wizard-back-navigation -> 0191
References updated repo-wide; master's own 0181/0183/0185/0186 refs
untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

Merged master into the branch to resolve the conflicts. Two things worth a reviewer's eye:

  1. Wizard flow reconciliation. Master's reconfigure-from-disk seeding (its LLP 0183) is now a tier of this branch's seed concept in pick.js: back-navigation selection > on-disk config > detection. Master's reconfigure tests answer the new defaults gate with 'customize'; the gate showing the on-disk selection on a reconfigure (enter keeps it) is intended. The scoped re-entry master kept is gone per LLP 0182 (one Reconfigure for every machine), while master's managed-truthful-on-invalid-config fix (Returning gate keeps managed truthful when the merged config is invalid (#605) #607) is retained.
  2. LLP renumbering. Master minted its own 0181/0183/0185/0186 while this branch used the same numbers, so the branch docs moved: enrolled-default-sync -> 0188, cli-severity-colour -> 0189, wizard-defaults-gate -> 0190, wizard-back-navigation -> 0191. References updated repo-wide; master's refs to its own docs untouched, and the uniqueness test passes.

npm test: 3544 pass, 1 pre-existing failure (usage-policy-fold, also fails on clean master).

@bgmcmullenbgmcmullen added the neutral:adopt Foreign PR adopted into neutral's reconcile scope label Aug 5, 2026
@philcunliffephilcunliffe added the neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) label Aug 5, 2026
…`managed`
A join whose org-config converge times out returns `status: 'ok'` with no
`managed`: the sign-in completed, so the machine is enrolled, but nothing
landed to lock. Keying the disconnect offer and the sync-scope lane on
`managed` let that run step back to the fork, choose Local, and finish
with neither question asked - an enrolled machine default-syncing every
picked source with the wizard never saying so, against LLP 0191
#join-not-undone ("choosing Local keeps the org's locked rows and the
managed sync lane") and LLP 0188 #never-silent. The same read also handed
that machine the solo 120-day retention default. All four decisions now
read `enrolled()`, which is `managed || joined !== undefined`; `joined` is
already this function's enrollment record and is cleared only by the
fork's `hyp leave`.
Also:
- `datasetOwnedSourceIdsFromCatalog` unions dataset owners across every
contributing plugin instead of taking the first manifest's. The rule it
feeds is "withhold wholesale when *every* source that could have
produced it is opted out" (LLP 0188 #enforcement-scope), and first-wins
would drop a locked co-owner's rows that LLP 0188 #locked guarantees
always sync. Latent today: no bundled dataset has two contributors.
- Repoint prose and refs stranded by the LLP renumber. "pre-0181" now
names an unrelated service-manager decision, so it becomes "pre-0188";
three cites of the superseded LLP 0132 become LLP 0188, including
LLP 0182's claim that the picker still renders the `stays on this
machine` suffix, which this branch's own pick tests retire.
- Pin three untested rules: the TUI runtime's `backed` arm (removing it
hangs escape forever with a green suite), the sync gate's option
values, and `shouldWithholdDataset`'s `every` (every fixture had a
single owner, so `some` passed too).
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor

Neutral review round 1 of 2 (adopted PR, neutral:adopt)

Reviewed at 97d277f. Fixes pushed to onboarding-wizard as ac32e1d. This is the review record only; the verdict rung is the reconciler's, not this comment's.

At +6048/-650 across 76 files I did not attempt a uniform pass. I prioritised the flow control and the privacy seam: the wizard orchestrator's back edges, the defaults gate, usage-policy/client_sync.js, runtime/source_withhold.js, cache/source-withhold.js + storage.js, commands/policy.js, daemon/status.js's sync split, and tui/keypress.js/runtime.js. Then the new tests, then @ref honesty. Deliberately skipped: the plugin-manifest label churn, the 524-line smoke flow, query/overview.js, mcp/proxy.js, and the picker-copy test updates.

The design work here is unusually careful. The store's absent-vs-empty distinction, the corrupt-store fail-closed path, and the migration ordering in bootKernel all hold up under probing. The findings below are edges, not foundations.


Findings

1. HIGH - the sync lane and the disconnect offer keyed on managed, which diverges from "enrolled" (fixed)

src/core/cli/wizard/index.js:188,326 (pre-fix) and src/core/cli/wizard/join.js:109.

runWizardJoin returns { status: 'ok', lockedSources: [] } with no managed when the org-config converge times out ("Didn't hear back from your org's config in time"). The sign-in completed, so the machine is enrolled and hyp leave is its only exit, but nothing landed to lock. The orchestrator then set managed = join.managed === true -> false.

Forward that is harmless, because the sync lane also tests pathway === 'team'. But this PR adds back-navigation, and on the new path the two diverge: join (converge timeout) -> pick -> escape -> fork -> "Local install and configuration" gave neither the "This machine syncs to your team server. Disconnect and go local-only?" question nor the sync-scope lane. An enrolled machine finishes default-syncing every picked source with the wizard never saying so. It also took the solo 120-day retention default instead of the team 90-day one.

That contradicts the decision this PR lands. LLP 0191 #join-not-undone: "choosing 'Local' keeps the org's locked rows and the managed sync lane", and "Every enrolled-state narration keys on the remembered join rather than the final pathway". Plus LLP 0188 #never-silent. The right property was already tracked in the same function: joined, which gates narrateEnrolledAbort and the closing privacy narration and is cleared only by the fork's hyp leave.

Fixed by introducing enrolled() = managed || joined !== undefined and keying the disconnect offer, the sync lane, the retention default, and all three step-counter denominators on it. Verified with a new test in back_navigation.test.js that fails against the unpatched orchestrator (not ok 20) and passes with it.

2. MEDIUM (latent) - datasetOwnedSourceIdsFromCatalog used first-manifest-wins for a rule that needs a union (fixed)

src/core/runtime/source_withhold.js:246.

The rule it feeds is LLP 0188 #enforcement-scope: withhold an attribution-column-less dataset wholesale when every source that could have produced it is opted out. That is a union by construction. The fold copied its sibling's !out.has(name) first-wins guard (right for datasetAttributionColumnsFromCatalog, where a dataset has one column; wrong here), so it returned only the first contributing plugin's picker rows.

Two plugins sharing one dataset name with the opted-out one declared first would withhold the whole dataset, dropping rows from a source the user never opted out - and when the co-owner is the org's, withholding a locked source that LLP 0188 #locked guarantees always syncs. No bundled dataset has two contributors today (I checked all ten), so this is latent, but it is in the privacy-enforcement path.

Fixed to union. Two new tests in source-withhold-build.test.js, both verified to fail without the fix.

3. LOW - @ref and prose stranded by the LLP renumber (fixed)

The merge renumbered 0181-0186 to 0187-0191, and some references did not follow:

  • Five "pre-0181" prose references (src/core/runtime/boot.js:179,181, src/core/runtime/source_withhold.js:27,39, test/core/source-withhold-build.test.js:7, test/core/cli/wizard/pick.test.js:364, llp/0188-...md:56). LLP 0181 on this branch is the unrelated tests-never-drive-the-real-service-manager decision, which other live @refs legitimately cite. Corrected to "pre-0188".
  • src/core/cli/wizard/index.js cited bare LLP 0132 for the export-seam org boundary, and test/core/init-configured-entry.test.js:69 cited "LLP 0132 #never-silent" while describing the local-only default that 0188 flips. This PR marks 0132 Superseded-by LLP 0188. Both repointed.
  • llp/0182-...md:56 asserted the picker renders every non-org row with the stays on this machine suffix, citing LLP 0132 #never-silent. This PR's own pick.test.js:364 retires that suffix. Corrected (editorial: a stale cross-reference in a doc landing in this same PR, not a change to what it decided).

Structural @ref validation is otherwise clean: every added @ref LLP NNNN#anchor resolves to an existing file and anchor.

4. Test gaps closed

All three verified to fail against the unpatched code:

  • src/core/cli/tui/runtime.js:114, the state.status === 'backed' arm, had no test. Delete it and escape on every wizard prompt hangs forever (the promise never settles) with a fully green suite. Everything downstream was tested against a hand-constructed PromptBackRequestedError, so the fake stood in for exactly the untested link. Added four runtime.test.js cases driving real key bytes through the real io loop: escape -> back on an allowBack multiselect and select, ctrl+c still cancels on an allowBack prompt, and escape still cancels without the flag.
  • The sync gate branches on choice !== 'customize' and no test pinned the option values (sync_scope.test.js fed the literal 'customize' from the test). Renaming that row's value while the branch stands would silently turn "Select what to sync" into accept-the-defaults. Pinned.
  • shouldWithholdDataset's every was indistinguishable from some: every fixture gave the dataset a single owner. The new shared-dataset test distinguishes them.

Residual findings (not fixed)

R1. MEDIUM - the readline (non-TTY) multiselect inverts the sync menu's defaults.legacyNumberedPromptFactory (src/core/cli/walkthrough.js:108) returns [] on a bare enter regardless of pre-checked options, and renders no [x]/[ ] state at all. In the new sync menu checked means "syncs", so HYP_NO_TUI=1 + "Select what to sync" + enter opts every candidate out, the opposite of the TUI path, with the defaults invisible. Locked rows also print as ordinary selectable numbered rows there. I did not change it: the direction is fail-safe, the user actively chose to open the menu, and !trimmed -> [] is master behaviour shared with the pick lane, so flipping it has blast radius past this PR. Worth a follow-up that either honours the pre-checked defaults on enter or refuses a bare enter in the sync menu specifically.

R2. LOW - ctrl+c at the disconnect question re-presents the fork rather than cancelling.wizard/index.js: the isPromptCancelledError arm falls through to the same continue as the back arm. LLP 0190 #fork-disconnect settles that "No, or a cancel, keeps today's behavior", which reads as "proceed down the local pathway"; LLP 0191 #esc-back separately says ctrl+c cancels regardless of the flag. Harmless in practice (ctrl+c at the fork still quits) and the LLP text is genuinely ambiguous between the two readings, so I left it for the author.

R3. LOW - a row with a null or empty attribution value is exported even when its source is opted out.shouldWithhold requires a non-empty string, so an unattributed row in an attributed dataset escapes both the per-row filter and the dataset-scoped one. Inherited from LLP 0132, but the promise is stronger under 0188 (an explicit user opt-out, not a derived class), so it is worth confirming every writer populates attribution_column.

R4. LOW - pre-existing broken @ref anchors, not introduced here but sitting in files this PR rewrites heavily: six in src/core/commands/policy.js (0103#cli, 0103#reporting at :44, :229, :265, :266, :303, :350), plus core_commands.js:429 (0138#one-command), walkthrough.js:1375 (0138#one-materializer), wizard/index.js:564 (0101#deadline), central.js:507 (0107#reversal), sync.js:41 (0101#no-release).

R5. Coverage shape - no test drives the wizard from a keystroke to a result.back_navigation.test.js and index.test.js stub gate/fork/join/pick/syncScope/configure/finaleRunner, so fork.js, join.js, pick.js and sync_scope.js never execute in an orchestrator test; the lane tests in turn inject prompt/confirm. Each layer is tested against a fake of the next, which is a legitimate scope but is how finding 1 and the backed-arm gap both survived. Also untested: pick.js:464 (the "allowBack but no gate" back-propagation, where a regression would spin forever), both policy client --json shapes, and style.js's documented ordered-first-match rule (style.test.js binds colours from the module under test, so swapping the values of ANSI.red and ANSI.yellow passes the file).


Numbers

result
npm test at 97d277f3547 tests, 3546 pass, 0 fail, 1 skipped
npm test at ac32e1d3555 tests, 3554 pass, 0 fail, 1 skipped
npm run typecheckexit 0, clean, both revisions

The PR description's "2 pre-existing failures also present on master" did not reproduce; the suite is fully green at both revisions with node_modules linked into the worktree.

Conventions checked across the whole diff: no U+2014 anywhere, no statement-terminating semicolons, no inline import('...') JSDoc types, no @typedef, and all eight added @import specifiers root-anchored.

…drives the whole wizard
Review round 2 of PR #629.
Ctrl+C at "Disconnect and go local-only?" shared the back arm's `continue`,
so it re-presented the fork instead of ending the run: "get me out" became
the first of two keystrokes, which is the shape LLP 0191 #esc-back
separates ctrl+c from escape to avoid, and the run had no single-keypress
exit from the fork/disconnect loop. It now cancels like every other wizard
prompt (exit 130, the enrolled-abort narration when a join stands), and
still never disconnects, which is all LLP 0190 #fork-disconnect asks of a
cancel. LLP 0190 is a Draft; its #fork-disconnect clause is amended to say
which of the two readings holds.
Adds the wizard's first keystroke-to-result test. Every existing wizard
test drives one layer against a fake of the next - the orchestrator with
the lanes scripted, the lanes with `prompt`/`confirm` injected - so no
test ever ran the fork, the pick lane and the sync lane inside an
orchestrator run. Both bugs found in review sat in exactly that gap. The
new test scripts real answers into the real readline prompts and stubs
only the two phases that would leave the machine (the browser login, the
configure/first-look command execution): join, back to the fork, local,
stay connected, record all, sync all, config on disk. It fails against the
pre-round-1 `managed`-as-enrolled orchestrator.
Also: register `source_optout_export_withhold` in the release smoke
battery (its sibling `local_only_export_withhold` was already there), and
drop a stray double blank line left in `query/overview.js` by the move to
the shared palette.
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor

Neutral review round 2 of 2 (adopted PR, neutral:adopt) - last round before triage

Reviewed at ac32e1d. Fixes pushed to onboarding-wizard as e0e60d1. Review record only; the verdict rung is the reconciler's.

Round 1 prioritised the flow control and the privacy seam and deliberately skipped five areas. This round covered those, re-derived R1-R5 rather than inheriting them, and audited round 1's own enrolled() helper at every call site. One MEDIUM finding fixed (a behavior the branch explicitly tested - see the caveat), plus the end-to-end wizard test round 1 argued for.


Verdict on enrolled() (round 1's own fix, re-derived)

enrolled() = managed || joined !== undefined (wizard/index.js:158). Correct at all seven sites. Enumerated, with what each actually needs:

sitereadsneedsverdict
:208 disconnect offeris this machine enrolledsame
:299pickProgressdoes the itinerary include syncsame
:300syncProgresssamesame
:325 retention defaultis the durable copy the org'ssame
:361 sync-lane gateis anything going to syncsame
:398finaleProgressitinerary lengthsame
:442joinedAlreadydid this run sign injoined alone, and it uses joined

Divergence probes on joined !== undefined itself:

  • False negative (enrolled but neither flag): the only way in is gate.managed = !!(report.layered && report.layered.hasCentral) (fork.js:159) reading false on an enrolled machine. An enrolling login always writes a central seed (enrollCentralSink), and hyp leave's own "not connected" test is the same resolveCentralLayerPath() === null (central.js:394), so a hand-authored local-layer central sink is consistently not enrollment across both. No divergence.
  • False positive: a login the server mints no gateway for (remote_commands.js: if (!session.gateway) return 0) exits 0, so the join lane returns status: 'ok' and joined is set on a query-only machine. Pre-existing on the team pathway (pathway === 'team' already drove the sync lane, the 90-day default and the privacy narration there on master), and on the new local path it self-heals: "yes, disconnect" runs hyp leave, which prints "not connected - nothing to do", returns 0, and clears managed/locked/joined - the correct end state. Not worth a change.

:298's ...(managed ? { managed } : {}) into the pick lane correctly stays raw managed: pick.js:155 uses it only as a span attribute meaning "a central layer owns rows here", which is what managed says.

Signature-failure check passed. The helper is not a new named-state proxy; it is the disjunction of the two states that each cover one half of the property, and the residual gap in each is closed by the other.


Findings

1. MEDIUM - ctrl+c at the disconnect question was a back-step, not a cancel (fixed; reverses an explicitly-tested behavior)

src/core/cli/wizard/index.js:225-227 (pre-fix). The isPromptCancelledError arm shared the back arm's continue, so ctrl+c at "This machine syncs to your team server. Disconnect and go local-only?" re-presented the fork. Two consequences:

  • It makes ctrl+c into exactly the "mouse-path through N back-steps" that LLP 0191 #esc-back separates it from escape to avoid ("Ctrl+C cancels regardless of the flag, so 'get me out' never needs the mouse-path"), and LLP 0191 #consequences says flatly "A cancelled run still exits 130 with the same narrations".
  • Practically, fork -> local -> ctrl+c -> fork -> local -> ctrl+c is a loop with no single-keypress exit; the only way out is a second ctrl+c aimed at the fork.

Round 1 left this as R2 calling the LLP text ambiguous. Re-deriving it: LLP 0190 #fork-disconnect's "No, or a cancel, keeps today's behavior: the org's rows stay locked and the machine stays enrolled" is a sentence about the disconnect decision, and every clause of it is satisfied by cancelling the run - nothing is disconnected. The current code satisfied neither LLP: not 0190 (which never mentions re-presenting the fork) and not 0191 (which says a cancel exits 130).

Fixed to the wizard's standard cancel: exit 130, cancelled: true, the hyp init: cancelled line, and narrateEnrolledAbort when a join stands (LLP 0190 #abort-narration). LLP 0190 is Status: Draft, so its #fork-disconnect clause is amended to say which reading holds and to name escape and ctrl+c separately.

Caveat for triage / @bgmcmullen:test/core/cli/wizard/index.test.js carried runInitWizard: cancelling the disconnect question returns to the fork, i.e. the old behavior was deliberate, not an oversight. That test is rewritten. If the intent really was "a cancel proceeds down the local pathway" (the other reading of 0190), the revert is one commit and the LLP amendment should go with it. I made the call rather than leaving it a second time because this is the last review round.

2. LOW - picker copy drops the disclosure that a pick changes the machine (not fixed - product-copy call)

hypaware-core/plugins-workspace/*/hypaware.plugin.json. The shortened summaries are accurate and read much better, but three of them dropped the only statement the wizard makes about what picking the row does:

rowdropped
claude"Configures Claude Code, installs Claude helper skills"
codex"Configures Codex to use the local gateway" (writes ~/.codex/config.toml)
otel"Starts a local OTLP HTTP receiver" (binds 127.0.0.1:4318)

This matters more than it did before because of this PR: LLP 0190 #pick-gate puts a one-keypress "Record all" in front of the menu and states explicitly that the gate's rows carry bare labels, no summaries. So the happy path is now enter, enter, and the config-mutating side effects are named nowhere - the picker summary was the disclosure, and the finale asks consent for backfill (walkthrough.js:286) but not for attach. No LLP mandates the disclosure, so I did not rewrite product copy the PR body says was tuned deliberately; a clause each ("...; points Codex at the local gateway.") would restore it without losing the terseness. Maintainer's call.

3. LOW - the new smoke was not in the release battery (fixed)

AGENTS.md:215 (CLAUDE.md is a symlink to it). local_only_export_withhold is listed; its new sibling source_optout_export_withhold was not, so the only flow that exercises the LLP 0188 seam end to end would not have run at release. Added. Verified green: smoke source_optout_export_withhold: ok.

4. LOW - stray double blank line in query/overview.js (fixed)

src/core/query/overview.js:255, left by the deletion of the private ANSI/paint pair.

5. Coverage gap closed - the wizard now has one keystroke-to-result test (R5)

test/core/cli/wizard/back_navigation.test.js. Round 1's R5 was that every layer is tested against a fake of the next - the orchestrator with gate/fork/join/pick/syncScope scripted, the lanes with prompt/confirm injected - so fork.js, pick.js and sync_scope.js never execute inside an orchestrator run, which is how both of round 1's real findings survived.

Added one run with no lane stubs: real fork, real pick lane, real disconnect question, real sync lane, real config commit, driven through the real readline prompt factories by scripted answers (Choose [1-3...], select (e.g. 1,3, "all", or b to go back):, select [1, b back]:). Only the two phases that would leave the machine are replaced - join's browser login (in the converge-timeout shape) and configure/firstLook's command execution. The script is: join a team, b back from the pick menu, local, stay connected, all, sync all.

Verified it earns its keep: reverting round 1's helper to const enrolled = () => managed fails this test independently of the unit test written for that bug.


R1-R5 re-derived

  • R1 - readline multiselect inverts the sync menu's defaults: confirmed, downgraded to LOW.legacyNumberedPromptFactory (walkthrough.js:105) still returns [] on a bare enter and renders no [x]/[ ], so HYP_NO_TUI=1 + "Select what to sync" + enter opts every candidate out. Two things round 1 did not weigh: the direction is fail-safe, and the outcome is not silent - sync_scope.js:112 prints Keeping local-only: <names>. Change later with 'hyp policy client ...', so LLP 0188 #never-silent is satisfied. The genuine defect left is display: buildPickOption sets checked: true and the legacy renderer discards it. Not fixed - the renderer is shared with runPickerWalkthrough and the pick lane, several tests assert its exact bytes, and churning shared output in the last review round is the wrong trade.
  • R2 - fixed. See finding 1.
  • R3 - confirmed, and less theoretical than round 1 could tell.shouldWithhold (cache/source-withhold.js:49) requires a non-empty string, and shouldWithholdDataset returns false for any dataset that has an attribution column, so an unattributed row escapes both. The only attributed bundled dataset is ai_gateway_messages, whose client_name is nullable: true (ai-gateway/src/message_projector.js:46) and is left null by stringValue(ctx.projection.client_name) whenever the request carries no client header - which is precisely the raw-anthropic / raw-openai picker rows. So on a machine with an opt-out, raw-gateway traffic ships regardless. Not fixed: the coherent rule is shouldWithholdDataset's every applied to null-attribution rows, and LLP 0188 #enforcement-scope is Accepted and explicitly scopes the wholesale rule to datasets without an attribution column. Needs a new LLP.
  • R4 - confirmed unchanged, all pre-existing. LLP 0103 has no #cli or #reporting section (its headings are Context / Decision / The surface matrix, settled / Consequences), so the six refs in commands/policy.js (:44, :229, :265, :266, :303, :350) plus core_commands.js:429, walkthrough.js:1375, wizard/index.js:564, central.js:507, sync.js:41 dangle. All present on origin/master; none introduced here. 0031#local-layer-writers is not among them - it resolves via the ## Local-layer writers heading slug.
  • R5 - addressed. See finding 5. Still open from round 1's list and not attempted here: pick.js:464's allowBack-without-gate back-propagation, both policy client --json shapes, and style.js's ordered-first-match rule (style.test.js:32-35 binds RED/YELLOW/DIM from the module under test, so swapping the values of ANSI.red and ANSI.yellow still passes the file).

The four areas round 1 skipped

  • Plugin-manifest label churn - reviewed all seven manifests. Copy is accurate: LLP 0141's requirement that the Codex row cover Desktop survives in the summary, LLP 0118 R8 is an implementation constraint and not a copy one, and no code resolves a picker row by label. test/core/config.test.js:368's old-label assertion is a local fixture, not the real manifest. One finding, [codex] Remove OpenTelemetry npm dependencies #2 above. Editorial: LLP 0135's example manifest at :362 still shows "label": "capture Claude Code conversations" - an illustrative snippet in an Accepted design doc; harmless, left alone.
  • source_optout_export_withhold (524 lines) - read in full and executed. Genuinely end-to-end for the seam: production buildSourceWithholdResolver, real sink driver, real @hypaware/central forward sink against a fake server, the real hyp policy client verb writing the real store, and three ticks that distinguish default-sync / withhold / no-retroactive-ship, plus a cache-intact check and a telemetry assertion on usage_policy.export_drop. Passes. Only gap was that nothing would run it (finding 3).
  • src/core/query/overview.js - the private ANSI/paint pair is replaced by the shared ones; style.js's ANSI is a superset (bold/dim/cyan/magenta/reset all present) and paint(text, sgr, on) is signature-identical, so the render is byte-identical. One cosmetic fix ([codex] add PR checks #4).
  • src/core/mcp/proxy.js - hyp mcp: proxying … to note: mcp proxying … is correct under LLP 0189's rule that hyp <cmd>: on stderr means a diagnostic, is documented in LLP 0189:114, and nothing greps the old string.
  • Picker-copy test updates - openclaw-manifest.test.js, walkthrough-prompt.test.js, walkthrough-tui-happy.test.js, client-provenance.test.js, pick.test.js. All follow the copy honestly rather than being loosened to hide a change; the two relaxations (/sweep/i to /session history/i, /claude-cli\/<model>/ to /claude-cli/) match copy that really did drop those words. pick.test.js's retirement of the · stays on this machine suffix is the correct read of LLP 0188 superseding 0132.

Residual findings

  1. LOW - picker rows no longer disclose that picking them edits the user's client config or binds a port (finding 2). Interacts with LLP 0190's one-keypress gate, which shows no summaries at all. A clause per row restores it. Maintainer's call; no LLP requires it today.
  2. LOW - HYP_NO_TUI=1 sync menu: a bare enter opts everything out, and no checkbox state is rendered (R1). Fail-safe and narrated, so not urgent; the fix is a display change to the shared legacy renderer plus a decision about what a bare enter means there, and it has blast radius past this PR.
  3. LOW - null/empty attribution rows escape both withhold filters (R3). Concretely: raw-gateway traffic with no client header ships even when a source is opted out. Needs a new LLP extending 0188 #enforcement-scope.
  4. LOW - eleven pre-existing broken @ref anchors (R4), six of them in commands/policy.js, a file this PR grows by 227 lines. Not this PR's regression; every @ref it adds resolves (28 distinct, validated against explicit ids, {#anchor} and heading slugs).
  5. Coverage - three specific gaps remain (R5 tail): pick.js:464's back-propagation, policy client --json, and style.js's ordered-first-match, whose test is tautological because it binds the colour constants from the module under test.
  6. Process - four of the LLPs this PR lands are still Draft (0190, 0191; 0182, 0188, 0189 are Accepted). Not a defect, but the two Drafts are the ones that settle the wizard's flow control, and finding 1 exists because a clause in one of them admits two readings.

Numbers

result
npm test at ac32e1d3555 tests, 3554 pass, 0 fail, 1 skipped
npm test at e0e60d13557 tests, 3556 pass, 0 fail, 1 skipped
npm run typecheckexit 0, clean, both revisions
npm run smoke -- source_optout_export_withholdok

Both runs with node_modules linked into the worktree; the PR description's two "pre-existing failures also present on master" do not reproduce at either revision.

Conventions re-checked across everything added this round: no U+2014, no statement-terminating semicolons, no @typedef, no inline import('...') types, root-anchored type-import specifiers.

@philcunliffephilcunliffe added the neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides) label Aug 5, 2026

@philcunliffephilcunliffe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two review rounds are complete at e0e60d1. Neutral healed what it could under neutral:adopt and pushed the fixes to this branch: a HIGH enrolled-flag divergence, a MEDIUM ctrl+c loop, a latent locked-source union bug, LLP renumber fallout, and five test gaps including the wizard's first keystroke-to-result test. Full suite is 3557 tests, 0 failures; typecheck clean.

Requesting changes on three items neutral deliberately did not decide for you. None is a defect neutral could fix without making a product or design call that is yours.

1. Confirm or revert the ctrl+c semantics (neutral changed behaviour you had explicitly tested)

Ctrl+C at "Disconnect and go local-only?" used to re-present the fork. It now exits 130 with the enrolled-abort narration.

The reason: sharing the back arm's continue made ctrl+c into exactly the mouse-path through N back-steps that LLP 0191 #esc-back separates it from escape to avoid, and left fork to local to ctrl+c as a loop with no single-keypress exit. As written, the old behaviour satisfied neither LLP: not 0190 (which never mentions re-presenting the fork) and not 0191 (which says a cancel exits 130).

But your branch carried a test pinning the old behaviour (runInitWizard: cancelling the disconnect question returns to the fork), so this was a deliberate choice, not an oversight. Neutral rewrote that test and amended LLP 0190's #fork-disconnect clause (still Draft) to name escape and ctrl+c separately.

If you meant the other reading of 0190 - that a cancel proceeds down the local pathway - the revert is one commit and the LLP amendment should go with it. Neutral made the call rather than leaving it a second time because this was the last review round, but it is your call to reverse.

2. Null-attribution rows escape both source-withhold filters (needs a new LLP)

Confirmed live, not theoretical: client_name is nullable: true and raw-gateway rows carry no client header, so a row with no attribution passes both the opt-out and locked-source filters.

Neutral did not fix this because closing it changes what LLP 0188 settled, and 0188 is Accepted - per CLAUDE.md that needs a new LLP extending it, not an edit. Outside what the neutral:adopt grant can heal.

3. The picker no longer discloses that a pick changes the machine (product copy)

The shortened plugin summaries are accurate and read better, but three dropped the only statement the wizard makes about what picking the row does:

rowdropped
claudeinstalls Claude helper skills
codexwrites ~/.codex/config.toml to point Codex at the local gateway
otelbinds a local OTLP receiver on 127.0.0.1:4318

This matters more because of this PR: LLP 0190 #pick-gate puts a one-keypress "Record all" in front of the menu with bare labels and no summaries, so the happy path is enter, enter, and the config-mutating side effects are named nowhere. The finale asks consent for backfill but not for attach.

No LLP mandates the disclosure and the PR body says this copy was tuned deliberately, so neutral did not rewrite it. A clause each ("...; points Codex at the local gateway.") would restore it without losing the terseness.

Also noted, no action requested

  • 11 pre-existing broken @ref anchors in policy.js and elsewhere, none introduced here. All 28 anchors this PR adds resolve.
  • Three residual coverage gaps remain after the new end-to-end test; the review comments name them.
  • The PR body claims 2 pre-existing test failures. They did not reproduce at either head.

Neutral will re-engage on its next tick after you push or reply. It will not merge or mark this ready - that stays yours.

@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

Re review round 2, finding 1 (ctrl+c at the disconnect question): confirmed, keep the fix. Cancel-means-exit-130 is the right reading - LLP 0191 #esc-back says ctrl+c cancels regardless of the flag, and the old behavior's fork -> local -> ctrl+c loop had no single-keypress exit. The rewritten test and the LLP 0190 #fork-disconnect amendment stand; no revert.

🤖 Generated with Claude Code

bgmcmullenand others added 4 commits August 5, 2026 11:02
The legacy numbered prompt gains an opt-in (enterKeepsChecked): render
each row's [x]/[ ] state and return the checked set on a bare enter,
mirroring the TUI multiselect. Only the sync menu sets it - there
checked means "syncs", so the fallback's historical enter-selects-none
silently opted every candidate out, the exact inverse of the TUI
default. The pick menus and runPickerWalkthrough keep the historical
semantics byte-identical (LLP 0190 promises the legacy walkthrough is
untouched). LLP 0190 #sync-gate amended (still Draft).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#631)
A row with no usable attribution value in an attributed dataset fell
between both withhold rules and always shipped, whatever the opt-out
store said - systematic for raw-openai (the codex projector's deliberate
refusal to guess, LLP 0151) and masked as 'claude' for raw-anthropic.
New shouldWithholdUnattributed: once ANY of the dataset's contributing
sources is opted out, an unlabeled row is withheld (some, not every -
it cannot be proven to belong to a synced source). Inert with no
opt-outs standing. Drops counted separately on usage_policy.export_drop
(dropped_unattributed_row_count) so the over-withholding is observable.
LLP 0192 records the gap, this interim rule, and defers the real fix
(attribution of last resort at capture) to its own decision - tracked
as issue #631. LLP 0188 #enforcement-scope gains the Extended-by ref.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iew finding 2)
The shortened copy dropped the only statement of side effects; with the
LLP 0190 gate making enter-enter the happy path, nothing named them.
One terse clause each: claude attaches and installs helper skills,
codex is pointed at the local gateway, otel starts a local receiver.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- style.test.js pinned colours off the module under test, so swapping
ANSI.red/yellow passed; the palette bytes and the ordered-first-match
rule are now pinned literally.
- pick.js's allowBack-without-gate back arm (a regression loops forever)
gets a propagation test.
- Both `hyp policy client --json` shapes pinned as whole parsed objects.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

Pushed 49fd628..56db7a2 addressing everything left open by the two neutral review rounds:

  • R1 (non-TTY sync menu inverts defaults) - fixed (49fd628). The legacy numbered prompt gains an opt-in enterKeepsChecked: it renders each row's [x]/[ ] state and a bare enter keeps the checked set, mirroring the TUI multiselect. Only the sync menu sets it; the pick menus and runPickerWalkthrough stay byte-identical per LLP 0190's "legacy walkthrough untouched". LLP 0190 #sync-gate amended (Draft). Tested at the prompt level and end to end through the real readline factories (fresh run and opt-out round-trip).
  • R3 (unattributed rows escape the opt-out) - interim fix (bb857a1). New shouldWithholdUnattributed: a row with no usable attribution value in an attributed dataset is withheld once ANY of that dataset's sources is opted out (fail closed - some, not every; inert with no opt-outs). Drops surface as dropped_unattributed_row_count on usage_policy.export_drop. The capture-side attribution decision (stamp raw traffic raw-anthropic/raw-openai; also covers the 'claude'-fallback misattribution half R3 noted) is deliberately deferred: filed as LLP 0192 + issue Unattributed rows escape the per-client sync opt-out (raw-anthropic / raw-openai) #631, with the Extended-by ref on LLP 0188 #enforcement-scope.
  • Finding 2 (picker copy dropped the side-effect disclosure) - restored (cbb4de4) as one terse clause per row: claude "attaches Claude Code and installs helper skills", codex "points Codex at the local gateway", otel "starts a local receiver".
  • R5 leftovers - closed (56db7a2): style.js palette bytes + ordered-first-match pinned literally (the red/yellow-swap hole), pick.js's allowBack-without-gate back-propagation tested (the would-spin-forever arm), both policy client --json shapes pinned as whole objects.
  • Ctrl+c reversal - accepted above; the rewritten test and LLP 0190 amendment stand.
  • R4 (pre-existing broken @ref anchors) - deferred; all six predate this branch and live on master.

npm test: 3567 pass, 1 pre-existing flake (usage-policy-fold, also red on clean master). npm run typecheck clean. Smokes re-run green: source_optout_export_withhold, walkthrough_picker_to_first_query, cli_bundled_plugins_activated.

🤖 Generated with Claude Code

@philcunliffephilcunliffe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neutral verdict at 56db7a2 - changes requested

Reviewed at 56db7a2. CI is fully green (test 22/24, typecheck 22/24, duplicate-numbers) and the PR is MERGEABLE.

Neutral is at its 2-round review cap, so this hands the residuals back rather than pushing more fixes. I did not re-post the previous verdict: the four commits since e0e60d1 were verified individually against the tree, and most of round 2's residuals are genuinely closed. What follows is the state at this head, not the old list.


Verified closed

R1 - non-TTY sync menu bare enter (49fd628): resolved, and the blast radius round 2 was worried about is genuinely contained. The change is gated behind an opt-in enterKeepsChecked flag (src/core/cli/walkthrough.js:97,120-122) set at exactly one production site (wizard/sync_scope.js:208). For the other two callers of the shared factory (wizard/pick.js:177, walkthrough.js:485) the output is byte-identical, including the allowBack prompt line. Confirmed at runtime, not just by reading: driving the real defaultPromptFactory with non-TTY streams, "\n" now returns the checked defaults instead of []. Tests were added, not loosened.

R5 (a) pick.js back-propagation (56db7a2): resolved. test/core/cli/wizard/pick.test.js:214 drives the real runWizardPick down the genuine allowBack-without-gate arm. Mutation-tested: removing the if (hasGate) guard makes it fail.

R5 (b) policy client --json (56db7a2): resolved. test/core/policy-command.test.js:746 goes through the real command registry and deepEquals whole parsed objects, so renames and extra fields are both caught. Mutation-tested: renaming state/path, and collapsing the state ternary, each fail it.

R5 (c) the style tautology: the named tautology is broken. test/core/cli/style.test.js:42 pins the palette as hard-coded literal bytes (\x1b[31m) rather than binding ANSI.* from the module under test. Mutation-tested: swapping the values of ANSI.red and ANSI.yellow, the exact mutation round 2 said survives, now fails.


Residual - 1 MEDIUM, 3 LOW

1. MEDIUM - the unattributed-row rule is narrower than LLP 0192 claims (bb857a1)

The named hole is genuinely closed and it fails closed: shouldWithholdUnattributed (src/core/cache/source-withhold.js:70-77) plus the routing at src/core/cache/storage.js:322-339 drops null/empty-attribution rows when an owning source is opted out, with real end-to-end tests (test/core/source-withhold-export-drop.test.js:219-254) that fail when reverted. LLP hygiene is right too: 0192 is a new Draft issue doc and 0188 got only a mechanical forward-ref inside #enforcement-scope.

The gap is the arming condition. Owners come from datasetOwnedSourceIdsFromCatalog (src/core/runtime/source_withhold.js:257-281), which reads manifest contributes.datasets. Folding the real bundled manifests gives:

ai_gateway_messages -> ['raw-anthropic', 'raw-openai']

@hypaware/claude and @hypaware/codex declare no datasets, yet their traffic lands in ai_gateway_messages via adapter projectors. So a user who opts out codex, openclaw, or claude while leaving the raw rows checked gets no fail-closed protection at all for unattributed rows, which is exactly the class LLP 0192 says "cannot be proven to belong to a synced source". The implementation and its own stated principle disagree.

This is still a strict improvement on the status quo, so it is not a regression. But 0192 currently overstates the protection, and no test binds the rule to the real bundled manifests, which is why the gap is invisible. Either broaden the owner set to include sources whose plugins write into the dataset, or narrow 0192's claim to what the code does.

Two smaller notes for #631 while you are in here:

  • 0192's rationale for some over every ("many contributors, a single synced client would keep all anonymous rows shipping") is factually wrong for the real catalog, which has exactly two owners, both raw rows. The direction chosen is still the safe one.
  • readRowsSince is also the read seam for local-fs/iceberg exports, so over-withheld rows silently vanish from local parquet exports too, not only the central forward. Pre-existing behaviour of 0188, amplified here.

2. LOW - the gate still does not disclose, which was the crux of the finding (cbb4de4)

The three summaries are restored and accurate; I verified the side effects are real (codex writes ~/.codex/config.toml, otel binds 127.0.0.1:4318, claude attach installs skills). But wizard/pick.js:419-432 builds the gate from d.label only and never reads d.summary, and neither gate option carries one. test/core/cli/wizard/pick.test.js:170 pins the options to exactly ['Record all', 'Select what to record'].

So disclosure now lands on the menu path and still not on the gate path, which LLP 0190 §consequences names as the common case ("enter, enter, finale"). The gate only appears because detection found Claude Code / Codex / OpenClaw, so the higher the side-effect load, the more likely the user never sees the restored copy. A one-line summary on the accept option would close it without touching 0190's settled "bare labels" rule for the rows. Alternatively, say explicitly that the gap is accepted.

Also in this family:

  • OpenClaw was a fourth row with the same problem and was not restored.openclaw/hypaware.plugin.json:38 reads "Live traffic plus local session history."; its attach rewrites ~/.openclaw/openclaw.json and needs a manual openclaw gateway restart (openclaw/src/attach.js:25,45,50). The pre-PR copy at least said "once attached".
  • The codex clause names the gateway but not the eight skills it installs (codex/hypaware.plugin.json:24,49-56), where the claude clause does name skill install. Asymmetric disclosure of an identical effect.
  • No test asserts any of the new clauses, which is how the copy was lost the first time.

3. LOW - the ordered-first-match test added for R5 (c) is vacuous (56db7a2)

test/core/cli/style.test.js:49-58 cannot distinguish rule order, because in both inputs the competing rule fails to match at all: /^([^:]*\bfailed:)/ cannot cross the colon in 'error: attach claude failed: boom', and /^(error:)/i is defeated by the leading spaces in ' error: quoted...'. Mutation-tested: moving the failed: rule to position 0 of RULES, the maximal ordering violation, leaves the file at 19 pass / 0 fail.

The regression it should catch is real and user-visible:

ORIGINAL paintLine(' attach claude failed: boom') -> " attach claude failed: boom"
MUTATED paintLine(' attach claude failed: boom') -> "�[31m attach claude failed:�[0m boom"

One assertion closes it, and it matches the test's own comment about indented continuation lines:

assert.equal(paintLine(' attach claude failed: boom'),' attach claude failed: boom')

(The palette byte-pinning in the same commit is good and does its job. This is only the ordering half.)

4. LOW - malformed answer at the non-TTY sync menu still opts everything out silently

Pre-existing, not introduced by 49fd628, and arguably out of scope. The index parser drops unparseable tokens and returns [], which the sync menu reads as "nothing syncs": "y", "9", "none", "0,5" all yield [] with no validation error (src/core/cli/walkthrough.js:123-129). The bare-enter case, the likely one, is fixed and the rendered [x] makes the default visible, so this is a follow-up rather than a blocker: re-prompt when an answer parses to zero valid indices.


Carried forward unchanged

  • Eleven pre-existing broken @ref anchors (round 2, R4), six in commands/policy.js. Present on origin/master, not this PR's regression. Every @ref this PR adds resolves.
  • Four of the LLPs this PR lands are Draft (0190, 0191, and now 0192). Not a defect.
  • Editorial: test/core/cli/style.test.js:41 has // @ref LLP 0189#palette [tests]: with an empty gloss.

What unsticks this

Items 2, 3 and 4 are small and self-contained. Item 1 is the one that wants a decision rather than a patch: either broaden the owner set, or narrow LLP 0192's claim so the doc matches the code. Neutral has spent its review rounds on this PR, so it will not push further fixes here; push to the branch and the ladder re-opens at the new head, or reply on this thread if you would rather hand any of these to a follow-up issue.

Nothing here blocks a maintainer who wants to merge as-is and track item 1 in #631. Neutral does not merge contributors' PRs, so that call is yours.

bgmcmullenand others added 4 commits August 5, 2026 11:34
…he real manifests (verdict item 1)
The rule arms on the dataset's manifest-declared owners, which for the
bundled catalog is exactly the raw rows - a client-only opt-out does not
arm it, and the doc now says so and why that is the intended scope (an
unattributed row is in practice raw traffic; the raw rows' sync state is
its governor; the metadata-lossy client row residual belongs to #631).
Replaces the factually wrong many-contributors rationale and names the
local-export amplification (readRowsSince is the shared export seam).
A new test pins the arming set against the real bundled manifests so
drift fails loudly instead of silently changing scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a test pins it (verdict item 2)
The pick gate's accept option now carries the one-line configures-your-
tools summary: the gate is the happy path (enter, enter, finale), so the
menu-row disclosures alone were never guaranteed to be seen. Rows stay
bare per LLP 0190 #pick-gate, amended (Draft). OpenClaw's summary gets
its side effect back (attach rewrites its gateway config) and codex now
names its skill install like claude does. picker-disclosure.test.js pins
each side-effect admission so the copy cannot silently vanish again.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m 3)
' attach claude failed: boom' matches both the continuation rule and
the failed: rule, so only the RULES order keeps it plain - hoisting
failed: to the top now fails the test (mutation-verified). The previous
assertions never had two competing matches on one line. Also fills the
empty @ref gloss.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"y", "0", or an out-of-range index used to fall through to [] - in the
sync menu that silently opted every candidate out. The fallback now
prints what happened and asks again; "none" becomes the explicit empty
selection so deliberate opt-everything-out stays one word, and a
partially valid answer still wins without a re-ask. Valid inputs, bare
enter, "all" and "b" are byte-identical. LLP 0190 #sync-gate amended.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

Pushed e8d65f1..991388d addressing the verdict at 56db7a2, all four items:

  1. MEDIUM (arming condition) - resolved by narrowing the claim, per the verdict's either/or (e8d65f1). The maintainer call: the owner set stays the manifest-declared owners, and LLP 0192 #fail-closed now states that precisely and argues it as the intended scope - an unattributed row is in practice raw traffic, so the raw rows' own sync state is its governor; a codex-only opt-out already withholds every codex-labeled row, and the unlabeled rows it leaves syncing belong to raw sources the user left checked. The metadata-lossy-client-row residual is named and assigned to Unattributed rows escape the per-client sync opt-out (raw-anthropic / raw-openai) #631. The factually wrong many-contributors rationale is replaced, the local-export amplification is named, and a new test pins the arming set against the real bundled manifests (ai_gateway_messages -> ['raw-anthropic','raw-openai']) so a manifest change reopens the question loudly.
  2. LOW (gate disclosure) - the accept option now carries a one-line summary ("Configures these tools to record through HypAware; the menu rows say how."), rows stay bare, LLP 0190 #pick-gate amended (985ba48). OpenClaw's summary gets its side effect back ("attach rewrites OpenClaw's gateway config"), codex now names its skill install symmetrically with claude, and test/plugins/picker-disclosure.test.js pins each admission so the copy cannot silently vanish a second time.
  3. LOW (vacuous ordering test) - replaced with the suggested two-competing-matches assertion (' attach claude failed: boom' stays plain); hoisting the failed: rule to the top of RULES now fails it, mutation-verified locally (429e07c). Empty gloss filled.
  4. LOW (malformed answer) - the numbered fallback re-asks with a nothing-matched line when an answer names no row; "none" is the explicit empty selection so deliberate opt-everything-out stays possible; partially valid answers still win; all valid inputs byte-identical (991388d).

npm test: 3575 pass, the one pre-existing usage-policy-fold flake. Typecheck clean. walkthrough_picker_to_first_query smoke green.

🤖 Generated with Claude Code

@philcunliffephilcunliffe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neutral verdict at 991388d - changes requested

Reviewed at 991388d. CI is green (test 22/24, typecheck 22/24, duplicate-numbers) and the PR is MERGEABLE.

Three of the four verdict items are genuinely closed. The fourth introduced a regression that the green suite cannot see, and it is the one item I had explicitly marked as optional. Details below, then the one thing that needs to change.


Closed

Item 1 - the unattributed-row arming set (e8d65f1): resolved. You took the narrow-the-doc option, which was one of the two I offered, and did it honestly. llp/0192 §#fail-closed now states plainly that the arming set is the declared owners, that client plugins "write into the dataset through their projectors but declare no datasets, so a client-only opt-out does not arm this rule", and repeats the exposure in #consequences naming all four clients. The factually-wrong some-vs-every rationale is deleted and replaced with the true cost. The local-export amplification I flagged for #631 was added too.

The test binding is real: test/core/source-withhold-build.test.js:240-249 folds the actual bundled manifests via discoverBundledPlugins() and deepEquals the owner set to ['raw-anthropic','raw-openai']. Unioning loaded + excluded means an opt-in plugin cannot dodge the pin. I independently re-folded all 22 manifests and confirm that is the true set. Production code is byte-identical to what I reviewed, so no new behaviour to re-verify.

Item 2 - gate disclosure (985ba48): resolved, crux included. I verified by execution rather than reading. The gate's accept option now carries a summary (wizard/pick.js:436) and it reaches the screen on both paths:

HypAware will record:
Codex
1) Record all
Configures these tools to record through HypAware; the menu rows say how.
2) Select what to record
select [1]:

and the TTY renderer prints it under every row (tui/render.js:153-155). So the "enter, enter, finale" happy path now discloses before the first enter. LLP 0190 is Status: Draft, so the in-place #pick-gate amendment is legitimate, and the settled "bare labels" rule for the rows is preserved. OpenClaw and codex are both completed, and test/plugins/picker-disclosure.test.js pins substance (/attaches/i, /skills/i, /starts a local receiver/i, /rewrites OpenClaw's gateway config/i), so dropping a clause fails it.

Item 3 - style ordering test (429e07c): resolved, confirmed by mutation. The new assertion has two genuinely competing rules on one line. Hoisting failed: to position 0 now fails it:

not ok 2 - rule order is observable: the continuation rule outranks failed: on an indented line
expected: ' attach claude failed: boom'
actual: '\x1B[31m attach claude failed:\x1B[0m boom'

The mirror-image mutation (continuation rule moved last) and deleting the continuation rule are also caught. Five other reorderings survive, but I differential-tested each over a 240-line corpus and they produce zero output differences: equivalent mutants, not blind spots. The palette byte-pinning still catches an ANSI.red/ANSI.yellow swap.


Blocking - HIGH: the item 4 re-ask never terminates on non-TTY stdin (991388d)

src/core/cli/walkthrough.js:100-146. The re-ask is a while (true) in the sharedlegacyNumberedPromptFactory, ungated, with no EOF handling and no attempt cap. A pipe can never answer it.

Reproduced through the packaged CLI, piped stdin with a pty stdout:

{ printf'2\n'; yes; } | node bin/hypaware.js init
parent 429e07chead 991388d
exitrc=1, ~1src=124 (killed at a 20s budget, still spinning)
stdout1,158 bytes1,011,750 bytes
re-asks09,445

A second, quieter mode: when stdin simply runs out mid-loop the rl.question() promise neither resolves nor rejects. With any live handle (OTEL exporters, timers) the process hangs; otherwise node exits 13 with Detected unsettled top-level await and the wizard dies mid-flight with no message. Also structural: y\n3\n arriving in one chunk loses the correction, because readline emits both lines synchronously and the loop registers its next question() a microtask later.

Blast radius is wider than the sync menu. The loop is keyed on nothing, so it also affects wizard/pick.js:177 and runPickerWalkthrough (walkthrough.js:444). Empirically, piped y into runPickerWalkthrough: parent exits 0 with sources []; head never settles. Tokens like y, 0, or an out-of-range index previously meant "capture nothing" for a script and now loop or hang. A bare enter and none still behave correctly, so the documented scripted path survives, but it is no longer the only one scripts use.

Reachability is narrow but real: hyp init gates interactivity on isTty(ctx.stdout) alone (src/core/commands/init.js:105) while shouldUseTui needs both, so piped stdin with a terminal stdout lands squarely in this fallback.

Why CI is green anyway (MEDIUM, worth fixing alongside):test/core/walkthrough-prompt.test.js's askLegacy helper writes each answer only after seeing a rendered select line. That reactive driver can always answer a re-ask, so it never exercises EOF or one-chunk delivery. Every assertion passes while the piped path hangs. A test that pipes y\n3\n as a single chunk, or y\n then EOF, fails today.

Simplest way out

Item 4 was the one I explicitly called "a follow-up rather than a blocker" - the bare-enter case was already fixed and the rendered [x] made the default visible. Reverting 991388d is a completely acceptable resolution and returns you to a state I would approve.

If you would rather keep it: cap the re-asks at one or two and then fall back to the historical [], treat readline close as terminal, and gate it on question.enterKeepsChecked so only the sync menu (the one with the dangerous inverse default) re-asks at all.


Non-blocking notes

  • LOW - comment drift on the very distinction item 1 fixed. Three @refs to #fail-closed still call the arming set the dataset's producers: src/core/cache/storage.js:326, src/core/cache/source-withhold.js:67, test/core/source-withhold-export-drop.test.js:219. Claude/codex/openclaw are producers and are not owners, so a reader of storage.js:326 concludes the opposite of what 0192 now says. CLAUDE.md's "keep refs honest" covers this.
  • LOW - stale fixture docstring.test/core/source-withhold-build.test.js:189-192 still says the fixture mirrors "the way the codex/raw picker rows join claude's on ai_gateway_messages". It no longer does, and now contradicts the real-manifest test 50 lines below.
  • LOW - the gate disclosure is pinned at the spec layer only. Nothing asserts it reaches the screen: there is no select-summary render test, and nothing checks the legacy fallback prints an option summary. If a renderer stopped forwarding summary, pick.test.js stays green and the disclosure vanishes silently - the same shape as the original loss.
  • LOW - raw-anthropic/raw-openai rows don't name the local gateway listener while otel names its receiver; claude-desktop dropped "asks before changing anything". Both pre-existing.
  • LOW - openclaw row doesn't mention the required openclaw gateway restart; the user learns it only after the config is rewritten. Pre-PR copy didn't either, so no regression.
  • LOW, pre-existing - severity patterns aren't pinned as ^-anchored. Unanchoring /^(error:)/i leaves the style file at 19 pass / 0 fail while corrupting output (paintLine slices from index 0 regardless of match position): 'reading config error: EACCES' becomes '\x1b[31merror:\x1b[0mg config error: EACCES'. One assertion closes it.
  • Process nit: the LLP 0190 #sync-gate paragraph describing the re-ask landed in 985ba48, one commit before the code in 991388d, against CLAUDE.md's "land the doc edit in the same commit as the code". Also llp/0190:77-81 claims the pick menus keep historical semantics untouched, which the ungated loop contradicts.

Items 1 through 3 are good work and I am satisfied with all three. Revert 991388d (or bound and gate it) and this is an approve from me. Neutral is past its review cap so it will not push fixes here; push to the branch and the ladder re-opens at the new head.

… item 4)"
This reverts commit 991388d, per the neutral verdict at that head: the
ungated while(true) re-ask never terminates on non-TTY stdin (a pipe
feeding invalid input loops forever; EOF mid-question leaves rl.question
permanently unsettled), it lived in the shared factory so the pick menus
and runPickerWalkthrough regressed too, and one-chunk answer delivery
lost the correction line. The malformed-answer papercut it addressed is
deferred to issue #634 with the bounded design (gate on
enterKeepsChecked, cap the re-asks, treat EOF as terminal) and the
test-harness gap that hid the regression. LLP 0190 #sync-gate's re-ask
sentence goes with the code it described.
The bare-enter half of verdict item 4's territory is untouched: enter
still keeps the rendered [x] defaults (commit 49fd628).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

Reverted 991388d as 192e33d, taking the verdict's first option. The malformed-answer papercut moves to #634, which carries the bounded design from the verdict (gate the re-ask on enterKeepsChecked, cap it, treat EOF as terminal, keep "none") plus the askLegacy reactive-driver test gap that let the regression through CI. LLP 0190 #sync-gate's re-ask sentence is removed in the same commit as the code it described, and now names the deferral.

The bare-enter fix (49fd628) is untouched: enter still keeps the rendered [x] defaults in the sync menu.

The non-blocking LOW notes (the producers-vs-owners comment drift, the stale fixture docstring, the render-layer disclosure pin, the unanchored severity patterns) are left for a follow-up sweep rather than risking new surface in this round.

npm test: 3572 pass, the one pre-existing usage-policy-fold flake. Typecheck clean.

🤖 Generated with Claude Code

@bgmcmullen

Copy link
Copy Markdown
ContributorAuthor

The non-blocking notes from the 991388d verdict are now tracked as issues: #635 (producers-vs-owners @ref drift + stale fixture docstring), #636 (pin the gate disclosure at the render layer), #637 (picker copy follow-ups: raw rows' listener, openclaw restart, claude-desktop reassurance), #638 (pin the severity patterns' ^ anchors), #639 (the eleven pre-existing broken @ref anchors, R4). The malformed-answer re-ask is already #634.

🤖 Generated with Claude Code

@philcunliffephilcunliffe added neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) and removed neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides) labels Aug 5, 2026

@philcunliffephilcunliffe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neutral verdict at 192e33d - approved

Reviewed at 192e33d. CI green (test 22/24, typecheck 22/24, duplicate-numbers), MERGEABLE. All four verdict items are resolved. Held for a maintainer to merge; neutral does not merge contributors' PRs.


The revert is clean, and it is the only change

The whole delta from 429e07c - the tree whose items 1 through 3 I verified last round - is one documentation edit:

llp/0190-wizard-defaults-gate.decision.md | 7 ++++---

src/core/cli/walkthrough.js and test/core/walkthrough-prompt.test.js are byte-identical to 429e07c (git diff empty on both), and no while (true) remains in the prompt factory. So nothing else was disturbed on the way out, and items 1 through 3 need no re-verification: same bytes, same verdict.

The doc edit is the right one, and honest. LLP 0190 no longer claims a re-ask that does not exist:

An answer that names no row ("y", "0", an out-of-range index) still reads as "select nothing" - a known papercut deferred to issue #634 (a first re-ask attempt looped forever on piped stdin and was reverted).

That also settles the process nit from last round: the doc paragraph that had landed a commit ahead of its code is now gone with it.

Item 4: verified fixed, and verified it did not take R1 with it

I drove the real defaultPromptFactory with non-TTY streams on all three inputs that failed at 991388d, plus the one that had to survive:

input991388d192e33d
invalid y, then EOFnever settledsettles []
y\n3\n in one chunknever settledsettles []
100,000 invalid linesinfinite loop (9,445 re-asks, ~1MB/20s)settles [] immediately
bare enter (enterKeepsChecked)defaults kept["a","b"] - still kept

That last row is the one that mattered most on a revert: the bare-enter fix from R1 is the reason the malformed-answer case was demotable to a papercut, and it survived intact. The ungated while (true) is gone from the shared factory, so wizard/pick.js and runPickerWalkthrough are off the hook too.

Issue #634 is a good deferral

It states the papercut precisely (malformed answer reads as "select nothing"; bare-enter already fixed), records why the first attempt was reverted with the measured numbers, and scopes the retry to "bounded and pipe-safe". That is enough for whoever picks it up to not repeat the mistake.

One housekeeping note for a maintainer: #634 currently carries no labels. If you want neutral's maintenance reconciler to pick it up and open a fix PR, it needs neutral:fix - that label is the authorization, so neutral will not add it to its own follow-up.


Final state of the four items

  1. Unattributed-row arming set - resolved. LLP 0192 §#fail-closed states the arming set is the declared owners and §#consequences says plainly that "A client-only opt-out (codex, claude, openclaw, hermes) does not arm the rule; only a raw-row opt-out does." The wrong some/every rationale is gone. test/core/source-withhold-build.test.js folds the real bundled manifests and pins the owner set to ['raw-anthropic','raw-openai']; I re-folded all 22 manifests independently and it matches.
  2. Gate disclosure - resolved. The accept option's summary reaches the screen on both the TTY and non-TTY paths, so "enter, enter, finale" now discloses before the first enter. OpenClaw and codex completed the set; test/plugins/picker-disclosure.test.js pins the substance.
  3. Style ordering test - resolved, mutation-confirmed: hoisting failed: to position 0 now fails, as does the mirror-image reorder and deleting the continuation rule.
  4. Numbered fallback - resolved by revert, with the residual documented and deferred.

Carried forward, none blocking

Unchanged from last round and all pre-existing or cosmetic: the producers vs owners comment drift on three @refs to #fail-closed (src/core/cache/storage.js:326, src/core/cache/source-withhold.js:67, test/core/source-withhold-export-drop.test.js:219) is the one I would most like to see tidied, since it says the opposite of what LLP 0192 now settles; plus the stale fixture docstring at test/core/source-withhold-build.test.js:189-192, the gate disclosure being pinned at the spec layer but not at the render layer, the raw-anthropic/raw-openai and claude-desktop rows not naming their effects, openclaw not naming the required gateway restart, and the unpinned ^-anchoring of the severity patterns. Several are follow-up material rather than this PR's business.

Nice work on the four rounds. The revert was the right call over trying to save the re-ask under time pressure.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:adoptForeign PR adopted into neutral's reconcile scopeneutral:adoptedAdoption completion record: merged while carrying neutral:adopt (LLP 0031)neutral:approvedneutral 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.

2 participants

@bgmcmullen@philcunliffe