Add hidden App Doctor commands - #8429
Conversation
|
/snapit |
|
🫰✨ 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-20260828224943Caution After installing, validate the version by running |
|
/snapit |
|
🫰✨ 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-20260831163348Caution After installing, validate the version by running |
|
/snapit |
|
🫰✨ 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-20260831165144Caution After installing, validate the version by running |
|
Can we make the scan command just |
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>
127e9ff to
66a786f
Compare
|
/snapit |
|
🫰✨ 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-20260901125419Caution After installing, validate the version by running |
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
|
/snapit |
|
🫰✨ 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-20260901133028Caution After installing, validate the version by running |
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>
|
/snapit |
|
🫰✨ 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-20260901155310Caution After installing, validate the version by running |
Do not present an incomplete scan as a clean security result. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Document-level rejection messages have no colon; do not slice them as IDs. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Cover missing, unreadable, invalid JSON, and oversized --findings input. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
loadChecks hardcodes the tier; the markdown field was never read. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Keep engine internals private to the engine package boundary. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Scanners own discovery types so rules depend on evidence, not the reverse. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Keep the sha256: prefix and canonical JSON conversion local. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Knip flagged the leftover export after evidence types moved. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
CodeQL flagged incomplete sanitization in shellQuote. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Keep the broader extension glob and prove unconfigured extensions are scanned. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Parse access, auth, and webhook sections independently so one invalid section becomes a coverage gap instead of dropping valid evidence. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Return tagged scan and compile data from executeAppDoctor, write artifacts in one module, and keep --json, prompts, and exit codes in the command layer. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Cover instruction embedding parity, an unmocked scan path, and a successful real npm audit. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
|
Architecture split: Resolved by e5ef447 |
Spawn Node directly instead of a PATH-shimmed npm, and accept overflow termination as well as timeout for noisy children. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
spawn(npm) with shell: false cannot execute npm.cmd, so the real audit test and Windows scans failed with ENOENT. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
CI type-check rejected readonly stdio tuples. Golden JSON tests now compare parsed objects so CRLF checkouts do not fail. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
cmd /s /c strips the first and last quote, so the shim must wrap the whole command line or Windows treats `npm" "audit` as the executable. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
npm.cmd locates npm-prefix.js via %~dp0. Invoking bare `npm` through cmd makes that the sandbox cwd, so resolve .cmd/.bat/.exe first. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
|
/snapit |
|
🫰✨ 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-20260902180008Caution After installing, validate the version by running |
jek
left a comment
There was a problem hiding this comment.
I focused on frictions for doctor as a CI and adhoc tool. No blocking concerns for this iteration, but we could do a couple passes as separate PRs because this one is pretty large:
- a correctness pass specifically for Windows & Windows CI execution (I tagged the subprocess spawn that popped out at me most)
- a general optimization pass on the overall scanning & metadata collection with an eye to keeping memory usage constant & generally being resource friendly on larger code bases.
| try { | ||
| execution = await Promise.race([ | ||
| executor(selection.command, auditArguments(selection, sandbox.userConfigPath), { | ||
| cwd: sandbox.workspace, | ||
| signal: controller.signal, | ||
| env: auditEnvironment(appRoot, sandbox), | ||
| }), | ||
| hardStop, | ||
| ]) | ||
| // Command absence, network failures, and aborts are expected audit outcomes. | ||
| // eslint-disable-next-line no-catch-all/no-catch-all | ||
| } catch (error) { | ||
| if (controller.signal.aborted) { | ||
| return {issues: [], unresolvedReason: 'Dependency audit timed out.', inspectedFiles} | ||
| } | ||
| return { | ||
| issues: [], | ||
| unresolvedReason: `Dependency audit could not run: ${redactAuditText(error instanceof Error ? error.message : String(error))}`, | ||
| inspectedFiles, | ||
| } | ||
| } finally { | ||
| clearTimeout(timeout) | ||
| clearTimeout(hardTimeout) | ||
| await removeAuditSandbox(sandbox.root) | ||
| } |
There was a problem hiding this comment.
Immediately cleaning the sandbox may be flaky here if the spawned command or its unreaped subprocesses still have files open on Windows.
| export function doctorExitCode(execution: AppDoctorExecution, blocking: AppDoctorBlockingLevel): number { | ||
| if (execution.operation === 'compile' && execution.findings.rejected.length > 0) return 2 | ||
| if (shouldBlock(execution.scan.issues, blocking)) return 1 | ||
| return 0 |
There was a problem hiding this comment.
This looks like it can exit success when a run failed with incomplete coverage.
cmd.exe expands %NAME% inside quotes and does not treat \ as an escape. Use %% and "" instead of POSIX-style quoting. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
dmerand
left a comment
There was a problem hiding this comment.
Thanks for all of the changes! This is close to good enough for hidden/early-access. I have a few more notes, which I've made explicitly verbose. I'll provide some other notes for follow-up.
| } | ||
| } | ||
|
|
||
| const document = options.findings |
There was a problem hiding this comment.
This command scans the app again. The findings document does not identify the scan that created its review pack. If a file path still exists, the command can accept a finding for old file contents. The final trace then records the new input hash for that old finding.
Please add a schema version to the findings document. Add the scan input hash or a generated review ID to review.json and findings.json. Before validation or merge, reject findings when this value does not match. Add a test that changes a scanned file after review generation and before compilation.
|
|
||
| export async function writeAppDoctorArtifacts(execution: AppDoctorExecution): Promise<AppDoctorArtifactPaths> { | ||
| const paths = appDoctorArtifactPaths(execution.appRoot) | ||
| await mkdir(paths.artifactDirectory) |
There was a problem hiding this comment.
The current checks examine only the final artifact file. mkdir() can follow a repository-controlled .shopify or .shopify/app-doctor symbolic link or Windows junction. The command can then write outside the app root.
Please reject symbolic-link or junction ancestors in the artifact directory path. Make sure that the final directory stays in the app root. Add an external-target test for a symbolic link. Add a Windows junction test.
| return { | ||
| appRoot, | ||
| scanCommand: `shopify app doctor --path ${quotedRoot}`, | ||
| compileCommand: `shopify app doctor --path ${quotedRoot} --findings ${shellQuote(findingsPath)}`, |
There was a problem hiding this comment.
App Doctor constructs this command more than once. Presentation selects shell syntax from process.platform. The terminal report omits the resolved --path. The agent instructions include it.
The CLI already uses a shell-free pattern. packageManagerBinaryCommand() returns separate command and args values in packages/cli-kit/src/public/node/node-package-manager.ts. runGraphqlCodegen() passes these values to exec() in packages/app/src/cli/services/function/build.ts.
Please create one resolved action with separate command and args values. Use this action for terminal output and agent instructions. Render shell-specific text only at the final presentation boundary. Add a test in which the current directory differs from --path. Use a Windows path that contains spaces and %.
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 doctorcommands directly to Shopify CLI.shopify app doctor--pathor a nested working directory;app-doctor-review.jsonwith versioned semantic checks for coding-agent follow-up;--findings <file>to validate agent results and compile them into the trace;high | medium | low | noneCI blocking;Security boundaries
shopify app doctor instructions--path,--copy, and--write <path>;shopify app doctorinvocation;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 submitremains 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
DoctorSuiterelease-test harness.Top-hatting
Run a scan:
Print, copy, or write coding-agent instructions:
After completing the generated review pack:
Verification
@shopify/appcommand registration test passing.shopify app doctorandshopify app doctor instructions.git diff --checkpasses.