diff --git a/docs/cli.md b/docs/cli.md index 4ef532e2..3ba2b854 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -159,7 +159,9 @@ Historical remote guides are excluded from the supported documentation. a safe default apply it (`hack env unset KEY` removes the key; `hack doctor --fix` applies safe remediations and skips destructive ones with a note); prompts that need a real answer fail closed with `E_INTERACTIVE_REQUIRED`, naming the flag or argument that makes the invocation - scriptable (for example `hack update --yes`, `hack mcp --all`, `hack session attach `). + scriptable (for example `hack update --yes`, `hack mcp --all`, `hack session attach `, + `hack secrets delete --yes`, `hack ssh --host `). Destructive confirmations without + such a flag decline or refuse; they never proceed on their own. Set `HACK_NO_INTERACTIVE=1` explicitly in scripts anyway so the intent is visible. - `NO_COLOR` (or `HACK_NO_COLOR`) disables colored/decorated output. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 16b2ac47..bf468383 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2254,6 +2254,9 @@ Delete a stored secret hack secrets delete [name] [options] ``` +Asks for confirmation before deleting. Without a TTY (or with HACK_NO_INTERACTIVE / --no-interactive) +the deletion is refused with E_INTERACTIVE_REQUIRED unless --yes is passed. + ### Arguments | Arg | Description | @@ -2265,6 +2268,7 @@ hack secrets delete [name] [options] | Option | Description | | --- | --- | | `--service ` | Override Bun.secrets service name (default: hack-cli) | +| `--yes` | Confirm the deletion without prompting (required without a TTY or with HACK_NO_INTERACTIVE) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | | `--version, -v` | Show version | diff --git a/src/commands/node.ts b/src/commands/node.ts index 96fb7507..9b7d1f04 100644 --- a/src/commands/node.ts +++ b/src/commands/node.ts @@ -21,6 +21,7 @@ import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { createGatewayClient } from "../control-plane/sdk/gateway-client.ts"; import { resolveDaemonPaths } from "../daemon/paths.ts"; import { pathExists } from "../lib/fs.ts"; +import { canPrompt } from "../lib/interactivity.ts"; import { cancelNodePairingSession, consumeNodePairingSession, @@ -2046,10 +2047,15 @@ async function handleNodePairWalkthrough({ ], }); - const executeNow = await confirm({ - message: "Execute remote approval and complete pairing now?", - initialValue: true, - }); + // Without a TTY the session is left pending: the remote approval runs SSH + // commands and needs an explicit answer, and a clack prompt cannot settle on + // a piped stdin (HACK-1385). + const executeNow = canPrompt() + ? await confirm({ + message: "Execute remote approval and complete pairing now?", + initialValue: true, + }) + : false; if (isCancel(executeNow)) { await cancelNodePairingSession({ sessionId: created.session.id }); logger.info({ @@ -2165,7 +2171,9 @@ type PairWalkthroughValueResult = PairWalkthroughValue | PairWalkthroughError; async function resolvePairWalkthroughInput(input: { readonly args: PairWalkthroughArgs; }): Promise { - const interactive = isTty(); + // canPrompt(), not isTty(): a TTY stdout with a piped stdin must not reach + // the clack prompts below (HACK-1385). + const interactive = canPrompt(); const sourceResult = await resolveRequiredWalkthroughValue({ currentValue: input.args.options.source, interactive, diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index e84bdebc..072eae03 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -21,13 +21,26 @@ const optService = defineOption({ defaultValue: DEFAULT_SECRETS_SERVICE, } as const); +const optYes = defineOption({ + name: "yes", + type: "boolean", + long: "--yes", + description: + "Confirm the deletion without prompting (required without a TTY or with HACK_NO_INTERACTIVE)", +} as const); + const secretsOptions = [optService] as const; +const deleteOptions = [optService, optYes] as const; const secretsPositionals = [{ name: "name", required: false }] as const; type SecretsArgs = CommandArgs< typeof secretsOptions, typeof secretsPositionals >; +type SecretsDeleteArgs = CommandArgs< + typeof deleteOptions, + typeof secretsPositionals +>; const setSpec = defineCommand({ name: "set", @@ -51,7 +64,11 @@ const deleteSpec = defineCommand({ name: "delete", summary: "Delete a stored secret", group: "Secrets", - options: secretsOptions, + description: [ + "Asks for confirmation before deleting. Without a TTY (or with HACK_NO_INTERACTIVE / --no-interactive)", + "the deletion is refused with E_INTERACTIVE_REQUIRED unless --yes is passed.", + ].join("\n"), + options: deleteOptions, positionals: secretsPositionals, subcommands: [], } as const); @@ -70,7 +87,7 @@ export const secretsCommand = defineCommand({ ], } as const); -function resolveService(args: SecretsArgs): string { +function resolveService(args: SecretsArgs | SecretsDeleteArgs): string { return args.options.service ?? DEFAULT_SECRETS_SERVICE; } @@ -158,23 +175,33 @@ async function handleSecretsGet({ return 0; } +/** + * `hack secrets delete `. + * + * `--yes` is the explicit confirmation that makes the deletion scriptable; it + * skips the prompt in every mode. Without it, a TTY run asks (default no) and + * a run that cannot prompt refuses closed with `E_INTERACTIVE_REQUIRED` + * before the keychain is touched. + */ async function handleSecretsDelete({ args, }: { readonly ctx: CliContext; - readonly args: SecretsArgs; + readonly args: SecretsDeleteArgs; }): Promise { const service = resolveService(args); const name = await resolveName( [args.positionals.name].filter((v) => typeof v === "string") ); - const ok = await confirmSafe({ - message: `Delete secret "${name}" (${service})?`, - initialValue: false, - nonInteractive: "fail", - hint: "Run `hack secrets delete ` in a terminal to confirm the deletion.", - }); + const ok = + args.options.yes === true || + (await confirmSafe({ + message: `Delete secret "${name}" (${service})?`, + initialValue: false, + nonInteractive: "fail", + hint: "Pass --yes to confirm the deletion (`hack secrets delete --yes`), or run it in a terminal.", + })); if (!ok) { return 0; } diff --git a/src/commands/session.ts b/src/commands/session.ts index d7f5a403..2eafb1f7 100644 --- a/src/commands/session.ts +++ b/src/commands/session.ts @@ -344,7 +344,16 @@ function buildSessionPickerOptions(opts: { return options; } -async function promptAttachedWorkspaceAction(opts: { +/** + * Asks whether to attach to a workspace that another client holds, or to + * create an isolated one. Without a TTY it refuses with + * `E_INTERACTIVE_REQUIRED` instead of entering the clack prompt, which cannot + * settle on a piped stdin (HACK-1385). + * + * Exported for the non-TTY regression test: in a spawned non-TTY CLI the + * picker refuses first, so this guard is exercised directly. + */ +export async function promptAttachedWorkspaceAction(opts: { readonly attachedWorkspaceName: string; readonly nextWorkspaceName: string; }): Promise<"attach" | "new" | null> { diff --git a/src/commands/ssh.ts b/src/commands/ssh.ts index 575d200a..ddf6b132 100644 --- a/src/commands/ssh.ts +++ b/src/commands/ssh.ts @@ -2,6 +2,7 @@ import * as p from "@clack/prompts"; import qrcode from "qrcode-terminal"; import type { CliContext, CommandArgs } from "../cli/command.ts"; import { defineCommand, defineOption, withHandler } from "../cli/command.ts"; +import { canPrompt, requireInteractive } from "../lib/interactivity.ts"; import { findProjectContext, type ProjectContext } from "../lib/project.ts"; import { run } from "../lib/shell.ts"; import { @@ -254,6 +255,13 @@ async function resolveConnection(opts: { } async function selectConnectionMethod(): Promise { + // A clack prompt cannot settle on a piped stdin and spins (HACK-1385). + if (!canPrompt()) { + requireInteractive({ + what: "hack ssh needs a connection method", + hint: "Pass --tailscale, or --direct with --host .", + }); + } const selected = await p.select({ message: "Connection method", options: [ @@ -283,6 +291,13 @@ async function resolveDirectHost(opts: { return hostname; } + if (!canPrompt()) { + requireInteractive({ + what: "hack ssh --direct needs a host", + hint: "Pass --host .", + }); + } + const hostInput = await p.text({ message: "SSH host (hostname or IP)", placeholder: "example.com or 192.168.1.100", @@ -344,6 +359,11 @@ async function resolveSessionNameToConnect(opts: { readonly backend: MuxBackendName; readonly sessionName: string; } | null> { + // Without a TTY, answer "Done": the connection info is already printed, and + // a workspace can be named as the positional argument instead. + if (!canPrompt()) { + return null; + } const action = await p.select({ message: "What would you like to do?", options: [ @@ -450,6 +470,12 @@ async function setupTailscale(): Promise< : "Tailscale is not connected" ); + // Never run `tailscale up` without an explicit answer. + if (!canPrompt()) { + p.log.info("Run `tailscale up`, then re-run `hack ssh`."); + return { ok: false }; + } + const turnOn = await p.confirm({ message: "Turn on Tailscale?", initialValue: true, diff --git a/tests/fixtures/release-fetch-stub-preload.ts b/tests/fixtures/release-fetch-stub-preload.ts new file mode 100644 index 00000000..5cf32e9f --- /dev/null +++ b/tests/fixtures/release-fetch-stub-preload.ts @@ -0,0 +1,42 @@ +/** + * Preload for a source CLI child (`bun --preload index.ts update`) that + * answers the GitHub "latest release" lookup with a release far newer than + * any real one, so `hack update` reaches its install confirmation without the + * network. The release lists the tarball for this platform; any other fetch + * (including the tarball download) throws, so nothing is downloaded. + */ +const LATEST_URL = + "https://api.github.com/repos/hack-dance/hack/releases/latest"; +const VERSION = "999.0.0"; + +function targetSuffix(): string { + const platform = process.platform === "darwin" ? "darwin" : "linux"; + const arch = process.arch === "arm64" ? "arm64" : "x86_64"; + return `${platform}-${arch}`; +} + +function requestUrl(input: unknown): string { + if (input instanceof Request) { + return input.url; + } + return String(input); +} + +const stubFetch = async (input: unknown): Promise => { + if (requestUrl(input) !== LATEST_URL) { + throw new Error("release-fetch-stub: unexpected fetch"); + } + return Response.json({ + tag_name: `v${VERSION}`, + assets: [ + { + name: `hack-${VERSION}-${targetSuffix()}.tar.gz`, + browser_download_url: "https://release-fetch-stub.invalid/hack.tar.gz", + }, + ], + }); +}; + +globalThis.fetch = Object.assign(stubFetch, { + preconnect: () => undefined, +}) as unknown as typeof globalThis.fetch; diff --git a/tests/fixtures/secrets-store-stub-preload.ts b/tests/fixtures/secrets-store-stub-preload.ts new file mode 100644 index 00000000..a12a96aa --- /dev/null +++ b/tests/fixtures/secrets-store-stub-preload.ts @@ -0,0 +1,37 @@ +import { spyOn } from "bun:test"; +import { appendFileSync } from "node:fs"; +import { secrets } from "bun"; + +/** + * Preload for a source CLI child (`bun --preload index.ts secrets ...`) + * that replaces `Bun.secrets` with a stub, so no test reaches the OS keychain. + * + * Each call appends one line ` ` to the file named by + * `HACK_TEST_SECRETS_LOG` (values are never written). `get` answers null, + * `set` stores nothing, and `delete` answers true, as if the secret existed. + */ +const log = process.env.HACK_TEST_SECRETS_LOG; +if (!log) { + throw new Error("Missing HACK_TEST_SECRETS_LOG"); +} +const logPath: string = log; + +function record(method: string, opts: unknown): void { + const { service, name } = (opts ?? {}) as { + readonly service?: unknown; + readonly name?: unknown; + }; + appendFileSync(logPath, `${method} ${String(service)} ${String(name)}\n`); +} + +spyOn(secrets, "get").mockImplementation(async (opts) => { + record("get", opts); + return null; +}); +spyOn(secrets, "set").mockImplementation(async (opts) => { + record("set", opts); +}); +spyOn(secrets, "delete").mockImplementation(async (opts) => { + record("delete", opts); + return true; +}); diff --git a/tests/fixtures/session-attached-prompt-worker.ts b/tests/fixtures/session-attached-prompt-worker.ts new file mode 100644 index 00000000..82f1b78d --- /dev/null +++ b/tests/fixtures/session-attached-prompt-worker.ts @@ -0,0 +1,25 @@ +import { promptAttachedWorkspaceAction } from "../../src/commands/session.ts"; +import { HackCliError } from "../../src/lib/cli-result.ts"; + +/** + * Worker for tests/no-tty-prompt-sites-cli.test.ts. It calls the session + * "workspace is attached elsewhere" prompt in its own process, so a regressed + * guard spins here and is killed by the test's timeout instead of hanging the + * test runner. Prints `action:` on success, or one `: ` + * line for a HackCliError, and exits 1 on any error. + */ +try { + const action = await promptAttachedWorkspaceAction({ + attachedWorkspaceName: "demo", + nextWorkspaceName: "demo-2", + }); + process.stdout.write(`action:${String(action)}\n`); + process.exit(0); +} catch (error: unknown) { + process.stderr.write( + error instanceof HackCliError + ? `${error.code}: ${error.message}\n` + : "E_WORKER_FAILED: unexpected error\n" + ); + process.exit(1); +} diff --git a/tests/fixtures/stdout-tty-preload.ts b/tests/fixtures/stdout-tty-preload.ts new file mode 100644 index 00000000..ebdb9dad --- /dev/null +++ b/tests/fixtures/stdout-tty-preload.ts @@ -0,0 +1,12 @@ +/** + * Preload for a source CLI child (`bun --preload index.ts ...`) whose + * stdout is a pipe: it makes `process.stdout.isTTY` report true, the shape of + * an interactive terminal whose stdin is piped (`echo | hack ...`). A prompt + * site gated on stdout alone would then enter a clack prompt that cannot + * settle on the piped stdin; `canPrompt()` must still refuse. Nothing else is + * changed. + */ +Object.defineProperty(process.stdout, "isTTY", { + configurable: true, + value: true, +}); diff --git a/tests/global-command.macos.test.ts b/tests/global-command.macos.test.ts index d483aa82..15596643 100644 --- a/tests/global-command.macos.test.ts +++ b/tests/global-command.macos.test.ts @@ -52,6 +52,8 @@ let pathExistsOverrides = new Map(); let statMetadataByPath = new Map(); let brewExecutablePath = "/opt/homebrew/bin/brew"; let confirmResponder: (() => boolean) | null = null; +/** Calls that reached the mocked clack confirm (an interactive prompt). */ +let confirmCalls = 0; const clackMock = await registerScopedModuleMock({ importerPath: import.meta.path, @@ -60,7 +62,10 @@ const clackMock = await registerScopedModuleMock({ access: async () => true, autocompleteMultiselect: async () => [], cancel: () => {}, - confirm: async () => confirmResponder?.() ?? true, + confirm: async () => { + confirmCalls += 1; + return confirmResponder?.() ?? true; + }, multiselect: async () => [], isCancel: () => false, log: { @@ -256,6 +261,7 @@ beforeEach(async () => { reachabilityByHost = {}; idUser = "mock-user"; confirmResponder = null; + confirmCalls = 0; // The clack confirm mock stands in for an interactive user, so the run must // look interactive: `confirmSafe` resolves TTY-less runs by policy without // entering the prompt (HACK-1385), and `bun test` has no TTY. @@ -516,6 +522,20 @@ test("global authorize installs passwordless dns recovery sudoers rule", async ( ]); }); +test("global authorize declines without a TTY (decline policy) and never prompts", async () => { + // HACK-1410: stdin and stdout are not TTYs and no opt-out is set. The + // decline policy answers no although the prompt's default is yes. + stubNoTty(); + const { runCli } = await import("../src/cli/run.ts"); + const code = await runCli(["global", "authorize"]); + + expect(code).toBe(0); + expect(confirmCalls).toBe(0); + expect(runCalls).not.toEqual( + expect.arrayContaining([expect.arrayContaining(["sudo", "install"])]) + ); +}); + test("global authorize refreshes an existing dns recovery sudoers rule", async () => { pathExistsOverrides.set("/etc/sudoers.d/dance.hack-dns-recovery", true); @@ -952,6 +972,20 @@ function stubInteractiveTty(): () => void { }; } +/** + * Makes stdin and stdout report no TTY for the rest of one test, on top of the + * per-test TTY stub; `afterEach` restores the original descriptors. With no + * opt-out set, `canPrompt()` is false only because there is no TTY. + */ +function stubNoTty(): void { + for (const stream of [process.stdin, process.stdout]) { + Object.defineProperty(stream, "isTTY", { + configurable: true, + value: false, + }); + } +} + /** * Restores a stubbed isTTY property. When the stream had no own isTTY * descriptor before the stub, the stubbed own property must be deleted @@ -1282,7 +1316,17 @@ test("global trust falls back to an existing exported CA when Caddy is unavailab ); }); -test("global trust proceeds under --no-interactive when sudo is passwordless", async () => { +test.each([ + { mode: "under --no-interactive", flag: ["--no-interactive"], tty: true }, + // HACK-1410: a TTY-less run with no opt-out resolves the same way. + { mode: "without a TTY", flag: [], tty: false }, +])("global trust proceeds $mode when sudo is passwordless (accept-default policy)", async ({ + flag, + tty, +}) => { + if (!tty) { + stubNoTty(); + } const caddyCompose = join( tempDir!, GLOBAL_HACK_DIR_NAME, @@ -1345,7 +1389,7 @@ test("global trust proceeds under --no-interactive when sudo is passwordless", a }; const { runCli } = await import("../src/cli/run.ts"); - const code = await runCli(["global", "trust", "--no-interactive"]); + const code = await runCli(["global", "trust", ...flag]); const bundlePath = join( tempDir!, @@ -1358,6 +1402,7 @@ test("global trust proceeds under --no-interactive when sudo is passwordless", a expect(code).toBe(0); // Non-interactive proceeds with the documented default (trust): the sudo // keychain install runs without ever calling clack's confirm prompt. + expect(confirmCalls).toBe(0); expect(runCalls).toEqual( expect.arrayContaining([ [ diff --git a/tests/no-tty-prompt-sites-cli.test.ts b/tests/no-tty-prompt-sites-cli.test.ts new file mode 100644 index 00000000..8e3df426 --- /dev/null +++ b/tests/no-tty-prompt-sites-cli.test.ts @@ -0,0 +1,444 @@ +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + expect, + test, +} from "bun:test"; +import { + chmod, + mkdir, + mkdtemp, + readFile, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1410: one bounded non-TTY regression test per `canPrompt()` guard in + * front of a clack prompt (HACK-1385). A clack prompt opens a terminal + * readline over stdin; on a piped stdin it never settles and spins at full + * CPU. Each case runs a real child process with a piped stdin that carries + * nothing and closes, no `HACK_NO_INTERACTIVE`, and a SIGKILL after + * {@link KILL_AFTER_MS}. A guard that is removed or made a no-op therefore + * fails by the kill (signal set, wall time at the bound) or by the wrong exit + * and output, never by hanging the suite. + * + * Isolation: every child gets `privateCliEnv` (private HOME and Docker client + * config) plus a private HACK_HOME, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1` + * and `HACK_SESSIONS_MUX=none` (no tmux or zellij is run). `secrets` children + * preload a `Bun.secrets` stub, so no case reaches the OS keychain even when a + * guard is mutated away. `update` children preload a fetch stub, so nothing + * reaches the network. No Docker socket, launchd or keychain is touched. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const FIXTURES = resolve(import.meta.dir, "fixtures"); +const STDOUT_TTY_PRELOAD = join(FIXTURES, "stdout-tty-preload.ts"); +const SECRETS_STUB_PRELOAD = join(FIXTURES, "secrets-store-stub-preload.ts"); +const RELEASE_STUB_PRELOAD = join(FIXTURES, "release-fetch-stub-preload.ts"); +const ATTACHED_PROMPT_WORKER = join( + FIXTURES, + "session-attached-prompt-worker.ts" +); +/** + * Hard bound for one child. A settled run takes well under a second once the + * shared Bun cache is warm; the regressed prompt loop never exits. + */ +const KILL_AFTER_MS = 10_000; +/** Overall bound for each test, covering a cold first start. */ +const TEST_TIMEOUT_MS = 30_000; +/** Clack's active-prompt marker: no case may render a live prompt. */ +const ACTIVE_PROMPT_MARKER = "◆"; + +type RunResult = { + readonly exitCode: number; + readonly signal: string | null; + readonly stdout: string; + readonly stderr: string; + readonly wallMs: number; +}; + +let fileRoot: string | null = null; +let caseRoot: string | null = null; + +beforeAll(async () => { + // One HOME per file keeps Bun's transpiler cache warm across cases. + fileRoot = await mkdtemp(join(tmpdir(), "hack-no-tty-prompt-sites-")); +}); + +afterAll(async () => { + if (fileRoot) { + await rm(fileRoot, { recursive: true, force: true }); + fileRoot = null; + } +}); + +beforeEach(async () => { + caseRoot = await mkdtemp(join(requireFileRoot(), "case-")); +}); + +afterEach(() => { + caseRoot = null; +}); + +function requireFileRoot(): string { + if (!fileRoot) { + throw new Error("Missing file root"); + } + return fileRoot; +} + +function requireCaseRoot(): string { + if (!caseRoot) { + throw new Error("Missing case root"); + } + return caseRoot; +} + +/** + * Spawns `bun [--preload ...]