From 4e7675ecf39bed93021fe932ca2e661a50127550 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Thu, 30 Jul 2026 12:53:37 +0200 Subject: [PATCH] Show required non-interactive flags in CLI help --- .changeset/clear-agents-plan.md | 5 + .../src/public/node/base-command.test.ts | 129 +++++++++++++++++- .../cli-kit/src/public/node/base-command.ts | 63 +++++++-- packages/cli-kit/src/public/node/cli.test.ts | 21 ++- packages/cli-kit/src/public/node/cli.ts | 19 +++ .../rules/command-conventional-flag-env.js | 16 +-- .../rules/command-flags-with-env.js | 14 +- .../rules/command-reserved-flags.js | 16 +-- .../eslint-plugin-cli/rules/flag-options.js | 10 ++ .../rules/flag-options.test.js | 28 ++++ packages/eslint-plugin-cli/vite.config.ts | 11 ++ vite.config.ts | 1 + 12 files changed, 297 insertions(+), 36 deletions(-) create mode 100644 .changeset/clear-agents-plan.md create mode 100644 packages/eslint-plugin-cli/rules/flag-options.js create mode 100644 packages/eslint-plugin-cli/rules/flag-options.test.js create mode 100644 packages/eslint-plugin-cli/vite.config.ts diff --git a/.changeset/clear-agents-plan.md b/.changeset/clear-agents-plan.md new file mode 100644 index 00000000000..71cdc85d6d7 --- /dev/null +++ b/.changeset/clear-agents-plan.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Show and validate all flags required for non-interactive commands before execution. diff --git a/packages/cli-kit/src/public/node/base-command.test.ts b/packages/cli-kit/src/public/node/base-command.test.ts index 31203ef5d24..582b5986327 100644 --- a/packages/cli-kit/src/public/node/base-command.test.ts +++ b/packages/cli-kit/src/public/node/base-command.test.ts @@ -1,7 +1,7 @@ import Command from './base-command.js' import {Environments} from './environments.js' import {encodeToml as encodeTOML} from './toml/codec.js' -import {globalFlags} from './cli.js' +import {globalFlags, requiredIfNonInteractive} from './cli.js' import {inTemporaryDirectory, mkdir, writeFile} from './fs.js' import {joinPath, resolvePath, cwd} from './path.js' import {mockAndCaptureOutput} from './testing/output.js' @@ -79,6 +79,50 @@ class MockCommandWithRequiredFlagInNonTTY extends MockCommand { } } +class MockCommandWithDeclarativeNonTTYRequirements extends MockCommand { + /* eslint-disable @shopify/cli/command-flags-with-env */ + static flags = { + ...MockCommand.flags, + 'first-required': requiredIfNonInteractive( + Flags.string({env: 'SHOPIFY_FLAG_TEST_FIRST_REQUIRED', description: 'The first required flag.'}), + ), + 'second-required': requiredIfNonInteractive( + Flags.string({env: 'SHOPIFY_FLAG_TEST_SECOND_REQUIRED', description: 'The second required flag.'}), + ), + 'alternative-one': Flags.string({}), + 'alternative-two': Flags.string({}), + conditional: Flags.string({}), + 'require-conditional': Flags.boolean({}), + } + /* eslint-enable @shopify/cli/command-flags-with-env */ + + static nonTTYFlagRequirements() { + return [ + {flags: ['alternative-one', 'alternative-two']}, + {flags: ['conditional'], when: (flags: Record) => Boolean(flags['require-conditional'])}, + ] + } + + async run(): Promise { + const {flags} = await this.parse(MockCommandWithDeclarativeNonTTYRequirements) + testResult = flags + } +} + +class MockCommandWithDefaultFalseNonTTYRequirement extends MockCommand { + /* eslint-disable @shopify/cli/command-flags-with-env */ + static flags = { + ...MockCommand.flags, + force: requiredIfNonInteractive(Flags.boolean({default: false})), + } + /* eslint-enable @shopify/cli/command-flags-with-env */ + + async run(): Promise { + const {flags} = await this.parse(MockCommandWithDefaultFalseNonTTYRequirement) + testResult = flags + } +} + class MockCommandWithoutEnvironmentFlag extends Command { /* eslint-disable @shopify/cli/command-flags-with-env */ static flags = { @@ -446,9 +490,90 @@ describe('applying environments', async () => { await MockCommandWithRequiredFlagInNonTTY.run(['--path', tmpDir]) // Then - expect(unstyled(testError!.message)).toMatch('Flag not specified:\n\nnonTTYRequiredFlag') + expect(unstyled(testError!.message)).toMatch('Flag not specified:\n\n--nonTTYRequiredFlag') }) + runTestInTmpDir('reports all missing declarative non-TTY requirements', async (tmpDir: string) => { + // Given + vi.stubEnv('CI', 'true') + + // When + await MockCommandWithDeclarativeNonTTYRequirements.run(['--path', tmpDir]) + + // Then + expect(unstyled(testError!.message)).toContain(`Flags not specified: + +--first-required +--second-required +--alternative-one or --alternative-two`) + }) + + runTestInTmpDir('accepts annotated flags supplied through environment variables', async (tmpDir: string) => { + // Given + vi.stubEnv('CI', 'true') + vi.stubEnv('SHOPIFY_FLAG_TEST_FIRST_REQUIRED', 'first') + vi.stubEnv('SHOPIFY_FLAG_TEST_SECOND_REQUIRED', 'second') + + // When + await MockCommandWithDeclarativeNonTTYRequirements.run(['--path', tmpDir, '--alternative-two', 'alternative']) + + // Then + expect(testError).toBeUndefined() + expect(testResult).toMatchObject({ + 'first-required': 'first', + 'second-required': 'second', + 'alternative-two': 'alternative', + }) + }) + + runTestInTmpDir('does not treat a false boolean default as a supplied flag', async (tmpDir: string) => { + // Given + vi.stubEnv('CI', 'true') + + // When + await MockCommandWithDefaultFalseNonTTYRequirement.run(['--path', tmpDir]) + + // Then + expect(unstyled(testError!.message)).toContain('--force') + + // When + testError = undefined + await MockCommandWithDefaultFalseNonTTYRequirement.run(['--path', tmpDir, '--force']) + + // Then + expect(testError).toBeUndefined() + }) + + runTestInTmpDir( + 'applies conditional non-TTY requirements only when their condition matches', + async (tmpDir: string) => { + // Given + vi.stubEnv('CI', 'true') + const requiredFlags = [ + '--path', + tmpDir, + '--first-required', + 'first', + '--second-required', + 'second', + '--alternative-one', + 'alternative', + ] + + // When + await MockCommandWithDeclarativeNonTTYRequirements.run(requiredFlags) + + // Then + expect(testError).toBeUndefined() + + // When + await MockCommandWithDeclarativeNonTTYRequirements.run([...requiredFlags, '--require-conditional']) + + // Then + expect(unstyled(testError!.message)).toContain('--conditional') + }, + ) + runTestInTmpDir('reports environment settings that do not match defaults', async (tmpDir: string) => { // Given const outputMock = mockAndCaptureOutput() diff --git a/packages/cli-kit/src/public/node/base-command.ts b/packages/cli-kit/src/public/node/base-command.ts index 1da872612e0..c81438e52ff 100644 --- a/packages/cli-kit/src/public/node/base-command.ts +++ b/packages/cli-kit/src/public/node/base-command.ts @@ -17,6 +17,13 @@ export type ArgOutput = OutputArgs // eslint-disable-next-line @typescript-eslint/no-explicit-any export type FlagOutput = OutputFlags +export interface NonTTYFlagRequirement { + /** At least one of these flags must be present when the requirement applies. */ + flags: string[] + /** Determines whether the requirement applies to the parsed flags. */ + when?: (flags: FlagOutput) => boolean +} + interface EnvironmentFlags { 'auth-alias'?: string environment?: string[] @@ -26,6 +33,10 @@ interface EnvironmentFlags { abstract class BaseCommand extends Command { static baseFlags: FlagInput<{}> = {} + public static nonTTYFlagRequirements(_flags: FlagOutput): NonTTYFlagRequirement[] { + return [] + } + // Replace markdown links to plain text like: "link label" (url) public static descriptionWithoutMarkdown(): string | undefined { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -108,6 +119,7 @@ abstract class BaseCommand extends Command { result = await this.resultWithEnvironment(result, options, argv) await setCurrentSessionAlias(result.flags['auth-alias']) await addFromParsedFlags(result.flags) + this.failMissingNonTTYFlagRequirements(result.flags, this.applicableNonTTYFlagRequirements(result.flags)) return {...result, ...{argv: result.argv as string[]}} } @@ -117,20 +129,51 @@ abstract class BaseCommand extends Command { } protected failMissingNonTTYFlags(flags: FlagOutput, requiredFlags: string[]): void { + this.failMissingNonTTYFlagRequirements( + flags, + requiredFlags.map((flag) => ({flags: [flag]})), + ) + } + + private failMissingNonTTYFlagRequirements(flags: FlagOutput, requirements: NonTTYFlagRequirement[]): void { if (terminalSupportsPrompting()) return - requiredFlags.forEach((name: string) => { - if (!(name in flags)) { - throw new AbortError( - outputContent`Flag not specified: + const missingRequirements = requirements.filter((requirement) => + requirement.flags.every((flag) => flags[flag] === undefined || flags[flag] === false), + ) + if (missingRequirements.length === 0) return -${outputToken.cyan(name)} + const heading = missingRequirements.length === 1 ? 'Flag not specified' : 'Flags not specified' + const formattedRequirements = missingRequirements + .map((requirement) => requirement.flags.map((flag) => `--${flag}`).join(' or ')) + .join('\n') + const explanation = missingRequirements.length === 1 ? 'This flag is required' : 'These flags are required' -This flag is required in non-interactive terminal environments, such as a CI environment, or when piping input from another process.`, - 'To resolve this, specify the option in the command, or run the command in an interactive environment such as your local terminal.', - ) - } - }) + throw new AbortError( + outputContent`${heading}: + +${outputToken.cyan(formattedRequirements)} + +${explanation} in non-interactive terminal environments, such as a CI environment, or when piping input from another process.`, + 'To resolve this, specify the options in the command, or run the command in an interactive environment such as your local terminal.', + ) + } + + private applicableNonTTYFlagRequirements(flags: FlagOutput): NonTTYFlagRequirement[] { + const command = this.constructor as unknown as { + flags?: FlagInput + baseFlags?: FlagInput + nonTTYFlagRequirements?: (flags: FlagOutput) => NonTTYFlagRequirement[] + } + const allFlags = {...command.baseFlags, ...command.flags} + const requirementsFromFlags = Object.entries(allFlags) + .filter(([, flag]) => Boolean((flag as {requiredIfNonInteractive?: boolean}).requiredIfNonInteractive)) + .map(([name]) => ({flags: [name]})) + const commandRequirements = (command.nonTTYFlagRequirements?.(flags) ?? []).filter( + (requirement) => requirement.when?.(flags) ?? true, + ) + + return [...requirementsFromFlags, ...commandRequirements] } private async resultWithEnvironment< diff --git a/packages/cli-kit/src/public/node/cli.test.ts b/packages/cli-kit/src/public/node/cli.test.ts index 6082c8dcadc..98478050f4d 100644 --- a/packages/cli-kit/src/public/node/cli.test.ts +++ b/packages/cli-kit/src/public/node/cli.test.ts @@ -1,8 +1,9 @@ -import {clearCache, runCLI, runCreateCLI, portFlag} from './cli.js' +import {clearCache, runCLI, runCreateCLI, portFlag, requiredIfNonInteractive} from './cli.js' import {findUpAndReadPackageJson} from './node-package-manager.js' import {mockAndCaptureOutput} from './testing/output.js' import * as confStore from '../../private/node/conf-store.js' import {describe, expect, test, vi} from 'vitest' +import {Flags} from '@oclif/core' vi.mock('./node-package-manager.js') @@ -148,3 +149,21 @@ describe('portFlag', () => { }, ) }) + +describe('requiredIfNonInteractive', () => { + test.each([ + ['The app template', 'The app template. Required if non interactive.'], + ['The app template.', 'The app template. Required if non interactive.'], + ])('annotates a copy without duplicating punctuation in %s', (description, expectedDescription) => { + const flag = Flags.string({description}) + + const got = requiredIfNonInteractive(flag) + + expect(got).toMatchObject({ + description: expectedDescription, + requiredIfNonInteractive: true, + }) + expect(flag.description).toBe(description) + expect(flag).not.toHaveProperty('requiredIfNonInteractive') + }) +}) diff --git a/packages/cli-kit/src/public/node/cli.ts b/packages/cli-kit/src/public/node/cli.ts index 14b2add6c1f..df56ccb99f0 100644 --- a/packages/cli-kit/src/public/node/cli.ts +++ b/packages/cli-kit/src/public/node/cli.ts @@ -173,6 +173,25 @@ export const portFlag = (options: {description?: string; env?: string; hidden?: return Flags.integer({min: 1, max: 65535, ...options, description}) } +/** + * Marks a flag as required when the CLI cannot prompt for a value. + * + * The flag remains optional in interactive terminals. In non-interactive environments, + * `BaseCommand` validates the flag automatically and the requirement is shown in `--help`. + * Use `BaseCommand.nonTTYFlagRequirements` for conditional or alternative requirements. + * + * @param flag - An oclif flag definition. + * @returns A new flag definition annotated for non-interactive validation and help output. + */ +export function requiredIfNonInteractive(flag: TFlag): TFlag { + const existingDescription = flag.description?.trimEnd() + const punctuatedDescription = + existingDescription && !existingDescription.endsWith('.') ? `${existingDescription}.` : existingDescription + const description = [punctuatedDescription, 'Required if non interactive.'].filter(Boolean).join(' ') + + return {...flag, description, requiredIfNonInteractive: true} +} + /** * Clear the CLI cache, used to store some API responses and handle notifications status */ diff --git a/packages/eslint-plugin-cli/rules/command-conventional-flag-env.js b/packages/eslint-plugin-cli/rules/command-conventional-flag-env.js index fa5ed2b984e..93c318deeeb 100644 --- a/packages/eslint-plugin-cli/rules/command-conventional-flag-env.js +++ b/packages/eslint-plugin-cli/rules/command-conventional-flag-env.js @@ -1,4 +1,6 @@ // https://eslint.org/docs/developer-guide/working-with-rules +const {findFlagOptions} = require('./flag-options') + const VALID_FLAGS = ['SHOPIFY_FLAG_'] module.exports = { @@ -14,16 +16,14 @@ module.exports = { PropertyDefinition(node) { if (node.key.name === 'flags') { node.value.properties.forEach((flag) => { - const arguments = flag.value?.arguments ?? [] - const argument = arguments[0] - if (!argument) { - return - } - const envProperty = argument.properties.find((property) => property.key.name === 'env')?.value?.value + const options = findFlagOptions(flag.value) + if (!options) return + + const envProperty = options.properties.find((property) => property.key?.name === 'env')?.value?.value if (envProperty) { - if (!VALID_FLAGS.some((flag) => envProperty.startsWith(flag))) { + if (!VALID_FLAGS.some((validPrefix) => envProperty.startsWith(validPrefix))) { context.report( - argument, + options, `Flags' environment variable must start with ${new Intl.ListFormat('en', { style: 'long', type: 'disjunction', diff --git a/packages/eslint-plugin-cli/rules/command-flags-with-env.js b/packages/eslint-plugin-cli/rules/command-flags-with-env.js index fbbcf0b73e7..ee9d11582d2 100644 --- a/packages/eslint-plugin-cli/rules/command-flags-with-env.js +++ b/packages/eslint-plugin-cli/rules/command-flags-with-env.js @@ -1,4 +1,6 @@ // https://eslint.org/docs/developer-guide/working-with-rules +const {findFlagOptions} = require('./flag-options') + module.exports = { meta: { type: 'problem', @@ -12,15 +14,13 @@ module.exports = { PropertyDefinition(node) { if (node.key.name === 'flags') { node.value.properties.forEach((flag) => { - const arguments = flag.value?.arguments ?? [] - const argument = arguments[0] - if (!argument) { - return - } - const properties = argument.properties.map((property) => property.key.name) + const options = findFlagOptions(flag.value) + if (!options) return + + const properties = options.properties.map((property) => property.key?.name) if (!properties.includes('env')) { context.report( - argument, + options, 'Flags must specify the environment variable that represents the flag through the env property', ) } diff --git a/packages/eslint-plugin-cli/rules/command-reserved-flags.js b/packages/eslint-plugin-cli/rules/command-reserved-flags.js index 8b07a17c94b..7dffb0f6b81 100644 --- a/packages/eslint-plugin-cli/rules/command-reserved-flags.js +++ b/packages/eslint-plugin-cli/rules/command-reserved-flags.js @@ -1,4 +1,6 @@ // https://eslint.org/docs/developer-guide/working-with-rules +const {findFlagOptions} = require('./flag-options') + module.exports = { meta: { type: 'problem', @@ -28,19 +30,17 @@ module.exports = { if (!flagName) { return } - if (!reservedFlags.hasOwnProperty(flagName)) { + if (!Object.prototype.hasOwnProperty.call(reservedFlags, flagName)) { return } - const arguments = flag.value?.arguments ?? [] - const argument = arguments[0] - if (!argument) { - return - } - const envProperty = argument.properties.find((property) => property.key.name === 'env')?.value?.value + const options = findFlagOptions(flag.value) + if (!options) return + + const envProperty = options.properties.find((property) => property.key?.name === 'env')?.value?.value if (envProperty) { if (envProperty !== reservedFlags[flagName]) { context.report( - argument, + options, `${flagName} is a reserved flags and its environment variable must be ${reservedFlags[flagName]}`, ) } diff --git a/packages/eslint-plugin-cli/rules/flag-options.js b/packages/eslint-plugin-cli/rules/flag-options.js new file mode 100644 index 00000000000..ad2247c31f2 --- /dev/null +++ b/packages/eslint-plugin-cli/rules/flag-options.js @@ -0,0 +1,10 @@ +function findFlagOptions(expression) { + if (expression?.type !== 'CallExpression') return undefined + + const firstArgument = expression.arguments[0] + if (firstArgument?.type === 'ObjectExpression') return firstArgument + + return findFlagOptions(firstArgument) +} + +module.exports = {findFlagOptions} diff --git a/packages/eslint-plugin-cli/rules/flag-options.test.js b/packages/eslint-plugin-cli/rules/flag-options.test.js new file mode 100644 index 00000000000..5d1fd253f19 --- /dev/null +++ b/packages/eslint-plugin-cli/rules/flag-options.test.js @@ -0,0 +1,28 @@ +import flagOptions from './flag-options' + +const {findFlagOptions} = flagOptions + +describe('findFlagOptions', () => { + test('returns options passed directly to a flag factory', () => { + const options = {type: 'ObjectExpression'} + const flagFactory = {type: 'CallExpression', arguments: [options]} + + expect(findFlagOptions(flagFactory)).toBe(options) + }) + + test('recursively finds options inside nested flag wrappers', () => { + const options = {type: 'ObjectExpression'} + const flagFactory = {type: 'CallExpression', arguments: [options]} + const innerWrapper = {type: 'CallExpression', arguments: [flagFactory]} + const outerWrapper = {type: 'CallExpression', arguments: [innerWrapper]} + + expect(findFlagOptions(outerWrapper)).toBe(options) + }) + + test.each([undefined, {type: 'Identifier'}, {type: 'CallExpression', arguments: [{type: 'Identifier'}]}])( + 'returns undefined when options cannot be found', + (expression) => { + expect(findFlagOptions(expression)).toBeUndefined() + }, + ) +}) diff --git a/packages/eslint-plugin-cli/vite.config.ts b/packages/eslint-plugin-cli/vite.config.ts new file mode 100644 index 00000000000..f374ad58afe --- /dev/null +++ b/packages/eslint-plugin-cli/vite.config.ts @@ -0,0 +1,11 @@ +import baseConfig from '../../configurations/vite.config' + +const config = baseConfig(__dirname) + +export default { + ...config, + test: { + ...config.test, + globals: true, + }, +} diff --git a/vite.config.ts b/vite.config.ts index 95a7828e0eb..14b1bd99528 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -12,6 +12,7 @@ export default defineConfig({ 'packages/app/vite.config.ts', 'packages/cli/vite.config.ts', 'packages/cli-kit/vite.config.ts', + 'packages/eslint-plugin-cli/vite.config.ts', 'packages/organizations/vite.config.ts', 'packages/plugin-cloudflare/vite.config.ts', 'packages/plugin-did-you-mean/vite.config.ts',