From b122fba5531e4acb34b3aed6a835421c6d07c612 Mon Sep 17 00:00:00 2001 From: Volker Scheuber Date: Fri, 9 Oct 2026 22:37:27 -0600 Subject: [PATCH 1/5] feat: interactive gap-fill prompts for missing command input (Phase 1) Implements Phase 1 of INTERACTIVE-COMMANDS-PLAN.md: on an interactive session, missing mandatory options and connection arguments are now prompted for before the action body runs; every non-interactive surface (CI, scripts, MCP, e2e) behaves byte-identically to before. - PromptGateState: pure prompt gate (FRODO_NO_PROMPT / FRODO_TEST / TTYs / setNeverPrompt kill switch), unit-testable, 4 unit tests. - PromptGate: promptInput/promptPassword hand-built on @inquirer/core, matching the established hand-built prompt pattern (Escape/empty-Enter resolve '' so callers keep their existing missing-input error paths). readline-buffer based (paste-safe); the completed line is mirrored from the preceding keystroke because rl.line is already consumed when Enter's keypress fires (verified against @inquirer/core 12.0.3). - ResolveInteractiveInputs: preAction resolution pass. Host picker from saved connection profiles (single profile skipped silently, dropdown with "Enter a host manually..." otherwise, free text when none); username/password prompts only when no saved profile can supply a usable credential; deferred mandatory-option prompts (dropdown when the option carries choices()). - FrodoStubCommand reimplements missingMandatoryOptionValue verbatim from commander (typings omit it) as the override point; FrodoCommand defers violations to the resolution pass instead of throwing, which makes all 57 existing makeOptionMandatory() sites prompt-capable with zero syntax change. Non-interactive sessions re-throw commander's exact message/code. - Injection writes both processedArgs (action parameter source) and command.args (handleDefaultArgsAndOpts source) - the two surfaces commander/this repo read, verified against both. - --no-prompt on every FrodoCommand (Runtime group); MCP stdio server start calls setNeverPrompt() as belt-and-braces. - inquirer packages moved devDependencies -> dependencies (production prompt surface now). All flows verified live in a PTY (host dropdown, single-profile skip, username prefill, password masking, escape abort, --no-prompt, FRODO_NO_PROMPT, enum dropdown, non-TTY error equivalence, bare frodo conn unchanged). Co-Authored-By: Claude Code --- package-lock.json | 24 +- package.json | 8 +- src/cli/FrodoCommand.ts | 88 ++++ src/cli/ResolveInteractiveInputs.ts | 412 ++++++++++++++++++ src/cli/mcp/server/server-start.ts | 9 + src/utils/interactive/PromptGate.ts | 174 ++++++++ src/utils/interactive/PromptGateState.test.ts | 44 ++ src/utils/interactive/PromptGateState.ts | 56 +++ 8 files changed, 795 insertions(+), 20 deletions(-) create mode 100644 src/cli/ResolveInteractiveInputs.ts create mode 100644 src/utils/interactive/PromptGate.ts create mode 100644 src/utils/interactive/PromptGateState.test.ts create mode 100644 src/utils/interactive/PromptGateState.ts diff --git a/package-lock.json b/package-lock.json index 9ddf5a806..3ab6f7109 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,15 +8,17 @@ "name": "@rockcarver/frodo-cli", "version": "5.0.0-5", "license": "MIT", + "dependencies": { + "@inquirer/confirm": "6.3.2", + "@inquirer/core": "^12.0.1", + "@inquirer/select": "^5.2.3" + }, "bin": { "frodo": "dist/launch.cjs" }, "devDependencies": { "@eslint/js": "10.0.1", "@ianvs/prettier-plugin-sort-imports": "4.7.1", - "@inquirer/confirm": "6.3.2", - "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0", @@ -893,7 +895,6 @@ "version": "2.0.8", "resolved": "https://registry.npmjs.org/@inquirer/ansi/-/ansi-2.0.8.tgz", "integrity": "sha512-WpQM+Ti6Z40EFwwt+uL2p4UabT+W179zHp6HhLVOzfbwnVn05IPO/eXIZXGNqcT1jbQ15SujNLzQ39k4QPPxBQ==", - "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -903,7 +904,6 @@ "version": "6.3.2", "resolved": "https://registry.npmjs.org/@inquirer/confirm/-/confirm-6.3.2.tgz", "integrity": "sha512-Xvr/0HggjddPtGppuqVmxhTw+Hr8PvsZ/k0HmOEaAqQEt80OITNkFWnsdNmyT0/eM4Ab+iJLx2R8rctlEyfSVg==", - "dev": true, "license": "MIT", "dependencies": { "@inquirer/core": "^12.0.3", @@ -925,7 +925,6 @@ "version": "12.0.3", "resolved": "https://registry.npmjs.org/@inquirer/core/-/core-12.0.3.tgz", "integrity": "sha512-wsSy0sznmXwkty+2PzZwx00Cazc/E0r0B7mAzdGROz2Ct+DFZXaK7WDjGZvgjRldxH5ZhFVfF2lgkYrqgOw2KA==", - "dev": true, "license": "MIT", "dependencies": { "@inquirer/ansi": "^2.0.8", @@ -952,7 +951,6 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz", "integrity": "sha512-bzyZ1e88w9O1iNJbKnOlvYTrWPDl46O1bG0D3XInv+9tkPrxrN8jUUTiFlDkkmKWgn1M6CfIA13SuGqOa9Korw==", - "dev": true, "license": "ISC", "engines": { "node": ">=14" @@ -965,7 +963,6 @@ "version": "2.0.9", "resolved": "https://registry.npmjs.org/@inquirer/figures/-/figures-2.0.9.tgz", "integrity": "sha512-EAWgUTGQ/Umgga51dE3B2PUHbufuXarDfg86uVgoSgNHNNQnyFKcOrQLWVqYMghuSyHh8+2HUH0Js9cTC1WAdg==", - "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -975,7 +972,6 @@ "version": "5.2.5", "resolved": "https://registry.npmjs.org/@inquirer/select/-/select-5.2.5.tgz", "integrity": "sha512-9kc15hr8r/kI+3DO/xLog5nOzTz1jqsHXa6JBFzmQKhkoJ8Slda1I1L/uD8ZSZ9tF1yp79wwXe7mclvX1rqR2Q==", - "dev": true, "license": "MIT", "dependencies": { "@inquirer/ansi": "^2.0.8", @@ -999,7 +995,6 @@ "version": "4.1.1", "resolved": "https://registry.npmjs.org/@inquirer/type/-/type-4.1.1.tgz", "integrity": "sha512-yJoHYrMnxIsJZCY+0Vb66Dy3he3kL3e2wOBKhoSwWWAzZAY82emlxwgprCtp6yRixvNRNq9ztfRWQYPNr3Go7A==", - "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -2634,7 +2629,7 @@ "version": "26.6.4", "resolved": "https://registry.npmjs.org/@types/node/-/node-26.6.4.tgz", "integrity": "sha512-ldVPDCzj7fsaGZrLB0NuHuTvJcsNasysBAqMolr/cgxrLd1xbqxIr3XJiPnHHJUCxj5sNF1vnRj9aWnrVh5Jcg==", - "dev": true, + "devOptional": true, "license": "MIT", "dependencies": { "undici-types": "~8.9.0" @@ -4171,7 +4166,6 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/cli-width/-/cli-width-4.1.0.tgz", "integrity": "sha512-ouuZd4/dm2Sw5Gmqy6bGyNNNe1qt9RpmxveLSO7KcgsTnU7RXfsw+/bukWGo1abgBiMAic068rclZsO4IWmmxQ==", - "dev": true, "license": "ISC", "engines": { "node": ">= 12" @@ -5091,14 +5085,12 @@ "version": "3.0.3", "resolved": "https://registry.npmjs.org/fast-string-truncated-width/-/fast-string-truncated-width-3.0.3.tgz", "integrity": "sha512-0jjjIEL6+0jag3l2XWWizO64/aZVtpiGE3t0Zgqxv0DPuxiMjvB3M24fCyhZUO4KomJQPj3LTSUnDP3GpdwC0g==", - "dev": true, "license": "MIT" }, "node_modules/fast-string-width": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/fast-string-width/-/fast-string-width-3.0.2.tgz", "integrity": "sha512-gX8LrtNEI5hq8DVUfRQMbr5lpaS4nMIWV+7XEbXk2b8kiQIizgnlr12B4dA3ZEx3308ze0O4Q1R+cHts8kyUJg==", - "dev": true, "license": "MIT", "dependencies": { "fast-string-truncated-width": "^3.0.2" @@ -5108,7 +5100,6 @@ "version": "0.2.2", "resolved": "https://registry.npmjs.org/fast-wrap-ansi/-/fast-wrap-ansi-0.2.2.tgz", "integrity": "sha512-7F2Fl+TjRSenLqlU3UjSH0iyqopqoZIu7eZVpEirP2g1GtWa2G/ecEmBdgz31+Mxr+ELclgg6sokpSFIQiZ02Q==", - "dev": true, "license": "MIT", "dependencies": { "fast-string-width": "^3.0.2" @@ -6838,7 +6829,6 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-3.0.0.tgz", "integrity": "sha512-dkEJPVvun4FryqBmZ5KhDo0K9iDXAwn08tMLDinNdRBNPcYEDiWYysLcc6k3mjTMlbP9KyylvRpd4wFtwrT9rw==", - "dev": true, "license": "ISC", "engines": { "node": "^20.17.0 || >=22.9.0" @@ -8551,7 +8541,7 @@ "version": "8.9.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.9.0.tgz", "integrity": "sha512-KTDyRTYX8sWmKXAikPHHSyc63CRPETMctyjKFupcC6OBLXT3xsN0e9aF7m+mIXutFWpUXuedtowG7iLOzp0kQg==", - "dev": true, + "devOptional": true, "license": "MIT" }, "node_modules/universalify": { diff --git a/package.json b/package.json index 6680324fe..8e3fd395b 100644 --- a/package.json +++ b/package.json @@ -124,9 +124,6 @@ "devDependencies": { "@eslint/js": "10.0.1", "@ianvs/prettier-plugin-sort-imports": "4.7.1", - "@inquirer/confirm": "6.3.2", - "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0", @@ -176,5 +173,10 @@ }, "overrides": { "argparse": "^2.0.1" + }, + "dependencies": { + "@inquirer/confirm": "6.3.2", + "@inquirer/core": "^12.0.1", + "@inquirer/select": "^5.2.3" } } diff --git a/src/cli/FrodoCommand.ts b/src/cli/FrodoCommand.ts index 15e6230f8..0145a6df9 100644 --- a/src/cli/FrodoCommand.ts +++ b/src/cli/FrodoCommand.ts @@ -25,7 +25,12 @@ import { updateProgressIndicator, verboseMessage, } from '../utils/Console.js'; +import { setNeverPrompt } from '../utils/interactive/PromptGate.js'; import { activatePersistedTheme } from '../utils/ThemeConfig.js'; +import { + deferMissingMandatoryOption, + resolveInteractiveInputs, +} from './ResolveInteractiveInputs.js'; // Frodo constants const constants = frodo.utils.constants; @@ -867,6 +872,25 @@ const forceUpdateOption = withHelpGroup( OptionCategory.Runtime ); +// Opt-out of the interactive gap-fill (INTERACTIVE-COMMANDS-PLAN.md, +// Phase 1): with this flag, missing mandatory options/connection args +// fail with the classic errors instead of prompting, even on a TTY. +// Categorized as Runtime (it is a per-invocation behavior control) and +// also honored process-wide via FRODO_NO_PROMPT (see PromptGate). +// Commander negation semantics: the lone `--no-prompt` flag's attribute +// name is `prompt`, defaults to true (prompting allowed), and passing +// --no-prompt sets it false. Do NOT chain .default(false) here -- that +// would read as "prompting disabled by default" and disable the whole +// feature (caught in review of the first cut). +const promptOption = withHelpGroup( + new Option( + '--no-prompt', + 'Never prompt for missing input; fail with the usual errors instead (same as setting FRODO_NO_PROMPT).' + ).default(true, 'Prompt for missing input where supported'), + RUNTIME_OPTIONS_HEADING, + OptionCategory.Runtime +); + const insecureOption = withHelpGroup( new Option( '-k, --insecure', @@ -1001,6 +1025,7 @@ const defaultOpts = [ envOption, envFileOption, forceUpdateOption, + promptOption, ]; /** @@ -2005,6 +2030,13 @@ export function normalizeExpandedHelpArgv(argv: string[] = process.argv) { } const warnedStabilityCommands = new Set(); +// Leaf commands whose interactive input resolution already ran (or is +// running) this process. The preAction hook is registered on every +// FrodoCommand in the ancestry and all receive the same actionCommand, so +// without this the resolution would run once per ancestor. Also guards +// against a re-entrant double-run if a hook ever fires twice for the same +// command instance. +const resolvedActionCommands = new WeakSet(); const warnedStabilityOptions = new Set(); /** @@ -2308,8 +2340,52 @@ export class FrodoStubCommand extends Command { cleanupProgressIndicators(); }); + // Async preAction hook: commander's hook chain awaits returned + // promises (_chainOrCall), so the action handler below only starts + // after interactive input resolution finished. The hook is registered + // on every FrodoCommand in the ancestry and all receive the same + // actionCommand, so dedupe on the executing command. this.hook('preAction', (_thisCommand, actionCommand) => { enforceStabilityAndWarn(actionCommand); + // Commander negation: `prompt` defaults to true; --no-prompt sets + // it false. Set once per process, before the resolution hook below + // (hook order = registration order). + if ( + actionCommand.getOptionValue(promptOption.attributeName()) === false + ) { + setNeverPrompt(); + } + }); + + this.hook('preAction', async (_thisCommand, actionCommand) => { + // Only leaf FrodoCommands get the resolution pass. Stub commands' + // own bare actions (conn, settings, login -- all already + // interactive by design) must not run it, or they would + // double-prompt. + if (!(actionCommand instanceof FrodoCommand)) return; + if (resolvedActionCommands.has(actionCommand)) return; + resolvedActionCommands.add(actionCommand); + await resolveInteractiveInputs(actionCommand); + }); + } + + /** + * Commander calls this during parsing, BEFORE the preAction hook chain, + * when a `makeOptionMandatory()` option is missing. Reimplemented here + * (verbatim from commander's own implementation, whose typings omit the + * method -- so an override below cannot `super` into it) as the shared + * declaration point both subclasses control: stub commands keep this + * verbatim throw (today's behavior, unchanged), and FrodoCommand + * replaces it with deferral (see ResolveInteractiveInputs) so the + * preAction resolution pass can prompt for the value on an interactive + * session, or re-throw this exact error on a non-interactive one. That + * is what makes all existing `makeOptionMandatory()` call sites + * prompt-capable with zero syntax change. + * @param option The mandatory option that is missing. + */ + missingMandatoryOptionValue(option: Option): void { + this.error(`error: required option '${option.flags}' not specified`, { + code: 'commander.missingMandatoryOptionValue', }); } @@ -2744,6 +2820,18 @@ export class FrodoCommand extends FrodoStubCommand { */ types: string[]; + /** + * Deferred mandatory handling (see FrodoStubCommand's implementation + * for the verbatim-commander default): record the violation instead of + * throwing, so the preAction resolution pass can prompt for the value + * on an interactive session, or re-throw this exact error on a + * non-interactive one. + * @param option The mandatory option that is missing. + */ + missingMandatoryOptionValue(option: Option): void { + deferMissingMandatoryOption(this, option.flags); + } + /** * Creates a new FrodoCommand instance * @param name Name of the command diff --git a/src/cli/ResolveInteractiveInputs.ts b/src/cli/ResolveInteractiveInputs.ts new file mode 100644 index 000000000..6d554624d --- /dev/null +++ b/src/cli/ResolveInteractiveInputs.ts @@ -0,0 +1,412 @@ +/** + * The Phase 1 "zero-touch" resolution pass: fills missing command input + * interactively before the action body runs (INTERACTIVE-COMMANDS-PLAN.md, + * Phase 1). Wired from FrodoCommand's preAction hook; never modifies + * frodo-lib. + * + * Two resolver kinds run here, connection arguments first (the host gives + * the later prompts their context), then deferred mandatory options: + * + * 1. Connection arguments. Missing `host` (dropdown of saved connection + * profiles, single profile silently assumed, free-text entry when none + * exist -- see runInteractivePreferredCredentialPicker for the + * precedent), missing `username`/`password` -- but ONLY when no saved + * profile can supply credentials for the resolved host, since frodo-lib + * already falls back to profile-stored credentials on its own + * (AuthenticateOps loadConnectionProfile); prompting when a profile + * would answer anyway would be pure friction. `--browser`/`--device` + * likewise suppress the username/password prompts (their flow is the + * interactive part) but not the host prompt, which those flows still + * need. + * + * 2. Deferred mandatory options. Commander's + * `missingMandatoryOptionValue()` normally throws during parsing -- + * BEFORE the preAction hook chain runs. FrodoCommand overrides it to + * record the violation instead, which makes all 57 existing + * `makeOptionMandatory()` call sites prompt-capable with zero syntax + * change. Options carrying commander `choices()` are offered as a + * dropdown instead of free typing. + * + * Everything here is gap-filling, and every gap-fill writes into the + * command's own `processedArgs`/option values -- NOT into frodo-lib state + * -- because `handleDefaultArgsAndOpts` (which copies positional args into + * state) runs INSIDE the action body, after this pass; writing state here + * would be overwritten by `undefined` a moment later. A fully-specified + * invocation reaches the action untouched, and outside the prompt gate + * (see PromptGate) the pass degrades to exactly the old error behavior. + */ + +import fs from 'fs'; +import { frodo, state } from '@rockcarver/frodo-lib'; +import type { Command } from 'commander'; +import { verboseMessage } from '../utils/Console'; +import { + escapableSelect, + ESCAPE, + type EscapableSelectChoice, +} from '../utils/interactive/EscapableSelectPrompt'; +import { + canPrompt, + promptInput, + promptPassword, +} from '../utils/interactive/PromptGate'; + +const { getConnectionProfilesPath, getConnectionProfileByHost } = frodo.conn; + +/** + * A mandatory-option violation that commander's parse recorded instead of + * throwing on. Lives here rather than on the command instance so the + * deferred state survives the parse->hook handoff regardless of how + * commander clones or nests commands. + */ +const pendingMandatoryViolations = new WeakMap(); + +/** + * Records a mandatory option that was missing at parse time, instead of + * throwing. Called from FrodoCommand's `missingMandatoryOptionValue` + * override. `optionLabel` is the option's full flags string (e.g. + * `-s, --schema `), matching what commander's own error message + * embeds. + */ +export function deferMissingMandatoryOption( + command: Command, + optionLabel: string +): void { + const pending = pendingMandatoryViolations.get(command) ?? []; + pending.push(optionLabel); + pendingMandatoryViolations.set(command, pending); +} + +/** + * The CommanderError code commander itself uses for this failure; the + * re-throw preserves it so scripts can match on it. + */ +export const MISSING_MANDATORY_OPTION_CODE = + 'commander.missingMandatoryOptionValue'; + +/** + * Rebuilds commander's exact error message for a deferred violation (see + * missingMandatoryOptionValue in commander's lib/command.js). + */ +function missingMandatoryOptionMessage(optionLabel: string): string { + return `error: required option '${optionLabel}' not specified`; +} + +/** + * Runs the interactive resolution pass for `command` (the leaf command + * about to execute). + * + * When prompting is disallowed, any deferred mandatory violation is + * re-thrown with commander's own message/code (identical to pre-Phase-1 + * behavior -- commander would have thrown during parsing) and everything + * else is a no-op. When the user backs out of a prompt (Escape/empty + * Enter), any deferred violations are re-thrown the same way (matching + * the old parse order, where the mandatory check ran first) and missing + * connection args are left for the action's getTokens() to fail on + * exactly as before. + */ +export async function resolveInteractiveInputs( + command: Command +): Promise { + const violations = pendingMandatoryViolations.get(command) ?? []; + + if (!canPrompt()) { + if (violations.length > 0) { + throwMissingMandatory(command, violations[0]); + } + return; + } + + // Interactive: connection args first (they give the mandatory-option + // prompts their tenant context), then mandatory options. Either step + // bailing out falls back to the pre-Phase-1 error semantics. + const connectionResolved = await resolveConnectionArgs(command); + const mandatoryResolved = connectionResolved + ? await resolveMandatoryOptions(command) + : false; + if (!connectionResolved || !mandatoryResolved) { + if (violations.length > 0) { + throwMissingMandatory(command, violations[0]); + } + } +} + +/** + * Emits commander's exact mandatory-option failure: same message, same + * CommanderError code, same stderr output and exit path as pre-Phase-1. + */ +function throwMissingMandatory(command: Command, optionLabel: string): never { + command.error(missingMandatoryOptionMessage(optionLabel), { + code: MISSING_MANDATORY_OPTION_CODE, + }); +} + +/** + * Prompts for each deferred mandatory option violation. Options with a + * closed value set (commander `choices()`) get a dropdown; everything + * else free text. Returns false as soon as the user declines one (Escape + * or empty Enter) -- the caller then restores the old error semantics. + */ +async function resolveMandatoryOptions(command: Command): Promise { + const violations = pendingMandatoryViolations.get(command) ?? []; + for (const violation of violations) { + const option = command.options.find((candidate) => { + return candidate.flags === violation; + }); + if (!option) { + // Should not happen (the label came from the option itself), but + // degrade to the old error rather than guessing. + throwMissingMandatory(command, violation); + } + const attributeName = option.attributeName(); + + let value: string | undefined; + if (option.argChoices && option.argChoices.length > 0) { + const choice = await escapableSelect({ + message: `Choose a value for ${option.flags}:`, + choices: option.argChoices.map( + (choiceValue): EscapableSelectChoice => ({ + name: choiceValue, + value: choiceValue, + }) + ), + }); + if (choice === ESCAPE) return false; + value = choice; + } else { + const answer = await promptInput({ + message: `Enter a value for ${option.flags}:`, + }); + if (!answer) return false; + value = answer; + } + command.setOptionValueWithSource(attributeName, value, 'cli'); + verboseMessage(`Using ${value} for ${option.flags}.`); + } + return true; +} + +/** + * Index of the default positional argument of that name on this command + * (commands omit default args via the FrodoCommand omits list, so the + * index differs per command), or -1 when the command doesn't declare it. + */ +function positionalIndex(command: Command, name: string): number { + return command.registeredArguments.findIndex( + (argument) => argument.name() === name + ); +} + +function positionalValue(command: Command, name: string): string | undefined { + const index = positionalIndex(command, name); + if (index === -1) return undefined; + const value = command.processedArgs?.[index]; + return typeof value === 'string' && value.length > 0 ? value : undefined; +} + +function setPositionalValue( + command: Command, + name: string, + value: string +): void { + const index = positionalIndex(command, name); + if (index === -1) return; + // Two surfaces must see the injected value: + // + // - `processedArgs` is what the action handler's positional parameters + // are sliced from (commander's action() wrapper slices + // this.processedArgs). + // - `args` (raw, options-removed user args) is what + // handleDefaultArgsAndOpts iterates to copy default arguments into + // frodo-lib state INSIDE the action body -- it does NOT read + // processedArgs, so a processedArgs-only injection would be + // overwritten with `undefined` the moment the action starts. + // Verified against commander's _processArguments (fills processedArgs + // from args, never writes back) and this repo's + // handleDefaultArgsAndOpts (iterates command.args). + if (command.processedArgs) { + command.processedArgs[index] = value; + } + // `args` holds only user-supplied positional values; a gap-fill by + // definition fills a position the user left empty, so extending/padding + // with the injected value is safe. Pad with nulls for any inner + // positions the user skipped (rare: inner default args are optional), + // since args is positional. + if (Array.isArray(command.args)) { + while (command.args.length < index) { + command.args.push(null); + } + if (index === command.args.length) { + command.args.push(value); + } else if (command.args[index] == null) { + command.args[index] = value; + } + } +} + +/** + * Whether the invocation carries an explicit interactive-auth mode + * (--browser/--device). Read from the parsed option values, NOT from + * frodo-lib state: `handleDefaultArgsAndOpts` sets authMode from these + * same options inside the action body, i.e. after this pass runs. + */ +function isInteractiveAuthMode(command: Command): boolean { + return ( + !!command.getOptionValue('browser') || !!command.getOptionValue('device') + ); +} + +/** + * Reads the saved connection profiles file directly (no auth, no network) + * to enumerate candidates for the host dropdown. Returns a sorted list; + * empty when the file is missing or unreadable. + */ +function listSavedProfiles(): { host: string; alias?: string }[] { + const filename = getConnectionProfilesPath(); + let connectionsData: Record; + try { + connectionsData = JSON.parse(fs.readFileSync(filename, 'utf8')); + } catch { + return []; + } + return Object.keys(connectionsData) + .sort() + .map((host) => ({ host, alias: connectionsData[host].alias })); +} + +// Distinct from any real host string so it can't collide with one. +const ENTER_MANUALLY = Symbol('resolveConnectionArgs:enterManually'); + +/** + * Prompts for the missing connection-level positional arguments: host, + * then username/password -- the latter only when no saved profile can + * supply credentials for the resolved host. Returns false when the user + * declines a prompt (Escape or empty Enter); the caller then restores the + * pre-Phase-1 error semantics. + * + * Skipped per-argument when: the command doesn't declare the argument, + * the value is already present (positional arg, or env var via frodo-lib + * state's own env fallback), or --browser/--device makes the + * username/password prompts redundant. + */ +async function resolveConnectionArgs(command: Command): Promise { + // -- host ------------------------------------------------------------- + if ( + positionalIndex(command, 'host') !== -1 && + !positionalValue(command, 'host') && + !state.getHost() // env fallback (FRODO_HOST) + ) { + const profiles = listSavedProfiles(); + let picked: string | undefined; + + if (profiles.length === 1) { + // Single profile: no menu -- mirror the established convention + // (runInteractivePreferredCredentialPicker). Announced only under + // --verbose, since this runs inside an otherwise-quiet command. + picked = profiles[0].host; + verboseMessage( + `No host given; using the only saved connection profile: ${picked}` + ); + } else if (profiles.length > 1) { + const choice = await escapableSelect({ + message: 'Choose a connection profile:', + choices: [ + { + value: ENTER_MANUALLY, + name: 'Enter a host manually...', + description: 'Type an AM base URL (or a profile substring).', + }, + ...profiles.map( + ( + profile + ): EscapableSelectChoice => ({ + value: profile.host, + name: profile.alias + ? `${profile.host} (${profile.alias})` + : profile.host, + }) + ), + ], + }); + if (choice === ESCAPE) return false; + if (choice === ENTER_MANUALLY) { + const answer = await promptInput({ + message: 'AM base URL (or profile alias/substring):', + }); + if (!answer) return false; + picked = answer; + } else { + picked = choice; + } + } else { + // No saved profiles at all: free-text entry, same as the "enter + // manually" path above. + const answer = await promptInput({ + message: 'AM base URL:', + }); + if (!answer) return false; + picked = answer; + } + setPositionalValue(command, 'host', picked); + } + + // -- username / password ---------------------------------------------- + // Prompted ONLY when frodo-lib's own resolution can't answer: a saved + // profile for the (now-resolved) host supplies stored credentials on + // its own (AuthenticateOps loadConnectionProfile), so prompting would + // be pure friction. No profile (new host, or host given as a URL with + // nothing saved): prompt bare, so e.g. `frodo login ` and + // `frodo conn save ` become fully interactive. + const usernameNeeded = + positionalIndex(command, 'username') !== -1 && + !positionalValue(command, 'username') && + !state.getUsername() && // env fallback (FRODO_USERNAME) + !isInteractiveAuthMode(command); + const passwordNeeded = + positionalIndex(command, 'password') !== -1 && + !positionalValue(command, 'password') && + !state.getPassword() && // env fallback (FRODO_PASSWORD) + !isInteractiveAuthMode(command); + + if (!usernameNeeded && !passwordNeeded) return true; + + const host = positionalValue(command, 'host') ?? state.getHost(); + if (!host) return true; // host unresolved; the action fails on it as before + + let savedUsername: string | undefined; + try { + const profile = await getConnectionProfileByHost(host); + // frodo-lib will authenticate from the profile alone when it carries + // any usable credential (user/svcacct/amster); a bare profile entry + // (host saved without credentials -- possible via earlier frodo + // versions and manual edits) answers nothing, so the prompts still + // run for it. A username-only entry prefills the username prompt. + if ( + (profile.username && profile.password) || + profile.svcacctId || + profile.amsterPrivateKey + ) { + return true; + } + savedUsername = profile.username ?? undefined; + } catch { + // No profile for this host (or unreadable): fall through to prompts. + } + + if (usernameNeeded) { + const answer = await promptInput({ + message: 'Username:', + default: savedUsername, + }); + if (!answer) return false; + setPositionalValue(command, 'username', answer); + } + + if (passwordNeeded) { + const answer = await promptPassword({ message: 'Password:' }); + if (!answer) return false; + setPositionalValue(command, 'password', answer); + } + + return true; +} diff --git a/src/cli/mcp/server/server-start.ts b/src/cli/mcp/server/server-start.ts index 8e029f045..dbce5d861 100644 --- a/src/cli/mcp/server/server-start.ts +++ b/src/cli/mcp/server/server-start.ts @@ -36,6 +36,7 @@ import { } from '../../../ops/McpServerOps.js'; import c from '../../../utils/ColorTheme'; import { printMessage } from '../../../utils/Console'; +import { setNeverPrompt } from '../../../utils/interactive/PromptGate'; import { FrodoCommand } from '../../FrodoCommand'; import { resolveMcpAuthTokenValue } from './server-auth'; import { @@ -387,6 +388,14 @@ export default function setup() { throw new Error('--json is only supported with --dry-run.'); } const transport = opts.transport ?? 'stdio'; + if (transport === 'stdio') { + // Belt and braces: a stdio MCP server's stdin/stdout ARE the + // protocol channel, so nothing in this process may ever prompt. + // (Pipes are non-TTY anyway, so the PromptGate's natural check + // already refuses; this makes the guarantee explicit rather than + // relying on the environment.) See PromptGate. + setNeverPrompt(); + } const authToken = transport === 'http' ? resolveMcpAuthToken(opts) : undefined; if (opts.oauthResourceServer) { diff --git a/src/utils/interactive/PromptGate.ts b/src/utils/interactive/PromptGate.ts new file mode 100644 index 000000000..6453b63a6 --- /dev/null +++ b/src/utils/interactive/PromptGate.ts @@ -0,0 +1,174 @@ +/** + * Central gate deciding whether frodo may prompt interactively, plus the + * terminal prompts built on `@inquirer/core` for gap-filling missing + * command input (INTERACTIVE-COMMANDS-PLAN.md, Phase 1). + * + * ## The gate + * + * Re-exported from `PromptGateState.ts` (which holds its full + * documentation and stays importable from unit tests -- this file pulls + * in the frodo-lib-importing ColorTheme, and this CLI's Jest setup cannot + * load frodo-lib; see src/app.commandWiring.test.ts's header comment). + * + * ## The prompts + * + * Deliberately hand-built on `@inquirer/core` (same pattern as the + * existing exemplars `EscapableSelectPrompt.ts`/`JourneyDebugPrompt.ts`) + * rather than adding `@inquirer/input`/`@inquirer/password`: the packaged + * prompts throw `ExitPromptError` on Escape and EOF, while the gap-fill + * layer needs Escape to resolve to a sentinel so callers can fall through + * to their pre-existing missing-input error paths -- which is exactly the + * contract the existing hand-built prompts already established. + */ + +import { + createPrompt, + isEnterKey, + useKeypress, + usePrefix, + useState, +} from '@inquirer/core'; +import c from '../ColorTheme'; +import { canPrompt } from './PromptGateState'; + +export { canPrompt, setNeverPrompt } from './PromptGateState'; + +/** + * The empty-string result of `promptInput`/`promptPassword`. The prompts + * resolve with `''` in two cases that callers must tell apart, so this + * sentinel is what they branch on: the user pressed Enter on an empty + * line (meaning "I have nothing to type", e.g. no password), versus + * `undefined`, which means prompting was disallowed by the gate and the + * caller should simply fall through to its pre-existing missing-input + * error path without printing anything prompt-related. + * + * Escape ALSO resolves with `''` -- for gap-fills, "never mind, I'll type + * the flag myself" and "there is no password" have the same practical + * outcome (no value supplied, command proceeds to its existing error), so + * a separate escape sentinel would only add API surface. `promptInput` + * renders `(no input)` after resolving so the transcript shows what + * happened. + */ +export const EMPTY_INPUT = ''; + +export type TextInputConfig = { + message: string; + /** + * Value applied when the user presses Enter on an empty line (shown as + * a muted hint). When omitted, Enter-on-empty resolves `''` and the + * caller decides what that means. + */ + default?: string; +}; + +/** + * A `promptInput`/`promptPassword` result: `undefined` (gate refused -- + * caller falls through silently to its existing missing-input path), + * `''` (user aborted/declined -- caller treats as missing input with its + * existing error), or a non-empty string. + */ +export type TextInputResult = string | undefined; + +/** + * Read a line of free-form text. Paste-safe and editable (the readline + * buffer does the editing; the prompt only reads it), unlike accumulating + * individual keypresses by hand. + */ +export async function promptInput( + config: TextInputConfig +): Promise { + if (!canPrompt()) return undefined; + return textPrompt(config, { suppress: false }); +} + +/** + * Read a line of text without echoing it (passwords, passphrases, and + * similar secrets). Same result contract as `promptInput`. + */ +export async function promptPassword( + config: TextInputConfig +): Promise { + if (!canPrompt()) return undefined; + return textPrompt(config, { suppress: true }); +} + +function textPrompt( + config: TextInputConfig, + { suppress }: { suppress: boolean } +): Promise { + const TextPrompt = createPrompt( + (cfg: TextInputConfig, done: (value: string) => void) => { + // The readline buffer (`rl.line`) is the source of truth for the + // typed text -- editing (backspace, ctrl+u, paste) is readline's + // job. It is mirrored into state on every keystroke because of an + // ordering subtlety verified against the installed @inquirer/core: + // when Enter arrives, readline has ALREADY consumed and cleared its + // buffer, so reading `rl.line` inside the Enter branch sees `''` + // (the typed text only ever visible on non-Enter keypresses). The + // mirror is written by the keypress immediately before Enter in + // stream order, so reading state there sees the completed line. + const [value, setValue] = useState(''); + const [status, setStatus] = useState<'idle' | 'done' | 'declined'>( + 'idle' + ); + const prefix = usePrefix({ + status: status === 'declined' ? 'done' : status, + }); + + useKeypress((key, rl) => { + if (isEnterKey(key)) { + // rl.line is already consumed at this point (see above); use the + // mirror, falling back defensively for a bare first-keystroke + // Enter. + const line = value || rl.line || ''; + if (line.length === 0 && cfg.default !== undefined) { + setValue(cfg.default); + setStatus('done'); + done(cfg.default); + } else if (line.length === 0) { + // Enter on an empty line with no default: a deliberate "no + // value". Same resolution as Escape (see EMPTY_INPUT), but + // rendered distinctly so the transcript shows the user + // actively declined rather than backed out. + setStatus('declined'); + done(''); + } else { + setValue(line); + setStatus('done'); + done(line); + } + } else if (key.name === 'escape') { + setStatus('declined'); + done(''); + } else { + // Everything else (typing, backspace, ctrl+u, paste...) is + // handled by the underlying readline instance itself. Mirroring + // `rl.line` into state drives the redraw AND keeps the + // completed line available to the Enter branch above. + setValue(rl.line); + } + }); + + const defaultHint = + cfg.default !== undefined && !suppress + ? ` ${c.muted(`(${cfg.default})`)}` + : ''; + + if (status === 'declined') { + return `${prefix} ${config.message} ${c.muted('(no input)')}`; + } + if (status === 'done') { + const shown = suppress ? '*'.repeat(value.length) : value; + return `${prefix} ${config.message} ${c.positive(shown)}`; + } + + // Idle view: readline's native echo of the buffer is muted (see + // create-prompt's output.mute()), so the typed characters would not + // appear at all unless the view renders them. A suppressed prompt + // shows one asterisk per typed character instead of the text. + const shown = suppress ? '*'.repeat(value.length) : value; + return `${prefix} ${config.message}${defaultHint}${shown ? ` ${c.command(shown)}` : ''}`; + } + ); + return TextPrompt(config, { clearPromptOnDone: true }); +} diff --git a/src/utils/interactive/PromptGateState.test.ts b/src/utils/interactive/PromptGateState.test.ts new file mode 100644 index 000000000..2b8b5d900 --- /dev/null +++ b/src/utils/interactive/PromptGateState.test.ts @@ -0,0 +1,44 @@ +import { afterEach, describe, expect, it } from '@jest/globals'; +import { + canPrompt, + resetNeverPromptForTests, + setNeverPrompt, +} from './PromptGateState'; + +// canPrompt() reads the real process streams. Jest's own stdio under this +// harness is NOT a TTY, so the "genuinely interactive" case can't be +// asserted true here -- that path is covered by the PTY-based manual +// verification of the full prompt layer (and by the e2e suite's inherent +// FRODO_TEST=1 coverage). Everything else is env/flag state, which IS +// fully assertable. +const SAVED_ENV_KEYS = ['FRODO_NO_PROMPT', 'FRODO_TEST'] as const; + +afterEach(() => { + for (const key of SAVED_ENV_KEYS) { + delete process.env[key]; + } + resetNeverPromptForTests(); +}); + +describe('PromptGateState.canPrompt', () => { + it('refuses under FRODO_TEST=1 even when everything else would allow', () => { + process.env.FRODO_TEST = '1'; + expect(canPrompt()).toBe(false); + }); + + it('refuses under FRODO_NO_PROMPT even when everything else would allow', () => { + process.env.FRODO_NO_PROMPT = '1'; + expect(canPrompt()).toBe(false); + }); + + it('refuses after setNeverPrompt() (the MCP stdio kill switch)', () => { + setNeverPrompt(); + expect(canPrompt()).toBe(false); + }); + + it('refuses on the test harness (non-TTY stdio)', () => { + // No FRODO_TEST, no FRODO_NO_PROMPT, no kill switch -- the only + // remaining reason to refuse is the non-TTY stdio this suite runs on. + expect(canPrompt()).toBe(false); + }); +}); diff --git a/src/utils/interactive/PromptGateState.ts b/src/utils/interactive/PromptGateState.ts new file mode 100644 index 000000000..f3fd9de9b --- /dev/null +++ b/src/utils/interactive/PromptGateState.ts @@ -0,0 +1,56 @@ +/** + * The prompt gate's pure state, split from PromptGate.ts so it stays + * importable from unit tests: PromptGate pulls in @inquirer/core and the + * frodo-lib-importing ColorTheme, and this CLI's Jest setup cannot load + * frodo-lib (see src/app.commandWiring.test.ts's header comment). + * + * Prompts fire only when EVERY one of these holds -- + * + * - `FRODO_NO_PROMPT` is unset (the explicit user opt-out, honored no + * matter how interactive the session looks; also settable per command + * via --no-prompt, see FrodoCommand); + * - `FRODO_TEST !== '1'` (the e2e harness tools/with-frodo-bin.mjs spawns + * with stdio: 'inherit', so local test runs DO have a TTY -- an + * isTTY-only gate would hang them); + * - both stdin and stdout are TTYs (piped/redirected sessions, the MCP + * server's stdio protocol channel, and CI all fail this naturally); + * - and setNeverPrompt() has not been called for this process (belt and + * braces for surfaces whose stdio is a protocol channel by + * construction, e.g. `frodo mcp server start --transport stdio`). + * + * Outside the gate, behavior is byte-identical to before the interactive + * layer existed: same errors, same exit codes. + */ + +/** Module-scoped hard kill switch for protocol-channel surfaces. */ +let neverPrompt = false; + +/** + * Permanently disables prompting for the remainder of this process. Used + * by surfaces whose stdin/stdout are a protocol channel by construction + * (e.g. the MCP server's stdio transport) as belt-and-braces alongside + * the natural isTTY check. + */ +export function setNeverPrompt(): void { + neverPrompt = true; +} + +/** + * Test-only escape hatch: clears the kill switch so a test can exercise + * the other gate conditions in any order. Not part of the product + * surface. + */ +export function resetNeverPromptForTests(): void { + neverPrompt = false; +} + +/** + * Whether prompts may fire in this process right now. Pure (no I/O) and + * cheap, so it can be consulted per prompt. + */ +export function canPrompt(): boolean { + if (neverPrompt) return false; + if (process.env.FRODO_NO_PROMPT) return false; + if (process.env.FRODO_TEST === '1') return false; + return !!process.stdin.isTTY && !!process.stdout.isTTY; +} From aa31f40a10db04309b0ec09163295efdbeb67d18 Mon Sep 17 00:00:00 2001 From: Volker Scheuber Date: Fri, 9 Oct 2026 23:26:00 -0600 Subject: [PATCH 2/5] fix: bundle inquirer deps in SEA bundle; treat prompt EOF/Ctrl-C as declined Two issues found after opening the PR (CI Test(26) caught the first): 1. tsdown auto-externalizes packages listed in package.json `dependencies` unless named in deps.alwaysBundle. Moving the inquirer packages from devDependencies to dependencies therefore left require("@inquirer/core") external in the SEA bundle, where no node_modules exists to resolve it (ERR_UNKNOWN_BUILTIN_MODULE at runtime; my earlier local SEA build predated the dependency move and masked it). Both tsdown configs now alwaysBundle the three inquirer packages. Verified: rebuilt SEA bundle has zero external inquirer requires and the full PTY prompt chain works against the fresh binary. 2. An @inquirer/core prompt rejects with ExitPromptError on stdin EOF (Ctrl-D) and Ctrl-C. For gap-fill prompts both mean "no value supplied" and now degrade to the declined result (''), so the caller falls through to its pre-existing missing-input error instead of dumping a raw stack trace. Also removes the failed baseline-worktree experiment's leftovers (the symlinked-node_modules worktree could not run the e2e harness at all - 623/632 suites failing is a broken environment, not a signal). Co-Authored-By: Claude Code --- src/utils/interactive/PromptGate.ts | 26 ++++++++++++++++++++++++-- tsdown.config.ts | 11 ++++++++++- tsdown.sea.config.mts | 10 +++++++++- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/utils/interactive/PromptGate.ts b/src/utils/interactive/PromptGate.ts index 6453b63a6..9270076f2 100644 --- a/src/utils/interactive/PromptGate.ts +++ b/src/utils/interactive/PromptGate.ts @@ -23,6 +23,7 @@ import { createPrompt, + ExitPromptError, isEnterKey, useKeypress, usePrefix, @@ -78,7 +79,7 @@ export async function promptInput( config: TextInputConfig ): Promise { if (!canPrompt()) return undefined; - return textPrompt(config, { suppress: false }); + return runTextPrompt(config, { suppress: false }); } /** @@ -89,7 +90,28 @@ export async function promptPassword( config: TextInputConfig ): Promise { if (!canPrompt()) return undefined; - return textPrompt(config, { suppress: true }); + return runTextPrompt(config, { suppress: true }); +} + +/** + * Terminal conditions outside the user's in-prompt choices: stdin EOF + * (Ctrl-D, e.g. the driver's shutdown write) and Ctrl-C both reject the + * prompt with @inquirer/core's ExitPromptError. For gap-fills both mean + * "no value supplied" and must degrade to the declined result (`''`) so + * the caller falls through to its pre-existing missing-input error -- + * never a raw stack trace. (Escape is handled in-prompt, so it never + * reaches this wrapper.) + */ +async function runTextPrompt( + config: TextInputConfig, + opts: { suppress: boolean } +): Promise { + try { + return await textPrompt(config, opts); + } catch (error) { + if (error instanceof ExitPromptError) return EMPTY_INPUT; + throw error; + } } function textPrompt( diff --git a/tsdown.config.ts b/tsdown.config.ts index 55533b123..95e31f5a3 100644 --- a/tsdown.config.ts +++ b/tsdown.config.ts @@ -40,5 +40,14 @@ export default defineConfig({ // Bundling all production deps into dist/ is the design (zero-dep // published package); onlyBundle:false is tsdown's "we know" switch that // silences the detected-dependencies hint and its long dep list. - deps: { neverBundle: devDeps, onlyBundle: false }, + // The inquirer prompts MUST be bundled explicitly (deps.alwaysBundle): they moved from + // devDependencies to dependencies with the interactive-prompt feature, + // and tsdown auto-externalizes packages listed in `dependencies` unless + // named here -- which surfaced as ERR_UNKNOWN_BUILTIN_MODULE at SEA + // runtime (there is no node_modules inside a SEA binary to resolve to). + deps: { + neverBundle: devDeps, + onlyBundle: false, + alwaysBundle: ['@inquirer/confirm', '@inquirer/core', '@inquirer/select'], + }, }); diff --git a/tsdown.sea.config.mts b/tsdown.sea.config.mts index df21c3bfa..fdb8ffdbf 100644 --- a/tsdown.sea.config.mts +++ b/tsdown.sea.config.mts @@ -16,7 +16,15 @@ export default defineConfig({ outputOptions: { codeSplitting: false }, // Self-contained by design (single SEA bundle); onlyBundle:false silences // tsdown's detected-dependencies hint and its long dep list. - deps: { onlyBundle: false }, + // The inquirer prompts MUST be bundled explicitly (deps.alwaysBundle): they moved from + // devDependencies to dependencies with the interactive-prompt feature, + // and tsdown auto-externalizes packages listed in `dependencies` unless + // named here -- which surfaced as ERR_UNKNOWN_BUILTIN_MODULE at SEA + // runtime (there is no node_modules inside a SEA binary to resolve to). + deps: { + onlyBundle: false, + alwaysBundle: ['@inquirer/confirm', '@inquirer/core', '@inquirer/select'], + }, define: { __CLI_BUILD_TIMESTAMP__: JSON.stringify(new Date().toISOString()), }, From 6968e05ecfa7233c3ec1d0c9508eab93e7de0ae2 Mon Sep 17 00:00:00 2001 From: Volker Scheuber Date: Sat, 10 Oct 2026 07:14:49 -0600 Subject: [PATCH 3/5] fix: keep inquirer packages in devDependencies, restoring zero-dep design Follow-up to the dependency move in b122fba55: in this repo, every bundled runtime package (commander, tinyrainbow, @inquirer/confirm, and now promptInput/promptPassword) deliberately lives in devDependencies -- that is what makes tsdown inline it and keeps the published package zero-dep (empty dependencies field, empty npm audit --omit=dev surface, no install weight). Declaring the inquirer packages as `dependencies` followed conventional npm metadata but inverted this repo's convention: tsdown auto-externalizes declared deps (needing deps.alwaysBundle to force inline, per aa31f40a1), consumers would install three packages whose code is also inlined, and npm audit would start scanning the inquirer tree again. Reverted: packages back to devDependencies, alwaysBundle dropped from both tsdown configs, placement convention documented in both configs so the next dep addition doesn't repeat the round trip. Verified: fresh dist + SEA bundles have zero external @inquirer requires (18 inlined regions), SEA binary runs, full PTY prompt chain re-verified against the fresh binary, npm audit --omit=dev = 0. Co-Authored-By: Claude Code --- package-lock.json | 24 +++++++++++++++++------- package.json | 8 +++----- tsdown.config.ts | 12 ++++++------ tsdown.sea.config.mts | 11 +++++------ 4 files changed, 31 insertions(+), 24 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3ab6f7109..9ddf5a806 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,17 +8,15 @@ "name": "@rockcarver/frodo-cli", "version": "5.0.0-5", "license": "MIT", - "dependencies": { - "@inquirer/confirm": "6.3.2", - "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3" - }, "bin": { "frodo": "dist/launch.cjs" }, "devDependencies": { "@eslint/js": "10.0.1", "@ianvs/prettier-plugin-sort-imports": "4.7.1", + "@inquirer/confirm": "6.3.2", + "@inquirer/core": "^12.0.1", + "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0", @@ -895,6 +893,7 @@ "version": "2.0.8", "resolved": "https://registry.npmjs.org/@inquirer/ansi/-/ansi-2.0.8.tgz", "integrity": "sha512-WpQM+Ti6Z40EFwwt+uL2p4UabT+W179zHp6HhLVOzfbwnVn05IPO/eXIZXGNqcT1jbQ15SujNLzQ39k4QPPxBQ==", + "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -904,6 +903,7 @@ "version": "6.3.2", "resolved": "https://registry.npmjs.org/@inquirer/confirm/-/confirm-6.3.2.tgz", "integrity": "sha512-Xvr/0HggjddPtGppuqVmxhTw+Hr8PvsZ/k0HmOEaAqQEt80OITNkFWnsdNmyT0/eM4Ab+iJLx2R8rctlEyfSVg==", + "dev": true, "license": "MIT", "dependencies": { "@inquirer/core": "^12.0.3", @@ -925,6 +925,7 @@ "version": "12.0.3", "resolved": "https://registry.npmjs.org/@inquirer/core/-/core-12.0.3.tgz", "integrity": "sha512-wsSy0sznmXwkty+2PzZwx00Cazc/E0r0B7mAzdGROz2Ct+DFZXaK7WDjGZvgjRldxH5ZhFVfF2lgkYrqgOw2KA==", + "dev": true, "license": "MIT", "dependencies": { "@inquirer/ansi": "^2.0.8", @@ -951,6 +952,7 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz", "integrity": "sha512-bzyZ1e88w9O1iNJbKnOlvYTrWPDl46O1bG0D3XInv+9tkPrxrN8jUUTiFlDkkmKWgn1M6CfIA13SuGqOa9Korw==", + "dev": true, "license": "ISC", "engines": { "node": ">=14" @@ -963,6 +965,7 @@ "version": "2.0.9", "resolved": "https://registry.npmjs.org/@inquirer/figures/-/figures-2.0.9.tgz", "integrity": "sha512-EAWgUTGQ/Umgga51dE3B2PUHbufuXarDfg86uVgoSgNHNNQnyFKcOrQLWVqYMghuSyHh8+2HUH0Js9cTC1WAdg==", + "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -972,6 +975,7 @@ "version": "5.2.5", "resolved": "https://registry.npmjs.org/@inquirer/select/-/select-5.2.5.tgz", "integrity": "sha512-9kc15hr8r/kI+3DO/xLog5nOzTz1jqsHXa6JBFzmQKhkoJ8Slda1I1L/uD8ZSZ9tF1yp79wwXe7mclvX1rqR2Q==", + "dev": true, "license": "MIT", "dependencies": { "@inquirer/ansi": "^2.0.8", @@ -995,6 +999,7 @@ "version": "4.1.1", "resolved": "https://registry.npmjs.org/@inquirer/type/-/type-4.1.1.tgz", "integrity": "sha512-yJoHYrMnxIsJZCY+0Vb66Dy3he3kL3e2wOBKhoSwWWAzZAY82emlxwgprCtp6yRixvNRNq9ztfRWQYPNr3Go7A==", + "dev": true, "license": "MIT", "engines": { "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" @@ -2629,7 +2634,7 @@ "version": "26.6.4", "resolved": "https://registry.npmjs.org/@types/node/-/node-26.6.4.tgz", "integrity": "sha512-ldVPDCzj7fsaGZrLB0NuHuTvJcsNasysBAqMolr/cgxrLd1xbqxIr3XJiPnHHJUCxj5sNF1vnRj9aWnrVh5Jcg==", - "devOptional": true, + "dev": true, "license": "MIT", "dependencies": { "undici-types": "~8.9.0" @@ -4166,6 +4171,7 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/cli-width/-/cli-width-4.1.0.tgz", "integrity": "sha512-ouuZd4/dm2Sw5Gmqy6bGyNNNe1qt9RpmxveLSO7KcgsTnU7RXfsw+/bukWGo1abgBiMAic068rclZsO4IWmmxQ==", + "dev": true, "license": "ISC", "engines": { "node": ">= 12" @@ -5085,12 +5091,14 @@ "version": "3.0.3", "resolved": "https://registry.npmjs.org/fast-string-truncated-width/-/fast-string-truncated-width-3.0.3.tgz", "integrity": "sha512-0jjjIEL6+0jag3l2XWWizO64/aZVtpiGE3t0Zgqxv0DPuxiMjvB3M24fCyhZUO4KomJQPj3LTSUnDP3GpdwC0g==", + "dev": true, "license": "MIT" }, "node_modules/fast-string-width": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/fast-string-width/-/fast-string-width-3.0.2.tgz", "integrity": "sha512-gX8LrtNEI5hq8DVUfRQMbr5lpaS4nMIWV+7XEbXk2b8kiQIizgnlr12B4dA3ZEx3308ze0O4Q1R+cHts8kyUJg==", + "dev": true, "license": "MIT", "dependencies": { "fast-string-truncated-width": "^3.0.2" @@ -5100,6 +5108,7 @@ "version": "0.2.2", "resolved": "https://registry.npmjs.org/fast-wrap-ansi/-/fast-wrap-ansi-0.2.2.tgz", "integrity": "sha512-7F2Fl+TjRSenLqlU3UjSH0iyqopqoZIu7eZVpEirP2g1GtWa2G/ecEmBdgz31+Mxr+ELclgg6sokpSFIQiZ02Q==", + "dev": true, "license": "MIT", "dependencies": { "fast-string-width": "^3.0.2" @@ -6829,6 +6838,7 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-3.0.0.tgz", "integrity": "sha512-dkEJPVvun4FryqBmZ5KhDo0K9iDXAwn08tMLDinNdRBNPcYEDiWYysLcc6k3mjTMlbP9KyylvRpd4wFtwrT9rw==", + "dev": true, "license": "ISC", "engines": { "node": "^20.17.0 || >=22.9.0" @@ -8541,7 +8551,7 @@ "version": "8.9.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.9.0.tgz", "integrity": "sha512-KTDyRTYX8sWmKXAikPHHSyc63CRPETMctyjKFupcC6OBLXT3xsN0e9aF7m+mIXutFWpUXuedtowG7iLOzp0kQg==", - "devOptional": true, + "dev": true, "license": "MIT" }, "node_modules/universalify": { diff --git a/package.json b/package.json index 8e3fd395b..6680324fe 100644 --- a/package.json +++ b/package.json @@ -124,6 +124,9 @@ "devDependencies": { "@eslint/js": "10.0.1", "@ianvs/prettier-plugin-sort-imports": "4.7.1", + "@inquirer/confirm": "6.3.2", + "@inquirer/core": "^12.0.1", + "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0", @@ -173,10 +176,5 @@ }, "overrides": { "argparse": "^2.0.1" - }, - "dependencies": { - "@inquirer/confirm": "6.3.2", - "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3" } } diff --git a/tsdown.config.ts b/tsdown.config.ts index 95e31f5a3..b3aa49268 100644 --- a/tsdown.config.ts +++ b/tsdown.config.ts @@ -40,14 +40,14 @@ export default defineConfig({ // Bundling all production deps into dist/ is the design (zero-dep // published package); onlyBundle:false is tsdown's "we know" switch that // silences the detected-dependencies hint and its long dep list. - // The inquirer prompts MUST be bundled explicitly (deps.alwaysBundle): they moved from - // devDependencies to dependencies with the interactive-prompt feature, - // and tsdown auto-externalizes packages listed in `dependencies` unless - // named here -- which surfaced as ERR_UNKNOWN_BUILTIN_MODULE at SEA - // runtime (there is no node_modules inside a SEA binary to resolve to). + // NOTE on placement: every bundled runtime package (including the + // inquirer prompts) lives in devDependencies -- tsdown inlines those + // automatically, while anything listed under `dependencies` is + // auto-externalized unless forced via deps.alwaysBundle, which would + // break the SEA binary (no node_modules to resolve against) and reopen + // the npm audit surface the zero-dep design deliberately keeps empty. deps: { neverBundle: devDeps, onlyBundle: false, - alwaysBundle: ['@inquirer/confirm', '@inquirer/core', '@inquirer/select'], }, }); diff --git a/tsdown.sea.config.mts b/tsdown.sea.config.mts index fdb8ffdbf..4c56e8d86 100644 --- a/tsdown.sea.config.mts +++ b/tsdown.sea.config.mts @@ -16,14 +16,13 @@ export default defineConfig({ outputOptions: { codeSplitting: false }, // Self-contained by design (single SEA bundle); onlyBundle:false silences // tsdown's detected-dependencies hint and its long dep list. - // The inquirer prompts MUST be bundled explicitly (deps.alwaysBundle): they moved from - // devDependencies to dependencies with the interactive-prompt feature, - // and tsdown auto-externalizes packages listed in `dependencies` unless - // named here -- which surfaced as ERR_UNKNOWN_BUILTIN_MODULE at SEA - // runtime (there is no node_modules inside a SEA binary to resolve to). + // NOTE on placement: every bundled runtime package (including the + // inquirer prompts) lives in devDependencies -- tsdown inlines those + // automatically, while anything listed under `dependencies` is + // auto-externalized unless forced via deps.alwaysBundle, which would + // break this binary outright (no node_modules to resolve against). deps: { onlyBundle: false, - alwaysBundle: ['@inquirer/confirm', '@inquirer/core', '@inquirer/select'], }, define: { __CLI_BUILD_TIMESTAMP__: JSON.stringify(new Date().toISOString()), From 2619b338ff7675e995c97ad724bcbb1707c38a3d Mon Sep 17 00:00:00 2001 From: Volker Scheuber Date: Sat, 10 Oct 2026 07:18:22 -0600 Subject: [PATCH 4/5] docs: document the zero-dep dependency-placement rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainers and contributors need to know that in this repo "imported at runtime" does NOT mean "declare as dependencies": the published package is zero-dep by design, bundled runtime packages live in devDependencies (tsdown inlines those), and anything declared under dependencies is externalized - which breaks the SEA binary (no node_modules to resolve against) and reopens the npm audit surface we keep empty. Hit for real with the @inquirer/* packages in this PR. - BUILD-ENV.md: new §2.2.1 (rule, tsup->tsdown option mapping, SEA failure mode, stale-binary gotcha, verification one-liner) + §11 history row. - CONTRIBUTE.md: callout in the build section pointing contributors at §2.2.1 before they add a package. Co-Authored-By: Claude Code --- docs/BUILD-ENV.md | 47 ++++++++++++++++++++++++++++++++++++++++++++++ docs/CONTRIBUTE.md | 11 +++++++++++ 2 files changed, 58 insertions(+) diff --git a/docs/BUILD-ENV.md b/docs/BUILD-ENV.md index 68a29264f..513ead4cb 100644 --- a/docs/BUILD-ENV.md +++ b/docs/BUILD-ENV.md @@ -74,6 +74,52 @@ loaded, then threw `SyntaxError: Unexpected token ','`. The workaround - `deps.neverBundle` lists every devDependency. tsdown (like tsup before it) otherwise bundles devDependencies — which is why our devDependencies are effectively the runtime dependency list of the binary. + +#### 2.2.1 The dependency-placement rule (read before adding any package) + +The published package is **zero-dep by design**: package.json's +`dependencies` field stays empty, and everything the binary needs at +runtime lives in **`devDependencies`** and is inlined into the bundle. +This is what keeps `npm install` weight minimal, keeps +`npm audit --omit=dev` empty (the audit surface we drove to zero in the +zero-audit push and want to keep empty), and is what makes the SEA binary +self-contained. + +Bundling is driven entirely by which bucket a package sits in — not by +any explicit bundle list: + +| Bucket | tsdown behavior (tsup had the same default) | +| ----------------- | ------------------------------------------------------------------------------------ | +| `devDependencies` | **inlined** into `dist/*.cjs` / SEA bundle | +| `dependencies` | **externalized** to a bare `require()` — unless forced back with `deps.alwaysBundle` | + +So the npm-conventional instinct — "imported at runtime ⇒ declare as +`dependencies`" — is **wrong in this repo**. It happened with the +interactive-prompt feature (PR #772): moving the `@inquirer/*` packages +to `dependencies` made tsdown externalize them, and the SEA binary (a +single embedded file with no `node_modules` next to it) failed at runtime +with `ERR_UNKNOWN_BUILTIN_MODULE` — caught by CI's SEA e2e leg only, +because a stale locally built binary still contained the old inlined +code. Declared deps would also have reopened the npm audit surface and +made consumers install three packages whose code is inlined anyway. + +**Rule: a package imported by shipped code goes in `devDependencies`, +and nothing ever goes in `dependencies`.** If a package genuinely cannot +be bundled, that is a design problem to raise — not something to paper +over with `alwaysBundle`. + +Verification one-liner after any dependency-graph change (both counts +must be 0): + +```console +grep -c 'require("@' dist/app.cjs dist-sea/app.cjs +``` + +(Non-`node:` external requires in a fresh bundle mean something got +externalized; the SEA build is the one that breaks at runtime. Build the +binary fresh first — `npm run build:binary` — a stale `dist-sea/` masks +the problem exactly the way it did in #772.) + - JSON imports (`src/ops/templates/*.json`) are inlined at build time; no runtime file reads, nothing to ship as assets. - `define: { __CLI_BUILD_TIMESTAMP__ }` stamps the build time that `-v` prints. @@ -361,4 +407,5 @@ pkg→SEA migration (it builds `dist/` from source). | 2026-10-05 | npm dev-dependency majors (Batch 2, merged sequentially with rebase discipline): @types/node 26, chokidar 5, uuid 14, commander 15 (SEA-validated on main before merge), properties-reader 3 (`propertiesReader({sourceFile})` object form; `.each` callback value widened to `Value` → env-file values coerced with `String()`) | #740, #725, #720, #739, #738 | | 2026-10-05 | frodo-lib pinned to exact versions (4.11.1 → 4.12.0-1) after each lib release; lib pin is deliberate (exact, not caret) so cli CI always validates against the version it will ship with | #754 | | 2026-10-05 | Deep-import type migration: all 110 `@rockcarver/frodo-lib/types/*` statements (62 files) migrated to root-entry imports with inline `type` qualifiers — removes the node10-moduleResolution dependency (`./types/*` subpath is deprecated in TS 6, removed in TS 7) and gives IDEs the standard single-entry type surface. Requires frodo-lib ≥ 4.12 (#687 root type exports) | #755 | +| 2026-10-10 | Dependency-placement rule documented (§2.2.1): all bundled runtime packages live in `devDependencies` (tsdown inlines those; `dependencies` entries are externalized and break the SEA binary + reopen the npm audit surface). The `@inquirer/*` move to `dependencies` in #772 was reverted on this rule — `dependencies` stays empty by design. tsdown's `deps.alwaysBundle` exists as the escape hatch but should never be needed here. tsup equivalent: `noExternal`/`external` | #772 | | planned | Polly→nock (library repo) — DEFERRED 2026-10-04 (deep record-harness coupling; revisit on Node 28 or real breakage) | — | diff --git a/docs/CONTRIBUTE.md b/docs/CONTRIBUTE.md index b0c3328d2..8bc522b3c 100644 --- a/docs/CONTRIBUTE.md +++ b/docs/CONTRIBUTE.md @@ -38,6 +38,17 @@ For how the build toolchain works — what each tool does and why, bundler configuration details, binary packaging and signing — see [BUILD-ENV.md](BUILD-ENV.md). +> **Adding a package? Read this first.** This CLI ships as a +> zero-dependency package: package.json's `dependencies` field stays +> empty on purpose, and every package imported by shipped code — +> including runtime prompt/library code — goes in **`devDependencies`**. +> The bundler (tsdown; tsup had the same default) inlines +> devDependencies into the bundle and externalizes anything listed under +> `dependencies` — which breaks the SEA binary at runtime (no +> `node_modules` inside the binary) and reopens the npm audit surface we +> deliberately keep empty. Details and the verification one-liner: +> [BUILD-ENV.md §2.2.1](BUILD-ENV.md). + #### Full build (npm bundle + platform binary) The following command builds the CLI and creates a native SEA binary for From b017895dd5d9d11e6d4c6a6d1b1df8a7ab415847 Mon Sep 17 00:00:00 2001 From: Volker Scheuber Date: Sat, 10 Oct 2026 08:32:18 -0600 Subject: [PATCH 5/5] chore: drop unused @inquirer/select dependency Pre-existing since the EscapableSelectPrompt work: installed but never imported (our hand-built select is built on @inquirer/core directly). The comment references in EscapableSelectPrompt.ts are provenance notes about that package's design, not imports. Dropping it removes an unneeded install/audit surface entry; if the Phase 1c search extension ever wants its source as reference, it remains readable upstream. Co-Authored-By: Claude Code --- package-lock.json | 25 ------------------------- package.json | 1 - 2 files changed, 26 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9ddf5a806..c137693c8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,6 @@ "@ianvs/prettier-plugin-sort-imports": "4.7.1", "@inquirer/confirm": "6.3.2", "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0", @@ -971,30 +970,6 @@ "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" } }, - "node_modules/@inquirer/select": { - "version": "5.2.5", - "resolved": "https://registry.npmjs.org/@inquirer/select/-/select-5.2.5.tgz", - "integrity": "sha512-9kc15hr8r/kI+3DO/xLog5nOzTz1jqsHXa6JBFzmQKhkoJ8Slda1I1L/uD8ZSZ9tF1yp79wwXe7mclvX1rqR2Q==", - "dev": true, - "license": "MIT", - "dependencies": { - "@inquirer/ansi": "^2.0.8", - "@inquirer/core": "^12.0.3", - "@inquirer/figures": "^2.0.9", - "@inquirer/type": "4.1.1" - }, - "engines": { - "node": ">=23.5.0 || ^22.13.0 || ^20.17.0" - }, - "peerDependencies": { - "@types/node": ">=18" - }, - "peerDependenciesMeta": { - "@types/node": { - "optional": true - } - } - }, "node_modules/@inquirer/type": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/@inquirer/type/-/type-4.1.1.tgz", diff --git a/package.json b/package.json index 6680324fe..99997fcd3 100644 --- a/package.json +++ b/package.json @@ -126,7 +126,6 @@ "@ianvs/prettier-plugin-sort-imports": "4.7.1", "@inquirer/confirm": "6.3.2", "@inquirer/core": "^12.0.1", - "@inquirer/select": "^5.2.3", "@modelcontextprotocol/client": "^2.0.0", "@modelcontextprotocol/node": "^2.0.0", "@modelcontextprotocol/server": "^2.0.0",