Skip to content

Add hidden App Doctor commands - #8429

Open
jplhomer wants to merge 17 commits into
mainfrom
app-doctor/cli-scan
Open

Add hidden App Doctor commands#8429
jplhomer wants to merge 17 commits into
mainfrom
app-doctor/cli-scan

Conversation

@jplhomer

@jplhomer jplhomer commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Shopify app developers need a Shopify-specific security review workflow that combines deterministic checks with structured prompts for deeper review by their coding agent. The coding-agent procedure should come from the same Shopify CLI build that owns the review artifacts rather than requiring developers to install AI Toolkit or copy a separately versioned skill.

Unsupported or incomplete analysis must never be presented as a clean security grade. Submitted agent findings are validated before they can affect the compiled trace.

Solution

Add hidden shopify app doctor commands directly to Shopify CLI.

shopify app doctor

  • discovers the app root from --path or a nested working directory;
  • runs the current 14-check deterministic catalog across supported Shopify React Router apps, theme app extensions, and structured Shopify configuration;
  • uses structured TOML parsing, narrow JavaScript/TypeScript analysis, Theme Check Liquid AST analysis, and isolated package-manager audits;
  • writes app-doctor-review.json with versioned semantic checks for coding-agent follow-up;
  • writes an unsigned App Doctor trace using schema version 2;
  • accepts --findings <file> to validate agent results and compile them into the trace;
  • supports human and JSON output plus high | medium | low | none CI blocking;
  • records per-check status, implementation provenance, inspected files, skipped inputs, and explicit coverage gaps;
  • withholds the grade when required coverage is unsupported, unresolved, unreadable, or rejected;
  • redacts matched secrets, including complete and malformed private-key material, from findings, evidence, output, and traces.

Security boundaries

  • nested apps, dependency trees, generated output, tests, and fixtures are excluded recursively;
  • generated scan artifacts are excluded from subsequent scan inputs;
  • dependency audits run in a private temporary sandbox containing only sanitized manifest data and the selected lockfile, with scripts/plugins/local dependencies removed, a trusted registry, a sanitized environment, and an explicitly selected Yarn Berry version;
  • submitted agent and external findings receive strict runtime shape, provenance, path, and scanned-input validation;
  • standalone instructions never infer completion from a persisted review pack, and agent fallback never instructs the agent to execute repository-controlled package-manager code.

shopify app doctor instructions

  • prints instructions to stdout by default;
  • supports --path, --copy, and --write <path>;
  • always starts from a fresh shopify app doctor invocation;
  • embeds adjacent Markdown as the source of truth, so the published CLI does not depend on runtime instruction assets.

Both commands remain callable but hidden from normal command listings while the workflow is in development. The engine lives inside @shopify/app; shopify app doctor submit remains reserved for future authenticated upload.

Risk

This adds a substantial local scanner and a new trace schema. Static analysis cannot prove an app secure, so the implementation explicitly records incomplete coverage and withholds a grade rather than reporting a false clean result. The command does not reuse the unrelated DoctorSuite release-test harness.

Top-hatting

pnpm install --frozen-lockfile
pnpm --filter @shopify/app type-check
pnpm --filter @shopify/app lint
pnpm --filter @shopify/app build
pnpm --filter @shopify/cli bundle

Run a scan:

node packages/cli/bin/run.js app doctor --path /path/to/app --skip-instructions
CI=1 node packages/cli/bin/run.js app doctor --path /path/to/app --json --skip-instructions

Print, copy, or write coding-agent instructions:

node packages/cli/bin/run.js app doctor instructions --path /path/to/app
node packages/cli/bin/run.js app doctor instructions --path /path/to/app --copy
node packages/cli/bin/run.js app doctor instructions --path /path/to/app --write /tmp/app-doctor.md

After completing the generated review pack:

node packages/cli/bin/run.js app doctor --path /path/to/app   --findings /path/to/app/findings.json   --skip-instructions

Verification

  • Focused App Doctor engine, command, and service suite: 15 files, 161 tests passing.
  • @shopify/app command registration test passing.
  • App type-check and lint pass.
  • Knip passes.
  • App build and CLI bundle pass.
  • Prompt embedding and OCLIF manifest regeneration are clean.
  • Bundled help confirms shopify app doctor and shopify app doctor instructions.
  • git diff --check passes.
  • All actionable findings from iterative Binks reviews were addressed.

