Fix #1452: Piped sync child leaves colorizeStderr mid-line after the send confirm, so the next diagnostic line loses its severity colour

[philcunliffe]

Feature or issue

Since PR #1448 the wizard's sync step spawns hyp sync with a piped stderr, which turns off the child's own painting (useColor is false on a pipe), leaving the parent's colorizeStderr as the only painter. That wrap infers the cursor from its own writes, but the child's send confirm ends without a newline and the answer, plus the newline the tty echoes beside it, never passes through the wrap. The wrap therefore still believes it is mid-question when the child's next diagnostic arrives, and paintChunk's line-start gate leaves it plain: hyp sync: nothing was sent - the sink driver is holding every tick reached the terminal red under stdio: 'inherit' and unpainted afterwards. Reverting only the src/ half of this branch reproduces it exactly, with the new wizard test asserting the red hyp sync: prefix and receiving the bare text.

Solution

• colorizeStderr gains a symbol-keyed hook and style.js exports resyncLineStart(stream), which tells a wrap the terminal is back at a line start. It is a no-op on an unwrapped stream, which is every stream that was not painting anyway.
• The runSyncChild echo loop tracks whether the chunk it last relayed ended without a newline and resyncs before writing the next one, so the diagnostic that follows an answered confirm is classified again.
• Verified fail-then-pass: with the two src/ files reverted to their pre-fix content the new wizard test fails on the missing SGR prefix and the style test file fails to load; with the fix restored all 42 tests in the two files pass. Full npm test and npm run typecheck show no new failures (the two hyparquet-pin test failures and the squirreling type error are present on master without this branch).

Code: +37 / -5 lines

Fixes #1452

[philcunliffe]

Neutral review record - PR #1461

Reviewed head: 81e86a08114c08fd2e4fd222a2016c5245a0221f
Scope: the four-file diff origin/master...fix/issue-1452 (src/core/cli/style.js, src/core/cli/wizard/sync_now.js, and the two test files).
Verdict: the fix is correct and it does meet #1452's acceptance condition, but it was one-sided. Two findings, both low; one was actionable and is fixed on this branch (df0b6192).

The change, as read

colorizeStderr infers the terminal cursor from its own writes. The piped sync child cannot paint (useColor is false on a pipe), so the parent's wrap is the only painter left, and the confirm question's answer plus the newline the tty echoes beside it never pass through that wrap. The PR adds a keyed resyncLineStart hook so the relay can tell the wrap the cursor is back at a line start. The Symbol key (rather than a named property on the proxy surface) is the right call: it cannot collide with the stream surface the wrap is careful to preserve.

Findings

1. (low, fixed) The wrap was resynced only before the next chunk, never when the child closed - leaving the commonest path broken.
src/core/cli/wizard/sync_now.js:236 (at the reviewed head).

A decline writes sync cancelled to stdout (src/core/commands/sync.js:261), so on that path the unterminated Send now to ...? [Y/n] is the child's last word on the piped stderr. midLine stayed true, no further chunk ever arrived, and the parent's colorizeStderr believed it was mid-line for the rest of the process. Any later stderr diagnostic - dispatch's hyp <cmd>: <message> if hyp init throws after the sync step - lost its severity colour. That is #1452 again, one step further out.

Probe against the reviewed head: child stderr ['Send now to the central server? [Y/n] '], close 0, then a parent stderr.write('hyp init: something else broke later\n') produced no SGR at all.

Fixed in df0b6192: the two call sites now share one settleLine, called at the top of the data handler and again on close. Covered by a new test, a child that ends mid-confirm leaves the wrap able to classify the run, which was confirmed load-bearing (it fails with the close call removed, passes with it).

2. (low, accepted as-is) The resync's premise is narrower than its gate.
src/core/cli/wizard/sync_now.js:242 (post-fix).

The gate is "the previous chunk lacked a newline"; the premise it stands on is "a newline the tty echoed past the wrap", which holds only when that chunk was an answered prompt. One logical line split across two data events - separate child writes, or a pipe read boundary - is therefore reclassified as a fresh line start and can be painted mid-sentence, weakening the guard that paintChunk(text, atLineStart) and the style test a write that does not end in a newline leaves the next write mid-line exist to provide.

