fix(cli): actionable config-scaffold errors instead of dead-ends#580
Conversation
🦋 Changeset detectedLatest commit: edf4db2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates CLI error handling, config scaffolding, and ChangesConfig guidance, scaffolding, and install flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/db/config-scaffold.ts`:
- Line 110: The CI check in the config scaffold command is duplicated and should
use the shared helper for consistency with resolveDatabaseUrl. Update the isTTY
logic in config-scaffold to call isCiEnv() instead of reading process.env.CI
directly, keeping the existing stdin TTY check intact and reusing the same CI
detection path used elsewhere.
In `@packages/cli/tests/e2e/config-scaffold.e2e.test.ts`:
- Around line 42-49: The e2e test in config-scaffold.e2e.test.ts is leaking
DATABASE_URL from the parent environment, so the `run` call can behave
nondeterministically. Update the `run(['eql', 'install'], ...)` setup to
explicitly unset or override DATABASE_URL in the child process environment,
while preserving required vars like PATH if `run` does not merge env by default.
Keep the existing assertions around `stash.config.ts` and the error output
intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e65dc0b-e0cb-47b1-937c-6df51433fb00
📒 Files selected for processing (6)
.changeset/stash-cli-config-scaffold-dx.mdpackages/cli/src/commands/db/__tests__/config-scaffold.test.tspackages/cli/src/commands/db/config-scaffold.tspackages/cli/src/commands/db/install.tspackages/cli/src/config/index.tspackages/cli/tests/e2e/config-scaffold.e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/stash-cli-config-scaffold-dx.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/config/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/db/install.ts (1)
100-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
--database-urlbefore loading/scaffolding project config.Line 100 takes the existing config branch before the explicit one-shot flag check, so
stash eql install --database-url ...can still loadstash.config.tsand then scaffold a missing encryption client via Line 153. That contradicts the one-shot contract described in this PR.Proposed fix
async function resolveInstallContext( options: InstallOptions, s: ReturnType<typeof p.spinner>, ): Promise<{ databaseUrl: string; clientPath: string | null }> { + // One-shot install (explicit --database-url): don't touch the project. + if (options.databaseUrl) { + const databaseUrl = await resolveDatabaseUrl({ + databaseUrlFlag: options.databaseUrl, + supabase: options.supabase, + }) + return { databaseUrl, clientPath: null } + } + if (findConfigFile(process.cwd())) { s.start('Loading stash.config.ts...') const config = await loadStashConfig({ databaseUrlFlag: options.databaseUrl, supabase: options.supabase, @@ - // One-shot install (explicit --database-url): don't touch the project. - if (options.databaseUrl) { - return { databaseUrl, clientPath: null } - } - const clientPath = await offerStashConfig() return { databaseUrl, clientPath } }Also applies to: 153-155
🤖 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/db/install.ts` around lines 100 - 107, The install flow in db/install.ts is checking findConfigFile() before honoring the one-shot --database-url flag, which lets install still load stash.config.ts and scaffold a client. Update the main command path and the client-scaffolding branch in install() so the explicit options.databaseUrl/one-shot flow is handled first and immediately returns without touching project config or generating the encryption client. Use the existing symbols loadStashConfig, findConfigFile, and the install command’s scaffold logic to keep the override behavior consistent.
🧹 Nitpick comments (1)
packages/cli/tests/e2e/config-scaffold.e2e.test.ts (1)
52-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd exit code assertion and a test-level timeout.
The test verifies no files are scaffolded but doesn't assert the command actually failed (
r.exitCode). Adding an exit code check strengthens the "one-shot fails cleanly" intent. A vitest timeout (e.g., 15s) is also a prudent safety net —connect_timeout=2bounds the DB connection, but other phases could theoretically hang.♻️ Proposed additions
it('`eql install --database-url` is one-shot — it never scaffolds project files', async () => { // An explicit --database-url means "install EQL against this DB now"; it // must not drop a stash.config.ts or an encryption client into the project. // The bogus URL fails fast at connect (past the config stage), which is all // we need to observe the no-scaffold behaviour. const r = await run( [ 'eql', 'install', '--database-url', 'postgres://u:p@127.0.0.1:1/db?connect_timeout=2', ], { cwd: tmpDir }, ) + expect(r.exitCode).not.toBe(0) expect(fs.existsSync(path.join(tmpDir, 'stash.config.ts'))).toBe(false) expect( fs.existsSync(path.join(tmpDir, 'src', 'encryption', 'index.ts')), ).toBe(false) expect(r.output).not.toContain('Created stash.config.ts') expect(r.output).not.toContain('Cannot find module') -}, 15_000) +})As per coding guidelines, e2e tests in
tests/e2e/**.e2e.test.tsshould cover exit codes for top-level commands.🤖 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/tests/e2e/config-scaffold.e2e.test.ts` around lines 52 - 73, The e2e test for the `eql install --database-url` flow should also verify the command failed and won’t hang. Update the `it(...)` case in `config-scaffold.e2e.test.ts` to assert `r.exitCode` is non-zero, and add a test-level timeout to this spec so the top-level `run(...)` invocation in the `eql install` scenario is bounded even if later phases stall.Source: Coding guidelines
🤖 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/db/install.ts`:
- Line 120: The direct-install dry-run path is mutating the project before the
dry-run guard runs, because `offerStashConfig()` and `ensureEncryptionClient()`
can write scaffold/client files in `install.ts` even when no changes should be
made. Move the dry-run handling earlier in the flow so the `db install` command
short-circuits before any file-writing work, and make sure the
`offerStashConfig()` and `ensureEncryptionClient()` calls are only reached when
the command is not running in dry-run mode. Keep the existing dry-run message
behavior, but ensure it is reached without creating `stash.config.ts` or the
client file.
---
Outside diff comments:
In `@packages/cli/src/commands/db/install.ts`:
- Around line 100-107: The install flow in db/install.ts is checking
findConfigFile() before honoring the one-shot --database-url flag, which lets
install still load stash.config.ts and scaffold a client. Update the main
command path and the client-scaffolding branch in install() so the explicit
options.databaseUrl/one-shot flow is handled first and immediately returns
without touching project config or generating the encryption client. Use the
existing symbols loadStashConfig, findConfigFile, and the install command’s
scaffold logic to keep the override behavior consistent.
---
Nitpick comments:
In `@packages/cli/tests/e2e/config-scaffold.e2e.test.ts`:
- Around line 52-73: The e2e test for the `eql install --database-url` flow
should also verify the command failed and won’t hang. Update the `it(...)` case
in `config-scaffold.e2e.test.ts` to assert `r.exitCode` is non-zero, and add a
test-level timeout to this spec so the top-level `run(...)` invocation in the
`eql install` scenario is bounded even if later phases stall.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f188809-a948-4d2b-b685-3be5f8d56507
📒 Files selected for processing (3)
.changeset/stash-cli-config-scaffold-dx.mdpackages/cli/src/commands/db/install.tspackages/cli/tests/e2e/config-scaffold.e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/stash-cli-config-scaffold-dx.md
There was a problem hiding this comment.
Pull request overview
This PR improves the stash CLI’s config-scaffolding and database URL resolution flows to avoid “dead-end” errors, particularly for standalone usage (npx stash ...) in projects that don’t yet have stash.config.ts or local dependencies installed.
Changes:
- Make missing-config and missing-module failures produce runner-aware, actionable guidance (
stash init/stash eql install) instead of dead-end instructions or raw loader errors. - Decouple
stash eql installfrom requiringstash.config.tsby resolvingDATABASE_URLdirectly when no config exists, and treating--database-urlas a one-shot “don’t scaffold” signal. - Add unit + E2E coverage for the new scaffold/loader behaviors and updated help text.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/config/index.ts | Improves missing-config messaging and translates missing-module loader failures into actionable install/init guidance. |
| packages/cli/src/config/database-url.ts | Clarifies resolver documentation; resolver remains the single source of truth for URL resolution priority. |
| packages/cli/src/commands/db/install.ts | Changes eql install to resolve DB URL without requiring config; respects one-shot --database-url behavior. |
| packages/cli/src/commands/db/config-scaffold.ts | Makes config creation optional/offer-based and supports non-interactive scaffolding paths. |
| packages/cli/src/commands/db/tests/config-scaffold.test.ts | Adds unit tests for offerStashConfig behavior (non-interactive create, detection, no overwrite). |
| packages/cli/src/tests/config.test.ts | Adds unit tests for actionable missing-config and missing-module guidance behavior. |
| packages/cli/tests/e2e/config-scaffold.e2e.test.ts | Adds E2E coverage in a temp project outside the monorepo to catch standalone npx failure modes. |
| packages/cli/src/cli/registry.ts | Updates shared --database-url flag description to document resolution precedence. |
| packages/cli/src/bin/main.ts | Updates --help output to describe --database-url resolution precedence. |
| .changeset/stash-cli-config-scaffold-dx.md | Adds a patch changeset documenting the user-facing CLI DX fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/cli/src/commands/db/install.ts (1)
107-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
resolveInstallContextcan write scaffold files before the dry-run guard.
resolveInstallContextcallsofferStashConfig(line 131) which can writestash.config.ts— in'ensure'mode unconditionally, in'offer'mode when TTY and user accepts. Neither path checksoptions.dryRun. If the dry-run guard ininstallCommandis still downstream of this call,eql install --dry-runcan modify the project before printing "no changes will be made."This was flagged in a prior review and does not appear to have been addressed. Verify the guard location and add an early return if needed:
🛡️ Proposed fix
const mode = options.scaffoldConfig ?? 'offer' if (mode === 'skip') { return { databaseUrl, clientPath: null } } + if (options.dryRun) { + return { databaseUrl, clientPath: null } + } + const clientPath = await offerStashConfig({ ensure: mode === 'ensure' }) return { databaseUrl, clientPath }Run the following script to verify the dry-run guard location in
installCommand:#!/bin/bash # Description: Check where the dry-run guard sits relative to resolveInstallContext # and ensureEncryptionClient calls in installCommand. # Map the file structure ast-grep outline packages/cli/src/commands/db/install.ts --items all --type function # Then read the installCommand function body to find the dry-run guard sed -n '135,230p' packages/cli/src/commands/db/install.ts🤖 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/db/install.ts` around lines 107 - 133, `resolveInstallContext` may still create or scaffold `stash.config.ts` before the dry-run check runs, so `installCommand` can mutate the project during `--dry-run`. Move the dry-run guard so it happens before calling `resolveInstallContext`, or add an early return inside `resolveInstallContext` when `options.dryRun` is set, and make sure the `offerStashConfig` path is never reached in dry-run mode. Use `installCommand` and `resolveInstallContext` as the main points to verify the guard placement.packages/cli/tests/e2e/config-scaffold.e2e.test.ts (1)
42-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIsolate
DATABASE_URLfrom the parent environment.
run(['eql', 'install'], { cwd: tmpDir })doesn't passenv, so the child process inherits the parent environment. IfDATABASE_URLis set in CI or the local shell, the CLI resolves a URL and proceeds past the "Cannot resolve DATABASE_URL" message, causing this test to fail nondeterministically.This was flagged in a prior review and remains unaddressed.
🛡️ Proposed fix
- const r = await run(['eql', 'install'], { cwd: tmpDir }) + const r = await run(['eql', 'install'], { + cwd: tmpDir, + env: { ...process.env, DATABASE_URL: '' }, + })If
runmerges rather than replacesenv, adjust accordingly to ensureDATABASE_URLis unset while preserving required vars.🤖 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/tests/e2e/config-scaffold.e2e.test.ts` around lines 42 - 49, The e2e test for config scaffolding is inheriting the parent shell’s DATABASE_URL, making `run(['eql', 'install'], { cwd: tmpDir })` nondeterministic. Update the `run` invocation in `config-scaffold.e2e.test.ts` to explicitly isolate the child environment by unsetting DATABASE_URL while preserving any required variables, so the `stash.config.ts` failure path is exercised consistently. Use the `run` helper call in this test as the fix point and keep the existing assertions against the unresolved DATABASE_URL error.
🤖 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/tests/e2e/config-scaffold.e2e.test.ts`:
- Around line 75-92: The non-interactive `eql install` e2e test is accidentally
relying on an inherited `DATABASE_URL`, so it may not cover the missing-env path
reliably. Update the `config-scaffold.e2e.test.ts` case that calls `run(['eql',
'install'], ...)` to explicitly clear `DATABASE_URL` in the test env, keeping
the existing assertions that no `stash.config.ts` or client scaffold is written
and that no module import errors appear.
---
Duplicate comments:
In `@packages/cli/src/commands/db/install.ts`:
- Around line 107-133: `resolveInstallContext` may still create or scaffold
`stash.config.ts` before the dry-run check runs, so `installCommand` can mutate
the project during `--dry-run`. Move the dry-run guard so it happens before
calling `resolveInstallContext`, or add an early return inside
`resolveInstallContext` when `options.dryRun` is set, and make sure the
`offerStashConfig` path is never reached in dry-run mode. Use `installCommand`
and `resolveInstallContext` as the main points to verify the guard placement.
In `@packages/cli/tests/e2e/config-scaffold.e2e.test.ts`:
- Around line 42-49: The e2e test for config scaffolding is inheriting the
parent shell’s DATABASE_URL, making `run(['eql', 'install'], { cwd: tmpDir })`
nondeterministic. Update the `run` invocation in `config-scaffold.e2e.test.ts`
to explicitly isolate the child environment by unsetting DATABASE_URL while
preserving any required variables, so the `stash.config.ts` failure path is
exercised consistently. Use the `run` helper call in this test as the fix point
and keep the existing assertions against the unresolved DATABASE_URL error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 605f8d38-d0ae-478b-9784-4aa229e36539
📒 Files selected for processing (9)
packages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/__tests__/config-scaffold.test.tspackages/cli/src/commands/db/config-scaffold.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/init/steps/__tests__/install-eql.test.tspackages/cli/src/commands/init/steps/install-eql.tspackages/cli/src/config/database-url.tspackages/cli/tests/e2e/config-scaffold.e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/config/database-url.ts
#579) Two related config-scaffold failures reported from testing the 0.17 CLI: - `eql status` (any read command) in a project with no stash.config.ts printed a "hand-write this file" message and exited — no scaffold, no pointer to the commands that set it up (#578). - `npx stash eql install` scaffolded a stash.config.ts, then immediately crashed with a raw `Cannot find module 'stash'`: the config imports `stash` (and the client imports `@cipherstash/stack`), which resolve only once those are project dependencies — something only `stash init` did. Run standalone via npx, they live in the npx cache, not the project (#579). Fixes: - Missing-config message now recommends `stash init` / `stash eql install` (runner-aware) before the hand-written fallback. - `eql install` checks for the config's dependencies after scaffolding and offers to install the missing ones (or prints the exact commands in non-interactive contexts) before jiti loads the config. - `loadStashConfig` translates a missing-module load failure into the same actionable guidance for every command, instead of a jiti/Node stack trace. Tests: config-scaffold.ts had zero tests and the install-command tests run inside the monorepo where `stash` always resolves via the workspace — which is why the crash escaped CI. Adds unit coverage for the dependency guard and the load-error translation (mocked jiti), plus e2e coverage in a throwaway temp project for the missing-config and missing-deps paths.
Follow-up to the previous commit: rather than guarding the scaffolded config's dependencies, remove the requirement entirely. `eql install` only needs a database URL, so it now resolves one directly (`--database-url` → env → supabase → prompt) via resolveDatabaseUrl instead of scaffolding a config and loading it. - An existing stash.config.ts is still authoritative (later workflow commands load the encryption client through it) — load it when present. - Without one, resolve the URL directly and *offer* to scaffold a config for the rest of the workflow (auto-creates it in non-interactive contexts) — it's a convenience, never a prerequisite. A standalone `npx stash eql install --database-url ...` now works in a bare project with zero deps: there's no `import 'stash'` to resolve, so the `Cannot find module 'stash'` crash can't happen. - Drop the now-redundant dependency guard (ensureConfigDependencies / missingConfigDependencies) and revert the isPackageInstalled cwd param. - Keep the improved messages: the actionable missing-config error (#578) and loadStashConfig's missing-module translation (#579) still cover the commands that genuinely load a config. Replaces the guard unit tests with offerStashConfig coverage; the e2e now asserts `eql install` resolves the URL directly and fails cleanly (no config written, no module crash) when no URL is available.
…olding) An explicit --database-url signals "install EQL against this DB now", so resolveInstallContext no longer scaffolds project files in that case — neither stash.config.ts nor the encryption client. Without the flag (URL from env / supabase / prompt) the config is still offered/created for the rest of the workflow. An existing config is honoured either way.
Make the precedence discoverable in three places: - `resolveDatabaseUrl` gets a function-level JSDoc with the authoritative order (flag → DATABASE_URL env → supabase status → prompt → fail), plus the nuance that stash.config.ts is a container, not a separate tier — its default `databaseUrl: await resolveDatabaseUrl()` re-runs the same chain, so the flag wins even with a config present (except a hand-edited literal, which bypasses the resolver). The module header's duplicated list now points here. - The registry `--database-url` descriptor (feeds `stash manifest`). - The `stash --help` banner's DB / EQL Flags entry.
The one-shot rule was keyed on `options.databaseUrl` being set, but that field is populated by two callers meaning different things: the CLI sets it only when `--database-url` is passed (a genuine one-shot), while `stash init` always sets it (a resolved URL, just to avoid re-prompting). So init tripped the one-shot branch and never scaffolded stash.config.ts — leaving every downstream command to dead-end on "Could not find stash.config.ts". Make the scaffold intent explicit and caller-owned instead of inferred: - InstallOptions gains `scaffoldConfig: 'ensure' | 'offer' | 'skip'`. - runInstall (CLI) passes 'skip' when --database-url is present, else 'offer'. - init's install-eql passes 'ensure' (create without a yes/no prompt). - resolveInstallContext switches on scaffoldConfig, not options.databaseUrl. - offerStashConfig gains an `ensure` mode; also folds the duplicated writeFileSync+log into a `writeStashConfig` helper. Tests: new install-eql.test.ts asserts init requests scaffoldConfig: 'ensure' (the exact wiring that regressed), plus an ensure-mode scaffold test. Fixed the stale install-eql comment (installCommand now scaffolds but no longer loads the config during init).
…#5) offerStashConfig returned a truthy DEFAULT_CLIENT_PATH even when it created nothing, so installCommand's `if (clientPath)` guard scaffolded the encryption client anyway. Three related fixes, all on this one surface: - #2: return null when no config is created (declined / cancelled / existing), so the caller skips the client scaffold too. Declining now touches nothing. - #4: in 'offer' mode a non-interactive run no longer silently writes a config (+ client) into a bare project — those files import stash / @cipherstash/stack and would just re-defer the MODULE_NOT_FOUND crash to the next command. It returns null; the missing-config guidance points the user at `stash init`. ('ensure'/init still creates unconditionally; init installs the deps.) - #5: use the shared isCiEnv() (handles CI=1 / CI=TRUE) instead of the raw `process.env.CI !== 'true'`, matching the sibling resolveDatabaseUrl so the two steps classify the same environment consistently. Tests: offerStashConfig now asserts null + no write for the non-interactive offer and existing-config cases; a new e2e drives `eql install` with the URL from env (non-TTY) and asserts neither a config nor a client is written.
… missing (review #3, #6) The missing-package translation only wrapped the CONFIG load, which imports bare `stash` — so its `@cipherstash/stack` branch never fired. The package actually fails to resolve in the CLIENT file (imported by loadEncryptConfig, used by db push / schema build / encrypt), whose catch dumped a raw jiti stack trace. - #3: apply the translation to loadEncryptConfig too, so a missing @cipherstash/stack (incl. subpaths like @cipherstash/stack/schema) yields the same "install it / run stash init" guidance for every command, not a trace. - #6: replace the brittle substring matching (includes("'stash'")) with a shared, subpath-aware helper. New src/module-error.ts holds the primitives (isModuleNotFound + moduleNotFoundSpecifier's `Cannot find (module|package)` regex); native.ts now reuses them instead of its own copy, and a new config/missing-package.ts reduces a specifier to its package name before matching (so @cipherstash/stack/schema resolves to @cipherstash/stack). Tests: unit coverage for missingCipherStashPackage (subpath, non-matches, code gate) and a loadEncryptConfig test asserting the client-load path translates a missing @cipherstash/stack into guidance rather than a raw trace.
… walk (#7, #8, #10) - #7: collapse three copies of the default encryption-client path (DEFAULT_ENCRYPT_CLIENT_PATH in config/index.ts, DEFAULT_CLIENT_PATH in config-scaffold.ts, and a private copy in build-schema.ts) into one exported DEFAULT_CLIENT_PATH in config/index.ts, imported everywhere — so the scaffold default and the Zod default can't drift. - #8: move the "`<pkg>` is not installed" guidance into messages.ts (db.missingCipherStashPackage) per the assertion-stable-strings convention. - #10: loadStashConfig accepts an optional knownConfigPath; resolveInstallContext passes the path it already located so the cwd→root filesystem walk runs once instead of twice on the existing-config path. No behaviour change; message output is byte-identical. Full unit + e2e green.
…, doc caveat
- Dry run no longer mutates the project (CodeRabbit): `eql install --dry-run`
reached offerStashConfig()/ensureEncryptionClient() before the dry-run guard,
so a config or client file could be written despite "no changes will be made".
resolveInstallContext now skips scaffolding under dryRun, and the client
scaffold is guarded on `!options.dryRun` (an existing config still yields a
clientPath). Adds an e2e that asserts no client file is written on a dry run.
- Isolate DATABASE_URL in the "resolves URL directly" e2e (CodeRabbit): pass
`env: { DATABASE_URL: '' }` so a value in the parent env can't leak in and
resolve past the guidance the test asserts on.
- Document the literal-databaseUrl exception in the --database-url help
(Copilot): a hand-set literal `databaseUrl` in stash.config.ts bypasses the
resolver and wins over the flag/env/etc.
coderdan
force-pushed
the
fix/cli-config-scaffold-dx
branch
from
cbb3079 to
7877bb4
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed with a multi-angle pass (correctness, removed-behavior, cross-file, reuse/simplification/efficiency/altitude), with each candidate independently verified against the head commit. The decoupling of eql install from the config is the right root-cause fix, tests are well-aimed at the actual failure modes, and several plausible-sounding concerns were checked and refuted (jiti 2.7.0 rethrows Node's coded module error unwrapped, so the MODULE_NOT_FOUND translation is sound; subpath skew on @cipherstash/stack throws ERR_PACKAGE_PATH_NOT_EXPORTED, which correctly falls through to the raw error). Four findings survived verification, most-severe first:
1. packages/cli/src/commands/db/install.ts:111 — --database-url is silently ignored when a parent-directory config hard-codes a literal databaseUrl (CONFIRMED, narrow trigger)
resolveInstallContext uses findConfigFile(process.cwd()), which walks up parent directories. Run eql install --database-url postgres://staging... from a monorepo subdirectory whose parent has a hand-edited config with a literal databaseUrl: 'postgres://prod...', and the literal wins — the resolver (and the flag threaded through withResolverContext) is never invoked, so EQL installs against the parent config's database with no warning. This is a behavior change: the old ensureStashConfig checked cwd only and would scaffold a fresh cwd config (whose resolveDatabaseUrl() honours the flag). The trigger is narrow (scaffolded configs never emit a literal), but the failure is "installed into the wrong database, silently." Consider logging which config was loaded from a parent directory, or warning when --database-url was passed but didn't determine the final URL.
2. packages/cli/src/commands/db/install.ts:174 — one-shot --database-url still scaffolds the client file when a config exists (PLAUSIBLE — docs/behavior mismatch)
The changeset promises an explicit --database-url run "leaves the project untouched (no config or client scaffolded)". But when a stash.config.ts already exists and points at a not-yet-created client, resolveInstallContext takes the config branch — which never consults scaffoldConfig: 'skip' — and returns a non-null clientPath, so ensureEncryptionClient writes src/encryption/index.ts into the project. The comment at install.ts:170-173 suggests this is deliberate, but then the changeset/PR wording overstates the guarantee. Either honour 'skip' in the config branch too, or scope the "untouched" claim to bare projects (and ideally add a non-dry-run test for the existing-config + --database-url case — the suite only covers it under --dry-run).
3. packages/cli/src/config/database-url.ts:153-160 — orphaned duplicate JSDoc on resolveDatabaseUrl
The new comprehensive doc block was added below the old one, so two adjacent /** */ blocks now document the same function; only the second attaches to the symbol. Delete the old block (its content is subsumed).
4. packages/cli/src/commands/db/config-scaffold.ts:136 — interactivity gate re-derived inline
Boolean(process.stdin.isTTY) && !isCiEnv() duplicates the identical isInteractive computation in config/database-url.ts:223 (and similar sites elsewhere). config/tty.ts exists precisely so every gate checks the same way — worth extracting an isInteractive() helper there so offerStashConfig can't diverge from the URL resolver it runs alongside. Minor.
Refuted during verification (for the record): --database-url=<url> equals-form parsing gap (pre-existing parser limitation, unchanged by this PR); missing-package misdiagnosis on subpath version skew (unreachable given the exports map); jiti wrapping module errors and defeating the translation (jiti 2.7.0 propagates the raw coded error).
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Provided some feedback - I'll leave it to you @coderdan to decide whether to fix in this this PR or in a follow-up.
Approved.
…config Four findings from @freshtonic's review: 1. `--database-url` was silently overridden by a parent-directory config with a literal `databaseUrl` (findConfigFile walks up). A one-shot install (scaffoldConfig 'skip') now bypasses config loading entirely, so the flag always wins and no parent config can redirect the install. For the other modes, warn when a config's literal databaseUrl overrides the flag. 2. One-shot `--database-url` no longer scaffolds the client file when a config already exists — the config branch is skipped, so the project is truly left untouched. Added a non-dry-run e2e covering existing-config + --database-url. 3. Removed the orphaned duplicate JSDoc block on resolveDatabaseUrl. 4. Extracted shared isInteractive() into config/tty.ts and routed the DATABASE_URL resolver, config scaffolder, and Supabase install-mode gate through it instead of re-deriving `isTTY && !CI` inline.
|
Thanks @freshtonic — addressed all four in edf4db2. 1. 2. One-shot 3. Orphaned duplicate JSDoc on 4. Interactivity gate re-derived inline — extracted Full suites green: 406 unit + 55 e2e (was 54), Biome + tsc clean on the changed files. |
Fixes two config-scaffold dead-ends reported from testing the 0.17 CLI.
Closes #578
Closes #579
Issue 1 — commands dead-end on a missing config (#578)
eql status(and any command that loads the encryption client) in a project without astash.config.tsprinted a hand-write-it message and cancelled — it never pointed at the commands that set it up.Fix: the missing-config message now recommends
stash init(full setup) /stash eql install(scaffolds a config) — runner-aware (npx/pnpm dlx/ …) — before the hand-written fallback.Issue 2 —
eql installdoesn't actually need a config (#579)The original crash:
eql installscaffolded astash.config.ts(whichimportsstash) and immediately loaded it — butstash/@cipherstash/stackare only project dependencies afterstash init. Run standalone vianpx, they're in the npx cache, not the project, so jiti couldn't resolve the config's import.Fix (the real root cause):
eql installonly needs a database URL — it reads nothing else off the config that isn't otherwise available. So it now resolves the URL directly (--database-url→DATABASE_URL→supabase status→ prompt) viaresolveDatabaseUrl, instead of requiring a config:stash.config.tsis still authoritative (later workflow commands —db push,schema build,encrypt *— load the client through it), so it's loaded when present.npx stash eql install --database-url <url>now works in a bare project with zero dependencies — there's noimport 'stash'to resolve, so the crash can't happen.--database-urlis treated as a one-shot install: it leaves the project untouched (nostash.config.tsor client scaffolded). Without the flag, a config is offered for the rest of the workflow.As a safety net,
loadStashConfigstill translates a missing-module load failure (a project that has a config but lacks the CLI packages) into actionable guidance for every command, rather than a jiti/Node stack trace.Why no test caught it
db/config-scaffold.tshad zero tests, and the install-command tests run inside the monorepo wherestashalways resolves via the workspace self-reference — so the npx-standalone failure mode was never exercised. (That same quirk means theloadStashConfigcatch can't be reproduced end-to-end in-repo; it's covered by a mocked-jiti unit test, and the e2e file documents why.)Tests
src/__tests__/config.test.ts— missing-config guidance (stash CLI: read commands dead-end when stash.config.ts is missing (no scaffold, no guidance) #578); missing-module → guidance for bothstashand@cipherstash/stack(stash CLI: standalonenpx stash eql installscaffolds a stash.config.ts that can't load (Cannot find module 'stash') #579); unrelated load errors still surface raw.src/commands/db/__tests__/config-scaffold.test.ts—offerStashConfig: non-interactive create, detected-client-path, never-overwrite.tests/e2e/config-scaffold.e2e.test.ts— runs the built CLI in a throwaway temp project (outside the monorepo): missing-config read command (stash CLI: read commands dead-end when stash.config.ts is missing (no scaffold, no guidance) #578), andeql installresolving the URL directly and failing cleanly with no config written / no module crash (stash CLI: standalonenpx stash eql installscaffolds a stash.config.ts that can't load (Cannot find module 'stash') #579).All unit + e2e pass; Biome clean. Changeset:
stashpatch.Summary by CodeRabbit
stash.config.tsis missing or can’t be loaded, replacing raw module-resolution noise with actionable next steps (stash init/stash eql install) tailored to the package manager.stash eql installno longer depends onstash.config.ts; it resolvesdatabaseUrlfrom--database-url→ env → Supabase status → prompt, honoring an existing config for later workflow steps.--database-urlnow runs as a one-shot install that does not scaffold files.--database-urlhelp text to reflect the resolution order and one-shot behavior.Closes #581