diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index adeffc40..5e6f5e27 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] + +### Fixed + +- External edits to `settings.json` are now preserved when pi reloads or saves unrelated settings. Previously, editing settings.json directly (e.g., removing a package from `packages` array) would be silently reverted on next pi startup when automatic setters like `setLastChangelogVersion()` triggered a save. ### Added - Added shell-style keybindings: `alt+b`/`alt+f` for word navigation, `ctrl+d` for delete character forward (when editor has text) ([#1043](https://github.com/badlogic/pi-mono/issues/1043) by [@jasonish](https://github.com/jasonish)) diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index 177c3ecd..d68533a0 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -123,6 +123,8 @@ export class SettingsManager { private inMemoryProjectSettings: Settings; // For in-memory mode private settings: Settings; private persist: boolean; + private modifiedFields = new Set(); // Track fields modified during session + private modifiedNestedFields = new Map>(); // Track nested field modifications private constructor( settingsPath: string | null, @@ -231,6 +233,17 @@ export class SettingsManager { this.settings = deepMergeSettings(this.settings, overrides); } + /** Mark a field as modified during this session */ + private markModified(field: keyof Settings, nestedKey?: string): void { + this.modifiedFields.add(field); + if (nestedKey) { + if (!this.modifiedNestedFields.has(field)) { + this.modifiedNestedFields.set(field, new Set()); + } + this.modifiedNestedFields.get(field)!.add(nestedKey); + } + } + private save(): void { if (this.persist && this.settingsPath) { try { @@ -239,13 +252,33 @@ export class SettingsManager { mkdirSync(dir, { recursive: true }); } - // Re-read current file to preserve any settings added externally while running + // Re-read current file to get latest external changes const currentFileSettings = SettingsManager.loadFromFile(this.settingsPath); - // Merge: file settings as base, globalSettings (in-memory changes) as overrides - const mergedSettings = deepMergeSettings(currentFileSettings, this.globalSettings); - this.globalSettings = mergedSettings; - // Save merged settings (project settings are read-only) + // 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) { console.error(`Warning: Could not save settings file: ${error}`); @@ -284,6 +317,7 @@ export class SettingsManager { setLastChangelogVersion(version: string): void { this.globalSettings.lastChangelogVersion = version; + this.markModified("lastChangelogVersion"); this.save(); } @@ -297,17 +331,21 @@ export class SettingsManager { setDefaultProvider(provider: string): void { this.globalSettings.defaultProvider = provider; + this.markModified("defaultProvider"); this.save(); } setDefaultModel(modelId: string): void { this.globalSettings.defaultModel = modelId; + this.markModified("defaultModel"); this.save(); } setDefaultModelAndProvider(provider: string, modelId: string): void { this.globalSettings.defaultProvider = provider; this.globalSettings.defaultModel = modelId; + this.markModified("defaultProvider"); + this.markModified("defaultModel"); this.save(); } @@ -317,6 +355,7 @@ export class SettingsManager { setSteeringMode(mode: "all" | "one-at-a-time"): void { this.globalSettings.steeringMode = mode; + this.markModified("steeringMode"); this.save(); } @@ -326,6 +365,7 @@ export class SettingsManager { setFollowUpMode(mode: "all" | "one-at-a-time"): void { this.globalSettings.followUpMode = mode; + this.markModified("followUpMode"); this.save(); } @@ -335,6 +375,7 @@ export class SettingsManager { setTheme(theme: string): void { this.globalSettings.theme = theme; + this.markModified("theme"); this.save(); } @@ -344,6 +385,7 @@ export class SettingsManager { setDefaultThinkingLevel(level: "off" | "minimal" | "low" | "medium" | "high" | "xhigh"): void { this.globalSettings.defaultThinkingLevel = level; + this.markModified("defaultThinkingLevel"); this.save(); } @@ -356,6 +398,7 @@ export class SettingsManager { this.globalSettings.compaction = {}; } this.globalSettings.compaction.enabled = enabled; + this.markModified("compaction", "enabled"); this.save(); } @@ -390,6 +433,7 @@ export class SettingsManager { this.globalSettings.retry = {}; } this.globalSettings.retry.enabled = enabled; + this.markModified("retry", "enabled"); this.save(); } @@ -407,6 +451,7 @@ export class SettingsManager { setHideThinkingBlock(hide: boolean): void { this.globalSettings.hideThinkingBlock = hide; + this.markModified("hideThinkingBlock"); this.save(); } @@ -416,6 +461,7 @@ export class SettingsManager { setShellPath(path: string | undefined): void { this.globalSettings.shellPath = path; + this.markModified("shellPath"); this.save(); } @@ -425,6 +471,7 @@ export class SettingsManager { setQuietStartup(quiet: boolean): void { this.globalSettings.quietStartup = quiet; + this.markModified("quietStartup"); this.save(); } @@ -434,6 +481,7 @@ export class SettingsManager { setShellCommandPrefix(prefix: string | undefined): void { this.globalSettings.shellCommandPrefix = prefix; + this.markModified("shellCommandPrefix"); this.save(); } @@ -443,6 +491,7 @@ export class SettingsManager { setCollapseChangelog(collapse: boolean): void { this.globalSettings.collapseChangelog = collapse; + this.markModified("collapseChangelog"); this.save(); } @@ -452,6 +501,7 @@ export class SettingsManager { setPackages(packages: PackageSource[]): void { this.globalSettings.packages = packages; + this.markModified("packages"); this.save(); } @@ -468,6 +518,7 @@ export class SettingsManager { setExtensionPaths(paths: string[]): void { this.globalSettings.extensions = paths; + this.markModified("extensions"); this.save(); } @@ -484,6 +535,7 @@ export class SettingsManager { setSkillPaths(paths: string[]): void { this.globalSettings.skills = paths; + this.markModified("skills"); this.save(); } @@ -500,6 +552,7 @@ export class SettingsManager { setPromptTemplatePaths(paths: string[]): void { this.globalSettings.prompts = paths; + this.markModified("prompts"); this.save(); } @@ -516,6 +569,7 @@ export class SettingsManager { setThemePaths(paths: string[]): void { this.globalSettings.themes = paths; + this.markModified("themes"); this.save(); } @@ -532,6 +586,7 @@ export class SettingsManager { setEnableSkillCommands(enabled: boolean): void { this.globalSettings.enableSkillCommands = enabled; + this.markModified("enableSkillCommands"); this.save(); } @@ -548,6 +603,7 @@ export class SettingsManager { this.globalSettings.terminal = {}; } this.globalSettings.terminal.showImages = show; + this.markModified("terminal", "showImages"); this.save(); } @@ -560,6 +616,7 @@ export class SettingsManager { this.globalSettings.images = {}; } this.globalSettings.images.autoResize = enabled; + this.markModified("images", "autoResize"); this.save(); } @@ -572,6 +629,7 @@ export class SettingsManager { this.globalSettings.images = {}; } this.globalSettings.images.blockImages = blocked; + this.markModified("images", "blockImages"); this.save(); } @@ -581,6 +639,7 @@ export class SettingsManager { setEnabledModels(patterns: string[] | undefined): void { this.globalSettings.enabledModels = patterns; + this.markModified("enabledModels"); this.save(); } @@ -590,6 +649,7 @@ export class SettingsManager { setDoubleEscapeAction(action: "fork" | "tree"): void { this.globalSettings.doubleEscapeAction = action; + this.markModified("doubleEscapeAction"); this.save(); } @@ -599,6 +659,7 @@ export class SettingsManager { setShowHardwareCursor(enabled: boolean): void { this.globalSettings.showHardwareCursor = enabled; + this.markModified("showHardwareCursor"); this.save(); } @@ -608,6 +669,7 @@ export class SettingsManager { setEditorPaddingX(padding: number): void { this.globalSettings.editorPaddingX = Math.max(0, Math.min(3, Math.floor(padding))); + this.markModified("editorPaddingX"); this.save(); } diff --git a/packages/coding-agent/test/settings-manager-bug.test.ts b/packages/coding-agent/test/settings-manager-bug.test.ts new file mode 100644 index 00000000..84938909 --- /dev/null +++ b/packages/coding-agent/test/settings-manager-bug.test.ts @@ -0,0 +1,99 @@ +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "fs"; +import { join } from "path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { SettingsManager } from "../src/core/settings-manager.js"; + +/** + * Tests for the fix to a bug where external file changes to arrays were overwritten. + * + * The bug scenario was: + * 1. Pi starts with settings.json containing packages: ["npm:some-pkg"] + * 2. User externally edits file to packages: [] + * 3. User changes an unrelated setting (e.g., theme) via UI + * 4. save() would overwrite packages back to ["npm:some-pkg"] from stale in-memory state + * + * The fix tracks which fields were explicitly modified during the session, and only + * those fields override file values during save(). + */ +describe("SettingsManager - External Edit Preservation", () => { + const testDir = join(process.cwd(), "test-settings-bug-tmp"); + const agentDir = join(testDir, "agent"); + const projectDir = join(testDir, "project"); + + beforeEach(() => { + if (existsSync(testDir)) { + rmSync(testDir, { recursive: true }); + } + mkdirSync(agentDir, { recursive: true }); + mkdirSync(join(projectDir, ".pi"), { recursive: true }); + }); + + afterEach(() => { + if (existsSync(testDir)) { + rmSync(testDir, { recursive: true }); + } + }); + + it("should preserve file changes to packages array when changing unrelated setting", () => { + const settingsPath = join(agentDir, "settings.json"); + + // Initial state: packages has one item + writeFileSync( + settingsPath, + JSON.stringify({ + theme: "dark", + packages: ["npm:pi-mcp-adapter"], + }), + ); + + // Pi starts up, loads settings into memory + const manager = SettingsManager.create(projectDir, agentDir); + + // At this point, globalSettings.packages = ["npm:pi-mcp-adapter"] + expect(manager.getPackages()).toEqual(["npm:pi-mcp-adapter"]); + + // User externally edits settings.json to remove the package + const currentSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); + currentSettings.packages = []; // User wants to remove this! + writeFileSync(settingsPath, JSON.stringify(currentSettings, null, 2)); + + // Verify file was changed + expect(JSON.parse(readFileSync(settingsPath, "utf-8")).packages).toEqual([]); + + // User changes an UNRELATED setting via UI (this triggers save) + manager.setTheme("light"); + + // With the fix, packages should be preserved as [] (not reverted to startup value) + const savedSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); + + expect(savedSettings.packages).toEqual([]); + expect(savedSettings.theme).toBe("light"); + }); + + it("should preserve file changes to extensions array when changing unrelated setting", () => { + const settingsPath = join(agentDir, "settings.json"); + + writeFileSync( + settingsPath, + JSON.stringify({ + theme: "dark", + extensions: ["/old/extension.ts"], + }), + ); + + const manager = SettingsManager.create(projectDir, agentDir); + + // User externally updates extensions + const currentSettings = JSON.parse(readFileSync(settingsPath, "utf-8")); + currentSettings.extensions = ["/new/extension.ts"]; + writeFileSync(settingsPath, JSON.stringify(currentSettings, null, 2)); + + // Change unrelated setting + manager.setDefaultThinkingLevel("high"); + + 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"]); + }); +});