Prevent app security rescans from dropping compiled findings

.changeset/safe-traces-submit.md

@@ -0,0 +1,5 @@
+---
+'@shopify/app': patch
+---
+
+Prevent new App Security scans from replacing local agent review work without `--clean`.

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

@@ -0,0 +1,219 @@
+import SecurityCheck from './check.js'
+import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
+import {Config} from '@oclif/core'
+import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
+import {unstyled} from '@shopify/cli-kit/node/output'
+import {joinPath} from '@shopify/cli-kit/node/path'
+import {describe, expect, test, vi} from 'vitest'
+import {mkdir, readFile, rm, writeFile} from 'node:fs/promises'
+// eslint-disable-next-line n/prefer-global/console
+import {Console} from 'node:console'
+
+// Exercise actual stdout/stderr instead of CLI-kit's unit-test log collector.
+vi.mock('@shopify/cli-kit/node/context/local', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@shopify/cli-kit/node/context/local')>()),
+  isUnitTest: () => false,
+  isDevelopment: () => true,
+}))
+// Command lifecycle telemetry is unrelated to scanning. Keep the real error handler and renderer.
+vi.mock('@shopify/cli-kit/node/analytics', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@shopify/cli-kit/node/analytics')>()),
+  reportAnalyticsEvent: vi.fn(),
+}))
+vi.mock('@shopify/cli-kit/node/session', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@shopify/cli-kit/node/session')>()),
+  setCurrentSessionAlias: vi.fn(),
+}))
+
+interface ReviewPack {
+  source_scan_id: string
+  checks: {id: string; version: number; prompt_hash: string}[]
+}
+
+interface Trace {
+  findings: {source: string; check_id?: string}[]
+  checks_executed: {kind: string; id: string; status: string}[]
+}
+
+const reviewedCheckId = 'MISSING_TENANT_ISOLATION'
+
+async function createApp(directory: string): Promise<{nestedDirectory: string}> {
+  const routesDirectory = joinPath(directory, 'app', 'routes')
+  await mkdir(routesDirectory, {recursive: true})
+  await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Test app"\nclient_id = "test"\n')
+  await writeFile(
+    joinPath(directory, 'package.json'),
+    '{"name":"test-app","dependencies":{"@shopify/shopify-app-react-router":"1.0.0"}}\n',
+  )
+  await writeFile(joinPath(directory, 'app', 'shopify.server.ts'), 'export const shopify = {}\n')
+  await writeFile(joinPath(routesDirectory, 'index.ts'), 'export const loader = () => ({ok: true})')
+  return {nestedDirectory: routesDirectory}
+}
+
+async function readReviewPack(reviewPath: string): Promise<ReviewPack> {
+  return JSON.parse(await readFile(reviewPath, 'utf8')) as ReviewPack
+}
+
+function findingsDocument(reviewPack: ReviewPack): string {
+  const check = reviewPack.checks.find((entry) => entry.id === reviewedCheckId)
+  if (!check) throw new Error(`Missing review pack check ${reviewedCheckId}`)
+  const identity = {check_id: check.id, check_version: check.version, prompt_hash: check.prompt_hash}
+  return `${JSON.stringify({
+    schema_version: 1,
+    source_scan_id: reviewPack.source_scan_id,
+    checks_executed: [{...identity, status: 'executed', inspected_files: ['app/routes/index.ts']}],
+    findings: [
+      {
+        ...identity,
+        file: 'app/routes/index.ts',
+        line: 1,
+        message: 'The query is not scoped to the current shop.',
+        evidence: [{file: 'app/routes/index.ts', line: 1, quote: 'loader'}],
+      },
+    ],
+  })}\n`
+}
+
+// Byte snapshot of every local artifact so a refused scan can be proven to leave them untouched.
+async function artifactBytes(paths: Record<'tracePath' | 'reviewPath' | 'findingsPath' | 'submissionPath', string>) {
+  const readOrMissing = async (path: string) => readFile(path).catch(() => 'missing')
+  return {
+    trace: await readOrMissing(paths.tracePath),
+    review: await readOrMissing(paths.reviewPath),
+    findings: await readOrMissing(paths.findingsPath),
+    submission: await readOrMissing(paths.submissionPath),
+  }
+}
+
+function errorText(stderr: string): string {
+  return unstyled(stderr).replaceAll('│', '').replace(/\s+/g, ' ')
+}
+
+// The error box wraps long paths across lines, so compare them with whitespace removed.
+function expectMentionsPath(message: string, path: string): void {
+  expect(message.replaceAll(' ', '')).toContain(path)
+}
+
+async function runCommand(argv: string[]) {
+  let stdout = ''
+  let stderr = ''
+  const previousExitCode = process.exitCode
+  process.exitCode = 0
+  const out = vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => {
+    stdout += chunk.toString()
+    return true
+  })
+  const err = vi.spyOn(process.stderr, 'write').mockImplementation((chunk) => {
+    stderr += chunk.toString()
+    return true
+  })
+  // Vitest intercepts console.warn; use Node's console to exercise the captured streams.
+  const warn = vi.spyOn(console, 'warn').mockImplementation(new Console(process.stdout, process.stderr).warn)
+  // Observe the real Oclif error handler's requested exit without terminating the test worker.
+  const exit = vi.spyOn(process, 'exit').mockImplementation((code) => {
+    process.exitCode = code ?? 0
+    return undefined as never
+  })
+  try {
+    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)
+    return {stdout, stderr, exitCode: process.exitCode}
+  } finally {
+    warn.mockRestore()
+    out.mockRestore()
+    err.mockRestore()
+    exit.mockRestore()
+    process.exitCode = previousExitCode
+  }
+}
+
+describe('app security check command boundary', () => {
+  test('refuses a plain scan once findings from a custom path are compiled, even after that file is gone', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await inTemporaryDirectory(async (findingsDirectory) => {
+        const {nestedDirectory} = await createApp(directory)
+        const paths = appSecurityArtifactPaths(directory)
+
+        const scan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
+        expect(scan.exitCode).toBe(0)
+        expect(JSON.parse(scan.stdout)).toMatchObject({operation: 'scan'})
+
+        const customFindingsPath = joinPath(findingsDirectory, 'agent-findings.json')
+        await writeFile(customFindingsPath, findingsDocument(await readReviewPack(paths.reviewPath)))
+        const compile = await runCommand([
+          '--path',
+          directory,
+          '--findings',
+          customFindingsPath,
+          '--json',
+          '--skip-instructions',
+        ])
+        expect(compile.exitCode).toBe(0)
+        expect(JSON.parse(compile.stdout)).toMatchObject({operation: 'compile'})
+        const trace = JSON.parse(await readFile(paths.tracePath, 'utf8')) as Trace
+        expect(trace.findings).toContainEqual(expect.objectContaining({source: 'agent', check_id: reviewedCheckId}))
+        expect(trace.checks_executed).toContainEqual(
+          expect.objectContaining({kind: 'agent', id: reviewedCheckId, status: 'executed'}),
+        )
+
+        await rm(customFindingsPath)
+        await expect(readFile(paths.findingsPath)).rejects.toMatchObject({code: 'ENOENT'})
+        await writeFile(paths.submissionPath, '{"sentinel":"submission"}\n')
+        const before = await artifactBytes(paths)
+
+        const refused = await runCommand(['--path', nestedDirectory, '--skip-instructions'])
+        expect(refused.exitCode).toBe(1)
+        expect(refused.stdout).toBe('')
+        const message = errorText(refused.stderr)
+        expect(message).toContain('App Security did not start a new scan.')
+        expect(message).toContain('The existing trace contains agent review results:')
+        expectMentionsPath(message, paths.tracePath)
+        expect(message).toContain('--clean')
+        await expect(artifactBytes(paths)).resolves.toEqual(before)
+        expect(before.findings).toBe('missing')
+      })
+    })
+  })
+
+  test('refuses a plain scan while default agent findings are pending', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const {nestedDirectory} = await createApp(directory)
+      const paths = appSecurityArtifactPaths(directory)
+
+      const scan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
+      expect(scan.exitCode).toBe(0)
+      expect(JSON.parse(scan.stdout)).toMatchObject({operation: 'scan'})
+
+      await writeFile(paths.findingsPath, findingsDocument(await readReviewPack(paths.reviewPath)))
+      await writeFile(paths.submissionPath, '{"sentinel":"submission"}\n')
+      const before = await artifactBytes(paths)
+
+      const refused = await runCommand(['--path', nestedDirectory, '--skip-instructions'])
+      expect(refused.exitCode).toBe(1)
+      expect(refused.stdout).toBe('')
+      const message = errorText(refused.stderr)
+      expect(message).toContain('App Security did not start a new scan.')
+      expect(message).toContain('Agent findings exist at:')
+      expectMentionsPath(message, paths.findingsPath)
+      expect(message).toContain('--findings')
+      expect(message).toContain('--clean')
+      await expect(artifactBytes(paths)).resolves.toEqual(before)
+    })
+  })
+
+  test('rejects --clean together with --findings before touching the app', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      const paths = appSecurityArtifactPaths(directory)
+
+      const result = await runCommand(['--path', directory, '--clean', '--findings', 'findings.json'])
+
+      expect(result.exitCode).toBe(2)
+      expect(result.stderr).toContain('--findings')
+      expect(result.stderr).toContain('--clean')
+      await expect(readFile(paths.tracePath)).rejects.toMatchObject({code: 'ENOENT'})
+    })
+  })
+})

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

