From 5fd6ce6955642d41dcea6b97cff3708cc04880f4 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 10:56:49 -0700 Subject: [PATCH 1/3] Ask which app configuration to scan when none is selected --- .../services/app-security-selection.test.ts | 98 ++++++++++++++++++- .../cli/services/app-security-selection.ts | 46 ++++++++- .../src/cli/services/security-check.test.ts | 21 ++++ .../app/src/cli/services/security-check.ts | 6 +- 4 files changed, 165 insertions(+), 6 deletions(-) diff --git a/packages/app/src/cli/services/app-security-selection.test.ts b/packages/app/src/cli/services/app-security-selection.test.ts index 487569e1550..7c048867278 100644 --- a/packages/app/src/cli/services/app-security-selection.test.ts +++ b/packages/app/src/cli/services/app-security-selection.test.ts @@ -11,7 +11,7 @@ import { type AppSecuritySelectionDependencies, } from './app-security-selection.js' import {validAppConfiguration} from './app-security-selection.test-data.js' -import {getCachedAppInfo} from './local-storage.js' +import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {appCreationDefaults} from './app/config/link.js' import {fetchOrCreateOrganizationApp} from './context.js' import {AbortError} from '@shopify/cli-kit/node/error' @@ -25,6 +25,7 @@ import type {OrganizationApp} from '../models/organization.js' vi.mock('./local-storage.js', async (importOriginal) => ({ ...(await importOriginal()), getCachedAppInfo: vi.fn(), + setCachedAppInfo: vi.fn(), })) vi.mock('./context.js', async (importOriginal) => ({ ...(await importOriginal()), @@ -51,6 +52,7 @@ function promptDependencies(overrides: Partial return { confirmScanWithoutAppConfig: vi.fn(async (_directory: string) => true), pickClientId: vi.fn(async (_appDirectory: string) => 'picked-client-id'), + pickConfigFile: vi.fn(async (_appDirectory: string) => 'shopify.app.staging.toml'), ...overrides, } } @@ -212,6 +214,100 @@ describe('resolveAppSecuritySelection with an app configuration', () => { }) }) +describe('resolveAppSecuritySelection with several TOMLs and none selected', () => { + async function writeProductionAndStaging(directory: string): Promise { + await writeConfiguration(directory, 'production-client-id', 'shopify.app.production.toml') + await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml') + } + + test('asks which TOML to scan, without saving the answer', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + const dependencies = promptDependencies() + + const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies) + + expect(selection).toMatchObject({ + kind: 'config', + appConfigFilePath: joinPath(await fileRealPath(directory), 'shopify.app.staging.toml'), + configClientId: 'staging-client-id', + appConfigFilePicked: true, + }) + expect(dependencies.pickConfigFile).toHaveBeenCalledOnce() + expect(setCachedAppInfo).not.toHaveBeenCalled() + }) + }) + + test('asks when the `app config use` choice no longer exists', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + vi.mocked(getCachedAppInfo).mockReturnValue({directory, configFile: 'shopify.app.deleted.toml'}) + const dependencies = promptDependencies() + + const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies) + + expect(selection).toMatchObject({configClientId: 'staging-client-id', appConfigFilePicked: true}) + expect(setCachedAppInfo).not.toHaveBeenCalled() + }) + }) + + test('scans shopify.app.toml without asking when it exists', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + await writeConfiguration(directory, 'default-client-id') + const dependencies = promptDependencies() + + const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies) + + expect(selection).toMatchObject({configClientId: 'default-client-id', appConfigFilePicked: undefined}) + expect(dependencies.pickConfigFile).not.toHaveBeenCalled() + }) + }) + + test('scans the only TOML without asking', async () => { + await inTemporaryDirectory(async (directory) => { + await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml') + const dependencies = promptDependencies() + + const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: false}, dependencies) + + expect(selection).toMatchObject({configClientId: 'staging-client-id', appConfigFilePicked: false}) + expect(dependencies.pickConfigFile).not.toHaveBeenCalled() + }) + }) + + test('aborts with the TOMLs to choose from when it cannot ask', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + const dependencies = promptDependencies() + + const error = await selectionError( + resolveAppSecuritySelection({path: directory, allowPrompts: false}, dependencies), + ) + + expect(error.message).toBe(`2 app configurations found in ${directory}, and none is selected.`) + expect(error.tryMessage).toBe('Pass `--config` with one of: production, staging.') + expect(dependencies.pickConfigFile).not.toHaveBeenCalled() + }) + }) + + test('aborts instead of asking with --client-id, which --config cannot repeat', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + const dependencies = promptDependencies() + + const error = await selectionError( + resolveAppSecuritySelection({path: directory, clientId: 'flag-client-id', allowPrompts: true}, dependencies), + ) + + expect(error.tryMessage).toBe( + "`--client-id` can't be combined with `--config`, so first select one with `shopify app config use `: production, staging.", + ) + expect(dependencies.pickConfigFile).not.toHaveBeenCalled() + }) + }) +}) + describe('resolveAppSecuritySelection with --without-app-config', () => { test('aborts without --client-id', async () => { await inTemporaryDirectory(async (directory) => { diff --git a/packages/app/src/cli/services/app-security-selection.ts b/packages/app/src/cli/services/app-security-selection.ts index 3701819835a..40375c78f64 100644 --- a/packages/app/src/cli/services/app-security-selection.ts +++ b/packages/app/src/cli/services/app-security-selection.ts @@ -1,9 +1,13 @@ import {localAppContext} from './app-context.js' import {appCreationDefaults} from './app/config/link.js' import {fetchOrCreateOrganizationApp} from './context.js' -import {NoAppConfigurationFoundError} from '../models/project/project.js' +import {getCachedAppInfo} from './local-storage.js' +import {NoAppConfigurationFoundError, Project} from '../models/project/project.js' +import {getAppConfigurationShorthand} from '../models/app/config-file-naming.js' +import {findConfigFiles, selectConfigFile} from '../prompts/config.js' +import {configurationFileNames} from '../constants.js' import {AbortError} from '@shopify/cli-kit/node/error' -import {fileRealPath, isDirectory} from '@shopify/cli-kit/node/fs' +import {fileExistsSync, fileRealPath, isDirectory} from '@shopify/cli-kit/node/fs' import {basename, cwd, isSubpath, joinPath, normalizePath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui' @@ -18,6 +22,8 @@ export type AppSecuritySelection = configClientId?: string /** The `--client-id` value, if passed. */ clientIdOverride?: string + /** True when `check` asked which TOML to scan, because nothing else selected one. */ + appConfigFilePicked?: boolean } | { kind: 'no-config' @@ -45,6 +51,7 @@ interface AppSecuritySelectionOptions { export interface AppSecuritySelectionDependencies { confirmScanWithoutAppConfig(directory: string): Promise pickClientId(appDirectory: string): Promise + pickConfigFile(appDirectory: string): Promise } const defaultDependencies: AppSecuritySelectionDependencies = { @@ -56,6 +63,7 @@ const defaultDependencies: AppSecuritySelectionDependencies = { defaultValue: false, }), pickClientId: async (appDirectory) => (await fetchOrCreateOrganizationApp(appCreationDefaults(appDirectory))).apiKey, + pickConfigFile: async (appDirectory) => (await selectConfigFile(appDirectory)).valueOrAbort(), } /** The name of the selected TOML, or undefined when there is none. */ @@ -110,9 +118,10 @@ export async function resolveAppSecuritySelection( await realDirectory(options.path) try { + const unselectedConfigFile = options.config ? undefined : await configFileWhenNoneIsSelected(options, dependencies) const {app} = await localAppContext({ directory: options.path, - userProvidedConfigName: options.config, + userProvidedConfigName: options.config ?? unselectedConfigFile?.fileName, skipPrompts: !options.allowPrompts, }) const appDirectory = await fileRealPath(app.directory) @@ -123,6 +132,7 @@ export async function resolveAppSecuritySelection( appConfigFilePath: joinPath(appDirectory, basename(app.configPath)), configClientId: app.configuration.client_id || undefined, clientIdOverride: options.clientId, + appConfigFilePicked: unselectedConfigFile?.picked, } } catch (error) { if (!(error instanceof NoAppConfigurationFoundError)) throw error @@ -130,6 +140,26 @@ export async function resolveAppSecuritySelection( } } +/** + * The TOML to scan when there's no `--config`, no `app config use` choice whose file exists and no shopify.app.toml. + * Other app commands abort then. With several TOMLs, `check` asks which one to scan instead, and doesn't save the + * answer: the printed commands carry it as `--config`. Undefined when the usual selection applies. + */ +async function configFileWhenNoneIsSelected( + options: AppSecuritySelectionOptions, + dependencies: AppSecuritySelectionDependencies, +): Promise<{fileName: string; picked: boolean} | undefined> { + const {directory} = await Project.load(options.path) + const selectedFileNames = [getCachedAppInfo(directory)?.configFile, configurationFileNames.app] + if (selectedFileNames.some((fileName) => fileName && fileExistsSync(joinPath(directory, fileName)))) return undefined + + const fileNames = (await findConfigFiles(directory)).map((path) => basename(path)) + if (fileNames.length === 1) return {fileName: fileNames[0]!, picked: false} + // `--client-id` can't be combined with `--config`, so a picked TOML couldn't be repeated by the printed commands. + if (!options.allowPrompts || options.clientId) abortNoAppConfigurationSelected(directory, fileNames, options.clientId) + return {fileName: await dependencies.pickConfigFile(directory), picked: true} +} + async function resolveWithoutAppConfigurationFile( options: AppSecuritySelectionOptions, dependencies: AppSecuritySelectionDependencies, @@ -208,6 +238,16 @@ function abortNoAppConfigurationFound(directory: string): never { ) } +function abortNoAppConfigurationSelected(directory: string, fileNames: string[], clientId?: string): never { + const configNames = fileNames.map((fileName) => getAppConfigurationShorthand(fileName) ?? fileName).join(', ') + throw new AbortError( + `${fileNames.length} app configurations found in ${directory}, and none is selected.`, + clientId + ? `\`--client-id\` can't be combined with \`--config\`, so first select one with \`shopify app config use \`: ${configNames}.` + : `Pass \`--config\` with one of: ${configNames}.`, + ) +} + async function realDirectory(path: string): Promise { const realPath = await realPathIfExists(path) if (realPath === undefined || !(await isDirectory(realPath))) throw new AbortError(`--path ${path}: not a directory.`) diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 0f95d7c6785..1b48eff2e46 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -464,6 +464,27 @@ describe('securityCheck', () => { ) }) + test('shows the generated check command, with --config, after asking which TOML to scan', async () => { + const selection: AppSecuritySelection = { + kind: 'config', + appDirectory, + appConfigFilePath: `${appDirectory}/shopify.app.staging.toml`, + configClientId: 'toml-client-id', + appConfigFilePicked: true, + } + const dependencies = testDependencies(scanExecution, selection) + dependencies.canPrompt.mockReturnValue(true) + + await securityCheck({...testOptions(), skipInstructions: true}, dependencies) + + const {scan} = commandsFor(selection) + expect(scan.args).toContainEqual({flag: '--config', value: 'staging'}) + expect(dependencies.renderInfo).toHaveBeenCalledWith({ + headline: 'To skip these prompts next time, run:', + body: [{command: formatAppSecurityCommand(scan)}], + }) + }) + test('does not show the prompt-flow command when a TOML was found', async () => { const dependencies = testDependencies() diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 0564c12d870..05ae9cf75ef 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -198,8 +198,10 @@ export default async function securityCheck( } const commands = resolveAppSecurityCommands(selection, options.directory, scope) const resolution = {selection, resultsKey: resultsKey(selection), commands} - // The prompt is only shown when no TOML was found and `--without-app-config` wasn't passed. - if (selection.kind === 'no-config' && !options.withoutAppConfig) { + // Prompts are shown when no TOML was found and `--without-app-config` wasn't passed, or when `check` asked which TOML + // to scan. + const prompted = selection.kind === 'no-config' ? !options.withoutAppConfig : selection.appConfigFilePicked === true + if (prompted) { dependencies.renderInfo({ headline: 'To skip these prompts next time, run:', body: [{command: formatAppSecurityCommand(commands.scan)}], From 5863860b2594e04a0ae6734f58fa12abd67ca6ec Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 12:30:12 -0700 Subject: [PATCH 2/3] Clean all App Security results without selecting an app configuration --- .../services/app-security-selection.test.ts | 28 +++++++++++++++++++ .../cli/services/app-security-selection.ts | 20 ++++++++++--- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/app-security-selection.test.ts b/packages/app/src/cli/services/app-security-selection.test.ts index 7c048867278..ef9bf9b7911 100644 --- a/packages/app/src/cli/services/app-security-selection.test.ts +++ b/packages/app/src/cli/services/app-security-selection.test.ts @@ -582,6 +582,34 @@ describe('resolveAppDirectory', () => { }) }) + test('finds the app directory when there are several TOMLs and none is selected', async () => { + await inTemporaryDirectory(async (directory) => { + await writeConfiguration(directory, 'production-client-id', 'shopify.app.production.toml') + await writeConfiguration(directory, 'staging-client-id', 'shopify.app.staging.toml') + + await expect(resolveAppDirectory({path: directory})).resolves.toBe(await fileRealPath(directory)) + }) + }) + + test('finds the app directory without validating its TOML', async () => { + await inTemporaryDirectory(async (directory) => { + await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = [') + + await expect(resolveAppDirectory({path: directory})).resolves.toBe(await fileRealPath(directory)) + }) + }) + + test('aborts when --path does not exist instead of walking up to the app above it', async () => { + await inTemporaryDirectory(async (directory) => { + await writeConfiguration(directory, 'default-client-id') + const missing = joinPath(directory, 'missing-app') + + await expect(resolveAppDirectory({path: missing})).rejects.toMatchObject({ + message: `--path ${missing}: not a directory.`, + }) + }) + }) + test('aborts when no TOML is found, without prompting', async () => { await inTemporaryDirectory(async (directory) => { await expect(resolveAppDirectory({path: directory})).rejects.toMatchObject({ diff --git a/packages/app/src/cli/services/app-security-selection.ts b/packages/app/src/cli/services/app-security-selection.ts index 40375c78f64..bfa5d6d2c6b 100644 --- a/packages/app/src/cli/services/app-security-selection.ts +++ b/packages/app/src/cli/services/app-security-selection.ts @@ -93,11 +93,23 @@ export function clientIdSource(selection: AppSecuritySelection): 'config' | 'fla /** * The app directory alone, for `clean --all`, which needs no client ID and no results key. With - * `--without-app-config` that's `--path` itself, so no client ID is required to find it. + * `--without-app-config` that's `--path` itself, so no client ID is required to find it. Otherwise it's the directory + * that holds the TOMLs, found by walking up as for the other commands. No TOML is selected or validated: they all share + * that directory, so `clean --all` also works with several TOMLs and none selected, or with a TOML that is invalid. */ -export async function resolveAppDirectory(options: Omit): Promise { - if (options.withoutAppConfig) return realDirectory(options.path) - return (await resolveAppSecuritySelection({...options, allowPrompts: false})).appDirectory +export async function resolveAppDirectory( + options: Pick, +): Promise { + // Checked before walking up: walking up from a missing directory would find, and clean, the app above it. + const directory = await realDirectory(options.path) + if (options.withoutAppConfig) return directory + + try { + return await fileRealPath((await Project.load(options.path)).directory) + } catch (error) { + if (!(error instanceof NoAppConfigurationFoundError)) throw error + abortNoAppConfigurationFound(options.path) + } } export async function resolveAppSecuritySelection( From a9c4f689cba341f4ac3453511985b938af6ec5d8 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 13:42:41 -0700 Subject: [PATCH 3/3] Scan shopify.app.toml without saving a choice when the app config use choice is stale --- .../services/app-security-selection.test.ts | 18 ++++++++++++++++++ .../src/cli/services/app-security-selection.ts | 10 ++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/app/src/cli/services/app-security-selection.test.ts b/packages/app/src/cli/services/app-security-selection.test.ts index ef9bf9b7911..8f16887ac58 100644 --- a/packages/app/src/cli/services/app-security-selection.test.ts +++ b/packages/app/src/cli/services/app-security-selection.test.ts @@ -14,6 +14,7 @@ import {validAppConfiguration} from './app-security-selection.test-data.js' import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {appCreationDefaults} from './app/config/link.js' import {fetchOrCreateOrganizationApp} from './context.js' +import use from './app/config/use.js' import {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' @@ -27,6 +28,7 @@ vi.mock('./local-storage.js', async (importOriginal) => ({ getCachedAppInfo: vi.fn(), setCachedAppInfo: vi.fn(), })) +vi.mock('./app/config/use.js', () => ({default: vi.fn()})) vi.mock('./context.js', async (importOriginal) => ({ ...(await importOriginal()), fetchOrCreateOrganizationApp: vi.fn(), @@ -251,6 +253,22 @@ describe('resolveAppSecuritySelection with several TOMLs and none selected', () }) }) + test('scans shopify.app.toml without asking or saving a choice when the `app config use` choice no longer exists', async () => { + await inTemporaryDirectory(async (directory) => { + await writeProductionAndStaging(directory) + await writeConfiguration(directory, 'default-client-id') + vi.mocked(getCachedAppInfo).mockReturnValue({directory, configFile: 'shopify.app.deleted.toml'}) + const dependencies = promptDependencies() + + const selection = await resolveAppSecuritySelection({path: directory, allowPrompts: true}, dependencies) + + expect(selection).toMatchObject({configClientId: 'default-client-id', appConfigFilePicked: false}) + expect(dependencies.pickConfigFile).not.toHaveBeenCalled() + expect(use).not.toHaveBeenCalled() + expect(setCachedAppInfo).not.toHaveBeenCalled() + }) + }) + test('scans shopify.app.toml without asking when it exists', async () => { await inTemporaryDirectory(async (directory) => { await writeProductionAndStaging(directory) diff --git a/packages/app/src/cli/services/app-security-selection.ts b/packages/app/src/cli/services/app-security-selection.ts index bfa5d6d2c6b..0c074e15336 100644 --- a/packages/app/src/cli/services/app-security-selection.ts +++ b/packages/app/src/cli/services/app-security-selection.ts @@ -156,14 +156,20 @@ export async function resolveAppSecuritySelection( * The TOML to scan when there's no `--config`, no `app config use` choice whose file exists and no shopify.app.toml. * Other app commands abort then. With several TOMLs, `check` asks which one to scan instead, and doesn't save the * answer: the printed commands carry it as `--config`. Undefined when the usual selection applies. + * + * With a stale `app config use` choice, shopify.app.toml is named explicitly: otherwise `localAppContext` would run + * `app config use`, which asks for a TOML and saves the answer. */ async function configFileWhenNoneIsSelected( options: AppSecuritySelectionOptions, dependencies: AppSecuritySelectionDependencies, ): Promise<{fileName: string; picked: boolean} | undefined> { const {directory} = await Project.load(options.path) - const selectedFileNames = [getCachedAppInfo(directory)?.configFile, configurationFileNames.app] - if (selectedFileNames.some((fileName) => fileName && fileExistsSync(joinPath(directory, fileName)))) return undefined + const cachedFileName = getCachedAppInfo(directory)?.configFile + if (cachedFileName && fileExistsSync(joinPath(directory, cachedFileName))) return undefined + if (fileExistsSync(joinPath(directory, configurationFileNames.app))) { + return cachedFileName ? {fileName: configurationFileNames.app, picked: false} : undefined + } const fileNames = (await findConfigFiles(directory)).map((path) => basename(path)) if (fileNames.length === 1) return {fileName: fileNames[0]!, picked: false}