diff --git a/.changeset/safe-credential-reconciliation.md b/.changeset/safe-credential-reconciliation.md new file mode 100644 index 00000000000..abb8dd5f36a --- /dev/null +++ b/.changeset/safe-credential-reconciliation.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Prevent concurrent Kilo startups from rewriting unchanged credentials, retry transient database locks, and redact bound values from database errors. diff --git a/packages/core/src/credential.ts b/packages/core/src/credential.ts index bed9bb25314..104b9897eb4 100644 --- a/packages/core/src/credential.ts +++ b/packages/core/src/credential.ts @@ -15,6 +15,7 @@ import { Global } from "./global" import { DataMigrationTable } from "./data-migration.sql" import path from "path" import { parse as parseKiloAccounts } from "./kilocode/credential-migration" +import { isBusy } from "./kilocode/sqlite-error" import { NonNegativeInt } from "./schema" // kilocode_change end @@ -170,6 +171,17 @@ export const legacyImportLayer = Layer.effectDiscard( const integration = Integration.ID.make(integrationID.replace(/\/+$/, "")) return [{ integration, value: legacyValue(integration, decoded.value) }] }) + const migrated = yield* db.select().from(DataMigrationTable).where(eq(DataMigrationTable.name, name)).get() + const existing = yield* db.select().from(CredentialTable).orderBy(desc(CredentialTable.time_created)).all() + const same = (left: Value, right: Value) => JSON.stringify(left) === JSON.stringify(right) + if ( + migrated && + values.every((item) => { + const current = existing.find((row) => row.integration_id === item.integration) + return current !== undefined && same(current.value, item.value) + }) + ) + return yield* db.transaction((tx) => Effect.gen(function* () { for (const item of values) { @@ -181,7 +193,12 @@ export const legacyImportLayer = Layer.effectDiscard( .orderBy(desc(CredentialTable.time_created)) // kilocode_change - reconcile the active imported account .get() if (current) { - yield* tx.update(CredentialTable).set({ value: item.value }).where(eq(CredentialTable.id, current.id)).run() + if (!same(current.value, item.value)) + yield* tx + .update(CredentialTable) + .set({ value: item.value }) + .where(eq(CredentialTable.id, current.id)) + .run() continue } yield* tx.insert(CredentialTable).values({ @@ -194,7 +211,15 @@ export const legacyImportLayer = Layer.effectDiscard( yield* tx.insert(DataMigrationTable).values({ name, time_completed: Date.now() }).onConflictDoNothing().run() }), ) - }).pipe(Effect.orDie), + }).pipe( + Effect.retry({ while: isBusy, times: 2 }), + Effect.catch((error) => + isBusy(error) + ? Effect.logWarning("legacy credential reconciliation deferred because the database is busy") + : Effect.fail(error), + ), + Effect.orDie, + ), ) // kilocode_change end diff --git a/packages/core/src/kilocode/database-compat.ts b/packages/core/src/kilocode/database-compat.ts index 9982d3d7285..733b2eece88 100644 --- a/packages/core/src/kilocode/database-compat.ts +++ b/packages/core/src/kilocode/database-compat.ts @@ -4,19 +4,29 @@ import type { Database } from "../database/database" type Db = Database.Interface["db"] export function ensure(db: Db) { - return db.transaction( - (tx) => - Effect.gen(function* () { - const rows = yield* tx.all<{ name: string }>("PRAGMA table_info('session_context_epoch')") - const names = new Set(rows.map((row) => row.name)) + const load = db.all<{ name: string }>("PRAGMA table_info('session_context_epoch')") + const ready = (rows: { name: string }[]) => { + const names = new Set(rows.map((row) => row.name)) + return ["agent", "replacement_seq", "revision"].every((name) => names.has(name)) + } + return load.pipe( + Effect.flatMap((rows) => { + if (ready(rows)) return Effect.void + return db.transaction( + (tx) => + Effect.gen(function* () { + const current = yield* tx.all<{ name: string }>("PRAGMA table_info('session_context_epoch')") + const names = new Set(current.map((row) => row.name)) - if (!names.has("agent")) - yield* tx.run("ALTER TABLE `session_context_epoch` ADD `agent` text DEFAULT 'build' NOT NULL") - if (!names.has("replacement_seq")) - yield* tx.run("ALTER TABLE `session_context_epoch` ADD `replacement_seq` integer") - if (!names.has("revision")) - yield* tx.run("ALTER TABLE `session_context_epoch` ADD `revision` integer DEFAULT 0 NOT NULL") - }), - { behavior: "immediate" }, + if (!names.has("agent")) + yield* tx.run("ALTER TABLE `session_context_epoch` ADD `agent` text DEFAULT 'build' NOT NULL") + if (!names.has("replacement_seq")) + yield* tx.run("ALTER TABLE `session_context_epoch` ADD `replacement_seq` integer") + if (!names.has("revision")) + yield* tx.run("ALTER TABLE `session_context_epoch` ADD `revision` integer DEFAULT 0 NOT NULL") + }), + { behavior: "immediate" }, + ) + }), ) } diff --git a/packages/core/src/kilocode/sqlite-error.ts b/packages/core/src/kilocode/sqlite-error.ts new file mode 100644 index 00000000000..60f52b531c0 --- /dev/null +++ b/packages/core/src/kilocode/sqlite-error.ts @@ -0,0 +1,10 @@ +import { Cause, Option } from "effect" +import { isSqlError } from "effect/unstable/sql/SqlError" + +export function isBusy(error: unknown): boolean { + if (isSqlError(error)) return error.reason._tag === "LockTimeoutError" + if (typeof error !== "object" || error === null || !("cause" in error) || error.cause === error) return false + if (!Cause.isCause(error.cause)) return isBusy(error.cause) + const failure = Cause.findErrorOption(error.cause) + return Option.isSome(failure) && isBusy(failure.value) +} diff --git a/packages/core/test/credential.test.ts b/packages/core/test/credential.test.ts index e6cbf820bb1..164f297cbc7 100644 --- a/packages/core/test/credential.test.ts +++ b/packages/core/test/credential.test.ts @@ -1,11 +1,15 @@ import path from "path" +import { Database as SQLite } from "bun:sqlite" // kilocode_change import { describe, expect } from "bun:test" +import { eq } from "drizzle-orm" // kilocode_change import { Effect, Layer } from "effect" import { Credential } from "@opencode-ai/core/credential" +import { CredentialTable } from "@opencode-ai/core/credential/sql" // kilocode_change import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { Integration } from "@opencode-ai/core/integration" // kilocode_change start import { Database } from "@opencode-ai/core/database/database" +import { FSUtil } from "@opencode-ai/core/fs-util" import { Global } from "@opencode-ai/core/global" // kilocode_change end import { tmpdir } from "./fixture/tmpdir" @@ -20,6 +24,16 @@ function localLayer(directory: string) { ) } +// kilocode_change start +function importer(dir: string, store: Database.Interface) { + return Credential.legacyImportLayer.pipe( + Layer.provide(Layer.succeed(Database.Service, store)), + Layer.provide(FSUtil.defaultLayer), + Layer.provide(Global.layerWith({ data: dir })), + ) +} +// kilocode_change end + describe("Credential", () => { it.live("stores, updates, lists, and removes credentials", () => Effect.acquireUseRelease( @@ -196,6 +210,70 @@ describe("Credential", () => { ), ) + it.live("skips unchanged legacy writes and defers locked reconciliation", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => { + const file = path.join(tmp.path, "credential.db") + const auth = path.join(tmp.path, "auth.json") + const write = (key: string) => + Effect.promise(() => Bun.write(auth, JSON.stringify({ kilo: { type: "api", key } }))) + return Effect.gen(function* () { + yield* write("first") + const store = yield* Database.Service + const layer = importer(tmp.path, store) + yield* Layer.build(Layer.fresh(layer)) + + const before = yield* store.db + .select() + .from(CredentialTable) + .where(eq(CredentialTable.integration_id, Integration.ID.make("kilo"))) + .get() + yield* Layer.build(Layer.fresh(layer)) + const unchanged = yield* store.db + .select() + .from(CredentialTable) + .where(eq(CredentialTable.integration_id, Integration.ID.make("kilo"))) + .get() + expect(unchanged?.time_updated).toBe(before?.time_updated) + + yield* write("second") + yield* store.db.run("PRAGMA busy_timeout = 0") + yield* Effect.acquireUseRelease( + Effect.sync(() => { + const holder = new SQLite(file) + holder.run("PRAGMA busy_timeout = 0") + holder.run("BEGIN IMMEDIATE") + return holder + }), + () => Layer.build(Layer.fresh(layer)), + (holder) => + Effect.sync(() => { + if (holder.inTransaction) holder.run("ROLLBACK") + holder.close() + }), + ) + + const stale = yield* store.db + .select() + .from(CredentialTable) + .where(eq(CredentialTable.integration_id, Integration.ID.make("kilo"))) + .get() + expect(stale?.value).toMatchObject({ type: "key", key: "first" }) + + yield* Layer.build(Layer.fresh(layer)) + const reconciled = yield* store.db + .select() + .from(CredentialTable) + .where(eq(CredentialTable.integration_id, Integration.ID.make("kilo"))) + .get() + expect(reconciled?.value).toMatchObject({ type: "key", key: "second" }) + }).pipe(Effect.provide(Database.layerFromPath(file)), Effect.scoped) + }, + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + it.live("dual-writes stored credentials for released auth.json readers", () => Effect.acquireRelease( Effect.promise(() => tmpdir()), diff --git a/packages/core/test/kilocode/database-migration-compat.test.ts b/packages/core/test/kilocode/database-migration-compat.test.ts index 8546df16b37..83d35cf1666 100644 --- a/packages/core/test/kilocode/database-migration-compat.test.ts +++ b/packages/core/test/kilocode/database-migration-compat.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test" +import { Database as SQLite } from "bun:sqlite" import { SqliteClient } from "@effect/sql-sqlite-bun" import { EffectDrizzleSqlite } from "@opencode-ai/effect-drizzle-sqlite" import { DatabaseMigration } from "@opencode-ai/core/database/migration" @@ -159,6 +160,22 @@ describe("database migration compatibility", () => { sql`SELECT agent, replacement_seq AS replacementSeq, revision FROM session_context_epoch WHERE session_id = 'session'`, ), ).toEqual({ agent: "build", replacementSeq: 4, revision: 1 }) + + yield* db.run("PRAGMA busy_timeout = 0") + yield* Effect.acquireUseRelease( + Effect.sync(() => { + const holder = new SQLite(filename) + holder.run("PRAGMA busy_timeout = 0") + holder.run("BEGIN IMMEDIATE") + return holder + }), + () => ensure(db), + (holder) => + Effect.sync(() => { + if (holder.inTransaction) holder.run("ROLLBACK") + holder.close() + }), + ) }), ).pipe(Effect.provide(Database.layerFromPath(filename)), Effect.scoped), ) diff --git a/packages/effect-drizzle-sqlite/src/sqlite-core/effect/session.ts b/packages/effect-drizzle-sqlite/src/sqlite-core/effect/session.ts index 15a56f2ca7d..535b232ac4b 100644 --- a/packages/effect-drizzle-sqlite/src/sqlite-core/effect/session.ts +++ b/packages/effect-drizzle-sqlite/src/sqlite-core/effect/session.ts @@ -279,7 +279,13 @@ export class SQLiteEffectPreparedQuery< assertUnreachable(cacheStrat) }).pipe( Effect.catch((e) => { - return Effect.fail(new EffectDrizzleQueryError({ query: queryString, params, cause: Cause.fail(e) })) + return Effect.fail( + new EffectDrizzleQueryError({ + query: queryString, + params: params.map(() => ""), // kilocode_change - bound values may contain credentials + cause: Cause.fail(e), + }), + ) }), ) } diff --git a/packages/effect-drizzle-sqlite/test/sqlite.test.ts b/packages/effect-drizzle-sqlite/test/sqlite.test.ts index 5303ee069ac..0148a1b9d4d 100644 --- a/packages/effect-drizzle-sqlite/test/sqlite.test.ts +++ b/packages/effect-drizzle-sqlite/test/sqlite.test.ts @@ -130,6 +130,24 @@ test("preserves failed transaction begin errors", async () => { } }) +// kilocode_change start - query errors must never expose bound credential values +test("redacts bound values from query errors", async () => { + await run( + Effect.gen(function* () { + const db = yield* makeDb + const secret = "must-not-leak" + yield* db.insert(users).values({ id: 1, name: "Ada" }) + + const error = yield* db.insert(users).values({ id: 1, name: secret }).pipe(Effect.flip) + + expect(error.message).not.toContain(secret) + expect(error.params).not.toContain(secret) + expect(error.params.every((param) => param === "")).toBe(true) + }), + ) +}) +// kilocode_change end + test("supports returning and rejects empty update sets", async () => { await run( Effect.gen(function* () { diff --git a/packages/opencode/src/kilocode/database/sqlite-error.ts b/packages/opencode/src/kilocode/database/sqlite-error.ts index 31e476a8005..fafda79b056 100644 --- a/packages/opencode/src/kilocode/database/sqlite-error.ts +++ b/packages/opencode/src/kilocode/database/sqlite-error.ts @@ -1,7 +1,5 @@ -import { isSqlError } from "effect/unstable/sql/SqlError" +import { isBusy } from "@opencode-ai/core/kilocode/sqlite-error" + +export { isBusy } export const busyMessage = "Database is busy. Please try again in a moment." - -export function isBusy(error: unknown) { - return isSqlError(error) && error.reason._tag === "LockTimeoutError" -} diff --git a/packages/opencode/test/kilocode/database/sqlite-error.test.ts b/packages/opencode/test/kilocode/database/sqlite-error.test.ts index 7a3ea7472e1..ff6e5e73d5f 100644 --- a/packages/opencode/test/kilocode/database/sqlite-error.test.ts +++ b/packages/opencode/test/kilocode/database/sqlite-error.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "bun:test" +import { Cause } from "effect" import { LockTimeoutError, SqlError, UnknownError } from "effect/unstable/sql/SqlError" +import { EffectDrizzleQueryError } from "drizzle-orm/effect-core/errors" import { busyMessage, isBusy } from "@/kilocode/database/sqlite-error" describe("SQLite errors", () => { @@ -27,4 +29,22 @@ describe("SQLite errors", () => { expect(isBusy(error)).toBe(false) }) + + test("recognizes lock timeouts wrapped by Drizzle", () => { + const error = new EffectDrizzleQueryError({ + query: "update credential set value = ?", + params: [""], + cause: Cause.fail( + new SqlError({ + reason: new LockTimeoutError({ + cause: new Error("database is locked"), + message: "Failed to execute statement", + operation: "execute", + }), + }), + ), + }) + + expect(isBusy(error)).toBe(true) + }) })