Probe: ['hyp sync: could not end the review window - ', 'error: this half is a continuation...\n'] paints error: red mid-sentence.

Left as-is, deliberately. A grep over src/ and hypaware-core/plugins-workspace/ found no stderr write in the tree lacking a trailing newline other than readline's prompt (src/core/commands/sync.js writes 16 stderr lines, all newline-terminated; the spinner goes to stdout), so today the only reachable trigger is a pipe-read split of a long line, and the outcome is cosmetic in either direction. The narrower alternative - sniffing for a [Y/n] suffix - would couple the relay to confirm.js's exact prompt wording, which is a worse dependency than the heuristic it would replace.

Non-findings, checked and cleared

• midLine is technically redundant with the wrap's own atLineStart (an unconditional resync would behave identically, since a resync after a newline-terminated chunk is a no-op). Keeping the flag makes the precondition explicit instead of resting on that equivalence, so it stays.
• resyncLineStart on an unwrapped stream (NO_COLOR, a piped run, process.stderr when opts.stderr is absent) is a single undefined property read and a no-op. Tested.
• sync_now.js is the only site in src/, hypaware-core/, and bin/ that relays a child's stderr to the terminal; the other seven stderr.on('data') sites buffer for a message. The fix is correctly scoped and nothing else needs it.
• The relayed noDestinations notice detection and its bounded pending tail are untouched.
• @ref LLP 0203#child-process resolves (llp/0203-setup-offers-the-first-sync.decision.md:109).
• Style: no em dashes, no semicolons, no new runtime dependencies, JSDoc types only.

CPU and memory pass

No concern. Per data event the change costs one boolean assignment, one endsWith('\n') (O(1), no allocation), and - only when the previous chunk was unterminated - one symbol property lookup through the proxy plus one no-arg call. Per process it costs one module-level Symbol, one closure per colorizeStderr call (bound once at dispatch), and one closure per runSyncChild. The proxy get trap gains one strict symbol comparison per property access, placed after the write check so the hottest path is untouched. Nothing allocates per line, nothing buffers, no loops, and nothing grows with data volume or uptime; the pre-existing pending tail truncation is unchanged and still bounded by SYNC_HELD_NO_DESTINATIONS_NOTICE.length * 2.

One pre-existing allocation, named for the record and not changed here: the proxy's get trap does value.bind(t) on every method access, so a fresh bound function is allocated per access to on, once, columns, and so on. Method access on stderr is rare (the write fast path short-circuits before it), so it is not a concern, and it predates this PR.

Checks

• npm test: 6149 pass, 2 fail. Both failures are test/core/hyparquet-floor-pin.test.js and are environmental, not this PR's: they fail on origin/master too against the same node_modules (3 failures there), and the branch is behind master, which rewrote that file.
• npx tsc -p tsconfig.json --noEmit: one error, hypaware-plugin-kernel-types.d.ts(14,58) TS2305 'squirreling' has no exported member 'ScannableDataSource'. Pre-existing and present on master.
• The two changed test files: 43/43 pass post-fix (42 at the reviewed head).

[philcunliffe]

Neutral review record - PR #1461 (round 2)

Reviewed head: a2fa03ccad3d77437a14d46495b66db8a592744c (the merge of origin/master onto the branch, which carries df0b6192 from round 1).
Scope: origin/master...a2fa03cc - src/core/cli/style.js, src/core/cli/wizard/sync_now.js, and the two test files - re-read in full, plus context reads of src/core/cli/wizard/output_guard.js, src/core/cli/confirm.js, src/core/commands/sync.js, dispatch.js, and bin/hypaware.js.
Verdict: findings. Two, both low. One actionable and fixed on this branch (e555037b); one re-confirmed as deliberately accepted.

Findings

1. (low, fixed) child.on('error') was the one settle path that did not clear the mid-line state.
src/core/cli/wizard/sync_now.js:273 at the reviewed head:

