From f0e6ba7c6da70a9e0a8ca2c529c9d1b78c6cd02d Mon Sep 17 00:00:00 2001 From: Phillip Cunliffe Date: Fri, 22 May 2026 22:31:30 -0700 Subject: [PATCH] fix(tui): thread env + handle cancel + portable test runner (hy-3a7ri) (PR #26) 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) --- package.json | 2 +- scripts/run-tests.js | 57 +++++++++++++++ src/core/cli/tui-router.js | 10 ++- src/core/cli/tui/index.js | 9 ++- src/core/cli/walkthrough.js | 91 ++++++++++++++++++------ test/core/cli/tui/non-tty.test.js | 21 ++++++ test/core/walkthrough-tui-happy.test.js | 79 ++++++++++++++------ test/core/walkthrough-tui-router.test.js | 19 +++++ 8 files changed, 242 insertions(+), 46 deletions(-) create mode 100644 scripts/run-tests.js diff --git a/package.json b/package.json index 541a695..8d15c76 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ }, "scripts": { "lint": "node scripts/check-syntax.js", - "test": "find test -name '*.test.js' -print0 | xargs -0 node --test", + "test": "node scripts/run-tests.js", "typecheck": "tsc -p tsconfig.json --noEmit", "smoke": "node ./hypaware-core/smoke/index.js" }, diff --git a/scripts/run-tests.js b/scripts/run-tests.js new file mode 100644 index 0000000..7bfb3f8 --- /dev/null +++ b/scripts/run-tests.js @@ -0,0 +1,57 @@ +#!/usr/bin/env node +// @ts-check + +import { spawnSync } from 'node:child_process' +import fs from 'node:fs' +import path from 'node:path' +import process from 'node:process' + +const ROOT = 'test' +const IGNORED_DIRS = new Set(['.git', '.github', 'node_modules']) + +/** @type {string[]} */ +const files = [] +collectTestFiles(path.resolve(ROOT), files) +files.sort() + +if (files.length === 0) { + process.stderr.write(`no test files found under ${ROOT}\n`) + process.exit(1) +} + +const result = spawnSync( + process.execPath, + ['--test', ...files, ...process.argv.slice(2)], + { stdio: 'inherit' }, +) + +if (result.error) { + process.stderr.write(`failed to spawn node --test: ${result.error.message}\n`) + process.exit(1) +} +process.exit(result.status ?? 1) + +/** + * @param {string} dir + * @param {string[]} out + */ +function collectTestFiles(dir, out) { + let entries + try { + entries = fs.readdirSync(dir, { withFileTypes: true }) + } catch (err) { + if (/** @type {NodeJS.ErrnoException} */ (err).code === 'ENOENT') return + throw err + } + for (const entry of entries) { + if (entry.isDirectory()) { + if (!IGNORED_DIRS.has(entry.name)) { + collectTestFiles(path.join(dir, entry.name), out) + } + continue + } + if (entry.isFile() && entry.name.endsWith('.test.js')) { + out.push(path.join(dir, entry.name)) + } + } +} diff --git a/src/core/cli/tui-router.js b/src/core/cli/tui-router.js index c4e7671..8b8ebdb 100644 --- a/src/core/cli/tui-router.js +++ b/src/core/cli/tui-router.js @@ -12,11 +12,17 @@ import process from 'node:process' * a deliberate escape hatch for CI shells that report `isTTY=true` but * are wrapped by something that mangles ANSI sequences. * - * @param {{ stdin?: NodeJS.ReadableStream, stdout?: unknown }} opts + * The env lookup goes through `opts.env` when supplied so callers that + * inject env (the walkthrough threads `ctx.env`) get the same answer + * the rest of the command pipeline does. Falls back to `process.env` + * only when no env is provided. + * + * @param {{ stdin?: NodeJS.ReadableStream, stdout?: unknown, env?: NodeJS.ProcessEnv }} opts * @returns {boolean} */ export function shouldUseTui(opts) { - if (process.env.HYP_NO_TUI === '1') return false + const env = opts.env ?? process.env + if (env.HYP_NO_TUI === '1') return false const inp = opts.stdin ?? process.stdin const out = opts.stdout return isTty(inp) && isTty(out) diff --git a/src/core/cli/tui/index.js b/src/core/cli/tui/index.js index 60511b8..288e710 100644 --- a/src/core/cli/tui/index.js +++ b/src/core/cli/tui/index.js @@ -24,6 +24,7 @@ export { PromptCancelledError } * @property {{ min?: number, max?: number }} [bounds] * @property {NodeJS.ReadableStream} [stdin] * @property {NodeJS.WritableStream} [stdout] + * @property {NodeJS.ProcessEnv} [env] */ /** @@ -70,6 +71,7 @@ export async function multiselect(spec) { * @property {string|number} [default] * @property {NodeJS.ReadableStream} [stdin] * @property {NodeJS.WritableStream} [stdout] + * @property {NodeJS.ProcessEnv} [env] */ /** @@ -112,6 +114,7 @@ export async function select(spec) { * @property {boolean} [mask] * @property {NodeJS.ReadableStream} [stdin] * @property {NodeJS.WritableStream} [stdout] + * @property {NodeJS.ProcessEnv} [env] */ /** @@ -146,6 +149,7 @@ export async function text(spec) { * @property {boolean} [default] * @property {NodeJS.ReadableStream} [stdin] * @property {NodeJS.WritableStream} [stdout] + * @property {NodeJS.ProcessEnv} [env] */ /** @@ -169,12 +173,13 @@ export async function confirm(spec) { } /** - * @param {{ stdin?: NodeJS.ReadableStream, stdout?: NodeJS.WritableStream }} spec - * @returns {{ stdin: NodeJS.ReadableStream, stdout: NodeJS.WritableStream }} + * @param {{ stdin?: NodeJS.ReadableStream, stdout?: NodeJS.WritableStream, env?: NodeJS.ProcessEnv }} spec + * @returns {{ stdin: NodeJS.ReadableStream, stdout: NodeJS.WritableStream, env?: NodeJS.ProcessEnv }} */ function resolveIo(spec) { return { stdin: spec.stdin ?? process.stdin, stdout: spec.stdout ?? process.stdout, + ...(spec.env !== undefined ? { env: spec.env } : {}), } } diff --git a/src/core/cli/walkthrough.js b/src/core/cli/walkthrough.js index 28a5417..f9cbdef 100644 --- a/src/core/cli/walkthrough.js +++ b/src/core/cli/walkthrough.js @@ -8,9 +8,17 @@ import { Attr, getLogger, withSpan } from '../observability/index.js' import { defaultConfigPath } from '../config/schema.js' import { readObservabilityEnv } from '../observability/env.js' import { ensureDurableBinForNpx } from './global_install.js' -import { multiselect, text } from './tui/index.js' +import { multiselect, text, PromptCancelledError } from './tui/index.js' import { shouldUseTui } from './tui-router.js' +/** + * Exit code returned when the user cancels the picker walkthrough + * (escape / ctrl+c at any TUI prompt). 130 matches the POSIX + * convention for SIGINT and keeps the dispatcher from reporting the + * cancel as an unhandled exception. + */ +export const WALKTHROUGH_CANCEL_EXIT_CODE = 130 + /** @typedef {import('../../../collectivus-plugin-kernel-types').AiGatewayCapability} AiGatewayCapability */ /** @typedef {import('../../../collectivus-plugin-kernel-types').CapabilityRegistry} CapabilityRegistry */ /** @typedef {import('../../../collectivus-plugin-kernel-types').HypAwareV2Config} HypAwareV2Config */ @@ -445,6 +453,7 @@ function tuiPromptFactory(opts) { ...(question.bounds ? { bounds: question.bounds } : {}), stdin: opts.stdin ?? process.stdin, stdout: /** @type {NodeJS.WritableStream} */ (/** @type {unknown} */ (opts.stdout)), + env: opts.env, }) return /** @type {string[]} */ (result) } @@ -470,6 +479,7 @@ function tuiRetentionPromptFactory(opts) { }, stdin: opts.stdin ?? process.stdin, stdout: /** @type {NodeJS.WritableStream} */ (/** @type {unknown} */ (opts.stdout)), + env: opts.env, }) const trimmed = v.trim() if (trimmed === '') return defaultDays @@ -667,26 +677,33 @@ export async function runPickerWalkthrough(opts) { stdout.write('Welcome to HypAware — the local logs+telemetry collector.\n\n') - const sourceRaw = await ask({ - pickType: 'sources', - title: 'What do you want to collect? (space to toggle, enter to confirm)', - options: PICKER_SOURCES.map((s) => ({ value: s.value, label: s.label, summary: s.summary })), - }) - const sources = /** @type {PickerSource[]} */ ( - sourceRaw.filter((v) => PICKER_SOURCES.some((s) => s.value === v)) - ) - - const exportRaw = await ask({ - pickType: 'sinks', - title: 'Where should HypAware export captured data?', - options: PICKER_EXPORTS.map((e) => ({ value: e.value, label: e.label, summary: e.summary })), - }) - const exportChoice = /** @type {PickerExport} */ ( - PICKER_EXPORTS.find((e) => exportRaw.includes(e.value))?.value ?? 'keep-local' - ) + try { + const sourceRaw = await ask({ + pickType: 'sources', + title: 'What do you want to collect? (space to toggle, enter to confirm)', + options: PICKER_SOURCES.map((s) => ({ value: s.value, label: s.label, summary: s.summary })), + }) + const sources = /** @type {PickerSource[]} */ ( + sourceRaw.filter((v) => PICKER_SOURCES.some((s) => s.value === v)) + ) + + const exportRaw = await ask({ + pickType: 'sinks', + title: 'Where should HypAware export captured data?', + options: PICKER_EXPORTS.map((e) => ({ value: e.value, label: e.label, summary: e.summary })), + }) + const exportChoice = /** @type {PickerExport} */ ( + PICKER_EXPORTS.find((e) => exportRaw.includes(e.value))?.value ?? 'keep-local' + ) - const retentionDays = await retentionAsk('Cache retention (days)', DEFAULT_RETENTION_DAYS) - picks = { sources, exportChoice, retentionDays } + const retentionDays = await retentionAsk('Cache retention (days)', DEFAULT_RETENTION_DAYS) + picks = { sources, exportChoice, retentionDays } + } catch (err) { + if (err instanceof PromptCancelledError) { + return cancelledResult(opts) + } + throw err + } } for (const value of picks.sources) { @@ -1148,6 +1165,40 @@ function clientSkillDir(client) { return '.codex/skills' } +/** + * Build the canonical cancel result returned by {@link runPickerWalkthrough} + * when the user cancels via escape / ctrl+c. Writes a one-line cancel + * notice to stderr so the dispatcher does not eat it silently, and + * surfaces {@link WALKTHROUGH_CANCEL_EXIT_CODE} (130, matching SIGINT + * convention) as the exit code. The returned object satisfies the + * required shape of {@link PickerWalkthroughResult} but contains no + * config — callers that key off `exitCode` already short-circuit on + * non-zero values. + * + * @param {RunPickerWalkthroughOptions} opts + * @returns {PickerWalkthroughResult} + */ +function cancelledResult(opts) { + try { + opts.stderr.write('hyp init: cancelled\n') + } catch { + // best-effort: stderr might be closed during cleanup + } + return { + exitCode: WALKTHROUGH_CANCEL_EXIT_CODE, + configPath: '', + config: /** @type {HypAwareV2Config} */ ({ + version: 2, + plugins: [], + query: { cache: { retention: { default_days: DEFAULT_RETENTION_DAYS } } }, + }), + sourcesPicked: [], + exportPicked: 'keep-local', + clientsPicked: [], + retentionDays: DEFAULT_RETENTION_DAYS, + } +} + /** * @param {string} src * @param {string} dest diff --git a/test/core/cli/tui/non-tty.test.js b/test/core/cli/tui/non-tty.test.js index 2d42b1e..c13f122 100644 --- a/test/core/cli/tui/non-tty.test.js +++ b/test/core/cli/tui/non-tty.test.js @@ -99,3 +99,24 @@ test('HYP_NO_TUI=1 forces the same TTY error even for fake-TTY streams', async ( else process.env.HYP_NO_TUI = prevFlag } }) + +test('injected env.HYP_NO_TUI=1 forces the TTY error even when process.env is clean', async () => { + const io = makeFakeTty() + const prevFlag = process.env.HYP_NO_TUI + delete process.env.HYP_NO_TUI + try { + await assert.rejects( + multiselect({ + title: 'pick', + options: [{ value: 'a', label: 'A' }], + stdin: io.stdin, + stdout: io.stdout, + env: { HYP_NO_TUI: '1' }, + }), + (err) => err instanceof Error && ERROR_RE.test(err.message), + ) + } finally { + if (prevFlag === undefined) delete process.env.HYP_NO_TUI + else process.env.HYP_NO_TUI = prevFlag + } +}) diff --git a/test/core/walkthrough-tui-happy.test.js b/test/core/walkthrough-tui-happy.test.js index a1893d8..c1a07c6 100644 --- a/test/core/walkthrough-tui-happy.test.js +++ b/test/core/walkthrough-tui-happy.test.js @@ -7,7 +7,10 @@ import os from 'node:os' import path from 'node:path' import { PassThrough } from 'node:stream' -import { runPickerWalkthrough } from '../../src/core/cli/walkthrough.js' +import { + runPickerWalkthrough, + WALKTHROUGH_CANCEL_EXIT_CODE, +} from '../../src/core/cli/walkthrough.js' /** * Build a PassThrough pair that satisfies the TUI runtime's `isTTY` and @@ -109,40 +112,74 @@ test('runPickerWalkthrough drives the TUI multiselect end-to-end when stdin+stdo } }) -test('runPickerWalkthrough falls back to the legacy numbered prompt under HYP_NO_TUI=1', async () => { - const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'hypaware-walkthrough-tui-fallback-')) - const input = new PassThrough() - // Mark BOTH ends as TTYs so the only signal that flips the router is - // the HYP_NO_TUI escape. This proves the env override wins over the - // TTY probe. - Object.defineProperty(input, 'isTTY', { value: true }) - const stdout = answerDrivenOutput(input, ['3\n', '1\n', '\n'], true) +test('runPickerWalkthrough returns a deterministic cancel exit code when the user cancels at the source prompt', async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'hypaware-walkthrough-cancel-')) + const io = makeFakeTty() const stderr = makeBuf() - const prev = process.env.HYP_NO_TUI - process.env.HYP_NO_TUI = '1' + const prevNoColor = process.env.NO_COLOR + process.env.NO_COLOR = '1' + const prevNoTui = process.env.HYP_NO_TUI + delete process.env.HYP_NO_TUI + try { - const result = await runPickerWalkthrough({ + const promise = runPickerWalkthrough({ capabilities: /** @type {any} */ ({}), - stdout, + stdout: io.stdout, stderr, - stdin: /** @type {any} */ (input), + stdin: io.stdin, env: { HOME: tmp, HYP_HOME: path.join(tmp, '.hyp'), }, }) - assert.equal(result.exitCode, 0) - assert.deepEqual(result.sourcesPicked, ['raw-anthropic']) - assert.equal(result.exportPicked, 'keep-local') - // The legacy prompt prints the numbered-list signature. - assert.match(stdout.text(), /select \(e\.g\. 1,3 or "all"\):/) + + // ctrl+c at the first prompt cancels the walkthrough. + await settle() + await feed(io.stdin, ['\x03']) + + const result = await promise + assert.equal(result.exitCode, WALKTHROUGH_CANCEL_EXIT_CODE) + assert.equal(result.exitCode, 130) + assert.match(stderr.text(), /hyp init: cancelled/) } finally { - if (prev === undefined) delete process.env.HYP_NO_TUI - else process.env.HYP_NO_TUI = prev + if (prevNoColor === undefined) delete process.env.NO_COLOR + else process.env.NO_COLOR = prevNoColor + if (prevNoTui === undefined) delete process.env.HYP_NO_TUI + else process.env.HYP_NO_TUI = prevNoTui } }) +test('runPickerWalkthrough falls back to the legacy numbered prompt under HYP_NO_TUI=1', async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'hypaware-walkthrough-tui-fallback-')) + const input = new PassThrough() + // Mark BOTH ends as TTYs so the only signal that flips the router is + // the HYP_NO_TUI escape. This proves the env override wins over the + // TTY probe. + Object.defineProperty(input, 'isTTY', { value: true }) + const stdout = answerDrivenOutput(input, ['3\n', '1\n', '\n'], true) + const stderr = makeBuf() + + // HYP_NO_TUI flows through opts.env — the same channel real callers + // use — so this test also exercises the env-threading contract. + const result = await runPickerWalkthrough({ + capabilities: /** @type {any} */ ({}), + stdout, + stderr, + stdin: /** @type {any} */ (input), + env: { + HOME: tmp, + HYP_HOME: path.join(tmp, '.hyp'), + HYP_NO_TUI: '1', + }, + }) + assert.equal(result.exitCode, 0) + assert.deepEqual(result.sourcesPicked, ['raw-anthropic']) + assert.equal(result.exportPicked, 'keep-local') + // The legacy prompt prints the numbered-list signature. + assert.match(stdout.text(), /select \(e\.g\. 1,3 or "all"\):/) +}) + /** * @param {PassThrough} input * @param {string[]} answers diff --git a/test/core/walkthrough-tui-router.test.js b/test/core/walkthrough-tui-router.test.js index 754826a..2d93568 100644 --- a/test/core/walkthrough-tui-router.test.js +++ b/test/core/walkthrough-tui-router.test.js @@ -92,6 +92,25 @@ test('shouldUseTui treats HYP_NO_TUI=0 as not-set (only the literal "1" disables }) }) +test('shouldUseTui honors opts.env over process.env when both are set', () => { + withHypNoTui(undefined, () => { + const stdin = makeStream(true) + const stdout = makeStream(true) + // process.env says yes-to-TUI but injected env says no. + assert.equal(shouldUseTui({ stdin, stdout, env: { HYP_NO_TUI: '1' } }), false) + }) +}) + +test('shouldUseTui ignores process.env.HYP_NO_TUI when opts.env is supplied without it', () => { + withHypNoTui('1', () => { + const stdin = makeStream(true) + const stdout = makeStream(true) + // process.env says no-TUI, but the injected env that overrides it + // doesn't carry HYP_NO_TUI, so the TUI path should win. + assert.equal(shouldUseTui({ stdin, stdout, env: {} }), true) + }) +}) + test('isTty rejects undefined, null, and primitives', () => { assert.equal(isTty(undefined), false) assert.equal(isTty(null), false)