From bc5f25af230f06df14b1be8c729fee310801dc90 Mon Sep 17 00:00:00 2001 From: usr_bin_roygbiv Date: Tue, 16 Jun 2026 17:57:01 -0500 Subject: [PATCH] fix(coding-agent/config): support comments in JSON config migration --- packages/coding-agent/CHANGELOG.md | 3 ++ .../coding-agent/src/config/config-file.ts | 2 +- .../coding-agent/src/config/keybindings.ts | 4 +- packages/coding-agent/src/config/settings.ts | 6 +-- .../test/keybindings-migration.test.ts | 37 +++++++++++++++++++ .../test/model-registry-create.test.ts | 19 ++++++++++ .../test/settings-manager.test.ts | 19 ++++++++++ 7 files changed, 84 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a0af3f081..f7a771e02 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,9 @@ ### Fixed - Fixed active `/goal` mode being paused by internal compaction and session-switch lifecycle aborts, and made those switches persist wall-clock goal usage without charging time spent in another session to a preserved goal. +### Fixed + +- Fixed legacy `settings.json`, `models.json`, and `keybindings.json` migration/loading to accept comments in JSON config files. ## [16.0.8] - 2026-06-18 diff --git a/packages/coding-agent/src/config/config-file.ts b/packages/coding-agent/src/config/config-file.ts index 7769350f6..018d25c8f 100644 --- a/packages/coding-agent/src/config/config-file.ts +++ b/packages/coding-agent/src/config/config-file.ts @@ -40,7 +40,7 @@ function migrateJsonToYml(jsonPath: string, ymlPath: string) { } const content = fs.readFileSync(jsonPath, "utf-8"); - const parsed = JSON.parse(content); + const parsed = JSONC.parse(content); if (!parsed) { logger.warn("migrateJsonToYml: invalid json structure", { path: jsonPath }); migratedPaths.add(key); diff --git a/packages/coding-agent/src/config/keybindings.ts b/packages/coding-agent/src/config/keybindings.ts index fe339862a..5ad80c670 100644 --- a/packages/coding-agent/src/config/keybindings.ts +++ b/packages/coding-agent/src/config/keybindings.ts @@ -10,7 +10,7 @@ import { KeybindingsManager as TuiKeybindingsManager, } from "@oh-my-pi/pi-tui"; import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils"; -import { YAML } from "bun"; +import { JSONC, YAML } from "bun"; /** * Application-level keybindings (coding agent specific). @@ -381,7 +381,7 @@ function loadRawConfig(filePath: string): unknown { try { const content = fs.readFileSync(filePath, "utf-8"); if (filePath.endsWith(".json")) { - return JSON.parse(content); + return JSONC.parse(content); } if (filePath.endsWith(".yml") || filePath.endsWith(".yaml")) { return YAML.parse(content); diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index 93b1c66d7..00578c18c 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -24,7 +24,7 @@ import { procmgr, setDefaultTabWidth, } from "@oh-my-pi/pi-utils"; -import { YAML } from "bun"; +import { JSONC, YAML } from "bun"; import { type Settings as SettingsCapabilityItem, settingsCapability } from "../capability/settings"; import type { ModelRole } from "../config/model-roles"; import { loadCapability } from "../discovery"; @@ -668,9 +668,9 @@ export class Settings { // 1. Migrate from settings.json const settingsJsonPath = path.join(this.#agentDir, "settings.json"); try { - const parsed = JSON.parse(await Bun.file(settingsJsonPath).text()); + const parsed: unknown = JSONC.parse(await Bun.file(settingsJsonPath).text()); if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { - settings = this.#deepMerge(settings, this.#migrateRawSettings(parsed)); + settings = this.#deepMerge(settings, this.#migrateRawSettings(parsed as RawSettings)); migrated = true; try { fs.renameSync(settingsJsonPath, `${settingsJsonPath}.bak`); diff --git a/packages/coding-agent/test/keybindings-migration.test.ts b/packages/coding-agent/test/keybindings-migration.test.ts index 6cca930c5..53c8727d1 100644 --- a/packages/coding-agent/test/keybindings-migration.test.ts +++ b/packages/coding-agent/test/keybindings-migration.test.ts @@ -55,6 +55,43 @@ describe("KeybindingsManager.create", () => { } }); + it("migrates legacy keybinding JSON with comments to YAML during create", async () => { + const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-keybindings-")); + const jsonPath = path.join(agentDir, "keybindings.json"); + const ymlPath = path.join(agentDir, "keybindings.yml"); + + await Bun.write( + jsonPath, + `{ + // Legacy config files may contain comments from hand-edited examples. + "fork": "ctrl+f", + "selectConfirm": "enter", + "cursorUp": "ctrl+p", + "app.clipboard.copyPrompt": ["alt+c", "ctrl+shift+c"] +} +`, + ); + + try { + const manager = KeybindingsManager.create(agentDir); + const writtenConfig = YAML.parse(await Bun.file(ymlPath).text()); + + expect(manager.getKeys("app.session.fork")).toEqual(["ctrl+f"]); + expect(manager.getKeys("tui.select.confirm")).toEqual(["enter"]); + expect(manager.getKeys("tui.editor.cursorUp")).toEqual(["ctrl+p"]); + expect(manager.getKeys("app.clipboard.copyPrompt")).toEqual(["alt+c", "ctrl+shift+c"]); + expect(writtenConfig).toEqual({ + "app.clipboard.copyPrompt": ["alt+c", "ctrl+shift+c"], + "app.session.fork": "ctrl+f", + "tui.editor.cursorUp": "ctrl+p", + "tui.select.confirm": "enter", + }); + expect(await Bun.file(jsonPath).exists()).toBe(true); + } finally { + await fs.rm(agentDir, { recursive: true, force: true }); + } + }); + it("loads keybindings.yml directly", async () => { const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-keybindings-")); const configPath = path.join(agentDir, "keybindings.yml"); diff --git a/packages/coding-agent/test/model-registry-create.test.ts b/packages/coding-agent/test/model-registry-create.test.ts index 2f809a66a..0253a3efc 100644 --- a/packages/coding-agent/test/model-registry-create.test.ts +++ b/packages/coding-agent/test/model-registry-create.test.ts @@ -69,4 +69,23 @@ describe("ModelRegistry.create() factory (F6)", () => { const mtime2 = fs.statSync(yml).mtimeMs; expect(mtime2).toBe(mtime1); }); + + test("ConfigFile migrates legacy models.json containing comments", async () => { + const yml = path.join(tempDir.path(), "models.yml"); + const json = path.join(tempDir.path(), "models.json"); + await Bun.write( + json, + `{ + // Custom models comment + "providers": { + /* Block comment */ + } + }`, + ); + + const cf = new ConfigFile("models", ModelsConfigSchema, yml); + const result = cf.tryLoad(); + expect(result.status).toBe("ok"); + expect(fs.existsSync(yml)).toBe(true); + }); }); diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index b7a1f7829..454948ace 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -494,5 +494,24 @@ describe("Settings", () => { expect(fs.readFileSync(path.join(agentDir, "last-changelog-version"), "utf8")).toBe("0.41.0"); }); + + it("migrates from settings.json containing comments", async () => { + const jsonPath = path.join(agentDir, "settings.json"); + await fs.promises.writeFile( + jsonPath, + `{ + // This is a comment + "display": { + /* Multiline comment */ + "showTokenUsage": true + } + }`, + ); + + const settings = await Settings.init({ cwd: projectDir, agentDir }); + expect(settings.get("display.showTokenUsage")).toBe(true); + expect(fs.existsSync(jsonPath)).toBe(false); + expect(fs.existsSync(`${jsonPath}.bak`)).toBe(true); + }); }); });