@github-actions github-actions Bot added the Area: @shopify/app @shopify/app package issues label Aug 28, 2026
@jplhomer

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260828224943

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@jplhomer jplhomer changed the title Add App Doctor scan command Add hidden App Doctor commands Aug 31, 2026
@jplhomer

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260831163348

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@jplhomer

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260831165144

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@nickwesselman

Copy link
Copy Markdown
Contributor

Can we make the scan command just shopify app doctor for simplicity? Commands can have subcommands so this doesn't preclude shopify app doctor submit

jplhomer and others added 8 commits September 1, 2026 07:35
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
Reuse cli-kit reads, writes, and Git probes instead of custom hostile-repository hardening.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
@jplhomer
jplhomer force-pushed the app-doctor/cli-scan branch from 127e9ff to 66a786f Compare September 1, 2026 12:37
Comment thread packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts Fixed
Comment thread packages/app/src/cli/services/app-doctor-engine/rules/js-rules.ts Fixed
@jplhomer

jplhomer commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260901125419

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
@jplhomer

jplhomer commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260901133028

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

jplhomer and others added 3 commits September 1, 2026 08:33
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/GPT-5.6 Sol) <noreply@pi.dev>
…udit

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
@jplhomer

jplhomer commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260901155310

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

jplhomer and others added 4 commits September 1, 2026 12:34
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Always offer agentic review, hide the score from the UI, write artifacts under .shopify/app-doctor, and stop dropping checks when inspected_files include extra relative paths.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Knip flagged it as an unused exported type after the production-only CVE narrowing.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Incomplete coverage is expected; agentic review is the default next step.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
@jplhomer

jplhomer commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260901184610

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
@jplhomer

jplhomer commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🫰✨ Thanks @jplhomer! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260901185844

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@jplhomer
jplhomer marked this pull request as ready for review September 1, 2026 19:03
@jplhomer
jplhomer requested review from a team as code owners September 1, 2026 19:03
Copilot AI lite review requested due to automatic review settings September 1, 2026 19:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds hidden shopify app doctor commands to the Shopify CLI and introduces the in-tree App Doctor engine in @shopify/app, including deterministic scanning, embedded coding-agent instructions, findings validation/merge, trace compilation, and redacted reporting.

Changes:

  • Register hidden app doctor and app doctor instructions OCLIF commands and wire them to new services.
  • Add the App Doctor engine (rules, checks registry, scanning, scoring, trace schema v2, redaction, and external/agent findings validation).
  • Embed Markdown-based semantic check prompts and workflow instructions into the shipped CLI bundle.

Reviewed changes

