Feature: tui-prompts#26
Merged
Merged
Conversation
Adds a zero-dependency TUI primitives module that future callers (the
`hyp init` walkthrough in phase 2, and plugin setup commands) can use
to render real interactive prompts. Phase 1 ships the widgets only —
walkthrough.js is untouched and `hyp init` keeps its numbered prompt.
Module layout (src/core/cli/tui/):
- keypress.js — pure reducer (state, key) → state for all four
primitives; cursor/wrap, toggle/toggle-all, 1-9 jump, bounds
validation, validate-aware text confirm, ctrl+c/escape cancel.
- render.js — pure frame builder; honors NO_COLOR; emits cursor-move
escapes separately from SGR color escapes.
- runtime.js — raw-mode keypress loop. Hides cursor, dispatches into
the reducer, restores raw mode and shows the cursor in `finally` on
every exit path (resolve, cancel, throw). Throws the documented
TTY error for non-TTY streams or HYP_NO_TUI=1.
- index.js — entrypoint exporting `multiselect`, `select`, `text`,
`confirm`, and `PromptCancelledError`.
Package export `./tui` lets sibling packages
`import { multiselect } from 'hypaware/tui'`.
Tests under test/core/cli/tui/ cover the reducer exhaustively, the
frame builder, runtime I/O via fake-TTY PassThrough streams, the
non-TTY guard (with and without HYP_NO_TUI), and the package export
resolution.
The npm test script switched to `find … | xargs node --test` so
deeper directories under test/ are picked up — the old shell glob
only matched two levels on the default macOS /bin/sh.
…8) (#29) `hyp init` now drives the new TUI multiselect/text prompts when running against a real TTY on both stdin and stdout. Existing non-TTY callers (CI, piped stdin, tests injecting PassThrough streams) keep getting the legacy numbered prompt unchanged, and `HYP_NO_TUI=1` forces the legacy path even on a TTY for shells that mangle ANSI sequences. - Rename `defaultPromptFactory` → `legacyNumberedPromptFactory` and `defaultRetentionPromptFactory` → `legacyRetentionPromptFactory` with their bodies untouched. Add `tuiPromptFactory` (multiselect) and `tuiRetentionPromptFactory` (text with non-negative-int validate). - Re-introduce `defaultPromptFactory` / `defaultRetentionPromptFactory` as thin routers that delegate to the right factory. Public `AsyncPickPrompt` / `AsyncRetentionPrompt` contracts are unchanged. - New `src/core/cli/tui-router.js` houses `shouldUseTui` + `isTty` so the routing decision is unit-testable in isolation. - Tests: 9 router cases (TTY combos, HYP_NO_TUI literal "1" only, duck-typed `{ write }` sink, omitted `opts.stdin`) plus 2 picker walkthrough flows — one TUI happy path driven by raw keypresses, one HYP_NO_TUI=1 fallback proving the env override beats the TTY probe. Manual `hyp init` against a real TTY remains a manual verification step; the bead allowed deferring that smoke when a hermetic harness isn't available.
Contributor
Author
Dual-agent review —
|
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| codex.md | Contract & Interface Fidelity (major, high) — src/core/cli/tui-router.js:19, src/core/cli/tui/index.js:173, src/core/cli/tui/runtime.js:32, src/core/cli/core_commands.js:1850 |
Targets (tui-router.js, tui/index.js, tui/runtime.js listed as Files added); Config field chain (HYP_NO_TUI read at tui-router.js:20, single consumer pair through walkthrough.js factories); Risks #5 (select and confirm exported but unused — same tui/index.js public-surface concern) |
| codex.md | Error Handling & Resilience (major, high) — src/core/cli/tui/runtime.js:92, src/core/cli/tui/runtime.js:157, src/core/cli/core_commands.js:1844, src/core/cli/dispatch.js:194 |
Targets (tui/runtime.js listed as Files added); Concurrency surface (run() cleanup lifetime: keypress listener, raw mode, cursor visibility); Risks #1 (listener throw leaves terminal in raw mode + cursor hidden — lines ~94-115; reviewer evidence at line 92 sits directly in this window and cleanup() exit at 157 is the same lifecycle); Risks #4 (no abrupt-exit cleanup — same runtime.js lifecycle surface) |
| codex.md | Release Safety (minor, high) — package.json:29 |
Targets (package.json modified — test-runner command change at the cited line); same file as Risks #6 (./tui subpath export semver commitment at package.json:14-15) but a distinct concern (POSIX find/xargs dependency for npm test, not the public subpath) |
Blast radius
- Listener throw leaves terminal in raw mode + cursor hidden. Sync throw inside
onKeypress(inreduceorwriteFrame) doesn't reject the outer Promise;.finally(cleanup)never fires. (src/core/cli/tui/runtime.js:~94-115) - Pre-Promise
writeFrame()throw leaks raw mode + cursor.CURSOR_HIDE+setRawMode(true)commit before the Promise constructor; the initialwriteFrame()between them can throw on a bad initial state with no surrounding cleanup. (src/core/cli/tui/runtime.js:~52-83) - No mutex on
run()against concurrent invocation. Current callers serialize, but the public./tuisubpath export means external consumers of@hyparam/hypaware/tuicould overlap prompts and race the sharedstdin. (src/core/cli/tui/runtime.js) - No abrupt-exit cleanup. Uncaught exception elsewhere or SIGTERM leaves cursor hidden + raw mode active. Low severity, but the CLI is user-visible. (
src/core/cli/tui/runtime.js) selectandconfirmexported but unused in production. Either staged public API needing a doc note, or delete-candidates. (src/core/cli/tui/index.js)- Public subpath
./tuiis a semver commitment. Once shipped, breaking changes totui/index.jsexports are semver-breaking for external@hyparam/hypawareconsumers. (package.json:14-15)
Codex review
Fix Validations
TTY fallback override (HYP_NO_TUI=1)
- Status: correct
- Evidence: test/core/walkthrough-tui-router.test.js:45, test/core/walkthrough-tui-happy.test.js:113
- Assessment: The new router path correctly disables TUI when the global env flag is set and falls back to legacy prompts.
Findings
Contract & Interface Fidelity
- Severity: major
- Confidence: high
- Evidence: src/core/cli/tui-router.js:19, src/core/cli/tui/index.js:173, src/core/cli/tui/runtime.js:32, src/core/cli/core_commands.js:1850
- Why it matters:
runPickerWalkthroughis passedctx.env, but TUI routing/runtime readprocess.env, so env-injected runs cannot reliably disable TUI (HYP_NO_TUI) or control color behavior. - Suggested fix: Thread
envthroughshouldUseTui(...)andrun(...)from walkthrough callsites, defaulting toprocess.envonly when no env is supplied.
Error Handling & Resilience
- Severity: major
- Confidence: high
- Evidence: src/core/cli/tui/runtime.js:92, src/core/cli/tui/runtime.js:157, src/core/cli/core_commands.js:1844, src/core/cli/dispatch.js:194
- Why it matters: TUI cancel (
Esc/Ctrl+C) throwsPromptCancelledError, but command flow does not handle it as a user cancel; it is reported as an unhandled command failure. - Suggested fix: Catch
PromptCancelledErrorinrunPickerWalkthroughorrunInitand return an explicit cancel exit path (with deterministic exit code/message), instead of falling into generic dispatcher error handling.
Release Safety
- Severity: minor
- Confidence: high
- Evidence: package.json:29
- Why it matters:
npm testnow depends on POSIXfind/xargs, which breaks on default Windows shells and narrows contributor/CI portability. - Suggested fix: Replace shell-dependent discovery with a Node-driven test entry (or
node --testwith explicit Node-side globbing) sonpm testis cross-platform.
No Finding
- Behavioral Correctness
- Change Impact / Blast Radius
- Concurrency, Ordering & State Safety
- Security Surface
- Resource Lifecycle & Cleanup
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Evidence Bundle
- Changed hot paths:
src/core/cli/walkthrough.js,src/core/cli/tui-router.js,src/core/cli/tui/index.js,src/core/cli/tui/runtime.js,src/core/cli/tui/keypress.js,src/core/cli/tui/render.js,package.json - Impacted callers: src/core/cli/core_commands.js:1844, src/core/cli/core_commands.js:2040, src/core/cli/dispatch.js:190
- Impacted tests: test/core/cli/tui/runtime.test.js:68, test/core/cli/tui/non-tty.test.js:81, test/core/walkthrough-tui-happy.test.js:49, test/core/walkthrough-tui-router.test.js:25
- Unresolved uncertainty: Local checkout did not contain the exact PR-head file contents for new
src/core/cli/tui/*files, so line evidence for those paths is derived from the provided diff rather than executable workspace files.
tokens used
122,812
Claude review
Code review
No issues found. Checked for bugs and CLAUDE.md compliance.
🤖 Generated with Claude Code
Reports: /Users/phil/testcity/.gc/pr-pipeline/reviews/pr-26 · Bead: hy-p831i · Blast-radius: hy-xp9si
…) (PR #26) (#32) Address the three findings on integration/tui-prompts dual-review: - Contract & Interface Fidelity (major): shouldUseTui and TUI runtime now read HYP_NO_TUI/NO_COLOR through opts.env when supplied, falling back to process.env only when nothing is injected. The walkthrough threads env through multiselect/text so env-injected runs flip the TUI path consistently. - Error Handling & Resilience (major): runPickerWalkthrough catches PromptCancelledError and returns exit code 130 (SIGINT convention) with a "hyp init: cancelled" stderr line, instead of letting cancels fall into the dispatcher's generic unhandled-exception path. - Release Safety (minor): replace shell find/xargs in `npm test` with scripts/run-tests.js (Node-driven, mirrors check-syntax.js). The script discovers *.test.js under test/ and execs `node --test` with the resolved paths so the suite runs on Windows shells too. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Dual-agent review —
|
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| codex.md | Contract & Interface Fidelity (major, high) - scripts/run-tests.js:22, package.json:31 |
Targets (scripts/run-tests.js, package.json); Risks around test runner/public package behavior. |
| codex.md | Resource Lifecycle & Cleanup (major, medium) - src/core/cli/tui/runtime.js:57, src/core/cli/tui/runtime.js:94 |
Concurrency surface and Risks for TUI runtime cleanup and stdin lifecycle. |
| codex.md | Debuggability & Operability (minor, medium) - src/core/cli/walkthrough.js:560, src/core/cli/walkthrough.js:680, src/core/cli/walkthrough.js:701 |
Targets (walkthrough.js) and observability/finish-path concerns adjacent to the TUI cancel path. |
| claude.md | (none reported) | (none) |
Blast radius
Risks
- Listener throw leaves terminal in raw mode + cursor hidden. Sync throw inside
onKeypress(inreduceorwriteFrame) doesn't reject the outer Promise;.finally(cleanup)never fires. (src/core/cli/tui/runtime.js:~94-115) - Pre-Promise
writeFrame()throw leaks raw mode + cursor.CURSOR_HIDE+setRawMode(true)commit before the Promise constructor; the initialwriteFrame()between them can throw on a bad initial state with no surrounding cleanup. (src/core/cli/tui/runtime.js:~52-83) - No mutex on
run()against concurrent invocation. Current callers serialize, but the public./tuisubpath export means external consumers of@hyparam/hypaware/tuicould overlap prompts and race the sharedstdin. (src/core/cli/tui/runtime.js) - No abrupt-exit cleanup. Uncaught exception elsewhere or SIGTERM leaves cursor hidden + raw mode active. Low severity, but the CLI is user-visible. (
src/core/cli/tui/runtime.js) selectandconfirmexported but unused in production. Either staged public API needing a doc note, or delete-candidates. (src/core/cli/tui/index.js)- Public subpath
./tuiis a semver commitment. Once shipped, breaking changes totui/index.jsexports are semver-breaking for external@hyparam/hypawareconsumers. (package.json:14-15)
Codex review
Fix Validations
TUI cancel should return a deterministic non-exceptional exit path
- Status: correct
- Evidence: src/core/cli/walkthrough.js:700, src/core/cli/walkthrough.js:1165, src/core/cli/core_commands.js:1628, src/core/cli/core_commands.js:1837
- Assessment:
PromptCancelledErroris now caught and converted toexitCode=130with a user-facing cancel line, and callers propagateresult.exitCodeinstead of rethrowing.
Existing fallback handling check for “unhandled cancel” behavior
- Status: correct
- Evidence: src/core/cli/dispatch.js:194
- Assessment: There was already generic unhandled-error capture in the dispatcher (would have produced exit
1), so this PR is still meaningful because it changes cancel semantics to explicit130instead of generic failure.
Non-TTY / HYP_NO_TUI=1 routing back to legacy prompt path
- Status: correct
- Evidence: src/core/cli/tui-router.js:24, src/core/cli/walkthrough.js:471, test/core/walkthrough-tui-router.test.js:39
- Assessment: Routing conditions are explicitly enforced and covered by tests for both env-flag and TTY detection paths.
Findings
Contract & Interface Fidelity
- Severity: major
- Confidence: high
- Evidence: scripts/run-tests.js:22, package.json:31
- Why it matters:
npm test -- <node --test flags>compatibility is broken because forwarded args are appended after positional test files, which Node treats as additional test targets rather than runner flags. - Suggested fix: Build argv as
['--test', ...process.argv.slice(2), ...files](flags first), or split/validate pass-through flags before appending file list.
Resource Lifecycle & Cleanup
- Severity: major
- Confidence: medium
- Evidence: src/core/cli/tui/runtime.js:57, src/core/cli/tui/runtime.js:94
- Why it matters: The runtime unconditionally calls
stdin.resume()but never restores paused/flowing state, which can leave stdin flowing after prompt completion and can keep CLI processes alive unexpectedly in embedded/reused contexts. - Suggested fix: Snapshot prior flow state (
isPaused()/readableFlowing) and restore it incleanup(pause when previously paused).
Debuggability & Operability
- Severity: minor
- Confidence: medium
- Evidence: src/core/cli/walkthrough.js:560, src/core/cli/walkthrough.js:701, src/core/cli/walkthrough.js:680
- Why it matters: The new early return on cancel bypasses the documented
walkthrough.finishspan path, so telemetry may show starts without terminal status for canceled sessions. - Suggested fix: Emit
walkthrough.finishwithstatus='cancelled'andexit_code=130before returningcancelledResult.
No Finding
- Behavioral Correctness
- Change Impact / Blast Radius
- Concurrency, Ordering & State Safety
- Error Handling & Resilience
- Security Surface
- Release Safety
- Test Evidence Quality
- Architectural Consistency
Evidence Bundle
- Changed hot paths:
src/core/cli/walkthrough.js,src/core/cli/tui/runtime.js,src/core/cli/tui-router.js,src/core/cli/tui/index.js,scripts/run-tests.js,package.json - Impacted callers: src/core/cli/core_commands.js:1628, src/core/cli/core_commands.js:1824, src/core/cli/dispatch.js:194
- Impacted tests: test/core/cli/tui/keypress.test.js:1, test/core/cli/tui/runtime.test.js:1, test/core/cli/tui/non-tty.test.js:1, test/core/walkthrough-tui-happy.test.js:1, test/core/walkthrough-tui-router.test.js:1
- Unresolved uncertainty: Exact runtime impact of the stdin flow-state leak depends on host TTY/process lifecycle; I did not run an interactive real-TTY end-to-end in this review session.
tokens used
171,689
Claude review
Code review
No issues found. Checked for bugs and CLAUDE.md compliance.
🤖 Generated with Claude Code
Reports: /Users/phil/testcity/.gc/pr-pipeline/reviews/pr-26 · Bead: hy-y0oqa · Blast-radius: hy-gge7r
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-generated by
/feature-launchto host the integration branch for feature tui-prompts.Work beads file individual sub-PRs into this branch via the refinery feature-flow loop. When all work + review + ship beads complete, the ship formula flips this PR to ready-for-review.
See
/feature-flowfor the flow architecture.