From e633fc362e132adc5b5a665ae3655d2a6fd45a07 Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Thu, 17 Sep 2026 13:01:14 -0500 Subject: [PATCH 1/2] Group repeated App Doctor findings in human-readable output Collapse the terminal report by rule, optional pattern, and severity. JSON, traces, scoring, and check counts still keep every occurrence. Co-authored-by: AI (Pi/Grok 4.6) --- .../cli/services/app-doctor-engine/index.ts | 6 +- .../output/group-issues.test.ts | 49 ++++++++++++ .../app-doctor-engine/output/group-issues.ts | 38 +++++++++ .../app-doctor-engine/rules/liquid-rules.ts | 4 + .../app-doctor-engine/rules/secret-rules.ts | 2 + .../tests/secret-safety.test.ts | 2 + .../cli/services/app-doctor-engine/types.ts | 2 + .../src/cli/services/doctor-output.test.ts | 79 ++++++++++++++++++ .../app/src/cli/services/doctor-output.ts | 80 +++++++++++-------- 9 files changed, 225 insertions(+), 37 deletions(-) create mode 100644 packages/app/src/cli/services/app-doctor-engine/output/group-issues.test.ts create mode 100644 packages/app/src/cli/services/app-doctor-engine/output/group-issues.ts 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 d0294f610ff..296f114db42 100644 --- a/packages/app/src/cli/services/app-doctor-engine/index.ts +++ b/packages/app/src/cli/services/app-doctor-engine/index.ts @@ -2,8 +2,8 @@ * 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 + * types: locate an app, scan, parse/compile findings, parse a stored trace, + * build a submission, and group issues for display. Keep scanners, registries, merge helpers, and redaction * inside the engine. */ export { @@ -27,4 +27,6 @@ export type { export {buildSubmission, SUBMISSION_SCHEMA_VERSION} from './submission/index.js' export type {AppDoctorSubmission, AppDoctorSubmissionReport, BuildSubmissionOptions} from './submission/index.js' export type {ReviewPack} from './checks/index.js' +export {groupIssues} from './output/group-issues.js' +export type {IssueGroup} from './output/group-issues.js' export type {Capabilities, Issue, ScanResult, Severity, TraceV2} from './types.js' diff --git a/packages/app/src/cli/services/app-doctor-engine/output/group-issues.test.ts b/packages/app/src/cli/services/app-doctor-engine/output/group-issues.test.ts new file mode 100644 index 00000000000..f0986733686 --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/output/group-issues.test.ts @@ -0,0 +1,49 @@ +import {groupIssues} from './group-issues.js' +import {describe, expect, test} from 'vitest' +import type {Issue} from '../types.js' + +function issue(overrides: Partial = {}): Issue { + return { + id: 'COMMITTED_SECRET', + pattern_id: 'Shopify token', + severity: 'high', + title: 'Hardcoded Shopify token detected', + message: 'A token was found.', + points: -50, + location: {file: 'app/a.ts', line: 1}, + fix: {automated: false, description: 'Rotate the token.'}, + ...overrides, + } +} + +describe('groupIssues', () => { + test('uses rule, pattern, and severity, not messages, titles, or locations', () => { + const issues = [issue(), issue({title: 'Updated wording', message: 'More detail', location: {file: 'app/b.ts'}})] + const before = structuredClone(issues) + const groups = groupIssues(issues) + + expect(groups).toHaveLength(1) + expect(groups[0]!.issues).toHaveLength(2) + expect(groups[0]!.files).toEqual(['app/a.ts', 'app/b.ts']) + expect(issues).toEqual(before) + }) + + test('keeps different rules, patterns, sources, and severities separate', () => { + expect( + groupIssues([ + issue(), + issue({id: 'ANOTHER_RULE'}), + issue({pattern_id: 'Private key'}), + issue({severity: 'low'}), + issue({found_by: 'agent'}), + issue({found_by: 'external'}), + ]), + ).toHaveLength(6) + }) + + test('preserves exactly repeated occurrences and handles empty input', () => { + expect(groupIssues([])).toEqual([]) + const repeated = issue() + expect(groupIssues([repeated, repeated])[0]!.issues).toHaveLength(2) + }) +}) diff --git a/packages/app/src/cli/services/app-doctor-engine/output/group-issues.ts b/packages/app/src/cli/services/app-doctor-engine/output/group-issues.ts new file mode 100644 index 00000000000..643f0c24f3f --- /dev/null +++ b/packages/app/src/cli/services/app-doctor-engine/output/group-issues.ts @@ -0,0 +1,38 @@ +import type {Issue, Severity} from '../types.js' + +export interface IssueGroup { + severity: Severity + issues: Issue[] + files: string[] +} + +const SEVERITY_ORDER: Record = {high: 0, medium: 1, low: 2} + +function groupKey(issue: Issue): string { + return [issue.found_by ?? 'static', issue.id, issue.pattern_id ?? '', issue.severity].join('|') +} + +/** A presentation view only: never replace scan issues or trace findings with these groups. */ +export function groupIssues(issues: Issue[]): IssueGroup[] { + const groups = new Map() + const sorted = [...issues].sort( + (left, right) => + SEVERITY_ORDER[left.severity] - SEVERITY_ORDER[right.severity] || + left.location.file.localeCompare(right.location.file) || + (left.location.line ?? 0) - (right.location.line ?? 0) || + (left.location.column ?? 0) - (right.location.column ?? 0) || + left.id.localeCompare(right.id) || + left.title.localeCompare(right.title), + ) + for (const issue of sorted) { + const key = groupKey(issue) + const group = groups.get(key) + if (group) { + group.issues.push(issue) + if (!group.files.includes(issue.location.file)) group.files.push(issue.location.file) + } else { + groups.set(key, {severity: issue.severity, issues: [issue], files: [issue.location.file]}) + } + } + return [...groups.values()] +} diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts index 22865b74c1f..1c9f1e4eb2c 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts @@ -54,6 +54,7 @@ function liquidUnsafeRenderVisitor(file: SourceFile) { unsafeRenderFix(context), 'medium', -10, + context, ), ] }, @@ -81,6 +82,7 @@ function liquidExecutableContextVisitor(file: SourceFile) { : 'Keep dynamic values out of executable attributes; use a fixed, versioned script asset and event listeners.', 'high', -25, + context, ), ] }, @@ -144,10 +146,12 @@ function makeLiquidIssue( fix: string, severity: Issue['severity'], points: number, + context: LiquidOutputContext, ): Issue { const lineStart = file.content!.lastIndexOf('\n', Math.max(0, offset - 1)) + 1 return { id, + pattern_id: `liquid-${context}`, severity, points, title, diff --git a/packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts b/packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts index 8d145604b1c..437021e2d47 100644 --- a/packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts +++ b/packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts @@ -143,6 +143,7 @@ function committedSecretFileIssue(file: SourceFile, status: GitFileStatus, envir return { id: 'COMMITTED_SECRET', + pattern_id: `${environmentFile ? 'environment-file' : 'secret-file'}:${tracked ? 'tracked' : 'unconfirmed'}`, severity: 'high', points: -50, title, @@ -192,6 +193,7 @@ export async function scanCommittedSecrets(secretEvidenceFiles: SourceFile[], ap if (!pattern.regex.test(line)) continue issues.push({ id: 'COMMITTED_SECRET', + pattern_id: pattern.name, severity: 'high', points: -50, title: `Hardcoded ${pattern.name} detected`, diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts index 9a5b855b1d6..63a9fabdb38 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts @@ -189,6 +189,7 @@ describe('git status drives severity, not .gitignore text', () => { expect(finding!.severity).toBe('high') expect(finding!.points).toBe(-50) expect(finding!.detection_evidence?.join(' ')).toContain('TRACKED') + expect(finding!.pattern_id).toBe('environment-file:tracked') rmSync(dir, {recursive: true, force: true}) }) @@ -239,6 +240,7 @@ describe('git status drives severity, not .gitignore text', () => { const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') expect(finding).toBeDefined() expect(finding!.severity).toBe('high') + expect(finding!.pattern_id).toBe('environment-file:unconfirmed') 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 7a917422138..ff9059ba40f 100644 --- a/packages/app/src/cli/services/app-doctor-engine/types.ts +++ b/packages/app/src/cli/services/app-doctor-engine/types.ts @@ -1,5 +1,7 @@ export interface Issue { id: string + /** Detector-defined variant within a rule, used to group similar findings in the report. */ + pattern_id?: string severity: Severity points: number title: string diff --git a/packages/app/src/cli/services/doctor-output.test.ts b/packages/app/src/cli/services/doctor-output.test.ts index fc9d412bab3..b965fc4f38c 100644 --- a/packages/app/src/cli/services/doctor-output.test.ts +++ b/packages/app/src/cli/services/doctor-output.test.ts @@ -107,11 +107,13 @@ describe('buildDoctorAlert', () => { [ {bold: 'Request input selects Admin API shop context'}, {subdued: 'REQUEST_CONTROLLED_ADMIN_CONTEXT'}, + '1 occurrence across 1 file', {filePath: 'app/routes/action.ts:42'}, ], [ {bold: 'Configured API version is no longer supported'}, {subdued: 'EOL_API_VERSION'}, + '1 occurrence across 1 file', {filePath: 'shopify.app.toml'}, ], ], @@ -137,6 +139,83 @@ describe('buildDoctorAlert', () => { ]) }) + test('collapses hundreds of occurrences without hiding their reach or changing severity', () => { + const issues = Array.from({length: 200}, (_, index) => ({ + ...scanWithIssues.issues[0]!, + location: {file: `app/routes/route-${String(index).padStart(3, '0')}.ts`, line: 42}, + })) + const input = reportInput({scan: {...scanWithIssues, issues}}) + const before = structuredClone(input.scan) + const alert = buildDoctorAlert(input) + const high = section(input, 'High')! + + expect(alert.type).toBe('error') + expect(alert.options.headline).toBe('1 security issue group found (200 occurrences).') + expect(high.body).toEqual({ + list: { + items: [ + [ + {bold: issues[0]!.title}, + {subdued: issues[0]!.id}, + '200 occurrences across 200 files', + {filePath: 'app/routes/route-000.ts:42'}, + {filePath: 'app/routes/route-001.ts:42'}, + {filePath: 'app/routes/route-002.ts:42'}, + {subdued: '+197 more files'}, + ], + ], + }, + }) + expect(JSON.stringify(alert)).toContain('Use --verbose') + expect(input.scan).toEqual(before) + expect(buildDoctorAlert({...input, scan: {...input.scan, issues: [...issues].reverse()}})).toEqual(alert) + }) + + test('samples distinct files, not just the first three occurrences', () => { + const issues = [ + {file: 'app/a.ts', line: 1}, + {file: 'app/a.ts', line: 2}, + {file: 'app/a.ts', line: 3}, + {file: 'app/b.ts', line: 4}, + {file: 'app/c.ts', line: 5}, + ].map((location) => ({...scanWithIssues.issues[0]!, location})) + const input = reportInput({scan: {...scanWithIssues, issues}}) + const serialized = JSON.stringify(section(input, 'High')) + expect(serialized).toContain('5 occurrences across 3 files') + expect(serialized).toContain('app/a.ts:1') + expect(serialized).toContain('app/b.ts:4') + expect(serialized).toContain('app/c.ts:5') + expect(serialized).not.toContain('app/a.ts:2') + }) + + test('verbose output expands every occurrence, including distinct messages and fixes', () => { + const issues = Array.from({length: 4}, (_, index) => ({ + ...scanWithIssues.issues[0]!, + location: {file: 'app/a.ts', line: index + 1}, + message: `Evidence ${index}`, + fix: {automated: false, description: `Remediation ${index}`}, + })) + const input = reportInput({verbose: true, scan: {...scanWithIssues, issues}}) + const serialized = JSON.stringify(section(input, 'High')) + expect(serialized).toContain('4 occurrences across 1 file') + for (const [index, issue] of issues.entries()) { + expect(serialized).toContain(`app/a.ts:${index + 1}`) + expect(serialized).toContain(issue.message) + expect(serialized).toContain(`Fix: ${issue.fix.description}`) + } + }) + + test.each(['low', 'medium'] as const)('does not promote widespread %s findings', (severity) => { + const issues = Array.from({length: 200}, (_, index) => ({ + ...scanWithIssues.issues[0]!, + severity, + location: {file: `app/${index}.ts`}, + })) + const input = reportInput({scan: {...scanWithIssues, issues}}) + expect(buildDoctorAlert(input).type).toBe('warning') + expect(section(input, 'High')).toBeUndefined() + }) + test('quotes compile commands for Windows paths with spaces and percents', () => { const commands = resolveAppDoctorCommands('C:/Users/50%/my app') const alert = buildDoctorAlert(reportInput({commands})) diff --git a/packages/app/src/cli/services/doctor-output.ts b/packages/app/src/cli/services/doctor-output.ts index 713c36a4094..4a10065193a 100644 --- a/packages/app/src/cli/services/doctor-output.ts +++ b/packages/app/src/cli/services/doctor-output.ts @@ -1,6 +1,7 @@ import {formatAppDoctorCommand, type AppDoctorCommands} from './app-doctor-commands.js' -import {renderError, renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui' +import {groupIssues, type IssueGroup} from './app-doctor-engine/index.js' import type {Capabilities, Issue, ScanResult, Severity} from './app-doctor-engine/index.js' +import {renderError, renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui' import type {AlertCustomSection, InlineToken, RenderAlertOptions, Token, TokenItem} from '@shopify/cli-kit/node/ui' interface DoctorEngineMetadata { @@ -33,21 +34,26 @@ interface DoctorAlert { } const SEVERITY_LABEL: Record = {high: 'High', medium: 'Medium', low: 'Low'} +const SAMPLE_FILE_COUNT = 3 export function buildDoctorAlert(input: DoctorReportInput): DoctorAlert { const type = doctorAlertType(input) + const groups = groupIssues(input.scan.issues) return { type, options: { - headline: doctorHeadline(input), + headline: doctorHeadline(input, groups), body: doctorBody(input), ...(input.findings ? {} : {nextSteps: doctorNextSteps(input.commands)}), reference: [ {subdued: `Engine: ${input.engine.name} ${input.engine.version}`}, {subdued: `Ruleset: ${input.engine.ruleset}`}, + ...(groups.some((group) => group.issues.length > 1) && !input.verbose + ? [{subdued: 'Use --verbose for every occurrence and fix. The trace retains all file and line details.'}] + : []), ], - customSections: doctorCustomSections(input), + customSections: doctorCustomSections(input, groups), }, } } @@ -77,12 +83,15 @@ function doctorAlertType(input: DoctorReportInput): DoctorAlertType { return 'success' } -function doctorHeadline(input: DoctorReportInput): string { +function doctorHeadline(input: DoctorReportInput, groups: IssueGroup[]): string { if (input.findings && input.findings.rejected.length > 0) { return 'App Doctor could not compile some agent findings.' } const count = input.scan.issues.length + if (groups.length < count) { + return `${groups.length} security issue ${groups.length === 1 ? 'group' : 'groups'} found (${count} occurrences).` + } if (count > 0) return `${count} security ${count === 1 ? 'issue' : 'issues'} found.` if (coverageIncomplete(input)) return 'Scan completed with coverage gaps.' return 'No security issues found.' @@ -116,15 +125,20 @@ function doctorNextSteps(commands: AppDoctorCommands): TokenItem[] ] } -function doctorCustomSections(input: DoctorReportInput): AlertCustomSection[] { +function doctorCustomSections(input: DoctorReportInput, groups: IssueGroup[]): AlertCustomSection[] { const sections: AlertCustomSection[] = [] - for (const group of groupIssuesBySeverity(input.scan.issues)) { + for (const severity of ['high', 'medium', 'low'] as const) { + const severityGroups = groups.filter((group) => group.severity === severity) + if (severityGroups.length === 0) continue sections.push({ - title: SEVERITY_LABEL[group.severity], + title: SEVERITY_LABEL[severity], body: { list: { - items: group.issues.map((issue) => issueListItem(issue, input.verbose)), + items: severityGroups.flatMap((group) => [ + issueGroupListItem(group), + ...(input.verbose ? group.issues.map(issueListItem) : []), + ]), }, }, }) @@ -187,43 +201,39 @@ function doctorCustomSections(input: DoctorReportInput): AlertCustomSection[] { return sections } -function issueListItem(issue: Issue, verbose: boolean): TokenItem { +function issueListItem(issue: Issue): 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}] - if (verbose) { - item.push({subdued: issue.message}, {subdued: `Fix: ${issue.fix.description}`}) - if (issue.fix.guide) { - if (issue.fix.guide.startsWith('https://') || issue.fix.guide.startsWith('http://')) { - item.push({link: {label: 'Docs', url: issue.fix.guide}}) - } else { - item.push({subdued: `Docs: ${issue.fix.guide}`}) - } + item.push({subdued: issue.message}, {subdued: `Fix: ${issue.fix.description}`}) + if (issue.fix.guide) { + if (issue.fix.guide.startsWith('https://') || issue.fix.guide.startsWith('http://')) { + item.push({link: {label: 'Docs', url: issue.fix.guide}}) + } else { + item.push({subdued: `Docs: ${issue.fix.guide}`}) } - if (issue.snippet) item.push({subdued: `Code: ${issue.snippet}`}) } + if (issue.snippet) item.push({subdued: `Code: ${issue.snippet}`}) return item } -function sortIssues(issues: Issue[]): Issue[] { - const severityOrder: Record = {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 issueGroupListItem(group: IssueGroup): TokenItem { + const issue = group.issues[0]! + const count = group.issues.length + const files = group.files.length + const samples = group.files.slice(0, SAMPLE_FILE_COUNT).map((file) => { + const sample = group.issues.find((occurrence) => occurrence.location.file === file)! + const location = sample.location.line ? `${file}:${sample.location.line}` : file + return {filePath: location} }) -} - -function groupIssuesBySeverity(issues: Issue[]): {severity: Severity; issues: Issue[]}[] { - const groups: {severity: Severity; issues: Issue[]}[] = [] - for (const issue of sortIssues(issues)) { - const last = groups[groups.length - 1] - if (last?.severity === issue.severity) last.issues.push(issue) - else groups.push({severity: issue.severity, issues: [issue]}) - } - return groups + return [ + {bold: issue.title}, + {subdued: issue.id}, + `${count} ${count === 1 ? 'occurrence' : 'occurrences'} across ${files} ${files === 1 ? 'file' : 'files'}`, + ...samples, + ...(files > SAMPLE_FILE_COUNT ? [{subdued: `+${files - SAMPLE_FILE_COUNT} more files`}] : []), + ] } function formatCapabilities(capabilities: Capabilities): string { From 6fc58ee1d4be5ddccd2deb0e513c5e96f86fff57 Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Thu, 17 Sep 2026 13:07:09 -0500 Subject: [PATCH 2/2] Fix App Doctor output import lint Co-authored-by: AI (Pi/Grok 4.6) --- packages/app/src/cli/services/doctor-output.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/app/src/cli/services/doctor-output.ts b/packages/app/src/cli/services/doctor-output.ts index 4a10065193a..a965875b456 100644 --- a/packages/app/src/cli/services/doctor-output.ts +++ b/packages/app/src/cli/services/doctor-output.ts @@ -1,6 +1,12 @@ import {formatAppDoctorCommand, type AppDoctorCommands} from './app-doctor-commands.js' -import {groupIssues, type IssueGroup} from './app-doctor-engine/index.js' -import type {Capabilities, Issue, ScanResult, Severity} from './app-doctor-engine/index.js' +import { + groupIssues, + type Capabilities, + type Issue, + type IssueGroup, + type ScanResult, + type Severity, +} from './app-doctor-engine/index.js' import {renderError, renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui' import type {AlertCustomSection, InlineToken, RenderAlertOptions, Token, TokenItem} from '@shopify/cli-kit/node/ui'