Combine deterministic and agent results in App Security review and submit

packages/app/src/cli/commands/app/security/blocking-flag.ts

@@ -0,0 +1,17 @@
+import {Flags} from '@oclif/core'
+import type {AppSecurityBlockingLevel} from '../../../services/app-security-api.js'
+
+const blockingLevels: AppSecurityBlockingLevel[] = ['high', 'medium', 'low', 'none']
+
+/**
+ * `--blocking`, shared by `check` and `review` so both accept the same levels and default.
+ * `Flags.custom` types the parsed value as an `AppSecurityBlockingLevel`, so the commands don't cast it.
+ */
+export const appSecurityBlockingFlag = {
+  blocking: Flags.custom<AppSecurityBlockingLevel>({
+    description: 'The minimum finding severity that causes a non-zero exit code.',
+    options: blockingLevels,
+    default: 'none',
+    env: 'SHOPIFY_FLAG_APP_SECURITY_BLOCKING',
+  })(),
+}

packages/app/src/cli/commands/app/security/check.integration.test.ts

@@ -102,8 +102,9 @@ describe('app security check command boundary', () => {
       await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministic_findings)
       await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
         schema_version: 1,
+        source: 'deterministic',
         engine: {name: 'shopify-app-security'},
-        findings: expect.any(Array),
+        checks: expect.any(Array),
       })
       await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({
         schema_version: 1,
@@ -136,7 +137,8 @@ describe('app security check command boundary', () => {
       expect(unstyled(rescan.stdout)).not.toMatch(/discard/i)
       await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
         schema_version: 1,
-        findings: expect.any(Array),
+        source: 'deterministic',
+        checks: expect.any(Array),
       })
       await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({
         schema_version: 1,

packages/app/src/cli/commands/app/security/check.ts

@@ -1,13 +1,11 @@
+import {appSecurityBlockingFlag} from './blocking-flag.js'
 import {appFlags} from '../../../flags.js'
 import {ignorePatternProblem} from '../../../services/app-security-engine/index.js'
 import securityCheck from '../../../services/security-check.js'
 import {Flags} from '@oclif/core'
 import BaseCommand from '@shopify/cli-kit/node/base-command'
 import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli'
 import {AbortError} from '@shopify/cli-kit/node/error'
-import type {AppSecurityBlockingLevel} from '../../../services/app-security-api.js'
-
-const blockingLevels: AppSecurityBlockingLevel[] = ['high', 'medium', 'low', 'none']
 
 export default class SecurityCheck extends BaseCommand {
   static hidden = true
@@ -42,12 +40,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio
       },
     }),
     ...jsonFlag,
-    blocking: Flags.string({
-      description: 'The minimum finding severity that causes a non-zero exit code.',
-      options: blockingLevels,
-      default: 'none',
-      env: 'SHOPIFY_FLAG_APP_SECURITY_BLOCKING',
-    }),
+    ...appSecurityBlockingFlag,
     yes: Flags.boolean({
       description: 'Print coding-agent instructions without prompting.',
       default: false,
@@ -70,7 +63,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio
       configName: flags.config,
       json: flags.json,
       verbose: Boolean(flags.verbose),
-      blocking: flags.blocking as AppSecurityBlockingLevel,
+      blocking: flags.blocking,
       yes: flags.yes,
       skipInstructions: flags['skip-instructions'],
       ignorePatterns: flags.ignore ?? [],

packages/app/src/cli/commands/app/security/review.test.ts

@@ -1,4 +1,5 @@
 import SecurityReview from './review.js'
+import SecurityCheck from './check.js'
 import {appFlags} from '../../../flags.js'
 import securityReview from '../../../services/security-review.js'
 import {securityReviewJsonOutputSchema} from '../../../services/security-review-json.js'
@@ -19,15 +20,52 @@ describe('app security review command', () => {
     expect(SecurityReview.jsonOutputSchema).toBe(securityReviewJsonOutputSchema)
   })
 
-  test('reviews the current directory by default', async () => {
+  test('shares the --blocking flag with check', () => {
+    expect(SecurityReview.flags.blocking).toBe(SecurityCheck.flags.blocking)
+    expect(SecurityReview.flags.blocking.options).toEqual(['high', 'medium', 'low', 'none'])
+    expect(SecurityReview.flags.blocking.env).toBe('SHOPIFY_FLAG_APP_SECURITY_BLOCKING')
+  })
+
+  test('reads --check-id repeatedly and from SHOPIFY_FLAG_CHECK_ID', () => {
+    expect(SecurityReview.flags['check-id'].multiple).toBe(true)
+    expect(SecurityReview.flags['check-id'].env).toBe('SHOPIFY_FLAG_CHECK_ID')
+  })
+
+  test('reviews the current directory by default with no filter and no blocking', async () => {
     await SecurityReview.run([], import.meta.url)
 
-    expect(securityReview).toHaveBeenCalledWith({directory: cwd(), json: false})
+    expect(securityReview).toHaveBeenCalledWith({
+      directory: cwd(),
+      json: false,
+      verbose: false,
+      checkIds: [],
+      blocking: 'none',
+    })
   })
 
-  test('forwards --path and --json', async () => {
-    await SecurityReview.run(['--path', './fixtures/app', '--json'], import.meta.url)
+  test('forwards --path, --json, --verbose, every --check-id and --blocking', async () => {
+    await SecurityReview.run(
+      [
+        '--path',
+        './fixtures/app',
+        '--json',
+        '--verbose',
+        '--check-id',
+        'OPEN_REDIRECT',
+        '--check-id',
+        'EOL_API_VERSION',
+        '--blocking',
+        'medium',
+      ],
+      import.meta.url,
+    )
 
-    expect(securityReview).toHaveBeenCalledWith({directory: resolvePath('./fixtures/app'), json: true})
+    expect(securityReview).toHaveBeenCalledWith({
+      directory: resolvePath('./fixtures/app'),
+      json: true,
+      verbose: true,
+      checkIds: ['OPEN_REDIRECT', 'EOL_API_VERSION'],
+      blocking: 'medium',
+    })
   })
 })

packages/app/src/cli/commands/app/security/review.ts

@@ -1,17 +1,19 @@
+import {appSecurityBlockingFlag} from './blocking-flag.js'
 import {appFlags} from '../../../flags.js'
 import securityReview from '../../../services/security-review.js'
 import {securityReviewJsonOutputSchema} from '../../../services/security-review-json.js'
+import {Flags} from '@oclif/core'
 import BaseCommand from '@shopify/cli-kit/node/base-command'
 import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli'
 
 export default class SecurityReview extends BaseCommand {
   static hidden = true
 
-  static summary = 'Show the stored App Security results.'
+  static summary = 'Show the combined App Security results.'
 
-  static descriptionWithMarkdown = `Prints the deterministic findings (\`.shopify/app-security/deterministic-findings.json\`) and the recorded agent findings (\`.shopify/app-security/agent-findings.json\`), each with its path and age.
+  static descriptionWithMarkdown = `Combines the deterministic results (\`.shopify/app-security/deterministic-findings.json\`, written by \`shopify app security check\`) with the recorded agent results (\`.shopify/app-security/agent-findings.json\`, written by \`shopify app security record\`) and shows one view of every check: its findings, status and source.
 
-The two files are shown as they are stored. They aren't compared with each other or with the current source files.`
+The agent results are optional. Use \`--check-id\` to narrow the review to specific checks, \`--verbose\` for full reasoning, evidence and suppressed findings, and \`--blocking\` to exit with code 1 when a check with findings is at or above a severity.`
 
   static get jsonOutputSchema() {
     return securityReviewJsonOutputSchema
@@ -23,11 +25,23 @@ The two files are shown as they are stored. They aren't compared with each other
     ...globalFlags,
     path: appFlags.path,
     ...jsonFlag,
+    'check-id': Flags.string({
+      description: 'Show only this check. Repeat the flag to show several checks.',
+      env: 'SHOPIFY_FLAG_CHECK_ID',
+      multiple: true,
+    }),
+    ...appSecurityBlockingFlag,
   }
 
   public async run(): Promise<void> {
     const {flags} = await this.parse(SecurityReview)
 
-    await securityReview({directory: flags.path, json: flags.json})
+    await securityReview({
+      directory: flags.path,
+      json: flags.json,
+      verbose: Boolean(flags.verbose),
+      checkIds: flags['check-id'] ?? [],
+      blocking: flags.blocking,
+    })
   }
 }

packages/app/src/cli/commands/app/security/submit.integration.test.ts

@@ -2,7 +2,10 @@ import SecuritySubmit from './submit.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
 import {resolveSecuritySubmitClientId} from '../../../services/app-security-submit-target.js'
 import {clearCachedAppInfo, setCachedAppInfo} from '../../../services/local-storage.js'
-import {submissionScanFixture} from '../../../services/app-security-engine/tests/fixtures/submission-scan.js'
+import {
+  agentFindingsDocument,
+  deterministicFindingsDocument,
+} from '../../../services/app-security-engine/tests/fixtures/findings-documents.js'
 import {testDeveloperPlatformClient, testOrganizationApp} from '../../../models/app/app.test-data.js'
 import {defaultDeveloperPlatformClient} from '../../../utilities/developer-platform-client.js'
 import {Config} from '@oclif/core'
@@ -78,14 +81,17 @@ function remoteClient() {
   return {appFromIdentifiers, accountInfo, generateSourceScanUploadUrl, createSourceScan}
 }
 
-async function writeApp(directory: string) {
+async function writeApp(directory: string, {agent = true}: {agent?: boolean} = {}) {
   const paths = appSecurityArtifactPaths(directory)
   await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "configured-client-id"\n')
   await mkdir(paths.artifactDirectory, {recursive: true})
-  await writeFile(paths.deterministicFindingsPath, JSON.stringify(submissionScanFixture))
+  await writeFile(paths.deterministicFindingsPath, JSON.stringify(deterministicFindingsDocument))
+  if (agent) await writeFile(paths.agentFindingsPath, JSON.stringify(agentFindingsDocument))
   return paths
 }
 
+const resultFileNames = ['agent-findings.json', 'deterministic-findings.json']
+
 async function runCommand(argv: string[]) {
   let stdout = ''
   let stderr = ''
@@ -141,11 +147,16 @@ describe('app security submit command boundary', () => {
     })
   })
 
-  test.each([false, true])('dry-run uses real root, scan and artifact I/O (json=%s)', async (json) => {
+  test.each([
+    {json: false, agent: true},
+    {json: true, agent: true},
+    {json: false, agent: false},
+    {json: true, agent: false},
+  ])('dry-run uses real root, results and artifact I/O (json=$json, agent=$agent)', async ({json, agent}) => {
     await inTemporaryDirectory(async (directory) => {
       const client = remoteClient()
-      const paths = await writeApp(directory)
-      const scan = await readFile(paths.deterministicFindingsPath)
+      const paths = await writeApp(directory, {agent})
+      const deterministic = await readFile(paths.deterministicFindingsPath)
       await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Unlinked app"\n')
       const result = await runCommand(['--path', directory, '--dry-run', ...(json ? ['--json'] : [])])
 
@@ -155,26 +166,59 @@ describe('app security submit command boundary', () => {
         expect(JSON.parse(result.stdout)).toEqual({
           operation: 'submit',
           dry_run: true,
-          payload: {path: paths.submissionPath, schema_version: 0},
+          payload: {path: paths.submissionPath, schema_version: 2},
         })
         expect(result.stderr).toBe('')
       } else {
         expect(result.stdout).toBe('')
-        expect(result.stderr).toContain('Prepared the App Security submission without uploading it.')
+        expect(result.stderr).toContain('Prepared the App Security submission without sending it.')
       }
       const submission = JSON.parse(await readFile(paths.submissionPath, 'utf8'))
-      expect(submission.schemaVersion).toBe(0)
+      expect(submission.schemaVersion).toBe(2)
       expect(submission.report).not.toHaveProperty('attestation')
       expect(submission.report.metadata).toEqual({version_tag: null})
+      expect(submission.report.sources.deterministic).toMatchObject({source: 'deterministic'})
+      expect(submission.report.sources.agent).toEqual(agent ? expect.objectContaining({source: 'agent'}) : null)
       expect(resolveSecuritySubmitClientId).not.toHaveBeenCalled()
       expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled()
       expect(client.appFromIdentifiers).not.toHaveBeenCalled()
       expect(fetch).not.toHaveBeenCalled()
-      await expect(readFile(paths.deterministicFindingsPath)).resolves.toEqual(scan)
+      await expect(readFile(paths.deterministicFindingsPath)).resolves.toEqual(deterministic)
       await expect(readdir(joinPath(directory, '.shopify'))).resolves.toEqual(['app-security'])
     })
   })
 
+  test.each([false, true])('missing results are an expected error with a next step (json=%s)', async (json) => {
+    await inTemporaryDirectory(async (directory) => {
+      remoteClient()
+      await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "configured-client-id"\n')
+      const paths = appSecurityArtifactPaths(directory)
+      const result = await runCommand(['--path', directory, '--force', ...(json ? ['--json'] : [])])
+
+      expect(result.exitCode).toBe(1)
+      if (json) {
+        expect(JSON.parse(result.stdout)).toEqual({
+          operation: 'submit',
+          error: {
+            stage: 'preparation',
+            message: `No App Security results found in ${paths.artifactDirectory}.`,
+            next_steps: [expect.stringMatching(/^Run shopify app security check --path .* first, then submit\.$/)],
+          },
+        })
+        expect(result.stderr).toBe('')
+      } else {
+        expect(result.stdout).toBe('')
+        const messageText = unstyled(result.stderr).replaceAll('│', '').replace(/\s+/g, ' ')
+        expect(messageText).toContain('No App Security results found in')
+        expect(messageText).toContain('first, then submit.')
+        expect(result.stderr).not.toContain('To investigate the issue, examine this stack trace:')
+      }
+      expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled()
+      expect(fetch).not.toHaveBeenCalled()
+      await expect(readdir(directory)).resolves.toEqual(['shopify.app.toml'])
+    })
+  })
+
   test.each([
     {flags: ['--config', 'staging'], clientId: undefined, configName: 'staging'},
     {flags: ['--client-id', 'explicit-client-id'], clientId: 'explicit-client-id', configName: undefined},
@@ -191,10 +235,10 @@ describe('app security submit command boundary', () => {
       expect(JSON.parse(result.stdout)).toEqual({
         operation: 'submit',
         dry_run: true,
-        payload: {path: paths.submissionPath, schema_version: 0},
+        payload: {path: paths.submissionPath, schema_version: 2},
       })
       expect(resolveSecuritySubmitClientId).toHaveBeenCalledExactlyOnceWith({directory, clientId, configName})
-      await expect(readFile(paths.submissionPath, 'utf8')).resolves.toContain('"schemaVersion": 0')
+      await expect(readFile(paths.submissionPath, 'utf8')).resolves.toContain('"schemaVersion": 2')
       expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled()
       expect(client.appFromIdentifiers).not.toHaveBeenCalled()
       expect(client.generateSourceScanUploadUrl).not.toHaveBeenCalled()
@@ -315,7 +359,7 @@ describe('app security submit command boundary', () => {
       expect(JSON.parse(result.stdout)).toEqual({
         operation: 'submit',
         dry_run: false,
-        payload: {path: paths.submissionPath, schema_version: 0},
+        payload: {path: paths.submissionPath, schema_version: 2},
         submitted_at: submission.report.submitted_at,
         client_id: 'api-key',
       })
@@ -392,7 +436,7 @@ describe('app security submit command boundary', () => {
         expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled()
         expect(client.appFromIdentifiers).not.toHaveBeenCalled()
         expect(fetch).not.toHaveBeenCalled()
-        await expect(readdir(paths.artifactDirectory)).resolves.toEqual(['deterministic-findings.json'])
+        await expect(readdir(paths.artifactDirectory)).resolves.toEqual(resultFileNames)
       })
     },
   )
@@ -431,7 +475,7 @@ describe('app security submit command boundary', () => {
         expect(client.generateSourceScanUploadUrl).not.toHaveBeenCalled()
         expect(client.createSourceScan).not.toHaveBeenCalled()
         expect(fetch).not.toHaveBeenCalled()
-        await expect(readdir(paths.artifactDirectory)).resolves.toEqual(['deterministic-findings.json'])
+        await expect(readdir(paths.artifactDirectory)).resolves.toEqual(resultFileNames)
         await expect(readFile(configPath, 'utf8')).resolves.toBe(configContent)
       } finally {
         if (selection === 'cached') clearCachedAppInfo(directory)

packages/app/src/cli/commands/app/security/submit.test.ts

@@ -24,21 +24,27 @@ describe('app security submit command', () => {
     process.exitCode = previousExitCode
   })
 
-  test('is hidden and lets the service link only after reading the scan', () => {
+  test('is hidden and lets the service link only after loading the results', () => {
     expect(SecuritySubmit.hidden).toBe(true)
     expect(SecuritySubmit.prototype).toBeInstanceOf(BaseCommand)
     expect(SecuritySubmit.prototype).not.toBeInstanceOf(AppLinkedCommand)
     expect(SecuritySubmit.flags.path).toBe(appFlags.path)
     expect(SecuritySubmit.flags.config).toBe(appFlags.config)
     expect(SecuritySubmit.flags['client-id']).toBe(appFlags['client-id'])
     expect(SecuritySubmit.args).not.toHaveProperty('directory')
-    expect(SecuritySubmit.descriptionWithMarkdown).toContain('`.shopify/app-security/deterministic-findings.json`')
-    expect(SecuritySubmit.descriptionWithMarkdown).toContain('Generated report fields exclude source code, file paths')
-    expect(SecuritySubmit.descriptionWithMarkdown).toContain('Optional feedback is included without redaction')
-    expect(SecuritySubmit.descriptionWithMarkdown).not.toContain(
-      'No source code, file paths, snippets, or commit identifiers are sent',
+  })
+
+  test('frames the upload as sending the results review shows, plus feedback', () => {
+    expect(SecuritySubmit.summary).toBe('Send App Security results and feedback to Shopify.')
+    expect(SecuritySubmit.descriptionWithMarkdown).toBe(
+      'Sends the App Security results that `shopify app security review` shows to Shopify, with your optional feedback. ' +
+        'Reads `.shopify/app-security/deterministic-findings.json` and, when present, `agent-findings.json`, writes ' +
+        '`.shopify/app-security/submission.json` for inspection, and asks for confirmation before uploading.\n\n' +
+        'The upload excludes source code, file paths, code snippets, evidence, finding messages, agent reasoning and ' +
+        'reasons, suppression justifications, and commit identifiers. Feedback is sent without redaction. Optionally ' +
+        'use `--version` to identify the app version these results came from. Use `--dry-run` to write and inspect ' +
+        'the exact payload without uploading it.',
     )
-    expect(SecuritySubmit.descriptionWithMarkdown).toContain('--version')
     expect(SecuritySubmit.descriptionWithMarkdown).not.toContain('--source-control-url')
   })
 
@@ -48,6 +54,12 @@ describe('app security submit command', () => {
     )
   })
 
+  test('describes feedback as optional and about the results or the tool', () => {
+    expect(SecuritySubmit.flags.feedback.description).toBe(
+      'Optional feedback about these App Security results or this tool. Use - to read from stdin.',
+    )
+  })
+
   test('does not offer a source-control URL or hash flag', () => {
     expect(SecuritySubmit.flags).not.toHaveProperty('source-control-url')
     expect(SecuritySubmit.flags).not.toHaveProperty('source-control-hash')