diff --git a/.changeset/quiet-bash-permissions.md b/.changeset/quiet-bash-permissions.md new file mode 100644 index 000000000000..e8e940c2a6d3 --- /dev/null +++ b/.changeset/quiet-bash-permissions.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Respect deleted bash permission overrides after upgrading. diff --git a/packages/opencode/src/kilocode/config/config.ts b/packages/opencode/src/kilocode/config/config.ts index bc7b9cd605ae..0d83de5be61e 100644 --- a/packages/opencode/src/kilocode/config/config.ts +++ b/packages/opencode/src/kilocode/config/config.ts @@ -2,7 +2,7 @@ import path from "path" import { pathToFileURL } from "url" import { existsSync } from "fs" import { Effect, Schema } from "effect" -import { applyEdits, modify, parse as parseJsonc } from "jsonc-parser" +import { applyEdits, modify, parse as parseJsonc, type ParseError } from "jsonc-parser" import { mergeDeep } from "remeda" import * as Log from "@opencode-ai/core/util/log" import { Global } from "@opencode-ai/core/global" @@ -442,6 +442,7 @@ export namespace KilocodeConfig { /** Global config file names in read-merge order (lowest-to-highest precedence). */ export const GLOBAL_CONFIG_FILES = ["config.json", "kilo.json", "kilo.jsonc", "opencode.json", "opencode.jsonc"] + const BASH_PERMISSION_MIGRATION = ".bash-permission-migrated" /** * Migrate bash permission for existing users before config is consumed. @@ -449,36 +450,71 @@ export namespace KilocodeConfig { * Existing users (those with at least one global config file or the legacy TOML * config) who have no explicit `permission.bash` setting get `bash: "allow"` * written to their highest-precedence config file. This preserves their current - * behavior now that the new default is `bash: "ask"`. + * behavior now that the new default is `bash: "ask"`. A completion marker makes + * the migration idempotent so subsequent user edits are not migrated again. */ export async function migrateBashPermission() { + const marker = path.join(Global.Path.config, BASH_PERMISSION_MIGRATION) + if (existsSync(marker)) return + const done = () => + Bun.write(marker, "").then( + () => undefined, + (err) => log.warn("failed to record bash permission migration", { path: marker, err }), + ) const files = GLOBAL_CONFIG_FILES.map((f) => path.join(Global.Path.config, f)) const legacy = path.join(Global.Path.config, "config") const existing = files.filter((f) => existsSync(f)) const hasLegacy = existsSync(legacy) // no global config → new user, they'll get the new bash:ask default - if (existing.length === 0 && !hasLegacy) return + if (existing.length === 0 && !hasLegacy) return done() const configs: Array<{ file: string; data: Record }> = [] + let hasFailure = false // check if any config file already has an explicit bash permission for (const file of existing) { - const text = await Bun.file(file) - .text() - .catch(() => "") - const data = parseJsonc(text) ?? {} + let text: string + try { + text = await Bun.file(file).text() + } catch (err) { + hasFailure = true + log.warn("skipping bash permission migration due to unreadable config", { file, err }) + continue + } + if (text.trim() === "") { + const data: Record = {} + configs.push({ file, data }) + continue + } + const errors: ParseError[] = [] + const data = (parseJsonc(text, errors, { allowTrailingComma: true }) as Record | undefined) ?? {} + if (errors.length > 0) { + hasFailure = true + log.warn("skipping bash permission migration due to malformed config", { file, errors }) + continue + } configs.push({ file, data }) - if (typeof data.permission === "string" || (isRecord(data.permission) && data.permission.bash)) return + if (typeof data.permission === "string" || (isRecord(data.permission) && data.permission.bash)) return done() } + if (hasFailure) return + // A schema-only file is generated for editor completion. It does not mean // the user predates the bash permission default. - if (!hasLegacy && configs.every((item) => Object.keys(item.data).every((key) => key === "$schema"))) return + if (!hasLegacy && configs.every((item) => Object.keys(item.data).every((key) => key === "$schema"))) return done() // also check legacy TOML config for bash permission if (hasLegacy) { - const toml = await import(pathToFileURL(legacy).href, { with: { type: "toml" } }).catch(() => undefined) - if (toml?.default?.permission?.bash) return + try { + const toml = await import(pathToFileURL(legacy).href, { with: { type: "toml" } }) + if (toml?.default?.permission?.bash) return done() + } catch (err) { + log.warn("skipping bash permission migration due to unreadable or malformed legacy config", { + path: legacy, + err, + }) + return + } } // existing user without bash permission → write bash:allow to highest-precedence file @@ -492,6 +528,7 @@ export namespace KilocodeConfig { formattingOptions: { insertSpaces: true, tabSize: 2 }, }) await Bun.write(target, applyEdits(text, edits)) + await done() log.info("migrated bash permission to allow for existing user", { path: target }) return } @@ -499,6 +536,7 @@ export namespace KilocodeConfig { const data = parseJsonc(text) ?? {} const merged = { ...data, permission: { ...data.permission, bash: "allow" } } await Bun.write(target, JSON.stringify(merged, null, 2)) + await done() log.info("migrated bash permission to allow for existing user", { path: target }) } diff --git a/packages/opencode/test/kilocode/config/config.test.ts b/packages/opencode/test/kilocode/config/config.test.ts index b1ff3ae3a9eb..d64bbaf14e5b 100644 --- a/packages/opencode/test/kilocode/config/config.test.ts +++ b/packages/opencode/test/kilocode/config/config.test.ts @@ -1541,4 +1541,139 @@ describe("bash permission migration", () => { await disposeAllInstances() } }) + + test("does not restore a migrated bash permission after the user deletes it", async () => { + await using tmp = await tmpdir({ + init: async (dir) => { + await writeConfig(dir, { permission: { read: "allow" } }, "kilo.jsonc") + }, + }) + + const prev = Global.Path.config + ;(Global.Path as { config: string }).config = tmp.path + await clear() + await disposeAllInstances() + + try { + const file = path.join(tmp.path, "kilo.jsonc") + await KilocodeConfig.migrateBashPermission() + expect(JSON.parse(await Filesystem.readText(file)).permission.bash).toBe("allow") + + await writeConfig(tmp.path, { permission: { read: "allow" } }, "kilo.jsonc") + await KilocodeConfig.migrateBashPermission() + + expect(JSON.parse(await Filesystem.readText(file)).permission).toEqual({ read: "allow" }) + } finally { + ;(Global.Path as { config: string }).config = prev + await clear() + await disposeAllInstances() + } + }) + + test("does not later migrate a fresh install after its config gains settings", async () => { + await using tmp = await tmpdir() + + const prev = Global.Path.config + ;(Global.Path as { config: string }).config = tmp.path + await clear() + await disposeAllInstances() + + try { + await KilocodeConfig.migrateBashPermission() + await writeConfig(tmp.path, { model: "test/model" }, "kilo.jsonc") + await KilocodeConfig.migrateBashPermission() + + expect(JSON.parse(await Filesystem.readText(path.join(tmp.path, "kilo.jsonc")))).toEqual({ + model: "test/model", + }) + } finally { + ;(Global.Path as { config: string }).config = prev + await clear() + await disposeAllInstances() + } + }) + + test("does not mark migration done for malformed config and retries after fix", async () => { + await using tmp = await tmpdir() + const prev = Global.Path.config + ;(Global.Path as { config: string }).config = tmp.path + await clear() + await disposeAllInstances() + + try { + const file = path.join(tmp.path, "kilo.jsonc") + const marker = path.join(tmp.path, ".bash-permission-migrated") + await Filesystem.write(file, "{ not valid json") + await KilocodeConfig.migrateBashPermission() + expect(await Bun.file(marker).exists()).toBe(false) + expect(await Filesystem.readText(file)).toBe("{ not valid json") + await Filesystem.write(file, JSON.stringify({ permission: { read: "allow" } })) + await KilocodeConfig.migrateBashPermission() + expect(await Bun.file(marker).exists()).toBe(true) + expect(JSON.parse(await Filesystem.readText(file)).permission.bash).toBe("allow") + } finally { + ;(Global.Path as { config: string }).config = prev + await clear() + await disposeAllInstances() + } + }) + + test("does not mark migration done for unreadable config and retries after fix", async () => { + await using tmp = await tmpdir() + const prev = Global.Path.config + ;(Global.Path as { config: string }).config = tmp.path + await clear() + await disposeAllInstances() + + try { + const file = path.join(tmp.path, "kilo.jsonc") + const marker = path.join(tmp.path, ".bash-permission-migrated") + await Filesystem.write(file, JSON.stringify({ permission: { read: "allow" } })) + await $`rm ${file}`.quiet().nothrow() + await $`mkdir -p ${file}`.quiet() + await KilocodeConfig.migrateBashPermission() + expect(await Bun.file(marker).exists()).toBe(false) + await $`rm -rf ${file}`.quiet() + await Filesystem.write(file, JSON.stringify({ permission: { read: "allow" } })) + await KilocodeConfig.migrateBashPermission() + expect(await Bun.file(marker).exists()).toBe(true) + expect(JSON.parse(await Filesystem.readText(file)).permission.bash).toBe("allow") + } finally { + ;(Global.Path as { config: string }).config = prev + await clear() + await disposeAllInstances() + } + }) + + test("migrates config with trailing commas", async () => { + await using tmp = await tmpdir() + const prev = Global.Path.config + ;(Global.Path as { config: string }).config = tmp.path + await clear() + await disposeAllInstances() + + try { + const file = path.join(tmp.path, "kilo.jsonc") + const marker = path.join(tmp.path, ".bash-permission-migrated") + await Filesystem.write( + file, + `{ + "$schema": "https://app.kilo.ai/config.json", + "permission": { + "read": "allow", + }, +}`, + ) + await KilocodeConfig.migrateBashPermission() + expect(await Bun.file(marker).exists()).toBe(true) + const text = await Filesystem.readText(file) + const parsed = ConfigParse.jsonc(text, file) as Record + expect(parsed.permission.bash).toBe("allow") + expect(text).toContain(`"read": "allow"`) + } finally { + ;(Global.Path as { config: string }).config = prev + await clear() + await disposeAllInstances() + } + }) })