From 816f47540a98093ee6a1393c795c6eec8edb7254 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 19:58:28 +0000 Subject: [PATCH] fix(rpc): preserved explicit caller config across host-defaulted paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-added the isConfigured guard inside applyDefaultSettingOverrides so the runtime override applied at RPC/ACP startup only fills holes — explicit embedder/project/--config/global values now survive for task.isolation.{mode, merge,commits}, task.{eager,batch,maxConcurrency,maxRecursionDepth,disabledAgents, agentModelOverrides}, memory.backend, memories.enabled, advisor.{enabled, subagents,syncBacklog,immuneTurns}, plus the RPC-only async.{enabled,maxJobs} and bash.autoBackground.{enabled,thresholdMs}. The guard was originally added for #2598 and had quietly regressed, so the override re-asserted the schema default and clobbered every explicit value — matching the contract problem #2828 already fixed for the todo paths. Replaced the prior 'default-disables advisor' test (which codified the clobbering as the contract) with a comprehensive 'honors explicit host-defaulted settings' regression covering every contested path across rpc/rpc-ui/acp. Fixes #3207 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/main.ts | 8 ++ .../test/acp-lazy-startup.test.ts | 90 ++++++++++--------- 3 files changed, 61 insertions(+), 41 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1fc767cea..c90bf2d3f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed RPC/ACP startup clobbering explicit caller/project/global configuration for `task.isolation.{mode,merge,commits}`, `task.eager`, `task.batch`, `task.maxConcurrency`, `task.maxRecursionDepth`, `task.disabledAgents`, `task.agentModelOverrides`, `memory.backend`, `memories.enabled`, `advisor.{enabled,subagents,syncBacklog,immuneTurns}`, plus the RPC-only `async.{enabled,maxJobs}` and `bash.autoBackground.{enabled,thresholdMs}`. `applyDefaultSettingOverrides` re-asserted the schema default as a runtime override after settings load, regressing the `isConfigured()` guard added for #2598 and ignoring every explicit value the embedder, project, `--config` overlay, or global config had set. The guard is restored, so the host default now only fills holes ([#3207](https://github.com/can1357/oh-my-pi/issues/3207)). + ## [16.1.11] - 2026-06-21 ### Added diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index 4428e0fcc..e8a09f817 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -149,8 +149,16 @@ const RPC_BACKGROUND_DEFAULTED_SETTING_PATHS: SettingPath[] = [ "bash.autoBackground.thresholdMs", ]; +// Protocol-mode hosts opt into a small set of paths whose host-default we +// re-apply at startup so embedders inherit OMP's neutral defaults instead of +// the local user's globally-persisted preferences for interactive use. The +// guard preserves any explicit configuration — caller `Settings.isolated` +// overrides, project `.claude/settings.yml`, `--config` overlays, or global +// `config.yml` — so the host default only kicks in when nothing is set. Without +// it the override clobbers every caller/host choice (#2598, #3207). function applyDefaultSettingOverrides(settingPaths: SettingPath[], targetSettings: Settings): void { for (const settingPath of settingPaths) { + if (targetSettings.isConfigured(settingPath)) continue; targetSettings.override(settingPath, getDefault(settingPath)); } } diff --git a/packages/coding-agent/test/acp-lazy-startup.test.ts b/packages/coding-agent/test/acp-lazy-startup.test.ts index 0d4cece67..b0e507afe 100644 --- a/packages/coding-agent/test/acp-lazy-startup.test.ts +++ b/packages/coding-agent/test/acp-lazy-startup.test.ts @@ -242,28 +242,57 @@ describe("ACP lazy startup", () => { }); }); - it("default-disables advisor for protocol hosts", async () => { + it("honors explicit host-defaulted settings for protocol hosts", async () => { + // Regression for #3207: in RPC/ACP startup, runtime overrides applied via + // `applyDefaultSettingOverrides` previously clobbered any explicitly + // configured value (caller, project, --config overlay, or global) with the + // schema default. The fix (re-)added an `isConfigured` guard so explicit + // configuration survives, and the schema default only fills holes. const { runRootCommand } = await import("@oh-my-pi/pi-coding-agent/main"); - type ObservedAdvisorSettings = { - enabled: boolean; - subagents: boolean; - syncBacklog: "off" | "1" | "3" | "5"; - immuneTurns: number; - }; + const explicit = { + "task.isolation.mode": "rcopy", + "task.isolation.merge": "branch", + "task.isolation.commits": "ai", + "task.eager": "always", + "task.batch": false, + "task.maxConcurrency": 4, + "task.maxRecursionDepth": 5, + "task.disabledAgents": ["explore"], + "task.agentModelOverrides": { task: "claude-sonnet-4-20250514" }, + "memory.backend": "local", + "memories.enabled": true, + "advisor.enabled": true, + "advisor.subagents": true, + "advisor.syncBacklog": "5", + "advisor.immuneTurns": 7, + } as const; + const rpcOnlyExplicit = { + "async.enabled": false, + "async.maxJobs": 7, + "bash.autoBackground.enabled": true, + "bash.autoBackground.thresholdMs": 5_000, + } as const; + const allPaths = [ + ...(Object.keys(explicit) as (keyof typeof explicit)[]), + ...(Object.keys(rpcOnlyExplicit) as (keyof typeof rpcOnlyExplicit)[]), + ]; + type ObservedSettings = Record; - const runProtocolStartup = async (mode: "rpc" | "rpc-ui" | "acp"): Promise => { - using tempDir = TempDir.createSync("@omp-protocol-advisor-settings-"); + const runProtocolStartup = async (mode: "rpc" | "rpc-ui" | "acp"): Promise => { + using tempDir = TempDir.createSync("@omp-protocol-host-defaulted-"); const cwd = tempDir.path(); const authStorage = await AuthStorage.create(path.join(cwd, "auth.db")); - const settings = Settings.isolated({ - "advisor.enabled": true, - "advisor.subagents": true, - "advisor.syncBacklog": "5", - "advisor.immuneTurns": 3, - }); - let observed: ObservedAdvisorSettings | undefined; - const stopMessage = "stop test protocol mode"; + const settings = Settings.isolated({ ...explicit, ...rpcOnlyExplicit }); + let observed: ObservedSettings | undefined; + const stopMessage = "stop test host-defaulted settings"; + const observe = () => { + observed = {}; + for (const key of allPaths) { + observed[key] = settings.get(key); + } + throw new Error(stopMessage); + }; try { await runRootCommand( @@ -284,24 +313,8 @@ describe("ACP lazy startup", () => { { discoverAuthStorage: async () => authStorage, settings, - createAgentSession: async () => { - observed = { - enabled: settings.get("advisor.enabled"), - subagents: settings.get("advisor.subagents"), - syncBacklog: settings.get("advisor.syncBacklog"), - immuneTurns: settings.get("advisor.immuneTurns"), - }; - throw new Error(stopMessage); - }, - runAcpMode: async () => { - observed = { - enabled: settings.get("advisor.enabled"), - subagents: settings.get("advisor.subagents"), - syncBacklog: settings.get("advisor.syncBacklog"), - immuneTurns: settings.get("advisor.immuneTurns"), - }; - throw new Error(stopMessage); - }, + createAgentSession: async () => observe(), + runAcpMode: async () => observe(), }, ); } catch (error) { @@ -319,12 +332,7 @@ describe("ACP lazy startup", () => { }; for (const mode of ["rpc", "rpc-ui", "acp"] as const) { - await expect(runProtocolStartup(mode)).resolves.toEqual({ - enabled: false, - subagents: false, - syncBacklog: "off", - immuneTurns: 3, - }); + await expect(runProtocolStartup(mode)).resolves.toEqual({ ...explicit, ...rpcOnlyExplicit }); } });