Fix #1445: Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal

[philcunliffe]

Feature or issue

The wizard's send-now step read the spawned hyp sync child's exit code and, on 3, unconditionally printed the "no destination is configured" closing statement and recorded sync_now: no-destinations, with nothing else corroborating it. Exit 3 is not exclusively HypAware's: Node itself returns 3 on an internal JS parse error, before a line of sync code has run, and nothing structurally stops a future runSync path from returning 3 for another reason. In either collision setup states a confident false explanation on its last screen and writes a wrong outcome into the LLP 0203 telemetry.

Solution

• The corroborating signal is the notice hyp sync already writes on exactly that branch. Its first line moves next to the exit code as SYNC_HELD_NO_DESTINATIONS_NOTICE in src/core/usage-policy/first_sync_hold.js, so the command that writes it and the wizard that matches on it cannot drift apart. No new config key or schema field.
• runSyncChild now spawns with ['inherit', 'inherit', 'pipe']: stdin and stdout stay on the terminal the child prompts on, and its stderr is echoed straight back out while being scanned for the notice (only the notice's own length is retained, so a loud failure cannot buffer unboundedly). The no-destinations branch requires both halves; an exit 3 with no notice falls through to the marker-driven outcome. The corroboration is deliberately not a second marker read, which fails open and would reintroduce the claimed-release polarity LLP 0203 #read-back settles against.
• Verified by a new regression test, an exit 3 the child never explained falls back to the marker, not to no-destinations, which reported no-destinations before the fix and now reports sync-declined; the existing a no-destinations child is not a release even when the marker reads absent still asserts the same outcome, with its fake child now emitting what the real one emits. Full suite 6135 pass / 0 fail, npm run typecheck and npm run build:types clean.

This branch is stacked on #1441 (fix/issue-1437), which introduced the code under repair and is not yet merged; the diff narrows to this fix once that lands. Own delta against #1441's head: +55 / -9 lines.

Code: +160 / -14 lines

Fixes #1445

[philcunliffe]

Neutral review - PR #1448 @ b1798d10

Verdict: approve with fixes landed. The design is sound and the central mechanism holds. Five findings; three fixed and pushed, two reported - one of those because every available fix is a design call larger than this PR.

Scope. Reviewed the own delta only: ddeb6d2e..b1798d10 (5 files, +127/-21). This branch is stacked on #1441, whose commits carry into the master diff and were reviewed and approved separately; nothing below is a finding against #1441's code. No findings were filtered out as belonging to #1441 - the independent second pass returned none from those commits either.

What holds up

• The tail-retention scan is correct. Fuzzed over every chunk size 1..200 and 3000 random splits of noise + notice + 5KB trailing output: zero misses, and no false positive on 100KB of noise. Retaining the notice's own length is provably sufficient, since any proper prefix of it is shorter than that. Also confirmed end-to-end against a real child that splits the notice across two writes 30ms apart and then calls process.exit(3).
• close rather than exit is right, and the child already flushes stderr before exiting (bin/hypaware.js:125), so the notice cannot be truncated away.
• The interactive prompt still works. requireConfirmation gates on isTty(ctx.stdin) only (src/core/cli/confirm.js:95), as the PR claims, and readline in non-terminal mode still writes its query (verified in Node 22). See finding 4 for the part that does change.
• No false positive or negative path. The notice has exactly one writer, on the same branch that returns 3; both halves are required; a spawn error yields code: null and misses the branch; a stub without .stderr degrades toward the marker, which is the conservative direction.
• The LLP 0203 edit is accurate to what the code now does, and 0203 is Draft, so editing it is permitted. The stillHeld !== null hardening remains correctly declined; nothing here weakens #read-back's polarity.
• Conventions clean: no semicolons, no U+2014, no new dependencies, no new config keys or schema fields. The shared constant is in the right home, next to the exit code it corroborates.

Findings

1. MEDIUM (fixed) - the notice match is defeated by the CLI's own severity colour, and only survives by accident.

src/core/cli/wizard/sync_now.js:269 compared raw stderr against the constant. But colorizeStderr paints a prefix, and paintLine's hyp <cmd>: rule (src/core/cli/style.js:124) matches this exact line. Running it in the worktree:

paintLine(NOTICE)        -> "ESC[31mhyp sync:ESC[0m no destinations are configured, ..."
painted.includes(NOTICE) -> false

The reset lands inside the sentence, so the styled line no longer contains the constant it was built from. It is inert today only because useColor is isTTY && !NO_COLOR (src/core/cli/stdio.js:22) and this child's stderr is now a pipe. The day useColor gains a FORCE_COLOR-style override - an ordinary, unrelated change - the corroboration fails silently and the user is back to a confidently wrong closing screen, with no error to say so. That is the same silent-wrongness class #1445 is about, so this fix should not lean on a coincidence.

Fixed: matched through a new stripSgr in style.js (lifted out of visibleWidth, which already carried the ANSI_SGR regex - reuse, not a second one), with the carried tail widened to 2 * notice.length so a styled notice still survives a chunk boundary. New regression test a notice split across chunks and painted by severity colour is still read - verified failing at b1798d10, passing after.

2. MEDIUM (fixed) - the newly piped child.stderr has no error listener.

src/core/cli/wizard/sync_now.js:263-271 attached only setEncoding and data. An error on that read pipe is an unhandled emitter event: no try/catch in runSyncChild can contain it, and bin/hypaware.js:107 installs installStreamErrorHandlers on process.stdout/process.stderr only. The result is an uncaught exception on the wizard's last step, after every act of setup has already succeeded - precisely the failure that helper's own doc comment exists to describe, now reintroduced on the read side.

Fixed: child.stderr?.on('error', () => {}), with a comment saying why nothing more is owed (close still fires, so the exit code is still judged, just without the corroboration the pipe was there to collect). New test a stderr pipe that fails does not take the run down - verified failing without the listener, passing with it. The stderr stub is now a real EventEmitter so an unlistened error throws the way the real pipe does; the handler-bag stub could not tell the difference.

3. LOW (fixed) - WizardSyncNowResult's doc left stale by this delta.

src/core/cli/wizard/types.d.ts:634 still read "It is told apart by the child's exit code (SYNC_HELD_NO_DESTINATIONS_EXIT), never by the marker". LLP 0203 and the inline comment were updated; this one was not. Fixed: it now names both halves and why both are required.

4. MEDIUM (reported, NOT fixed - needs your call) - piping stderr flips the child's send-confirm out of readline terminal mode, which changes what ^C does.

askYesNo builds readline.createInterface({ input, output: ctx.stderr }) with no explicit terminal (src/core/cli/confirm.js:59-62), so terminal defaults to output.isTTY. Under stdio: 'inherit' that was true; with stderr a pipe it is now false. Both halves verified in Node 22:

• Before: terminal: true meant readline called setRawMode(true) on the tty. Node documents that raw mode does not raise SIGINT on ^C, so a ^C at hyp sync's confirm was a byte readline consumed: it emitted pause then close, askLineOnce's close handler settled null, askYesNo declined, the child printed cancelled and exited 0, and the wizard went on to print its still-held epilogue and closing question list.
• After: readline never touches raw mode, so the tty stays cooked and ^C is a SIGINT for the whole foreground process group. The child is spawned without detached, and nothing in src/ or bin/ installs a CLI SIGINT handler (only src/core/daemon/runtime.js:1268), so the wizard dies too and its closing screen is lost - against LLP 0188 #never-silent, which this very file cites at sync_now.js:96 for exactly that epilogue.

The prompt itself still renders (non-terminal prompt() writes the query), so this is a mode change, not a vanished prompt. It also loses the cursor bookkeeping that src/core/cli/line_asker.js:10-16 names as the reason this prompt keeps rl.question at all. Worth noting the code comment and the LLP both say the child "prompts on" the inherited terminal; the child's question is actually written to stderr, which is the stream this PR pipes.

I did not land a fix because both candidates are your call, not a reviewer's:

• terminal: isTty(input) in confirm.js - principled, since it aligns readline's mode with the stdin gate requireConfirmation already uses, and a faithful restore of the ^C behaviour. But it changes shared confirm machinery for every command, an interactive run with redirected stderr (hyp purge 2>log) would start writing raw-mode redraw escapes into the redirect, and it makes readline redraw into a pipe, which I cannot verify without a real TTY.
• Absorbing SIGINT in runSyncChild for the child's lifetime - contained in this file, but not a faithful restore: a ^C during the child's plan rendering killed both processes before this PR too, and would stop doing so.

A third answer is to accept it - ^C killing the foreground group is standard Unix behaviour and arguably what a user pressing it expects. Whichever you choose, it is worth a sentence in LLP 0203 #child-process, which currently says only that inheriting the terminal is safe.

5. LOW (reported, cosmetic) - output interleaving. The child's stdout still goes straight to fd 1 while its stderr now detours through the parent's event loop, so an echoed stderr chunk can land mid-frame in the withSpinner animation hyp sync runs on stdout (src/core/commands/sync.js:307). Ordering is preserved in the common case because stdout to a TTY is synchronous. Not worth code.

Fixes pushed

fix/issue-1445 moved b1798d10 to d748fa16, one commit touching style.js, sync_now.js, types.d.ts and the test file (+120/-14). Each fix was positively verified present in the committed tree by diffing the named symbols against the pre-fix head, not inferred from a green suite.

npm test 6138 pass / 0 fail / 2 skipped; npm run typecheck and npm run build:types clean. The stale-node_modules artifacts mentioned in the review brief did not reproduce here - the icebird pin tests and the kernel-types typecheck were green on the unmodified base as well.

[philcunliffe]

Review round: head 771c87b6 (high effort)

Verdict: approve with findings. The fix is correct, minimal, and does what #1445 asked. Four findings, all low, one of which is fixed here; none block. New head after this round: 7e88fbca.

Reviewed the PR's own delta (ddeb6d2e..d748fa16, plus the caa53cfb master merge), not the stacked #1441 base.

LLP 0002 bug-fix gate: satisfied, verified

test/core/cli/wizard/sync_now.test.js carries an exit 3 the child never explained is a plain child-failed, not no-destinations. Positively verified rather than assumed: reverting only the corroboration conjunction at src/core/cli/wizard/sync_now.js:122 (dropping && result.noDestinations) turns exactly that one test red and leaves the other 18 green, including the pre-existing a no-destinations child is not a release even when the marker reads absent. That is the acceptance condition of #1445, minus the outcome label: the fallback is child-failed rather than the sync-declined the issue predicted, because master's caa53cfb landed the child-failed arm in between. The property the issue cares about (an unexplained 3 is not reported as no-destinations) holds.

Findings

1. low - src/core/cli/wizard/sync_now.js:200 - the piped stream carries the send confirm, not just diagnostics. askYesNo builds its readline over ctx.stderr (src/core/cli/confirm.js:60-63), so Send now and end the review window? [Y/n] goes down the new pipe and reaches the terminal only through the echo loop. The docstring at :177-185 describes stdout as "the screen the user answers" and stderr as carrying "the child's diagnostics", which is now the one description a future narrowing of this pipe must not believe. Two consequences, both measured rather than argued:

• readline resolves terminal from output.isTTY, so the interface is now built with terminal: false, which src/core/cli/line_asker.js:10-16 names as the case askLineOnce is explicitly not for. Reproduced the exact stdio shape out of tree: the query is still written and the answer still read (terminal=false, query echoed, got="y"). No raw mode is taken, so the tty stays canonical and echoes the typed answer itself. Functionally intact, contractually drifted.
• delivery now runs through the wizard's guarded, colorized stderr (opts.stderr is guard.stderr, wizard/index.js:101,964), whose writes become no-ops once wrapSink marks it dead, while LLP 0341 says stderr's death never ends the run. Checked whether that is a regression: it is not. In production ctx.stderr proxies the real process.stderr, so a sink the guard can mark dead is a process.stderr that was already dead, and the pre-change stdio: 'inherit' child wrote to the same dead fd 2. No behaviour is lost here; the risk arrives only for a host that injects its own stderr, which hyp setup does not.
  Fixed in 7e88fbca: the docstring now names the confirm as the thing the pipe carries and records both consequences, so the invariant is stated where the next change to this stdio shape will read it. No behaviour change.

2. low - src/core/cli/wizard/sync_now.js:238 - close now waits on the pipe. child.on('close') fires only once the stderr pipe has closed, so any process that inherits the child's stderr and outlives it stalls runWizardSyncNow for as long as it lives - a hang class stdio: 'inherit' structurally could not have. Not reachable today: runSync and the sink drivers spawn nothing (grepped src/core/commands/sync.js and src/core/sinks/). Left alone deliberately: close is the right event for the corroboration (exit can beat the last stderr read), and hardening a hang that has no producer would be speculation.

3. low - src/core/cli/wizard/sync_now.js:222 - one diagnostic line loses its severity colour. The child's stderr is a pipe, so useColor is false there and it no longer paints its own lines; colour is recovered only because the parent's stderr is itself colorized. But paintChunk classifies a chunk's first line only when atLineStart is true (src/core/cli/style.js:173-179,209-217), and the confirm question is written without a trailing newline, so the parent is left mid-line: the next line the child writes (hyp sync: nothing was sent - the sink driver is holding every tick, src/core/commands/sync.js:314, or the marker-clear failure at :270) arrives red before this change and plain after. Cosmetic and exactly one line. Not fixed, and the reason is a repo rule rather than laziness: the only small repair is to let the child paint again, and useColor (src/core/cli/stdio.js:21-24) honours nothing but NO_COLOR and isTTY, so restoring it means inventing a FORCE_COLOR-style key. Recorded in the docstring instead.

4. low, inherited, out of scope for this head - src/core/commands/sync.js:123. The notice ends with "Configure a destination, then run hyp sync again", but the branch is not exempt from --yes, and a caller who reached it via hyp sync --yes and follows that advice hits the held --yes refusal (exit 2) on the rerun. Same run, contradictory instruction. This PR only lifts the sentence into SYNC_HELD_NO_DESTINATIONS_NOTICE; the branch, the text, and the --yes reachability are #1441's. Belongs to that PR's review, not this one.

Verified correct, no finding

• The notice cannot be lost to process.exit truncation. hyp sync's own stderr is a pipe now, and pipe writes are async, but bin/hypaware.js:125 already awaits flushStream(process.stderr) before exiting, and flushStream resolves on the write callback, so the notice is on the wire before the code the wizard judges.
• The chunk-tail trim is safe against a split SGR escape. pending.slice(-NOTICE.length * 2) can cut an escape in half, but the residue lands 138 chars ahead of any notice start, so includes still matches the intact sentence behind it. The painted+split case is pinned by a notice split across chunks and painted by severity colour is still read.
• stripSgr extraction from visibleWidth is behaviour-preserving, and paintLine is unchanged.
• child.stderr?. optional chaining keeps the older test stubs on the documented child-failed fallback instead of crashing, and stillHeld ?? opts.deadline is safe because opts.deadline is narrowed to number by the early return.
• LLP 0203 is Status: Draft, so amending its decision text is permitted rather than a violation of the accepted-docs rule. node scripts/llp-numbers.js check: no collision.
• No em dashes, no NUL bytes, no semicolons, no new runtime dependency.

CPU and memory pass

Explicit pass over runSyncChild, the notice matcher, the echo, and runSync's reordered deadline read.

• Bounded, as claimed. pending is trimmed to 2 x SYNC_HELD_NO_DESTINATIONS_NOTICE.length (138 chars) on every non-matching chunk, so peak retention is one chunk plus 138 bytes regardless of how loudly the child fails. Once noDestinations is set the handler returns before touching pending at all, so a chatty tail costs nothing but the echo.
• Per-chunk work is linear and non-backtracking. stripSgr is one String.replace over /\x1b\[[0-9;]*m/g, O(chunk) with no catastrophic case, and includes is O(pending). Both run at most once per data event on a stream that emits a handful of short diagnostics.
• One avoidable allocation, immaterial. String(chunk) re-wraps a string that setEncoding('utf8') already guarantees. One-line, per-chunk, on a stream with single-digit chunk counts. Not worth a change.
• One unbounded queue, and it is the only one. echo.write(text) discards its return value and the readable is never paused, so a child flooding stderr into a slow parent stderr grows the parent's write queue without limit. The comment at :215-219 promises boundedness of pending, which is true, not of the forwarding queue. In practice the parent's stderr is a TTY (writes are synchronous on POSIX) and hyp sync emits diagnostics, not payload, so there is no producer for this today. Noted, not fixed.
• No growth with uptime or data volume, no busy loop, no repeated work. This path runs once per attended enrolling hyp setup, and the listeners die with the child. runSync's moved readFirstSyncDeadline adds one stat/read on a previously-early-return path, amortised to nothing.

No CPU or memory concern that warrants a change at this head.

Suite

npm test: 6141 pass / 2 fail, and both failures are an artefact of this review worktree, not the PR: icebird uses the root parquet pins directly or through overrides and every read-path dependency that carries hyparquet is held at the floor read the installed node_modules, which the worktree borrowed from a checkout predating the 6ed81daa hyparquet 1.29.1 -> 1.29.2 bump this branch merged. test/core/cli/wizard/sync_now.test.js 19/19. npm run typecheck: only the pre-existing squirreling/ScannableDataSource error, unrelated to this diff.

[philcunliffe]

Neutral triage - PR #1448 @ 7e88fbca

Review rounds are exhausted. The review of record (771c87b6) reported four findings; finding 1 is fixed at head 7e88fbca (docstring verified in the tree). The three residual findings were each verified against the head and classified non-blocking, so each is deferred to its own issue:

• Review finding 2 (low): close waits on the child's stderr pipe, a hang class with no producer today -> Wizard sync-now resolves on close, so a descendant inheriting the child's stderr pipe would stall the closing step #1451
• Review finding 3 (low, cosmetic): one diagnostic line after the send confirm loses its severity colour -> Piped sync child leaves colorizeStderr mid-line after the send confirm, so the next diagnostic line loses its severity colour #1452
• Review finding 4 (low, inherited from Fix #1437: Wizard sync-now step counts a no-sinks exit-0 hyp sync child as a user decline #1441): the held no-destinations notice advises a rerun the held --yes refusal then rejects -> Held no-destinations notice tells a --yes caller to rerun hyp sync, which the held --yes refusal then rejects #1453

No residual finding is a production blocker: the pipe-wait hang has no reachable producer (nothing under src/core/commands/sync.js or src/core/sinks/ spawns a subprocess), the colour loss is one cosmetic line, and the advice contradiction fails safely with exit 2 and its own hint.

[philcunliffe]

Ship risk: low

Who could be affected: People finishing setup on a machine that has just been connected to a team, and anyone who runs the "send my history now" command from a script.

What could happen:

• At the end of setup, the closing message changes for one case: a machine with no destination set up yet is now told that nothing was sent and why, instead of being told to re-run a command that would find the same nothing.
• A script that runs the send command during the review window on a machine with no destination now gets a failure result where it used to get success. Running it by hand, asking for a preview, or replaying past history are unaffected.

Why this level: Nothing about what is sent, kept, or deleted changes, and the confirmation question that guards sending still has to be answered by a person. The only visible differences are wording and one result code, both easy to undo.

What was checked: Setup was run end to end against the real send command, which reported the new message without sending anything and left the review window intact. The confirmation question was then exercised on a real terminal: it still appears, still reads the answer, still stops on "no", and still stops when nobody is there to answer. All related tests passed.