child.on('error', (err) => done({ code: null, error: err instanceof Error ? err.message : 'spawn failed' }))

Round 1 established that a settle which is the run's last word on this pipe has to resync the wrap, and fixed close and the graced exit. error is the third way this promise resolves and it was missed, so a run ending there resolves with midLine still true and the parent's colorizeStderr stays mid-confirm for every later wizard diagnostic - runWizardSyncNow's own Could not start hyp sync: ... at sync_now.js:96 included. That is #1452 again, one path further out. The comment added directly below it at sync_now.js:282 asserted the opposite ("Both settles resync the line, because either can be the run's last word on this pipe"), naming two settles where the file has three.

Not reachable with today's child: Node emits ChildProcess error for a spawn failure, a failed kill, or an aborted signal, nothing here kills the child or passes a signal, and a spawn failure precedes any stderr data - so midLine is false whenever this arm actually fires. The finding is the asymmetry and the comment that documented it away, not a live defect.

Fixed in e555037b: the arm is now child.on('error', (err) => { settleLine(); done(...) }), making all three settles uniform, and the comment now says every settle rather than both. settleLine is defined above the listener registration and the listener is asynchronous, so there is no TDZ hazard; the synchronous-throw catch arm deliberately still calls done alone, because spawnFn can throw before settleLine exists.

Positive verification (committed tree, not a test run): git show a2fa03cc:src/core/cli/wizard/sync_now.js | grep "child.on('error'" yields the bare done(...) call; the same grep against e555037b yields the settleLine(); done(...) form. settleLine() now has four call sites (data, error, close, graced exit) where the reviewed head had three.

2. (low, accepted as-is, unchanged from round 1) The resync's gate is broader than its premise.
src/core/cli/wizard/sync_now.js:255-259.

The gate is "the previous chunk did not end in \n"; the premise is "a newline the tty echoed past the wrap", which holds only when that chunk was an answered prompt. A single logical line split across two data events takes the same branch, settleLine() fires, and paintChunk re-classifies the tail as a fresh diagnostic - hyp sync: upload to central + failed: timeout\n would get an SGR run injected mid-sentence by the failed: rule. This is the same file that already assumes chunk splits are real: the pending tail-carry three lines below (sync_now.js:270-271) and its comment exist precisely because the notice is one write in runSync but arrives as whatever chunks the pipe hands over. So the premise is weaker than the surrounding code's own stated assumptions.

Impact is cosmetic and bounded: pending += text uses the raw text, so no-destinations detection is untouched, and every ctx.stderr.write in src/core/commands/sync.js is newline-terminated today (the spinner goes to stdout), leaving a >64KB write or a partial pipe read as the only trigger. Left as-is deliberately, as in round 1: the narrower alternative is sniffing for the [Y/n] suffix, which couples the relay to confirm.js's exact prompt wording - a worse dependency than the heuristic it replaces.

Non-findings, checked and cleared

• The symbol hook survives the double proxy. The real wizard hands sync_now the output_guard sink, which is itself a Proxy over dispatch's colorizeStderr Proxy. wrapSink's get trap (src/core/cli/wizard/output_guard.js:122-127) does Reflect.get(target, RESYNC, target), reaching the inner trap, and then .bind(target)s the arrow it gets back - which ignores this. So resyncLineStart works through the guard. Worth stating because it is not obvious and the new tests exercise a bare colorizeStderr wrap only; a wrap-of-wrap test would keep it working if output_guard ever stops delegating unknown props. Noted, not filed: no behaviour is wrong today.
• The Symbol key rather than a named property is the right call - it cannot collide with the stream surface colorizeStderr is careful to preserve, and the prop === RESYNC check sits after the write fast path.
• resyncLineStart on an unwrapped stream (NO_COLOR, a piped run, process.stderr when opts.stderr is absent) is one undefined property read and a no-op. Tested.
• sync_now.js remains the only site in src/, hypaware-core/, and bin/ that relays a child's stderr to the terminal; the other stderr.on('data') sites buffer for a message. Scope is correct.
• The second new wizard test locks in the child-dies-with-the-prompt-unanswered path, where the resync claims a line start the terminal is not at. No worse than pre-fix (the diagnostic was appended to the prompt row either way, just unpainted), so not a regression.
• Both new @ref LLP 0203#child-process annotations resolve (llp/0203-setup-offers-the-first-sync.decision.md).
• Style: no em dashes, no semicolons, no new runtime dependencies, JSDoc types only, no invented config keys.

CPU and memory pass (repo-mandated)

No concern.

Per stderr chunk the change costs one boolean assignment, one endsWith('\n') (O(1), no allocation), and - only when the previous chunk was unterminated - one symbol property read through one or two proxies plus one no-arg call. Per process it costs one module-level Symbol, one closure per colorizeStderr call (bound once at dispatch), and one settleLine closure per runSyncChild. The proxy get trap gains one strict symbol comparison per property access, placed after the write check so the hottest path is untouched. e555037b adds one conditional call on a path that fires at most once per run.

Nothing allocates per line, nothing buffers, there are no loops, and nothing grows with data volume or uptime. The pre-existing pending tail truncation is unchanged and still bounded by SYNC_HELD_NO_DESTINATIONS_NOTICE.length * 2, so an unbounded child stderr still costs O(1) retained memory. paintChunk's split('\n')/join('\n') is unchanged and proportional to the chunk already in hand.

One pre-existing allocation, named for the record and not changed here: both proxy get traps do value.bind(target) on every method access, allocating a fresh bound function per access to on, once, columns, and so on. Method access on stderr is rare (the write fast path short-circuits before it) so it is not a concern, and it predates this PR.

Checks

• Two changed test files at e555037b: 45/45 pass.
• npm test: 6157 pass, 3 fail. All three failures are test/core/hyparquet-floor-pin.test.js and are environmental, not this PR's - the same file fails against the same node_modules outside this branch. Nothing in test/core/cli/ fails.
• npm run typecheck: one error, hypaware-plugin-kernel-types.d.ts(14,58) TS2305 'squirreling' has no exported member 'ScannableDataSource'. Pre-existing on master.

New head after this round: e555037b04949b9fc8689c0621a6089d31524d30.

[philcunliffe]

Neutral triage - PR #1461

The review-round cap was reached with findings open. Triage at head e555037b04949b9fc8689c0621a6089d31524d30 rules every residual finding non-blocking, so the PR can ship; each is deferred to its own neutral:fix issue.

• Round 2 finding 1 (child.on('error') settle path missing settleLine): resolved at this head, verified against the committed tree (settleLine(); done(...) in the error arm, four settleLine call sites). No issue needed.
• Round 2 finding 2 (resync gate at src/core/cli/wizard/sync_now.js:255-259 broader than its premise; a pipe-split logical line can be repainted mid-sentence): non-blocking, deferred to Wizard sync relay resync gate treats a pipe-split logical line as a fresh line start and can repaint it mid-sentence #1471. Verified independently: every stderr write in src/core/commands/sync.js at this head is newline-terminated and the spinner writes to stdout, so the only trigger besides the confirm prompt is a pipe-read split of one logical line, and the impact is a cosmetic colour run with no effect on detection or data.

[philcunliffe]

Ship risk: low

Who could be affected: People running the setup walkthrough who are asked whether to send their data now, on a terminal that shows colour.

What could happen:

• The only thing this change can alter is the colour of the messages the setup step prints. In an unusual case a word part-way through a line could pick up a colour it did not need.
• Nothing about the wording, the order of the messages, the question itself, or whether anything is sent changes.

Why this level: The change is confined to how setup messages are tinted on screen. It cannot send data, withhold data, change what is stored, or change whether setup succeeds, and anything odd would look wrong for one line and be gone on the next run.

What was checked: The messages after the send question were confirmed to come out with the right colour, including when the helper is cancelled or stalls; the same helper was confirmed to do nothing at all when colour is off; the exact text passed through was confirmed unchanged; and the wider set of 475 command-line checks all passed. Removing the fix made the new checks fail, confirming they test the real behaviour.