Repository navigation
fix(storage): route every getDb call through tenant resolution - #84
Conversation
Phase 2 converted the 18 ensureDb() helpers but missed 53 call sites that build a path inline — combat-handlers.ts alone had 30, plus tools.ts and spatial-handlers.ts. Those still called getDb(path), which now throws outside tests, so combat and several world/spatial tool paths were broken in production the moment Phase 2 deployed. The test suite could not catch this: under NODE_ENV=test the path argument is the supported escape hatch, so every one of those call sites resolved happily while production threw. A structural test now asserts that no module outside storage/index.ts passes an argument to getDb, which is what found these. Also adds the Phase 3 boundary guards: - assertNoLegacyDatabase() refuses to start when a pre-split rpg-mcp.db or rpg.db is still present. A leftover file means an incomplete cutover or a rollback-forward, and starting anyway would serve empty campaigns while the real data sat in a file nothing reads. - listCampaignDatabases() and deleteCampaignDatabase() make erasure a file operation rather than a cascading multi-table delete, so there is no way to miss a table and leave a fragment of one customer game behind. Deletion drops the pooled handle first and removes the -wal/-shm sidecars, since a surviving -wal would let a later open recover deleted rows. Six test files relied on handlers implicitly creating the singleton database. They now install one explicitly via a shared helper, scoped with beforeAll to match the lifetime they were already depending on. One of them carried a vi.mock for src/server/db.js — a path that does not exist, so it had never intercepted anything.
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b913db4db4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const root = process.env.RPG_DATA_DIR || getAppDataDir(); | ||
| const found = LEGACY_DATABASE_NAMES | ||
| .map(name => join(root, name)) | ||
| .filter(path => existsSync(path)); |
There was a problem hiding this comment.
When an exi
AC60
sting deployment sets RPG_MCP_DB_PATH or uses --db-path, the legacy database can be anywhere and need not be named rpg.db or rpg-mcp.db; however, this guard only checks those two names under RPG_DATA_DIR or the app-data directory. Such a deployment therefore starts successfully, ignores its populated configured database, and creates empty per-campaign files, producing exactly the silent apparent data loss this assertion is intended to prevent. Include the configured/resolved legacy path in the startup check.
Useful? React with 👍 / 👎.
…os#84) Phase 2 converted the 18 ensureDb() helpers but missed 53 call sites that build a path inline — combat-handlers.ts alone had 30, plus tools.ts and spatial-handlers.ts. Those still called getDb(path), which now throws outside tests, so combat and several world/spatial tool paths were broken in production the moment Phase 2 deployed. The test suite could not catch this: under NODE_ENV=test the path argument is the supported escape hatch, so every one of those call sites resolved happily while production threw. A structural test now asserts that no module outside storage/index.ts passes an argument to getDb, which is what found these. Also adds the Phase 3 boundary guards: - assertNoLegacyDatabase() refuses to start when a pre-split rpg-mcp.db or rpg.db is still present. A leftover file means an incomplete cutover or a rollback-forward, and starting anyway would serve empty campaigns while the real data sat in a file nothing reads. - listCampaignDatabases() and deleteCampaignDatabase() make erasure a file operation rather than a cascading multi-table delete, so there is no way to miss a table and leave a fragment of one customer game behind. Deletion drops the pooled handle first and removes the -wal/-shm sidecars, since a surviving -wal would let a later open recover deleted rows. Six test files relied on handlers implicitly creating the singleton database. They now install one explicitly via a shared helper, scoped with beforeAll to match the lifetime they were already depending on. One of them carried a vi.mock for src/server/db.js — a path that does not exist, so it had never intercepted anything.
Phase 3 of the multi-user plan — plus a production hotfix that Phase 3's own structural test uncovered.
The hotfix (why this should land promptly)
Phase 2 (#83) converted the 18
ensureDb()helpers but missed 53 call sites that build a path inline.combat-handlers.tsalone had 30, plustools.tsandspatial-handlers.ts.Those still called
getDb(path), which now throws outside tests. So combat and several world/spatial tool paths have been broken in production since Phase 2 deployed.The suite could not have caught this. Under
NODE_ENV=testthe path argument is the supported escape hatch, so all 53 sites resolved happily in tests while production threw. That asymmetry is exactly why the structural guard below exists — it found them.Phase 3: boundary guards
Structural test — asserts no module outside
storage/index.tspasses an argument togetDb. This is the rule a future contributor is most likely to break by habit, and breaking it silently reattaches a caller to a database chosen without a verified tenant.assertNoLegacyDatabase()— refuses to start when a pre-splitrpg-mcp.dborrpg.dbis still present inRPG_DATA_DIR. A leftover file means an incomplete cutover or a rollback-forward; starting anyway would serve empty campaigns while real data sat in a file nothing reads. Both historical filenames are checked, sinceRPG_MCP_DB_PATHpointed atrpg-mcp.dbwhile the tool helpers builtrpg.db.listCampaignDatabases()/deleteCampaignDatabase()— erasure becomes a file operation rather than a cascading multi-table delete, so there is no way to miss a table and leave a fragment of one customer's game behind. Deletion drops the pooled handle first and removes the-wal/-shmsidecars — a surviving-walwould let a later open recover rows from the database just deleted.Test-suite changes
Six files relied on handlers implicitly creating the singleton database. They now install one explicitly through a shared helper, scoped with
beforeAllto match the lifetime they were already depending on (these suites build state in one test and read it in the next).One of them carried a
vi.mockforsrc/server/db.js— a path that does not exist, so it had never intercepted anything. Those tests were really running against the implicitly-created database; that is now explicit rather than accidental.Risk
assertNoLegacyDatabase()will refuse startup if a legacy file is present. Both environments have been cleared and verified (/app/datanow holds onlycampaigns/andlost+found/), so this is safe to deploy. It is a deliberate fail-closed: a silent wrong-database start is far worse to diagnose.deleteCampaignDatabase()is not wired to any tool or route in this PR — it is a primitive with tests, not yet an exposed capability. Nothing can invoke it remotely.Acceptance criteria
getDb.Verification
npm test— 2,357 passed / 7 skipped, no failures.npx tsc --noEmit— exit 0.npm run build— clean.14 new tests in
tests/storage/tenant-boundary.test.ts.🤖 Generated with Claude Code