From 2cb8ff4e46d6eaca264cb09d9ca45359c1c3423f Mon Sep 17 00:00:00 2001 From: Yury Zialionka Date: Thu, 13 Aug 2026 13:49:08 -0600 Subject: [PATCH] test(storage): make the storage root injectable and fail closed on disk databases Closes #12997. Part of #12986. - Storage.layerFromDir(dir) roots the service at an explicit directory; the default layer still resolves Global.Path.data/storage lazily, so production composition (app-runtime) is unchanged. - storage.test.ts injects a tmp root directly instead of remapping six FSUtil methods under Global.Path.data - the remap hack and its keep-in-sync burden are gone. - Both test preloads now assert the resolved database path is ":memory:" after all preload imports. Previously a dropped env write or import-order regression silently pointed unit tests at the real ~/.local/share/kilo database (locking errors, Windows EBUSY teardown crashes); now the run fails immediately with a clear message. --- packages/core/test/preload.ts | 10 + packages/opencode/src/storage/storage.ts | 211 ++++++++++-------- packages/opencode/test/preload.ts | 17 ++ .../opencode/test/storage/storage.test.ts | 52 ++--- 4 files changed, 153 insertions(+), 137 deletions(-) diff --git a/packages/core/test/preload.ts b/packages/core/test/preload.ts index d44cffd849e..8bca015c253 100644 --- a/packages/core/test/preload.ts +++ b/packages/core/test/preload.ts @@ -3,3 +3,13 @@ import path from "path" process.env.KILO_DB = ":memory:" process.env.KILO_MODELS_PATH = path.join(import.meta.dir, "plugin", "fixtures", "models-dev.json") process.env.KILO_DISABLE_MODELS_FETCH = "true" + +// kilocode_change start - fail closed: core unit tests do not redirect XDG dirs, so KILO_DB +// is the only thing keeping them off the real ~/.local/share/kilo database. Verify the +// resolved path (env is read at flag import time, so this must stay after the env writes). +const { Database } = await import("../src/database/database") +const resolved = Database.path() +if (resolved !== ":memory:") { + throw new Error(`unit test preload: database path must resolve to ":memory:", got "${resolved}"`) +} +// kilocode_change end diff --git a/packages/opencode/src/storage/storage.ts b/packages/opencode/src/storage/storage.ts index ddbd83d7931..7dd68faef97 100644 --- a/packages/opencode/src/storage/storage.ts +++ b/packages/opencode/src/storage/storage.ts @@ -210,117 +210,130 @@ const MIGRATIONS: Migration[] = [ }), ] -const layer = Layer.effect( - Service, - Effect.gen(function* () { - const fs = yield* FSUtil.Service - const git = yield* Git.Service - const locks = yield* RcMap.make({ - lookup: () => TxReentrantLock.make(), - idleTimeToLive: 0, - }) - const state = yield* Effect.cached( - Effect.gen(function* () { - const dir = path.join(Global.Path.data, "storage") - const marker = path.join(dir, "migration") - const migration = yield* fs.readFileString(marker).pipe( - Effect.map(parseMigration), - Effect.catchIf(missing, () => Effect.succeed(0)), - Effect.orElseSucceed(() => 0), - ) - for (let i = migration; i < MIGRATIONS.length; i++) { - yield* Effect.logInfo("running migration", { index: i }) - const step = MIGRATIONS[i]! - const exit = yield* Effect.exit(step(dir, fs, git)) - if (Exit.isFailure(exit)) { - yield* Effect.logError("failed to run migration", { index: i, cause: exit.cause }) - break - } - yield* fs.writeWithDirs(marker, String(i + 1)) - } - return { dir } - }), - ) - - const fail = (target: string): Effect.Effect => - Effect.fail(new NotFoundError({ message: `Resource not found: ${target}` })) - - const wrap = (target: string, body: Effect.Effect) => - body.pipe(Effect.catchIf(missing, () => fail(target))) - - const writeJson = Effect.fnUntraced(function* (target: string, content: unknown) { - yield* fs.writeWithDirs(target, JSON.stringify(content, null, 2)) - }) - - const withResolved = ( - key: string[], - fn: (target: string, rw: TxReentrantLock.TxReentrantLock) => Effect.Effect, - ): Effect.Effect => - Effect.scoped( +// kilocode_change start - the storage root is injectable so tests can point Storage at a +// temporary directory instead of remapping FSUtil paths under Global.Path.data. The +// default (production) root is unchanged and resolved lazily on first use. Note for +// callers of layerFromDir: migration 1 walks `../project` relative to this directory, +// so inject `/storage` when legacy layouts must be reachable from the parent. +const make = (root?: string) => + Layer.effect( + Service, + Effect.gen(function* () { + const fs = yield* FSUtil.Service + const git = yield* Git.Service + const locks = yield* RcMap.make({ + lookup: () => TxReentrantLock.make(), + idleTimeToLive: 0, + }) + const state = yield* Effect.cached( Effect.gen(function* () { - const target = file((yield* state).dir, key) - return yield* fn(target, yield* RcMap.get(locks, target)) + const dir = root ?? path.join(Global.Path.data, "storage") + const marker = path.join(dir, "migration") + const migration = yield* fs.readFileString(marker).pipe( + Effect.map(parseMigration), + Effect.catchIf(missing, () => Effect.succeed(0)), + Effect.orElseSucceed(() => 0), + ) + for (let i = migration; i < MIGRATIONS.length; i++) { + yield* Effect.logInfo("running migration", { index: i }) + const step = MIGRATIONS[i]! + const exit = yield* Effect.exit(step(dir, fs, git)) + if (Exit.isFailure(exit)) { + yield* Effect.logError("failed to run migration", { index: i, cause: exit.cause }) + break + } + yield* fs.writeWithDirs(marker, String(i + 1)) + } + return { dir } }), ) - const remove: Interface["remove"] = Effect.fn("Storage.remove")(function* (key: string[]) { - yield* withResolved(key, (target, rw) => - TxReentrantLock.withWriteLock(rw, fs.remove(target).pipe(Effect.catchIf(missing, () => Effect.void))), - ) - }) + const fail = (target: string): Effect.Effect => + Effect.fail(new NotFoundError({ message: `Resource not found: ${target}` })) - const read: Interface["read"] = (key: string[]) => - Effect.gen(function* () { - const value = yield* withResolved(key, (target, rw) => - TxReentrantLock.withReadLock(rw, wrap(target, fs.readJson(target))), + const wrap = (target: string, body: Effect.Effect) => + body.pipe(Effect.catchIf(missing, () => fail(target))) + + const writeJson = Effect.fnUntraced(function* (target: string, content: unknown) { + yield* fs.writeWithDirs(target, JSON.stringify(content, null, 2)) + }) + + const withResolved = ( + key: string[], + fn: (target: string, rw: TxReentrantLock.TxReentrantLock) => Effect.Effect, + ): Effect.Effect => + Effect.scoped( + Effect.gen(function* () { + const target = file((yield* state).dir, key) + return yield* fn(target, yield* RcMap.get(locks, target)) + }), ) - return value as T - }) - const update: Interface["update"] = (key: string[], fn: (draft: T) => void) => - Effect.gen(function* () { - const value = yield* withResolved(key, (target, rw) => - TxReentrantLock.withWriteLock( - rw, - Effect.gen(function* () { - const content = yield* wrap(target, fs.readJson(target)) - fn(content as T) - yield* writeJson(target, content) - return content - }), - ), + const remove: Interface["remove"] = Effect.fn("Storage.remove")(function* (key: string[]) { + yield* withResolved(key, (target, rw) => + TxReentrantLock.withWriteLock(rw, fs.remove(target).pipe(Effect.catchIf(missing, () => Effect.void))), ) - return value as T }) - const write: Interface["write"] = (key: string[], content: unknown) => - Effect.gen(function* () { - yield* withResolved(key, (target, rw) => TxReentrantLock.withWriteLock(rw, writeJson(target, content))) - }) - - const list: Interface["list"] = Effect.fn("Storage.list")(function* (prefix: string[]) { - const dir = (yield* state).dir - const cwd = path.join(dir, ...prefix) - const result = yield* fs - .glob("**/*", { - cwd, - include: "file", + const read: Interface["read"] = (key: string[]) => + Effect.gen(function* () { + const value = yield* withResolved(key, (target, rw) => + TxReentrantLock.withReadLock(rw, wrap(target, fs.readJson(target))), + ) + return value as T }) - .pipe(Effect.catch(() => Effect.succeed([]))) - return result - .map((x) => [...prefix, ...x.slice(0, -5).split(path.sep)]) - .toSorted((a, b) => a.join("/").localeCompare(b.join("/"))) - }) - return Service.of({ - remove, - read, - update, - write, - list, - }) - }), -) + const update: Interface["update"] = (key: string[], fn: (draft: T) => void) => + Effect.gen(function* () { + const value = yield* withResolved(key, (target, rw) => + TxReentrantLock.withWriteLock( + rw, + Effect.gen(function* () { + const content = yield* wrap(target, fs.readJson(target)) + fn(content as T) + yield* writeJson(target, content) + return content + }), + ), + ) + return value as T + }) + + const write: Interface["write"] = (key: string[], content: unknown) => + Effect.gen(function* () { + yield* withResolved(key, (target, rw) => TxReentrantLock.withWriteLock(rw, writeJson(target, content))) + }) + + const list: Interface["list"] = Effect.fn("Storage.list")(function* (prefix: string[]) { + const dir = (yield* state).dir + const cwd = path.join(dir, ...prefix) + const result = yield* fs + .glob("**/*", { + cwd, + include: "file", + }) + .pipe(Effect.catch(() => Effect.succeed([]))) + return result + .map((x) => [...prefix, ...x.slice(0, -5).split(path.sep)]) + .toSorted((a, b) => a.join("/").localeCompare(b.join("/"))) + }) + + return Service.of({ + remove, + read, + update, + write, + list, + }) + }), + ) + +const layer = make() + +// kilocode_change start +/** Storage rooted at an explicit directory — for tests and multi-instance isolation. */ +export const layerFromDir = (dir: string) => make(dir) +// kilocode_change end export const node = LayerNode.make({ service: Service, layer: layer, deps: [FSUtil.node, Git.node] }) diff --git a/packages/opencode/test/preload.ts b/packages/opencode/test/preload.ts index c89dd27ca6c..6b512d94053 100644 --- a/packages/opencode/test/preload.ts +++ b/packages/opencode/test/preload.ts @@ -96,3 +96,20 @@ const { installMemoryRuntime } = await import("../src/kilocode/memory/runtime") initProjectors() installMemoryRuntime() // kilocode_change + +// kilocode_change start - fail closed: unit tests must never open a disk database. Both DB +// path resolvers (core Database.path and the v1 client in src/storage/db.ts) honor KILO_DB +// verbatim when it is ":memory:", so asserting the resolved core path after all preload +// imports catches env mutations, import-order regressions, and channel/absolute fallbacks +// that would silently point tests at ~/.local/share/kilo/kilo.db. +if (process.env["KILO_DB"] !== ":memory:") { + throw new Error(`unit test preload: KILO_DB must be ":memory:", got "${process.env["KILO_DB"]}"`) +} +{ + const { Database } = await import("@opencode-ai/core/database/database") + const resolved = Database.path() + if (resolved !== ":memory:") { + throw new Error(`unit test preload: database path must resolve to ":memory:", got "${resolved}"`) + } +} +// kilocode_change end diff --git a/packages/opencode/test/storage/storage.test.ts b/packages/opencode/test/storage/storage.test.ts index a3f3aef4464..282b617cd68 100644 --- a/packages/opencode/test/storage/storage.test.ts +++ b/packages/opencode/test/storage/storage.test.ts @@ -24,41 +24,17 @@ const scope = Effect.fnUntraced(function* () { return { root, svc } }) -// remap(root) rewrites any path under Global.Path.data to live under `root` instead. -// Used by remappedFs to build an FSUtil that Storage thinks is the real global -// data dir but actually targets a tmp dir — letting migration tests stage legacy layouts. -// NOTE: only the 6 methods below are intercepted. If Storage starts using a different -// FSUtil method that touches Global.Path.data, add it here. -function remap(root: string, file: string) { - if (file === Global.Path.data) return root - if (file.startsWith(Global.Path.data + path.sep)) return path.join(root, path.relative(Global.Path.data, file)) - return file -} - -function remappedFs(root: string) { - return Layer.effect( - FSUtil.Service, - Effect.gen(function* () { - const fs = yield* FSUtil.Service - return FSUtil.Service.of({ - ...fs, - isDir: (file) => fs.isDir(remap(root, file)), - readJson: (file) => fs.readJson(remap(root, file)), - writeWithDirs: (file, content, mode) => fs.writeWithDirs(remap(root, file), content, mode), - readFileString: (file) => fs.readFileString(remap(root, file)), - remove: (file) => fs.remove(remap(root, file)), - glob: (pattern, options) => - fs.glob(pattern, options?.cwd ? { ...options, cwd: remap(root, options.cwd) } : options), - }) - }), - ).pipe(Layer.provide(LayerNode.compile(FSUtil.node))) -} - -// Layer.fresh forces a new Storage instance — without it, Effect's in-test layer cache -// returns the outer testEffect's Storage (which uses the real FSUtil), not a new -// one built on top of remappedFs. -const remappedStorage = (root: string) => - Layer.fresh(LayerNode.compile(Storage.node, [[FSUtil.node, remappedFs(root)]])) +// Storage rooted at `/storage` via the injectable layer — migration tests stage +// legacy layouts (e.g. `/project`) as siblings, matching migration 1's `../project` +// walk. Layer.fresh forces a new Storage instance — without it, Effect's in-test layer +// cache returns the outer testEffect's Storage (rooted at the real data dir), not a new +// one rooted at the tmp dir. +const injectedStorage = (root: string) => + Layer.fresh( + Storage.layerFromDir(path.join(root, "storage")).pipe( + Layer.provide(LayerNode.compile(LayerNode.group([FSUtil.node, Git.node]))), + ), + ) describe("Storage", () => { it.live("round-trips JSON content", () => @@ -228,7 +204,7 @@ describe("Storage", () => { title: "legacy", summary: { additions: 5, deletions: 5 }, }) - }).pipe(Effect.provide(remappedStorage(tmp))) + }).pipe(Effect.provide(injectedStorage(tmp))) expect(yield* fs.readFileString(path.join(storage, "migration"))).toBe("2") }), @@ -270,7 +246,7 @@ describe("Storage", () => { role: "user", text: "hello", }) - }).pipe(Effect.provide(remappedStorage(tmp))) + }).pipe(Effect.provide(injectedStorage(tmp))) expect(yield* fs.readFileString(path.join(storage, "migration"))).toBe("2") }), @@ -288,7 +264,7 @@ describe("Storage", () => { yield* Effect.gen(function* () { const svc = yield* Storage.Service expect(yield* svc.list(["project"])).toEqual([]) - }).pipe(Effect.provide(remappedStorage(tmp))) + }).pipe(Effect.provide(injectedStorage(tmp))) const exit = yield* fs.access(path.join(storage, "migration")).pipe(Effect.exit) expect(Exit.isFailure(exit)).toBe(true)