Copilot reviewed 85 out of 87 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/cli/package.json Adds clipboardy dependency for instruction copy support in the bundled CLI.
packages/cli/oclif.manifest.json Registers hidden app:doctor and app:doctor:instructions commands and flags.
packages/cli/bin/bundle.js Marks clipboardy as external so its platform binaries remain alongside package sources.
packages/app/src/cli/services/doctor.ts Implements doctor service orchestration, output mode selection, and instruction handoff prompting.
packages/app/src/cli/services/doctor.test.ts Tests doctor service behavior (flag interactions, prompting, instruction delivery, exit codes).
packages/app/src/cli/services/doctor-output.ts Implements human/JSON rendering helpers for scan results, coverage, findings merge summaries, and redaction.
packages/app/src/cli/services/doctor-output.test.ts Tests alert rendering, grouping, verbosity behavior, and redaction in output formatting.
packages/app/src/cli/services/app-doctor-instructions.ts Provides embedded instructions delivery (print/copy/write) for coding-agent handoff.
packages/app/src/cli/services/app-doctor-instructions.test.ts Tests instruction content selection and delivery modes.
packages/app/src/cli/services/app-doctor-engine/version.ts Exposes engine version derived from CLI Kit version.
packages/app/src/cli/services/app-doctor-engine/types.ts Defines App Doctor scan, issues, checks execution, scoring, and trace v1/v2 types.
packages/app/src/cli/services/app-doctor-engine/tests/registry.test.ts Tests registry invariants across deterministic rules and agent checks.
packages/app/src/cli/services/app-doctor-engine/tests/interaction.test.ts Tests that registry is exposed for list/explain interaction surfaces.
packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts Tests app root discovery and scan boundary exclusions (nested apps, artifacts, build dirs).
packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts Tests deterministic rule contract, JS/Liquid scanners, and audit behavior.
packages/app/src/cli/services/app-doctor-engine/scorer/index.ts Implements scoring and scan metadata hashing/result hashing.
packages/app/src/cli/services/app-doctor-engine/rules/types.ts Defines rule interfaces and scan context types.
packages/app/src/cli/services/app-doctor-engine/rules/token-rules.ts Implements expiring offline token detection heuristics and related issue creation.
packages/app/src/cli/services/app-doctor-engine/rules/shopify-rules.ts Adds deterministic scanning for deprecated ScriptTag API usage.
packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts Adds secret detection patterns and redaction helpers; implements committed secret scanning with git probes.
packages/app/src/cli/services/app-doctor-engine/rules/proxy-rules.ts Adds deterministic scanning for app-proxy Liquid/HTML injection patterns.
packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts Adds Theme Check AST-based Liquid security scanning and parser-failure surfacing.
packages/app/src/cli/services/app-doctor-engine/rules/config-rules.ts Adds deterministic config rules for ScriptTag scope and insecure webhook/redirect URLs.
packages/app/src/cli/services/app-doctor-engine/rules/compliance-rules.ts Adds deterministic rules for compliance webhook presence and API version EOL detection.
packages/app/src/cli/services/app-doctor-engine/rules/catalog.ts Adds rule catalog entries and metadata for deterministic and agentic checks.
packages/app/src/cli/services/app-doctor-engine/registry/index.ts Builds authoritative registry and enforces catalog/implementation invariants.
packages/app/src/cli/services/app-doctor-engine/output/format.ts Adds JSON formatting with string redaction and issue sorting.
packages/app/src/cli/services/app-doctor-engine/INSTRUCTIONS.md Adds embedded workflow instructions for coding-agent review procedure.
packages/app/src/cli/services/app-doctor-engine/index.ts Exports engine entry points (scan, checks, trace, registry, formatting).
packages/app/src/cli/services/app-doctor-engine/external/index.ts Validates and merges externally-sourced findings with boundary checks and redaction.
packages/app/src/cli/services/app-doctor-engine/embed-checks.mjs Generates embedded TypeScript sources from adjacent Markdown checks and instructions.
packages/app/src/cli/services/app-doctor-engine/checks/WEAK_SHOP_VALIDATION.md Adds semantic check prompt source (embedded) for weak shop validation.
packages/app/src/cli/services/app-doctor-engine/checks/UNSCOPED_SHOP_CONFIG_WRITE.md Adds semantic check prompt source (embedded) for unscoped config writes.
packages/app/src/cli/services/app-doctor-engine/checks/UNSAFE_INNERHTML.md Adds semantic check prompt source (embedded) for XSS via unsafe DOM sinks.
packages/app/src/cli/services/app-doctor-engine/checks/UNAUTHENTICATED_ENDPOINT.md Adds semantic check prompt source (embedded) for unauthenticated handlers.
packages/app/src/cli/services/app-doctor-engine/checks/THEME_EXTENSION_XSS.md Adds semantic check prompt source (embedded) for theme extension XSS.
packages/app/src/cli/services/app-doctor-engine/checks/TEXT_SETTING_HTML_SMUGGLING.md Adds semantic check prompt source (embedded) for HTML smuggling via settings.
packages/app/src/cli/services/app-doctor-engine/checks/STATIC_FRAME_ANCESTORS.md Adds semantic check prompt source (embedded) for frame-ancestors CSP issues.
packages/app/src/cli/services/app-doctor-engine/checks/SSRF_REQUEST_FORGERY.md Adds semantic check prompt source (embedded) for SSRF and related injection risks.
packages/app/src/cli/services/app-doctor-engine/checks/SCRIPT_TAG_URL_INJECTION.md Adds semantic check prompt source (embedded) for ScriptTag URL injection.
packages/app/src/cli/services/app-doctor-engine/checks/SCOPE_OVER_REQUEST.md Adds semantic check prompt source (embedded) for over/under requested scopes.
packages/app/src/cli/services/app-doctor-engine/checks/REQUEST_DERIVED_SHOP_SCOPE.md Adds semantic check prompt source (embedded) for request-derived shop scoping/IDOR.
packages/app/src/cli/services/app-doctor-engine/checks/REQUEST_CONTROLLED_ADMIN_CONTEXT.md Adds semantic check prompt source (embedded) for request-controlled Admin context.
packages/app/src/cli/services/app-doctor-engine/checks/OVERBROAD_DATA_ACCESS.md Adds semantic check prompt source (embedded) for overbroad data returns.
packages/app/src/cli/services/app-doctor-engine/checks/OPEN_REDIRECT.md Adds semantic check prompt source (embedded) for open redirects.
packages/app/src/cli/services/app-doctor-engine/checks/MISSING_TENANT_ISOLATION.md Adds semantic check prompt source (embedded) for missing tenant isolation.
packages/app/src/cli/services/app-doctor-engine/checks/MISSING_EMBEDDED_CSP.md Adds semantic check prompt source (embedded) for missing embedded CSP protection.
packages/app/src/cli/services/app-doctor-engine/checks/MISSING_COMPLIANCE_WEBHOOKS.md Adds semantic check prompt source (embedded) for compliance webhook validation.
packages/app/src/cli/services/app-doctor-engine/checks/MISSING_AUTHORIZATION_CHECK.md Adds semantic check prompt source (embedded) for missing authorization checks.
packages/app/src/cli/services/app-doctor-engine/checks/METAFIELD_OFFLINE_TOKEN.md Adds semantic check prompt source (embedded) for offline-token metafield writes.
packages/app/src/cli/services/app-doctor-engine/checks/LIQUID_UNSAFE_RENDER.md Adds semantic check prompt source (embedded) for Liquid unsafe render review.
packages/app/src/cli/services/app-doctor-engine/checks/KNOWN_CVE_IN_DEPENDENCY.md Adds semantic check prompt source (embedded) for dependency CVE evaluation without executing PM code.
packages/app/src/cli/services/app-doctor-engine/checks/INSECURE_WEBHOOK_URL.md Adds semantic check prompt source (embedded) for insecure webhook/redirect URL review.
packages/app/src/cli/services/app-doctor-engine/checks/EXPIRING_OFFLINE_TOKEN.md Adds semantic check prompt source (embedded) for expiring offline token verification.
packages/app/src/cli/services/app-doctor-engine/checks/EOL_API_VERSION.md Adds semantic check prompt source (embedded) for API version EOL detection guidance.
packages/app/src/cli/services/app-doctor-engine/checks/DEPRECATED_SCRIPT_TAG_SCOPE.md Adds semantic check prompt source (embedded) for ScriptTag deprecation contract.
packages/app/src/cli/services/app-doctor-engine/checks/CSRF_MISSING_PROTECTION.md Adds semantic check prompt source (embedded) for CSRF protection checks.
packages/app/src/cli/services/app-doctor-engine/checks/CREDENTIAL_LOG_LEAKAGE.md Adds semantic check prompt source (embedded) for credential-to-logs leakage.
packages/app/src/cli/services/app-doctor-engine/checks/CREDENTIAL_BROWSER_LEAKAGE.md Adds semantic check prompt source (embedded) for credential-to-browser leakage.
packages/app/src/cli/services/app-doctor-engine/checks/COMMITTED_SECRET.md Adds semantic check prompt source (embedded) for committed secret investigation.
packages/app/src/cli/services/app-doctor-engine/checks/APP_PROXY_UNVERIFIED_SIGNATURE.md Adds semantic check prompt source (embedded) for missing app-proxy signature verification.
packages/app/src/cli/services/app-doctor-engine/checks/APP_PROXY_LIQUID_INJECTION.md Adds semantic check prompt source (embedded) for app-proxy Liquid injection.
packages/app/src/cli/services/app-doctor-api.ts Adds engine API wrapper: scan, review pack generation, findings validation/merge, trace compilation, and blocking exit codes.
packages/app/src/cli/services/app-doctor-api.test.ts Tests API behavior: artifact writing, seeded review replacement, blocking, and rejection handling.
packages/app/src/cli/index.ts Registers new App Doctor commands in @shopify/app command map.
packages/app/src/cli/index.test.ts Tests that App Doctor commands are registered and a reserved command is absent.
packages/app/src/cli/commands/app/doctor/instructions.ts Implements hidden shopify app doctor instructions command and flags.
packages/app/src/cli/commands/app/doctor/instructions.test.ts Tests instructions command wiring, defaults, and flag exclusivity.
packages/app/src/cli/commands/app/doctor.ts Implements hidden shopify app doctor command wiring and flags.
packages/app/src/cli/commands/app/doctor.test.ts Tests doctor command wiring and flag semantics.
packages/app/package.json Adds clipboardy dependency and a generation script for embedded checks/instructions.
package.json Updates tooling ignore list to include clipboardy.
.changeset/bright-doctors-scan.md Adds changeset for releasing App Doctor commands in @shopify/app.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (1)

