Wizard prompts take their printed default at EOF instead of hanging - #773
Conversation
…772) `rl.question()` leaves its promise permanently unsettled when the input stream ends without a line, so the three legacy readline prompts in `src/core/cli/walkthrough.js` hung forever on a spent stdin: the overwrite confirm (`hyp init < /dev/null` never returns), the defaults gate, and the backfill consent. The same file already solves this for the numbered picker with `queuedLineAsker`, which resolves a pending ask as `null` on `close` and seeds `closed` from `readableEnded` so an interface built over an already-ended stream does not wait on an `end` it will never see. All three prompts now read through that helper and coalesce `null` to the empty line, so EOF takes exactly the default the question printed (`[y/N]` -> no, `select [2]` -> option 2, `[Y/n]` -> yes) and the branch cannot drift from the advertised default. Output is byte-identical: `queuedLineAsker` writes the prompt itself, the way `rl.question` did. test/core/walkthrough-prompt-eof.test.js races each prompt against a timer, because the pre-fix failure is a hang rather than a wrong value. 4 of its 7 cases fail on master and all 7 pass here. Co-Authored-By: Claude <noreply@anthropic.com>
Conflict: src/core/cli/walkthrough.js, defaultOverwriteConfirmFactory's return expression. #771 flipped the confirm's default to yes ("only an explicit no declines"); this branch routes the prompt through queuedLineAsker so EOF settles as `null` instead of hanging. Both intents compose: the null is coalesced to the empty line and read by #771's parse, so a spent stdin takes the yes the printed [Y/n] promises and the config is regenerated over the backup the caller already takes. The EOF regression test's two overwrite cases follow the default they assert: EOF now returns true and the prompt reads [Y/n]. Its third case now scripts an explicit `n` rather than a `y`, since after the flip only the decline distinguishes an answered prompt from a defaulted one.
This branch merged master mid-flight and inherited two documents both claiming LLP 0223: the converge-on-applied-config decision (PR #770, landed first) and the prune-direct-children-and-unreadable-assets decision (PR #749, landed second). CI's duplicate-numbers check and test/core/llp-ref-hygiene.test.js both fail on the collision. Per LLP 0156#renumber, the later claimant moves. 0226 is already spoken for by fix/issue-774, a sibling branch fixing the same collision on master directly, so this renumbers to 0227, the next free number above the highest claimed across origin/master and every remote branch. Mechanical rename only: no content, status, date, or reasoning changed. The inbound sweep retargets the Extended-by header and four body links in LLP 0219, two @ref [implements] annotations in src/core/runtime/client_assets.js, one in src/core/runtime/client_asset_ledger.js, and five @ref [tests] annotations in test/core/client-assets-prune.test.js. References to LLP 0223 that mean the converge decision (src/core/config/apply.js, src/core/cli/wizard/join.js, src/core/cli/remote_commands.js, test/core/remote-login-command.test.js, llp/0129, llp/0135) are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
This reverts commit 5ce4283.
`hyp init < /dev/null` was cited in both the overwrite confirm's JSDoc and the test header as the run this change unhangs. It is not: the wizard's first screen is `runWizardFork`, whose `legacyMenuPrompt` (src/core/cli/wizard/fork.js) still reads through `rl.question` and so still hangs a fully unanswered `hyp init` one screen before any of the three prompts fixed here. What these three do fix is real and reachable: `hyp clients enable` reaches the backfill consent directly through `maybeBackfillAfterEnable` with no TTY gate, and a partially scripted wizard run (fork answered, stdin then dry) reaches the express gate, the defaults gate and the commit-point confirm. The examples now name those instead, and the test header records the fork prompt as the remaining member of the class. Comment-only: no behaviour, no output bytes, all 7 cases still pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
Verdict: approve with findings. Reviewed manually against the round brief ( Verified
Findings1.
|
…unreachable Round 1 replaced `hyp init < /dev/null` with `hyp clients enable < /dev/null` in the test header. That run is wrong twice over: there is no `clients` command (`hyp attach <client>` is the one that enables), and the backfill consent is not reachable on a piped stdin at all - `maybeBackfillAfterEnable` runs only when `activatedViaPrompt` is set, and both sites that set it go through `maybeInteractiveEnableAttach`, which returns early on `!isTty(ctx.stdin)`. Verified: `hyp attach claude < /dev/null` exits 1 on the not_enabled refusal without asking anything. The header now names the run actually probed on this branch - answer the fork, let stdin dry, and the express gate, the commit-point confirm and the backfill consent all settle on their printed defaults over the spent stream - and states the narrower terminal-drop shape that reaches the attach caller. LLP 0190 #sync-gate said these three prompts "still call `rl.question` directly and still hang at EOF", which the code this PR annotates with that very anchor contradicts. 0190 is Draft, so the sentence is corrected in place and now records the fork screen as the one prompt left outside the file. Comment and doc only: no behaviour, no output bytes. npm test 4091/0 fail, npm run typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
Verdict: approve with findings. Round 2 of 2 (last review round). Reviewed manually in a fresh worktree with a real Round 1's comment-only fix: landed, half accurate
1.
|
| # | Finding | file:line | Severity | State |
|---|---|---|---|---|
| 1 | Round 1's replacement citation names a nonexistent command and an unreachable run | test/core/walkthrough-prompt-eof.test.js:7 |
blocker for the claim | fixed in df64da4 |
| 4 | LLP 0190 asserts the opposite of the code that @refs it |
llp/0190-wizard-defaults-gate.decision.md:118 |
blocker for ref honesty | fixed in df64da4 |
| A | PR body still claims hyp init < /dev/null is fixed, in two places, one of them under the "Fixed" heading. It is not; the run still hangs at src/core/cli/wizard/fork.js:386. |
PR description | not a production blocker (no code defect) but must be corrected by a human before merge if this body becomes the squash-merge message, or master permanently records a false fix claim. Reviewers may not gh pr edit, so I could not. |
unfixed, needs a human |
| B | legacyMenuPrompt hangs at EOF |
src/core/cli/wizard/fork.js:386 |
not a blocker for this PR; a real production bug on master, pre-existing and untouched here. Deferral with a tracking issue, argued above. |
unfixed, deliberate |
| 5 | A dropped stdin mid-wizard now completes an install instead of hanging | src/core/cli/walkthrough.js |
preference (by design, matches LLP 0190) | no action |
| 6 | Fourth rl.question site not on round 1's sibling list |
hypaware-core/plugins-workspace/claude-account/src/index.js:167 |
preference | add to the follow-up issue |
Round 1's findings 2 (backfill consent EOF equals yes: defensible) and 3 (the 500ms race is not a flake risk) were resolved there and are not re-litigated.
Production blockers in this PR: none. One item (A) needs a human before merge; one (B) needs an issue after it.
|
Triage verdict: ship. No production blockers. Both review rounds are spent; this is the triage decision at head What triage re-verified independently
Independent judgement on
|
| Finding | Class | Resolution |
|---|---|---|
PR body claimed hyp init < /dev/null fixed; would have become a false squash-merge record |
factual defect in the record, correctable | Corrected by triage via gh pr edit: the body now opens with an explicit correction block stating what was believed, what is actually fixed, and what remains. Nothing true was removed. |
Stale [y/N] table, "15 failures" testing note, and "#771 (open)" after the base moved |
same | Corrected in the same edit (table lines now 316/439/507 on current master, suite state re-measured fresh: 4090/4091 pass). |
fork.js:386 EOF unsettled promise |
pre-existing production bug on master, outside this diff | Deferred to #783 with the two owner decisions and the sibling list. |
Remaining rl.question siblings: hypaware-core/plugins-workspace/claude-account/src/index.js:167, src/core/cli/confirm.js:30, src/core/plugin_install/confirm.js:150 |
preference / follow-up | Named in #783. |
| A dropped stdin mid-wizard now completes an install unattended | preference, by design (LLP 0190's rule, identical to four bare Enters) | Stated plainly in the corrected PR body so it is discoverable, not discovered. |
| Backfill consent EOF equals yes; 500ms race margin | resolved in round 1 as defensible / non-issues | No action. |
Follow-up issue: #783 (label neutral:fix), "Follow-up: close the remaining rl.question EOF-hang class".
Not done by triage, per policy: no gh pr ready, no merge. The PR is ready for the maintainer's merge decision as it stands.
|
No description provided. |
philcunliffe
added
the
neutral:approved
…confirms, and the OAuth paste (#785) * Close the rl.question EOF-hang class: fork menu, both y/N confirms, the OAuth paste `node:readline`'s `rl.question()` leaves its promise permanently unsettled when the input stream ends without a line. PR #773 fixed the three prompts in `walkthrough.js`; the four sites outside it had the same defect. The worst is the wizard's fork menu, which is the first screen `hyp init` shows, so a terminal whose stdin dried up never got past it: exit 13 on an unsettled top-level await when nothing else held the event loop, an indefinite hang when something did. Two decisions this makes, both stated in LLP 0190 #eof-everywhere: - The asker lives in `src/core/cli/line_asker.js`, beside `stdio.js` and `flush-streams.js` in the tree's existing home for small shared CLI helpers, not in `src/core/util/` (fs and JSON) and not in a new top-level location. Plugin workspaces already import `src/core/...` by relative path, so `claude-account` reaches it the way it reaches the observability and usage-policy modules. `queuedLineAsker` moves there unchanged; `askLineOnce` joins it for the prompts that ask once on an interface that may be a real terminal, keeping `rl.question` as the thing that writes the query (readline redraws a terminal line from its own cursor bookkeeping) and replacing only its promise. - EOF at the fork exits 0, not 130. The prompt prints `default 3` and LLP 0129 #fork settled that the default is Quit, so a spent stdin takes the answer the screen advertised; the fork's TUI path already returns `quit` for a real ctrl+c, so 130 in the readline fallback would judge a dropped terminal more harshly than the TUI judges a deliberate cancel. 130 stays where LLP 0135's cancel put it: prompts whose enter answers nothing. The `Code: ` paste in `claude-account login` is the one prompt with no default, so it does not invent one. With no loopback listener left to finish the sign-in, EOF is a failure that says so; with a listener up the paste lane stays pending rather than losing a race the browser may still win. Regression tests race every case against a 500ms timer, because the pre-fix failure mode is a hang and an unraced assertion never runs. Nine of the fourteen fail on master's behaviour and all fourteen pass here. * askLineOnce must not let EOF outrun an answer that arrived (review of #785) `rl.question` hands its answer back through a promise, a microtask late, while `close` fires synchronously. A stdin that delivers the line and the EOF in one burst - `Readable.from(['y\n'])`, the idiom this repo's own stdin fixtures use, or any readable that pushes data and `null` together - had `close` win that race, so a typed `y` at an irreversible `[y/N]` was silently read as the printed no, and a pasted OAuth code was silently discarded. `rl.question` alone got these right, so the new asker was strictly worse than what it replaced. The EOF settlements now run a turn behind the answer, and the last line of a stream that ends without a trailing newline (readline hands that one to `line`, never to the pending question) is taken too. Genuine EOF, spent streams, and real-terminal redraw are unchanged: verified on a pty for both the paced-keystroke redraw and ctrl+D, and for `hyp init < /dev/null` still exiting 0. Also corrects the stale claim beside the OAuth race that closing the interface rejects a pending question. It does not; that is the defect being worked around. Co-Authored-By: Claude <noreply@anthropic.com> * askLineOnce must answer with the line that answered it (review 2 of #785) The `rl.on('line', done)` added in 2843b78 takes the LAST line of a burst rather than the first. Readline emits `line` only while no question is pending, so the handler fires for everything typed or pasted past the answer, and it fires synchronously while the question hands its own answer back a microtask later. A stdin delivering "n\ny\n" in one chunk therefore answered `y`. That is the one direction an irreversible `[y/N]` must never drift in: `hyp purge`, `hyp report delete` and `hyp attach`'s enable prompt read a typed `n` as a confirmation. Reproduced on a real pty, where a paced `n` is still correct but a pasted "n\ny\n" proceeded with the delete; both `rl.question` and the pre-2843b78 asker returned "n" there, so it was a regression introduced by the fix rather than a pre-existing gap. The line is now held rather than settled on, and read only at EOF and only if the question never answered. That keeps 2843b78's two cases (an answer delivered in the same burst as the EOF, and the unterminated last line readline routes past the question) and drops the overwrite. On every input where `rl.question` answers at all, `askLineOnce` now returns exactly what it returns. Three regression tests, each verified to fail against 2843b78's body and pass against this one. Co-Authored-By: Claude <noreply@anthropic.com> * Drop npm-install.log, committed by accident It was picked up by the round 2 fix commit. It is not gitignored and is not in the published files set, so it has no runtime effect, but it would otherwise land on master as build noise. --------- Co-authored-by: test <test@example.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: neutral <neutral@example.com> Co-authored-by: test <test@test.com>
1 participant
Issue #772 bundles four deferred findings of very different natures. This PR fixes one of them - the only one that is a reproducible bug rather than a design change or an explicitly-declined polish. The other three are left, with reasons below.
Fixed: the EOF hang (issue item 3, first bullet)
node:readline/promises'rl.question()leaves its promise permanently unsettled when the input stream ends without a line. Three prompts insrc/core/cli/walkthrough.jscalled it directly, so a stdin that can no longer answer hung the wizard forever instead of taking the default it had just printed:defaultOverwriteConfirmFactory(the one named in the issue)Continue? [Y/n]legacyConfirmSelectPromptFactory(the LLP 0190 defaults gate)select [N]legacyBackfillConsentPromptFactory[Y/n]This is reachable in normal use:
interactiveincommitWizardPickedConfig/runPickerWalkthroughis!opts.picks, not "stdin is a TTY", so a partially scripted wizard run whose input runs out (fork answered, stdin then dry at the express gate, the commit-point confirm and the backfill consent - for exampleprintf '2\n' | hyp initwith a terminal stdout) or a terminal that drops mid-wizard lands here. The failure was a hang - no error, no exit code, nothing to grep for. (A fully unansweredhyp init < /dev/nullnever gets this far; see the correction above.)The same file already solves exactly this for the numbered picker with
queuedLineAsker, whose JSDoc spells out both halves: resolve a pending ask asnullonclose, and seedclosedfrom the stream's ownreadableEnded, because readline registers itsendlistener at construction and so an interface built over an already-ended stream never emitscloseat all. All three prompts now read through that helper.nullis coalesced to the empty line rather than branched on, so EOF takes exactly the answer a bare Enter gives and the EOF branch cannot drift from the default the question advertises. On this base that is[Y/n]-> yes (the config is rewritten, over a backup),select [2]-> option 2,[Y/n]-> yes. Output is byte-identical:queuedLineAskerwrites the prompt itself, exactly asrl.questiondid withterminal: false, and the tests assert the prompt string is still printed.This applies the rule the file already documents under
@ref LLP 0190#sync-gate("EOF takes the stated default where the prompt has one"), and matches the sibling gate inhypaware-core/plugins-workspace/claude-desktop/src/consent.js, which was written against theline/closeevents specifically to dodge this class. No design change, no new LLP.Regression test: before / after
test/core/walkthrough-prompt-eof.test.js. Each case is raced against a 500ms timer, because the pre-fix failure mode is a hang rather than a wrong value - an unraced assertion would simply never run.Before (fix stashed, test kept):
After:
# tests 7 / # pass 7 / # fail 0.Case 2 is the one a
close-only guard would still hang on, so it is pinned separately. Cases 3, 5 and 7 pin that an answered prompt is unchanged.Escalated, not fixed: item 1, the claude-desktop attach probe
This belongs in the design pipeline, not in a fix PR. The issue names the remedy as "give the claude-desktop client a plist-reading attach probe", but two Accepted LLPs settled the opposite, and
attach_probeis not a label - it is the shared input to three consumers (probeClientAttachFromDescriptor,detachClientFromDisk, and attach-eligibility insrc/core/config/action_attach.js).llp/0135-install-experience-overhaul.design.md#no-probegives three independent reasons, any one sufficient:JSON.parses the settings file.settings_fileis$HOME-relative by contract (resolveClientSettingsPath,@ref LLP 0045#settings_file-is-home-relative-and-a-violation-is-loud), so/Library/Managed Preferences/...re-anchors under$HOME.llp/0115-claude-desktop-managed-config-attach.decision.md#no-attach-on-join(Accepted) already settled that Desktop registers no probe, andhypaware-core/plugins-workspace/claude-desktop/src/index.jscarries the@refrecording it. Declaring one here would also make claude-desktop attach-eligible in the LLP 0044 join loop, which LLP 0115 forbids.Quieting
client_attach_missingby observation therefore needs a new mechanism - a read-only state channel separate fromattach_probe, or a relaxation of the$HOME-relative contract plus a plist probe format - not a fix. That is a new request LLP re-entering the design pipeline. Nothing in this PR presupposes either shape.Left: item 2 and item 3's second bullet
signedIn()predicate onAnthropicCredentialCapability. The issue itself calls it "purely optional polish" that "widens a plugin contract" and a "maintainer's call". Not a bug, no failing behaviour to pin, and a capability-contract widening is a design decision.runWizardSyncNow'scatch. Reachable only via a throwing confirm factory, and the review that found it explicitly judged a guard not worthwhile. The issue records it so the judgment is findable; re-litigating it here would be scope creep with no reproducing test behind it.Testing
npm test-> 4091 tests, 4090 pass, 0 fail, 1 skipped.npm run typecheckclean. (The originally reported "15 failures before and after, identical set" came from a stalenode_modulesand does not reproduce on a fresh install; measured independently in review round 1 and again in triage.)npm run smoke -- walkthrough_picker_to_first_query: fails identically on the original base with and without the change; output diff was empty modulo the run-id and tmpdir.Note on PR #771
#771 flipped
defaultOverwriteConfirmFactory's default to yes and has since merged; this branch merged that master in (c741a98) and the expected textual conflict was resolved there. The compatibility claim held: this PR does not hardcode an EOF answer, it routesnullthrough the existing parse, so the[Y/n]default that function now advertises is the one EOF takes.Fixes #772