diff --git a/packages/app/src/cli/services/app-doctor-api.test.ts b/packages/app/src/cli/services/app-doctor-api.test.ts index f7835871ac3..ed724eaf87b 100644 --- a/packages/app/src/cli/services/app-doctor-api.test.ts +++ b/packages/app/src/cli/services/app-doctor-api.test.ts @@ -7,7 +7,6 @@ import { } from './app-doctor-api.js' import {writeAppDoctorArtifacts} from './app-doctor-artifacts.js' import doctor from './doctor.js' -import {loadChecks} from './app-doctor-engine/index.js' import {AbortError} from '@shopify/cli-kit/node/error' import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' @@ -54,6 +53,14 @@ async function sourceScanId(directory: string): Promise { return execution.scan.scan.input_hash } +async function reviewCheck(directory: string, id: string) { + const execution = await executeAppDoctor({appRoot: resolveAppDoctorRoot(directory)}) + if (execution.operation !== 'scan') throw new Error('Expected a scan result') + const check = execution.reviewPack.checks.find((entry) => entry.id === id) + if (!check) throw new Error(`Missing review pack check ${id}`) + return {check, sourceScanId: execution.scan.scan.input_hash} +} + async function appFindingsPath(directory: string): Promise { const path = artifactPath(directory, 'findings.json') await mkdir(joinPath(directory, '.shopify', 'app-doctor')) @@ -84,13 +91,13 @@ describe('App Doctor CLI integration', () => { expect(review.schema_version).toBe(1) expect(review.source_scan_id).toBe(result.execution.scan.scan.input_hash) - expect(review.checks).toHaveLength(loadChecks().size) + expect(review.checks.length).toBeGreaterThan(0) expect(review.checks.every((check: {prompt: string}) => check.prompt.length > 0)).toBe(true) expect(trace.schema_version).toBe(2) expect(trace.engine.name).toBe('shopify-app-doctor') expect(result.engine).toEqual(trace.engine) expect(result.reviewPath).toBe(artifactPath(directory, 'review.json')) - expect(result.reviewCheckCount).toBe(loadChecks().size) + expect(result.reviewCheckCount).toBe(review.checks.length) expect(result.exitCode).toBe(0) }) }) @@ -108,7 +115,7 @@ describe('App Doctor CLI integration', () => { const review = JSON.parse(await readFile(artifactPath(directory, 'review.json'))) expect(review.instructions).not.toContain('expose secrets') - expect(review.checks).toHaveLength(loadChecks().size) + expect(review.checks.length).toBeGreaterThan(0) }) }) @@ -128,13 +135,13 @@ describe('App Doctor CLI integration', () => { test('marks an execution unresolved when its submitted finding is rejected', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) - const check = loadChecks().get('MISSING_TENANT_ISOLATION')! + const {check, sourceScanId: scanId} = await reviewCheck(directory, 'MISSING_TENANT_ISOLATION') const findingsPath = await appFindingsPath(directory) await writeFile( findingsPath, `${JSON.stringify({ schema_version: 1, - source_scan_id: await sourceScanId(directory), + source_scan_id: scanId, checks_executed: [ { check_id: check.id, @@ -182,13 +189,13 @@ describe('App Doctor CLI integration', () => { test('returns structured rejections for malformed finding field types', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) - const check = loadChecks().get('MISSING_TENANT_ISOLATION')! + const {check, sourceScanId: scanId} = await reviewCheck(directory, 'MISSING_TENANT_ISOLATION') const findingsPath = await appFindingsPath(directory) await writeFile( findingsPath, `${JSON.stringify({ schema_version: 1, - source_scan_id: await sourceScanId(directory), + source_scan_id: scanId, checks_executed: [ { check_id: check.id, @@ -230,14 +237,14 @@ describe('App Doctor CLI integration', () => { test('validates agent findings outside the app root and compiles them into the trace', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) - const check = loadChecks().get('MISSING_TENANT_ISOLATION')! + const {check, sourceScanId: scanId} = await reviewCheck(directory, 'MISSING_TENANT_ISOLATION') await inTemporaryDirectory(async (findingsDirectory) => { const findingsPath = joinPath(findingsDirectory, 'findings.json') await writeFile( findingsPath, `${JSON.stringify({ schema_version: 1, - source_scan_id: await sourceScanId(directory), + source_scan_id: scanId, checks_executed: [ { check_id: check.id, @@ -373,13 +380,13 @@ describe('App Doctor CLI integration', () => { test('keeps a check when inspected_files includes extra relative paths', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) - const check = loadChecks().get('MISSING_TENANT_ISOLATION')! + const {check, sourceScanId: scanId} = await reviewCheck(directory, 'MISSING_TENANT_ISOLATION') const findingsPath = await appFindingsPath(directory) await writeFile( findingsPath, `${JSON.stringify({ schema_version: 1, - source_scan_id: await sourceScanId(directory), + source_scan_id: scanId, checks_executed: [ { check_id: check.id, diff --git a/packages/app/src/cli/services/app-doctor-api.ts b/packages/app/src/cli/services/app-doctor-api.ts index d01c359269b..aa31d0facd6 100644 --- a/packages/app/src/cli/services/app-doctor-api.ts +++ b/packages/app/src/cli/services/app-doctor-api.ts @@ -1,69 +1,27 @@ import { AppRootDiscoveryError, - buildReviewPack, - compileTrace, - computeResultHash, - FINDINGS_SCHEMA_VERSION, + FindingsDocumentError, + compileFindings, findAppRoot, - getEngineVersion, - loadChecks, - mergeFindings, - scan, - searchBoundaryFiles, - validateAgentChecksExecuted, - type AgentFindingsDocument, - type CheckExecution, - type ReviewPack, - type ScanResult, + parseFindings, + scanApp, + type AppDoctorCompile, + type AppDoctorEngineMetadata, + type AppDoctorFindings, + type AppDoctorScan, + type FindingsDocument, type Severity, - type Suppression, - type TraceV2, } from './app-doctor-engine/index.js' import {AbortError} from '@shopify/cli-kit/node/error' import {fileSize, readFile} from '@shopify/cli-kit/node/fs' const MAX_FINDINGS_FILE_SIZE_BYTES = 5_000_000 -const SOURCE_SCAN_ID = /^sha256:[0-9a-f]{64}$/ -export interface AppDoctorEngineMetadata { - name: string - version: string - ruleset: string -} +export type {AppDoctorEngineMetadata, AppDoctorFindings} export type AppDoctorBlockingLevel = Severity | 'none' -export interface AppDoctorFindings { - accepted: number - rejected: string[] - warnings: string[] -} - -export type AppDoctorExecution = - | { - operation: 'scan' - appRoot: string - scan: ScanResult - trace: TraceV2 - reviewPack: ReviewPack - engine: AppDoctorEngineMetadata - elapsedMilliseconds: number - } - | { - operation: 'compile' - appRoot: string - scan: ScanResult - trace: TraceV2 - findings: AppDoctorFindings - engine: AppDoctorEngineMetadata - elapsedMilliseconds: number - } - -interface FindingsDocument extends AgentFindingsDocument { - schema_version: typeof FINDINGS_SCHEMA_VERSION - source_scan_id: string - suppressions?: Suppression[] -} +export type AppDoctorExecution = (AppDoctorScan | AppDoctorCompile) & {elapsedMilliseconds: number} const severityRank: Record = { high: 3, @@ -82,13 +40,6 @@ function shouldBlock(issues: {severity: Severity}[], blocking: AppDoctorBlocking return issues.some((issue) => severityRank[issue.severity] >= severityRank[blocking]) } -function checkIdFromRejection(message: string, knownCheckIds: Set): string | undefined { - const delimiter = message.indexOf(':') - if (delimiter <= 0) return undefined - const checkId = message.slice(0, delimiter) - return knownCheckIds.has(checkId) ? checkId : undefined -} - export async function loadAppDoctorFindings(path: string): Promise { let content: string try { @@ -115,33 +66,14 @@ export async function loadAppDoctorFindings(path: string): Promise { const startTime = Date.now() - const result = await scan(options.appRoot) - const elapsedMilliseconds = Date.now() - startTime - const engineVersion = getEngineVersion() - let agentChecksExecuted: CheckExecution[] = [] - let suppressions: Suppression[] = [] - - if (!options.findings) { - const reviewPack = buildReviewPack(engineVersion, result) - const trace = compileTrace(result, {engineVersion, agentChecksExecuted, suppressions}) - return { - operation: 'scan', - appRoot: options.appRoot, - scan: result, - trace, - reviewPack, - engine: trace.engine, - elapsedMilliseconds, - } - } - - const document = options.findings - const knownFiles = new Set(searchBoundaryFiles(result)) - const provenanceRejected = - document.source_scan_id === result.scan.input_hash - ? [] - : [`Findings source scan ${document.source_scan_id} does not match the current scan ${result.scan.input_hash}.`] - const executed = - provenanceRejected.length > 0 - ? {executions: [] as CheckExecution[], rejected: provenanceRejected, warnings: [] as string[]} - : validateAgentChecksExecuted(document, {detection: result.detection, knownFiles}) - const merged = - provenanceRejected.length > 0 - ? {accepted: 0, rejected: [] as string[]} - : mergeFindings(result.issues, document.findings, { - knownFiles, - executedChecks: new Set( - executed.executions - .filter((execution) => execution.status === 'executed' || execution.status === 'unresolved') - .map((execution) => execution.id), - ), - }) - const accepted = merged.accepted - const rejected = [...executed.rejected, ...merged.rejected] - const warnings = executed.warnings - const checks = loadChecks() - const knownCheckIds = new Set(checks.keys()) - const rejectedCheckIds = new Set( - rejected.flatMap((message) => { - const checkId = checkIdFromRejection(message, knownCheckIds) - return checkId ? [checkId] : [] - }), - ) - agentChecksExecuted = executed.executions.map((execution) => - rejectedCheckIds.has(execution.id) - ? { - ...execution, - status: 'unresolved', - applicable: true, - reason: { - code: 'input_rejected', - message: `One or more submitted results for ${execution.id} were rejected.`, - }, - guidance: 'Correct the rejected check record or findings, then compile the trace again.', - } - : execution, - ) - for (const checkId of rejectedCheckIds) { - if (agentChecksExecuted.some((execution) => execution.id === checkId)) continue - const check = checks.get(checkId)! - agentChecksExecuted.push({ - id: check.id, - version: check.version, - kind: 'agent', - status: 'unresolved', - required: false, - applicable: true, - languages: result.detection.languages.map((language) => language.name), - framework: result.detection.framework, - surface: result.detection.surface, - inspected_files: [], - findings: 0, - analysis_mode: 'agent', - reason: {code: 'input_rejected', message: `The submitted execution or findings for ${check.id} were rejected.`}, - prompt: check.prompt, - prompt_hash: check.prompt_hash, - guidance: 'Correct the rejected check record or findings, then compile the trace again.', - }) - } - // Stale documents must not suppress current findings or crash compile when fingerprints no longer exist. - suppressions = provenanceRejected.length > 0 ? [] : (document.suppressions ?? []) - if (rejected.length > 0) { - result.score = null - result.scan.coverage_complete = false - result.scan.coverage_gaps.push( - ...rejected.map((message) => { - const checkId = checkIdFromRejection(message, knownCheckIds) - return { - code: 'unresolved_check' as const, - ...(checkId ? {check_id: checkId} : {}), - message: `Rejected agent result: ${message}`, - } - }), - ) - } - result.scan.result_hash = computeResultHash(result.issues, result.score) - - const trace = compileTrace(result, {engineVersion, agentChecksExecuted, suppressions}) + const result = options.findings + ? await compileFindings(options.appRoot, options.findings) + : await scanApp(options.appRoot) return { - operation: 'compile', - appRoot: options.appRoot, - scan: result, - trace, - findings: {accepted, rejected, warnings}, - engine: trace.engine, - elapsedMilliseconds, + ...result, + elapsedMilliseconds: Date.now() - startTime, } } 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 6ba24ee77ba..16d19ec26fa 100644 --- a/packages/app/src/cli/services/app-doctor-artifacts.test.ts +++ b/packages/app/src/cli/services/app-doctor-artifacts.test.ts @@ -1,30 +1,9 @@ import {appDoctorArtifactPaths, readTrace, writeSubmission} from './app-doctor-artifacts.js' -import {sha256, SUBMISSION_SCHEMA_VERSION, type AppDoctorSubmission, type TraceV2} from './app-doctor-engine/index.js' +import {scanApp, SUBMISSION_SCHEMA_VERSION, type AppDoctorSubmission} from './app-doctor-engine/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' -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: {}}, @@ -46,8 +25,9 @@ describe('appDoctorArtifactPaths', () => { describe('readTrace', () => { test('returns a validated v2 trace', async () => { await inTemporaryDirectory(async (directory) => { + await writeFile(joinPath(directory, 'shopify.app.toml'), 'name = "Test"\nclient_id = "test"\n') + const {trace} = await scanApp(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}) diff --git a/packages/app/src/cli/services/app-doctor-artifacts.ts b/packages/app/src/cli/services/app-doctor-artifacts.ts index 6f4e51381f2..3e721b2a07f 100644 --- a/packages/app/src/cli/services/app-doctor-artifacts.ts +++ b/packages/app/src/cli/services/app-doctor-artifacts.ts @@ -1,4 +1,4 @@ -import {validateTrace, type TraceV2} from './app-doctor-engine/index.js' +import {parseTrace, type TraceV2} 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' @@ -151,12 +151,9 @@ export async function readTrace(path: string): Promise { 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} + const trace = parseTrace(parsed) + if (!trace.ok) return {status: 'invalid', errors: trace.errors} + return {status: 'ok', trace: trace.trace} } export async function writeSubmission(appRoot: string, bytes: Buffer): Promise { diff --git a/packages/app/src/cli/services/app-doctor-engine/index.ts b/packages/app/src/cli/services/app-doctor-engine/index.ts index 006325d15d0..d0294f610ff 100644 --- a/packages/app/src/cli/services/app-doctor-engine/index.ts +++ b/packages/app/src/cli/services/app-doctor-engine/index.ts @@ -1,64 +1,30 @@ -export {scan, DETERMINISTIC_CHECKS, DETERMINISTIC_RULES} from './scanners/index.js' -export type {DeterministicCheckDefinition} from './scanners/index.js' -export {AppRootDiscoveryError, findAppRoot} from './scanners/discover.js' -export {computeResultHash} from './scorer/index.js' -export {redactText} from './rules/secret-rules.js' -export {EMBEDDED_APP_DOCTOR_INSTRUCTIONS} from './checks/embedded.js' +/** + * Public App Doctor engine API. + * + * CLI code outside this directory should import only these operations and result + * types: locate an app, scan, parse/compile findings, parse a stored trace, and + * build a submission. Keep scanners, registries, merge helpers, and redaction + * inside the engine. + */ export { - buildReviewPack, - loadChecks, - mergeFindings, - searchBoundaryFiles, - validateFinding, - validateAgentChecksExecuted, -} from './checks/index.js' -export type {AgentFinding, AgentFindingsDocument, Check, ReviewPack} from './checks/index.js' -export {assertRegistryInvariants, getRegistry} from './registry/index.js' -export type {RegistryEntry} from './registry/index.js' -export { - compileTrace, - validateTrace, - validateSuppression, - assertCompatibleTrace, - isTraceSchemaVersionSupported, - canonicalJson, - findingFingerprint, - redactIssue, - sha256, -} from './trace/index.js' -export type {CompileTraceOptions, TraceValidationResult} from './trace/index.js' -export {mergeExternalFindings, validateExternalFinding} from './external/index.js' -export type {ExternalFinding} from './external/index.js' + AppRootDiscoveryError, + FindingsDocumentError, + compileFindings, + findAppRoot, + getAgentInstructions, + parseFindings, + parseTrace, + scanApp, +} from './run.js' +export type { + AppDoctorCompile, + AppDoctorEngineMetadata, + AppDoctorFindings, + AppDoctorScan, + FindingsDocument, + ParseTraceResult, +} from './run.js' export {buildSubmission, SUBMISSION_SCHEMA_VERSION} from './submission/index.js' export type {AppDoctorSubmission, AppDoctorSubmissionReport, BuildSubmissionOptions} from './submission/index.js' -export {formatJson, sortIssues} from './output/format.js' -export {ENGINE_NAME, FINDINGS_SCHEMA_VERSION, SUPPORTED_TRACE_SCHEMA_VERSIONS, TRACE_SCHEMA_VERSION} from './types.js' -export {getEngineVersion} from './version.js' -export type { - AnalysisMode, - Capabilities, - CheckExecution, - CheckExecutionReason, - CheckExecutionStatus, - Confidence, - CoverageGap, - DetectedFramework, - DetectedLanguage, - DetectedSurface, - FindingEvidence, - FindingSource, - Fix, - Grade, - Issue, - Location, - ScanMetadata, - ScanResult, - ScoreResult, - Severity, - SkippedFile, - Suppression, - SuppressionProvenance, - TraceFinding, - TraceV1, - TraceV2, -} from './types.js' +export type {ReviewPack} from './checks/index.js' +export type {Capabilities, Issue, ScanResult, Severity, TraceV2} from './types.js' diff --git a/packages/app/src/cli/services/app-doctor-engine/output/format.ts b/packages/app/src/cli/services/app-doctor-engine/output/format.ts index ebc6f426d7d..acc4bdd4de1 100644 --- a/packages/app/src/cli/services/app-doctor-engine/output/format.ts +++ b/packages/app/src/cli/services/app-doctor-engine/output/format.ts @@ -1,17 +1,6 @@ import {redactText} from '../rules/secret-rules.js' -import type {Issue, ScanResult, Severity} from '../types.js' - -const SEVERITY_ORDER: Record = {high: 3, medium: 2, low: 1} +import type {ScanResult} from '../types.js' export function formatJson(result: ScanResult): string { return JSON.stringify(result, (_key, value) => (typeof value === 'string' ? redactText(value) : value), 2) } - -export function sortIssues(issues: Issue[]): Issue[] { - return [...issues].sort((left, right) => { - const severityDifference = SEVERITY_ORDER[right.severity] - SEVERITY_ORDER[left.severity] - if (severityDifference !== 0) return severityDifference - const fileDifference = left.location.file.localeCompare(right.location.file) - return fileDifference === 0 ? (left.location.line ?? 0) - (right.location.line ?? 0) : fileDifference - }) -} diff --git a/packages/app/src/cli/services/app-doctor-engine/run.ts b/packages/app/src/cli/services/app-doctor-engine/run.ts new file mode 100644 index 00000000000..6e21c274f78 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/run.ts @@ -0,0 +1,233 @@ +import {EMBEDDED_APP_DOCTOR_INSTRUCTIONS} from './checks/embedded.js' +import { + buildReviewPack, + loadChecks, + mergeFindings, + searchBoundaryFiles, + validateAgentChecksExecuted, + type AgentFindingsDocument, + type ReviewPack, +} from './checks/index.js' +import {redactText} from './rules/secret-rules.js' +import {AppRootDiscoveryError, findAppRoot} from './scanners/discover.js' +import {scan} from './scanners/index.js' +import {computeResultHash} from './scorer/index.js' +import {compileTrace, validateTrace} from './trace/index.js' +import {FINDINGS_SCHEMA_VERSION} from './types.js' +import {getEngineVersion} from './version.js' +import type {CheckExecution, ScanResult, Suppression, TraceV2} from './types.js' + +export {AppRootDiscoveryError, findAppRoot} + +const SOURCE_SCAN_ID = /^sha256:[0-9a-f]{64}$/ + +export interface AppDoctorEngineMetadata { + name: string + version: string + ruleset: string +} + +export interface AppDoctorFindings { + accepted: number + rejected: string[] + warnings: string[] +} + +export interface FindingsDocument extends AgentFindingsDocument { + schema_version: typeof FINDINGS_SCHEMA_VERSION + source_scan_id: string + suppressions?: Suppression[] +} + +export interface AppDoctorScan { + operation: 'scan' + appRoot: string + scan: ScanResult + trace: TraceV2 + reviewPack: ReviewPack + engine: AppDoctorEngineMetadata +} + +export interface AppDoctorCompile { + operation: 'compile' + appRoot: string + scan: ScanResult + trace: TraceV2 + findings: AppDoctorFindings + engine: AppDoctorEngineMetadata +} + +export type ParseTraceResult = {ok: true; trace: TraceV2} | {ok: false; errors: string[]} + +/** Expected user error while reading an agent findings document. */ +export class FindingsDocumentError extends Error { + readonly tryMessage?: string + + constructor(message: string, tryMessage?: string) { + super(message) + this.name = 'FindingsDocumentError' + this.tryMessage = tryMessage + } +} + +export function getAgentInstructions(): string { + return EMBEDDED_APP_DOCTOR_INSTRUCTIONS +} + +export function parseTrace(value: unknown): ParseTraceResult { + const validation = validateTrace(value) + return validation.valid ? {ok: true, trace: value as TraceV2} : {ok: false, errors: validation.errors} +} + +export function parseFindings(value: unknown): FindingsDocument { + if (!value || typeof value !== 'object') { + throw new FindingsDocumentError('The App Doctor findings file must contain a JSON object.') + } + if (!('schema_version' in value) || value.schema_version !== FINDINGS_SCHEMA_VERSION) { + throw new FindingsDocumentError( + `The App Doctor findings file must use schema version ${FINDINGS_SCHEMA_VERSION}.`, + 'Generate a new review pack and use its findings schema.', + ) + } + if ( + !('source_scan_id' in value) || + typeof value.source_scan_id !== 'string' || + !SOURCE_SCAN_ID.test(value.source_scan_id) + ) { + throw new FindingsDocumentError( + 'The App Doctor findings file must identify its source scan.', + 'Copy the source_scan_id from the generated review.json.', + ) + } + if (!('findings' in value) || !Array.isArray(value.findings)) { + throw new FindingsDocumentError('The App Doctor findings file must contain a findings array.') + } + if ('suppressions' in value && value.suppressions !== undefined && !Array.isArray(value.suppressions)) { + throw new FindingsDocumentError('The App Doctor findings file suppressions field must be an array.') + } + + return value as FindingsDocument +} + +export async function scanApp(directory?: string): Promise { + const appRoot = findAppRoot(directory) + const result = await scan(appRoot) + const engineVersion = getEngineVersion() + const reviewPack = buildReviewPack(engineVersion, result) + const trace = compileTrace(result, {engineVersion, agentChecksExecuted: [], suppressions: []}) + return { + operation: 'scan', + appRoot, + scan: result, + trace, + reviewPack, + engine: trace.engine, + } +} + +export async function compileFindings(directory: string, document: FindingsDocument): Promise { + const appRoot = findAppRoot(directory) + const result = await scan(appRoot) + const engineVersion = getEngineVersion() + const knownFiles = new Set(searchBoundaryFiles(result)) + const provenanceRejected = + document.source_scan_id === result.scan.input_hash + ? [] + : [`Findings source scan ${document.source_scan_id} does not match the current scan ${result.scan.input_hash}.`] + const executed = + provenanceRejected.length > 0 + ? {executions: [] as CheckExecution[], rejected: provenanceRejected, warnings: [] as string[]} + : validateAgentChecksExecuted(document, {detection: result.detection, knownFiles}) + const merged = + provenanceRejected.length > 0 + ? {accepted: 0, rejected: [] as string[]} + : mergeFindings(result.issues, document.findings, { + knownFiles, + executedChecks: new Set( + executed.executions + .filter((execution) => execution.status === 'executed' || execution.status === 'unresolved') + .map((execution) => execution.id), + ), + }) + const accepted = merged.accepted + const rejected = [...executed.rejected, ...merged.rejected].map((message) => redactText(message)) + const warnings = executed.warnings.map((message) => redactText(message)) + const checks = loadChecks() + const knownCheckIds = new Set(checks.keys()) + const rejectedCheckIds = new Set( + rejected.flatMap((message) => { + const checkId = checkIdFromRejection(message, knownCheckIds) + return checkId ? [checkId] : [] + }), + ) + const agentChecksExecuted: CheckExecution[] = executed.executions.map((execution) => + rejectedCheckIds.has(execution.id) + ? { + ...execution, + status: 'unresolved', + applicable: true, + reason: { + code: 'input_rejected', + message: `One or more submitted results for ${execution.id} were rejected.`, + }, + guidance: 'Correct the rejected check record or findings, then compile the trace again.', + } + : execution, + ) + for (const checkId of rejectedCheckIds) { + if (agentChecksExecuted.some((execution) => execution.id === checkId)) continue + const check = checks.get(checkId)! + agentChecksExecuted.push({ + id: check.id, + version: check.version, + kind: 'agent', + status: 'unresolved', + required: false, + applicable: true, + languages: result.detection.languages.map((language) => language.name), + framework: result.detection.framework, + surface: result.detection.surface, + inspected_files: [], + findings: 0, + analysis_mode: 'agent', + reason: {code: 'input_rejected', message: `The submitted execution or findings for ${check.id} were rejected.`}, + prompt: check.prompt, + prompt_hash: check.prompt_hash, + guidance: 'Correct the rejected check record or findings, then compile the trace again.', + }) + } + // Stale documents must not suppress current findings or crash compile when fingerprints no longer exist. + const suppressions = provenanceRejected.length > 0 ? [] : (document.suppressions ?? []) + if (rejected.length > 0) { + result.score = null + result.scan.coverage_complete = false + result.scan.coverage_gaps.push( + ...rejected.map((message) => { + const checkId = checkIdFromRejection(message, knownCheckIds) + return { + code: 'unresolved_check' as const, + ...(checkId ? {check_id: checkId} : {}), + message: `Rejected agent result: ${message}`, + } + }), + ) + } + result.scan.result_hash = computeResultHash(result.issues, result.score) + + const trace = compileTrace(result, {engineVersion, agentChecksExecuted, suppressions}) + return { + operation: 'compile', + appRoot, + scan: result, + trace, + findings: {accepted, rejected, warnings}, + engine: trace.engine, + } +} + +function checkIdFromRejection(message: string, knownCheckIds: Set): string | undefined { + const delimiter = message.indexOf(':') + if (delimiter <= 0) return undefined + const checkId = message.slice(0, delimiter) + return knownCheckIds.has(checkId) ? checkId : undefined +} diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index 62191d00191..5f2399594f7 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -1,5 +1,6 @@ /* eslint-disable no-restricted-imports -- deterministic scanners use real temporary repositories */ -import {DETERMINISTIC_CHECKS, getRegistry} from '../index.js' +import {getRegistry} from '../registry/index.js' +import {DETERMINISTIC_CHECKS} from '../scanners/index.js' import {RULE_CATALOG} from '../rules/catalog.js' import {parseAppToml} from '../scanners/discover.js' import { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/registry.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/registry.test.ts index a2032857c2d..329c28b1e6e 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/registry.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/registry.test.ts @@ -1,4 +1,6 @@ -import {DETERMINISTIC_RULES, getRegistry, loadChecks} from '../index.js' +import {loadChecks} from '../checks/index.js' +import {getRegistry} from '../registry/index.js' +import {DETERMINISTIC_RULES} from '../scanners/index.js' import {RULE_CATALOG} from '../rules/catalog.js' import {describe, expect, test} from 'vitest' diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts index c494c1f519e..8e8fd377d33 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/scan-contract.test.ts @@ -1,14 +1,8 @@ /* eslint-disable no-restricted-imports -- detector coverage uses real temporary repositories */ -import { - DETERMINISTIC_CHECKS, - assertRegistryInvariants, - buildReviewPack, - compileTrace, - scan, - getRegistry, - sha256, - validateTrace, -} from '../index.js' +import {buildReviewPack} from '../checks/index.js' +import {assertRegistryInvariants, getRegistry} from '../registry/index.js' +import {DETERMINISTIC_CHECKS, scan} from '../scanners/index.js' +import {compileTrace, sha256, validateTrace} from '../trace/index.js' import {RULE_CATALOG} from '../rules/catalog.js' import {calculateScore} from '../scorer/index.js' import {afterEach, describe, expect, test} from 'vitest' 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 index 9aec1b1099c..66d18ba6475 100644 --- 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 @@ -1,14 +1,10 @@ import {submissionTraceFixture, submissionTraceHashes} from './fixtures/submission-trace.js' -import { - buildSubmission, - SUBMISSION_SCHEMA_VERSION, - validateTrace, - type AppDoctorSubmission, - type BuildSubmissionOptions, -} from '../index.js' +import {buildSubmission, SUBMISSION_SCHEMA_VERSION} 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, BuildSubmissionOptions} from '../submission/index.js' const fixturesDirectory = joinPath(moduleDirectory(import.meta.url), 'fixtures') diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts index ba268ddce30..68b0eab16e6 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/trace.test.ts @@ -1,22 +1,14 @@ /* eslint-disable no-restricted-imports -- trace fixtures use Node temporary-directory primitives */ import {computeResultHash} from '../scorer/index.js' -import { - compileTrace, - formatJson, - mergeExternalFindings, - scan, - sha256, - validateTrace, - validateExternalFinding, - validateSuppression, - type Issue, - type ScanResult, - type Suppression, -} from '../index.js' +import {mergeExternalFindings, validateExternalFinding} from '../external/index.js' +import {formatJson} from '../output/format.js' +import {scan} from '../scanners/index.js' +import {compileTrace, sha256, validateSuppression, validateTrace} from '../trace/index.js' import {afterEach, describe, expect, test} from 'vitest' import {mkdtempSync, rmSync, writeFileSync} from 'node:fs' import {tmpdir} from 'node:os' import {join} from 'node:path' +import type {Issue, ScanResult, Suppression} from '../types.js' const dirs: string[] = [] afterEach(() => dirs.splice(0).forEach((dir) => rmSync(dir, {recursive: true, force: true}))) diff --git a/packages/app/src/cli/services/app-doctor-engine/types.ts b/packages/app/src/cli/services/app-doctor-engine/types.ts index 0482c9e9fd2..17934439ddb 100644 --- a/packages/app/src/cli/services/app-doctor-engine/types.ts +++ b/packages/app/src/cli/services/app-doctor-engine/types.ts @@ -180,45 +180,6 @@ export interface ScanMetadata { checks_executed: CheckExecution[] } -/** Trace v1 is retained as a legacy type. Its shape is intentionally frozen. */ -export interface TraceV1 { - schema_version: 1 - engine: { - name: typeof ENGINE_NAME - version: string - ruleset: string - } - generated_at: string - project: { - commit: string | null - dirty: boolean | null - input_hash: string - input_hashes: Record - } - findings: TraceFinding[] - checks_executed: LegacyCheckExecution[] - suppressions: Suppression[] - coverage: { - files_scanned: number - files_skipped: SkippedFile[] - complete: boolean - } - attestation: { - digest: string - signed: false - } -} - -interface LegacyCheckExecution { - id: string - version: number - kind: 'rule' | 'check' | 'external' - status: 'executed' | 'skipped' - findings: number - prompt_hash?: string - reason?: string -} - export const TRACE_SCHEMA_VERSION = 2 as const export const FINDINGS_SCHEMA_VERSION = 1 as const export const SUPPORTED_TRACE_SCHEMA_VERSIONS = [TRACE_SCHEMA_VERSION] as const diff --git a/packages/app/src/cli/services/app-doctor-instructions.test.ts b/packages/app/src/cli/services/app-doctor-instructions.test.ts index 978c304cc67..6011dffacdf 100644 --- a/packages/app/src/cli/services/app-doctor-instructions.test.ts +++ b/packages/app/src/cli/services/app-doctor-instructions.test.ts @@ -1,5 +1,5 @@ import deliverAppDoctorInstructions, {appDoctorInstructions, shellQuote} from './app-doctor-instructions.js' -import {EMBEDDED_APP_DOCTOR_INSTRUCTIONS} from './app-doctor-engine/index.js' +import {getAgentInstructions} from './app-doctor-engine/index.js' import {AbortError} from '@shopify/cli-kit/node/error' import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath, normalizePath} from '@shopify/cli-kit/node/path' @@ -24,7 +24,7 @@ async function createApp(directory: string): Promise { describe('embedded instructions', () => { test('matches INSTRUCTIONS.md', () => { const source = readFileSync(fileURLToPath(new URL('./app-doctor-engine/INSTRUCTIONS.md', import.meta.url)), 'utf8') - expect(EMBEDDED_APP_DOCTOR_INSTRUCTIONS).toBe(source) + expect(getAgentInstructions()).toBe(source) }) }) diff --git a/packages/app/src/cli/services/app-doctor-instructions.ts b/packages/app/src/cli/services/app-doctor-instructions.ts index b35e32e758a..2efdd6a5008 100644 --- a/packages/app/src/cli/services/app-doctor-instructions.ts +++ b/packages/app/src/cli/services/app-doctor-instructions.ts @@ -6,7 +6,7 @@ import { shellForPlatform, type AppDoctorCommands, } from './app-doctor-commands.js' -import {EMBEDDED_APP_DOCTOR_INSTRUCTIONS} from './app-doctor-engine/index.js' +import {getAgentInstructions} from './app-doctor-engine/index.js' import {writeFile} from '@shopify/cli-kit/node/fs' import {outputResult} from '@shopify/cli-kit/node/output' import {joinPath, resolvePath} from '@shopify/cli-kit/node/path' @@ -109,7 +109,8 @@ export function appDoctorInstructions(options: { }): string { const paths = instructionPaths(options.directory, options.commands) const scanContext = options.scanComplete ? completedScanInstructions(paths) : initialScanInstructions(paths) - return EMBEDDED_APP_DOCTOR_INSTRUCTIONS.replace(SCAN_CONTEXT_PLACEHOLDER, scanContext) + return getAgentInstructions() + .replace(SCAN_CONTEXT_PLACEHOLDER, scanContext) .replaceAll('{{SCAN_COMMAND}}', paths.scanCommand) .replaceAll('{{COMPILE_COMMAND}}', paths.compileCommand) .replaceAll('{{REVIEW_PATH}}', markdownPath(paths.reviewPath)) diff --git a/packages/app/src/cli/services/doctor-output.test.ts b/packages/app/src/cli/services/doctor-output.test.ts index dc002689d7f..fc9d412bab3 100644 --- a/packages/app/src/cli/services/doctor-output.test.ts +++ b/packages/app/src/cli/services/doctor-output.test.ts @@ -245,22 +245,25 @@ describe('buildDoctorAlert', () => { expect(serialized).toContain('ignored inspected file outside the scanned inputs: vitest.config.ts') }) - test('redacts secrets from titles, paths, and verbose evidence', () => { - const secret = `shpat_${'a'.repeat(24)}` + test('renders engine-redacted titles, paths, and verbose evidence unchanged', () => { const serialized = JSON.stringify( buildDoctorAlert( reportInput({ verbose: true, scan: { ...scanWithIssues, - app: {name: `app ${secret}`, type: 'public'}, + app: {name: 'app [REDACTED: Shopify token]', type: 'public'}, issues: [ { ...scanWithIssues.issues[0]!, - title: `title ${secret}`, - message: `message ${secret}`, - snippet: `snippet ${secret}`, - fix: {automated: false, description: `fix ${secret}`, guide: `https://example.com/${secret}`}, + title: 'title [REDACTED: Shopify token]', + message: 'message [REDACTED: Shopify token]', + snippet: 'snippet [REDACTED: Shopify token]', + fix: { + automated: false, + description: 'fix [REDACTED: Shopify token]', + guide: 'https://example.com/[REDACTED: Shopify token]', + }, }, ], }, @@ -268,7 +271,7 @@ describe('buildDoctorAlert', () => { ), ) - expect(serialized).not.toContain(secret) - expect(serialized).toContain('[REDACTED:') + expect(serialized).toContain('[REDACTED: Shopify token]') + expect(serialized).not.toContain('shpat_') }) }) diff --git a/packages/app/src/cli/services/doctor-output.ts b/packages/app/src/cli/services/doctor-output.ts index 0b94bb321ed..713c36a4094 100644 --- a/packages/app/src/cli/services/doctor-output.ts +++ b/packages/app/src/cli/services/doctor-output.ts @@ -1,14 +1,6 @@ -import { - redactIssue, - redactText, - sortIssues, - type Capabilities, - type Issue, - type ScanResult, - type Severity, -} from './app-doctor-engine/index.js' import {formatAppDoctorCommand, type AppDoctorCommands} from './app-doctor-commands.js' import {renderError, renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui' +import type {Capabilities, Issue, ScanResult, Severity} from './app-doctor-engine/index.js' import type {AlertCustomSection, InlineToken, RenderAlertOptions, Token, TokenItem} from '@shopify/cli-kit/node/ui' interface DoctorEngineMetadata { @@ -99,7 +91,7 @@ function doctorHeadline(input: DoctorReportInput): string { function doctorBody(input: DoctorReportInput): TokenItem { const scan = input.scan const tokens: Token[] = [ - {userInput: redactText(scan.app.name)}, + {userInput: scan.app.name}, {char: '.'}, `${scan.scan.files_scanned} files scanned in ${formatElapsed(input.elapsedMilliseconds)}.`, ] @@ -140,7 +132,7 @@ function doctorCustomSections(input: DoctorReportInput): AlertCustomSection[] { if (input.scan.scan.coverage_gaps.length > 0) { const gaps = input.scan.scan.coverage_gaps - const items: TokenItem[] = gaps.slice(0, 8).map((gap) => redactText(gap.message)) + const items: TokenItem[] = gaps.slice(0, 8).map((gap) => gap.message) if (gaps.length > 8) items.push({info: `${gaps.length - 8} more coverage gaps`}) sections.push({title: 'Coverage gaps', body: {list: {items}}}) } @@ -150,8 +142,8 @@ function doctorCustomSections(input: DoctorReportInput): AlertCustomSection[] { input.findings.accepted === 0 && input.findings.rejected.length > 0 ? 'No agent findings were merged.' : `Merged ${input.findings.accepted} agent finding(s) into the trace.`, - ...input.findings.rejected.map((reason) => ({error: `Rejected: ${redactText(reason)}`})), - ...(input.findings.warnings ?? []).map((reason) => ({warn: redactText(reason)})), + ...input.findings.rejected.map((reason) => ({error: `Rejected: ${reason}`})), + ...(input.findings.warnings ?? []).map((reason) => ({warn: reason})), ['Trace written to', {filePath: input.tracePath}], ] sections.push({title: 'Agent findings', body: {list: {items}}}) @@ -195,8 +187,7 @@ function doctorCustomSections(input: DoctorReportInput): AlertCustomSection[] { return sections } -function issueListItem(issueInput: Issue, verbose: boolean): TokenItem { - const issue = redactIssue(issueInput) +function issueListItem(issue: Issue, verbose: boolean): TokenItem { const location = issue.location.line ? `${issue.location.file}:${issue.location.line}` : issue.location.file const item: InlineToken[] = [{bold: issue.title}, {subdued: issue.id}, {filePath: location}] @@ -215,6 +206,16 @@ function issueListItem(issueInput: Issue, verbose: boolean): TokenItem = {high: 3, medium: 2, low: 1} + return [...issues].sort((left, right) => { + const severityDifference = severityOrder[right.severity] - severityOrder[left.severity] + if (severityDifference !== 0) return severityDifference + const fileDifference = left.location.file.localeCompare(right.location.file) + return fileDifference === 0 ? (left.location.line ?? 0) - (right.location.line ?? 0) : fileDifference + }) +} + function groupIssuesBySeverity(issues: Issue[]): {severity: Severity; issues: Issue[]}[] { const groups: {severity: Severity; issues: Issue[]}[] = [] for (const issue of sortIssues(issues)) {