packages/app/src/cli/services/app-doctor-api.ts:184

  • The same slice(0, indexOf(':')) parsing is used when emitting coverage_gaps for rejected agent results; for messages without a colon this produces an incorrect checkId (last character dropped) and makes it harder to correlate gaps back to checks. Guard the delimiter before slicing.
        ...rejected.map((message) => {
          const checkId = message.slice(0, message.indexOf(':'))
          return {
            code: 'unresolved_check' as const,
            ...(knownCheckIds.has(checkId) ? {check_id: checkId} : {}),
            message: `Rejected agent result: ${message}`,

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +133 to +137
const checks = loadChecks()
const knownCheckIds = new Set(checks.keys())
const rejectedCheckIds = new Set(
rejected.map((message) => message.slice(0, message.indexOf(':'))).filter((checkId) => knownCheckIds.has(checkId)),
)
Comment on lines +81 to +86
function doctorAlertType(input: DoctorReportInput): DoctorAlertType {
if (input.findings && input.findings.rejected.length > 0) return 'error'
if (input.scan.issues.some((issue) => issue.severity === 'high')) return 'error'
if (input.scan.issues.length > 0) return 'warning'
return 'success'
}
Comment on lines +93 to +96
const count = input.scan.issues.length
if (count > 0) return `${count} security ${count === 1 ? 'issue' : 'issues'} found.`
return 'No security issues found.'
}
}
}

