feat(cli): add vendure doctor command with project check#4777
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a new non-mutating CLI command, vendure doctor, which runs composable checks: project discovery, dependency health, config loading/compatibility, GraphQL schema generation, optional read-only DB connectivity, and an optional production-safety profile. Each check returns structured CheckResult entries; an orchestrator aggregates them into a DoctorReport, computes overallStatus (honoring --strict), formats output (console or JSON), and exits with an appropriate code. The change includes type definitions, formatters, command registration, orchestration logic, and extensive tests for individual checks and command behavior. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nd session strategy check
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/cli/src/commands/doctor/checks/production-check.spec.ts (1)
169-180: ⚡ Quick winAdd a wildcard-origin CORS test case.
Please add coverage for
cors: { origin: '*', credentials: true }so broad-origin detection is regression-safe alongside theorigin: truecase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/doctor/checks/production-check.spec.ts` around lines 169 - 180, Add a new spec in production-check.spec.ts mirroring the existing "detects broad CORS with credentials" test but using cors: { origin: '*', credentials: true } to assert the check also warns for wildcard origins; use createTestConfig(...) to build the config and call runProductionCheck(config), then expect result.status toBe('warn') and that result.details contains a CORS-related message (similar assertions as the existing test used for origin: true) so wildcard-origin detection is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/command-declarations.ts`:
- Around line 296-300: The action handler currently forces a success exit with
process.exit(0) which hides non-zero diagnostic results; modify the action for
the doctor command (the async action that imports and calls doctorCommand) to
remove the unconditional process.exit(0) and instead await
doctorCommand(options) and let it return/throw its own status (or, if
doctorCommand returns a numeric exit code, capture that value and call
process.exit(code) only when non-zero), so that doctorCommand’s outcome is
propagated rather than suppressed.
In `@packages/cli/src/commands/doctor/checks/config-check.ts`:
- Line 87: Replace assignments that set process.env.VENDURE_RUNNING_IN_CLI =
undefined with delete process.env.VENDURE_RUNNING_IN_CLI so the environment
variable is removed instead of becoming the string "undefined"; update the
occurrence in config-check.ts (around the line setting the env flag), and apply
the same change where the flag is cleared in schema.ts and migrate.ts; ensure
isRunningFromVendureCli() continues to check process.env.VENDURE_RUNNING_IN_CLI
!= null without modification.
In `@packages/cli/src/commands/doctor/checks/dependency-check.ts`:
- Around line 319-324: detectDbTypeFromSource() currently only returns a type
when it exists in DB_DRIVER_MAP, which prevents checkDbDriver() from ever
hitting its "Unknown database type" warning; update detectDbTypeFromSource (and
the similar block around the earlier match at lines ~275-280) to return the
extracted type unconditionally when a db type string is parsed (e.g., after
extracting const type = dbBlockMatch[1]; return type;), letting checkDbDriver()
perform the DB_DRIVER_MAP lookup and emit the warning for unknown types.
- Around line 195-197: The current loop in dependency-check.ts that skips
entries starting with '.' (iterating "for (const entry of entries)") causes
hidden package-manager stores to be ignored and yields false negatives; update
the logic so you no longer blanket-skip hidden dirs: still skip the exact
targetPkg, but explicitly handle known hidden dependency stores (e.g.
node_modules/.pnpm, node_modules/.store, .yarn, pnpm/yarn global stores) by
recursing into them or treating them as candidate locations for package
installs, and/or replace the filesystem deep-scan with lockfile/package-manager
metadata-based detection as an alternative; locate and change the loop around
entries/targetPkg in dependency-check.ts to implement these checks and
additions.
In `@packages/cli/src/commands/doctor/checks/production-check.ts`:
- Around line 76-82: The CORS check currently only warns when cors.origin ===
true with credentials enabled; update the condition in the production-check
block that inspects config.apiOptions.cors (the cors variable) so it also treats
wildcard-string origins and arrays containing wildcards as broad origins: i.e.,
if cors.credentials === true and (cors.origin === true || cors.origin === '*' ||
(Array.isArray(cors.origin) && cors.origin.includes('*'))) then call warn('CORS
allows all origins with credentials enabled'); adjust the condition around the
existing warn call in production-check.ts accordingly.
In `@packages/cli/src/commands/doctor/doctor.ts`:
- Around line 37-45: When short-circuiting due to projectResult.status ===
'fail', ensure the report still includes a skipped "Production" check when the
user requested the production profile: in the block that currently pushes skip
entries for checksToRun.filter(c => c !== 'project') and calls
outputReport(buildReport(...)), also detect when options.profile ===
'production' (or when the original requested checks included 'production') and
push a results entry { name: 'Production', status: 'skip', message: 'Skipped due
to project check failure' } before calling outputReport; apply the same addition
to the other early-return block around the later lines referenced (the similar
project-failure branch at the 64-66 area) so both early exits include the
Production skip.
---
Nitpick comments:
In `@packages/cli/src/commands/doctor/checks/production-check.spec.ts`:
- Around line 169-180: Add a new spec in production-check.spec.ts mirroring the
existing "detects broad CORS with credentials" test but using cors: { origin:
'*', credentials: true } to assert the check also warns for wildcard origins;
use createTestConfig(...) to build the config and call
runProductionCheck(config), then expect result.status toBe('warn') and that
result.details contains a CORS-related message (similar assertions as the
existing test used for origin: true) so wildcard-origin detection is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ae44991-e6d1-45d8-b0e8-48a4d851f3b7
📒 Files selected for processing (15)
packages/cli/src/commands/command-declarations.tspackages/cli/src/commands/doctor/checks/config-check.tspackages/cli/src/commands/doctor/checks/database-check.tspackages/cli/src/commands/doctor/checks/dependency-check.spec.tspackages/cli/src/commands/doctor/checks/dependency-check.tspackages/cli/src/commands/doctor/checks/production-check.spec.tspackages/cli/src/commands/doctor/checks/production-check.tspackages/cli/src/commands/doctor/checks/project-check.spec.tspackages/cli/src/commands/doctor/checks/project-check.tspackages/cli/src/commands/doctor/checks/schema-check.tspackages/cli/src/commands/doctor/doctor.spec.tspackages/cli/src/commands/doctor/doctor.tspackages/cli/src/commands/doctor/formatters/console-formatter.tspackages/cli/src/commands/doctor/formatters/json-formatter.tspackages/cli/src/commands/doctor/types.ts
| action: async options => { | ||
| const { doctorCommand } = await import('./doctor/doctor'); | ||
| await doctorCommand(options); | ||
| process.exit(0); | ||
| }, |
There was a problem hiding this comment.
Avoid forcing success exit code for doctor command.
Hard-coding process.exit(0) can suppress diagnostic failure signaling (notably --strict CI use-cases) when the command intends to report non-zero outcomes.
Suggested change
action: async options => {
const { doctorCommand } = await import('./doctor/doctor');
await doctorCommand(options);
- process.exit(0);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| action: async options => { | |
| const { doctorCommand } = await import('./doctor/doctor'); | |
| await doctorCommand(options); | |
| process.exit(0); | |
| }, | |
| action: async options => { | |
| const { doctorCommand } = await import('./doctor/doctor'); | |
| await doctorCommand(options); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/command-declarations.ts` around lines 296 - 300,
The action handler currently forces a success exit with process.exit(0) which
hides non-zero diagnostic results; modify the action for the doctor command (the
async action that imports and calls doctorCommand) to remove the unconditional
process.exit(0) and instead await doctorCommand(options) and let it return/throw
its own status (or, if doctorCommand returns a numeric exit code, capture that
value and call process.exit(code) only when non-zero), so that doctorCommand’s
outcome is propagated rather than suppressed.
There was a problem hiding this comment.
This is the standard pattern across ALL 4 existing commands (add, migrate, codemod, schema). doctorCommand() calls process.exit(1) internally before this line is reached on failure
There was a problem hiding this comment.
@Ryrahul, you're right — I missed that doctorCommand already calls process.exit(1) internally when overallStatus is 'failed', so process.exit(0) here is only ever reached on success. This is indeed consistent with the add, migrate, codemod, and schema commands. My earlier comment was mistaken. No change needed here.
🧠 Learnings used
Learnt from: michaelbromley
Repo: vendurehq/vendure PR: 4488
File: packages/core/src/service/services/order.service.ts:1907-1940
Timestamp: 2026-03-10T21:20:55.233Z
Learning: In Vendure's TypeORM usage, when transactionSupport is set to 'nested' (SQLite via AbstractSqliteQueryRunner, MySQL, PostgreSQL), nested connection.withTransaction() calls inside an active transaction should not be treated as errors. TypeORM will increase transactionDepth and use a SAVEPOINT (typeorm_N) for savepoint-level isolation. TransactionAlreadyStartedError should only be thrown when transactionSupport === 'simple'. Do not flag nested withTransaction usage in Vendure as incorrect; apply this understanding across TypeORM-using code in Vendure.
| vendureVersion: VENDURE_VERSION, | ||
| }; | ||
| } finally { | ||
| process.env.VENDURE_RUNNING_IN_CLI = undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify downstream impact by locating all reads/writes of this env variable.
rg -n -C2 'VENDURE_RUNNING_IN_CLI' packages/cli/srcRepository: vendurehq/vendure
Length of output: 4114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the env flag is interpreted
rg -n "isRunningFromVendureCli|VENDURE_RUNNING_IN_CLI" packages | head -n 200
# Show the implementation around isRunningFromVendureCli (if present)
rg -n "function isRunningFromVendureCli|isRunningFromVendureCli\s*=" packages -SRepository: vendurehq/vendure
Length of output: 1664
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const key = 'VENDURE_RUNNING_IN_CLI';
console.log('initial:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
process.env[key] = 'true';
console.log('after "true":', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
process.env[key] = undefined;
console.log('after = undefined:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
delete process.env[key];
console.log('after delete:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
NODERepository: vendurehq/vendure
Length of output: 178
Use delete to clear the CLI env flag
On Line 87, setting process.env.VENDURE_RUNNING_IN_CLI = undefined leaves the string "undefined" in process.env. Since isRunningFromVendureCli() checks process.env.VENDURE_RUNNING_IN_CLI != null, the flag can remain enabled and leak CLI-mode state across checks in the same process.
Proposed fix
- process.env.VENDURE_RUNNING_IN_CLI = undefined;
+ delete process.env.VENDURE_RUNNING_IN_CLI;Apply the same cleanup anywhere else this env var is cleared with = undefined (e.g. in schema.ts and migrate.ts).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.env.VENDURE_RUNNING_IN_CLI = undefined; | |
| delete process.env.VENDURE_RUNNING_IN_CLI; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/doctor/checks/config-check.ts` at line 87, Replace
assignments that set process.env.VENDURE_RUNNING_IN_CLI = undefined with delete
process.env.VENDURE_RUNNING_IN_CLI so the environment variable is removed
instead of becoming the string "undefined"; update the occurrence in
config-check.ts (around the line setting the env flag), and apply the same
change where the flag is cleared in schema.ts and migrate.ts; ensure
isRunningFromVendureCli() continues to check process.env.VENDURE_RUNNING_IN_CLI
!= null without modification.
There was a problem hiding this comment.
The existing schema.ts and migrate.ts both use = undefined across 4 occurrences. Changing only our code would be inconsistent. The behavior is equivalent here since the check is === 'true'. Happy to address this repo-wide in a separate PR if desired.
There was a problem hiding this comment.
@Ryrahul, that's a fair point. If isRunningFromVendureCli() guards against === 'true', then assigning undefined (which coerces to the string "undefined") does not satisfy that condition, so CLI mode is effectively cleared — the behavior is equivalent. My concern was predicated on a != null check, which doesn't apply here.
The consistency argument also holds; introducing delete only in the new code while leaving the existing pattern unchanged in schema.ts and migrate.ts would create unnecessary inconsistency.
A repo-wide cleanup PR would be the right place to standardize this. Would you like me to open a follow-up issue to track that?
🧠 Learnings used
Learnt from: michaelbromley
Repo: vendurehq/vendure PR: 4488
File: packages/core/src/service/services/order.service.ts:1907-1940
Timestamp: 2026-03-10T21:20:55.233Z
Learning: In Vendure's TypeORM usage, when transactionSupport is set to 'nested' (SQLite via AbstractSqliteQueryRunner, MySQL, PostgreSQL), nested connection.withTransaction() calls inside an active transaction should not be treated as errors. TypeORM will increase transactionDepth and use a SAVEPOINT (typeorm_N) for savepoint-level isolation. TransactionAlreadyStartedError should only be thrown when transactionSupport === 'simple'. Do not flag nested withTransaction usage in Vendure as incorrect; apply this understanding across TypeORM-using code in Vendure.
There was a problem hiding this comment.
I double-checked and isRunningFromVendureCli() in core/src/migrate.ts:256 uses != null (loose inequality), not === 'true' as I initially stated. So = undefined does coerce to the string "undefined" which satisfies != null, meaning the flag technically leaks. In practice it doesn't cause a bug since the doctor command exits after running and no migration code shares the same process.
There was a problem hiding this comment.
@Ryrahul, thanks for double-checking — that confirms my original analysis was correct. The string "undefined" does satisfy != null, so the flag technically leaks within the process.
You're right that it's benign here because the doctor command exits before any migration code could run in the same process. But it's still a latent footgun if the check order ever changes or the function gets called in a context that doesn't exit immediately.
Would you like me to open a follow-up issue to track a repo-wide cleanup (replacing = undefined with delete process.env.VENDURE_RUNNING_IN_CLI across config-check.ts, schema.ts, migrate.ts, and any other occurrences)?
✏️ Learnings added
Learnt from: Ryrahul
Repo: vendurehq/vendure PR: 4777
File: packages/cli/src/commands/doctor/checks/config-check.ts:87-87
Timestamp: 2026-05-24T20:06:04.223Z
Learning: In vendurehq/vendure, `isRunningFromVendureCli()` (e.g. packages/core/src/migrate.ts line 256) checks `process.env.VENDURE_RUNNING_IN_CLI != null` using loose inequality. Assigning `process.env.VENDURE_RUNNING_IN_CLI = undefined` coerces to the string `"undefined"`, which satisfies `!= null`, so the flag leaks instead of being cleared. The correct cleanup is `delete process.env.VENDURE_RUNNING_IN_CLI`. Existing code in schema.ts and migrate.ts also uses `= undefined`; a repo-wide cleanup is tracked as a follow-up.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: michaelbromley
Repo: vendurehq/vendure PR: 4488
File: packages/core/src/service/services/order.service.ts:1907-1940
Timestamp: 2026-03-10T21:20:55.233Z
Learning: In Vendure's TypeORM usage, when transactionSupport is set to 'nested' (SQLite via AbstractSqliteQueryRunner, MySQL, PostgreSQL), nested connection.withTransaction() calls inside an active transaction should not be treated as errors. TypeORM will increase transactionDepth and use a SAVEPOINT (typeorm_N) for savepoint-level isolation. TransactionAlreadyStartedError should only be thrown when transactionSupport === 'simple'. Do not flag nested withTransaction usage in Vendure as incorrect; apply this understanding across TypeORM-using code in Vendure.
| for (const entry of entries) { | ||
| // Skip the target package itself and hidden directories | ||
| if (entry === targetPkg || entry.startsWith('.')) continue; |
There was a problem hiding this comment.
Duplicate package scan has a false-negative blind spot.
Skipping hidden directories means nested installs under hidden package-manager folders are never scanned, so singleton duplication checks can incorrectly pass even when multiple versions are installed.
Suggested direction
- // Skip the target package itself and hidden directories
- if (entry === targetPkg || entry.startsWith('.')) continue;
+ // Skip the target package itself
+ if (entry === targetPkg) continue;Then explicitly handle known hidden dependency stores (or switch to lockfile/package-manager metadata based detection) to avoid deep/manual traversal pitfalls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/doctor/checks/dependency-check.ts` around lines 195
- 197, The current loop in dependency-check.ts that skips entries starting with
'.' (iterating "for (const entry of entries)") causes hidden package-manager
stores to be ignored and yields false negatives; update the logic so you no
longer blanket-skip hidden dirs: still skip the exact targetPkg, but explicitly
handle known hidden dependency stores (e.g. node_modules/.pnpm,
node_modules/.store, .yarn, pnpm/yarn global stores) by recursing into them or
treating them as candidate locations for package installs, and/or replace the
filesystem deep-scan with lockfile/package-manager metadata-based detection as
an alternative; locate and change the loop around entries/targetPkg in
dependency-check.ts to implement these checks and additions.
|
Hi, could you make this PR against |
Done 👍 |
|
Some initial feedback from testing this on a couple of local Vendure projects: 
Overall I think it’s mainly the presentation of the results that needs some work. Each line needs some visual indicator as to whether it is OK, warn, or fail. Additional feedback from Claude review:
|
Yeah sure , thanks for the feedback . Will look into it a bit more , it definitely needs bit more better presentation |
|
@michaelbromley One question on the version mismatch severity — I’ve updated the logic to distinguish between patch and minor/major mismatches: Patch mismatch → warn Dependencies warn Dependency warnings detected
Mismatched @vendure/* package versions (patch):
3.6.3: @vendure/core, @vendure/common, @vendure/dashboard
3.6.2: @vendure/email-plugin
Result: passed (1 warning)Minor/major mismatch → fail Dependencies fail Dependency issues detected
Mismatched @vendure/* package versions (minor/major):
3.7.0: @vendure/core, @vendure/common
3.6.3: @vendure/email-plugin, @vendure/dashboard
Result: failed (1 failure)Does this feel like the right balance, or would you prefer all |
2 participants
Fixes #4776
Description
Adds a new
vendure doctorCLI command that runs diagnostic checks on a Vendure project and produces actionable output for debugging.Implemented checks
@vendure/*package versions, duplicate singleton dependencies (graphql,typeorm,@nestjs/core,@nestjs/common,@apollo/server), verifies DB driver is installedpreBootstrapConfig()to validate custom fields and entities, checks plugin compatibility rangessynchronize: true--profile production) Checks for unsafe production settings:disableAuth, default superadmin credentials, missing cookie secret, introspection/playground/debug enabled, broad CORS, in-memory job queue/cache/session strategies, missing asset storageCLI options
Example output
What is NOT included (documented as TODO for follow-up)
createSchemaBuilder().log()pattern fromcore/src/migrate.ts)Both are documented as TODOs in
database-check.tswith implementation hints.Existing utilities reused
analyzeProject(),VendureConfigRef,loadVendureConfigFile()from CLI shared modulespreBootstrapConfig(),getCompatibility(),VENDURE_VERSION,resetConfig()from@vendure/coredetectMonorepoStructure(),findPackageJsonWithDependency()from CLI monorepo utilsBreaking changes
None. This is a new command with no changes to existing functionality.
Checklist