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 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", 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..9270076f2 --- /dev/null +++ b/src/utils/interactive/PromptGate.ts @@ -0,0 +1,196 @@ +/** + * 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, + ExitPromptError, + 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 runTextPrompt(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 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( + 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; +} diff --git a/tsdown.config.ts b/tsdown.config.ts index 55533b123..b3aa49268 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 }, + // 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, + }, }); diff --git a/tsdown.sea.config.mts b/tsdown.sea.config.mts index df21c3bfa..4c56e8d86 100644 --- a/tsdown.sea.config.mts +++ b/tsdown.sea.config.mts @@ -16,7 +16,14 @@ 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 }, + // 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, + }, define: { __CLI_BUILD_TIMESTAMP__: JSON.stringify(new Date().toISOString()), },