@@ -33,6 +33,7 @@ describe('app security check command', () => {
       yes: false,
       skipInstructions: true,
       findingsPath: undefined,
+      clean: false,
     })
   })
 
@@ -48,6 +49,7 @@ describe('app security check command', () => {
       yes: true,
       skipInstructions: false,
       findingsPath: undefined,
+      clean: false,
     })
   })
 
@@ -60,6 +62,33 @@ describe('app security check command', () => {
     expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({configName: 'staging', skipInstructions: true}))
   })
 
+  test('forwards --clean and keeps it mutually exclusive with --findings', async () => {
+    await SecurityCheck.run(['--clean', '--skip-instructions'], import.meta.url)
+
+    expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({clean: true, findingsPath: undefined}))
+    expect(SecurityCheck.flags.clean.exclusive).toEqual(['findings'])
+  })
+
+  test.each(['true', 'false'])(
+    'ignores an inherited SHOPIFY_FLAG_APP_SECURITY_CLEAN=%s so a plain scan stays non-destructive',
+    async (inheritedValue) => {
+      vi.stubEnv('SHOPIFY_FLAG_APP_SECURITY_CLEAN', inheritedValue)
+      try {
+        expect(SecurityCheck.flags.clean).not.toHaveProperty('env')
+
+        await SecurityCheck.run(['--skip-instructions'], import.meta.url)
+        expect(securityCheck).toHaveBeenLastCalledWith(expect.objectContaining({clean: false}))
+
+        await SecurityCheck.run(['--findings', './findings.json', '--skip-instructions'], import.meta.url)
+        expect(securityCheck).toHaveBeenLastCalledWith(
+          expect.objectContaining({clean: false, findingsPath: resolvePath('./findings.json')}),
+        )
+      } finally {
+        vi.unstubAllEnvs()
+      }
+    },
+  )
+
   test('resolves and forwards an agent findings file', async () => {
     await SecurityCheck.run(['--findings', './findings.json', '--skip-instructions'], import.meta.url)
 
@@ -69,12 +98,14 @@ describe('app security check command', () => {
   test('describes --yes as printing instructions and keeps it mutually exclusive with --skip-instructions', () => {
     expect(SecurityCheck.flags.yes.description).toBe('Print coding-agent instructions without prompting.')
     expect(SecurityCheck.flags['skip-instructions'].description).toBe("Don't offer to show coding-agent instructions.")
+    expect(SecurityCheck.flags.clean.description).toBe('Discard the current local review and start a new scan.')
     expect(SecurityCheck.flags.yes.exclusive).toEqual(['skip-instructions'])
     expect(SecurityCheck.flags['skip-instructions'].exclusive).toEqual(['yes'])
     expect(SecurityCheck.descriptionWithMarkdown).toContain('copy the coding-agent instructions')
     expect(SecurityCheck.descriptionWithMarkdown).toContain('`--config`')
     expect(SecurityCheck.descriptionWithMarkdown).toContain('copying is the default')
     expect(SecurityCheck.descriptionWithMarkdown).toContain('shopify app security instructions')
+    expect(SecurityCheck.descriptionWithMarkdown).toContain('Pass `--clean` to discard that work and start over')
   })
 
   test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => {

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

@@ -15,7 +15,9 @@ export default class SecurityCheck extends BaseCommand {
 
   static descriptionWithMarkdown = `Runs Shopify App Security locally and creates its review pack and trace.
 
-Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. Use \`--config\` to select a specific app configuration when the project has multiple \`shopify.app*.toml\` files; App Security inspects only that configuration. In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app security instructions\` to print, copy, or write them later.`
+Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass \`--clean\` to discard that work and start over. Use \`--config\` to select a specific app configuration when the project has multiple \`shopify.app*.toml\` files; App Security inspects only that configuration.
+
+In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app security instructions\` to print, copy, or write them later.`
 
   static description = this.descriptionWithoutMarkdown()
 
@@ -28,6 +30,15 @@ Pass \`--findings\` after completing the review pack to validate agent findings
       description: 'Validate agent findings from a JSON file and compile them into the trace.',
       parse: async (input) => resolvePath(input),
       env: 'SHOPIFY_FLAG_APP_SECURITY_FINDINGS',
+      exclusive: ['clean'],
+    }),
+    // Deliberately not bound to an environment variable: clean discards local review work, so it must be an
+    // explicit per-invocation decision rather than something inherited from a shell or CI environment.
+    // eslint-disable-next-line @shopify/cli/command-flags-with-env
+    clean: Flags.boolean({
+      description: 'Discard the current local review and start a new scan.',
+      default: false,
+      exclusive: ['findings'],
     }),
     blocking: Flags.string({
       description: 'The minimum finding severity that causes a non-zero exit code.',
@@ -61,6 +72,7 @@ Pass \`--findings\` after completing the review pack to validate agent findings
       yes: flags.yes,
       skipInstructions: flags['skip-instructions'],
       findingsPath: flags.findings,
+      clean: flags.clean,
     })
   }
 }

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

@@ -145,6 +145,8 @@ describe('app security submit command boundary', () => {
     await inTemporaryDirectory(async (directory) => {
       const client = remoteClient()
       const paths = await writeApp(directory)
+      const compiledTrace = await readFile(paths.tracePath)
+      expect(submissionTraceFixture.findings.some((finding) => finding.source === 'agent')).toBe(true)
       await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Unlinked app"\n')
       const result = await runCommand(['--path', directory, '--dry-run', ...(json ? ['--json'] : [])])
 
@@ -168,6 +170,7 @@ describe('app security submit command boundary', () => {
       expect(defaultDeveloperPlatformClient).not.toHaveBeenCalled()
       expect(client.appFromIdentifiers).not.toHaveBeenCalled()
       expect(fetch).not.toHaveBeenCalled()
+      await expect(readFile(paths.tracePath)).resolves.toEqual(compiledTrace)
       await expect(readdir(joinPath(directory, '.shopify'))).resolves.toEqual(['app-security'])
     })
   })
@@ -306,6 +309,8 @@ describe('app security submit command boundary', () => {
     await inTemporaryDirectory(async (directory) => {
       remoteClient()
       const paths = await writeApp(directory)
+      const compiledTrace = await readFile(paths.tracePath)
+      expect(submissionTraceFixture.findings.some((finding) => finding.source === 'agent')).toBe(true)
       const result = await runCommand(['--path', directory, '--json', '--force'])
       const submission = JSON.parse(await readFile(paths.submissionPath, 'utf8'))
       expect(JSON.parse(result.stdout)).toEqual({
@@ -317,6 +322,7 @@ describe('app security submit command boundary', () => {
       })
       expect(submission).not.toHaveProperty('client_id')
       expect(submission.report).not.toHaveProperty('client_id')
+      await expect(readFile(paths.tracePath)).resolves.toEqual(compiledTrace)
       expect(result.stderr).toBe('')
       expect(result.exitCode).toBe(0)
     })

packages/app/src/cli/services/app-security-api.test.ts

@@ -5,10 +5,10 @@ import {
   resolveAppSecurityRoot,
   type AppSecurityBlockingLevel,
 } from './app-security-api.js'
-import {writeAppSecurityArtifacts} from './app-security-artifacts.js'
+import {appSecurityArtifactPaths, readTrace, writeAppSecurityArtifacts} from './app-security-artifacts.js'
 import securityCheck from './security-check.js'
 import {AbortError} from '@shopify/cli-kit/node/error'
-import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs'
+import {fileExists, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs'
 import {joinPath} from '@shopify/cli-kit/node/path'
 import {describe, expect, test, vi} from 'vitest'
 import {symlink} from 'node:fs/promises'
@@ -612,10 +612,14 @@ describe('App Security CLI integration', () => {
           blocking: 'none',
           yes: false,
           skipInstructions: true,
+          clean: false,
         },
         {
-          execute: async ({directory: appDirectory, findingsPath}) => {
-            const appRoot = resolveAppSecurityRoot(appDirectory)
+          resolveRoot: resolveAppSecurityRoot,
+          artifactPaths: appSecurityArtifactPaths,
+          findingsFileExists: fileExists,
+          readTrace,
+          execute: async ({appRoot, findingsPath}) => {
             const findings = findingsPath ? await loadAppSecurityFindings(findingsPath) : undefined
             return executeAppSecurity({appRoot, findings})
           },