feat(cli): add --introspect option for studio command for bootstrapping without schema - #2775
Open
jiashengguo wants to merge 6 commits into
Open
feat(cli): add --introspect option for studio command for bootstrapping without schema #2775jiashengguo wants to merge 6 commits into
jiashengguo wants to merge 6 commits into
Conversation
…schema file is found
…r options to PullOptions and exporting related functions
Contributor
📝 WalkthroughWalkthroughThe CLI adds package-aware database driver loading and an optional ChangesCLI introspection and driver loading
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an --introspect option to the CLI’s Studio/proxy command flow to bootstrap a project when no ZModel schema file is present, including auto-installing the correct DB driver and generating a schema via the existing pull/introspection pipeline.
Changes:
- Adds
--introspectflag to the CLI command definition and routes schema resolution through a newresolveSchemahelper. - Introduces shared optional-dependency helpers (
isPackageInstalled,loadPackage) and uses them across DB introspection providers and proxy dialect creation. - Extends
runPullto accept explicitprovider/databaseUrloverrides to support schema generation from an introspected database.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/index.ts | Adds the --introspect CLI option to the command. |
| packages/cli/src/actions/proxy.ts | Implements --introspect schema resolution, driver installation, schema generation, and generation follow-up. |
| packages/cli/src/actions/db.ts | Exports runPull and allows provider/URL overrides for introspection-driven generation. |
| packages/cli/src/actions/action-utils.ts | Adds isPackageInstalled and loadPackage helpers for optional dependency loading. |
| packages/cli/src/actions/pull/provider/sqlite.ts | Switches SQLite introspection to loadPackage and adds file URL handling. |
| packages/cli/src/actions/pull/provider/postgresql.ts | Switches PostgreSQL introspection to loadPackage. |
| packages/cli/src/actions/pull/provider/mysql.ts | Switches MySQL introspection to loadPackage. |
| packages/cli/test/proxy.test.ts | Adds unit tests for getProviderFromUrl and resolveSchema behaviors. |
| packages/cli/test/action-utils.test.ts | Adds tests for isPackageInstalled and loadPackage. |
Comments suppressed due to low confidence (1)
packages/cli/test/proxy.test.ts:862
- This assertion looks for
zen studio --introspect, butresolveSchemaemitsnpx @zenstackhq/cli studio --introspect .... Usingstudio --introspectas the substring match should make the test align with the actual message formatting.
).rejects.toThrow('zen studio --introspect');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { Client } = await import('pg'); | ||
| const client = new Client({ connectionString }); | ||
| const pg = await loadPackage('pg'); | ||
| const ClientClass: typeof PgClient = pg?.Client || pg?.default?.Client; |
Comment on lines
+147
to
+149
| if (!path.isAbsolute(filePath)) { | ||
| resolvedUrl = path.join(process.cwd(), filePath); | ||
| } |
| port: 3000, | ||
| signatureToleranceSecs: 60, | ||
| }), | ||
| ).rejects.toThrow('zen studio --introspect -d <url>'); |
| urlFromEnv: boolean, | ||
| ): Promise<string> { | ||
| // Write a minimal datasource-only schema so runPull has something to load | ||
| const urlExpr = urlFromEnv ? 'env("DATABASE_URL")' : `'${databaseUrl}'`; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/cli/src/actions/action-utils.ts (1)
386-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
createRequire(package.json)logic.The same
createRequire(path.resolve(process.cwd(), 'package.json'))pattern is repeated 3 times acrossisPackageInstalledandloadPackage. A small shared helper (getProjectRequire()) would reduce duplication and keep the two functions in sync if the resolution base ever changes.🤖 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/actions/action-utils.ts` around lines 386 - 415, Extract the repeated createRequire(path.resolve(process.cwd(), 'package.json')) construction into a shared getProjectRequire() helper. Update isPackageInstalled and both fallback branches in loadPackage to reuse this helper, preserving their existing resolution and error-handling behavior.
🤖 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/actions/action-utils.ts`:
- Around line 396-415: Update loadPackage so it preserves and propagates the
underlying error when the package cannot be loaded, rather than returning null
after all import and require fallbacks fail. Track the original and fallback
failures through the nested dynamic import, projectRequire.resolve, and
projectRequire calls, then throw a diagnostic error that retains the relevant
cause for callers such as createDialect and the database providers.
In `@packages/cli/src/actions/db.ts`:
- Around line 38-39: Add a warning in the introspection flow when the resulting
tables and enums are both empty, regardless of provider or schema selection.
Update the logic around allSchemas and the PostgreSQL introspect result so the
warning clearly indicates that no database objects were found, while preserving
successful empty-result behavior.
In `@packages/cli/src/actions/proxy.ts`:
- Around line 517-530: Restrict the existing default-schema fallback around
getSchemaFile to cases where options.schema was not explicitly provided. When an
explicit --schema path fails, propagate the original CliError instead of
returning zenstack/schema.zmodel; preserve the fallback for missing schemas in
default locations.
- Around line 619-649: Update introspectAndGenerateSchema so the generated
datasource always references env("DATABASE_URL") instead of embedding
databaseUrl when urlFromEnv is false. Preserve the existing provider and runPull
behavior, and ensure the literal connection string is never written to
schema.zmodel.
- Around line 542-544: Update the runGenerate invocation in the introspection
flow to pass options.output as the code generation output path. Preserve the
existing schema, silent, and watch arguments so generated files and subsequent
imports use the same custom output directory resolved by resolveSchema().
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 144-151: Align SQLite relative file URL resolution between the
introspection path and createDialect. Update the resolution logic surrounding
the SQLite constructor and createDialect’s SQLite branch to use the same base
directory, preserving identical database targeting for the --introspect
bootstrap flow; prefer the existing shared resolution behavior rather than
introducing divergent path handling.
In `@packages/cli/test/action-utils.test.ts`:
- Around line 60-62: Update the successful-load test around loadPackage to
assert that the returned module is not null, replacing the insufficient
toBeDefined assertion. Keep the test focused on rejecting loadPackage’s
documented null failure result.
In `@packages/cli/test/proxy.test.ts`:
- Around line 836-846: Update the expected error substrings in the tests around
resolveSchema to match its actual command text: assert the npx `@zenstackhq/cli`
studio prefix and the -d <databaseUrl> placeholder. Apply the same correction to
both affected assertions while preserving the existing test setup and
error-checking behavior.
---
Nitpick comments:
In `@packages/cli/src/actions/action-utils.ts`:
- Around line 386-415: Extract the repeated
createRequire(path.resolve(process.cwd(), 'package.json')) construction into a
shared getProjectRequire() helper. Update isPackageInstalled and both fallback
branches in loadPackage to reuse this helper, preserving their existing
resolution and error-handling behavior.
🪄 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 Plus
Run ID: 312bca78-7c2c-4751-8fea-9074176dca5b
📒 Files selected for processing (9)
packages/cli/src/actions/action-utils.tspackages/cli/src/actions/db.tspackages/cli/src/actions/proxy.tspackages/cli/src/actions/pull/provider/mysql.tspackages/cli/src/actions/pull/provider/postgresql.tspackages/cli/src/actions/pull/provider/sqlite.tspackages/cli/src/index.tspackages/cli/test/action-utils.test.tspackages/cli/test/proxy.test.ts
Comment on lines
+517
to
+530
| // Resolve database URL | ||
| const databaseUrl = options.databaseUrl ?? process.env['DATABASE_URL']; | ||
| if (!databaseUrl) { | ||
| throw new CliError('--introspect requires a database connection — pass -d <url> or set DATABASE_URL.'); | ||
| } | ||
|
|
||
| // If the default schema file already exists, skip introspection | ||
| const defaultSchemaPath = path.resolve('zenstack', 'schema.zmodel'); | ||
| if (fs.existsSync(defaultSchemaPath)) { | ||
| console.log( | ||
| colors.gray('Note: --introspect ignored because a schema file already exists at ' + defaultSchemaPath), | ||
| ); | ||
| return defaultSchemaPath; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Default-schema fallback can mask a genuinely wrong --schema path.
This branch is reached for any CliError thrown by getSchemaFile, including the case where the user passed an explicit --schema <wrong-path> that doesn't exist. If zenstack/schema.zmodel happens to already exist from a prior run, this silently returns that stale file instead of surfacing the original "Schema file not found: <wrong-path>" error — the user never learns their --schema flag was wrong.
Consider only taking this fallback path when the failure was specifically the "not found in default locations" case (e.g., check when options.schema was not explicitly provided), rather than for every CliError from getSchemaFile.
🤖 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/actions/proxy.ts` around lines 517 - 530, Restrict the
existing default-schema fallback around getSchemaFile to cases where
options.schema was not explicitly provided. When an explicit --schema path
fails, propagate the original CliError instead of returning
zenstack/schema.zmodel; preserve the fallback for missing schemas in default
locations.
Comment on lines
+60
to
+62
| it('loads installed package successfully', async () => { | ||
| const mod = await loadPackage('better-sqlite3'); | ||
| expect(mod).toBeDefined(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the successful-load assertion reject null.
loadPackage returns null on failure, but toBeDefined() passes for null; this test stays green when package loading fails.
Proposed fix
const mod = await loadPackage('better-sqlite3');
- expect(mod).toBeDefined();
+ expect(mod).toBeTruthy();📝 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.
Suggested change
| it('loads installed package successfully', async () => { | |
| const mod = await loadPackage('better-sqlite3'); | |
| expect(mod).toBeDefined(); | |
| it('loads installed package successfully', async () => { | |
| const mod = await loadPackage('better-sqlite3'); | |
| expect(mod).toBeTruthy(); |
🤖 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/test/action-utils.test.ts` around lines 60 - 62, Update the
successful-load test around loadPackage to assert that the returned module is
not null, replacing the insufficient toBeDefined assertion. Keep the test
focused on rejecting loadPackage’s documented null failure result.
Comment on lines
+836
to
+846
| it('should throw helpful error without --introspect when DATABASE_URL is not set', async () => { | ||
| const originalEnv = process.env['DATABASE_URL']; | ||
| delete process.env['DATABASE_URL']; | ||
| try { | ||
| await expect( | ||
| resolveSchema({ | ||
| schema: '/nonexistent/path/schema.zmodel', | ||
| port: 3000, | ||
| signatureToleranceSecs: 60, | ||
| }), | ||
| ).rejects.toThrow('zen studio --introspect -d <url>'); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the asserted commands to resolveSchema’s actual error text.
The implementation emits npx @zenstackhq/cli studio ... (and -d <databaseUrl>), not zen studio ...; both assertions currently fail.
Proposed fix
- ).rejects.toThrow('zen studio --introspect -d <url>');
+ ).rejects.toThrow('npx `@zenstackhq/cli` studio --introspect -d <databaseUrl>');
- ).rejects.toThrow('zen studio --introspect');
+ ).rejects.toThrow('npx `@zenstackhq/cli` studio --introspect');Also applies to: 852-862
🤖 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/test/proxy.test.ts` around lines 836 - 846, Update the expected
error substrings in the tests around resolveSchema to match its actual command
text: assert the npx `@zenstackhq/cli` studio prefix and the -d <databaseUrl>
placeholder. Apply the same correction to both affected assertions while
preserving the existing test setup and error-checking behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/actions/pull/provider/sqlite.ts (1)
131-143: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winSQLite
file:/sqlite:scheme stripping is now split across files and only partially implemented. The root cause:sqlite.ts'sintrospect()used to normalizefile:-prefixed connection strings itself; that logic was removed and only partially reimplemented indb.ts'srunPull, covering just thefile:prefix and only the explicit--introspectoverride path.
packages/cli/src/actions/pull/provider/sqlite.ts#L131-L143: reinstate scheme stripping (file:/sqlite:→ raw path) directly beforenew SQLite(connectionString, ...)as a defensive last line of normalization, since callers cannot be relied upon to always pre-resolve the URL.packages/cli/src/actions/db.ts#L137-L149: extend the existingdatabaseUrl.startsWith('file:')branch to also coversqlite:, and consider applying equivalent normalization in the non-overridegetDatasource(model)branch too, since the documented ZenStack SQLite format (url = 'file:./dev.db') needs the same treatment there.packages/cli/src/actions/proxy.ts#L607-L631: either confirmsqlite:is a genuinely supported input format and ensurerunPull/introspect()honor it end-to-end, or drop thesqlite:branch here if it's dead/speculative handling.🤖 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/actions/pull/provider/sqlite.ts` around lines 131 - 143, Normalize SQLite connection schemes consistently: in packages/cli/src/actions/pull/provider/sqlite.ts lines 131-143, strip file: and sqlite: immediately before new SQLite in introspect(); in packages/cli/src/actions/db.ts lines 137-149, extend runPull’s normalization to sqlite: and apply equivalent handling in the non-override getDatasource(model) path; in packages/cli/src/actions/proxy.ts lines 607-631, verify sqlite: is supported end-to-end and retain the branch only if runPull and introspect honor it, otherwise remove it.packages/cli/src/actions/proxy.ts (1)
209-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the schema directory as the SQLite URL base when the path is relative.
outputPathresolves tozenstack/withgetOutputPath(), so relative SQLite URLs likefile:database.sqlite,file:./database.sqlite, orsqlite:database.sqliteare incorrectly resolved from the cwd (/app/database.sqlite) while the generateddatasource.urlpreserves them as relative tozenstack/(/app/database.sqlitefromzenstack/schema.zmodel) or intentionally up one level. Resolvefile:/sqlite:relative URLs againstpath.resolve('zenstack'), not by joining withoutputPath.🤖 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/actions/proxy.ts` around lines 209 - 230, Update createDialect’s SQLite URL resolution to use path.resolve('zenstack') as the base for relative file: and sqlite: URLs, preserving intentional relative paths such as ./ and ../. Do not use outputPath as the URL base; retain absolute URL handling and Windows normalization.
🤖 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.
Outside diff comments:
In `@packages/cli/src/actions/proxy.ts`:
- Around line 209-230: Update createDialect’s SQLite URL resolution to use
path.resolve('zenstack') as the base for relative file: and sqlite: URLs,
preserving intentional relative paths such as ./ and ../. Do not use outputPath
as the URL base; retain absolute URL handling and Windows normalization.
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 131-143: Normalize SQLite connection schemes consistently: in
packages/cli/src/actions/pull/provider/sqlite.ts lines 131-143, strip file: and
sqlite: immediately before new SQLite in introspect(); in
packages/cli/src/actions/db.ts lines 137-149, extend runPull’s normalization to
sqlite: and apply equivalent handling in the non-override getDatasource(model)
path; in packages/cli/src/actions/proxy.ts lines 607-631, verify sqlite: is
supported end-to-end and retain the branch only if runPull and introspect honor
it, otherwise remove it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98c2175a-b357-4c8e-8bfe-ec5f3d6a9336
📒 Files selected for processing (5)
packages/cli/src/actions/action-utils.tspackages/cli/src/actions/db.tspackages/cli/src/actions/proxy.tspackages/cli/src/actions/pull/provider/sqlite.tspackages/cli/test/proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/actions/action-utils.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
--introspectoption to generate a schema from a database when no schema file is available.--introspectvs. missing schema, including better handling ofDATABASE_URLfor SQLite.--providerand--database-url, and exported the updated pull routine.file:URL normalization for more reliable SQLite introspection.