fix(cli): unwrap default export when loading stash.config.ts#375
fix(cli): unwrap default export when loading stash.config.ts#375
Conversation
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 46 seconds.Comment |
Closes #374. `loadStashConfig` was passing `interopDefault: true` to `createJiti(...)`, but in jiti 2.x the constructor option only applies to the deprecated synchronous `jiti(id)` callable form — the async `jiti.import()` ignores it and always returns the full module namespace. With `export default defineConfig({...})` that meant Zod was validating `{ default: { databaseUrl, client } }` and emitting databaseUrl: Invalid input: expected nonoptional, received undefined even though the user's config plainly set the field. The jiti 2.x async API exposes a per-call `{ default: true }` option that does work. Switch to it and drop the now-misleading constructor option from both `loadStashConfig` and `loadEncryptConfig`. `loadEncryptConfig` wasn't symptom-bugged (it iterates `Object.values` to find the EncryptionClient, which flattens both shapes equally) but keeping the two call sites consistent prevents the next reader from reasoning their way to the same wrong conclusion. Adds `config-jiti-integration.test.ts` — drives `loadStashConfig` against real jiti and a real temp `stash.config.ts`. The existing `config.test.ts` mocks `jiti.import` past the bug and so couldn't catch wrap/unwrap regressions on its own.
68d82f5 to
e3fe3cd
Compare
Closes #374.
Symptom
Even when the user's
stash.config.tsclearly setsdatabaseUrlfromprocess.env.DATABASE_URL, and aconsole.logof the same env var prints the URL just before the error fires.Root cause
loadStashConfig(packages/cli/src/config/index.ts) was constructing jiti withinteropDefault: trueand then callingjiti.import(...):In jiti 2.x, the constructor's
interopDefaultonly applies to the deprecated synchronousjiti(id)callable form. The asyncjiti.import()ignores it and always returns the module namespace. So forexport default defineConfig({...}),rawConfigis{ default: { databaseUrl, client } }. Zod then validates the wrapper, finds no top-leveldatabaseUrl, and emits the misleading "expected nonoptional, received undefined" error.Verified empirically against
jiti@2.6.1:Fix
Switch to the per-call
{ default: true }option (the jiti 2.x async-API way to ask for default-export unwrapping). Drop the now-misleadinginteropDefault: truefrom bothloadStashConfigand the symmetricloadEncryptConfigcall site (which wasn't symptom-bugged because it iteratesObject.valuesto find the EncryptionClient — but consistency keeps the next reader from reaching the same wrong conclusion).Why the existing test missed it
packages/cli/src/__tests__/config.test.tsmocksjiti.importto return the already-unwrapped shape:So the test never exercised the real jiti behavior and would pass regardless of which option was set.
Regression test
New
src/__tests__/config-jiti-integration.test.tsdrivesloadStashConfigagainst real jiti and a realstash.config.tsfile written into a temp dir. Two cases:export default {...}is unwrapped to the inner config (the regression we're fixing).databaseUrlproduces a useful error message containingInvalid stash.config.tsanddatabaseUrl.The mocked
config.test.tsstays in place for fast schema iteration.Test plan
pnpm --filter @cipherstash/cli buildcleanpnpm --filter @cipherstash/cli test— 78 pass (was 76; +2 from the new integration test)biome checkclean on changed filesnpx @cipherstash/cli db installagainst a config that readsdatabaseUrlfromprocess.env.DATABASE_URL) succeeds with this branch