Close the rl.question EOF-hang class: the wizard fork menu, both y/N confirms, and the OAuth paste - #785
Conversation
…he 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.
…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>
|
Verdict: approve with one fix, pushed. One confirmed correctness defect in the new Reviewed manually against the brief: Decision 1: the
|
) 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>
|
Verdict: approve with one fix, pushed. Round 2 of 2, reviewing Reviewed manually: Finding 1 (high, fixed in
|
|
Triage verdict: ship. Head Independent verification of the current
|
| input | rl.question |
askLineOnce |
|---|---|---|
paced y\n |
"y" |
"y" |
y\n + EOF in one burst |
"y" |
"y" |
final y, no trailing newline, then EOF |
hang | "y" |
n\ny\n one chunk |
"n" |
"n" |
n\ny one chunk |
"n" |
"n" |
y\nzzz (junk after answer) |
"y" |
"y" |
| EOF, nothing typed | hang | null |
| already-spent stream | hang | null |
unterminated y, stream still open |
hang (correctly) | hang (correctly) |
Repeated prompts on one interface behave exactly like repeated rl.question calls (a line arriving between questions is dropped by both, not queued); no call site asks twice on one interface, and the JSDoc says ask once. Process-level headline re-confirmed: script -qec "node bin/hypaware.js init < /dev/null" prints Choose [1-3, default 3]: and exits 0.
Both rounds' tests are load-bearing: spot-checked
Reverting askLineOnce to 2843b78's body fails exactly tests 10, 11, 19. Reverting to 171e0e9's original body fails exactly 8, 9, 18, 19 (round 2 reported "8, 9 and 16" against the 17-test file; 16 is now 18 after round 2's additions, and 19 also catches the original body). Full suite on head: 4104 tests, 4103 pass, 1 pre-existing skip; typecheck clean.
Call sites and the paste lane
All four sites use the right asker for their interface's terminal setting; the three rl.question left in walkthrough.js (:253, :376, :444) are exactly the three #773 closes. pasteAuthorizationLane with a listener up returns a forever-pending promise, so it cannot settle the race early, and runLogin's finally closes both the interface and the listener; with no listener it throws and runLogin exits 1.
Classification of what was unresolved
- Stray
npm-install.logat repo root, committed by75a3effitself (the round 2 fix commit; neither review round noticed it). Non-blocking: excluded from the npm publish file set, no runtime effect. Deferred to Follow-up: deferred review findings from PR #785 #786 for removal after merge. - LLP 0190
#eof-everywhereplacement: preference, closed as no-change. Verified round 2's grounds myself: 0190 isStatus: Draft;7c1f187(Numbered fallback re-asks once on an answer naming no row (#634) #648) and4fb95dc(Integrate the three wizard PRs (#675, #674, #677): conflicts resolved, LLP 0200 collision renumbered, cross-PR seams closed #679) amended this same document after its code shipped;#sync-gateexplicitly deferred this class ("closing that class is a separate change", line 121). The Systems-narrowness counter is a real but future-facing discoverability point, recorded in Follow-up: deferred review findings from PR #785 #786. - PR body prose nits: fixed in the body (
:247corrected to:248; "seven new@refs" corrected to eight, which I recounted: six[implements]plus two[tests]). The body is the squash-merge message, so this was worth doing.
One behaviour change beyond EOF that round 1 already named, restated for the merger: with terminal: true readline handles ctrl+c by closing the interface, so ctrl+c at a [y/N] confirm now returns the printed no instead of hanging. Same rule, safe direction, deliberate.
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.
|
Re-triage at head a1787e0. The only delta since the cleared head 75a3eff is one commit deleting |
|
Heads up from the #786 follow-up (PR #796, which adds a Whichever of the two lands second will need the other rebased in; if #796 goes first, the new lint fails here until |
… with a lint on the tree (#796) * Tool transcripts cannot be committed again: .gitignore refuses *.log, with a lint on the tree Triage of PR #785 found an npm-install.log committed at the repo root. It was removed by hand, but nothing in the toolchain had objected to it: it sits outside the package files allowlist, touches no code path, and .gitignore carried no rule for tool transcripts, so git status listed it as an ordinary new file. Only a human reading the diff caught it. This closes the class from both ends. .gitignore now ignores *.log, so an install/test/typecheck transcript never reaches git add. test/core/ repo-scratch-hygiene.test.js asserts both halves: no .log is tracked, and the ignore rule actually covers the paths that showed up (root and nested). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Ignore-rule probe reads the rules, not the index, so a tracked .log names the right fix * Ignore probe answers from the committed .gitignore, not a per-clone ignore source --------- Co-authored-by: test <test@test.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: neutral <review@neutral>
…#853) `test/core/repo-scratch-hygiene.test.js` has been failing on `master` since it merged: `x/npm-test.log` and `x/typecheck.log` are tracked, and the test's first half asserts no `.log` is. Both came in on `adb448ab` (#785) via the `git add -A` sweep that #786 wrote this test to catch; the files predate the test, so it was red on arrival. Every branch cut since inherits it, currently blocking #833, #849, #850 and #851 for a reason none of them caused. The transcripts are deleted rather than the test relaxed, which is what its message asks for. `.gitignore` needs nothing: `*.log` is already committed and the rule test already passes, since an ignore rule cannot reach a path that is already tracked. That asymmetry is the whole reason the file carries two tests. Scope is exactly the two `.log` paths. The other eight files under `x/` are untouched: the hygiene test forbids tracked transcripts and nothing else, and sweeping up scratch it does not name would be a judgement this fix has no authority to make. Fixes #852 Co-authored-by: test <test@test.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
x/npm-test.log and x/typecheck.log are tracked leftovers from the git add -A sweep in #785 (pre-dating the repo-scratch-hygiene test added by #786). master already carries this fix via #853; this branch forked before that landed, so it inherited the red test separately. Same fix, scoped to the same two paths.
…sage a failed batch consumed (#851) * Deferred #843 findings: the listener's counters, its refusal log, and the usage a failed batch consumed Six of the ten findings deferred from PR #818, the ones that are a bug with a reproducing test rather than a design call. - A batch whose message write fails no longer loses the usage it claimed. The usage index outlives one POST because an `api_request` and the `assistant_response` that names its `request_id` routinely arrive in different flushes; projection claimed the entry, the write then failed, and the exporter's retry re-projected against a drained index and wrote the same rows with no `attributes.usage` and no `claude.cost_usd`. The catch puts back only what the batch consumed. - `client_attach_stale` no longer fires at an `otel`-attached client. The marker records the gateway port that mode never uses, so a gateway rebind printed a re-attach warning at a client the rebind did not touch; `client_telemetry_stale` already watches the port that decides whether anything is captured. - A refused `body_ref` is logged as a 12-hex digest, not as the raw wire-supplied path (LLP 0257 S23). - `deleteSpooledBodies` counts what it removed. `fs.rm(..., { force: true })` succeeds on a missing path, so `bodies_deleted` and `bodies_dropped` over-reported every already-evicted ref. - A policy drop brings `spool_bytes` down with the files it deleted, instead of leaving the gauge high until the next sweep restates it. - `last_event_at` is maxed by instant, not by string. Mixed `Z` / `.000Z` / offset shapes let the capture-gap baseline run backwards. The other six findings stay open on the issue: the picker's proxy-mode contradiction between two Accepted docs, the `dedupeStoredPartIds` scan cost, `hyp session status` reading only the gateway, the `lastEventAt` stamping order, the telemetry-endpoint prevention half, and the two shapes in finding 10 - each needs a decision this PR is not the place to make. Refs #843 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Delete two committed tool transcripts holding this branch red x/npm-test.log and x/typecheck.log are tracked leftovers from the git add -A sweep in #785 (pre-dating the repo-scratch-hygiene test added by #786). master already carries this fix via #853; this branch forked before that landed, so it inherited the red test separately. Same fix, scoped to the same two paths. * Review fixes: a malformed stamp cannot pin last_event_at, and spool_bytes counts what left the disk Two follow-ups from the review of this branch, both in the code it added. `newerEventTimestamp` fell back to the string compare whenever EITHER side failed to parse. `event.timestamp` is read off the wire unvalidated (`telemetry/events.js` takes whatever string the attribute carried), so a producer that stamps a non-date wins that compare outright - nothing an ISO stamp can begin with sorts above `u` in `unknown` - and because the fallback is symmetric it then beats every genuinely newer event that follows. The baseline is pinned for the life of the daemon, `hyp status` parses it to undefined, falls back to `listener_started_at`, and raises `capture_gap` against a listener that is capturing fine. A value that names an instant now beats one that names none, whichever side it arrives on; the string compare survives only for the pair where neither parses. The projected-body arm still subtracted `spooled.consumedBytes`, the bytes READ, while the PR added `bytesRemoved` precisely so the gauge tracks what left the disk. A body whose unlink fails (EPERM, a read-only spool) is still occupying the cap, and deducting it under-reported `spool_bytes` until the next sweep restated it: the drop arm's bug in the other direction. The new listener case fails on 40e40f9 and passes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Review fixes: a failed batch's usage restore respects the index cap, and a ref stops citing S18 for something S18 does not say The catch-block restore reinstated every key missing from the pre-projection snapshot, which includes the ones USAGE_INDEX_LIMIT evicted during the batch, not only the ones projection claimed. During a sustained dataset outage nothing is ever claimed and nothing ever shrinks the index, so each failed batch added its `api_request` entries on top of a map that could no longer trim itself and the 512 cap stopped bounding anything. The restore is now `restoreUnclaimedUsage`, which puts the snapshot back and re-applies the cap, with a unit test that drives 40 failed batches through the real projector and asserts the index settles at the cap instead of growing past it (it reaches 576 by round 8 without the trim). The `@ref LLP 0257#failure-modes` glosses on the source and on the accounting test cited "S18 - a retried batch is re-projected from the same inputs, so its inputs have to survive". S18 says delivery is best effort and that a down daemon's behavioral-event loss is accepted; it says nothing about retry inputs surviving. 0257 is Accepted, so the mis-citation is removed rather than the spec edited, and the prose above each already carries the reasoning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: neutral <neutral@hyperparam.app> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: test <test@test.com>
node:readline'srl.question()leaves its promise permanently unsettled when the input stream ends without a line. PR #773 closed the three members of that class insidesrc/core/cli/walkthrough.js. This closes the remaining four, and settles the two decisions #783 said an owner had to make. Based onmaster, not on #773's branch.The two decisions
1. Where the shared helper lives:
src/core/cli/line_asker.jsNot a new top-level location, and not
src/core/util/.src/core/util/is the fs-and-JSON drawer (fs_atomic.js,fs_copy.js,json_util.js); nothing about streams or terminals lives there.src/core/cli/is where this repo already keeps its small shared CLI helpers:stdio.js(isTty,useColor),flush-streams.js,stream_errors.js,spinner.js,style.js. A line asker is one of those, so it sits with them.The plugin-workspace member is not an obstacle: plugins already import core modules by relative path (
hypaware-core/plugins-workspace/claude/src/settle.jsreachessrc/core/observability/index.jsandsrc/core/usage-policy/index.js,ai-gateway/src/dataset.jsreachessrc/core/cache/partition.js), soclaude-accountreachessrc/core/cli/line_asker.jsthe same way. No package export orimportsentry is needed, and none is added.The module exports two things:
queuedLineAsker, moved verbatim out ofwalkthrough.js(where it was module-private at line 123), including thereadableEndedseeding an already-spent stream needs. Used by the prompts built withterminal: false, where writing the query straight to the output stream is byte-identical to whatrl.questiondoes.askLineOnce, new, for the prompts that ask once on an interface that may be a real terminal. There,output.write(prompt)is not equivalent to asking: readline redraws the line it is editing from its own cursor bookkeeping, and a query it never saw is a query it cannot redraw. SoaskLineOncekeepsrl.questionas the thing that writes and reads, and replaces only its promise, adding the two settlements it is missing (close, for a stream that ends while the question is on screen;readableEnded, for a stream that was already spent when the interface was built and so never emitscloseat all).src/core/cli/confirm.jsbuilds its interface withoutterminal: falseand is TTY-gated by design, so it takesaskLineOnce. That distinction is the one non-mechanical thing in this diff and is the part most worth pushing back on.2. Exit code when the terminal dropped: 0, not 130
The rule is already in the corpus, in LLP 0190 §sync-gate: "A closed stdin takes that same default where enter has one. Where it does not ... it is a cancel rather than an answer ... (exit 130)." The fork menu prints
Choose [1-3, default 3], and LLP 0129 §fork settled that the default is Quit. So EOF there is Quit, andrunInitWizardturns Quit into exit 0 with nothing written.Three reasons not to reach for 130 instead:
promptForkChoicemapsisPromptCancelledErrorto'quit'(fork.js:121, and again at:248for the returning gate), andwizard/index.js:106,232turn'quit'into{ exitCode: 0 }. Making the readline fallback return 130 would have the fallback judge a dropped terminal more harshly than the TUI judges a deliberate cancel, at the same screen, for the same non-answer.130 keeps its existing meaning: prompts whose enter answers nothing (
WALKTHROUGH_CANCEL_EXIT_CODE, LLP 0190's cancel case). The two[y/N]confirms take their printed no, which is the safe direction for the irreversible verbs behind them (LLP 0104, LLP 0155 §delete-confirm); their callers' exit codes are untouched, so an EOF decline is reported exactly as a typednis.If a maintainer disagrees on either, both are one-line changes and both are recorded in the LLP so the disagreement has somewhere to land.
Sites fixed: all four
src/core/cli/wizard/fork.js:386legacyMenuPrompt(behindlegacyForkPromptandlegacyReturningGatePrompt)quit, the printeddefault 3; wizard exits 0src/core/cli/confirm.js:30askYesNofalse, the printed[y/N]defaultsrc/core/plugin_install/confirm.js:150buildTtyPromptfalse, sodecideConfirmationreturnsrejected, same as a typednhypaware-core/plugins-workspace/claude-account/src/index.js:167Code:OAuth pasteNone left. One site needed more than the mechanical change:
The OAuth paste is the only prompt with no default, so it does not invent one.
parsePastedAuthorization('')throwsempty authorization code, so folding EOF into the empty line the way the wizard's prompts fold it would report an unanswerable prompt as a malformed paste the user never made. Worse, the paste lane is raced against the loopback callback server, so any settlement of the paste lane settles the race: a rejection at EOF would abort a browser sign-in that was still on its way. The lane (extracted aspasteAuthorizationLane, so the rule is readable and testable without a browser) now parts the two cases:runLoginprints it and exits 1. This is the hang Follow-up: close the remaining rl.question EOF-hang class #783 names.Test evidence
test/core/readline-prompt-eof.test.jsandtest/plugins/claude-account-paste-eof.test.js. Both race every case against a 500ms timer, reusingtest/core/walkthrough-prompt-eof.test.js's pattern, because the pre-fix failure mode is a hang rather than a wrong value and an unraced assertion would simply never run.Before (behavioural change reverted in all four files, helper module and extraction kept so imports still resolve, tests unchanged):
After:
# tests 14 / # pass 14 / # fail 0.The five that pass in both are the guards: each fixed prompt still honours a real answer, and the paste lane still waits for the browser and still parses a paste.
Process-level, the headline case
hyp initon a pty stdout with< /dev/nullstdin and a tempHYP_HOME, which is the exposure #783 describes (a terminal run whose stdin dries up):Suite
npm test: 4098 tests, 4097 pass, 0 fail, 1 skipped (the pre-existing skip).npm run typecheck: clean.Docs
LLP 0190 is
Status: Draft, so it is still editable. A new §eof-everywhere records both decisions, the shared home, and theaskLineOnce/queuedLineAskersplit; the eight new@refs point at it. LLP 0129 §fork is@ref'd as[constrained-by]fromlegacyMenuPromptand[tests]from the test, and is not edited: it already settled that quit is the fork's default, and this only applies that to a stdin that cannot answer.Expected conflict with #773
#773 is unmerged and this is based on
master, as #783 asked. Two mechanical conflicts if #773 lands first:src/core/cli/walkthrough.js. This PR deletesqueuedLineAskerfrom the file (lines 87-154) and imports it from./line_asker.js; Wizard prompts take their printed default at EOF instead of hanging #773 adds three callers of it in the same file. The callers are far from the deleted block, so the likely outcome is a clean merge or a trivial one; the resolution in either direction is "keep Wizard prompts take their printed default at EOF instead of hanging #773's three call sites, keep the import, drop the local definition".llp/0190-wizard-defaults-gate.decision.md. Wizard prompts take their printed default at EOF instead of hanging #773 rewrites the paragraph ending "closing that class is a separate change"; this PR appends a new section a few lines below it. Both edits are wanted; take both. Note that after both land, Wizard prompts take their printed default at EOF instead of hanging #773's rewritten sentence ("The wizard's fork screen ... is the one prompt outside this file still asking throughrl.question, and closing that is a separate change") is stale and should be dropped in the resolution.Nothing here depends on #773 landing, and #773 does not depend on this.
Fixes #783