From 6c26f5c682bb6c3d577770207b32db0865926a5b Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Wed, 2 Sep 2026 10:50:57 -0700 Subject: [PATCH 1/2] Add hidden App Doctor submit command Adds `shopify app doctor submit`, which reads the local App Doctor trace, builds a fail-closed allowlisted submission, and uploads it to App Management. The submission carries only check outcomes, finding fingerprints, redacted titles and justifications, engine and CLI versions, and a trace digest. No source code, file paths, snippets, evidence, messages, or commit identifiers are sent. The command extends BaseCommand rather than AppLinkedCommand so trace validation runs before the app is linked. It always writes `.shopify/app-doctor/submission.json` for inspection, then either stops (`--dry-run`), asks for confirmation, or uploads (`--force`). `--json` emits a single tagged result and links non-interactively. In non-interactive terminals `--force` is required unless `--dry-run` is set. Plumbing: `appRequestScanUploadUrl` and `appScanCreate` mutations with generated types, `generateScanUploadUrl`/`createAppScan` on the developer platform client, an `artifactName` label for `uploadToGCS`, and a `skipPrompts` option on `linkedAppContext`. The command is hidden, so there is no changeset. The App Management schema additions live in the gitignored local schema; codegen and the `graphql-schema` CI job will fail until Core ships the contract. Co-authored-by: AI Assisted-By: devx/f000c74b-02bd-420a-abaf-487fbbbd8b3e --- .../generated/create-source-scan.ts | 76 +++ .../request-source-scan-upload-url.ts | 79 +++ .../queries/create-source-scan.graphql | 9 + .../request-source-scan-upload-url.graphql | 9 + .../cli/commands/app/doctor/submit.test.ts | 123 +++++ .../app/src/cli/commands/app/doctor/submit.ts | 72 +++ packages/app/src/cli/index.ts | 2 + .../app/src/cli/models/app/app.test-data.ts | 17 + .../app/src/cli/services/app-context.test.ts | 73 +++ packages/app/src/cli/services/app-context.ts | 28 +- .../cli/services/app-doctor-artifacts.test.ts | 128 +++++ .../src/cli/services/app-doctor-artifacts.ts | 66 ++- .../app-doctor-engine/submission/index.ts | 228 ++++++++ .../doctor-submit-dry-run-result.json | 8 + .../tests/fixtures/doctor-submit-result.json | 9 + .../fixtures/submission-forbidden-values.json | 35 ++ .../tests/fixtures/submission-trace.ts | 247 +++++++++ .../tests/fixtures/submission.json | 149 +++++ .../tests/submission.test.ts | 98 ++++ .../services/app-doctor-submit-api.test.ts | 127 +++++ .../src/cli/services/app-doctor-submit-api.ts | 57 ++ packages/app/src/cli/services/bundle.test.ts | 48 ++ packages/app/src/cli/services/bundle.ts | 31 +- .../cli/services/doctor-submit-output.test.ts | 120 ++++ .../src/cli/services/doctor-submit-output.ts | 98 ++++ .../src/cli/services/doctor-submit.test.ts | 513 ++++++++++++++++++ .../app/src/cli/services/doctor-submit.ts | 228 ++++++++ .../utilities/developer-platform-client.ts | 20 + .../app-management-client.test.ts | 57 ++ .../app-management-client.ts | 27 + packages/cli/oclif.manifest.json | 119 ++++ 31 files changed, 2887 insertions(+), 14 deletions(-) create mode 100644 packages/app/src/cli/api/graphql/app-management/generated/create-source-scan.ts create mode 100644 packages/app/src/cli/api/graphql/app-management/generated/request-source-scan-upload-url.ts create mode 100644 packages/app/src/cli/api/graphql/app-management/queries/create-source-scan.graphql create mode 100644 packages/app/src/cli/api/graphql/app-management/queries/request-source-scan-upload-url.graphql create mode 100644 packages/app/src/cli/commands/app/doctor/submit.test.ts create mode 100644 packages/app/src/cli/commands/app/doctor/submit.ts create mode 100644 packages/app/src/cli/services/app-doctor-artifacts.test.ts create mode 100644 packages/app/src/cli/services/app-doctor-engine/submission/index.ts create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-dry-run-result.json create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-result.json create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-forbidden-values.json create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-trace.ts create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission.json create mode 100644 packages/app/src/cli/services/app-doctor-engine/tests/submission.test.ts create mode 100644 packages/app/src/cli/services/app-doctor-submit-api.test.ts create mode 100644 packages/app/src/cli/services/app-doctor-submit-api.ts create mode 100644 packages/app/src/cli/services/doctor-submit-output.test.ts create mode 100644 packages/app/src/cli/services/doctor-submit-output.ts create mode 100644 packages/app/src/cli/services/doctor-submit.test.ts create mode 100644 packages/app/src/cli/services/doctor-submit.ts diff --git a/packages/app/src/cli/api/graphql/app-management/generated/create-source-scan.ts b/packages/app/src/cli/api/graphql/app-management/generated/create-source-scan.ts new file mode 100644 index 00000000000..37e969c5b0b --- /dev/null +++ b/packages/app/src/cli/api/graphql/app-management/generated/create-source-scan.ts @@ -0,0 +1,76 @@ +/* eslint-disable @typescript-eslint/consistent-type-definitions */ +import * as Types from './types.js' + +import {TypedDocumentNode as DocumentNode} from '@graphql-typed-document-node/core' + +export type CreateSourceScanMutationVariables = Types.Exact<{ + appId: Types.Scalars['ID']['input'] + sourceScanUrl: Types.Scalars['URL']['input'] +}> + +export type CreateSourceScanMutation = { + appSourceScanCreate: {accepted: boolean; userErrors: {field?: string[] | null; message: string}[]} +} + +export const CreateSourceScan = { + kind: 'Document', + definitions: [ + { + kind: 'OperationDefinition', + operation: 'mutation', + name: {kind: 'Name', value: 'CreateSourceScan'}, + variableDefinitions: [ + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'appId'}}, + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'ID'}}}, + }, + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'sourceScanUrl'}}, + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'URL'}}}, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + { + kind: 'Field', + name: {kind: 'Name', value: 'appSourceScanCreate'}, + arguments: [ + { + kind: 'Argument', + name: {kind: 'Name', value: 'appId'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'appId'}}, + }, + { + kind: 'Argument', + name: {kind: 'Name', value: 'sourceScanUrl'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'sourceScanUrl'}}, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'accepted'}}, + { + kind: 'Field', + name: {kind: 'Name', value: 'userErrors'}, + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'field'}}, + {kind: 'Field', name: {kind: 'Name', value: 'message'}}, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + ], + }, + }, + ], +} as unknown as DocumentNode diff --git a/packages/app/src/cli/api/graphql/app-management/generated/request-source-scan-upload-url.ts b/packages/app/src/cli/api/graphql/app-management/generated/request-source-scan-upload-url.ts new file mode 100644 index 00000000000..322c0a477d8 --- /dev/null +++ b/packages/app/src/cli/api/graphql/app-management/generated/request-source-scan-upload-url.ts @@ -0,0 +1,79 @@ +/* eslint-disable @typescript-eslint/consistent-type-definitions */ +import * as Types from './types.js' + +import {TypedDocumentNode as DocumentNode} from '@graphql-typed-document-node/core' + +export type RequestSourceScanUploadUrlMutationVariables = Types.Exact<{ + appId: Types.Scalars['ID']['input'] + byteSize: Types.Scalars['Int']['input'] +}> + +export type RequestSourceScanUploadUrlMutation = { + appRequestSourceScanUploadUrl: { + sourceScanUploadUrl?: string | null + userErrors: {field?: string[] | null; message: string}[] + } +} + +export const RequestSourceScanUploadUrl = { + kind: 'Document', + definitions: [ + { + kind: 'OperationDefinition', + operation: 'mutation', + name: {kind: 'Name', value: 'RequestSourceScanUploadUrl'}, + variableDefinitions: [ + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'appId'}}, + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'ID'}}}, + }, + { + kind: 'VariableDefinition', + variable: {kind: 'Variable', name: {kind: 'Name', value: 'byteSize'}}, + type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'Int'}}}, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + { + kind: 'Field', + name: {kind: 'Name', value: 'appRequestSourceScanUploadUrl'}, + arguments: [ + { + kind: 'Argument', + name: {kind: 'Name', value: 'appId'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'appId'}}, + }, + { + kind: 'Argument', + name: {kind: 'Name', value: 'byteSize'}, + value: {kind: 'Variable', name: {kind: 'Name', value: 'byteSize'}}, + }, + ], + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'sourceScanUploadUrl'}}, + { + kind: 'Field', + name: {kind: 'Name', value: 'userErrors'}, + selectionSet: { + kind: 'SelectionSet', + selections: [ + {kind: 'Field', name: {kind: 'Name', value: 'field'}}, + {kind: 'Field', name: {kind: 'Name', value: 'message'}}, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + {kind: 'Field', name: {kind: 'Name', value: '__typename'}}, + ], + }, + }, + ], + }, + }, + ], +} as unknown as DocumentNode diff --git a/packages/app/src/cli/api/graphql/app-management/queries/create-source-scan.graphql b/packages/app/src/cli/api/graphql/app-management/queries/create-source-scan.graphql new file mode 100644 index 00000000000..e59232c70da --- /dev/null +++ b/packages/app/src/cli/api/graphql/app-management/queries/create-source-scan.graphql @@ -0,0 +1,9 @@ +mutation CreateSourceScan($appId: ID!, $sourceScanUrl: URL!) { + appSourceScanCreate(appId: $appId, sourceScanUrl: $sourceScanUrl) { + accepted + userErrors { + field + message + } + } +} diff --git a/packages/app/src/cli/api/graphql/app-management/queries/request-source-scan-upload-url.graphql b/packages/app/src/cli/api/graphql/app-management/queries/request-source-scan-upload-url.graphql new file mode 100644 index 00000000000..e01884eb654 --- /dev/null +++ b/packages/app/src/cli/api/graphql/app-management/queries/request-source-scan-upload-url.graphql @@ -0,0 +1,9 @@ +mutation RequestSourceScanUploadUrl($appId: ID!, $byteSize: Int!) { + appRequestSourceScanUploadUrl(appId: $appId, byteSize: $byteSize) { + sourceScanUploadUrl + userErrors { + field + message + } + } +} diff --git a/packages/app/src/cli/commands/app/doctor/submit.test.ts b/packages/app/src/cli/commands/app/doctor/submit.test.ts new file mode 100644 index 00000000000..5e59c2884a4 --- /dev/null +++ b/packages/app/src/cli/commands/app/doctor/submit.test.ts @@ -0,0 +1,123 @@ +import DoctorSubmit from './submit.js' +import {appFlags} from '../../../flags.js' +import doctorSubmit from '../../../services/doctor-submit.js' +import AppLinkedCommand from '../../../utilities/app-linked-command.js' +import BaseCommand from '@shopify/cli-kit/node/base-command' +import {cwd, resolvePath} from '@shopify/cli-kit/node/path' +import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' +import {beforeEach, describe, expect, test, vi} from 'vitest' + +vi.mock('../../../services/doctor-submit.js') +vi.mock('@shopify/cli-kit/node/system') + +describe('app doctor submit command', () => { + beforeEach(() => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(true) + }) + + test('is hidden and lets the service link only after trace validation', () => { + expect(DoctorSubmit.hidden).toBe(true) + expect(DoctorSubmit.prototype).toBeInstanceOf(BaseCommand) + expect(DoctorSubmit.prototype).not.toBeInstanceOf(AppLinkedCommand) + expect(DoctorSubmit.flags.path).toBe(appFlags.path) + expect(DoctorSubmit.flags.config).toBe(appFlags.config) + expect(DoctorSubmit.flags['client-id']).toBe(appFlags['client-id']) + expect(DoctorSubmit.args).not.toHaveProperty('directory') + expect(DoctorSubmit.descriptionWithMarkdown).toContain('Generated report fields exclude source code, file paths') + expect(DoctorSubmit.descriptionWithMarkdown).toContain('Optional feedback is included without redaction') + expect(DoctorSubmit.descriptionWithMarkdown).not.toContain( + 'No source code, file paths, snippets, or commit identifiers are sent', + ) + expect(DoctorSubmit.descriptionWithMarkdown).toContain('--version') + expect(DoctorSubmit.descriptionWithMarkdown).toContain('--source-control-url') + }) + + test('forwards defaults from the current directory', async () => { + await DoctorSubmit.run([], import.meta.url) + + expect(doctorSubmit).toHaveBeenCalledWith({ + directory: cwd(), + json: false, + force: false, + dryRun: false, + clientId: undefined, + configName: undefined, + versionTag: undefined, + sourceControlUrl: undefined, + feedback: undefined, + }) + }) + + test('forwards submit flags with --client-id', async () => { + await DoctorSubmit.run( + [ + '--path', + './fixtures/app', + '--client-id', + 'client-id', + '--json', + '--force', + '--dry-run', + '--version', + 'v1.2.3', + '--source-control-url', + 'https://github.com/example/app/tree/v1.2.3', + '--feedback', + 'The authorization result was inaccurate.', + ], + import.meta.url, + ) + + expect(doctorSubmit).toHaveBeenCalledWith({ + directory: resolvePath('./fixtures/app'), + json: true, + force: true, + dryRun: true, + clientId: 'client-id', + configName: undefined, + versionTag: 'v1.2.3', + sourceControlUrl: 'https://github.com/example/app/tree/v1.2.3', + feedback: 'The authorization result was inaccurate.', + }) + }) + + test('forwards --config separately because --config and --client-id are exclusive', async () => { + await DoctorSubmit.run(['--config', 'staging'], import.meta.url) + + expect(doctorSubmit).toHaveBeenCalledWith({ + directory: cwd(), + json: false, + force: false, + dryRun: false, + clientId: undefined, + configName: 'staging', + versionTag: undefined, + sourceControlUrl: undefined, + feedback: undefined, + }) + }) + + test('fails at parse time in a non-interactive terminal without --force', async () => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(false) + + await expect(DoctorSubmit.run([], import.meta.url)).rejects.toThrow() + expect(doctorSubmit).not.toHaveBeenCalled() + }) + + test('allows --dry-run in a non-interactive terminal without --force', async () => { + vi.mocked(terminalSupportsPrompting).mockReturnValue(false) + + await DoctorSubmit.run(['--json', '--dry-run'], import.meta.url) + + expect(doctorSubmit).toHaveBeenCalledWith(expect.objectContaining({json: true, dryRun: true, force: false})) + }) + + test('uses the established flag aliases and environment variables', () => { + expect(DoctorSubmit.flags.force.char).toBe('f') + expect(DoctorSubmit.flags.force.env).toBe('SHOPIFY_FLAG_FORCE') + expect(DoctorSubmit.flags['dry-run'].env).toBe('SHOPIFY_FLAG_APP_DOCTOR_DRY_RUN') + expect(DoctorSubmit.flags.version.env).toBe('SHOPIFY_FLAG_VERSION') + expect(DoctorSubmit.flags['source-control-url'].env).toBe('SHOPIFY_FLAG_SOURCE_CONTROL_URL') + expect(DoctorSubmit.flags.feedback.env).toBe('SHOPIFY_FLAG_APP_DOCTOR_FEEDBACK') + }) +}) diff --git a/packages/app/src/cli/commands/app/doctor/submit.ts b/packages/app/src/cli/commands/app/doctor/submit.ts new file mode 100644 index 00000000000..65c092a689f --- /dev/null +++ b/packages/app/src/cli/commands/app/doctor/submit.ts @@ -0,0 +1,72 @@ +import {appFlags} from '../../../flags.js' +import doctorSubmit from '../../../services/doctor-submit.js' +import {Flags} from '@oclif/core' +import BaseCommand, {type NonTTYFlagRequirement} from '@shopify/cli-kit/node/base-command' +import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' + +export default class DoctorSubmit extends BaseCommand { + static hidden = true + + static summary = 'Submit App Doctor results to Shopify.' + + static descriptionWithMarkdown = `Reads the most recent App Doctor trace, writes a \`.shopify/app-doctor/submission.json\` file for inspection, asks for confirmation, and uploads the result to Shopify. + +Generated report fields exclude source code, file paths, code snippets, evidence, finding messages, and commit identifiers. Optional feedback is included without redaction. Optional \`--version\` and \`--source-control-url\` metadata is included only when supplied. Use \`--dry-run\` to write and inspect the exact payload without uploading it.` + + static description = this.descriptionWithoutMarkdown() + + static flags = { + ...globalFlags, + path: appFlags.path, + config: appFlags.config, + 'client-id': appFlags['client-id'], + ...jsonFlag, + version: Flags.string({ + hidden: false, + description: + 'Optional version tag that will be associated with this app version. If not provided, an auto-generated identifier will be generated for this app version.', + env: 'SHOPIFY_FLAG_VERSION', + }), + 'source-control-url': Flags.string({ + hidden: false, + description: 'URL associated with the new app version.', + env: 'SHOPIFY_FLAG_SOURCE_CONTROL_URL', + }), + feedback: Flags.string({ + description: 'Optional feedback about inaccurate or unhelpful App Doctor results. Use - to read from stdin.', + env: 'SHOPIFY_FLAG_APP_DOCTOR_FEEDBACK', + }), + force: Flags.boolean({ + char: 'f', + description: 'Skip confirmation. Required if non interactive.', + env: 'SHOPIFY_FLAG_FORCE', + default: false, + }), + 'dry-run': Flags.boolean({ + description: 'Write the submission payload without uploading it.', + env: 'SHOPIFY_FLAG_APP_DOCTOR_DRY_RUN', + default: false, + }), + } + + static nonTTYFlagRequirements(): NonTTYFlagRequirement[] { + // Dry runs never upload, so they may run non-interactively without --force. + return [{flags: ['force'], when: (flags) => !flags['dry-run']}] + } + + public async run(): Promise { + const {flags} = await this.parse(DoctorSubmit) + + await doctorSubmit({ + directory: flags.path, + json: flags.json, + force: flags.force, + dryRun: flags['dry-run'], + clientId: flags['client-id'], + configName: flags.config, + versionTag: flags.version, + sourceControlUrl: flags['source-control-url'], + feedback: flags.feedback, + }) + } +} diff --git a/packages/app/src/cli/index.ts b/packages/app/src/cli/index.ts index b08883345a5..fb4f7ec4749 100644 --- a/packages/app/src/cli/index.ts +++ b/packages/app/src/cli/index.ts @@ -8,6 +8,7 @@ import DemoWatcher from './commands/app/demo/watcher.js' import Deploy from './commands/app/deploy.js' import Dev from './commands/app/dev.js' import DoctorInstructions from './commands/app/doctor/instructions.js' +import DoctorSubmit from './commands/app/doctor/submit.js' import Doctor from './commands/app/doctor.js' import Logs from './commands/app/logs.js' import Sources from './commands/app/app-logs/sources.js' @@ -55,6 +56,7 @@ export const commands: {[key: string]: typeof AppLinkedCommand | typeof AppUnlin 'app:dev': Dev, 'app:dev:clean': DevClean, 'app:doctor:instructions': DoctorInstructions, + 'app:doctor:submit': DoctorSubmit, 'app:doctor': Doctor, 'app:logs': Logs, 'app:logs:sources': Sources, diff --git a/packages/app/src/cli/models/app/app.test-data.ts b/packages/app/src/cli/models/app/app.test-data.ts index f66757b9036..ee16ab80c39 100644 --- a/packages/app/src/cli/models/app/app.test-data.ts +++ b/packages/app/src/cli/models/app/app.test-data.ts @@ -27,6 +27,10 @@ import {WebhooksConfig} from '../extensions/specifications/types/app_config_webh import {PaymentsAppExtensionConfigType} from '../extensions/specifications/payments_app_extension.js' import { AppLogsResponse, + SourceScanCreateInput, + SourceScanCreateSchema, + SourceScanUploadUrlInput, + SourceScanUploadUrlSchema, AppVersion, AppVersionIdentifiers, AppVersionWithContext, @@ -1242,6 +1246,16 @@ const generateSignedUploadUrlResponse: AssetUrlSchema = { userErrors: [], } +const generateSourceScanUploadUrlResponse: SourceScanUploadUrlSchema = { + sourceScanUploadUrl: 'source-scan-upload-url', + userErrors: [], +} + +const createSourceScanResponse: SourceScanCreateSchema = { + accepted: true, + userErrors: [], +} + const organizationsResponse: Organization[] = [testOrganization()] const sendSampleWebhookResponse: SendSampleWebhookSchema = { @@ -1330,6 +1344,9 @@ export function testDeveloperPlatformClient( deploy: (_input: AppDeployVariables) => Promise.resolve(deployResponse), release: (_input: {app: MinimalAppIdentifiers; version: AppVersionIdentifiers}) => Promise.resolve(releaseResponse), generateSignedUploadUrl: (_app: MinimalAppIdentifiers) => Promise.resolve(generateSignedUploadUrlResponse), + generateSourceScanUploadUrl: (_input: SourceScanUploadUrlInput) => + Promise.resolve(generateSourceScanUploadUrlResponse), + createSourceScan: (_input: SourceScanCreateInput) => Promise.resolve(createSourceScanResponse), sendSampleWebhook: (_input: SendSampleWebhookVariables) => Promise.resolve(sendSampleWebhookResponse), apiVersions: () => Promise.resolve(apiVersionsResponse), topics: (_input: WebhookTopicsVariables) => Promise.resolve(topicsResponse), diff --git a/packages/app/src/cli/services/app-context.test.ts b/packages/app/src/cli/services/app-context.test.ts index 95fe5c3ea0b..e3ac627d2be 100644 --- a/packages/app/src/cli/services/app-context.test.ts +++ b/packages/app/src/cli/services/app-context.test.ts @@ -11,6 +11,7 @@ import metadata from '../metadata.js' import * as loader from '../models/app/loader.js' import {loadLocalExtensionsSpecifications} from '../models/extensions/load-specifications.js' import {beforeEach, describe, expect, test, vi} from 'vitest' +import {AbortError} from '@shopify/cli-kit/node/error' import {inTemporaryDirectory, writeFile, mkdir} from '@shopify/cli-kit/node/fs' import {joinPath, normalizePath} from '@shopify/cli-kit/node/path' import {tryParseInt} from '@shopify/cli-kit/common/string' @@ -46,6 +47,78 @@ beforeEach(() => { }) describe('linkedAppContext', () => { + test('passes skipPrompts to active config selection', async () => { + await inTemporaryDirectory(async (tmp) => { + const content = ` +name = "test-app" +client_id="test-api-key"` + await writeAppConfig(tmp, content) + const getAppConfigSpy = vi.spyOn(loader, 'getAppConfigurationContext') + + try { + await linkedAppContext({ + directory: tmp, + forceRelink: false, + userProvidedConfigName: undefined, + clientId: undefined, + skipPrompts: true, + }) + + expect(getAppConfigSpy).toHaveBeenCalledWith(tmp, undefined, {skipPrompts: true}) + } finally { + getAppConfigSpy.mockRestore() + } + }) + }) + + test('aborts before linking an unlinked app without a client ID when prompts are skipped', async () => { + await inTemporaryDirectory(async (tmp) => { + const content = ` +name = "test-app"` + await writeAppConfig(tmp, content) + + const error = await linkedAppContext({ + directory: tmp, + forceRelink: false, + userProvidedConfigName: undefined, + clientId: undefined, + skipPrompts: true, + }).catch((error: unknown) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(error).toMatchObject({ + message: 'This app must be linked before continuing in non-interactive mode.', + nextSteps: ['Pass `--client-id ` to select the app without prompting.'], + }) + expect(link).not.toHaveBeenCalled() + }) + }) + + test('links an unlinked app without rendering success when prompts are skipped and client ID is explicit', async () => { + await inTemporaryDirectory(async (tmp) => { + const content = ` +name = "test-app"` + await writeAppConfig(tmp, content) + const stoppedAfterLink = new Error('stop after verifying link arguments') + vi.mocked(link).mockRejectedValueOnce(stoppedAfterLink) + + await expect( + linkedAppContext({ + directory: tmp, + forceRelink: false, + userProvidedConfigName: undefined, + clientId: 'explicit-client-id', + skipPrompts: true, + }), + ).rejects.toBe(stoppedAfterLink) + + expect(link).toHaveBeenCalledWith( + {directory: tmp, apiKey: 'explicit-client-id', configName: 'shopify.app.toml'}, + false, + ) + }) + }) + test('returns linked app context when app is already linked', async () => { await inTemporaryDirectory(async (tmp) => { // Given diff --git a/packages/app/src/cli/services/app-context.ts b/packages/app/src/cli/services/app-context.ts index 00d37b86fce..a082b06c780 100644 --- a/packages/app/src/cli/services/app-context.ts +++ b/packages/app/src/cli/services/app-context.ts @@ -1,7 +1,7 @@ import {appFromIdentifiers} from './context.js' import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {fetchSpecifications} from './generate/fetch-extension-specifications.js' -import link from './app/config/link.js' +import link, {type LinkOptions} from './app/config/link.js' import {fetchOrgFromId} from './dev/fetch.js' import {addUidToTomlsIfNecessary} from './app/add-uid-to-extension-toml.js' import {loadLocalExtensionsSpecifications} from '../models/extensions/load-specifications.js' @@ -45,6 +45,7 @@ export interface LoadedAppContextOutput { * @param forceRelink - Whether to force a relink of the app, this includes re-selecting the remote org and app. * @param clientId - The client ID to use when linking the app or when fetching the remote app. * @param userProvidedConfigName - The name of an existing config file in the app, if not provided, the cached/default one will be used. + * @param skipPrompts - When true, config selection and required linking must not prompt or render link success. * @param unsafeTolerateErrors - When true, the loaded app may contain validation errors without throwing. * Only use this for commands that explicitly handle invalid configs (e.g. `app info`, `app validate`). */ @@ -53,6 +54,7 @@ interface LoadedAppContextOptions { forceRelink: boolean clientId: string | undefined userProvidedConfigName: string | undefined + skipPrompts?: boolean unsafeTolerateErrors?: boolean } @@ -77,11 +79,22 @@ interface LocalAppContextOptions { * * @returns The local app, the remote app, the correct developer platform client, and the remote specifications list. */ +async function linkForAppContext(options: LinkOptions, skipPrompts: boolean) { + if (skipPrompts && !options.apiKey) { + throw new AbortError('This app must be linked before continuing in non-interactive mode.', null, [ + 'Pass `--client-id ` to select the app without prompting.', + ]) + } + + return skipPrompts ? link(options, false) : link(options) +} + export async function linkedAppContext({ directory, clientId, forceRelink, userProvidedConfigName, + skipPrompts = false, unsafeTolerateErrors = false, }: LoadedAppContextOptions): Promise { let project: Project @@ -91,13 +104,13 @@ export async function linkedAppContext({ if (forceRelink) { // Skip getAppConfigurationContext() when force-relinking — it may prompt the // user to select a TOML file that will be immediately discarded by link(). - const result = await link({directory, apiKey: clientId}) + const result = await linkForAppContext({directory, apiKey: clientId}, skipPrompts) remoteApp = result.remoteApp - const reloaded = await getAppConfigurationContext(directory, result.configFileName) + const reloaded = await getAppConfigurationContext(directory, result.configFileName, {skipPrompts}) project = reloaded.project activeConfig = reloaded.activeConfig } else { - const loaded = await getAppConfigurationContext(directory, userProvidedConfigName) + const loaded = await getAppConfigurationContext(directory, userProvidedConfigName, {skipPrompts}) project = loaded.project activeConfig = loaded.activeConfig @@ -106,9 +119,12 @@ export async function linkedAppContext({ } if (!activeConfig.isLinked) { - const result = await link({directory, apiKey: clientId, configName: basename(activeConfig.file.path)}) + const result = await linkForAppContext( + {directory, apiKey: clientId, configName: basename(activeConfig.file.path)}, + skipPrompts, + ) remoteApp = result.remoteApp - const reloaded = await getAppConfigurationContext(directory, result.configFileName) + const reloaded = await getAppConfigurationContext(directory, result.configFileName, {skipPrompts}) project = reloaded.project activeConfig = reloaded.activeConfig } diff --git a/packages/app/src/cli/services/app-doctor-artifacts.test.ts b/packages/app/src/cli/services/app-doctor-artifacts.test.ts new file mode 100644 index 00000000000..ade25845b0d --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-artifacts.test.ts @@ -0,0 +1,128 @@ +import {appDoctorArtifactPaths, readTrace, writeSubmission} from './app-doctor-artifacts.js' +import {sha256} from './app-doctor-engine/index.js' +import {SUBMISSION_SCHEMA_VERSION} from './app-doctor-engine/submission/index.js' +import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {joinPath} from '@shopify/cli-kit/node/path' +import {describe, expect, test} from 'vitest' +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {TraceV2} from './app-doctor-engine/types.js' + +function validTrace(): TraceV2 { + const unsigned: Omit = { + schema_version: 2, + engine: {name: 'shopify-app-doctor', version: '0.1.0', ruleset: 'app-doctor-rules@0.1.0'}, + generated_at: '2026-09-01T00:00:00.000Z', + project: { + commit: null, + dirty: false, + input_hash: `sha256:${'a'.repeat(64)}`, + input_hashes: {}, + }, + detection: {framework: 'none', surface: 'config_only', languages: []}, + score: {total: 100, baseline: 100, grade: 'EXCELLENT'}, + findings: [], + checks_executed: [], + suppressions: [], + coverage: {files_scanned: 1, files_skipped: [], complete: true, gaps: []}, + } + return {...unsigned, attestation: {digest: sha256(unsigned), signed: false}} +} + +const submission = { + schemaVersion: SUBMISSION_SCHEMA_VERSION, + report: {metadata: {}}, +} as AppDoctorSubmission + +describe('appDoctorArtifactPaths', () => { + test('resolves every artifact under .shopify/app-doctor', () => { + const paths = appDoctorArtifactPaths('/tmp/example-app') + + expect(paths).toEqual({ + artifactDirectory: joinPath('/tmp/example-app', '.shopify', 'app-doctor'), + tracePath: joinPath('/tmp/example-app', '.shopify', 'app-doctor', 'trace.json'), + reviewPath: joinPath('/tmp/example-app', '.shopify', 'app-doctor', 'review.json'), + submissionPath: joinPath('/tmp/example-app', '.shopify', 'app-doctor', 'submission.json'), + }) + }) +}) + +describe('readTrace', () => { + test('returns a validated v2 trace', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, 'trace.json') + const trace = validTrace() + await writeFile(path, `${JSON.stringify(trace)}\n`) + + await expect(readTrace(path)).resolves.toEqual({status: 'ok', trace}) + }) + }) + + test('returns missing when the file does not exist', async () => { + await inTemporaryDirectory(async (directory) => { + await expect(readTrace(joinPath(directory, 'trace.json'))).resolves.toEqual({status: 'missing'}) + }) + }) + + test('returns a parse error for invalid JSON', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, 'trace.json') + await writeFile(path, '{invalid') + + const result = await readTrace(path) + + expect(result.status).toBe('invalid') + if (result.status === 'invalid') expect(result.errors[0]).toContain('Could not parse JSON') + }) + }) + + test('preserves every validateTrace schema error as a list', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, 'trace.json') + await writeFile(path, '{}') + + const result = await readTrace(path) + + expect(result.status).toBe('invalid') + if (result.status === 'invalid') { + expect(result.errors.length).toBeGreaterThan(1) + expect(result.errors).toContain('unsupported schema_version: undefined') + } + }) + }) + + test('returns invalid for an unreadable artifact path', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, 'trace.json') + await mkdir(path) + + const result = await readTrace(path) + + expect(result.status).toBe('invalid') + if (result.status === 'invalid') expect(result.errors).toHaveLength(1) + }) + }) + + test('rejects a real file larger than 5 MB before parsing', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, 'trace.json') + await writeFile(path, 'x'.repeat(5_000_001)) + + await expect(readTrace(path)).resolves.toEqual({ + status: 'invalid', + errors: ['The trace file is larger than 5 MB.'], + }) + }) + }) +}) + +describe('writeSubmission', () => { + test('creates parent directories and writes pretty JSON with a trailing newline', async () => { + await inTemporaryDirectory(async (directory) => { + const path = joinPath(directory, '.shopify', 'app-doctor', 'submission.json') + + await writeSubmission(directory, submission) + + await expect(readFile(path)).resolves.toBe(`${JSON.stringify(submission, null, 2)}\n`) + }) + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-artifacts.ts b/packages/app/src/cli/services/app-doctor-artifacts.ts index 6d8a313c688..1da53529be4 100644 --- a/packages/app/src/cli/services/app-doctor-artifacts.ts +++ b/packages/app/src/cli/services/app-doctor-artifacts.ts @@ -1,8 +1,14 @@ +import {validateTrace} from './app-doctor-engine/index.js' +import {fileExists, fileSize, readFile} from '@shopify/cli-kit/node/fs' import {AbortError} from '@shopify/cli-kit/node/error' import {joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' import {randomBytes} from 'node:crypto' import {lstat, mkdir, realpath, rename, unlink, writeFile} from 'node:fs/promises' import type {AppDoctorExecution} from './app-doctor-api.js' +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {TraceV2} from './app-doctor-engine/types.js' + +const MAX_TRACE_FILE_SIZE_BYTES = 5_000_000 export interface AppDoctorArtifactPaths { artifactDirectory: string @@ -10,11 +16,21 @@ export interface AppDoctorArtifactPaths { reviewPath?: string } -function appDoctorArtifactPaths(appRoot: string): Required { +export interface ResolvedAppDoctorArtifactPaths extends Required { + submissionPath: string +} + +export type ReadTraceResult = + | {status: 'ok'; trace: TraceV2} + | {status: 'missing'} + | {status: 'invalid'; errors: string[]} + +export function appDoctorArtifactPaths(appRoot: string): ResolvedAppDoctorArtifactPaths { const artifactDirectory = joinPath(appRoot, '.shopify', 'app-doctor') return { artifactDirectory, reviewPath: joinPath(artifactDirectory, 'review.json'), + submissionPath: joinPath(artifactDirectory, 'submission.json'), tracePath: joinPath(artifactDirectory, 'trace.json'), } } @@ -28,7 +44,11 @@ export async function writeAppDoctorArtifacts(execution: AppDoctorExecution): Pr } await writeAtomicArtifact(paths.reviewPath, `${JSON.stringify(execution.reviewPack, null, 2)}\n`) - return paths + return { + artifactDirectory: paths.artifactDirectory, + reviewPath: paths.reviewPath, + tracePath: paths.tracePath, + } } async function ensureArtifactDirectory(appRoot: string, artifactDirectory: string): Promise { @@ -104,3 +124,45 @@ async function assertNotSymbolicLink(path: string): Promise { throw error } } + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + +export async function readTrace(path: string): Promise { + if (!(await fileExists(path))) return {status: 'missing'} + + let content: string + try { + if ((await fileSize(path)) > MAX_TRACE_FILE_SIZE_BYTES) { + return {status: 'invalid', errors: ['The trace file is larger than 5 MB.']} + } + content = await readFile(path) + // Filesystem failures are returned for command-layer rendering. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + return {status: 'invalid', errors: [`Could not read the trace file: ${errorMessage(error)}`]} + } + + let parsed: unknown + try { + parsed = JSON.parse(content) + // JSON is an untrusted artifact boundary. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + return {status: 'invalid', errors: [`Could not parse JSON: ${errorMessage(error)}`]} + } + + // Keep validation errors structured. Do not replace this with assertCompatibleTrace, + // which joins them into one exception string. + const validation = validateTrace(parsed) + if (!validation.valid) return {status: 'invalid', errors: validation.errors} + + return {status: 'ok', trace: parsed as TraceV2} +} + +export async function writeSubmission(appRoot: string, payload: AppDoctorSubmission): Promise { + const paths = appDoctorArtifactPaths(appRoot) + await ensureArtifactDirectory(appRoot, paths.artifactDirectory) + await writeAtomicArtifact(paths.submissionPath, `${JSON.stringify(payload, null, 2)}\n`) +} diff --git a/packages/app/src/cli/services/app-doctor-engine/submission/index.ts b/packages/app/src/cli/services/app-doctor-engine/submission/index.ts new file mode 100644 index 00000000000..efa85c97f4f --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/submission/index.ts @@ -0,0 +1,228 @@ +import {redactText} from '../rules/secret-rules.js' +import type { + AnalysisMode, + CheckExecution, + CheckExecutionReasonCode, + CheckExecutionStatus, + DetectedFramework, + DetectedSurface, + FindingSource, + LanguageSupport, + Severity, + SuppressionProvenance, + TraceFinding, + TraceV2, +} from '../types.js' + +export const SUBMISSION_SCHEMA_VERSION = 1 as const + +export interface BuildSubmissionOptions { + cliVersion: string + submittedAt: string + versionTag?: string + sourceControlUrl?: string + feedback?: string +} + +interface SubmissionFinding { + fingerprint: string + source: FindingSource + severity: Severity + title: string + rule_id?: string + rule_version?: number + check_id?: string + check_version?: number + prompt_hash?: string + suppressed: boolean + suppression_id?: string +} + +interface SubmissionCheckImplementation { + id: string + analysis_mode: AnalysisMode + status: CheckExecutionStatus + finding_count: number + inspected_file_count: number + reason_code?: CheckExecutionReasonCode +} + +interface SubmissionCheck { + id: string + version: number + kind: CheckExecution['kind'] + status: CheckExecutionStatus + required: boolean + applicable: boolean + analysis_mode: AnalysisMode + finding_count: number + inspected_file_count: number + reason_code?: CheckExecutionReasonCode + prompt_hash?: string + implementations?: SubmissionCheckImplementation[] +} + +export interface AppDoctorSubmission { + // Envelope keys are camelCase because Core's Apps::Management::SourceScans::Envelope reads them verbatim. + schemaVersion: typeof SUBMISSION_SCHEMA_VERSION + report: AppDoctorSubmissionReport +} + +export interface AppDoctorSubmissionReport { + trace_schema_version: TraceV2['schema_version'] + engine: {name: string; version: string; ruleset: string} + cli_version: string + generated_at: string + submitted_at: string + feedback: string | null + // Always-applicable slots use null when unavailable (like project.commit); variant-dependent fields are omitted. + metadata: {version_tag: string | null; source_control_url: string | null} + project: {dirty: boolean | null; input_hash: string} + detection: { + framework: DetectedFramework + surface: DetectedSurface + languages: {name: string; support: LanguageSupport; file_count: number}[] + } + findings: SubmissionFinding[] + checks_executed: SubmissionCheck[] + suppressions: { + id: string + finding_fingerprint: string + justification: string + provenance: {source: SuppressionProvenance['source']; created_at: string} + }[] + coverage: { + files_scanned: number + complete: boolean + files_skipped: {too_large: number; unreadable: number} + gaps: {code: TraceV2['coverage']['gaps'][number]['code']; check_id?: string}[] + } + attestation: {trace_digest: string} +} + +function submissionFinding(finding: TraceFinding): SubmissionFinding { + const common = { + fingerprint: finding.fingerprint, + source: finding.source, + severity: finding.severity, + title: redactText(finding.title), + suppressed: finding.suppressed, + ...(finding.suppression === undefined ? {} : {suppression_id: finding.suppression.id}), + } + + switch (finding.source) { + case 'agent': + return { + ...common, + ...(finding.check_id === undefined ? {} : {check_id: finding.check_id}), + ...(finding.check_version === undefined ? {} : {check_version: finding.check_version}), + ...(finding.prompt_hash === undefined ? {} : {prompt_hash: finding.prompt_hash}), + } + case 'deterministic': + case 'external': + return { + ...common, + ...(finding.rule_id === undefined ? {} : {rule_id: finding.rule_id}), + ...(finding.rule_version === undefined ? {} : {rule_version: finding.rule_version}), + } + } +} + +function submissionImplementation( + implementation: NonNullable[number], +): SubmissionCheckImplementation { + return { + id: implementation.id, + analysis_mode: implementation.analysis_mode, + status: implementation.status, + finding_count: implementation.findings, + inspected_file_count: implementation.inspected_files.length, + ...(implementation.reason === undefined ? {} : {reason_code: implementation.reason.code}), + } +} + +function submissionCheck(check: CheckExecution): SubmissionCheck { + return { + id: check.id, + version: check.version, + kind: check.kind, + status: check.status, + required: check.required, + applicable: check.applicable, + analysis_mode: check.analysis_mode, + finding_count: check.findings, + inspected_file_count: check.inspected_files.length, + ...(check.reason === undefined ? {} : {reason_code: check.reason.code}), + ...(check.prompt_hash === undefined ? {} : {prompt_hash: check.prompt_hash}), + ...(check.implementations === undefined + ? {} + : {implementations: check.implementations.map(submissionImplementation)}), + } +} + +function skippedFileCounts(trace: TraceV2): {too_large: number; unreadable: number} { + return trace.coverage.files_skipped.reduce( + (counts, file) => + file.reason === 'too_large' + ? {...counts, too_large: counts.too_large + 1} + : {...counts, unreadable: counts.unreadable + 1}, + {too_large: 0, unreadable: 0}, + ) +} + +export function buildSubmission(trace: TraceV2, options: BuildSubmissionOptions): AppDoctorSubmission { + return { + schemaVersion: SUBMISSION_SCHEMA_VERSION, + report: { + trace_schema_version: trace.schema_version, + engine: { + name: redactText(trace.engine.name), + version: redactText(trace.engine.version), + ruleset: redactText(trace.engine.ruleset), + }, + cli_version: options.cliVersion, + generated_at: trace.generated_at, + submitted_at: options.submittedAt, + // Feedback intentionally bypasses redactText; callers are responsible for the accompanying disclosure. + feedback: options.feedback ?? null, + metadata: { + version_tag: options.versionTag === undefined ? null : redactText(options.versionTag), + source_control_url: options.sourceControlUrl === undefined ? null : redactText(options.sourceControlUrl), + }, + project: { + dirty: trace.project.dirty, + input_hash: trace.project.input_hash, + }, + detection: { + framework: trace.detection.framework, + surface: trace.detection.surface, + languages: trace.detection.languages.map((language) => ({ + name: language.name, + support: language.support, + file_count: language.files.length, + })), + }, + findings: trace.findings.map(submissionFinding), + checks_executed: trace.checks_executed.map(submissionCheck), + suppressions: trace.suppressions.map((suppression) => ({ + id: suppression.id, + finding_fingerprint: suppression.finding_fingerprint, + justification: redactText(suppression.justification), + provenance: { + source: suppression.provenance.source, + created_at: suppression.provenance.created_at, + }, + })), + coverage: { + files_scanned: trace.coverage.files_scanned, + complete: trace.coverage.complete, + files_skipped: skippedFileCounts(trace), + gaps: trace.coverage.gaps.map((gap) => ({ + code: gap.code, + ...(gap.check_id === undefined ? {} : {check_id: gap.check_id}), + })), + }, + attestation: {trace_digest: trace.attestation.digest}, + }, + } +} diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-dry-run-result.json b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-dry-run-result.json new file mode 100644 index 00000000000..0bca12d688a --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-dry-run-result.json @@ -0,0 +1,8 @@ +{ + "operation": "submit", + "dry_run": true, + "payload": { + "path": "/.shopify/app-doctor/submission.json", + "schema_version": 1 + } +} diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-result.json b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-result.json new file mode 100644 index 00000000000..1dfa9a4115d --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/doctor-submit-result.json @@ -0,0 +1,9 @@ +{ + "operation": "submit", + "dry_run": false, + "payload": { + "path": "/.shopify/app-doctor/submission.json", + "schema_version": 1 + }, + "submitted_at": "2026-09-01T09:30:00.000Z" +} diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-forbidden-values.json b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-forbidden-values.json new file mode 100644 index 00000000000..175419b85fd --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-forbidden-values.json @@ -0,0 +1,35 @@ +[ + "LEAK_COMMIT_SHA_0123456789abcdef", + "web/app/routes/private.ts", + "web/package.json", + "extensions/private.liquid", + "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "LEAK_MESSAGE_DEPENDENCY_CHAIN", + "LEAK_EVIDENCE_QUOTE", + "LEAK_CODE_SNIPPET", + "LEAK_FIX_DESCRIPTION", + "LEAK_ACTOR@example.com", + "LEAK_AGENT_MESSAGE", + "LEAK_AGENT_EVIDENCE", + "LEAK_AGENT_SNIPPET", + "LEAK_AGENT_FIX", + "LEAK_EXTERNAL_MESSAGE", + "LEAK_EXTERNAL_FIX", + "LEAK_DETERMINISTIC_PROMPT", + "LEAK_DETERMINISTIC_GUIDANCE", + "LEAK_AGENT_PROMPT", + "LEAK_AGENT_GUIDANCE", + "LEAK_REASON_MESSAGE", + "LEAK_UNRESOLVED_REASON", + "LEAK_UNRESOLVED_PROMPT", + "LEAK_UNRESOLVED_GUIDANCE", + "LEAK_SKIPPED_DETAIL", + "LEAK_GAP_MESSAGE", + "LEAK_UNRESOLVED_GAP", + "LEAK_FUTURE_FINDING", + "LEAK_FUTURE_CHECK", + "LEAK_FUTURE_ROOT", + "private/too-large.js", + "private/unreadable.js", + "private/unreadable-two.js" +] diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-trace.ts b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-trace.ts new file mode 100644 index 00000000000..df578935ce8 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission-trace.ts @@ -0,0 +1,247 @@ +import {findingFingerprint, sha256} from '../../trace/index.js' +import type {TraceFinding, TraceV2} from '../../types.js' + +const inputHash = `sha256:${'a'.repeat(64)}` +const privateFileHash = `sha256:${'b'.repeat(64)}` +// Valid-shape placeholders make the object easy to declare; every integrity +// value below is overwritten from the current helper before export. +const deterministicFingerprint = `sha256:${'1'.repeat(64)}` +const agentFingerprint = `sha256:${'2'.repeat(64)}` +const externalFingerprint = `sha256:${'3'.repeat(64)}` +const agentPromptHash = `sha256:${'4'.repeat(64)}` +const unresolvedPromptHash = `sha256:${'5'.repeat(64)}` + +const suppression = { + id: 'accepted-migration-risk', + finding_fingerprint: deterministicFingerprint, + justification: 'Accepted until migration finishes', + provenance: { + source: 'human' as const, + actor: 'LEAK_ACTOR@example.com', + created_at: '2026-08-31T11:00:00.000Z', + }, +} + +const traceWithLeakageSentinels = { + schema_version: 2, + engine: { + name: 'shopify-app-doctor', + version: '0.1.0', + ruleset: 'app-doctor-rules@0.1.0', + }, + generated_at: '2026-08-31T10:00:00.000Z', + project: { + commit: 'LEAK_COMMIT_SHA_0123456789abcdef', + dirty: true, + input_hash: inputHash, + input_hashes: {'web/app/routes/private.ts': privateFileHash}, + }, + detection: { + framework: 'react_router', + surface: 'mixed', + languages: [ + {name: 'typescript', support: 'supported', files: ['web/app/routes/private.ts', 'web/package.json']}, + {name: 'liquid', support: 'supported', files: ['extensions/private.liquid']}, + ], + }, + // Coverage is incomplete and a required check is unresolved, so validateTrace requires null. + score: null, + findings: [ + { + fingerprint: deterministicFingerprint, + source: 'deterministic', + rule_id: 'KNOWN_CVE_IN_DEPENDENCY', + rule_version: 2, + severity: 'high', + title: 'Vulnerable package lodash (CVE-2026-0001)', + message: 'LEAK_MESSAGE_DEPENDENCY_CHAIN', + location: {file: 'web/package.json', line: 12}, + evidence: [{location: {file: 'web/package.json', line: 12}, quote: 'LEAK_EVIDENCE_QUOTE'}], + snippet: 'LEAK_CODE_SNIPPET', + fix: {automated: false, guide: 'https://example.com/private-fix', description: 'LEAK_FIX_DESCRIPTION'}, + suppressed: true, + suppression, + }, + { + fingerprint: agentFingerprint, + source: 'agent', + check_id: 'MISSING_AUTHORIZATION_CHECK', + check_version: 3, + prompt_hash: agentPromptHash, + severity: 'medium', + title: 'Authorization check is missing', + message: 'LEAK_AGENT_MESSAGE', + location: {file: 'web/app/routes/private.ts', line: 27, column: 3}, + evidence: [{location: {file: 'web/app/routes/private.ts', line: 27}, quote: 'LEAK_AGENT_EVIDENCE'}], + snippet: 'LEAK_AGENT_SNIPPET', + fix: {automated: false, description: 'LEAK_AGENT_FIX'}, + suppressed: false, + future_finding_field: 'LEAK_FUTURE_FINDING', + }, + { + fingerprint: externalFingerprint, + source: 'external', + rule_id: 'EXTERNAL_SAST_001', + rule_version: 1, + severity: 'low', + title: 'External scanner finding', + message: 'LEAK_EXTERNAL_MESSAGE', + location: {file: 'extensions/private.liquid', line: 4}, + evidence: [], + fix: {automated: false, description: 'LEAK_EXTERNAL_FIX'}, + suppressed: false, + }, + ], + checks_executed: [ + { + id: 'KNOWN_CVE_IN_DEPENDENCY', + version: 2, + kind: 'deterministic', + status: 'executed', + required: true, + applicable: true, + languages: ['typescript'], + framework: 'react_router', + surface: 'mixed', + inspected_files: ['web/package.json'], + findings: 1, + analysis_mode: 'audit', + prompt: 'LEAK_DETERMINISTIC_PROMPT', + guidance: 'LEAK_DETERMINISTIC_GUIDANCE', + implementations: [ + { + id: 'npm-audit', + analysis_mode: 'audit', + status: 'executed', + inspected_files: ['web/package.json'], + findings: 1, + }, + ], + future_check_field: 'LEAK_FUTURE_CHECK', + }, + { + id: 'MISSING_AUTHORIZATION_CHECK', + version: 3, + kind: 'agent', + status: 'executed', + required: false, + applicable: true, + languages: ['typescript'], + framework: 'react_router', + surface: 'mixed', + inspected_files: ['web/app/routes/private.ts'], + findings: 1, + analysis_mode: 'agent', + prompt: 'LEAK_AGENT_PROMPT', + prompt_hash: agentPromptHash, + guidance: 'LEAK_AGENT_GUIDANCE', + }, + { + id: 'EXTERNAL_SAST_001', + version: 1, + kind: 'external', + status: 'executed', + required: false, + applicable: true, + languages: ['liquid'], + framework: 'react_router', + surface: 'mixed', + inspected_files: [], + findings: 1, + analysis_mode: 'external', + }, + { + id: 'NO_RELEVANT_CHECK', + version: 1, + kind: 'deterministic', + status: 'not_applicable', + required: false, + applicable: false, + languages: ['typescript'], + framework: 'react_router', + surface: 'mixed', + inspected_files: [], + findings: 0, + analysis_mode: 'regex', + reason: {code: 'no_relevant_files', message: 'LEAK_REASON_MESSAGE'}, + }, + { + id: 'UNREPORTED_AGENT_CHECK', + version: 1, + kind: 'agent', + status: 'unresolved', + required: true, + applicable: true, + languages: ['typescript'], + framework: 'react_router', + surface: 'mixed', + inspected_files: [], + findings: 0, + analysis_mode: 'agent', + reason: {code: 'not_reported', message: 'LEAK_UNRESOLVED_REASON'}, + prompt: 'LEAK_UNRESOLVED_PROMPT', + prompt_hash: unresolvedPromptHash, + guidance: 'LEAK_UNRESOLVED_GUIDANCE', + }, + ], + suppressions: [suppression], + coverage: { + files_scanned: 3, + files_skipped: [ + {path: 'private/too-large.js', reason: 'too_large', size_bytes: 6_000_000}, + {path: 'private/unreadable.js', reason: 'unreadable', detail: 'LEAK_SKIPPED_DETAIL'}, + {path: 'private/unreadable-two.js', reason: 'unreadable'}, + ], + complete: false, + gaps: [ + {code: 'skipped_file', message: 'LEAK_GAP_MESSAGE', file: 'private/unreadable.js'}, + {code: 'unresolved_check', check_id: 'UNREPORTED_AGENT_CHECK', message: 'LEAK_UNRESOLVED_GAP'}, + ], + }, + future_root_field: 'LEAK_FUTURE_ROOT', +} + +function computedFindingFingerprint(finding: TraceFinding): string { + return findingFingerprint({ + source: finding.source, + ...(finding.source === 'agent' + ? { + check_id: finding.check_id!, + check_version: finding.check_version!, + prompt_hash: finding.prompt_hash!, + } + : {rule_id: finding.rule_id!, rule_version: finding.rule_version!}), + severity: finding.severity, + title: finding.title, + message: finding.message, + location: finding.location, + evidence: finding.evidence, + ...(finding.snippet === undefined ? {} : {snippet: finding.snippet}), + fix: finding.fix, + }) +} + +// Compute every integrity field from the final semantic inputs. Unknown-field +// sentinels are already present before the unsigned trace digest is calculated. +const unsignedTrace = traceWithLeakageSentinels as unknown as Omit +const agentCheck = unsignedTrace.checks_executed[1]! +const unresolvedCheck = unsignedTrace.checks_executed[4]! +agentCheck.prompt_hash = sha256(agentCheck.prompt!) +unresolvedCheck.prompt_hash = sha256(unresolvedCheck.prompt!) +unsignedTrace.findings[1]!.prompt_hash = agentCheck.prompt_hash +for (const finding of unsignedTrace.findings) finding.fingerprint = computedFindingFingerprint(finding) +suppression.finding_fingerprint = unsignedTrace.findings[0]!.fingerprint + +export const submissionTraceHashes = { + deterministicFingerprint: unsignedTrace.findings[0]!.fingerprint, + agentFingerprint: unsignedTrace.findings[1]!.fingerprint, + externalFingerprint: unsignedTrace.findings[2]!.fingerprint, + agentPromptHash: agentCheck.prompt_hash, + unresolvedPromptHash: unresolvedCheck.prompt_hash, + traceDigest: sha256(unsignedTrace), +} as const + +export const submissionTraceFixture = { + ...unsignedTrace, + attestation: {digest: submissionTraceHashes.traceDigest, signed: false}, +} as TraceV2 diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission.json b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission.json new file mode 100644 index 00000000000..f1470f36c1d --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/fixtures/submission.json @@ -0,0 +1,149 @@ +{ + "schemaVersion": 1, + "report": { + "trace_schema_version": 2, + "engine": { + "name": "shopify-app-doctor", + "version": "0.1.0", + "ruleset": "app-doctor-rules@0.1.0" + }, + "cli_version": "3.99.0", + "generated_at": "2026-08-31T10:00:00.000Z", + "submitted_at": "2026-09-01T09:30:00.000Z", + "feedback": "The result for /Users/example/app included AKIA1234567890ABCDEF inaccurately.", + "metadata": { + "version_tag": "v1.2.3", + "source_control_url": "https://github.com/example/app/tree/v1.2.3" + }, + "project": { + "dirty": true, + "input_hash": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + }, + "detection": { + "framework": "react_router", + "surface": "mixed", + "languages": [ + {"name": "typescript", "support": "supported", "file_count": 2}, + {"name": "liquid", "support": "supported", "file_count": 1} + ] + }, + "findings": [ + { + "fingerprint": "sha256:298c67d67fe725a4636e1650ef5ff8149dac7d911d647b566073b3ff7a7614aa", + "source": "deterministic", + "severity": "high", + "title": "Vulnerable package lodash (CVE-2026-0001)", + "rule_id": "KNOWN_CVE_IN_DEPENDENCY", + "rule_version": 2, + "suppressed": true, + "suppression_id": "accepted-migration-risk" + }, + { + "fingerprint": "sha256:65ff76089b245ca615248ca0c1802cf6cccc2815b24c1359cacb3b1d679ad59d", + "source": "agent", + "severity": "medium", + "title": "Authorization check is missing", + "check_id": "MISSING_AUTHORIZATION_CHECK", + "check_version": 3, + "prompt_hash": "sha256:45da65952a69e7ed52d574a142fa37194222224fed232541a0387b494f9f174a", + "suppressed": false + }, + { + "fingerprint": "sha256:e199e53672bb3ec8a7b819c3588b7514d89d2eb02f16284594578b01497c91e8", + "source": "external", + "severity": "low", + "title": "External scanner finding", + "rule_id": "EXTERNAL_SAST_001", + "rule_version": 1, + "suppressed": false + } + ], + "checks_executed": [ + { + "id": "KNOWN_CVE_IN_DEPENDENCY", + "version": 2, + "kind": "deterministic", + "status": "executed", + "required": true, + "applicable": true, + "analysis_mode": "audit", + "finding_count": 1, + "inspected_file_count": 1, + "implementations": [ + { + "id": "npm-audit", + "analysis_mode": "audit", + "status": "executed", + "finding_count": 1, + "inspected_file_count": 1 + } + ] + }, + { + "id": "MISSING_AUTHORIZATION_CHECK", + "version": 3, + "kind": "agent", + "status": "executed", + "required": false, + "applicable": true, + "analysis_mode": "agent", + "finding_count": 1, + "inspected_file_count": 1, + "prompt_hash": "sha256:45da65952a69e7ed52d574a142fa37194222224fed232541a0387b494f9f174a" + }, + { + "id": "EXTERNAL_SAST_001", + "version": 1, + "kind": "external", + "status": "executed", + "required": false, + "applicable": true, + "analysis_mode": "external", + "finding_count": 1, + "inspected_file_count": 0 + }, + { + "id": "NO_RELEVANT_CHECK", + "version": 1, + "kind": "deterministic", + "status": "not_applicable", + "required": false, + "applicable": false, + "analysis_mode": "regex", + "finding_count": 0, + "inspected_file_count": 0, + "reason_code": "no_relevant_files" + }, + { + "id": "UNREPORTED_AGENT_CHECK", + "version": 1, + "kind": "agent", + "status": "unresolved", + "required": true, + "applicable": true, + "analysis_mode": "agent", + "finding_count": 0, + "inspected_file_count": 0, + "reason_code": "not_reported", + "prompt_hash": "sha256:9ebb0f49910f170703a64d75b0d3e50c0031dd71c2f40cfaa70088de88a91c18" + } + ], + "suppressions": [ + { + "id": "accepted-migration-risk", + "finding_fingerprint": "sha256:298c67d67fe725a4636e1650ef5ff8149dac7d911d647b566073b3ff7a7614aa", + "justification": "Accepted until migration finishes", + "provenance": {"source": "human", "created_at": "2026-08-31T11:00:00.000Z"} + } + ], + "coverage": { + "files_scanned": 3, + "complete": false, + "files_skipped": {"too_large": 1, "unreadable": 2}, + "gaps": [{"code": "skipped_file"}, {"code": "unresolved_check", "check_id": "UNREPORTED_AGENT_CHECK"}] + }, + "attestation": { + "trace_digest": "sha256:d21dc8ca875f71ef87c386edcef6f6b97df81719425c7a398d430d32034064cc" + } + } +} diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/submission.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/submission.test.ts new file mode 100644 index 00000000000..9f0fd3022b3 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/tests/submission.test.ts @@ -0,0 +1,98 @@ +import {submissionTraceFixture, submissionTraceHashes} from './fixtures/submission-trace.js' +import {buildSubmission} from '../submission/index.js' +import {validateTrace} from '../trace/index.js' +import {readFile} from '@shopify/cli-kit/node/fs' +import {joinPath, moduleDirectory} from '@shopify/cli-kit/node/path' +import {describe, expect, test} from 'vitest' +import type {AppDoctorSubmission} from '../submission/index.js' + +const fixturesDirectory = joinPath(moduleDirectory(import.meta.url), 'fixtures') + +async function jsonFixture(name: string): Promise { + return JSON.parse(await readFile(joinPath(fixturesDirectory, name))) as T +} + +const options = { + cliVersion: '3.99.0', + submittedAt: '2026-09-01T09:30:00.000Z', + versionTag: 'v1.2.3', + sourceControlUrl: 'https://github.com/example/app/tree/v1.2.3', + feedback: 'The result for /Users/example/app included AKIA1234567890ABCDEF inaccurately.', +} + +describe('buildSubmission', () => { + test('validates the source fixture before mapping and matches the independently pinned golden payload', async () => { + // This assertion must run before buildSubmission so a malformed fixture cannot + // make mapper expectations look correct. The golden file is hand-pinned and + // must never be generated by buildSubmission. + expect(validateTrace(structuredClone(submissionTraceFixture))).toEqual({valid: true, errors: []}) + const expected = await jsonFixture('submission.json') + + expect(expected.report.findings.map(({fingerprint}) => fingerprint)).toEqual([ + submissionTraceHashes.deterministicFingerprint, + submissionTraceHashes.agentFingerprint, + submissionTraceHashes.externalFingerprint, + ]) + expect(expected.report.findings[1]!.prompt_hash).toBe(submissionTraceHashes.agentPromptHash) + expect(expected.report.checks_executed[1]!.prompt_hash).toBe(submissionTraceHashes.agentPromptHash) + expect(expected.report.checks_executed[4]!.prompt_hash).toBe(submissionTraceHashes.unresolvedPromptHash) + expect(expected.report.attestation.trace_digest).toBe(submissionTraceHashes.traceDigest) + expect(buildSubmission(structuredClone(submissionTraceFixture), options)).toEqual(expected) + }) + + test('emits null metadata when no version tag or source control URL is supplied', () => { + const submission = buildSubmission(structuredClone(submissionTraceFixture), { + cliVersion: '3.99.0', + submittedAt: '2026-09-01T09:30:00.000Z', + }) + + expect(submission.report.metadata).toEqual({version_tag: null, source_control_url: null}) + expect(submission.report.feedback).toBeNull() + }) + + test('does not serialize the fixture’s excluded structural fields or unknown sentinels', async () => { + const forbiddenValues = await jsonFixture('submission-forbidden-values.json') + const serialized = JSON.stringify(buildSubmission(structuredClone(submissionTraceFixture), options)) + + for (const forbiddenValue of forbiddenValues) expect(serialized).not.toContain(forbiddenValue) + }) + + test('applies a second redaction pass to every free-text output field', () => { + const secret = 'AKIA1234567890ABCDEF' + const trace = structuredClone(submissionTraceFixture) + const mutableEngine = trace.engine as unknown as Record + mutableEngine.name = `engine-${secret}` + mutableEngine.version = `version-${secret}` + mutableEngine.ruleset = `ruleset-${secret}` + trace.findings[0]!.title = `CVE detected: ${secret}` + trace.suppressions[0]!.justification = `Approved with ${secret}` + + const submission = buildSubmission(trace, { + cliVersion: '3.99.0', + submittedAt: '2026-09-01T09:30:00.000Z', + versionTag: `version-${secret}`, + sourceControlUrl: `https://example.com/${secret}`, + }) + const serialized = JSON.stringify(submission) + + expect(serialized).not.toContain(secret) + expect(submission.report.findings[0]!.title).toContain('[REDACTED:20]') + expect(submission.report.suppressions[0]!.justification).toContain('[REDACTED:20]') + expect(submission.report.metadata.version_tag).toContain('[REDACTED:20]') + expect(submission.report.metadata.source_control_url).toContain('[REDACTED:20]') + }) + + test('preserves feedback exactly without applying the free-text redactor', () => { + const feedback = 'The result for /Users/example/app included AKIA1234567890ABCDEF inaccurately.' + + const submission = buildSubmission(structuredClone(submissionTraceFixture), {...options, feedback}) + + expect(submission.report.feedback).toBe(feedback) + }) + + test('keeps the public package and CVE identifiers in a known-CVE title', () => { + const submission = buildSubmission(structuredClone(submissionTraceFixture), options) + + expect(submission.report.findings[0]!.title).toBe('Vulnerable package lodash (CVE-2026-0001)') + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-submit-api.test.ts b/packages/app/src/cli/services/app-doctor-submit-api.test.ts new file mode 100644 index 00000000000..8490b450035 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-submit-api.test.ts @@ -0,0 +1,127 @@ +import {submitAppDoctorScan} from './app-doctor-submit-api.js' +import {testDeveloperPlatformClient} from '../models/app/app.test-data.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {describe, expect, test, vi} from 'vitest' +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {SourceScanCreateSchema, SourceScanUploadUrlSchema} from '../utilities/developer-platform-client.js' + +const app = { + apiKey: 'api-key', + organizationId: '123', + id: 'gid://shopify/App/1', +} + +const submission = {schemaVersion: 1, report: {}} as AppDoctorSubmission + +function dependencies(upload = vi.fn(async () => {})) { + return {fileSize: vi.fn(async () => 1234), upload} +} + +function options() { + const generateSourceScanUploadUrl = vi.fn( + async (): Promise => ({ + sourceScanUploadUrl: 'source-scan-upload-url', + userErrors: [], + }), + ) + const createSourceScan = vi.fn(async (): Promise => ({accepted: true, userErrors: []})) + return { + input: { + app, + submission, + submissionPath: '/tmp/app/.shopify/app-doctor/submission.json', + // testDeveloperPlatformClient defaults are plain functions, not spies. + // Always inject explicit vi.fn stubs before making call/mocking assertions. + developerPlatformClient: testDeveloperPlatformClient({generateSourceScanUploadUrl, createSourceScan}), + }, + generateSourceScanUploadUrl, + createSourceScan, + } +} + +describe('submitAppDoctorScan', () => { + test('preserves multiple upload-URL user errors in server order and does not upload', async () => { + const {input, generateSourceScanUploadUrl, createSourceScan} = options() + const upload = vi.fn() + generateSourceScanUploadUrl.mockResolvedValue({ + sourceScanUploadUrl: 'unused-upload-url', + userErrors: [{message: 'First upload error'}, {message: 'Second upload error'}], + }) + + await expect(submitAppDoctorScan(input, dependencies(upload))).rejects.toThrow( + new AbortError('First upload error, Second upload error'), + ) + expect(upload).not.toHaveBeenCalled() + expect(createSourceScan).not.toHaveBeenCalled() + }) + + test('uses the missing-URL fallback and neither uploads nor creates a source scan', async () => { + const {input, generateSourceScanUploadUrl, createSourceScan} = options() + const upload = vi.fn() + generateSourceScanUploadUrl.mockResolvedValue({sourceScanUploadUrl: null, userErrors: []}) + + await expect(submitAppDoctorScan(input, dependencies(upload))).rejects.toThrow( + new AbortError('Shopify did not return a source scan upload URL.'), + ) + expect(upload).not.toHaveBeenCalled() + expect(createSourceScan).not.toHaveBeenCalled() + }) + + test('propagates PUT failures and does not create a source scan', async () => { + const {input, createSourceScan} = options() + const uploadError = new AbortError('Storage failed') + const upload = vi.fn(async () => { + throw uploadError + }) + + await expect(submitAppDoctorScan(input, dependencies(upload))).rejects.toBe(uploadError) + expect(createSourceScan).not.toHaveBeenCalled() + }) + + test('preserves multiple create user errors in server order', async () => { + const {input, createSourceScan} = options() + createSourceScan.mockResolvedValue({ + accepted: false, + userErrors: [{message: 'First create error'}, {message: 'Second create error'}], + }) + + await expect(submitAppDoctorScan(input, dependencies())).rejects.toThrow( + new AbortError('First create error, Second create error'), + ) + }) + + test('throws with a retry suggestion when Shopify does not accept the submission', async () => { + const {input, createSourceScan} = options() + createSourceScan.mockResolvedValue({accepted: false, userErrors: []}) + + const result = submitAppDoctorScan(input, dependencies()) + + await expect(result).rejects.toThrow( + new AbortError( + 'Shopify did not accept the App Doctor submission.', + 'Try submitting the App Doctor results again.', + ), + ) + await expect(result).rejects.toMatchObject({tryMessage: 'Try submitting the App Doctor results again.'}) + }) + + test('requests an upload URL with the artifact byte size before uploading', async () => { + const {input, generateSourceScanUploadUrl, createSourceScan} = options() + const fileSize = vi.fn(async () => 1234) + const upload = vi.fn(async () => {}) + + await expect(submitAppDoctorScan(input, {fileSize, upload})).resolves.toBeUndefined() + + expect(fileSize).toHaveBeenCalledWith('/tmp/app/.shopify/app-doctor/submission.json') + expect(generateSourceScanUploadUrl).toHaveBeenCalledWith({appId: app.id, byteSize: 1234}) + expect(generateSourceScanUploadUrl.mock.invocationCallOrder[0]).toBeLessThan(upload.mock.invocationCallOrder[0]!) + expect(upload).toHaveBeenCalledWith('source-scan-upload-url', '/tmp/app/.shopify/app-doctor/submission.json', { + artifactName: 'App Doctor submission', + contentType: 'application/json', + }) + expect(createSourceScan).toHaveBeenCalledWith({ + appId: app.id, + sourceScanUrl: 'source-scan-upload-url', + }) + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-submit-api.ts b/packages/app/src/cli/services/app-doctor-submit-api.ts new file mode 100644 index 00000000000..3a8e3a6bb4d --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-submit-api.ts @@ -0,0 +1,57 @@ +import {uploadToGCS} from './bundle.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {fileSize} from '@shopify/cli-kit/node/fs' +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {MinimalAppIdentifiers} from '../models/organization.js' +import type {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' + +export interface SubmitAppDoctorScanOptions { + app: MinimalAppIdentifiers + submission: AppDoctorSubmission + submissionPath: string + developerPlatformClient: DeveloperPlatformClient +} + +interface SubmitAppDoctorScanDependencies { + fileSize: typeof fileSize + upload: typeof uploadToGCS +} + +const defaultDependencies: SubmitAppDoctorScanDependencies = {fileSize, upload: uploadToGCS} + +function userErrorMessage(userErrors: {message: string}[], fallback: string): string { + return userErrors.map(({message}) => message).join(', ') || fallback +} + +export async function submitAppDoctorScan( + options: SubmitAppDoctorScanOptions, + dependencies: SubmitAppDoctorScanDependencies = defaultDependencies, +): Promise { + const byteSize = await dependencies.fileSize(options.submissionPath) + const uploadResult = await options.developerPlatformClient.generateSourceScanUploadUrl({ + appId: options.app.id, + byteSize, + }) + if (!uploadResult.sourceScanUploadUrl || uploadResult.userErrors.length > 0) { + throw new AbortError(userErrorMessage(uploadResult.userErrors, 'Shopify did not return a source scan upload URL.')) + } + + await dependencies.upload(uploadResult.sourceScanUploadUrl, options.submissionPath, { + artifactName: 'App Doctor submission', + contentType: 'application/json', + }) + + const createResult = await options.developerPlatformClient.createSourceScan({ + appId: options.app.id, + sourceScanUrl: uploadResult.sourceScanUploadUrl, + }) + if (createResult.userErrors.length > 0) { + throw new AbortError(userErrorMessage(createResult.userErrors, 'Shopify could not create the App Doctor scan.')) + } + if (!createResult.accepted) { + throw new AbortError( + 'Shopify did not accept the App Doctor submission.', + 'Try submitting the App Doctor results again.', + ) + } +} diff --git a/packages/app/src/cli/services/bundle.test.ts b/packages/app/src/cli/services/bundle.test.ts index b6ac856d765..102c84d5a42 100644 --- a/packages/app/src/cli/services/bundle.test.ts +++ b/packages/app/src/cli/services/bundle.test.ts @@ -228,6 +228,26 @@ describe('uploadToGCS', () => { expect.objectContaining({method: 'put'}), 'slow-request', ) + expect(vi.mocked(fetch).mock.calls[0]![1]).not.toHaveProperty('headers') + }) + }) + + test('sends the content type when the signed URL requires it', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const submissionPath = joinPath(tmpDir, 'submission.json') + await writeFile(submissionPath, '{}') + vi.mocked(fetch).mockResolvedValue({ok: true, status: 200} as never) + + await uploadToGCS('https://signed.example/upload', submissionPath, {contentType: 'application/json'}) + + expect(fetch).toHaveBeenCalledWith( + 'https://signed.example/upload', + expect.objectContaining({ + method: 'put', + headers: {'Content-Type': 'application/json'}, + }), + 'slow-request', + ) }) }) @@ -302,4 +322,32 @@ describe('uploadToGCS', () => { expect(fetch).not.toHaveBeenCalled() }) }) + + test('uses a custom artifact label in storage failure copy', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const artifactPath = joinPath(tmpDir, 'submission.json') + await writeFile(artifactPath, '{}') + vi.mocked(fetch).mockResolvedValue({ + ok: false, + status: 403, + text: () => Promise.resolve('forbidden'), + } as never) + + await expect( + uploadToGCS('https://signed.example/upload', artifactPath, {artifactName: 'App Doctor submission'}), + ).rejects.toThrow('Failed to upload your App Doctor submission to storage (HTTP 403).') + }) + }) + + test('uses a custom artifact label in size-limit copy', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const artifactPath = joinPath(tmpDir, 'submission.json') + await writeFile(artifactPath, '{}') + vi.mocked(fileSize).mockResolvedValueOnce(101 * 1024 * 1024) + + await expect( + uploadToGCS('https://signed.example/upload', artifactPath, {artifactName: 'App Doctor submission'}), + ).rejects.toThrow('Your App Doctor submission exceeds the 100 MB upload limit') + }) + }) }) diff --git a/packages/app/src/cli/services/bundle.ts b/packages/app/src/cli/services/bundle.ts index ec0f0b9d20b..aed4e350553 100644 --- a/packages/app/src/cli/services/bundle.ts +++ b/packages/app/src/cli/services/bundle.ts @@ -35,6 +35,11 @@ export async function compressBundle(inputDirectory: string, outputPath: string, } } +interface UploadToGCSOptions { + artifactName?: string + contentType?: string +} + /** * Upload a file to GCS using a signed URL. * @@ -48,14 +53,19 @@ export async function compressBundle(inputDirectory: string, outputPath: string, * * @param signedURL - The signed URL to upload the file to * @param filePath - The path to the file + * @param options - Optional settings; `artifactName` labels the uploaded artifact in error copy (defaults to `app bundle`), and `contentType` sends a signed Content-Type header. */ -export async function uploadToGCS(signedURL: string, filePath: string) { +export async function uploadToGCS( + signedURL: string, + filePath: string, + {artifactName = 'app bundle', contentType}: UploadToGCSOptions = {}, +) { const size = await fileSize(filePath) if (size > MAX_BUNDLE_SIZE_BYTES) { // Round up so a size that barely exceeds the cap never displays as the cap. const humanSize = `${(Math.ceil((size / MEGABYTE) * 100) / 100).toFixed(2)} MB` throw new AbortError( - `Your app bundle exceeds the ${MAX_BUNDLE_SIZE_MB} MB upload limit (it is ${humanSize}).`, + `Your ${artifactName} exceeds the ${MAX_BUNDLE_SIZE_MB} MB upload limit (it is ${humanSize}).`, `Check the asset paths in your extension configuration — a misconfigured source can pull in much more than intended. Exclude large files or directories from your bundle, then try again.`, ) } @@ -64,10 +74,19 @@ export async function uploadToGCS(signedURL: string, filePath: string) { let response: Response | undefined for (let attempt = 1; attempt <= UPLOAD_MAX_ATTEMPTS; attempt++) { - // The signed URL only signs the `host` header, so no extra headers are - // required; node-fetch derives Content-Length from the buffer body. + // Most signed URLs only bind the `host` header, but some (including App + // Doctor source scans) are also bound to a Content-Type and must send it. + // node-fetch derives Content-Length from the buffer body. // eslint-disable-next-line no-await-in-loop - response = await fetch(signedURL, {method: 'put', body: buffer}, 'slow-request') + response = await fetch( + signedURL, + { + method: 'put', + body: buffer, + ...(contentType === undefined ? {} : {headers: {'Content-Type': contentType}}), + }, + 'slow-request', + ) if (response.ok) return const lastAttempt = attempt === UPLOAD_MAX_ATTEMPTS const retryable = RETRYABLE_UPLOAD_STATUS_CODES.has(response.status) @@ -87,7 +106,7 @@ export async function uploadToGCS(signedURL: string, filePath: string) { const status = response?.status const responseBody = (await response?.text().catch(() => ''))?.trim() throw new AbortError( - `Failed to upload your app bundle to storage${status ? ` (HTTP ${status})` : ''}.`, + `Failed to upload your ${artifactName} to storage${status ? ` (HTTP ${status})` : ''}.`, 'This is usually transient. Please try again, and check your network connection if it persists.', responseBody ? [`Storage responded with: ${responseBody.slice(0, 300)}`] : undefined, ) diff --git a/packages/app/src/cli/services/doctor-submit-output.test.ts b/packages/app/src/cli/services/doctor-submit-output.test.ts new file mode 100644 index 00000000000..2949bd16ab5 --- /dev/null +++ b/packages/app/src/cli/services/doctor-submit-output.test.ts @@ -0,0 +1,120 @@ +import { + renderDoctorSubmitConfirmation, + renderDoctorSubmitDryRun, + renderDoctorSubmitFeedbackPrompt, + renderDoctorSubmitSuccess, +} from './doctor-submit-output.js' +import {submissionTraceFixture} from './app-doctor-engine/tests/fixtures/submission-trace.js' +import {buildSubmission} from './app-doctor-engine/submission/index.js' +import {renderInfo, renderSelectPrompt, renderSuccess, renderTextPrompt, renderWarning} from '@shopify/cli-kit/node/ui' +import {describe, expect, test, vi} from 'vitest' + +vi.mock('@shopify/cli-kit/node/ui') + +const submission = buildSubmission(submissionTraceFixture, { + cliVersion: '3.99.0', + submittedAt: '2026-09-01T09:30:00.000Z', +}) +const submissionPath = '/tmp/app/.shopify/app-doctor/submission.json' + +describe('renderDoctorSubmitFeedbackPrompt', () => { + test('warns immediately before prompting and configures optional validated input', async () => { + vi.mocked(renderTextPrompt).mockResolvedValue(' entered feedback ') + + await expect(renderDoctorSubmitFeedbackPrompt({maxLength: 2000})).resolves.toBe(' entered feedback ') + + expect(renderWarning).toHaveBeenCalledWith({ + headline: "Don't include source code, file paths or secrets in your optional feedback.", + }) + expect(vi.mocked(renderWarning).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(renderTextPrompt).mock.invocationCallOrder[0]!, + ) + expect(renderTextPrompt).toHaveBeenCalledWith({ + message: 'Optional: What was inaccurate or unhelpful about these App Doctor results?', + allowEmpty: true, + emptyDisplayedValue: '(skipped)', + validate: expect.any(Function), + }) + + const [{validate}] = vi.mocked(renderTextPrompt).mock.calls[0]! + expect(validate?.('')).toBeUndefined() + expect(validate?.(` ${'a'.repeat(2000)} `)).toBeUndefined() + expect(validate?.('a'.repeat(2001))).toBe('Feedback must be 2,000 characters or fewer.') + }) +}) + +describe('renderDoctorSubmitConfirmation', () => { + test('offers an action to add optional feedback when none was supplied', async () => { + vi.mocked(renderSelectPrompt).mockResolvedValue('submit-with-feedback') + + await expect( + renderDoctorSubmitConfirmation({appTitle: 'Example app', submissionPath, submission, canAddFeedback: true}), + ).resolves.toBe('submit-with-feedback') + + expect(renderSelectPrompt).toHaveBeenCalledWith({ + message: 'Submit App Doctor results for Example app to Shopify?', + choices: [ + {label: 'Yes, submit', value: 'submit', key: 'y'}, + {label: 'Add optional feedback, then submit', value: 'submit-with-feedback', key: 'f'}, + {label: 'No, cancel', value: 'cancel', key: 'n'}, + ], + defaultValue: 'submit', + isConfirmationPrompt: true, + infoTable: { + Findings: ['1 high · 1 medium · 1 low (1 suppressed)'], + Checks: ['3 executed · 1 not applicable · 1 unresolved'], + Excluded: ['file paths, code snippets, evidence, finding messages, commit SHA'], + Payload: [{filePath: submissionPath}], + Warning: [{warn: 'The trace was generated with uncommitted changes.'}], + }, + }) + }) + + test('discloses supplied feedback and does not offer to collect it again', async () => { + vi.mocked(renderSelectPrompt).mockResolvedValue('submit') + const submissionWithFeedback = buildSubmission(submissionTraceFixture, { + cliVersion: '3.99.0', + submittedAt: '2026-09-01T09:30:00.000Z', + feedback: 'Something was inaccurate.', + }) + + await renderDoctorSubmitConfirmation({ + appTitle: 'Example app', + submissionPath, + submission: submissionWithFeedback, + canAddFeedback: false, + }) + + expect(renderSelectPrompt).toHaveBeenCalledWith( + expect.objectContaining({ + choices: [ + {label: 'Yes, submit', value: 'submit', key: 'y'}, + {label: 'No, cancel', value: 'cancel', key: 'n'}, + ], + infoTable: expect.objectContaining({Included: ['Optional feedback, sent without redaction']}), + }), + ) + }) +}) + +describe('renderDoctorSubmitDryRun', () => { + test('states that nothing was uploaded and points to the payload', () => { + renderDoctorSubmitDryRun({submissionPath}) + + expect(renderInfo).toHaveBeenCalledWith({ + headline: 'Prepared the App Doctor submission without uploading it.', + body: ['Payload: ', {filePath: submissionPath}], + }) + }) +}) + +describe('renderDoctorSubmitSuccess', () => { + test('includes the app and payload path', () => { + renderDoctorSubmitSuccess({appTitle: 'Example app', submissionPath}) + + expect(renderSuccess).toHaveBeenCalledWith({ + headline: 'Submitted App Doctor results for Example app.', + body: ['Payload: ', {filePath: submissionPath}], + }) + }) +}) diff --git a/packages/app/src/cli/services/doctor-submit-output.ts b/packages/app/src/cli/services/doctor-submit-output.ts new file mode 100644 index 00000000000..fa1ed8e2eb2 --- /dev/null +++ b/packages/app/src/cli/services/doctor-submit-output.ts @@ -0,0 +1,98 @@ +import {renderInfo, renderSelectPrompt, renderSuccess, renderTextPrompt, renderWarning} from '@shopify/cli-kit/node/ui' +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' + +export type DoctorSubmitConfirmationAction = 'submit' | 'submit-with-feedback' | 'cancel' + +export interface DoctorSubmitConfirmationInput { + appTitle: string + submissionPath: string + submission: AppDoctorSubmission + canAddFeedback: boolean +} + +export interface DoctorSubmitFeedbackPromptInput { + maxLength: number +} + +export interface DoctorSubmitDryRunInput { + submissionPath: string +} + +export interface DoctorSubmitSuccessInput { + appTitle: string + submissionPath: string +} + +function findingsSummary(submission: AppDoctorSubmission): string { + const count = (severity: 'high' | 'medium' | 'low') => + submission.report.findings.filter((finding) => finding.severity === severity).length + const suppressed = submission.report.findings.filter((finding) => finding.suppressed).length + return `${count('high')} high · ${count('medium')} medium · ${count('low')} low${ + suppressed === 0 ? '' : ` (${suppressed} suppressed)` + }` +} + +function checksSummary(submission: AppDoctorSubmission): string { + const executed = submission.report.checks_executed.filter((check) => check.status === 'executed').length + const notApplicable = submission.report.checks_executed.filter((check) => check.status === 'not_applicable').length + const unresolved = submission.report.checks_executed.filter( + (check) => check.status === 'unresolved' || check.status === 'unsupported_framework', + ).length + return `${executed} executed · ${notApplicable} not applicable · ${unresolved} unresolved` +} + +export async function renderDoctorSubmitFeedbackPrompt({maxLength}: DoctorSubmitFeedbackPromptInput): Promise { + renderWarning({headline: "Don't include source code, file paths or secrets in your optional feedback."}) + return renderTextPrompt({ + message: 'Optional: What was inaccurate or unhelpful about these App Doctor results?', + allowEmpty: true, + emptyDisplayedValue: '(skipped)', + validate: (value) => + value.trim().length <= maxLength + ? undefined + : `Feedback must be ${new Intl.NumberFormat('en-US').format(maxLength)} characters or fewer.`, + }) +} + +export function renderDoctorSubmitConfirmation( + input: DoctorSubmitConfirmationInput, +): Promise { + const choices: {label: string; value: DoctorSubmitConfirmationAction; key: string}[] = [ + {label: 'Yes, submit', value: 'submit', key: 'y'}, + ...(input.canAddFeedback + ? [{label: 'Add optional feedback, then submit', value: 'submit-with-feedback' as const, key: 'f'}] + : []), + {label: 'No, cancel', value: 'cancel', key: 'n'}, + ] + + return renderSelectPrompt({ + message: `Submit App Doctor results for ${input.appTitle} to Shopify?`, + choices, + defaultValue: 'submit', + isConfirmationPrompt: true, + infoTable: { + Findings: [findingsSummary(input.submission)], + Checks: [checksSummary(input.submission)], + ...(input.submission.report.feedback === null ? {} : {Included: ['Optional feedback, sent without redaction']}), + Excluded: ['file paths, code snippets, evidence, finding messages, commit SHA'], + Payload: [{filePath: input.submissionPath}], + ...(input.submission.report.project.dirty === true + ? {Warning: [{warn: 'The trace was generated with uncommitted changes.'}]} + : {}), + }, + }) +} + +export function renderDoctorSubmitDryRun({submissionPath}: DoctorSubmitDryRunInput): void { + renderInfo({ + headline: 'Prepared the App Doctor submission without uploading it.', + body: ['Payload: ', {filePath: submissionPath}], + }) +} + +export function renderDoctorSubmitSuccess({appTitle, submissionPath}: DoctorSubmitSuccessInput): void { + renderSuccess({ + headline: `Submitted App Doctor results for ${appTitle}.`, + body: ['Payload: ', {filePath: submissionPath}], + }) +} diff --git a/packages/app/src/cli/services/doctor-submit.test.ts b/packages/app/src/cli/services/doctor-submit.test.ts new file mode 100644 index 00000000000..b4a3e5e845d --- /dev/null +++ b/packages/app/src/cli/services/doctor-submit.test.ts @@ -0,0 +1,513 @@ +import doctorSubmit, {MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH} from './doctor-submit.js' +import {appDoctorArtifactPaths, writeSubmission} from './app-doctor-artifacts.js' +import {buildSubmission} from './app-doctor-engine/submission/index.js' +import {submissionTraceFixture} from './app-doctor-engine/tests/fixtures/submission-trace.js' +import {testDeveloperPlatformClient} from '../models/app/app.test-data.js' +import {inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {joinPath, moduleDirectory} from '@shopify/cli-kit/node/path' +import {AbortError} from '@shopify/cli-kit/node/error' +import {describe, expect, test, vi} from 'vitest' +import type {DoctorSubmitDependencies, DoctorSubmitOptions} from './doctor-submit.js' +import type {ReadTraceResult} from './app-doctor-artifacts.js' + +const submittedAt = '2026-09-01T09:30:00.000Z' + +function options(directory: string): DoctorSubmitOptions { + return { + directory, + json: false, + force: false, + dryRun: false, + clientId: undefined, + configName: undefined, + versionTag: undefined, + sourceControlUrl: undefined, + feedback: undefined, + } +} + +function testDependencies(directory: string): DoctorSubmitDependencies { + return { + findRoot: vi.fn(() => directory), + artifactPaths: appDoctorArtifactPaths, + readTrace: vi.fn( + async (): Promise => ({ + status: 'ok', + trace: structuredClone(submissionTraceFixture), + }), + ), + linkApp: vi.fn(async () => ({ + remoteApp: { + apiKey: 'api-key', + organizationId: '123', + id: 'gid://shopify/App/1', + title: 'Example app', + }, + developerPlatformClient: testDeveloperPlatformClient(), + })), + buildSubmission: vi.fn(buildSubmission), + writeSubmission: vi.fn(writeSubmission), + canPrompt: vi.fn(() => false), + readStdin: vi.fn(async () => undefined), + promptForFeedback: vi.fn(async () => ''), + confirm: vi.fn(async () => 'submit' as const), + submitScan: vi.fn(async () => {}), + renderDryRun: vi.fn(), + renderSuccess: vi.fn(), + output: vi.fn(), + now: vi.fn(() => submittedAt), + cliVersion: '3.99.0', + } +} + +async function jsonFixture(name: string): Promise { + const directory = joinPath(moduleDirectory(import.meta.url), 'app-doctor-engine', 'tests', 'fixtures') + return JSON.parse(await readFile(joinPath(directory, name))) as T +} + +async function capturedAbort(run: Promise): Promise { + try { + await run + throw new Error('Expected doctorSubmit to throw') + // This helper intentionally catches the command's unknown rejection to assert its public AbortError fields. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + expect(error).toBeInstanceOf(AbortError) + return error as AbortError + } +} + +function expectNoOutput(dependencies: DoctorSubmitDependencies): void { + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.confirm).not.toHaveBeenCalled() + expect(dependencies.renderDryRun).not.toHaveBeenCalled() + expect(dependencies.renderSuccess).not.toHaveBeenCalled() + expect(dependencies.output).not.toHaveBeenCalled() +} + +describe('doctorSubmit', () => { + test('fails before linking when the trace is missing', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.readTrace).mockResolvedValue({status: 'missing'}) + + const error = await capturedAbort(doctorSubmit(options(directory), dependencies)) + + expect(error.message).toContain( + `No App Doctor trace found in ${appDoctorArtifactPaths(directory).artifactDirectory}.`, + ) + expect(error.nextSteps).toEqual([`Run \`shopify app doctor --path ${directory}\` first, then submit.`]) + expect(dependencies.linkApp).not.toHaveBeenCalled() + expectNoOutput(dependencies) + }) + }) + + test('preserves invalid trace errors as separate next steps', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.readTrace).mockResolvedValue({ + status: 'invalid', + errors: ['schema error one', 'schema error two'], + }) + + const error = await capturedAbort(doctorSubmit(options(directory), dependencies)) + + expect(error.message).toContain('is not valid') + expect(error.nextSteps).toEqual(['schema error one', 'schema error two']) + expect(dependencies.linkApp).not.toHaveBeenCalled() + expectNoOutput(dependencies) + }) + }) + + test('collects selected feedback, rewrites the payload, and uploads the rewritten submission', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockResolvedValue('submit-with-feedback') + vi.mocked(dependencies.promptForFeedback).mockResolvedValue(' The authorization finding was inaccurate. ') + + await doctorSubmit(options(directory), dependencies) + + expect(dependencies.confirm).toHaveBeenCalledWith( + expect.objectContaining({ + canAddFeedback: true, + submission: expect.objectContaining({report: expect.objectContaining({feedback: null})}), + }), + ) + expect(dependencies.promptForFeedback).toHaveBeenCalledWith({maxLength: MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH}) + expect(dependencies.buildSubmission).toHaveBeenCalledTimes(2) + expect(dependencies.buildSubmission).toHaveBeenLastCalledWith( + expect.anything(), + expect.objectContaining({feedback: 'The authorization finding was inaccurate.'}), + ) + expect(dependencies.writeSubmission).toHaveBeenCalledTimes(2) + + const rewrittenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[1]![1] + expect(rewrittenSubmission.report.feedback).toBe('The authorization finding was inaccurate.') + expect(vi.mocked(dependencies.submitScan).mock.calls[0]![0].submission).toBe(rewrittenSubmission) + expect(vi.mocked(dependencies.confirm).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(dependencies.promptForFeedback).mock.invocationCallOrder[0]!, + ) + expect(vi.mocked(dependencies.promptForFeedback).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(dependencies.writeSubmission).mock.invocationCallOrder[1]!, + ) + expect(vi.mocked(dependencies.writeSubmission).mock.invocationCallOrder[1]).toBeLessThan( + vi.mocked(dependencies.submitScan).mock.invocationCallOrder[0]!, + ) + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain( + '"feedback": "The authorization finding was inaccurate."', + ) + }) + }) + + test.each(['', ' \n '])('normalizes selected empty feedback %j to null', async (feedback) => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockResolvedValue('submit-with-feedback') + vi.mocked(dependencies.promptForFeedback).mockResolvedValue(feedback) + + await doctorSubmit(options(directory), dependencies) + + expect(dependencies.buildSubmission).toHaveBeenLastCalledWith( + expect.anything(), + expect.objectContaining({feedback: undefined}), + ) + expect(dependencies.submitScan).toHaveBeenCalledWith( + expect.objectContaining({ + submission: expect.objectContaining({report: expect.objectContaining({feedback: null})}), + }), + ) + }) + }) + + test('trims explicit feedback and bypasses the prompt', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), force: true, feedback: ' Direct feedback '}, dependencies) + + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.readStdin).not.toHaveBeenCalled() + expect(dependencies.buildSubmission).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({feedback: 'Direct feedback'}), + ) + }) + }) + + test('reads and normalizes multiline feedback from piped stdin', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.readStdin).mockResolvedValue(' First line\nsecond line \n') + + await doctorSubmit({...options(directory), force: true, feedback: '-'}, dependencies) + + expect(dependencies.readStdin).toHaveBeenCalledOnce() + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.buildSubmission).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({feedback: 'First line\nsecond line'}), + ) + }) + }) + + test('errors actionably when --feedback - has no piped stdin', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + const error = await capturedAbort(doctorSubmit({...options(directory), force: true, feedback: '-'}, dependencies)) + + expect(error.message).toContain('No piped stdin was provided for --feedback -') + expect(error.nextSteps).toEqual(['Pipe feedback to the command or pass it directly with --feedback .']) + expect(dependencies.buildSubmission).not.toHaveBeenCalled() + expect(dependencies.linkApp).not.toHaveBeenCalled() + }) + }) + + test('accepts feedback at exactly the maximum length', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + const feedback = 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH) + + await doctorSubmit({...options(directory), force: true, feedback}, dependencies) + + expect(dependencies.buildSubmission).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({feedback})) + }) + }) + + test.each([ + {source: 'flag', feedback: 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH + 1), stdin: undefined}, + {source: 'stdin', feedback: '-', stdin: 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH + 1)}, + ])('rejects overlong $source feedback instead of truncating it', async ({feedback, stdin}) => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.readStdin).mockResolvedValue(stdin) + + const error = await capturedAbort(doctorSubmit({...options(directory), force: true, feedback}, dependencies)) + + expect(error.message).toContain('Feedback must be 2,000 characters or fewer.') + expect(dependencies.buildSubmission).not.toHaveBeenCalled() + expect(dependencies.writeSubmission).not.toHaveBeenCalled() + }) + }) + + test.each([ + {name: '--dry-run', overrides: {dryRun: true}}, + {name: '--json', overrides: {json: true, force: true}}, + {name: '--force', overrides: {force: true}}, + ])('does not prompt for feedback with $name', async ({overrides}) => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + + await doctorSubmit({...options(directory), ...overrides}, dependencies) + + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.buildSubmission).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({feedback: undefined}), + ) + }) + }) + + test('dry run writes explicit feedback into the exact payload without prompting', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), dryRun: true, feedback: ' Dry-run feedback '}, dependencies) + + const writtenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + expect(writtenSubmission.report.feedback).toBe('Dry-run feedback') + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain( + '"feedback": "Dry-run feedback"', + ) + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + }) + }) + + test('submits without collecting feedback when the submit action is selected', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + + await doctorSubmit(options(directory), dependencies) + + expect(dependencies.confirm).toHaveBeenCalledWith(expect.objectContaining({canAddFeedback: true})) + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.writeSubmission).toHaveBeenCalledOnce() + expect(dependencies.submitScan).toHaveBeenCalledOnce() + }) + }) + + test('uploads the same explicitly supplied feedback-bearing object that was written and confirmed', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + + await doctorSubmit({...options(directory), feedback: 'Submitted feedback'}, dependencies) + + const writtenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + expect(dependencies.confirm).toHaveBeenCalledWith(expect.objectContaining({canAddFeedback: false})) + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(vi.mocked(dependencies.confirm).mock.calls[0]![0].submission).toBe(writtenSubmission) + expect(vi.mocked(dependencies.submitScan).mock.calls[0]![0].submission).toBe(writtenSubmission) + expect(writtenSubmission.report.feedback).toBe('Submitted feedback') + }) + }) + + test('dry run writes the payload without linking or authenticating', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), dryRun: true}, dependencies) + + const payloadPath = appDoctorArtifactPaths(directory).submissionPath + await expect(readFile(payloadPath)).resolves.toContain('"schemaVersion": 1') + expect(dependencies.buildSubmission).toHaveBeenCalledOnce() + expect(dependencies.writeSubmission).toHaveBeenCalledOnce() + expect(dependencies.linkApp).not.toHaveBeenCalled() + expect(dependencies.submitScan).not.toHaveBeenCalled() + expect(dependencies.confirm).not.toHaveBeenCalled() + expect(dependencies.renderDryRun).toHaveBeenCalledWith({submissionPath: payloadPath}) + }) + }) + + test('--json --dry-run does not require --force and emits exactly the dry-run golden', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), json: true, dryRun: true}, dependencies) + + expect(dependencies.buildSubmission).toHaveBeenCalledOnce() + expect(dependencies.writeSubmission).toHaveBeenCalledOnce() + expect(dependencies.linkApp).not.toHaveBeenCalled() + expect(dependencies.canPrompt).not.toHaveBeenCalled() + expect(dependencies.confirm).not.toHaveBeenCalled() + expect(dependencies.submitScan).not.toHaveBeenCalled() + expect(dependencies.renderDryRun).not.toHaveBeenCalled() + expect(dependencies.renderSuccess).not.toHaveBeenCalled() + expect(dependencies.output).toHaveBeenCalledOnce() + const actual = JSON.parse(vi.mocked(dependencies.output).mock.calls[0]![0]) as { + payload: {path: string} + submitted_at?: string + } + actual.payload.path = actual.payload.path.replace(directory, '') + expect(actual).toEqual(await jsonFixture('doctor-submit-dry-run-result.json')) + expect(actual).not.toHaveProperty('submitted_at') + }) + }) + + test('leaves the written artifact and uploads nothing when confirmation is declined', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockResolvedValue('cancel') + + await doctorSubmit(options(directory), dependencies) + + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain('"schemaVersion": 1') + expect(dependencies.submitScan).not.toHaveBeenCalled() + }) + }) + + test('--force skips prompting even when prompting is unavailable', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), force: true}, dependencies) + + expect(dependencies.linkApp).toHaveBeenCalledWith({ + directory, + clientId: undefined, + forceRelink: false, + userProvidedConfigName: undefined, + skipPrompts: false, + }) + expect(dependencies.canPrompt).not.toHaveBeenCalled() + expect(dependencies.confirm).not.toHaveBeenCalled() + expect(dependencies.submitScan).toHaveBeenCalledOnce() + expect(dependencies.renderSuccess).toHaveBeenCalledWith({ + appTitle: 'Example app', + submissionPath: appDoctorArtifactPaths(directory).submissionPath, + }) + }) + }) + + test('links only after the payload is written', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit({...options(directory), force: true}, dependencies) + + const writeCallOrder = vi.mocked(dependencies.writeSubmission).mock.invocationCallOrder[0]! + const linkCallOrder = vi.mocked(dependencies.linkApp).mock.invocationCallOrder[0]! + expect(writeCallOrder).toBeLessThan(linkCallOrder) + }) + }) + + test('--json without --force writes the artifact and then requires force', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + const error = await capturedAbort(doctorSubmit({...options(directory), json: true}, dependencies)) + + expect(error.message).toBe('Pass --force to submit without confirmation.') + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain('"schemaVersion": 1') + expect(dependencies.submitScan).not.toHaveBeenCalled() + expectNoOutput(dependencies) + }) + }) + + test('non-TTY submission without --force requires force', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + const error = await capturedAbort(doctorSubmit(options(directory), dependencies)) + + expect(error.message).toBe('Pass --force to submit without confirmation.') + expect(dependencies.submitScan).not.toHaveBeenCalled() + expectNoOutput(dependencies) + }) + }) + + test('does not emit or render output when scan submission fails', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + const failure = new AbortError('Submission failed') + vi.mocked(dependencies.submitScan).mockRejectedValue(failure) + + await expect(doctorSubmit({...options(directory), force: true}, dependencies)).rejects.toBe(failure) + + expectNoOutput(dependencies) + }) + }) + + test('--json --force emits only the tagged golden result and forwards metadata', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + + await doctorSubmit( + { + ...options(directory), + json: true, + force: true, + versionTag: 'v1.2.3', + sourceControlUrl: 'https://github.com/example/app/tree/v1.2.3', + clientId: 'client-id', + }, + dependencies, + ) + + expect(dependencies.linkApp).toHaveBeenCalledWith({ + directory, + clientId: 'client-id', + forceRelink: false, + userProvidedConfigName: undefined, + skipPrompts: true, + }) + expect(dependencies.submitScan).toHaveBeenCalledWith( + expect.objectContaining({ + submission: expect.objectContaining({ + report: expect.objectContaining({ + metadata: { + version_tag: 'v1.2.3', + source_control_url: 'https://github.com/example/app/tree/v1.2.3', + }, + }), + }), + }), + ) + expect(dependencies.renderSuccess).not.toHaveBeenCalled() + expect(dependencies.renderDryRun).not.toHaveBeenCalled() + expect(dependencies.output).toHaveBeenCalledOnce() + + const actual = JSON.parse(vi.mocked(dependencies.output).mock.calls[0]![0]) as { + payload: {path: string} + } + actual.payload.path = actual.payload.path.replace(directory, '') + expect(actual).toEqual(await jsonFixture('doctor-submit-result.json')) + expect(actual).not.toHaveProperty('app') + expect(actual).toHaveProperty('submitted_at', submittedAt) + }) + }) + + test('passes dirty state to the human confirmation renderer', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + + await doctorSubmit(options(directory), dependencies) + + expect(dependencies.confirm).toHaveBeenCalledWith( + expect.objectContaining({ + appTitle: 'Example app', + canAddFeedback: true, + submission: expect.objectContaining({ + report: expect.objectContaining({project: expect.objectContaining({dirty: true})}), + }), + }), + ) + }) + }) +}) diff --git a/packages/app/src/cli/services/doctor-submit.ts b/packages/app/src/cli/services/doctor-submit.ts new file mode 100644 index 00000000000..e8a4a7259ea --- /dev/null +++ b/packages/app/src/cli/services/doctor-submit.ts @@ -0,0 +1,228 @@ +import {linkedAppContext} from './app-context.js' +import {appDoctorArtifactPaths, readTrace, writeSubmission} from './app-doctor-artifacts.js' +import {findAppRoot} from './app-doctor-engine/scanners/discover.js' +import {buildSubmission, SUBMISSION_SCHEMA_VERSION} from './app-doctor-engine/submission/index.js' +import {submitAppDoctorScan} from './app-doctor-submit-api.js' +import { + renderDoctorSubmitConfirmation, + renderDoctorSubmitDryRun, + renderDoctorSubmitFeedbackPrompt, + renderDoctorSubmitSuccess, +} from './doctor-submit-output.js' +import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version' +import {AbortError} from '@shopify/cli-kit/node/error' +import {outputResult} from '@shopify/cli-kit/node/output' +import {readStdinString, terminalSupportsPrompting} from '@shopify/cli-kit/node/system' +import type {ReadTraceResult, ResolvedAppDoctorArtifactPaths} from './app-doctor-artifacts.js' +import type {AppDoctorSubmission, BuildSubmissionOptions} from './app-doctor-engine/submission/index.js' +import type {TraceV2} from './app-doctor-engine/types.js' +import type {SubmitAppDoctorScanOptions} from './app-doctor-submit-api.js' +import type { + DoctorSubmitConfirmationAction, + DoctorSubmitConfirmationInput, + DoctorSubmitDryRunInput, + DoctorSubmitFeedbackPromptInput, + DoctorSubmitSuccessInput, +} from './doctor-submit-output.js' +import type {MinimalAppIdentifiers} from '../models/organization.js' +import type {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' + +export const MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH = 2000 + +export interface DoctorSubmitOptions { + directory: string + json: boolean + force: boolean + dryRun: boolean + clientId?: string + configName?: string + versionTag?: string + sourceControlUrl?: string + feedback?: string +} + +interface DoctorSubmitApp extends MinimalAppIdentifiers { + title: string +} + +interface DoctorSubmitAppContext { + remoteApp: DoctorSubmitApp + developerPlatformClient: DeveloperPlatformClient +} + +type DoctorSubmitJsonResult = + | {operation: 'submit'; dry_run: true; payload: DoctorSubmitJsonPayload} + | {operation: 'submit'; dry_run: false; payload: DoctorSubmitJsonPayload; submitted_at: string} + +interface DoctorSubmitJsonPayload { + path: string + schema_version: typeof SUBMISSION_SCHEMA_VERSION +} + +export interface DoctorSubmitDependencies { + findRoot(directory: string): string + artifactPaths(appRoot: string): ResolvedAppDoctorArtifactPaths + readTrace(path: string): Promise + linkApp(options: { + directory: string + clientId: string | undefined + forceRelink: boolean + userProvidedConfigName: string | undefined + skipPrompts: boolean + }): Promise + buildSubmission(trace: TraceV2, options: BuildSubmissionOptions): AppDoctorSubmission + writeSubmission(appRoot: string, payload: AppDoctorSubmission): Promise + canPrompt(): boolean + readStdin(): Promise + promptForFeedback(input: DoctorSubmitFeedbackPromptInput): Promise + confirm(input: DoctorSubmitConfirmationInput): Promise + submitScan(options: SubmitAppDoctorScanOptions): Promise + renderDryRun(input: DoctorSubmitDryRunInput): void + renderSuccess(input: DoctorSubmitSuccessInput): void + output(content: string): void + now(): string + cliVersion: string +} + +const defaultDependencies: DoctorSubmitDependencies = { + findRoot: findAppRoot, + artifactPaths: appDoctorArtifactPaths, + readTrace, + linkApp: linkedAppContext, + buildSubmission, + writeSubmission, + canPrompt: terminalSupportsPrompting, + readStdin: readStdinString, + promptForFeedback: renderDoctorSubmitFeedbackPrompt, + confirm: renderDoctorSubmitConfirmation, + submitScan: submitAppDoctorScan, + renderDryRun: renderDoctorSubmitDryRun, + renderSuccess: renderDoctorSubmitSuccess, + output: outputResult, + now: () => new Date().toISOString(), + cliVersion: CLI_KIT_VERSION, +} + +function dryRunJsonResult(submissionPath: string, submission: AppDoctorSubmission): DoctorSubmitJsonResult { + return { + operation: 'submit', + dry_run: true, + payload: {path: submissionPath, schema_version: submission.schemaVersion}, + } +} + +function submitJsonResult(submissionPath: string, submission: AppDoctorSubmission): DoctorSubmitJsonResult { + return { + operation: 'submit', + dry_run: false, + payload: {path: submissionPath, schema_version: submission.schemaVersion}, + submitted_at: submission.report.submitted_at, + } +} + +function normalizeFeedback(feedback: string): string | undefined { + const normalizedFeedback = feedback.trim() + if (normalizedFeedback.length > MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH) { + throw new AbortError('Feedback must be 2,000 characters or fewer.') + } + return normalizedFeedback === '' ? undefined : normalizedFeedback +} + +async function resolveExplicitFeedback( + options: DoctorSubmitOptions, + dependencies: DoctorSubmitDependencies, +): Promise { + if (options.feedback === undefined) return undefined + + const feedback = options.feedback === '-' ? await dependencies.readStdin() : options.feedback + if (feedback === undefined) { + throw new AbortError('No piped stdin was provided for --feedback -.', null, [ + 'Pipe feedback to the command or pass it directly with --feedback .', + ]) + } + return normalizeFeedback(feedback) +} + +export default async function doctorSubmit( + options: DoctorSubmitOptions, + dependencies: DoctorSubmitDependencies = defaultDependencies, +): Promise { + const appRoot = dependencies.findRoot(options.directory) + const paths = dependencies.artifactPaths(appRoot) + const traceResult = await dependencies.readTrace(paths.tracePath) + + if (traceResult.status === 'missing') { + throw new AbortError(`No App Doctor trace found in ${paths.artifactDirectory}.`, null, [ + `Run \`shopify app doctor --path ${options.directory}\` first, then submit.`, + ]) + } + if (traceResult.status === 'invalid') { + throw new AbortError(`The App Doctor trace at ${paths.tracePath} is not valid.`, null, traceResult.errors) + } + + const submittedAt = dependencies.now() + const buildSubmissionWithFeedback = (feedback: string | undefined) => + dependencies.buildSubmission(traceResult.trace, { + cliVersion: dependencies.cliVersion, + submittedAt, + versionTag: options.versionTag, + sourceControlUrl: options.sourceControlUrl, + feedback, + }) + + let feedback = await resolveExplicitFeedback(options, dependencies) + let submission = buildSubmissionWithFeedback(feedback) + await dependencies.writeSubmission(appRoot, submission) + + if (options.dryRun) { + if (options.json) { + dependencies.output(JSON.stringify(dryRunJsonResult(paths.submissionPath, submission), null, 2)) + } else { + dependencies.renderDryRun({submissionPath: paths.submissionPath}) + } + return + } + + const {remoteApp, developerPlatformClient} = await dependencies.linkApp({ + directory: appRoot, + clientId: options.clientId, + forceRelink: false, + userProvidedConfigName: options.configName, + skipPrompts: options.json, + }) + + if (!options.force) { + if (options.json || !dependencies.canPrompt()) { + throw new AbortError('Pass --force to submit without confirmation.') + } + const confirmationAction = await dependencies.confirm({ + appTitle: remoteApp.title, + submissionPath: paths.submissionPath, + submission, + canAddFeedback: options.feedback === undefined, + }) + if (confirmationAction === 'cancel') return + + if (confirmationAction === 'submit-with-feedback') { + feedback = normalizeFeedback(await dependencies.promptForFeedback({maxLength: MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH})) + submission = buildSubmissionWithFeedback(feedback) + await dependencies.writeSubmission(appRoot, submission) + } + } + + await dependencies.submitScan({ + app: remoteApp, + submission, + submissionPath: paths.submissionPath, + developerPlatformClient, + }) + + if (options.json) { + dependencies.output(JSON.stringify(submitJsonResult(paths.submissionPath, submission), null, 2)) + } else { + dependencies.renderSuccess({ + appTitle: remoteApp.title, + submissionPath: paths.submissionPath, + }) + } +} diff --git a/packages/app/src/cli/utilities/developer-platform-client.ts b/packages/app/src/cli/utilities/developer-platform-client.ts index 412499c1b8f..3e378c3d9c5 100644 --- a/packages/app/src/cli/utilities/developer-platform-client.ts +++ b/packages/app/src/cli/utilities/developer-platform-client.ts @@ -141,6 +141,24 @@ export type AssetUrlSchema = WithUserErrors<{ assetUrl?: string | null }> +export type SourceScanUploadUrlSchema = WithUserErrors<{ + sourceScanUploadUrl?: string | null +}> + +export interface SourceScanUploadUrlInput { + appId: string + byteSize: number +} + +export interface SourceScanCreateInput { + appId: string + sourceScanUrl: string +} + +export type SourceScanCreateSchema = WithUserErrors<{ + accepted: boolean +}> + export enum Flag {} const FlagMap: {[key: string]: Flag} = {} @@ -221,6 +239,8 @@ export interface DeveloperPlatformClient { appVersionByTag: (app: MinimalOrganizationApp, tag: string) => Promise appVersionsDiff: (app: MinimalOrganizationApp, version: AppVersionIdentifiers) => Promise generateSignedUploadUrl: (app: MinimalAppIdentifiers) => Promise + generateSourceScanUploadUrl: (input: SourceScanUploadUrlInput) => Promise + createSourceScan: (input: SourceScanCreateInput) => Promise deploy: (input: AppDeployOptions) => Promise release: (input: {app: MinimalOrganizationApp; version: AppVersionIdentifiers}) => Promise sendSampleWebhook: (input: SendSampleWebhookVariables, organizationId: string) => Promise diff --git a/packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts b/packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts index 16bd3985473..3eea71b1ac9 100644 --- a/packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts +++ b/packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts @@ -43,6 +43,8 @@ import {AppHomeSpecIdentifier} from '../../models/extensions/specifications/app_ import {AppAccessSpecIdentifier} from '../../models/extensions/specifications/app_config_app_access.js' import {MinimalAppIdentifiers} from '../../models/organization.js' import {CreateAssetUrl} from '../../api/graphql/app-management/generated/create-asset-url.js' +import {RequestSourceScanUploadUrl} from '../../api/graphql/app-management/generated/request-source-scan-upload-url.js' +import {CreateSourceScan} from '../../api/graphql/app-management/generated/create-source-scan.js' import {SourceExtension} from '../../api/graphql/app-management/generated/types.js' import {fetchOrganizations} from '@shopify/organizations' import {describe, expect, test, vi, beforeEach} from 'vitest' @@ -1416,6 +1418,61 @@ describe('AppManagementClient', () => { }) }) + describe('generateSourceScanUploadUrl', () => { + test('passes the app ID and byte size, does not cache, and maps the upload response', async () => { + const client = AppManagementClient.getInstance() + client.token = () => Promise.resolve('token') + vi.mocked(appManagementRequestDoc).mockResolvedValueOnce({ + appRequestSourceScanUploadUrl: { + sourceScanUploadUrl: 'https://example.com/source-scan-upload', + userErrors: [], + }, + }) + + const result = await client.generateSourceScanUploadUrl({ + appId: 'gid://shopify/App/1', + byteSize: 1234, + }) + + expect(result).toEqual({sourceScanUploadUrl: 'https://example.com/source-scan-upload', userErrors: []}) + expect(appManagementRequestDoc).toHaveBeenCalledWith( + expect.objectContaining({ + query: RequestSourceScanUploadUrl, + token: 'token', + variables: {appId: 'gid://shopify/App/1', byteSize: 1234}, + }), + ) + expect(vi.mocked(appManagementRequestDoc).mock.calls[0]![0]).not.toHaveProperty('cacheOptions') + }) + }) + + describe('createSourceScan', () => { + test('passes the app ID and source scan URL and maps the accepted result', async () => { + const client = AppManagementClient.getInstance() + client.token = () => Promise.resolve('token') + vi.mocked(appManagementRequestDoc).mockResolvedValueOnce({ + appSourceScanCreate: {accepted: true, userErrors: []}, + }) + + const result = await client.createSourceScan({ + appId: 'gid://shopify/App/1', + sourceScanUrl: 'https://example.com/source-scan-upload', + }) + + expect(result).toEqual({accepted: true, userErrors: []}) + expect(appManagementRequestDoc).toHaveBeenCalledWith( + expect.objectContaining({ + query: CreateSourceScan, + token: 'token', + variables: { + appId: 'gid://shopify/App/1', + sourceScanUrl: 'https://example.com/source-scan-upload', + }, + }), + ) + }) + }) + describe('bundleFormat', () => { test('returns br for Brotli compression format', () => { // Given diff --git a/packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts b/packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts index 225446fb75b..ec44f4ded5b 100644 --- a/packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts +++ b/packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts @@ -18,6 +18,10 @@ import { AppVersionWithContext, AppDeployOptions, AssetUrlSchema, + SourceScanCreateInput, + SourceScanCreateSchema, + SourceScanUploadUrlInput, + SourceScanUploadUrlSchema, AppVersionIdentifiers, filterDisabledFlags, ClientName, @@ -92,6 +96,14 @@ import { CreateAppVersionMutationVariables, } from '../../api/graphql/app-management/generated/create-app-version.js' import {CreateAssetUrl} from '../../api/graphql/app-management/generated/create-asset-url.js' +import { + RequestSourceScanUploadUrl, + RequestSourceScanUploadUrlMutationVariables, +} from '../../api/graphql/app-management/generated/request-source-scan-upload-url.js' +import { + CreateSourceScan, + CreateSourceScanMutationVariables, +} from '../../api/graphql/app-management/generated/create-source-scan.js' import {AppVersionById} from '../../api/graphql/app-management/generated/app-version-by-id.js' import {AppVersions} from '../../api/graphql/app-management/generated/app-versions.js' import {AppInstallCount} from '../../api/graphql/app-management/generated/app-install-count.js' @@ -741,6 +753,21 @@ export class AppManagementClient implements DeveloperPlatformClient { } } + async generateSourceScanUploadUrl({appId, byteSize}: SourceScanUploadUrlInput): Promise { + const variables: RequestSourceScanUploadUrlMutationVariables = {appId, byteSize} + const result = await this.appManagementRequest({ + query: RequestSourceScanUploadUrl, + variables, + }) + return result.appRequestSourceScanUploadUrl + } + + async createSourceScan({appId, sourceScanUrl}: SourceScanCreateInput): Promise { + const variables: CreateSourceScanMutationVariables = {appId, sourceScanUrl} + const result = await this.appManagementRequest({query: CreateSourceScan, variables}) + return result.appSourceScanCreate + } + async deploy({ appManifest, appId, diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 692da2c1faa..3221e93ca1e 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -1470,6 +1470,125 @@ "strict": true, "summary": "Provide App Doctor instructions to a coding agent." }, + "app:doctor:submit": { + "aliases": [ + ], + "args": { + }, + "customPluginName": "@shopify/app", + "description": "Reads the most recent App Doctor trace, writes a `.shopify/app-doctor/submission.json` file for inspection, asks for confirmation, and uploads the result to Shopify.\n\nGenerated report fields exclude source code, file paths, code snippets, evidence, finding messages, and commit identifiers. Optional feedback is included without redaction. Optional `--version` and `--source-control-url` metadata is included only when supplied. Use `--dry-run` to write and inspect the exact payload without uploading it.", + "descriptionWithMarkdown": "Reads the most recent App Doctor trace, writes a `.shopify/app-doctor/submission.json` file for inspection, asks for confirmation, and uploads the result to Shopify.\n\nGenerated report fields exclude source code, file paths, code snippets, evidence, finding messages, and commit identifiers. Optional feedback is included without redaction. Optional `--version` and `--source-control-url` metadata is included only when supplied. Use `--dry-run` to write and inspect the exact payload without uploading it.", + "enableJsonFlag": false, + "flags": { + "client-id": { + "description": "The Client ID of your app.", + "env": "SHOPIFY_FLAG_CLIENT_ID", + "exclusive": [ + "config" + ], + "hasDynamicHelp": false, + "hidden": false, + "multiple": false, + "name": "client-id", + "type": "option" + }, + "config": { + "char": "c", + "description": "The name of the app configuration.", + "env": "SHOPIFY_FLAG_APP_CONFIG", + "hasDynamicHelp": false, + "hidden": false, + "multiple": false, + "name": "config", + "type": "option" + }, + "dry-run": { + "allowNo": false, + "description": "Write the submission payload without uploading it.", + "env": "SHOPIFY_FLAG_APP_DOCTOR_DRY_RUN", + "name": "dry-run", + "type": "boolean" + }, + "feedback": { + "description": "Optional feedback about inaccurate or unhelpful App Doctor results. Use - to read from stdin.", + "env": "SHOPIFY_FLAG_APP_DOCTOR_FEEDBACK", + "hasDynamicHelp": false, + "multiple": false, + "name": "feedback", + "type": "option" + }, + "force": { + "allowNo": false, + "char": "f", + "description": "Skip confirmation. Required if non interactive.", + "env": "SHOPIFY_FLAG_FORCE", + "name": "force", + "type": "boolean" + }, + "json": { + "allowNo": false, + "char": "j", + "description": "Output the result as JSON. Automatically disables color output.", + "env": "SHOPIFY_FLAG_JSON", + "hidden": false, + "name": "json", + "type": "boolean" + }, + "no-color": { + "allowNo": false, + "description": "Disable color output.", + "env": "SHOPIFY_FLAG_NO_COLOR", + "hidden": false, + "name": "no-color", + "type": "boolean" + }, + "path": { + "description": "The path to your app directory.", + "env": "SHOPIFY_FLAG_PATH", + "hasDynamicHelp": false, + "multiple": false, + "name": "path", + "noCacheDefault": true, + "type": "option" + }, + "source-control-url": { + "description": "URL associated with the new app version.", + "env": "SHOPIFY_FLAG_SOURCE_CONTROL_URL", + "hasDynamicHelp": false, + "hidden": false, + "multiple": false, + "name": "source-control-url", + "type": "option" + }, + "verbose": { + "allowNo": false, + "description": "Increase the verbosity of the output. May include sensitive data.", + "env": "SHOPIFY_FLAG_VERBOSE", + "hidden": false, + "name": "verbose", + "type": "boolean" + }, + "version": { + "description": "Optional version tag that will be associated with this app version. If not provided, an auto-generated identifier will be generated for this app version.", + "env": "SHOPIFY_FLAG_VERSION", + "hasDynamicHelp": false, + "hidden": false, + "multiple": false, + "name": "version", + "type": "option" + } + }, + "hasDynamicHelp": false, + "hidden": true, + "hiddenAliases": [ + ], + "id": "app:doctor:submit", + "pluginAlias": "@shopify/cli", + "pluginName": "@shopify/cli", + "pluginType": "core", + "strict": true, + "summary": "Submit App Doctor results to Shopify." + }, "app:env:pull": { "aliases": [ ], From f14c3f0f0324b30975afec67c1eec762d4ffc22a Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 8 Sep 2026 13:40:46 -0700 Subject: [PATCH 2/2] Preserve App Doctor submission bytes through upload Prepare a typed submission payload and reuse its JSON bytes for artifact writes, upload sizing, and upload retries instead of rereading the file. Remove arbitrary length cap from feedback. Co-authored-by: AI Assisted-By: devx/f000c74b-02bd-420a-abaf-487fbbbd8b3e --- .../cli/services/app-doctor-artifacts.test.ts | 7 +- .../src/cli/services/app-doctor-artifacts.ts | 7 +- .../app-doctor-submission-payload.test.ts | 55 ++++++ .../services/app-doctor-submission-payload.ts | 18 ++ .../services/app-doctor-submit-api.test.ts | 26 +-- .../src/cli/services/app-doctor-submit-api.ts | 13 +- packages/app/src/cli/services/bundle.test.ts | 15 ++ packages/app/src/cli/services/bundle.ts | 10 +- .../cli/services/doctor-submit-output.test.ts | 15 +- .../src/cli/services/doctor-submit-output.ts | 10 +- .../src/cli/services/doctor-submit.test.ts | 161 +++++++++++++----- .../app/src/cli/services/doctor-submit.ts | 59 +++---- 12 files changed, 273 insertions(+), 123 deletions(-) create mode 100644 packages/app/src/cli/services/app-doctor-submission-payload.test.ts create mode 100644 packages/app/src/cli/services/app-doctor-submission-payload.ts diff --git a/packages/app/src/cli/services/app-doctor-artifacts.test.ts b/packages/app/src/cli/services/app-doctor-artifacts.test.ts index ade25845b0d..e8d1d595fe1 100644 --- a/packages/app/src/cli/services/app-doctor-artifacts.test.ts +++ b/packages/app/src/cli/services/app-doctor-artifacts.test.ts @@ -116,13 +116,14 @@ describe('readTrace', () => { }) describe('writeSubmission', () => { - test('creates parent directories and writes pretty JSON with a trailing newline', async () => { + test('creates parent directories and writes the provided bytes without re-encoding', async () => { await inTemporaryDirectory(async (directory) => { const path = joinPath(directory, '.shopify', 'app-doctor', 'submission.json') - await writeSubmission(directory, submission) + const bytes = Buffer.from(`${JSON.stringify(submission)}\n`, 'utf8') + await writeSubmission(directory, bytes) - await expect(readFile(path)).resolves.toBe(`${JSON.stringify(submission, null, 2)}\n`) + await expect(readFile(path)).resolves.toBe(bytes.toString()) }) }) }) diff --git a/packages/app/src/cli/services/app-doctor-artifacts.ts b/packages/app/src/cli/services/app-doctor-artifacts.ts index 1da53529be4..9dd3c885e95 100644 --- a/packages/app/src/cli/services/app-doctor-artifacts.ts +++ b/packages/app/src/cli/services/app-doctor-artifacts.ts @@ -5,7 +5,6 @@ import {joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' import {randomBytes} from 'node:crypto' import {lstat, mkdir, realpath, rename, unlink, writeFile} from 'node:fs/promises' import type {AppDoctorExecution} from './app-doctor-api.js' -import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' import type {TraceV2} from './app-doctor-engine/types.js' const MAX_TRACE_FILE_SIZE_BYTES = 5_000_000 @@ -100,7 +99,7 @@ function refuseArtifactPath(path: string): never { ) } -async function writeAtomicArtifact(path: string, contents: string): Promise { +async function writeAtomicArtifact(path: string, contents: string | Buffer): Promise { await assertNotSymbolicLink(path) const temporaryPath = `${path}.${randomBytes(8).toString('hex')}.tmp` await assertNotSymbolicLink(temporaryPath) @@ -161,8 +160,8 @@ export async function readTrace(path: string): Promise { return {status: 'ok', trace: parsed as TraceV2} } -export async function writeSubmission(appRoot: string, payload: AppDoctorSubmission): Promise { +export async function writeSubmission(appRoot: string, bytes: Buffer): Promise { const paths = appDoctorArtifactPaths(appRoot) await ensureArtifactDirectory(appRoot, paths.artifactDirectory) - await writeAtomicArtifact(paths.submissionPath, `${JSON.stringify(payload, null, 2)}\n`) + await writeAtomicArtifact(paths.submissionPath, bytes) } diff --git a/packages/app/src/cli/services/app-doctor-submission-payload.test.ts b/packages/app/src/cli/services/app-doctor-submission-payload.test.ts new file mode 100644 index 00000000000..563ce909147 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-submission-payload.test.ts @@ -0,0 +1,55 @@ +import {prepareSubmissionPayload} from './app-doctor-submission-payload.js' +import {buildSubmission} from './app-doctor-engine/submission/index.js' +import {submissionTraceFixture} from './app-doctor-engine/tests/fixtures/submission-trace.js' +import {describe, expect, test} from 'vitest' + +function submissionFixture() { + return buildSubmission(submissionTraceFixture, {cliVersion: 'test', submittedAt: '2026-09-08'}) +} + +describe('prepareSubmissionPayload', () => { + test('encodes reports larger than the former 1 MiB limit', () => { + const submission = submissionFixture() + submission.report.metadata.version_tag = 'a'.repeat(1024 * 1024) + + const payload = prepareSubmissionPayload(submission) + + expect(payload.bytes.length).toBeGreaterThan(1024 * 1024) + expect(payload.bytes.toString()).toBe(`${JSON.stringify(payload.submission, null, 2)}\n`) + expect(payload.submission).toEqual(submission) + }) + + test('preserves feedback longer than the former 2,000-character cap without truncation', () => { + const submission = submissionFixture() + const feedback = 'a'.repeat(2001) + submission.report.feedback = ` ${feedback} ` + + const payload = prepareSubmissionPayload(submission) + + expect(payload.submission.report.feedback).toBe(feedback) + expect(payload.bytes.toString()).toContain(feedback) + expect(submission.report.feedback).toBe(` ${feedback} `) + }) + + test('encodes Unicode and JSON escaping in the same bytes as the normalized submission', () => { + const submission = submissionFixture() + const feedback = 'é😀 "quoted" \\ slash\nline two\u0000' + submission.report.feedback = feedback + + const payload = prepareSubmissionPayload(submission) + + expect(payload.bytes).toEqual(Buffer.from(`${JSON.stringify(payload.submission, null, 2)}\n`, 'utf8')) + expect(payload.submission.report.feedback).toBe(feedback) + expect(JSON.parse(payload.bytes.toString())).toEqual(payload.submission) + }) + + test.each([null, '', ' \n '])('normalizes absent or empty feedback %j to null', (feedback) => { + const submission = submissionFixture() + submission.report.feedback = feedback + + const payload = prepareSubmissionPayload(submission) + + expect(payload.submission.report.feedback).toBeNull() + expect(payload.bytes.toString()).toContain('"feedback": null') + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-submission-payload.ts b/packages/app/src/cli/services/app-doctor-submission-payload.ts new file mode 100644 index 00000000000..e5e5d2b9892 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-submission-payload.ts @@ -0,0 +1,18 @@ +import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' + +export interface AppDoctorSubmissionPayload { + submission: AppDoctorSubmission + bytes: Buffer +} + +export function prepareSubmissionPayload(submission: AppDoctorSubmission): AppDoctorSubmissionPayload { + const feedback = submission.report.feedback?.trim() ?? '' + const normalizedSubmission = { + ...submission, + report: {...submission.report, feedback: feedback === '' ? null : feedback}, + } + return { + submission: normalizedSubmission, + bytes: Buffer.from(`${JSON.stringify(normalizedSubmission, null, 2)}\n`, 'utf8'), + } +} diff --git a/packages/app/src/cli/services/app-doctor-submit-api.test.ts b/packages/app/src/cli/services/app-doctor-submit-api.test.ts index 8490b450035..1a993f6330d 100644 --- a/packages/app/src/cli/services/app-doctor-submit-api.test.ts +++ b/packages/app/src/cli/services/app-doctor-submit-api.test.ts @@ -3,6 +3,7 @@ import {testDeveloperPlatformClient} from '../models/app/app.test-data.js' import {AbortError} from '@shopify/cli-kit/node/error' import {describe, expect, test, vi} from 'vitest' import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {uploadToGCS} from './bundle.js' import type {SourceScanCreateSchema, SourceScanUploadUrlSchema} from '../utilities/developer-platform-client.js' const app = { @@ -14,7 +15,7 @@ const app = { const submission = {schemaVersion: 1, report: {}} as AppDoctorSubmission function dependencies(upload = vi.fn(async () => {})) { - return {fileSize: vi.fn(async () => 1234), upload} + return {upload} } function options() { @@ -28,8 +29,7 @@ function options() { return { input: { app, - submission, - submissionPath: '/tmp/app/.shopify/app-doctor/submission.json', + payload: {submission, bytes: Buffer.from(JSON.stringify(submission))}, // testDeveloperPlatformClient defaults are plain functions, not spies. // Always inject explicit vi.fn stubs before making call/mocking assertions. developerPlatformClient: testDeveloperPlatformClient({generateSourceScanUploadUrl, createSourceScan}), @@ -105,20 +105,20 @@ describe('submitAppDoctorScan', () => { await expect(result).rejects.toMatchObject({tryMessage: 'Try submitting the App Doctor results again.'}) }) - test('requests an upload URL with the artifact byte size before uploading', async () => { + test('requests an upload URL for bytes above the former 1 MiB cap and uploads the same buffer', async () => { const {input, generateSourceScanUploadUrl, createSourceScan} = options() - const fileSize = vi.fn(async () => 1234) - const upload = vi.fn(async () => {}) + const upload = vi.fn().mockResolvedValue(undefined) + input.payload.bytes = Buffer.alloc(1024 * 1024 + 1, 'a') - await expect(submitAppDoctorScan(input, {fileSize, upload})).resolves.toBeUndefined() + await expect(submitAppDoctorScan(input, {upload})).resolves.toBeUndefined() - expect(fileSize).toHaveBeenCalledWith('/tmp/app/.shopify/app-doctor/submission.json') - expect(generateSourceScanUploadUrl).toHaveBeenCalledWith({appId: app.id, byteSize: 1234}) + expect(generateSourceScanUploadUrl).toHaveBeenCalledWith({appId: app.id, byteSize: input.payload.bytes.length}) expect(generateSourceScanUploadUrl.mock.invocationCallOrder[0]).toBeLessThan(upload.mock.invocationCallOrder[0]!) - expect(upload).toHaveBeenCalledWith('source-scan-upload-url', '/tmp/app/.shopify/app-doctor/submission.json', { - artifactName: 'App Doctor submission', - contentType: 'application/json', - }) + expect(upload).toHaveBeenCalledOnce() + const [url, bytes, uploadOptions] = upload.mock.calls[0]! + expect(url).toBe('source-scan-upload-url') + expect(bytes).toBe(input.payload.bytes) + expect(uploadOptions).toEqual({artifactName: 'App Doctor submission', contentType: 'application/json'}) expect(createSourceScan).toHaveBeenCalledWith({ appId: app.id, sourceScanUrl: 'source-scan-upload-url', diff --git a/packages/app/src/cli/services/app-doctor-submit-api.ts b/packages/app/src/cli/services/app-doctor-submit-api.ts index 3a8e3a6bb4d..31134bc1636 100644 --- a/packages/app/src/cli/services/app-doctor-submit-api.ts +++ b/packages/app/src/cli/services/app-doctor-submit-api.ts @@ -1,23 +1,20 @@ import {uploadToGCS} from './bundle.js' import {AbortError} from '@shopify/cli-kit/node/error' -import {fileSize} from '@shopify/cli-kit/node/fs' -import type {AppDoctorSubmission} from './app-doctor-engine/submission/index.js' +import type {AppDoctorSubmissionPayload} from './app-doctor-submission-payload.js' import type {MinimalAppIdentifiers} from '../models/organization.js' import type {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' export interface SubmitAppDoctorScanOptions { app: MinimalAppIdentifiers - submission: AppDoctorSubmission - submissionPath: string + payload: AppDoctorSubmissionPayload developerPlatformClient: DeveloperPlatformClient } interface SubmitAppDoctorScanDependencies { - fileSize: typeof fileSize upload: typeof uploadToGCS } -const defaultDependencies: SubmitAppDoctorScanDependencies = {fileSize, upload: uploadToGCS} +const defaultDependencies: SubmitAppDoctorScanDependencies = {upload: uploadToGCS} function userErrorMessage(userErrors: {message: string}[], fallback: string): string { return userErrors.map(({message}) => message).join(', ') || fallback @@ -27,7 +24,7 @@ export async function submitAppDoctorScan( options: SubmitAppDoctorScanOptions, dependencies: SubmitAppDoctorScanDependencies = defaultDependencies, ): Promise { - const byteSize = await dependencies.fileSize(options.submissionPath) + const byteSize = options.payload.bytes.length const uploadResult = await options.developerPlatformClient.generateSourceScanUploadUrl({ appId: options.app.id, byteSize, @@ -36,7 +33,7 @@ export async function submitAppDoctorScan( throw new AbortError(userErrorMessage(uploadResult.userErrors, 'Shopify did not return a source scan upload URL.')) } - await dependencies.upload(uploadResult.sourceScanUploadUrl, options.submissionPath, { + await dependencies.upload(uploadResult.sourceScanUploadUrl, options.payload.bytes, { artifactName: 'App Doctor submission', contentType: 'application/json', }) diff --git a/packages/app/src/cli/services/bundle.test.ts b/packages/app/src/cli/services/bundle.test.ts index 102c84d5a42..c18c81de84c 100644 --- a/packages/app/src/cli/services/bundle.test.ts +++ b/packages/app/src/cli/services/bundle.test.ts @@ -212,6 +212,21 @@ describe('compressBundle', () => { }) describe('uploadToGCS', () => { + test('uploads prepared bytes directly and reuses them when retrying', async () => { + const bytes = Buffer.from('{"feedback":"é😀"}\n', 'utf8') + vi.mocked(fetch) + .mockResolvedValueOnce({ok: false, status: 503, text: async () => 'retry'} as never) + .mockResolvedValueOnce({ok: true, status: 200} as never) + + await uploadToGCS('https://signed.example/upload', bytes, {contentType: 'application/json'}) + + expect(fetch).toHaveBeenCalledTimes(2) + for (const [, options] of vi.mocked(fetch).mock.calls) { + expect(options?.body).toBe(bytes) + expect(options?.headers).toEqual({'Content-Type': 'application/json'}) + } + }) + test('uploads the bundle when it is under the size limit', async () => { await inTemporaryDirectory(async (tmpDir) => { // Given diff --git a/packages/app/src/cli/services/bundle.ts b/packages/app/src/cli/services/bundle.ts index aed4e350553..123c9fb3b32 100644 --- a/packages/app/src/cli/services/bundle.ts +++ b/packages/app/src/cli/services/bundle.ts @@ -41,7 +41,7 @@ interface UploadToGCSOptions { } /** - * Upload a file to GCS using a signed URL. + * Upload a file or prepared bytes to GCS using a signed URL. * * GCS replies to the signed PUT with a non-2xx status code when the upload fails * (for example an expired signature, a malformed request, or a transient server @@ -52,15 +52,15 @@ interface UploadToGCSOptions { * the bundle is consumed (e.g. during devSessionCreate). * * @param signedURL - The signed URL to upload the file to - * @param filePath - The path to the file + * @param filePathOrBytes - The path to the file, or prepared bytes to upload without rereading a file * @param options - Optional settings; `artifactName` labels the uploaded artifact in error copy (defaults to `app bundle`), and `contentType` sends a signed Content-Type header. */ export async function uploadToGCS( signedURL: string, - filePath: string, + filePathOrBytes: string | Buffer, {artifactName = 'app bundle', contentType}: UploadToGCSOptions = {}, ) { - const size = await fileSize(filePath) + const size = typeof filePathOrBytes === 'string' ? await fileSize(filePathOrBytes) : filePathOrBytes.length if (size > MAX_BUNDLE_SIZE_BYTES) { // Round up so a size that barely exceeds the cap never displays as the cap. const humanSize = `${(Math.ceil((size / MEGABYTE) * 100) / 100).toFixed(2)} MB` @@ -70,7 +70,7 @@ export async function uploadToGCS( ) } - const buffer = readFileSync(filePath) + const buffer = typeof filePathOrBytes === 'string' ? readFileSync(filePathOrBytes) : filePathOrBytes let response: Response | undefined for (let attempt = 1; attempt <= UPLOAD_MAX_ATTEMPTS; attempt++) { diff --git a/packages/app/src/cli/services/doctor-submit-output.test.ts b/packages/app/src/cli/services/doctor-submit-output.test.ts index 2949bd16ab5..9c48b246889 100644 --- a/packages/app/src/cli/services/doctor-submit-output.test.ts +++ b/packages/app/src/cli/services/doctor-submit-output.test.ts @@ -18,12 +18,13 @@ const submission = buildSubmission(submissionTraceFixture, { const submissionPath = '/tmp/app/.shopify/app-doctor/submission.json' describe('renderDoctorSubmitFeedbackPrompt', () => { - test('warns immediately before prompting and configures optional validated input', async () => { - vi.mocked(renderTextPrompt).mockResolvedValue(' entered feedback ') + test('shows only the privacy warning and prompts without a size validator', async () => { + const feedback = 'a'.repeat(2001) + vi.mocked(renderTextPrompt).mockResolvedValue(feedback) - await expect(renderDoctorSubmitFeedbackPrompt({maxLength: 2000})).resolves.toBe(' entered feedback ') + await expect(renderDoctorSubmitFeedbackPrompt()).resolves.toBe(feedback) - expect(renderWarning).toHaveBeenCalledWith({ + expect(renderWarning).toHaveBeenCalledExactlyOnceWith({ headline: "Don't include source code, file paths or secrets in your optional feedback.", }) expect(vi.mocked(renderWarning).mock.invocationCallOrder[0]).toBeLessThan( @@ -33,13 +34,7 @@ describe('renderDoctorSubmitFeedbackPrompt', () => { message: 'Optional: What was inaccurate or unhelpful about these App Doctor results?', allowEmpty: true, emptyDisplayedValue: '(skipped)', - validate: expect.any(Function), }) - - const [{validate}] = vi.mocked(renderTextPrompt).mock.calls[0]! - expect(validate?.('')).toBeUndefined() - expect(validate?.(` ${'a'.repeat(2000)} `)).toBeUndefined() - expect(validate?.('a'.repeat(2001))).toBe('Feedback must be 2,000 characters or fewer.') }) }) diff --git a/packages/app/src/cli/services/doctor-submit-output.ts b/packages/app/src/cli/services/doctor-submit-output.ts index fa1ed8e2eb2..21e6fa8ab6f 100644 --- a/packages/app/src/cli/services/doctor-submit-output.ts +++ b/packages/app/src/cli/services/doctor-submit-output.ts @@ -10,10 +10,6 @@ export interface DoctorSubmitConfirmationInput { canAddFeedback: boolean } -export interface DoctorSubmitFeedbackPromptInput { - maxLength: number -} - export interface DoctorSubmitDryRunInput { submissionPath: string } @@ -41,16 +37,12 @@ function checksSummary(submission: AppDoctorSubmission): string { return `${executed} executed · ${notApplicable} not applicable · ${unresolved} unresolved` } -export async function renderDoctorSubmitFeedbackPrompt({maxLength}: DoctorSubmitFeedbackPromptInput): Promise { +export async function renderDoctorSubmitFeedbackPrompt(): Promise { renderWarning({headline: "Don't include source code, file paths or secrets in your optional feedback."}) return renderTextPrompt({ message: 'Optional: What was inaccurate or unhelpful about these App Doctor results?', allowEmpty: true, emptyDisplayedValue: '(skipped)', - validate: (value) => - value.trim().length <= maxLength - ? undefined - : `Feedback must be ${new Intl.NumberFormat('en-US').format(maxLength)} characters or fewer.`, }) } diff --git a/packages/app/src/cli/services/doctor-submit.test.ts b/packages/app/src/cli/services/doctor-submit.test.ts index b4a3e5e845d..0d736d16991 100644 --- a/packages/app/src/cli/services/doctor-submit.test.ts +++ b/packages/app/src/cli/services/doctor-submit.test.ts @@ -1,12 +1,14 @@ -import doctorSubmit, {MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH} from './doctor-submit.js' +import doctorSubmit from './doctor-submit.js' import {appDoctorArtifactPaths, writeSubmission} from './app-doctor-artifacts.js' import {buildSubmission} from './app-doctor-engine/submission/index.js' +import {submitAppDoctorScan} from './app-doctor-submit-api.js' import {submissionTraceFixture} from './app-doctor-engine/tests/fixtures/submission-trace.js' import {testDeveloperPlatformClient} from '../models/app/app.test-data.js' -import {inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs' +import {inTemporaryDirectory, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath, moduleDirectory} from '@shopify/cli-kit/node/path' import {AbortError} from '@shopify/cli-kit/node/error' import {describe, expect, test, vi} from 'vitest' +import type {uploadToGCS} from './bundle.js' import type {DoctorSubmitDependencies, DoctorSubmitOptions} from './doctor-submit.js' import type {ReadTraceResult} from './app-doctor-artifacts.js' @@ -85,7 +87,91 @@ function expectNoOutput(dependencies: DoctorSubmitDependencies): void { expect(dependencies.output).not.toHaveBeenCalled() } +function useReportSize(dependencies: DoctorSubmitDependencies, byteSize: number) { + vi.mocked(dependencies.buildSubmission).mockImplementation((trace, buildOptions) => { + const submission = buildSubmission(trace, {...buildOptions, feedback: undefined, versionTag: ''}) + const baseSize = Buffer.byteLength(`${JSON.stringify(submission, null, 2)}\n`) + submission.report.metadata.version_tag = 'a'.repeat(byteSize - baseSize) + submission.report.feedback = buildOptions.feedback ?? null + return submission + }) +} + describe('doctorSubmit', () => { + test.each([false, true])('accepts reports above the former 1 MiB cap (dryRun=%s)', async (dryRun) => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + const byteSize = 1024 * 1024 + 1 + useReportSize(dependencies, byteSize) + + await doctorSubmit({...options(directory), force: true, dryRun}, dependencies) + + const bytes = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + expect(bytes.length).toBe(byteSize) + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toBe(bytes.toString()) + expect(dependencies.submitScan).toHaveBeenCalledTimes(dryRun ? 0 : 1) + expect(dependencies.linkApp).toHaveBeenCalledTimes(dryRun ? 0 : 1) + }) + }) + + test('collects long interactive feedback even when the report exceeds the former byte budget', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + useReportSize(dependencies, 1024 * 1024 + 1) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockResolvedValue('submit-with-feedback') + const feedback = 'é'.repeat(2001) + vi.mocked(dependencies.promptForFeedback).mockResolvedValue(feedback) + + await doctorSubmit(options(directory), dependencies) + + const uploaded = vi.mocked(dependencies.submitScan).mock.calls[0]![0].payload + expect(uploaded.bytes.length).toBeGreaterThan(1024 * 1024) + expect(uploaded.submission.report.feedback).toBe(feedback) + expect(uploaded.bytes).toBe(vi.mocked(dependencies.writeSubmission).mock.calls[1]![1]) + }) + }) + + test('does not upload if writing the final feedback payload fails', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockResolvedValue('submit-with-feedback') + vi.mocked(dependencies.promptForFeedback).mockResolvedValue('Accepted feedback') + vi.mocked(dependencies.writeSubmission) + .mockImplementationOnce(writeSubmission) + .mockRejectedValueOnce(new Error('Disk full')) + + await expect(doctorSubmit(options(directory), dependencies)).rejects.toThrow('Disk full') + + expect(dependencies.submitScan).not.toHaveBeenCalled() + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain('"feedback": null') + }) + }) + + test('does not upload changes made to the artifact during confirmation', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies(directory) + const upload = vi.fn().mockResolvedValue(undefined) + vi.mocked(dependencies.submitScan).mockImplementation((input) => submitAppDoctorScan(input, {upload})) + vi.mocked(dependencies.canPrompt).mockReturnValue(true) + vi.mocked(dependencies.confirm).mockImplementation(async ({submissionPath}) => { + await writeFile(submissionPath, 'tampered artifact') + return 'submit' + }) + + await doctorSubmit({...options(directory), feedback: 'Approved feedback'}, dependencies) + + const uploaded = vi.mocked(dependencies.submitScan).mock.calls[0]![0].payload + expect(uploaded.bytes).toBe(vi.mocked(dependencies.writeSubmission).mock.calls[0]![1]) + expect(uploaded.bytes.toString()).toContain('Approved feedback') + expect(uploaded.bytes.toString()).not.toContain('tampered artifact') + expect(upload).toHaveBeenCalledOnce() + expect(upload.mock.calls[0]![1]).toBe(uploaded.bytes) + await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toBe('tampered artifact') + }) + }) + test('fails before linking when the trace is missing', async () => { await inTemporaryDirectory(async (directory) => { const dependencies = testDependencies(directory) @@ -134,7 +220,7 @@ describe('doctorSubmit', () => { submission: expect.objectContaining({report: expect.objectContaining({feedback: null})}), }), ) - expect(dependencies.promptForFeedback).toHaveBeenCalledWith({maxLength: MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH}) + expect(dependencies.promptForFeedback).toHaveBeenCalledExactlyOnceWith() expect(dependencies.buildSubmission).toHaveBeenCalledTimes(2) expect(dependencies.buildSubmission).toHaveBeenLastCalledWith( expect.anything(), @@ -142,9 +228,10 @@ describe('doctorSubmit', () => { ) expect(dependencies.writeSubmission).toHaveBeenCalledTimes(2) - const rewrittenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[1]![1] - expect(rewrittenSubmission.report.feedback).toBe('The authorization finding was inaccurate.') - expect(vi.mocked(dependencies.submitScan).mock.calls[0]![0].submission).toBe(rewrittenSubmission) + const rewrittenBytes = vi.mocked(dependencies.writeSubmission).mock.calls[1]![1] + const uploadedPayload = vi.mocked(dependencies.submitScan).mock.calls[0]![0].payload + expect(uploadedPayload.submission.report.feedback).toBe('The authorization finding was inaccurate.') + expect(uploadedPayload.bytes).toBe(rewrittenBytes) expect(vi.mocked(dependencies.confirm).mock.invocationCallOrder[0]).toBeLessThan( vi.mocked(dependencies.promptForFeedback).mock.invocationCallOrder[0]!, ) @@ -175,7 +262,9 @@ describe('doctorSubmit', () => { ) expect(dependencies.submitScan).toHaveBeenCalledWith( expect.objectContaining({ - submission: expect.objectContaining({report: expect.objectContaining({feedback: null})}), + payload: expect.objectContaining({ + submission: expect.objectContaining({report: expect.objectContaining({feedback: null})}), + }), }), ) }) @@ -225,30 +314,23 @@ describe('doctorSubmit', () => { }) }) - test('accepts feedback at exactly the maximum length', async () => { - await inTemporaryDirectory(async (directory) => { - const dependencies = testDependencies(directory) - const feedback = 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH) - - await doctorSubmit({...options(directory), force: true, feedback}, dependencies) - - expect(dependencies.buildSubmission).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({feedback})) - }) - }) - test.each([ - {source: 'flag', feedback: 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH + 1), stdin: undefined}, - {source: 'stdin', feedback: '-', stdin: 'a'.repeat(MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH + 1)}, - ])('rejects overlong $source feedback instead of truncating it', async ({feedback, stdin}) => { + {source: 'flag', input: 'a'.repeat(2001), dryRun: false}, + {source: 'stdin', input: '-', dryRun: false}, + {source: 'flag', input: 'a'.repeat(2001), dryRun: true}, + {source: 'stdin', input: '-', dryRun: true}, + ])('preserves $source feedback above the former character cap (dryRun=$dryRun)', async ({input, dryRun}) => { await inTemporaryDirectory(async (directory) => { const dependencies = testDependencies(directory) - vi.mocked(dependencies.readStdin).mockResolvedValue(stdin) + const feedback = 'a'.repeat(2001) + vi.mocked(dependencies.readStdin).mockResolvedValue(feedback) - const error = await capturedAbort(doctorSubmit({...options(directory), force: true, feedback}, dependencies)) + await doctorSubmit({...options(directory), force: true, dryRun, feedback: input}, dependencies) - expect(error.message).toContain('Feedback must be 2,000 characters or fewer.') - expect(dependencies.buildSubmission).not.toHaveBeenCalled() - expect(dependencies.writeSubmission).not.toHaveBeenCalled() + const bytes = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + expect(JSON.parse(bytes.toString()).report.feedback).toBe(feedback) + expect(dependencies.promptForFeedback).not.toHaveBeenCalled() + expect(dependencies.submitScan).toHaveBeenCalledTimes(dryRun ? 0 : 1) }) }) @@ -277,8 +359,8 @@ describe('doctorSubmit', () => { await doctorSubmit({...options(directory), dryRun: true, feedback: ' Dry-run feedback '}, dependencies) - const writtenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] - expect(writtenSubmission.report.feedback).toBe('Dry-run feedback') + const writtenBytes = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + expect(writtenBytes.toString()).toContain('"feedback": "Dry-run feedback"') await expect(readFile(appDoctorArtifactPaths(directory).submissionPath)).resolves.toContain( '"feedback": "Dry-run feedback"', ) @@ -307,12 +389,13 @@ describe('doctorSubmit', () => { await doctorSubmit({...options(directory), feedback: 'Submitted feedback'}, dependencies) - const writtenSubmission = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + const writtenBytes = vi.mocked(dependencies.writeSubmission).mock.calls[0]![1] + const uploadedPayload = vi.mocked(dependencies.submitScan).mock.calls[0]![0].payload expect(dependencies.confirm).toHaveBeenCalledWith(expect.objectContaining({canAddFeedback: false})) expect(dependencies.promptForFeedback).not.toHaveBeenCalled() - expect(vi.mocked(dependencies.confirm).mock.calls[0]![0].submission).toBe(writtenSubmission) - expect(vi.mocked(dependencies.submitScan).mock.calls[0]![0].submission).toBe(writtenSubmission) - expect(writtenSubmission.report.feedback).toBe('Submitted feedback') + expect(vi.mocked(dependencies.confirm).mock.calls[0]![0].submission).toBe(uploadedPayload.submission) + expect(uploadedPayload.bytes).toBe(writtenBytes) + expect(uploadedPayload.submission.report.feedback).toBe('Submitted feedback') }) }) @@ -468,12 +551,14 @@ describe('doctorSubmit', () => { }) expect(dependencies.submitScan).toHaveBeenCalledWith( expect.objectContaining({ - submission: expect.objectContaining({ - report: expect.objectContaining({ - metadata: { - version_tag: 'v1.2.3', - source_control_url: 'https://github.com/example/app/tree/v1.2.3', - }, + payload: expect.objectContaining({ + submission: expect.objectContaining({ + report: expect.objectContaining({ + metadata: { + version_tag: 'v1.2.3', + source_control_url: 'https://github.com/example/app/tree/v1.2.3', + }, + }), }), }), }), diff --git a/packages/app/src/cli/services/doctor-submit.ts b/packages/app/src/cli/services/doctor-submit.ts index e8a4a7259ea..e4974dad8d9 100644 --- a/packages/app/src/cli/services/doctor-submit.ts +++ b/packages/app/src/cli/services/doctor-submit.ts @@ -3,6 +3,7 @@ import {appDoctorArtifactPaths, readTrace, writeSubmission} from './app-doctor-a import {findAppRoot} from './app-doctor-engine/scanners/discover.js' import {buildSubmission, SUBMISSION_SCHEMA_VERSION} from './app-doctor-engine/submission/index.js' import {submitAppDoctorScan} from './app-doctor-submit-api.js' +import {prepareSubmissionPayload} from './app-doctor-submission-payload.js' import { renderDoctorSubmitConfirmation, renderDoctorSubmitDryRun, @@ -21,14 +22,11 @@ import type { DoctorSubmitConfirmationAction, DoctorSubmitConfirmationInput, DoctorSubmitDryRunInput, - DoctorSubmitFeedbackPromptInput, DoctorSubmitSuccessInput, } from './doctor-submit-output.js' import type {MinimalAppIdentifiers} from '../models/organization.js' import type {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' -export const MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH = 2000 - export interface DoctorSubmitOptions { directory: string json: boolean @@ -71,10 +69,10 @@ export interface DoctorSubmitDependencies { skipPrompts: boolean }): Promise buildSubmission(trace: TraceV2, options: BuildSubmissionOptions): AppDoctorSubmission - writeSubmission(appRoot: string, payload: AppDoctorSubmission): Promise + writeSubmission(appRoot: string, bytes: Buffer): Promise canPrompt(): boolean readStdin(): Promise - promptForFeedback(input: DoctorSubmitFeedbackPromptInput): Promise + promptForFeedback(): Promise confirm(input: DoctorSubmitConfirmationInput): Promise submitScan(options: SubmitAppDoctorScanOptions): Promise renderDryRun(input: DoctorSubmitDryRunInput): void @@ -120,14 +118,6 @@ function submitJsonResult(submissionPath: string, submission: AppDoctorSubmissio } } -function normalizeFeedback(feedback: string): string | undefined { - const normalizedFeedback = feedback.trim() - if (normalizedFeedback.length > MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH) { - throw new AbortError('Feedback must be 2,000 characters or fewer.') - } - return normalizedFeedback === '' ? undefined : normalizedFeedback -} - async function resolveExplicitFeedback( options: DoctorSubmitOptions, dependencies: DoctorSubmitDependencies, @@ -140,7 +130,7 @@ async function resolveExplicitFeedback( 'Pipe feedback to the command or pass it directly with --feedback .', ]) } - return normalizeFeedback(feedback) + return feedback.trim() || undefined } export default async function doctorSubmit( @@ -161,22 +151,26 @@ export default async function doctorSubmit( } const submittedAt = dependencies.now() - const buildSubmissionWithFeedback = (feedback: string | undefined) => - dependencies.buildSubmission(traceResult.trace, { - cliVersion: dependencies.cliVersion, - submittedAt, - versionTag: options.versionTag, - sourceControlUrl: options.sourceControlUrl, - feedback, - }) + const prepareFeedback = (feedback: string | undefined) => { + const normalizedFeedback = feedback?.trim() + return prepareSubmissionPayload( + dependencies.buildSubmission(traceResult.trace, { + cliVersion: dependencies.cliVersion, + submittedAt, + versionTag: options.versionTag, + sourceControlUrl: options.sourceControlUrl, + feedback: normalizedFeedback === '' ? undefined : normalizedFeedback, + }), + ) + } - let feedback = await resolveExplicitFeedback(options, dependencies) - let submission = buildSubmissionWithFeedback(feedback) - await dependencies.writeSubmission(appRoot, submission) + const feedback = await resolveExplicitFeedback(options, dependencies) + let payload = prepareFeedback(feedback) + await dependencies.writeSubmission(appRoot, payload.bytes) if (options.dryRun) { if (options.json) { - dependencies.output(JSON.stringify(dryRunJsonResult(paths.submissionPath, submission), null, 2)) + dependencies.output(JSON.stringify(dryRunJsonResult(paths.submissionPath, payload.submission), null, 2)) } else { dependencies.renderDryRun({submissionPath: paths.submissionPath}) } @@ -198,27 +192,26 @@ export default async function doctorSubmit( const confirmationAction = await dependencies.confirm({ appTitle: remoteApp.title, submissionPath: paths.submissionPath, - submission, + submission: payload.submission, canAddFeedback: options.feedback === undefined, }) if (confirmationAction === 'cancel') return if (confirmationAction === 'submit-with-feedback') { - feedback = normalizeFeedback(await dependencies.promptForFeedback({maxLength: MAX_DOCTOR_SUBMIT_FEEDBACK_LENGTH})) - submission = buildSubmissionWithFeedback(feedback) - await dependencies.writeSubmission(appRoot, submission) + const enteredFeedback = await dependencies.promptForFeedback() + payload = prepareFeedback(enteredFeedback) + await dependencies.writeSubmission(appRoot, payload.bytes) } } await dependencies.submitScan({ app: remoteApp, - submission, - submissionPath: paths.submissionPath, + payload, developerPlatformClient, }) if (options.json) { - dependencies.output(JSON.stringify(submitJsonResult(paths.submissionPath, submission), null, 2)) + dependencies.output(JSON.stringify(submitJsonResult(paths.submissionPath, payload.submission), null, 2)) } else { dependencies.renderSuccess({ appTitle: remoteApp.title,