From de2736bad0bdc8998ab583c44af3aa8e4a519199 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Tue, 17 Feb 2026 00:08:32 +0100 Subject: [PATCH] refactor(coding-agent): improve settings storage semantics and error handling --- packages/coding-agent/CHANGELOG.md | 16 + packages/coding-agent/docs/sdk.md | 7 + .../coding-agent/examples/sdk/10-settings.ts | 13 + .../coding-agent/src/core/settings-manager.ts | 409 +++++++++++------- packages/coding-agent/src/main.ts | 13 + .../test/settings-manager-bug.test.ts | 52 ++- .../test/settings-manager.test.ts | 28 +- 7 files changed, 386 insertions(+), 152 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a74b82af..d87bf471 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,22 @@ ## [Unreleased] +### Breaking Changes + +- `SettingsManager` persistence semantics changed for SDK consumers. Setters now update in-memory state immediately and queue disk writes. Code that requires durable on-disk settings must call `await settingsManager.flush()`. + +### Added + +- Added `SettingsManager.drainErrors()` for caller-controlled settings I/O error handling without manager-side console output. + +### Changed + +- `SettingsManager` now uses scoped storage abstraction with per-scope locked read/merge/write persistence for global and project settings. + +### Fixed + +- Fixed project settings persistence to preserve unrelated external edits via merge-on-write, while still applying in-memory changes for modified keys. + ## [0.52.12] - 2026-02-13 ### Added diff --git a/packages/coding-agent/docs/sdk.md b/packages/coding-agent/docs/sdk.md index 33d6b0d6..0437f777 100644 --- a/packages/coding-agent/docs/sdk.md +++ b/packages/coding-agent/docs/sdk.md @@ -700,6 +700,13 @@ Settings load from two locations and merge: Project overrides global. Nested objects merge keys. Setters modify global settings by default. +**Persistence and error handling semantics:** + +- Settings getters/setters are synchronous for in-memory state. +- Setters enqueue persistence writes asynchronously. +- Call `await settingsManager.flush()` when you need a durability boundary (for example, before process exit or before asserting file contents in tests). +- `SettingsManager` does not print settings I/O errors. Use `settingsManager.drainErrors()` and report them in your app layer. + > See [examples/sdk/10-settings.ts](../examples/sdk/10-settings.ts) ## ResourceLoader diff --git a/packages/coding-agent/examples/sdk/10-settings.ts b/packages/coding-agent/examples/sdk/10-settings.ts index fac2bc5e..b2abf7e4 100644 --- a/packages/coding-agent/examples/sdk/10-settings.ts +++ b/packages/coding-agent/examples/sdk/10-settings.ts @@ -24,6 +24,19 @@ await createAgentSession({ console.log("Session created with custom settings"); +// Setters update memory immediately and queue persistence writes. +// Call flush() when you need a durability boundary. +settingsManager.setDefaultThinkingLevel("low"); +await settingsManager.flush(); + +// Surface settings I/O errors at the app layer. +const settingsErrors = settingsManager.drainErrors(); +if (settingsErrors.length > 0) { + for (const { scope, error } of settingsErrors) { + console.warn(`Warning (${scope} settings): ${error.message}`); + } +} + // For testing without file I/O: const inMemorySettings = SettingsManager.inMemory({ compaction: { enabled: false }, diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index 2d6fe85d..f6ac85e2 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -1,6 +1,7 @@ import type { Transport } from "@mariozechner/pi-ai"; import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { dirname, join } from "path"; +import lockfile from "proper-lockfile"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.js"; export interface CompactionSettings { @@ -123,67 +124,156 @@ function deepMergeSettings(base: Settings, overrides: Settings): Settings { return result; } +export type SettingsScope = "global" | "project"; + +export interface SettingsStorage { + withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void; +} + +export interface SettingsError { + scope: SettingsScope; + error: Error; +} + +export class FileSettingsStorage implements SettingsStorage { + private globalSettingsPath: string; + private projectSettingsPath: string; + + constructor(cwd: string = process.cwd(), agentDir: string = getAgentDir()) { + this.globalSettingsPath = join(agentDir, "settings.json"); + this.projectSettingsPath = join(cwd, CONFIG_DIR_NAME, "settings.json"); + } + + withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { + const path = scope === "global" ? this.globalSettingsPath : this.projectSettingsPath; + const dir = dirname(path); + if (!existsSync(dir)) { + mkdirSync(dir, { recursive: true }); + } + + let release: (() => void) | undefined; + try { + release = lockfile.lockSync(path, { realpath: false }); + const current = existsSync(path) ? readFileSync(path, "utf-8") : undefined; + const next = fn(current); + if (next !== undefined) { + writeFileSync(path, next, "utf-8"); + } + } finally { + if (release) { + release(); + } + } + } +} + +export class InMemorySettingsStorage implements SettingsStorage { + private global: string | undefined; + private project: string | undefined; + + withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { + const current = scope === "global" ? this.global : this.project; + const next = fn(current); + if (next !== undefined) { + if (scope === "global") { + this.global = next; + } else { + this.project = next; + } + } + } +} + export class SettingsManager { - private settingsPath: string | null; - private projectSettingsPath: string | null; + private storage: SettingsStorage; private globalSettings: Settings; - private inMemoryProjectSettings: Settings; // For in-memory mode + private projectSettings: Settings; private settings: Settings; - private persist: boolean; - private modifiedFields = new Set(); // Track fields modified during session - private modifiedNestedFields = new Map>(); // Track nested field modifications - private globalSettingsLoadError: Error | null = null; // Track if settings file had parse errors + private modifiedFields = new Set(); // Track global fields modified during session + private modifiedNestedFields = new Map>(); // Track global nested field modifications + private modifiedProjectFields = new Set(); // Track project fields modified during session + private modifiedProjectNestedFields = new Map>(); // Track project nested field modifications + private globalSettingsLoadError: Error | null = null; // Track if global settings file had parse errors + private projectSettingsLoadError: Error | null = null; // Track if project settings file had parse errors + private writeQueue: Promise = Promise.resolve(); + private errors: SettingsError[]; private constructor( - settingsPath: string | null, - projectSettingsPath: string | null, - initialSettings: Settings, - persist: boolean, - loadError: Error | null = null, + storage: SettingsStorage, + initialGlobal: Settings, + initialProject: Settings, + globalLoadError: Error | null = null, + projectLoadError: Error | null = null, + initialErrors: SettingsError[] = [], ) { - this.settingsPath = settingsPath; - this.projectSettingsPath = projectSettingsPath; - this.persist = persist; - this.globalSettings = initialSettings; - this.inMemoryProjectSettings = {}; - this.globalSettingsLoadError = loadError; - const projectSettings = this.loadProjectSettings(); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); + this.storage = storage; + this.globalSettings = initialGlobal; + this.projectSettings = initialProject; + this.globalSettingsLoadError = globalLoadError; + this.projectSettingsLoadError = projectLoadError; + this.errors = [...initialErrors]; + this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); } /** Create a SettingsManager that loads from files */ static create(cwd: string = process.cwd(), agentDir: string = getAgentDir()): SettingsManager { - const settingsPath = join(agentDir, "settings.json"); - const projectSettingsPath = join(cwd, CONFIG_DIR_NAME, "settings.json"); + const storage = new FileSettingsStorage(cwd, agentDir); + return SettingsManager.fromStorage(storage); + } - let globalSettings: Settings = {}; - let loadError: Error | null = null; - - try { - globalSettings = SettingsManager.loadFromFile(settingsPath); - } catch (error) { - loadError = error as Error; - console.error(`Warning: Invalid JSON in ${settingsPath}: ${error}`); - console.error(`Fix the syntax error to enable settings persistence.`); + /** Create a SettingsManager from an arbitrary storage backend */ + static fromStorage(storage: SettingsStorage): SettingsManager { + const globalLoad = SettingsManager.tryLoadFromStorage(storage, "global"); + const projectLoad = SettingsManager.tryLoadFromStorage(storage, "project"); + const initialErrors: SettingsError[] = []; + if (globalLoad.error) { + initialErrors.push({ scope: "global", error: globalLoad.error }); + } + if (projectLoad.error) { + initialErrors.push({ scope: "project", error: projectLoad.error }); } - return new SettingsManager(settingsPath, projectSettingsPath, globalSettings, true, loadError); + return new SettingsManager( + storage, + globalLoad.settings, + projectLoad.settings, + globalLoad.error, + projectLoad.error, + initialErrors, + ); } /** Create an in-memory SettingsManager (no file I/O) */ static inMemory(settings: Partial = {}): SettingsManager { - return new SettingsManager(null, null, settings, false); + const storage = new InMemorySettingsStorage(); + return new SettingsManager(storage, settings, {}); } - private static loadFromFile(path: string): Settings { - if (!existsSync(path)) { + private static loadFromStorage(storage: SettingsStorage, scope: SettingsScope): Settings { + let content: string | undefined; + storage.withLock(scope, (current) => { + content = current; + return undefined; + }); + + if (!content) { return {}; } - const content = readFileSync(path, "utf-8"); const settings = JSON.parse(content); return SettingsManager.migrateSettings(settings); } + private static tryLoadFromStorage( + storage: SettingsStorage, + scope: SettingsScope, + ): { settings: Settings; error: Error | null } { + try { + return { settings: SettingsManager.loadFromStorage(storage, scope), error: null }; + } catch (error) { + return { settings: {}, error: error as Error }; + } + } + /** Migrate old settings format to new format */ private static migrateSettings(settings: Record): Settings { // Migrate queueMode -> steeringMode @@ -222,55 +312,39 @@ export class SettingsManager { return settings as Settings; } - private loadProjectSettings(): Settings { - // In-memory mode: return stored in-memory project settings - if (!this.persist) { - return structuredClone(this.inMemoryProjectSettings); - } - - if (!this.projectSettingsPath || !existsSync(this.projectSettingsPath)) { - return {}; - } - - try { - const content = readFileSync(this.projectSettingsPath, "utf-8"); - const settings = JSON.parse(content); - return SettingsManager.migrateSettings(settings); - } catch (error) { - console.error(`Warning: Could not read project settings file: ${error}`); - return {}; - } - } - getGlobalSettings(): Settings { return structuredClone(this.globalSettings); } getProjectSettings(): Settings { - return this.loadProjectSettings(); + return structuredClone(this.projectSettings); } reload(): void { - let nextGlobalSettings: Settings | null = null; - - if (this.persist && this.settingsPath) { - try { - nextGlobalSettings = SettingsManager.loadFromFile(this.settingsPath); - this.globalSettingsLoadError = null; - } catch (error) { - this.globalSettingsLoadError = error as Error; - } - } - - if (nextGlobalSettings) { - this.globalSettings = nextGlobalSettings; + const globalLoad = SettingsManager.tryLoadFromStorage(this.storage, "global"); + if (!globalLoad.error) { + this.globalSettings = globalLoad.settings; + this.globalSettingsLoadError = null; + } else { + this.globalSettingsLoadError = globalLoad.error; + this.recordError("global", globalLoad.error); } this.modifiedFields.clear(); this.modifiedNestedFields.clear(); + this.modifiedProjectFields.clear(); + this.modifiedProjectNestedFields.clear(); - const projectSettings = this.loadProjectSettings(); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); + const projectLoad = SettingsManager.tryLoadFromStorage(this.storage, "project"); + if (!projectLoad.error) { + this.projectSettings = projectLoad.settings; + this.projectSettingsLoadError = null; + } else { + this.projectSettingsLoadError = projectLoad.error; + this.recordError("project", projectLoad.error); + } + + this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); } /** Apply additional overrides on top of current settings */ @@ -278,7 +352,7 @@ export class SettingsManager { this.settings = deepMergeSettings(this.settings, overrides); } - /** Mark a field as modified during this session */ + /** Mark a global field as modified during this session */ private markModified(field: keyof Settings, nestedKey?: string): void { this.modifiedFields.add(field); if (nestedKey) { @@ -289,80 +363,123 @@ export class SettingsManager { } } - private save(): void { - if (this.persist && this.settingsPath) { - // Don't overwrite if the file had parse errors on initial load - if (this.globalSettingsLoadError) { - // Re-merge to update active settings even though we can't persist - const projectSettings = this.loadProjectSettings(); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); - return; + /** Mark a project field as modified during this session */ + private markProjectModified(field: keyof Settings, nestedKey?: string): void { + this.modifiedProjectFields.add(field); + if (nestedKey) { + if (!this.modifiedProjectNestedFields.has(field)) { + this.modifiedProjectNestedFields.set(field, new Set()); } + this.modifiedProjectNestedFields.get(field)!.add(nestedKey); + } + } - try { - const dir = dirname(this.settingsPath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } + private recordError(scope: SettingsScope, error: unknown): void { + const normalizedError = error instanceof Error ? error : new Error(String(error)); + this.errors.push({ scope, error: normalizedError }); + } - // Re-read current file to get latest external changes - const currentFileSettings = SettingsManager.loadFromFile(this.settingsPath); - - // Start with file settings as base - preserves external edits - const mergedSettings: Settings = { ...currentFileSettings }; - - // Only override with in-memory values for fields that were explicitly modified during this session - for (const field of this.modifiedFields) { - const value = this.globalSettings[field]; - - // Handle nested objects specially - merge at nested level to preserve unmodified nested keys - if (this.modifiedNestedFields.has(field) && typeof value === "object" && value !== null) { - const nestedModified = this.modifiedNestedFields.get(field)!; - const baseNested = (currentFileSettings[field] as Record) ?? {}; - const inMemoryNested = value as Record; - const mergedNested = { ...baseNested }; - for (const nestedKey of nestedModified) { - mergedNested[nestedKey] = inMemoryNested[nestedKey]; - } - (mergedSettings as Record)[field] = mergedNested; - } else { - // For top-level primitives and arrays, use the modified value directly - (mergedSettings as Record)[field] = value; - } - } - - this.globalSettings = mergedSettings; - writeFileSync(this.settingsPath, JSON.stringify(this.globalSettings, null, 2), "utf-8"); - } catch (error) { - // File may have been externally modified with invalid JSON - don't overwrite - console.error(`Warning: Could not save settings file: ${error}`); - } + private clearModifiedScope(scope: SettingsScope): void { + if (scope === "global") { + this.modifiedFields.clear(); + this.modifiedNestedFields.clear(); + return; } - // Always re-merge to update active settings (needed for both file and inMemory modes) - const projectSettings = this.loadProjectSettings(); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); + this.modifiedProjectFields.clear(); + this.modifiedProjectNestedFields.clear(); + } + + private enqueueWrite(scope: SettingsScope, task: () => void): void { + this.writeQueue = this.writeQueue + .then(() => { + task(); + this.clearModifiedScope(scope); + }) + .catch((error) => { + this.recordError(scope, error); + }); + } + + private cloneModifiedNestedFields(source: Map>): Map> { + const snapshot = new Map>(); + for (const [key, value] of source.entries()) { + snapshot.set(key, new Set(value)); + } + return snapshot; + } + + private persistScopedSettings( + scope: SettingsScope, + snapshotSettings: Settings, + modifiedFields: Set, + modifiedNestedFields: Map>, + ): void { + this.storage.withLock(scope, (current) => { + const currentFileSettings = current + ? SettingsManager.migrateSettings(JSON.parse(current) as Record) + : {}; + const mergedSettings: Settings = { ...currentFileSettings }; + for (const field of modifiedFields) { + const value = snapshotSettings[field]; + if (modifiedNestedFields.has(field) && typeof value === "object" && value !== null) { + const nestedModified = modifiedNestedFields.get(field)!; + const baseNested = (currentFileSettings[field] as Record) ?? {}; + const inMemoryNested = value as Record; + const mergedNested = { ...baseNested }; + for (const nestedKey of nestedModified) { + mergedNested[nestedKey] = inMemoryNested[nestedKey]; + } + (mergedSettings as Record)[field] = mergedNested; + } else { + (mergedSettings as Record)[field] = value; + } + } + + return JSON.stringify(mergedSettings, null, 2); + }); + } + + private save(): void { + this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); + + if (this.globalSettingsLoadError) { + return; + } + + const snapshotGlobalSettings = structuredClone(this.globalSettings); + const modifiedFields = new Set(this.modifiedFields); + const modifiedNestedFields = this.cloneModifiedNestedFields(this.modifiedNestedFields); + + this.enqueueWrite("global", () => { + this.persistScopedSettings("global", snapshotGlobalSettings, modifiedFields, modifiedNestedFields); + }); } private saveProjectSettings(settings: Settings): void { - // In-memory mode: store in memory - if (!this.persist) { - this.inMemoryProjectSettings = structuredClone(settings); + this.projectSettings = structuredClone(settings); + this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); + + if (this.projectSettingsLoadError) { return; } - if (!this.projectSettingsPath) { - return; - } - try { - const dir = dirname(this.projectSettingsPath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - writeFileSync(this.projectSettingsPath, JSON.stringify(settings, null, 2), "utf-8"); - } catch (error) { - console.error(`Warning: Could not save project settings file: ${error}`); - } + const snapshotProjectSettings = structuredClone(this.projectSettings); + const modifiedFields = new Set(this.modifiedProjectFields); + const modifiedNestedFields = this.cloneModifiedNestedFields(this.modifiedProjectNestedFields); + this.enqueueWrite("project", () => { + this.persistScopedSettings("project", snapshotProjectSettings, modifiedFields, modifiedNestedFields); + }); + } + + async flush(): Promise { + await this.writeQueue; + } + + drainErrors(): SettingsError[] { + const drained = [...this.errors]; + this.errors = []; + return drained; } getLastChangelogVersion(): string | undefined { @@ -571,10 +688,10 @@ export class SettingsManager { } setProjectPackages(packages: PackageSource[]): void { - const projectSettings = this.loadProjectSettings(); + const projectSettings = structuredClone(this.projectSettings); projectSettings.packages = packages; + this.markProjectModified("packages"); this.saveProjectSettings(projectSettings); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); } getExtensionPaths(): string[] { @@ -588,10 +705,10 @@ export class SettingsManager { } setProjectExtensionPaths(paths: string[]): void { - const projectSettings = this.loadProjectSettings(); + const projectSettings = structuredClone(this.projectSettings); projectSettings.extensions = paths; + this.markProjectModified("extensions"); this.saveProjectSettings(projectSettings); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); } getSkillPaths(): string[] { @@ -605,10 +722,10 @@ export class SettingsManager { } setProjectSkillPaths(paths: string[]): void { - const projectSettings = this.loadProjectSettings(); + const projectSettings = structuredClone(this.projectSettings); projectSettings.skills = paths; + this.markProjectModified("skills"); this.saveProjectSettings(projectSettings); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); } getPromptTemplatePaths(): string[] { @@ -622,10 +739,10 @@ export class SettingsManager { } setProjectPromptTemplatePaths(paths: string[]): void { - const projectSettings = this.loadProjectSettings(); + const projectSettings = structuredClone(this.projectSettings); projectSettings.prompts = paths; + this.markProjectModified("prompts"); this.saveProjectSettings(projectSettings); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); } getThemePaths(): string[] { @@ -639,10 +756,10 @@ export class SettingsManager { } setProjectThemePaths(paths: string[]): void { - const projectSettings = this.loadProjectSettings(); + const projectSettings = structuredClone(this.projectSettings); projectSettings.themes = paths; + this.markProjectModified("themes"); this.saveProjectSettings(projectSettings); - this.settings = deepMergeSettings(this.globalSettings, projectSettings); } getEnableSkillCommands(): boolean { diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index ecd1242d..c7d79630 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -55,6 +55,16 @@ async function readPipedStdin(): Promise { }); } +function reportSettingsErrors(settingsManager: SettingsManager, context: string): void { + const errors = settingsManager.drainErrors(); + for (const { scope, error } of errors) { + console.error(chalk.yellow(`Warning (${context}, ${scope} settings): ${error.message}`)); + if (error.stack) { + console.error(chalk.dim(error.stack)); + } + } +} + type PackageCommand = "install" | "remove" | "update" | "list"; interface PackageCommandOptions { @@ -200,6 +210,7 @@ async function handlePackageCommand(args: string[]): Promise { const cwd = process.cwd(); const agentDir = getAgentDir(); const settingsManager = SettingsManager.create(cwd, agentDir); + reportSettingsErrors(settingsManager, "package command"); const packageManager = new DefaultPackageManager({ cwd, agentDir, settingsManager }); packageManager.setProgressCallback((event) => { @@ -508,6 +519,7 @@ async function handleConfigCommand(args: string[]): Promise { const cwd = process.cwd(); const agentDir = getAgentDir(); const settingsManager = SettingsManager.create(cwd, agentDir); + reportSettingsErrors(settingsManager, "config command"); const packageManager = new DefaultPackageManager({ cwd, agentDir, settingsManager }); const resolvedPaths = await packageManager.resolve(); @@ -541,6 +553,7 @@ export async function main(args: string[]) { const cwd = process.cwd(); const agentDir = getAgentDir(); const settingsManager = SettingsManager.create(cwd, agentDir); + reportSettingsErrors(settingsManager, "startup"); const authStorage = new AuthStorage(); const modelRegistry = new ModelRegistry(authStorage, getModelsPath()); diff --git a/packages/coding-agent/test/settings-manager-bug.test.ts b/packages/coding-agent/test/settings-manager-bug.test.ts index 84938909..b79fd9f6 100644 --- a/packages/coding-agent/test/settings-manager-bug.test.ts +++ b/packages/coding-agent/test/settings-manager-bug.test.ts @@ -34,7 +34,7 @@ describe("SettingsManager - External Edit Preservation", () => { } }); - it("should preserve file changes to packages array when changing unrelated setting", () => { + it("should preserve file changes to packages array when changing unrelated setting", async () => { const settingsPath = join(agentDir, "settings.json"); // Initial state: packages has one item @@ -62,6 +62,7 @@ describe("SettingsManager - External Edit Preservation", () => { // User changes an UNRELATED setting via UI (this triggers save) manager.setTheme("light"); + await manager.flush(); // With the fix, packages should be preserved as [] (not reverted to startup value) const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); @@ -70,7 +71,7 @@ describe("SettingsManager - External Edit Preservation", () => { expect(savedSettings.theme).toBe("light"); }); - it("should preserve file changes to extensions array when changing unrelated setting", () => { + it("should preserve file changes to extensions array when changing unrelated setting", async () => { const settingsPath = join(agentDir, "settings.json"); writeFileSync( @@ -90,10 +91,57 @@ describe("SettingsManager - External Edit Preservation", () => { // Change unrelated setting manager.setDefaultThinkingLevel("high"); + await manager.flush(); const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); // With the fix, extensions should be preserved (not reverted to startup value) expect(savedSettings.extensions).toEqual(["/new/extension.ts"]); }); + + it("should preserve external project settings changes when updating unrelated project field", async () => { + const projectSettingsPath = join(projectDir, ".pi", "settings.json"); + writeFileSync( + projectSettingsPath, + JSON.stringify({ + extensions: ["./old-extension.ts"], + prompts: ["./old-prompt.md"], + }), + ); + + const manager = SettingsManager.create(projectDir, agentDir); + + const currentProjectSettings = JSON.parse(readFileSync(projectSettingsPath, "utf-8")); + currentProjectSettings.prompts = ["./new-prompt.md"]; + writeFileSync(projectSettingsPath, JSON.stringify(currentProjectSettings, null, 2)); + + manager.setProjectExtensionPaths(["./updated-extension.ts"]); + await manager.flush(); + + const savedProjectSettings = JSON.parse(readFileSync(projectSettingsPath, "utf-8")); + expect(savedProjectSettings.prompts).toEqual(["./new-prompt.md"]); + expect(savedProjectSettings.extensions).toEqual(["./updated-extension.ts"]); + }); + + it("should let in-memory project changes override external changes for the same project field", async () => { + const projectSettingsPath = join(projectDir, ".pi", "settings.json"); + writeFileSync( + projectSettingsPath, + JSON.stringify({ + extensions: ["./initial-extension.ts"], + }), + ); + + const manager = SettingsManager.create(projectDir, agentDir); + + const currentProjectSettings = JSON.parse(readFileSync(projectSettingsPath, "utf-8")); + currentProjectSettings.extensions = ["./external-extension.ts"]; + writeFileSync(projectSettingsPath, JSON.stringify(currentProjectSettings, null, 2)); + + manager.setProjectExtensionPaths(["./in-memory-extension.ts"]); + await manager.flush(); + + const savedProjectSettings = JSON.parse(readFileSync(projectSettingsPath, "utf-8")); + expect(savedProjectSettings.extensions).toEqual(["./in-memory-extension.ts"]); + }); }); diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index eb81806b..df8199e2 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -24,7 +24,7 @@ describe("SettingsManager", () => { }); describe("preserves externally added settings", () => { - it("should preserve enabledModels when changing thinking level", () => { + it("should preserve enabledModels when changing thinking level", async () => { // Create initial settings file const settingsPath = join(agentDir, "settings.json"); writeFileSync( @@ -45,6 +45,7 @@ describe("SettingsManager", () => { // User changes thinking level via Shift+Tab manager.setDefaultThinkingLevel("high"); + await manager.flush(); // Verify enabledModels is preserved const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); @@ -54,7 +55,7 @@ describe("SettingsManager", () => { expect(savedSettings.defaultModel).toBe("claude-sonnet"); }); - it("should preserve custom settings when changing theme", () => { + it("should preserve custom settings when changing theme", async () => { const settingsPath = join(agentDir, "settings.json"); writeFileSync( settingsPath, @@ -73,6 +74,7 @@ describe("SettingsManager", () => { // User changes theme manager.setTheme("light"); + await manager.flush(); // Verify all settings preserved const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); @@ -81,7 +83,7 @@ describe("SettingsManager", () => { expect(savedSettings.theme).toBe("light"); }); - it("should let in-memory changes override file changes for same key", () => { + it("should let in-memory changes override file changes for same key", async () => { const settingsPath = join(agentDir, "settings.json"); writeFileSync( settingsPath, @@ -99,6 +101,7 @@ describe("SettingsManager", () => { // But then changes it via UI to "high" manager.setDefaultThinkingLevel("high"); + await manager.flush(); // In-memory change should win const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); @@ -193,6 +196,22 @@ describe("SettingsManager", () => { }); }); + describe("error tracking", () => { + it("should collect and clear load errors via drainErrors", () => { + const globalSettingsPath = join(agentDir, "settings.json"); + const projectSettingsPath = join(projectDir, ".pi", "settings.json"); + writeFileSync(globalSettingsPath, "{ invalid global json"); + writeFileSync(projectSettingsPath, "{ invalid project json"); + + const manager = SettingsManager.create(projectDir, agentDir); + const errors = manager.drainErrors(); + + expect(errors).toHaveLength(2); + expect(errors.map((e) => e.scope).sort()).toEqual(["global", "project"]); + expect(manager.drainErrors()).toEqual([]); + }); + }); + describe("shellCommandPrefix", () => { it("should load shellCommandPrefix from settings", () => { const settingsPath = join(agentDir, "settings.json"); @@ -212,12 +231,13 @@ describe("SettingsManager", () => { expect(manager.getShellCommandPrefix()).toBeUndefined(); }); - it("should preserve shellCommandPrefix when saving unrelated settings", () => { + it("should preserve shellCommandPrefix when saving unrelated settings", async () => { const settingsPath = join(agentDir, "settings.json"); writeFileSync(settingsPath, JSON.stringify({ shellCommandPrefix: "shopt -s expand_aliases" })); const manager = SettingsManager.create(projectDir, agentDir); manager.setTheme("light"); + await manager.flush(); const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); expect(savedSettings.shellCommandPrefix).toBe("shopt -s expand_aliases");