Reuse Shopify CLI app identity and config schemas in App Doctor - #8514
Reuse Shopify CLI app identity and config schemas in App Doctor#8514jplhomer wants to merge 1 commit into
Conversation
jplhomer
changed the title
github-actions
Bot
added
the
no-changelog
Walk-up discovery and config-file names now share isValidFormatAppConfigurationFileName. Access, auth, and webhook parsing use the CLI schemas, with URI strings kept only so INSECURE_WEBHOOK_URL can still report http. This is not Project.load(): bounded reads, coverage gaps, and unconfigured extensions stay Doctor-specific. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
54d874a to
43f85ce
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A new validation check in findAppRoot makes an existing error message misleading for non-app .toml paths and should be updated before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns App Doctor’s app discovery and TOML parsing with Shopify CLI’s established app configuration identity and schemas, reducing drift as CLI rules evolve.
Changes:
- Reuse CLI app config file identity (
APP_CONFIG_FILE_GLOB+isValidFormatAppConfigurationFileName) in Project root discovery, config-file prompting, and App Doctor discovery. - Parse
access_scopes,auth, and webhook subscriptions using CLI schemas while keeping webhook URIs as strings to preserveINSECURE_WEBHOOK_URLbehavior. - Add/adjust tests to cover invalid “glob-matching but not CLI-shaped” config files and non-CLI-shaped webhook subscriptions.
File summaries
| File | Description |
|---|---|
| packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts | Adds coverage ensuring glob matches are filtered to CLI-valid app config filenames. |
| packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts | Extends parsing contract tests for webhook URI handling and shape filtering. |
| packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts | Switches discovery/parsing to CLI naming + schemas; keeps webhook URIs as strings. |
| packages/app/src/cli/prompts/config.ts | Filters config-file candidates using the CLI filename validator. |
| packages/app/src/cli/models/project/project.ts | Uses shared naming identity (glob + validator) for project root/config discovery. |
| packages/app/src/cli/models/app/config-file-naming.ts | Introduces a shared glob constant for app config discovery. |
| packages/app/src/cli/models/app/config-file-naming.test.ts | Adds filename validator test cases for near-miss filenames. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!isValidFormatAppConfigurationFileName(basename(requestedPath))) { | ||
| throw new AppRootDiscoveryError(`App path is not a directory or TOML file: ${startPath}`) | ||
| } |
| const evidence = EvidenceWebhookSubscriptionSchema.safeParse(value) | ||
| if (!evidence.success) return [] | ||
| const parsed = SecurityWebhookSubscriptionSchema.safeParse(value) | ||
| if (!parsed.success) return [] |
There was a problem hiding this comment.
My agent sez: Keep insecure URI evidence when another subscription field is invalid.
SecurityWebhookSubscriptionSchema validates the complete object. For example, this subscription has an insecure URI and an invalid filter:
[[webhooks.subscriptions]]
topics = ["orders/create"]
uri = "http://insecure.example/webhooks"
filter = 42The schema rejects filter, so this function returns []. It does not record a coverage gap. INSECURE_WEBHOOK_URL then receives no URI, and the scan can report complete coverage without this evidence.
Keep an independently valid URI for the security check and record the schema failure as a coverage gap. At minimum, do not discard the subscription without a coverage gap.
|
|
||
| export async function findConfigFiles(directory: string): Promise<string[]> { | ||
| return glob(joinPath(directory, 'shopify.app*.toml')) | ||
| const files = await glob(joinPath(directory, 'shopify.app*.toml')) |
There was a problem hiding this comment.
Can we also use APP_CONFIG_FILE_GLOB for this call?
WHY are these changes introduced?
App Doctor walked up for
shopify.app*.tomland parsed access, auth, and webhooks with its own schemas and fallbacks. That can disagree with Shopify CLI as those rules change.Stacked on #8513.
WHAT is this pull request doing?
Share CLI app-config identity (
isValidFormatAppConfigurationFileName/APP_CONFIG_FILE_GLOB) in Project discovery, config-file prompts, and App Doctor. Parse access, auth, and webhook subscriptions with the CLI schemas. Keep webhook URIs as strings soINSECURE_WEBHOOK_URLcan still report http.This is not
Project.load(). Bounded reads, coverage gaps, no-cwd-fallback, and scanning unconfigured extensions stay Doctor-specific.No user-facing CLI command change.
How to manually test your changes?
From an app directory:
A directory that only has
shopify.application.tomlshould not be treated as an app root.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add