fix(storage): honour MAGIC_CONTEXT_TEST_DATA_DIR in the shared storage resolver - #296
Conversation
…e resolver test-preload.ts documents MAGIC_CONTEXT_TEST_DATA_DIR as the guard that "cannot be defeated", but only resolveDatabasePath() consulted it. Every other caller of getMagicContextStorageDir() bypassed it — including the CLI doctors, which build join(getMagicContextStorageDir(), "context.db") themselves and run PRAGMA integrity_check against it. A test cannot restore isolation by setting process.env.HOME: bun caches os.homedir() at startup, so getDataDir() still resolves to the real home. Any test that deletes XDG_DATA_HOME to exercise path fallbacks therefore reached the user's production database. Observed as two doctor tests taking 8s and 16.5s against a 1.6 GB live DB — a read, not a migration, but the same escape that migrated the live DB in the v26 and v41 incidents. Move the guard into getMagicContextStorageDir() so it covers every caller. XDG_DATA_HOME still wins, so tests that manage their own data home are unaffected. resolveDatabasePath() drops the now-redundant branch and keeps the NODE_ENV backstop, which needs a memoized throwaway dir that a pure path helper has no business creating; its condition now defers to the test data dir so precedence is unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/storage-db.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/storage-db.ts:187">
P2: The rewritten comment/stated goal claim uniform coverage "for every other caller alike" and "structurally impossible for ANY test to read or migrate production data", but the NODE_ENV=test redirect (getTestBackstopDbDir) stays only inside resolveDatabasePath. In the documented no-preload window (NODE_ENV=test, MAGIC_CONTEXT_TEST_DATA_DIR unset, XDG_DATA_HOME unset) callers that use getMagicContextStorageDir() directly — CLI doctors integrity_check (doctor.ts:64, doctor-pi.ts:575, diagnostics-pi.ts:454) and announcement.ts/models-dev-cache.ts — still resolve to the real ~/.local/share and can touch/migrate the production DB, i.e. the v26/v41 incident class this PR says it closes. If MAGIC/CWD-independent protection is intended to be truly uniform, the backstop needs to apply to getMagicContextStorageDir() rather than only the DB-resolver path; otherwise the claim in the comment is overstated for those callers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // helper calls), getMagicContextStorageDir() already points inside that | ||
| // controlled dir — honor it, do not override. | ||
| if (process.env.NODE_ENV === "test" && !process.env.XDG_DATA_HOME) { | ||
| if ( |
There was a problem hiding this comment.
P2: The rewritten comment/stated goal claim uniform coverage "for every other caller alike" and "structurally impossible for ANY test to read or migrate production data", but the NODE_ENV=test redirect (getTestBackstopDbDir) stays only inside resolveDatabasePath. In the documented no-preload window (NODE_ENV=test, MAGIC_CONTEXT_TEST_DATA_DIR unset, XDG_DATA_HOME unset) callers that use getMagicContextStorageDir() directly — CLI doctors integrity_check (doctor.ts:64, doctor-pi.ts:575, diagnostics-pi.ts:454) and announcement.ts/models-dev-cache.ts — still resolve to the real ~/.local/share and can touch/migrate the production DB, i.e. the v26/v41 incident class this PR says it closes. If MAGIC/CWD-independent protection is intended to be truly uniform, the backstop needs to apply to getMagicContextStorageDir() rather than only the DB-resolver path; otherwise the claim in the comment is overstated for those callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/storage-db.ts, line 187:
<comment>The rewritten comment/stated goal claim uniform coverage "for every other caller alike" and "structurally impossible for ANY test to read or migrate production data", but the NODE_ENV=test redirect (getTestBackstopDbDir) stays only inside resolveDatabasePath. In the documented no-preload window (NODE_ENV=test, MAGIC_CONTEXT_TEST_DATA_DIR unset, XDG_DATA_HOME unset) callers that use getMagicContextStorageDir() directly — CLI doctors integrity_check (doctor.ts:64, doctor-pi.ts:575, diagnostics-pi.ts:454) and announcement.ts/models-dev-cache.ts — still resolve to the real ~/.local/share and can touch/migrate the production DB, i.e. the v26/v41 incident class this PR says it closes. If MAGIC/CWD-independent protection is intended to be truly uniform, the backstop needs to apply to getMagicContextStorageDir() rather than only the DB-resolver path; otherwise the claim in the comment is overstated for those callers.</comment>
<file context>
@@ -198,7 +184,11 @@ export function resolveDatabasePath(dbPathOverride?: string): { dbDir: string; d
// helper calls), getMagicContextStorageDir() already points inside that
// controlled dir — honor it, do not override.
- if (process.env.NODE_ENV === "test" && !process.env.XDG_DATA_HOME) {
+ if (
+ process.env.NODE_ENV === "test" &&
+ !process.env.XDG_DATA_HOME &&
</file context>
There was a problem hiding this comment.
Valid, and fixed in 9cfcad5 by hoisting the backstop rather than softening the claim.
Reproduced first, with NODE_ENV=test and neither guard var set:
getMagicContextStorageDir() -> /home/…/.local/share/cortexkit/magic-context # production
resolveDatabasePath() -> /tmp/mc-test-db-backstop-XJ6lvp/…/context.db # protected
So the callers you named were reaching production while the DB resolver was safely redirected — the exact class this PR claims to close.
getTestBackstopDbDir() moved into data-path.ts and getMagicContextStorageDir() now applies both guards; resolveDatabasePath() has no isolation branch left. Path shape is unchanged (the backstop dir already ended in cortexkit/magic-context). The one-time warning uses console.warn because logger.ts imports data-path.ts.
After: both resolve to the same temp root, memoized across calls, and production (no NODE_ENV) still resolves to the real path. New test pins it; plugin suite 3622 pass, CLI 296 pass / 2 skip.
Review feedback on cortexkit#296 (cubic, storage-db.ts:187): the PR claimed uniform coverage, but only MAGIC_CONTEXT_TEST_DATA_DIR moved into getMagicContextStorageDir(). The CWD-independent NODE_ENV backstop stayed in resolveDatabasePath(), so in the no-preload window the claim was false for exactly the callers this PR was about. Reproduced with NODE_ENV=test and neither guard var set: getMagicContextStorageDir() -> ~/.local/share/cortexkit/magic-context resolveDatabasePath() -> /tmp/mc-test-db-backstop-*/…/context.db The CLI doctors build join(getMagicContextStorageDir(), "context.db") themselves and run PRAGMA integrity_check on it, so they took the first path — production — while the DB resolver was safely redirected. Same for announcement.ts and models-dev-cache.ts. Move the backstop next to the guard it backs up. resolveDatabasePath() now has no isolation branch at all; both guards live in one place and every caller inherits them. Path shape is unchanged: the backstop dir already ended in cortexkit/magic-context, so the resolver still appends context.db to the same root. The warning uses console.warn rather than the logger because logger.ts imports data-path.ts. Also correct the doc comment's claim that no test mutates the guard var (cubic, data-path.ts:180) — data-path.test.ts sets and restores it.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The suite's afterEach restored a saved value only when it was defined, so a var that started unset stayed set after the test that set it. Two tests lift a guard to assert production shape (the layout test deletes MAGIC_CONTEXT_TEST_DATA_DIR and NODE_ENV; the backstop test sets NODE_ENV), which made the result order-dependent in any run where those vars are not already populated — and NODE_ENV was not saved at all. Replace the hand-rolled restores with a loop over the saved snapshot that deletes when the original was undefined, and add NODE_ENV to it. Verified with --rerun-each 5 (125 pass) and against the full plugin suite.
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/shared/data-path.ts">
<violation number="1" location="packages/plugin/src/shared/data-path.ts:202">
P1: Tests that unset `XDG_DATA_HOME` still open and checkpoint the user's legacy OpenCode DB during `openDatabase()` migration; apply the same test isolation to the legacy source or skip legacy migration under this guard.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!process.env.XDG_DATA_HOME) { | ||
| const testDataDir = process.env.MAGIC_CONTEXT_TEST_DATA_DIR; | ||
| if (testDataDir) { | ||
| return path.join(testDataDir, "cortexkit", "magic-context"); |
There was a problem hiding this comment.
P1: Tests that unset XDG_DATA_HOME still open and checkpoint the user's legacy OpenCode DB during openDatabase() migration; apply the same test isolation to the legacy source or skip legacy migration under this guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/shared/data-path.ts, line 202:
<comment>Tests that unset `XDG_DATA_HOME` still open and checkpoint the user's legacy OpenCode DB during `openDatabase()` migration; apply the same test isolation to the legacy source or skip legacy migration under this guard.</comment>
<file context>
@@ -167,11 +167,76 @@ export function getOpenCodeStorageDir(): string {
+ if (!process.env.XDG_DATA_HOME) {
+ const testDataDir = process.env.MAGIC_CONTEXT_TEST_DATA_DIR;
+ if (testDataDir) {
+ return path.join(testDataDir, "cortexkit", "magic-context");
+ }
+ if (process.env.NODE_ENV === "test") {
</file context>
2 participants
Problem
packages/plugin/test-preload.tsdocumentsMAGIC_CONTEXT_TEST_DATA_DIRas the guard that "cannot be defeated", but onlyresolveDatabasePath()consulted it. Every other caller ofgetMagicContextStorageDir()bypassed it — including the CLI doctors, which buildjoin(getMagicContextStorageDir(), "context.db")themselves and runPRAGMA integrity_checkagainst the result.A test cannot restore isolation by setting
process.env.HOME: bun cachesos.homedir()at process startup, sogetDataDir()still resolves to the real home.Any test that deletes
XDG_DATA_HOMEto exercise path fallbacks therefore reaches the user's production database. Observed as two CLI doctor tests taking 8s and 16.5s against a 1.6 GB live DB — a read, not a migration, but the same escape route as the v26 and v41 incidents.Change
Move the guard into
getMagicContextStorageDir()so every caller inherits it:XDG_DATA_HOMEstill wins, so suites that manage their own data home are unaffected.resolveDatabasePath()drops the now-redundant branch and keeps theNODE_ENVbackstop — that one needs a memoized throwaway dir, which a pure path helper has no business creating. The backstop's condition now defers to the test data dir, so precedence is exactly as before; otherwise a test that deleted onlyXDG_DATA_HOMEwould have been diverted from the preload dir into a fresh backstop dir.3 files, +66/−24.
Verification
To prove the hole is closed rather than merely unused, the pre-fix
doctor-omp.test.ts(withdelete process.env.XDG_DATA_HOMErestored) was regenerated into a scratch file and run against the patched resolver: 101ms, 5 pass, versus 34.65s and 2 failures before. The dangerous pattern is now inert.Three new tests in
data-path.test.tspin the precedence: production layout (test dir lifted), guard honoured whenXDG_DATA_HOMEis unset, andXDG_DATA_HOMEwinning over the guard. The pre-existing layout test needed the test dir temporarily lifted — it had only been passing because the helper ignored the preload entirely.Against a clean
master:it.skipplaceholders) / 0 failcortexkitpath, confirming the guard stays inert when neitherNODE_ENV=testnorMAGIC_CONTEXT_TEST_DATA_DIRis setgit diff --checkcleanThe Rust e2e lane was not run: it needs a sibling
subconsciouscheckout to buildck-subc, which CI also does not provision.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes test isolation by honoring
MAGIC_CONTEXT_TEST_DATA_DIRand moving theNODE_ENV=testbackstop intogetMagicContextStorageDir()so tests, CLI doctors, and other direct callers never touch the real DB whenXDG_DATA_HOMEis unset or preload is missing. Keeps precedence and simplifies path resolution.NODE_ENV=testbackstop intogetMagicContextStorageDir()alongsideMAGIC_CONTEXT_TEST_DATA_DIR; covers CLI doctors, announcements, and models-dev cache.XDG_DATA_HOME>MAGIC_CONTEXT_TEST_DATA_DIR>NODE_ENV=testbackstop > default.resolveDatabasePath(); rely on the shared resolver.NODE_ENV) to prevent leaks; doctor test drops from ~34s to ~100ms with no production DB access.Written for commit 86a7d56. Summary will update on new commits.
Greptile Summary
The PR centralizes test-storage isolation in the shared storage resolver so direct callers and database-opening paths use consistent precedence.
XDG_DATA_HOME,MAGIC_CONTEXT_TEST_DATA_DIR,NODE_ENV=testbackstop, then the platform default.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[getMagicContextStorageDir] --> B{XDG_DATA_HOME set?} B -- Yes --> C[XDG data directory] B -- No --> D{MAGIC_CONTEXT_TEST_DATA_DIR set?} D -- Yes --> E[Test data directory] D -- No --> F{NODE_ENV equals test?} F -- Yes --> G[Memoized temporary directory] F -- No --> H[Platform default data directory] C --> I[cortexkit/magic-context] E --> I G --> I H --> IReviews (3): Last reviewed commit: "test(data-path): restore-or-delete every..." | Re-trigger Greptile