export function parseAppToml(raw: Record<string, unknown>, path: string, content?: string): AppTomlContent {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not already exist in the CLI?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: most of this already exists. Project.load() finds the app root and all shopify.app*.toml files, collects TomlFile parse errors, and owns the extension_directories contract. TomlFile already owns TOML decoding.

The complete Project.load() is not a direct replacement. It also reads package, environment, and hidden configuration data, and it does not retain the bounded raw bytes that App Doctor needs for hashing and coverage reporting.

I suggest that we extract or reuse the project root and app-config candidate discovery. App Doctor can then keep its bounded raw read and coverage adapter.

Comment on lines +130 to +142
function asRecord(value: unknown): Record<string, unknown> | undefined {
return value !== null && typeof value === 'object' && !Array.isArray(value)
? (value as Record<string, unknown>)
: undefined
}

function stringArray(value: unknown): string[] {
return Array.isArray(value) ? value.filter((item): item is string => typeof item === 'string') : []
}

function optionalString(value: unknown): string | undefined {
return typeof value === 'string' ? value : undefined
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like AI slop that could go away if we properly had TOML types; TODO find out if we get this for free from the CLI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. We do not get one complete drop-in type today, but the CLI already has most of this structure in AppSchema, the app-access schema, the webhook schemas, AppConfigurationUsedByCli, and WebhooksConfig.

Please export and reuse the relevant access, auth, and webhook schema fragments instead of keeping asRecord(), stringArray(), and optionalString() as a second loose parser. Parse sections independently so that one invalid section becomes a coverage gap instead of discarding all valid configuration data.

App Doctor still needs a small projection for normalized scopes, webhook topics, exact raw bytes, and evidence hashes.

@dmerand

dmerand commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

architecture suggestion

This PR does not need to wait for the planned shared CLI JSON support. However, we can structure it now so that the later update is small.

  1. Make runAppDoctor() return typed data only:

    • {operation: 'scan', scan, trace, reviewPack}
    • {operation: 'compile', scan, trace, findings: {accepted, rejected, warnings}}

    Remove jsonReport: unknown, artifact paths, file writes, exit codes, and CLI-specific errors from this function.

  2. Put all .shopify/app-doctor writes in one artifact module. This module must own file paths, atomic writes, file permissions, and symbolic-link checks. It returns the paths that it wrote.

  3. Define one typed JSON result with operation: 'scan' | 'compile'. Add one pure function that converts the execution result to this JSON result. Add one function that encodes it. Keep doctor-output.ts for human-readable output only.

  4. Let doctor.ts control the command sequence:

    execute → write artifacts → encode or render → deliver instructions → set exit status

    Rejected agent data is a compile result with exit code 2. Unreadable or invalid input becomes a CLI error only at this command boundary.

  5. Do not build temporary local frameworks for JSON schemas, command events, schema discovery, or JSON error documents. A later change can connect these typed results to the shared CLI support when it is available.

Acceptance checks:

  • Public JSON data does not use unknown.
  • Scan and compile use one tagged JSON contract.
  • Only the artifact module writes result files.
  • Only the command layer handles --json, prompts, and process exit codes.
  • Engine execution does not import terminal, renderer, Oclif, or CLI-error APIs.
  • Golden JSON fixtures cover scan and compile results.

The command is still hidden. This is the least costly time to choose its JSON structure, before other tools depend on it.

@dmerand dmerand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a broad first-pass with the agent to find things to dig into a bit more. The functionality overall seems to be working as intended. Per our discussion, I'm not really looking at the rules engine or the rules themselves.

@@ -0,0 +1,9 @@
---
id: APP_PROXY_LIQUID_INJECTION
version: 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is tier: agentic needed here?

child.once('error', reject)
child.once('close', (exitCode) => resolve({stdout, stderr, exitCode: exitCode ?? 1}))
})
const TRUSTED_REGISTRY = 'https://registry.npmjs.org/'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the public npm registry part of a normal local scan. For a JavaScript app with a lockfile, the audit can send package names and versions, including private scoped names. npm can also send the full dependency tree through its fallback endpoint.

