Fix #1437: Wizard sync-now step counts a no-sinks exit-0 hyp sync child as a user decline

[philcunliffe]

Feature or issue

On an enrolled attended hyp init, the closing send-now offer spawns hyp sync and then reads the hold marker to decide what happened. A child that found no destination exits 0 without ever rendering a plan (no sinks instantiated; nothing to do), which leaves the marker standing exactly as a user who read the plan and answered no does. The wizard therefore printed "run hyp sync any time to send it sooner" at a machine with nowhere to send, and reported sync_now: sync-declined, inflating the declined rate LLP 0203 #consequences uses to size the review window.

Solution

• hyp sync now reads the hold before the handle count and, when a window is open and no destination was instantiated, says so on stderr and exits with SYNC_HELD_NO_DESTINATIONS_EXIT (3) instead of exiting 0 having sent nothing, which is what LLP 0101 #no-release already requires of a release that cannot happen. With no window open the case is unchanged: exit 0, "nothing to do".
• The wizard reads that code only to pick its closing statement and outcome (no-destinations, new in WizardSyncNowResult and in LLP 0203's sync_now value list, whose Status is Draft). The release claim itself stays on the marker. The new statement drops both claims that are false here: the deadline is not when this history leaves, and re-running hyp sync is not the way to send it sooner.
• Regression tests in test/core/sync-command.test.js and test/core/cli/wizard/sync_now.test.js fail on master (the wizard reports sync-declined; the command exits 0) and pass after. npm test 6132 pass / 0 fail, npm run typecheck and npm run build:types clean.

Code: +76 / -4 lines

Fixes #1437

[philcunliffe]

Review round: 61f7e02e (review rung, high effort)

Verdict: changes requested, and the two actionable ones are fixed on the branch. The core of the PR is right: a hyp sync that never rendered a plan is not a decline, and giving it its own exit code is the smallest signal that separates the two. Four findings, all low; two were real behavior defects and are now fixed in 3adc64b8, two are reported and deliberately left.

Fixed

1. low - src/core/commands/sync.js:107 (pre-fix): the new branch broke the --dry-run contract.
The held/no-destinations branch sits inside if (handles.length === 0), ahead of every dry-run path in runSync. So hyp sync --dry-run on a held machine with no instantiated sinks returned 3 with an error on stderr, where before it printed no sinks instantiated; nothing to do and returned 0. That contradicts the doctrine stated 40 lines below in the same function (sync.js:154-155, "--dry-run is exempt: it sends nothing"), and it is inconsistent with the unheld case, where a no-sinks dry run still exits 0. Concrete impact: a scripted or CI hyp sync --dry-run inspection run right after an attended enroll fails a step that used to pass.

Fix: gated on !dryRun, the same condition the held refusals below already use. docs/CLI_REFERENCE.md now says so.
Regression test: test/core/sync-command.test.js - "a held machine with no destinations still exits 0 under --dry-run". Verified failing on 61f7e02e (not ok 43), passing after.

2. low - src/core/cli/wizard/sync_now.js:130 (pre-fix): a provably-empty run could still be reported as released.
The exit-code check was nested inside if (stillHeld !== null), so it only fired when the marker re-read came back non-null. readFirstSyncDeadline fails open by design (LLP 0101: it swallows every readFile/JSON.parse error, and it unlinks and reports absent for a lapsed deadline), so readHold returns null for a corrupt or expired marker as well as for a real release. In those cases a child that exited SYNC_HELD_NO_DESTINATIONS_EXIT - and therefore provably sent nothing, since code 3 is returned before any export - fell through to return { asked: true, released: true } and sync_now: released. That is the false "your history is on its way" claim this whole step exists to prevent (LLP 0203 #read-back: "claiming a sync happened is the one wrong answer that cannot be corrected later"), and it skews the same metric the PR is protecting, in the other direction.

Fix: the exit code is read before the marker. Code 3 is the one code that outranks the re-read, because it is proof nothing was sent while an absent marker is only weak evidence that something was. span.released is set to false on that path so the telemetry agrees with the returned result.
LLP: 0203#read-back's new paragraph said setup "reads that code only to pick which closing statement to print"; it now records the ordering and why it is the same polarity as the paragraph above, not an exception to it. 0203 is Draft, so the edit is permitted.
Regression test: test/core/cli/wizard/sync_now.test.js - "a no-destinations child is not a release even when the marker reads absent". Verified failing on 61f7e02e (not ok 8), passing after.

Reported, not fixed

3. low - src/core/cli/wizard/sync_now.js:135: the other non-decline is still counted as sync-declined.
When the child hits the unlink-failure path (sync.js:255: "could not end the review window - the hold marker could not be removed / Nothing was sent", exit 1), the wizard falls through to writeStillHeld and reports sync-declined, then advises "run hyp sync any time to send it sooner" - to a user whose hyp sync just failed on a permissions problem. Same metric inflation and same misleading advice this PR removes for exit 3. Pre-existing, outside issue #1437's acceptance criteria, and a third reason value would want its own LLP line; left for a follow-up rather than widened here.

4. low - src/core/commands/sync.js:109: "no destinations are configured" is not always true.
materializeSinks collects errors and continues (src/core/sinks/materialize.js:43), so a machine with a configured central sink that fails to materialize (bad identity, unavailable plugin) also reaches handles.length === 0. That user is told to "Configure a destination" they already configured, and the wizard reports no-destinations. The replaced wording ("no sinks instantiated") covered both. Not fixed: it is a copy-accuracy question rather than a behavior defect, the wording is right for the dominant case (a wizard-written config with no sinks), and changing it churns the assertions in both new tests. Worth a decision rather than a silent reword.

Checks

• npm test: 6134 pass, 0 fail, 1 skipped.
• npm run typecheck: clean.
• Both new @ref anchors resolve (llp/0203...#read-back via its <a id>, llp/0101...#no-release).
• Exit code 3 collides with nothing: bin/hypaware.js uses 0/1/2 and 75 (self-update restart).
• hyp join writes no hold marker, so the headless CI hyp sync --yes teardown in docs/HEADLESS.md is unaffected by the new code.

Head after fixes: 3adc64b865559069f893467887077457f6ca890f.

[philcunliffe]

Review round: head 3adc64b8 (/code-review 1441 high)

Verdict: approve with fixes. The core change is sound: reading the hold before the handle count, minting an exit code for "held, nowhere to send", and having the wizard read that code to pick a closing statement rather than mislabelling it a decline. npm test at the reviewed head: 6133 pass / 0 fail. No blocking defect.

Six findings. Four were actionable and are fixed in 4a11a543; one is a false positive; one is declined with reasoning.

Fixed

1. --history on a held machine with no destinations regressed from exit 0 to exit 3 — medium. src/core/commands/sync.js:112 (reviewed head). The new branch sits ahead of the if (history) dispatch, so hyp sync --history claude with zero handles and an open window now exits 3 printing "The first-sync review window stays open until X ... Configure a destination, then run hyp sync again." A replay can never release the hold — runHistorySync refuses with exit 2 for exactly that reason (src/core/commands/sync.js:358) — so the window framing misattributes the no-op and the advice names a command the caller did not run. Previously that invocation printed "nothing to do" and exited 0.
Fix: --history is exempt from the new branch alongside --dry-run, for the stated opposite reason (a replay has no early release to be silent about). Regression test a held machine with no destinations still exits 0 under --history added to test/core/sync-command.test.js. docs/CLI_REFERENCE.md updated to name both exemptions.

2. Setup's closing line dropped the deadline, but the deadline still fires — medium. src/core/cli/wizard/sync_now.js:298 (reviewed head). writeNoDestinations's JSDoc justified dropping the deadline on "with no destination configured it never does [leave]". The hold marker is untouched on this path and still lapses on schedule, and the driver gates on the marker alone (src/core/sinks/driver.js:58-66). If a destination appears before the deadline by a route the user did not drive — a pulled org config carrying a sinks block, a retried hyp remote login/hyp join, hyp sink add — the driver forwards this history at 11:59 PM with no hyp sync from anyone, and setup's last words on the one screen the step exists to be honest about read as a promise the machine does not keep.
Fix: the line now states the deadline conditionally rather than omitting it: "Your history stays here while that is true. Once one is configured it leaves on the <deadline> deadline, or sooner with hyp sync." JSDoc rewritten to record why the deadline must stay.

3. Duplicated advice at the close of setup — low. src/core/cli/wizard/sync_now.js:299. The child is spawned stdio: 'inherit', so the user reads the child's own four-line "Configure a destination, then run hyp sync again" and then the wizard's three-line restatement of the same advice. The other outcomes are complementary (child says "sync cancelled", wizard adds the deadline); this one was a straight duplicate.
Fix: folded into finding 2 — the reworded line is complementary now, not a repeat.

4. Inaccurate rationale for minting a third exit code — low. src/core/usage-policy/first_sync_hold.js:26. The comment said "1 is an export that ran and failed", but runSync also returns 1 when the marker will not clear (sync.js:265) and when report.held (sync.js:308) — both runs that sent nothing.
Fix: reworded to "1 already covers every run that got as far as a plan and could not finish", which is what actually makes 1 unusable here.

Not actionable

5. @ref LLP 0203#read-back anchors reported broken — false positive. The review tool grepped for {#slug} markers only. llp/0203-setup-offers-the-first-sync.decision.md defines #offer, #no-new-consent, #child-process and #read-back as <a id="..."> HTML anchors (lines 43, 81, 96, 114), which is one of the three anchor forms the corpus uses (LLP 0001). All 13 refs to 0203 resolve. No change.

6. result.code === 3 is taken as proof of "no destinations" with no corroborating signal — low, declined. Node reserves 3 for an internal JS parse error, and nothing stops a future path returning 3, in which case the wizard prints a confident false explanation and records sync_now: no-destinations. The suggested hardening (also require stillHeld !== null) is exactly what commit 3adc64b8 deliberately removed, so that a fail-open marker read cannot turn a provably-sent-nothing run into a claimed release. That is the more important of the two failure modes and the polarity LLP 0203's new paragraph settles. Left as is; noting it here so the trade-off is on the record rather than re-litigated next round.

Verification

• Each fix confirmed present in the committed tree versus 3adc64b8 (git show HEAD:<file> per changed symbol), not inferred from a green suite.
• npm test: 6134 pass / 0 fail / 2 skipped. npm run typecheck: clean.
• New head: 4a11a5432ec37ef1ee2edeabcdbc4ed3d2720ee2.

[philcunliffe]

Triage: head ddeb6d2e535a1405f369aa1191cba82a73b23f9b

The last review round (head 3adc64b8) left one finding unresolved. Re-read against the current tree, it is a hardening preference, not a production risk, so it is deferred to its own issue and the PR can merge:

• Finding 1 (review finding 6): the wizard takes hyp sync exit code 3 as proof of no-destinations with no corroborating signal (src/core/cli/wizard/sync_now.js:139). Non-blocking: a code-3 collision is speculative, the worst case is a wrong closing sentence and one mislabelled sync_now telemetry value, and the reviewed trade-off keeps the more important false-release guard intact. Deferred as Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1445.

All other findings from that round were fixed on the branch (4a11a543) or judged false positives by the reviewer.

[philcunliffe]

Ship risk: medium

Who could be affected: People running HypAware in an automated pipeline, and people finishing setup on a machine that has no destination configured yet.

What could happen:

• A build or automation step that ends with a "send everything now" command could newly report a failure, on a machine enrolled the same day that has nowhere to send. Nothing is lost; the step simply stops reporting success when it sent nothing.
• Our automation guide still shows that command in an example without mentioning the new failure result, so someone following it could be surprised.
• Anyone finishing setup in this state now reads an accurate closing message instead of being told, wrongly, that their history is on its way and can be sent sooner.

Why this level: The only real downside is a pipeline step turning red where it used to pass quietly, in a narrow situation where nothing could have been sent anyway. Nobody loses history, privacy, or access, and it is easy to recover from.

What was checked: The refusal happens before anything is sent and leaves the privacy review window fully intact; the review deadline cannot be shortened. Ordinary runs, preview runs, and history replays behave exactly as before. Verified by running the real code, including a check that the same test fails without this change, plus the full test suite (6134 passing).

[philcunliffe]

Triage: head 791a5d996229ce660add831648aebec3b11075f7

This head is the previously triaged head plus a merge of origin/master; the merge preserved every fix from the two review rounds (the --dry-run and --history exemptions in src/core/commands/sync.js, the exit-code-before-marker ordering and the conditional deadline line in src/core/cli/wizard/sync_now.js, and all three regression tests). One finding from the last review round (head 3adc64b8) remains unresolved, and re-read against this tree it is a hardening preference, not a production risk, so the PR can merge:

• Finding 1 (review finding 6): the wizard takes hyp sync exit code 3 as proof of no-destinations with no corroborating signal (src/core/cli/wizard/sync_now.js:115). Non-blocking: the collision is speculative, the worst case is a wrong closing sentence and one mislabelled sync_now telemetry value, and the false-release guard stays intact regardless. Tracked by Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1445 (existing tracker from the prior triage of this same finding; its marker now covers this head too).

[philcunliffe]

Ship risk: medium

Who could be affected: People who run the "send everything now" command from a script or an automated build, and people finishing setup on a machine that was enrolled today and has nowhere to send data yet.

What could happen:

• An automated step that ends with that command could newly be reported as failed, on a freshly enrolled machine with no destination set up. Nothing is lost and nothing is sent differently; the step simply stops reporting success when it sent nothing.
• One of our automation guides still shows that command in an example without accounting for the new result, so someone copying it could hit the same surprise.
• Someone finishing setup on such a machine now sees an honest closing line saying nothing was sent and why, instead of being told they declined.

Why this level: Only machines inside their first-day review window with no destination configured behave differently, and only in what they report, not in what they send. No history is uploaded, deleted, hidden, or exposed, and the wait and its deadline are untouched. Setting up a destination, or letting the window pass, restores the old result.

What was checked: Tests covered the new case and every neighbouring one - no review window open, inspection-only runs, and replay runs - all of which still report success as before, and confirmed the wait itself is left in place. Deliberately breaking each safeguard made those tests fail, so they are really testing it. Setup's closing message was checked to never claim a send that did not happen.

[philcunliffe]

What neutral was doing

The resolve-conflict rung for this PR (fix/issue-1437 -> master), which GitHub reported DIRTY. The conflict is resolved and pushed: head is now e1022cdf, mergeable is MERGEABLE. A precautionary local run in the worktree was green (npm test 6144 pass / 0 fail / 1 skipped; npm run typecheck clean).

Why it cannot proceed

Resolving the conflict revealed that this PR is now empty, and that is a disposition question neutral has no authority to settle:

• gh pr view 1441 reports changedFiles=0, +0/-0.
• git diff --stat origin/master origin/fix/issue-1437 is empty: the branch tree is byte-identical to master.
• Master commit 82d49bbf ("Fix Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1445 ... (Fix #1445: Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1448)") already carries this PR's content. PR Fix #1445: Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1448 was branched from this PR's work and landed first, squashed, plus three further review-fix commits on top. (The three original commits 61f7e02e, 3adc64b8, 4a11a543 are not ancestors of master, as expected after a squash merge; the content match is the ground truth, not the commit ids.)
• The conflict resolution itself dropped nothing. All 6 conflicting files (llp/0203-setup-offers-the-first-sync.decision.md, src/core/cli/wizard/sync_now.js, src/core/cli/wizard/types.d.ts, src/core/commands/sync.js, src/core/usage-policy/first_sync_hold.js, test/core/cli/wizard/sync_now.test.js) were compared side-by-side in full. Master's side is a strict evolution of this branch's side in every case: it keeps SYNC_HELD_NO_DESTINATIONS_EXIT, the no-destinations wizard outcome and the LLP 0203 sync_now value-list update, and adds SYNC_HELD_NO_DESTINATIONS_NOTICE, the piped-stderr read, and the --dry-run/--history exemptions.

Issue #1437's acceptance condition is already satisfied on master (src/core/cli/wizard/sync_now.js:25,121,124 carry the no-destinations outcome). But issue #1437 is still OPEN, because the Fixes #1437 trailer lives on this PR, not on the merged #1448.

What it needs from you

A disposition call. Neutral does not close PRs or issues, so it is holding:

1. Close this PR as superseded by Fix #1445: Wizard sync-now step trusts exit code 3 as proof of no-destinations with no corroborating signal #1448, and close Wizard sync-now step counts a no-sinks exit-0 hyp sync child as a user decline #1437 by hand (or move a Fixes #1437 reference onto the record). This is the honest record, but Wizard sync-now step counts a no-sinks exit-0 hyp sync child as a user decline #1437 will not close itself.
2. Merge this PR as a no-op. Harmless (zero diff) and it closes Wizard sync-now step counts a no-sinks exit-0 hyp sync child as a user decline #1437 automatically via the existing trailer, but it lands an empty commit.

How to unstick

Reply with a comment on this PR (or push to the branch). Neutral monitors this thread and will re-engage with your guidance on its next tick.