Add JSON output schemas to app security check and instructions

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

@@ -1,8 +1,13 @@
 import SecurityCheck from './check.js'
+import SecurityInstructions from './instructions.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
+import {appFromIdentifiers} from '../../../services/context.js'
 import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
+import {securityCheckJsonOutputSchema} from '../../../services/security-check-json.js'
+import {securityInstructionsJsonOutputSchema} from '../../../services/security-instructions-json.js'
 import {Config} from '@oclif/core'
-import {fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {fileExists, fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
 import {unstyled} from '@shopify/cli-kit/node/output'
 import {joinPath, normalizePath} from '@shopify/cli-kit/node/path'
 import {describe, expect, test, vi} from 'vitest'
@@ -25,6 +30,11 @@ vi.mock('@shopify/cli-kit/node/session', async (importOriginal) => ({
   ...(await importOriginal<typeof import('@shopify/cli-kit/node/session')>()),
   setCurrentSessionAlias: vi.fn(),
 }))
+// The --client-id lookup needs a login and the network. The mock finds every client ID unless a test rejects it.
+vi.mock('../../../services/context.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../../../services/context.js')>()),
+  appFromIdentifiers: vi.fn(),
+}))
 
 async function createApp(directory: string): Promise<{nestedDirectory: string}> {
   const routesDirectory = joinPath(directory, 'app', 'routes')
@@ -52,7 +62,7 @@ function expectMentionsPath(message: string, path: string): void {
   expect(message.replaceAll(' ', '')).toContain(path)
 }
 
-async function runCommand(argv: string[]) {
+async function runCommand(argv: string[], command: typeof SecurityCheck | typeof SecurityInstructions = SecurityCheck) {
   let stdout = ''
   let stderr = ''
   const previousExitCode = process.exitCode
@@ -76,7 +86,7 @@ async function runCommand(argv: string[]) {
     const config = await Config.load(import.meta.url)
     // This test invokes the app command directly, not as a separately installed CLI plugin.
     config.plugins.clear()
-    await SecurityCheck.run(argv, config)
+    await command.run(argv, config)
     return {stdout, stderr, exitCode: process.exitCode}
   } finally {
     warn.mockRestore()
@@ -88,6 +98,23 @@ async function runCommand(argv: string[]) {
 }
 
 describe('app security check command boundary', () => {
+  test('puts the post-scan instructions in the JSON result with --yes, and prints nothing else to stdout', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+
+      const result = await runCommand(['--path', directory, '--json', '--yes'])
+
+      expect(result.exitCode).toBe(0)
+      const output = JSON.parse(result.stdout)
+      expect(securityCheckJsonOutputSchema.validate(output)).toEqual(output)
+      expect(output.instructions).toEqual({
+        content: expect.stringContaining('Use the existing scan results'),
+        copiedToClipboard: false,
+        path: null,
+      })
+    })
+  })
+
   test('scans an app from a nested directory and writes deterministic-findings.json and agent-checks.json', async () => {
     await inTemporaryDirectory(async (directory) => {
       const {nestedDirectory} = await createApp(directory)
@@ -98,17 +125,25 @@ describe('app security check command boundary', () => {
 
       expect(result.exitCode).toBe(0)
       const output = JSON.parse(result.stdout)
-      expect(Object.keys(output).sort()).toEqual(['agent_checks_path', 'deterministic_findings', 'engine', 'selection'])
-      expect(output.agent_checks_path).toBe(paths.agentChecksPath)
+      expect(securityCheckJsonOutputSchema.validate(output)).toEqual(output)
+      expect(Object.keys(output).sort()).toEqual([
+        'agentChecksPath',
+        'deterministicFindings',
+        'instructions',
+        'selection',
+        'status',
+      ])
+      expect(output.status).toBe('success')
+      expect(output.agentChecksPath).toBe(paths.agentChecksPath)
+      expect(output.instructions).toBeNull()
       expect(output.selection).toEqual({
-        app_directory: appDirectory,
-        app_config_file: joinPath(appDirectory, 'shopify.app.toml'),
-        client_id: 'test-client-id',
-        client_id_source: 'config',
-        scan_directories: [{directory: appDirectory, origin: 'app_directory'}],
+        directory: appDirectory,
+        configPath: joinPath(appDirectory, 'shopify.app.toml'),
+        clientId: 'test-client-id',
+        clientIdSource: 'config',
+        scanDirectories: [{directory: appDirectory, origin: 'app-directory'}],
       })
-      expect(output.engine).toMatchObject({name: 'shopify-app-security'})
-      await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministic_findings)
+      await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministicFindings)
       await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
         schema_version: 1,
         source: 'deterministic',
@@ -141,7 +176,7 @@ describe('app security check command boundary', () => {
       ])
 
       expect(result.exitCode).toBe(0)
-      expect(JSON.parse(result.stdout).agent_checks_path).toBe(paths.agentChecksPath)
+      expect(JSON.parse(result.stdout).agentChecksPath).toBe(paths.agentChecksPath)
       await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({source: 'deterministic'})
       await expect(
         readFile(appSecurityArtifactPaths(appDirectory, 'shopify.app').agentChecksPath),
@@ -170,10 +205,10 @@ describe('app security check command boundary', () => {
 
       expect(result.exitCode).toBe(0)
       expect(JSON.parse(result.stdout).selection).toMatchObject({
-        app_directory: appDirectory,
-        app_config_file: null,
-        client_id: 'configless-client-id',
-        client_id_source: 'flag',
+        directory: appDirectory,
+        configPath: null,
+        clientId: 'configless-client-id',
+        clientIdSource: 'flag',
       })
       const deterministicFindings = await readJson(paths.deterministicFindingsPath)
       expect(deterministicFindings).toMatchObject({
@@ -262,4 +297,75 @@ describe('app security check command boundary', () => {
       await expect(readFile(paths.deterministicFindingsPath)).rejects.toMatchObject({code: 'ENOENT'})
     })
   })
+
+  test.each([[['--skip-instructions']], [['--list-files']]])(
+    'aborts on an unknown --client-id before writing or listing anything (%j)',
+    async (modeFlags) => {
+      await inTemporaryDirectory(async (directory) => {
+        await createApp(directory)
+        const paths = appSecurityArtifactPaths(await fileRealPath(directory), 'unknown-client-id')
+        vi.mocked(appFromIdentifiers).mockRejectedValue(new AbortError('No app with client ID unknown-client-id found'))
+
+        const result = await runCommand(['--path', directory, '--client-id', 'unknown-client-id', ...modeFlags])
+
+        expect(result.exitCode).toBe(1)
+        expect(errorText(result.stderr)).toContain('No app with client ID unknown-client-id found')
+        expect(result.stdout).toBe('')
+        expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'unknown-client-id', offerReset: false})
+        await expect(fileExists(paths.resultsDirectory)).resolves.toBe(false)
+      })
+    },
+  )
+
+  test('looks up an empty --client-id= before listing anything', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      vi.mocked(appFromIdentifiers).mockRejectedValue(new AbortError('No app with client ID found'))
+
+      const result = await runCommand(['--path', directory, '--client-id=', '--list-files'])
+
+      expect(result.exitCode).toBe(1)
+      expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: '', offerReset: false})
+      expect(result.stdout).toBe('')
+    })
+  })
+
+  test('looks up --client-id, not the TOML client ID', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+
+      await runCommand(['--path', directory, '--client-id', 'other-client-id', '--json', '--skip-instructions'])
+      await runCommand(['--path', directory, '--json', '--skip-instructions'])
+
+      expect(appFromIdentifiers).toHaveBeenCalledOnce()
+      expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'other-client-id', offerReset: false})
+    })
+  })
+})
+
+describe('app security instructions command boundary', () => {
+  test('prints only the JSON result with --json --write, and writes the same instructions to the file', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      // instructions needs the results directory that check creates.
+      await runCommand(['--path', directory, '--json', '--skip-instructions'])
+      const instructionsPath = joinPath(directory, 'handoff.md')
+
+      const result = await runCommand(
+        ['--path', directory, '--json', '--write', instructionsPath],
+        SecurityInstructions,
+      )
+
+      expect(result.exitCode).toBe(0)
+      const output = JSON.parse(result.stdout)
+      expect(securityInstructionsJsonOutputSchema.validate(output)).toEqual(output)
+      expect(output.instructions).toEqual({
+        content: expect.stringContaining('Run the scan'),
+        copiedToClipboard: false,
+        path: instructionsPath,
+      })
+      await expect(readFile(instructionsPath, 'utf8')).resolves.toBe(`${output.instructions.content}\n`)
+      expect(result.stderr).not.toContain('Wrote app security check instructions')
+    })
+  })
 })