Please require explicit user consent before this network request. Without consent, record the CVE check as unresolved. The help text must state the destination and data sent. It must also cover private registries, proxies, custom certificate authorities, and possible Corepack downloads.

Official npm behavior: https://docs.npmjs.com/cli/v10/commands/npm-audit/

The current invocation's initial scan has already completed. It generated \`.shopify/app-doctor/review.json\` and the initial local \`.shopify/app-doctor/trace.json\`. Don't rerun the scan unless those results are missing or the app has changed. Continue by reading that generated review pack.`

interface AppDoctorInstructionsOptions {
directory: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directory is required but unused. If a user runs from /app-a with --path /app-b, these instructions still use an unqualified command and relative .shopify/app-doctor/* paths. The coding agent can scan or read /app-a.

Please generate the complete workflow from the resolved app root. Include the explicit path in the scan and compile commands and in all artifact paths. Encode the path safely for shell and Markdown use. Add tests where CWD differs from --path.

}
} finally {
if (timeout) clearTimeout(timeout)
await removeAuditSandbox(sandbox.root)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove the sandbox until the audit process and its descendants have stopped. The current timeout aborts the immediate child, wins Promise.race(), and starts cleanup without waiting for close. Cleanup errors are then suppressed. stdout and stderr also have no byte limit.

Please bound both streams, terminate the process tree, wait for closure, and add a short escalation timeout and Windows cleanup retry. Add a real test with a noisy child and a child that creates a descendant.

The direct spawn is appropriate here. The current CLI Kit capture path lets Execa extend the supplied environment.

/** Find the nearest app root without ever substituting CWD for a bad explicit path. */
export function findAppRoot(startPath?: string): string {
const requestedPath = resolvePath(startPath ?? cwd())
if (startPath && !fileExistsSync(requestedPath)) throw new Error(`App path does not exist: ${startPath}`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A missing or invalid --path is an expected user error. A raw Error is classified as an unexpected CLI defect and can be sent to crash reporting.

Please translate these root-discovery cases to an actionable AbortError at the command or service boundary. Include a next step to run from a Shopify app directory or supply --path. Keep scanner invariant failures as unexpected errors.

}
}

export function parseAppToml(raw: Record<string, unknown>, path: string, content?: string): AppTomlContent {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: most of this already exists. Project.load() finds the app root and all shopify.app*.toml files, collects TomlFile parse errors, and owns the extension_directories contract. TomlFile already owns TOML decoding.

The complete Project.load() is not a direct replacement. It also reads package, environment, and hidden configuration data, and it does not retain the bounded raw bytes that App Doctor needs for hashing and coverage reporting.

I suggest that we extract or reuse the project root and app-config candidate discovery. App Doctor can then keep its bounded raw read and coverage adapter.

Comment on lines +130 to +142
function asRecord(value: unknown): Record<string, unknown> | undefined {
return value !== null && typeof value === 'object' && !Array.isArray(value)
? (value as Record<string, unknown>)
: undefined
}

function stringArray(value: unknown): string[] {
return Array.isArray(value) ? value.filter((item): item is string => typeof item === 'string') : []
}

function optionalString(value: unknown): string | undefined {
return typeof value === 'string' ? value : undefined
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. We do not get one complete drop-in type today, but the CLI already has most of this structure in AppSchema, the app-access schema, the webhook schemas, AppConfigurationUsedByCli, and WebhooksConfig.

Please export and reuse the relevant access, auth, and webhook schema fragments instead of keeping asRecord(), stringArray(), and optionalString() as a second loose parser. Parse sections independently so that one invalid section becomes a coverage gap instead of discarding all valid configuration data.

App Doctor still needs a small projection for normalized scopes, webhook topics, exact raw bytes, and evidence hashes.

import {ENGINE_NAME, SUPPORTED_TRACE_SCHEMA_VERSIONS, TRACE_SCHEMA_VERSION} from '../types.js'
import {loadChecks} from '../checks/index.js'
import {redactText} from '../rules/secret-rules.js'
import {createHash} from 'node:crypto'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: CLI Kit already exports sha256() from @shopify/cli-kit/node/crypto. The three App Doctor createHash('sha256') call sites all hash strings, so they can reuse it and keep the App Doctor-specific hexadecimal conversion and sha256: prefix:

`sha256:${sha256(value).toString('hex')}`

This can replace the direct imports in checks/index.ts, scanners/index.ts, and this file. The canonical JSON conversion should remain local to App Doctor.

searchBoundaryFiles,
validateAgentChecksExecuted,
} from './app-doctor-engine/index.js'
import {computeResultHash} from './app-doctor-engine/scorer/index.js'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This service imports the engine entry point and several engine implementation files. Other outer services also import output, rules, trace, and generated-resource internals directly.

Please make app-doctor-engine/index.ts the boundary for code outside the engine. Export stable use cases and result types, not every internal helper. For example, expose scan, findings compilation, safe result projection, and instruction access through the facade. Internal engine tests can continue to import focused modules.

This keeps the current code inside @shopify/app while making later extraction or result-boundary work much smaller.

import {decodeToml} from '@shopify/cli-kit/node/toml/codec'
import {lstatSync} from 'node:fs'
import type {SourceCandidate} from '../types.js'
import type {AppTomlContent, ExtensionInfo, SourceFile, ManifestFile, WebhookSubscription} from '../rules/types.js'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are discovery and evidence types, but scanners and capability detection import them from rules/types.ts. This makes the evidence layer depend on the rules layer.

Please move AppTomlContent, ExtensionInfo, SourceFile, ManifestFile, and their supporting types to an evidence or scanner-owned type module. Keep Rule and its execution context under rules. The intended dependency direction is rules → evidence, not evidence → rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: @shopify/app @shopify/app package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants