From 0e9f8ad1a18b0cc8988859da246d39acde2f4bce Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 10:40:14 +0200 Subject: [PATCH 1/9] fix(launch): snapshot agent config so direct calls don't inherit the session URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit opper launch was rewriting persistent agent config files (codex, pi, openclaw, opencode) with a per-session URL and never putting them back, so subsequent direct invocations of the agent inherited the previous session's attribution. Adds withConfigSnapshot(path, fn) — captures the file (bytes + mode, or absence) before the spawn and restores it in finally, even on non-zero exits and thrown errors. Each affected adapter wraps its existing rewrite + spawnSync in this helper. Special cases: - openclaw default `gateway start` skips the snapshot: the daemon outlives spawnSync and owns models.json from then on, so restoring would either break a running gateway or be cosmetic. Pass-through args (e.g. `agent --local`) still get the snapshot. - opencode --project uses a narrower revert (baseURL only, captured after configureOpenCode) so a checked-in project config keeps its opper provider — and any custom self-hosted baseURL — across launches. Hermes and Claude Code already weren't leaking (Hermes runs against an isolated HERMES_HOME; Claude Code routes via env vars), so they need no changes. --- src/agents/codex.ts | 30 +++-- src/agents/openclaw.ts | 49 ++++--- src/agents/opencode.ts | 71 +++++++--- src/agents/pi.ts | 33 ++--- src/util/config-snapshot.ts | 62 +++++++++ test/agents/codex.test.ts | 77 ++++++++++- test/agents/openclaw.test.ts | 70 ++++++++-- test/agents/opencode.test.ts | 215 ++++++++++++++++++++++++++++-- test/agents/pi.test.ts | 80 ++++++++--- test/util/config-snapshot.test.ts | 109 +++++++++++++++ 10 files changed, 694 insertions(+), 102 deletions(-) create mode 100644 src/util/config-snapshot.ts create mode 100644 test/util/config-snapshot.test.ts diff --git a/src/agents/codex.ts b/src/agents/codex.ts index 8038849..b6e5ad8 100644 --- a/src/agents/codex.ts +++ b/src/agents/codex.ts @@ -14,6 +14,7 @@ import type { import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { PICKER_MODELS } from "../config/models.js"; +import { withConfigSnapshot } from "../util/config-snapshot.js"; const SENTINEL_OPEN = "# >>> opper-cli >>>"; const SENTINEL_CLOSE = "# <<< opper-cli <<<"; @@ -140,19 +141,22 @@ function hasProfileArg(args: string[]): boolean { } async function spawn(args: string[], routing: OpperRouting): Promise { - // Rewrite our provider/profile block on every launch so the latest - // session URL (and any tags it carries) is the active base_url. - await writeOpperBlock(routing.baseUrl); - - const env: NodeJS.ProcessEnv = { - ...process.env, - OPPER_API_KEY: routing.apiKey, - }; - const finalArgs = hasProfileArg(args) - ? args - : ["--profile", DEFAULT_PROFILE, ...args]; - const result = spawnSync("codex", finalArgs, { stdio: "inherit", env }); - return result.status ?? -1; + // Snapshot config.toml so direct `codex` invocations after the launch + // don't inherit this session's URL. We rewrite the block with the + // session URL for the duration of spawn, then restore on exit. + return withConfigSnapshot(codexConfigPath(), async () => { + await writeOpperBlock(routing.baseUrl); + + const env: NodeJS.ProcessEnv = { + ...process.env, + OPPER_API_KEY: routing.apiKey, + }; + const finalArgs = hasProfileArg(args) + ? args + : ["--profile", DEFAULT_PROFILE, ...args]; + const result = spawnSync("codex", finalArgs, { stdio: "inherit", env }); + return result.status ?? -1; + }); } export const codex: AgentAdapter = { diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index d979726..19a711e 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -9,6 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; +import { withConfigSnapshot } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -118,10 +119,6 @@ async function unconfigure(): Promise { } async function spawn(args: string[], routing: OpperRouting): Promise { - // Ensure our provider is current with the latest credentials, the - // chosen launch model, and the per-session base URL on every spawn. - await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); - // OpenClaw is a gateway/daemon, not an interactive REPL. Default to // `gateway start` — installs/starts the background service via // launchd/systemd, returns quickly, and the gateway keeps serving @@ -133,18 +130,40 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // opper launch openclaw -- agent --local -m "summarise ..." // opper launch openclaw -- gateway run # foreground if you // # really want it - const finalArgs = args.length > 0 ? args : ["gateway", "start"]; - const result = spawnSync("openclaw", finalArgs, { stdio: "inherit" }); - - if (args.length === 0 && result.status === 0) { - console.log( - "\nOpenClaw gateway started in the background.\n" + - " Stop it with: openclaw gateway stop\n" + - " Status: openclaw gateway status\n" + - " Logs: openclaw logs\n", - ); + const isDaemonStart = args.length === 0; + const finalArgs = isDaemonStart ? ["gateway", "start"] : args; + + // Snapshot/restore only fits one-shot synchronous invocations. For the + // daemon path, `spawnSync` returns as soon as the gateway detaches — + // restoring models.json then would either break the running gateway's + // routing (if it re-reads the file) or be cosmetic at best (if it + // cached at startup). Leave the file as-is for that path; the daemon + // owns its lifecycle and the user controls it via `gateway stop/start`. + // + // Trade-off: a direct `openclaw gateway start` run *after* an `opper + // launch openclaw` (without `opper launch` in front) will pick up the + // session-tagged URL until the next `opper launch` or `opper agents + // add openclaw` rewrites it. Acceptable — direct gateway use after a + // launch is rare, and the leak only persists in that narrow window. + if (isDaemonStart) { + await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); + const result = spawnSync("openclaw", finalArgs, { stdio: "inherit" }); + if (result.status === 0) { + console.log( + "\nOpenClaw gateway started in the background.\n" + + " Stop it with: openclaw gateway stop\n" + + " Status: openclaw gateway status\n" + + " Logs: openclaw logs\n", + ); + } + return result.status ?? -1; } - return result.status ?? -1; + + return withConfigSnapshot(modelsPath(), async () => { + await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); + const result = spawnSync("openclaw", finalArgs, { stdio: "inherit" }); + return result.status ?? -1; + }); } export const openclaw: AgentAdapter = { diff --git a/src/agents/opencode.ts b/src/agents/opencode.ts index 60fb3ad..5730373 100644 --- a/src/agents/opencode.ts +++ b/src/agents/opencode.ts @@ -7,7 +7,9 @@ import { configureOpenCode, readProjectConfigState, } from "../setup/opencode.js"; +import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { opencodeConfigPath } from "../util/editor-paths.js"; +import { withConfigSnapshot } from "../util/config-snapshot.js"; import { brand } from "../ui/colors.js"; import type { AgentAdapter, @@ -101,20 +103,61 @@ async function setSessionBaseUrl( await writeFile(cfg, JSON.stringify(parsed, null, 2), "utf8"); } +/** + * Read `provider.opper.options.baseURL` from an opencode config without + * mutating the file. Returns undefined when the file or the provider is + * absent, or when the JSON is malformed — callers fall back to a default. + */ +function readBaseUrl(location: "global" | "local"): string | undefined { + const cfg = opencodeConfigPath(location); + if (!existsSync(cfg)) return undefined; + try { + const parsed = JSON.parse(readFileSync(cfg, "utf8")) as { + provider?: { opper?: { options?: { baseURL?: unknown } } }; + }; + const url = parsed.provider?.opper?.options?.baseURL; + return typeof url === "string" ? url : undefined; + } catch { + return undefined; + } +} + async function spawn( args: string[], routing: OpperRouting, opts: SpawnOptions = {}, ): Promise { const scope = opts.configScope ?? "user"; - const location = scope === "project" ? "local" : "global"; if (scope === "project") { - // Explicit opt-in to writing the cwd-local config. We never silently - // mutate a project config the user didn't ask us to touch — that file - // is usually checked in. + // `--project` is opt-in to a persistent, usually-checked-in project + // config. Reverting the whole opper provider on exit would defeat + // the point — instead we apply the session URL only for the spawn + // and reset baseURL afterwards. The opper provider block stays in + // place across launches. await configureOpenCode({ location: "local", overwrite: true }); - } else { + // Capture *after* configureOpenCode so a fresh project picks up the + // template's compat URL, while a hand-edited config (e.g. pointing + // at a self-hosted Opper) keeps that custom URL across launches. + const restoreUrl = readBaseUrl("local") ?? OPPER_COMPAT_URL; + await setSessionBaseUrl(routing.baseUrl, "local"); + try { + const env: NodeJS.ProcessEnv = { + ...process.env, + OPPER_API_KEY: routing.apiKey, + }; + const result = spawnSync("opencode", args, { stdio: "inherit", env }); + return result.status ?? -1; + } finally { + await setSessionBaseUrl(restoreUrl, "local"); + } + } + + // User-scope: snapshot the global config so direct `opencode` + // invocations after the launch don't inherit this session's URL, + // and so a launch on a machine with no prior opencode.json doesn't + // leave one behind. + return withConfigSnapshot(opencodeConfigPath("global"), async () => { await configureOpenCode({ location: "global", overwrite: true }); // OpenCode reads `./opencode.json` if present and uses it instead of @@ -130,18 +173,16 @@ async function spawn( ), ); } - } - // Rewrite the baseURL to the per-session URL on every launch so - // generations land on the right session. - await setSessionBaseUrl(routing.baseUrl, location); + await setSessionBaseUrl(routing.baseUrl, "global"); - const env: NodeJS.ProcessEnv = { - ...process.env, - OPPER_API_KEY: routing.apiKey, - }; - const result = spawnSync("opencode", args, { stdio: "inherit", env }); - return result.status ?? -1; + const env: NodeJS.ProcessEnv = { + ...process.env, + OPPER_API_KEY: routing.apiKey, + }; + const result = spawnSync("opencode", args, { stdio: "inherit", env }); + return result.status ?? -1; + }); } export const opencode: AgentAdapter = { diff --git a/src/agents/pi.ts b/src/agents/pi.ts index ae0e692..47f3680 100644 --- a/src/agents/pi.ts +++ b/src/agents/pi.ts @@ -9,6 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; +import { withConfigSnapshot } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -120,21 +121,23 @@ async function unconfigure(): Promise { } async function spawn(args: string[], routing: OpperRouting): Promise { - // Re-write the provider on every launch so the latest credentials, the - // chosen launch model, and the per-session base URL are always active. - await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); - - // pi's CLI requires *both* --provider and --model to resolve a non-default - // provider — passing only --provider falls through to the auto-resolver - // and silently picks the first available provider (usually ollama). - const userPicked = args.some( - (a) => a === "--model" || a === "-m" || a.startsWith("--model="), - ); - const piArgs = userPicked - ? ["--provider", PROVIDER_KEY, ...args] - : ["--provider", PROVIDER_KEY, "--model", routing.model, ...args]; - const result = spawnSync("pi", piArgs, { stdio: "inherit" }); - return result.status ?? -1; + // Snapshot models.json so direct `pi` invocations after the launch + // don't inherit this session's URL. + return withConfigSnapshot(piConfigPath(), async () => { + await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); + + // pi's CLI requires *both* --provider and --model to resolve a non-default + // provider — passing only --provider falls through to the auto-resolver + // and silently picks the first available provider (usually ollama). + const userPicked = args.some( + (a) => a === "--model" || a === "-m" || a.startsWith("--model="), + ); + const piArgs = userPicked + ? ["--provider", PROVIDER_KEY, ...args] + : ["--provider", PROVIDER_KEY, "--model", routing.model, ...args]; + const result = spawnSync("pi", piArgs, { stdio: "inherit" }); + return result.status ?? -1; + }); } export const pi: AgentAdapter = { diff --git a/src/util/config-snapshot.ts b/src/util/config-snapshot.ts new file mode 100644 index 0000000..abd85c6 --- /dev/null +++ b/src/util/config-snapshot.ts @@ -0,0 +1,62 @@ +import { dirname } from "node:path"; +import { existsSync, readFileSync, statSync } from "node:fs"; +import { chmod, mkdir, rm, writeFile } from "node:fs/promises"; + +/** + * Captures the contents (or absence) of `path`, runs `fn`, and restores + * the file to its pre-`fn` state in a finally block — including when the + * file didn't exist before (in which case "restore" means deleting it). + * + * Used by adapters that bake a per-launch session URL into a persistent + * config file: snapshot ensures direct invocations of the agent (without + * `opper launch`) don't inherit the previous session's URL. + * + * Restore failures are written to stderr but do not mask `fn`'s own error + * — the caller's error is always more informative than a follow-up I/O + * failure. + */ +export async function withConfigSnapshot( + path: string, + fn: () => Promise, +): Promise { + const snapshot = capture(path); + try { + return await fn(); + } finally { + await restore(path, snapshot); + } +} + +interface Snapshot { + existed: boolean; + bytes?: Buffer; + mode?: number; +} + +function capture(path: string): Snapshot { + if (!existsSync(path)) return { existed: false }; + const bytes = readFileSync(path); + const mode = statSync(path).mode & 0o777; + return { existed: true, bytes, mode }; +} + +async function restore(path: string, snap: Snapshot): Promise { + try { + if (!snap.existed) { + await rm(path, { force: true }); + return; + } + await mkdir(dirname(path), { recursive: true }); + // `writeFile`'s `mode` only applies when the file is being created. + // If the agent rewrote the file in place (still exists), we have to + // chmod explicitly to actually restore the original permissions. + await writeFile(path, snap.bytes!, { mode: snap.mode }); + if (snap.mode !== undefined) await chmod(path, snap.mode); + } catch (err) { + process.stderr.write( + `opper: failed to restore ${path} after launch: ${ + err instanceof Error ? err.message : String(err) + }\n`, + ); + } +} diff --git a/test/agents/codex.test.ts b/test/agents/codex.test.ts index 7f25d1b..db093f7 100644 --- a/test/agents/codex.test.ts +++ b/test/agents/codex.test.ts @@ -163,7 +163,15 @@ describe("codex adapter", () => { }); it("spawn injects OPPER_API_KEY and prepends --profile opper-opus when no profile is set", async () => { - spawnSyncMock.mockReturnValue({ status: 0 }); + // The session URL from routing.baseUrl is written into the config.toml + // managed block during spawn — that's how Codex picks up the per-launch + // session. Capture the file mid-run since we restore on exit. + const cfgPath = join(sandbox, ".codex", "config.toml"); + let midRunCfg = ""; + spawnSyncMock.mockImplementation(() => { + midRunCfg = readFileSync(cfgPath, "utf8"); + return { status: 0 }; + }); const code = await codex.spawn!(["chat"], { baseUrl: SESSION_URL, apiKey: "op_live_run", @@ -178,12 +186,8 @@ describe("codex adapter", () => { const init = call[2] as { env: NodeJS.ProcessEnv }; expect(init.env.OPPER_API_KEY).toBe("op_live_run"); - // The session URL from routing.baseUrl is written into the config.toml - // managed block — that's how Codex picks up the per-launch session. - const cfgPath = join(sandbox, ".codex", "config.toml"); - const text = readFileSync(cfgPath, "utf8"); - expect(text).toContain(`base_url = "${SESSION_URL}"`); - expect(text).not.toContain('base_url = "https://api.opper.ai/v3/compat"'); + expect(midRunCfg).toContain(`base_url = "${SESSION_URL}"`); + expect(midRunCfg).not.toContain('base_url = "https://api.opper.ai/v3/compat"'); }); it("spawn does not add --profile when the user already passed one", async () => { @@ -208,4 +212,63 @@ describe("codex adapter", () => { }); expect(code).toBe(2); }); + + it("spawn restores the pre-launch config so direct `codex` runs don't inherit the session URL", async () => { + // User has run `opper agents add codex` previously — config has the + // default compat URL baked in. When `opper launch` runs we rewrite + // base_url to the session URL during the run, but afterwards it must + // be back to what the user had. + await codex.configure({}); + const cfgPath = join(sandbox, ".codex", "config.toml"); + const before = readFileSync(cfgPath, "utf8"); + expect(before).toContain('base_url = "https://api.opper.ai/v3/compat"'); + + spawnSyncMock.mockImplementation(() => { + // Mid-run the session URL is active — that's how Codex picks it up. + const mid = readFileSync(cfgPath, "utf8"); + expect(mid).toContain(`base_url = "${SESSION_URL}"`); + return { status: 0 }; + }); + await codex.spawn!([], { + baseUrl: SESSION_URL, + apiKey: "k", + model: "m", + compatShape: "openai", + }); + + expect(readFileSync(cfgPath, "utf8")).toBe(before); + }); + + it("spawn deletes the config it created when none existed before", async () => { + // User never configured codex through opper. `opper launch codex` + // shouldn't leave a config file behind. + const cfgPath = join(sandbox, ".codex", "config.toml"); + expect(existsSync(cfgPath)).toBe(false); + + spawnSyncMock.mockReturnValue({ status: 0 }); + await codex.spawn!([], { + baseUrl: SESSION_URL, + apiKey: "k", + model: "m", + compatShape: "openai", + }); + + expect(existsSync(cfgPath)).toBe(false); + }); + + it("spawn restores the pre-launch config even if the agent exits non-zero", async () => { + await codex.configure({}); + const cfgPath = join(sandbox, ".codex", "config.toml"); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 17 }); + const code = await codex.spawn!([], { + baseUrl: SESSION_URL, + apiKey: "k", + model: "m", + compatShape: "openai", + }); + expect(code).toBe(17); + expect(readFileSync(cfgPath, "utf8")).toBe(before); + }); }); diff --git a/test/agents/openclaw.test.ts b/test/agents/openclaw.test.ts index 873f2f4..c2f5ef7 100644 --- a/test/agents/openclaw.test.ts +++ b/test/agents/openclaw.test.ts @@ -77,22 +77,61 @@ describe("openclaw adapter", () => { expect(models.providers?.opper?.apiKey).toBe("op_live_test"); }); - it("spawn writes routing.baseUrl (the session URL) into models.json before launching", async () => { - spawnSyncMock.mockReturnValue({ status: 0 }); + it("spawn writes routing.baseUrl (the session URL) into models.json mid-launch", async () => { + // Mid-spawn the session URL is the active baseUrl. We capture inside + // the spawn callback because we restore the file on exit. + const cfgPath = join( + sandbox, ".openclaw", "agents", "main", "agent", "models.json", + ); + let midRun: ReturnType | undefined; + spawnSyncMock.mockImplementation(() => { + midRun = JSON.parse(readFileSync(cfgPath, "utf8")); + return { status: 0 }; + }); const code = await openclaw.spawn!(["agent"], ROUTING); expect(code).toBe(0); - const models = readModels(sandbox); - expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); - expect(models.providers?.opper?.apiKey).toBe("op_live_run"); + expect(midRun?.providers?.opper?.baseUrl).toBe(SESSION_URL); + expect(midRun?.providers?.opper?.apiKey).toBe("op_live_run"); + }); - // The default compat URL should NOT have leaked into the file. - const raw = readFileSync( - join(sandbox, ".openclaw", "agents", "main", "agent", "models.json"), - "utf8", + it("spawn restores the pre-launch config so direct `openclaw` runs don't inherit the session URL", async () => { + await openclaw.configure({ apiKey: "op_user_key" }); + const cfgPath = join( + sandbox, ".openclaw", "agents", "main", "agent", "models.json", ); - expect(raw).not.toContain('"baseUrl": "https://api.opper.ai/v3/compat"'); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["agent"], ROUTING); + + expect(readFileSync(cfgPath, "utf8")).toBe(before); + }); + + it("spawn deletes the config it created when none existed before", async () => { + const cfgPath = join( + sandbox, ".openclaw", "agents", "main", "agent", "models.json", + ); + expect(existsSync(cfgPath)).toBe(false); + + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["agent"], ROUTING); + + expect(existsSync(cfgPath)).toBe(false); + }); + + it("spawn restores the pre-launch config even on non-zero exit", async () => { + await openclaw.configure({ apiKey: "op_user_key" }); + const cfgPath = join( + sandbox, ".openclaw", "agents", "main", "agent", "models.json", + ); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 17 }); + const code = await openclaw.spawn!(["agent"], ROUTING); + expect(code).toBe(17); + expect(readFileSync(cfgPath, "utf8")).toBe(before); }); it("spawn places the launch model at models[0] even when it isn't opus", async () => { @@ -117,6 +156,17 @@ describe("openclaw adapter", () => { expect(call[1]).toEqual(["gateway", "start"]); }); + it("spawn does NOT snapshot the config on the daemon path — the gateway daemon outlives spawnSync and owns the file from then on", async () => { + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!([], ROUTING); + // Post-spawn, models.json keeps the session URL: the daemon is + // running, will keep using whatever URL it loaded, and the file + // mirrors that. Restoring would either break the live daemon or + // be cosmetic — neither is correct. + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + it("spawn forwards user-supplied args verbatim", async () => { spawnSyncMock.mockReturnValue({ status: 0 }); await openclaw.spawn!(["agent", "--local"], ROUTING); diff --git a/test/agents/opencode.test.ts b/test/agents/opencode.test.ts index 30a65b5..9eb3040 100644 --- a/test/agents/opencode.test.ts +++ b/test/agents/opencode.test.ts @@ -4,6 +4,7 @@ import { rmSync, writeFileSync, readFileSync, + existsSync, mkdirSync, } from "node:fs"; import { tmpdir } from "node:os"; @@ -154,25 +155,88 @@ describe("opencode adapter", () => { expect(init.env.OPPER_API_KEY).toBe("op_live_run"); }); - it("spawn rewrites provider.opper.options.baseURL to routing.baseUrl on every launch", async () => { + it("spawn rewrites provider.opper.options.baseURL to routing.baseUrl mid-launch", async () => { configureOpenCodeMock.mockResolvedValue({ path: opencodeConfigPath(sandbox), wrote: false, }); - spawnSyncMock.mockReturnValue({ status: 0 }); seedOpencodeConfig(sandbox); + // Mid-spawn the session URL is the active baseURL — we capture + // inside the spawn callback because we restore on exit. + let midRun: { + provider: { opper: { options: { baseURL: string; apiKey: string } } }; + } | undefined; + spawnSyncMock.mockImplementation(() => { + midRun = JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8")); + return { status: 0 }; + }); + await opencode.spawn!([], ROUTING); - const cfg = JSON.parse( - readFileSync(opencodeConfigPath(sandbox), "utf8"), - ) as { - provider: { opper: { options: { baseURL: string; apiKey: string } } }; - }; - expect(cfg.provider.opper.options.baseURL).toBe(SESSION_URL); + expect(midRun?.provider.opper.options.baseURL).toBe(SESSION_URL); // The {env:OPPER_API_KEY} placeholder is preserved — we only rewrote // baseURL. - expect(cfg.provider.opper.options.apiKey).toBe("{env:OPPER_API_KEY}"); + expect(midRun?.provider.opper.options.apiKey).toBe("{env:OPPER_API_KEY}"); + }); + + it("spawn restores the pre-launch opencode.json so direct `opencode` runs don't inherit the session URL", async () => { + configureOpenCodeMock.mockResolvedValue({ + path: opencodeConfigPath(sandbox), + wrote: false, + }); + spawnSyncMock.mockReturnValue({ status: 0 }); + seedOpencodeConfig(sandbox); + const before = readFileSync(opencodeConfigPath(sandbox), "utf8"); + + await opencode.spawn!([], ROUTING); + + expect(readFileSync(opencodeConfigPath(sandbox), "utf8")).toBe(before); + }); + + it("spawn deletes any opencode.json it caused to be created when none existed before", async () => { + // Simulate `configureOpenCode` writing the template config from + // scratch — the real implementation does this on first launch. + const cfg = opencodeConfigPath(sandbox); + expect(existsSync(cfg)).toBe(false); + configureOpenCodeMock.mockImplementation(async () => { + mkdirSync(join(sandbox, ".config", "opencode"), { recursive: true }); + writeFileSync( + cfg, + JSON.stringify({ + provider: { + opper: { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: "https://api.opper.ai/v3/compat", + apiKey: "{env:OPPER_API_KEY}", + }, + }, + }, + }), + "utf8", + ); + return { path: cfg, wrote: true }; + }); + spawnSyncMock.mockReturnValue({ status: 0 }); + + await opencode.spawn!([], ROUTING); + + expect(existsSync(cfg)).toBe(false); + }); + + it("spawn restores the pre-launch config even on non-zero exit", async () => { + configureOpenCodeMock.mockResolvedValue({ + path: opencodeConfigPath(sandbox), + wrote: false, + }); + seedOpencodeConfig(sandbox); + const before = readFileSync(opencodeConfigPath(sandbox), "utf8"); + + spawnSyncMock.mockReturnValue({ status: 17 }); + const code = await opencode.spawn!([], ROUTING); + expect(code).toBe(17); + expect(readFileSync(opencodeConfigPath(sandbox), "utf8")).toBe(before); }); it("spawn does not crash if the opencode.json doesn't exist yet (configureOpenCode was a no-op stub)", async () => { @@ -205,6 +269,139 @@ describe("opencode adapter", () => { expect(configureOpenCodeMock).toHaveBeenCalledWith({ location: "local", overwrite: true }); }); + it("project scope: opper provider persists across launches but baseURL is reset to compat", async () => { + // `--project` is opt-in to a checked-in config — the opper provider + // should stay. Only the session URL should not leak. + const prevCwd = process.cwd(); + process.chdir(sandbox); + try { + const projectCfg = join(sandbox, "opencode.json"); + configureOpenCodeMock.mockImplementation(async () => { + writeFileSync( + projectCfg, + JSON.stringify({ + provider: { + opper: { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: "https://api.opper.ai/v3/compat", + apiKey: "{env:OPPER_API_KEY}", + }, + }, + }, + }, null, 2), + "utf8", + ); + return { path: projectCfg, wrote: true }; + }); + + let midRunBaseUrl: string | undefined; + spawnSyncMock.mockImplementation(() => { + midRunBaseUrl = ( + JSON.parse(readFileSync(projectCfg, "utf8")) as { + provider: { opper: { options: { baseURL: string } } }; + } + ).provider.opper.options.baseURL; + return { status: 0 }; + }); + + await opencode.spawn!([], ROUTING, { configScope: "project" }); + expect(midRunBaseUrl).toBe(SESSION_URL); + + // Post-spawn: opper provider still there, baseURL reset to compat. + const after = JSON.parse(readFileSync(projectCfg, "utf8")) as { + provider: { opper: { options: { baseURL: string } } }; + }; + expect(after.provider.opper.options.baseURL).toBe( + "https://api.opper.ai/v3/compat", + ); + } finally { + process.chdir(prevCwd); + } + }); + + it("project scope: a hand-edited custom baseURL is preserved across launches", async () => { + // Self-hosted Opper / staging users may pin a custom baseURL in + // their checked-in opencode.json. The launch should swap to the + // session URL during spawn and restore *that* custom URL on exit, + // not blindly reset to the public compat URL. + const prevCwd = process.cwd(); + process.chdir(sandbox); + try { + const projectCfg = join(sandbox, "opencode.json"); + const customBaseUrl = "https://opper.internal.example.com/v3/compat"; + writeFileSync( + projectCfg, + JSON.stringify({ + provider: { + opper: { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: customBaseUrl, + apiKey: "{env:OPPER_API_KEY}", + }, + }, + }, + }, null, 2), + "utf8", + ); + // configureOpenCode is a no-op when an opper provider already + // exists (see src/setup/opencode.ts:80-82). + configureOpenCodeMock.mockResolvedValue({ + path: projectCfg, wrote: false, reason: "exists" as const, + }); + spawnSyncMock.mockReturnValue({ status: 0 }); + + await opencode.spawn!([], ROUTING, { configScope: "project" }); + + const after = JSON.parse(readFileSync(projectCfg, "utf8")) as { + provider: { opper: { options: { baseURL: string } } }; + }; + expect(after.provider.opper.options.baseURL).toBe(customBaseUrl); + } finally { + process.chdir(prevCwd); + } + }); + + it("project scope: baseURL is reset even when the agent exits non-zero", async () => { + const prevCwd = process.cwd(); + process.chdir(sandbox); + try { + const projectCfg = join(sandbox, "opencode.json"); + configureOpenCodeMock.mockImplementation(async () => { + writeFileSync( + projectCfg, + JSON.stringify({ + provider: { + opper: { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: "https://api.opper.ai/v3/compat", + apiKey: "{env:OPPER_API_KEY}", + }, + }, + }, + }, null, 2), + "utf8", + ); + return { path: projectCfg, wrote: true }; + }); + spawnSyncMock.mockReturnValue({ status: 17 }); + + const code = await opencode.spawn!([], ROUTING, { configScope: "project" }); + expect(code).toBe(17); + + const after = JSON.parse(readFileSync(projectCfg, "utf8")) as { + provider: { opper: { options: { baseURL: string } } }; + }; + expect(after.provider.opper.options.baseURL).toBe( + "https://api.opper.ai/v3/compat", + ); + } finally { + process.chdir(prevCwd); + } + }); + it("spawn warns when a project opencode.json exists without an Opper provider", async () => { configureOpenCodeMock.mockResolvedValue({ path: "/tmp/g", wrote: true }); spawnSyncMock.mockReturnValue({ status: 0 }); diff --git a/test/agents/pi.test.ts b/test/agents/pi.test.ts index bdc0404..f4650db 100644 --- a/test/agents/pi.test.ts +++ b/test/agents/pi.test.ts @@ -76,31 +76,72 @@ describe("pi adapter", () => { expect(models.providers?.opper?.apiKey).toBe("op_live_test"); }); - it("spawn writes routing.baseUrl (the session URL) into models.json before launching", async () => { - spawnSyncMock.mockReturnValue({ status: 0 }); + it("spawn writes routing.baseUrl (the session URL) into models.json mid-launch", async () => { + // Mid-spawn the session URL is the active baseUrl — that's how Pi + // picks it up. We capture inside the spawn callback because we + // restore the file on exit. + const cfgPath = join(sandbox, ".pi", "agent", "models.json"); + let midRun: ReturnType | undefined; + spawnSyncMock.mockImplementation(() => { + midRun = JSON.parse(readFileSync(cfgPath, "utf8")); + return { status: 0 }; + }); const code = await pi.spawn!(["chat"], ROUTING); expect(code).toBe(0); - const models = readModels(sandbox); - expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); - expect(models.providers?.opper?.apiKey).toBe("op_live_run"); + expect(midRun?.providers?.opper?.baseUrl).toBe(SESSION_URL); + expect(midRun?.providers?.opper?.apiKey).toBe("op_live_run"); + }); - const raw = readFileSync( - join(sandbox, ".pi", "agent", "models.json"), - "utf8", - ); - expect(raw).not.toContain('"baseUrl": "https://api.opper.ai/v3/compat"'); + it("spawn restores the pre-launch config so direct `pi` runs don't inherit the session URL", async () => { + // User has run `opper agents add pi` previously — config has the + // default compat URL baked in. After `opper launch pi` exits, that + // file must be back to what the user had. + await pi.configure({ apiKey: "op_user_key" }); + const cfgPath = join(sandbox, ".pi", "agent", "models.json"); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 0 }); + await pi.spawn!([], ROUTING); + + expect(readFileSync(cfgPath, "utf8")).toBe(before); }); - it("spawn places the launch model at models[0] even when it isn't opus", async () => { + it("spawn deletes the config it created when none existed before", async () => { + const cfgPath = join(sandbox, ".pi", "agent", "models.json"); + expect(existsSync(cfgPath)).toBe(false); + spawnSyncMock.mockReturnValue({ status: 0 }); + await pi.spawn!([], ROUTING); + + expect(existsSync(cfgPath)).toBe(false); + }); + + it("spawn restores the pre-launch config even on non-zero exit", async () => { + await pi.configure({ apiKey: "op_user_key" }); + const cfgPath = join(sandbox, ".pi", "agent", "models.json"); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 17 }); + const code = await pi.spawn!([], ROUTING); + expect(code).toBe(17); + expect(readFileSync(cfgPath, "utf8")).toBe(before); + }); + + it("spawn places the launch model at models[0] even when it isn't opus", async () => { + // Read mid-spawn — snapshot/restore wipes the config on exit when + // there was no pre-launch config. + let midRun: ReturnType | undefined; + spawnSyncMock.mockImplementation(() => { + midRun = readModels(sandbox); + return { status: 0 }; + }); await pi.spawn!([], { ...ROUTING, model: "claude-haiku-4-5" }); - const models = readModels(sandbox) as { + const list = (midRun as { providers?: { opper?: { models?: Array<{ id: string; _launch?: boolean }> } }; - }; - const list = models.providers?.opper?.models ?? []; + } | undefined)?.providers?.opper?.models ?? []; expect(list[0]?.id).toBe("claude-haiku-4-5"); expect(list[0]?._launch).toBe(true); // Other curated models still present after the launch entry. @@ -109,13 +150,16 @@ describe("pi adapter", () => { }); it("spawn prepends a non-curated --model id so it still appears in the picker", async () => { - spawnSyncMock.mockReturnValue({ status: 0 }); + let midRun: ReturnType | undefined; + spawnSyncMock.mockImplementation(() => { + midRun = readModels(sandbox); + return { status: 0 }; + }); await pi.spawn!([], { ...ROUTING, model: "deepinfra/some-future-model" }); - const models = readModels(sandbox) as { + const list = (midRun as { providers?: { opper?: { models?: Array<{ id: string; _launch?: boolean }> } }; - }; - const list = models.providers?.opper?.models ?? []; + } | undefined)?.providers?.opper?.models ?? []; expect(list[0]?.id).toBe("deepinfra/some-future-model"); expect(list[0]?._launch).toBe(true); }); diff --git a/test/util/config-snapshot.test.ts b/test/util/config-snapshot.test.ts new file mode 100644 index 0000000..8d07aec --- /dev/null +++ b/test/util/config-snapshot.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + mkdtempSync, + rmSync, + writeFileSync, + readFileSync, + existsSync, + statSync, + chmodSync, + mkdirSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { withConfigSnapshot } from "../../src/util/config-snapshot.js"; + +describe("withConfigSnapshot", () => { + let sandbox: string; + + beforeEach(() => { + sandbox = mkdtempSync(join(tmpdir(), "opper-snap-")); + }); + afterEach(() => { + rmSync(sandbox, { recursive: true, force: true }); + }); + + it("restores the original bytes when the file pre-existed", async () => { + const path = join(sandbox, "config.toml"); + writeFileSync(path, "original\n", "utf8"); + + const result = await withConfigSnapshot(path, async () => { + writeFileSync(path, "mutated\n", "utf8"); + return 42; + }); + + expect(result).toBe(42); + expect(readFileSync(path, "utf8")).toBe("original\n"); + }); + + it("removes the file if it did not exist before", async () => { + const path = join(sandbox, "models.json"); + expect(existsSync(path)).toBe(false); + + await withConfigSnapshot(path, async () => { + writeFileSync(path, "{}\n", "utf8"); + expect(existsSync(path)).toBe(true); + }); + + expect(existsSync(path)).toBe(false); + }); + + it("restores even when fn throws, and rethrows the error", async () => { + const path = join(sandbox, "config.toml"); + writeFileSync(path, "before\n", "utf8"); + + await expect( + withConfigSnapshot(path, async () => { + writeFileSync(path, "during\n", "utf8"); + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + + expect(readFileSync(path, "utf8")).toBe("before\n"); + }); + + it("removes a created file even when fn throws", async () => { + const path = join(sandbox, "models.json"); + + await expect( + withConfigSnapshot(path, async () => { + writeFileSync(path, "{}\n", "utf8"); + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + + expect(existsSync(path)).toBe(false); + }); + + it("preserves the original file mode", async () => { + const path = join(sandbox, "secret.json"); + writeFileSync(path, "{}\n", { mode: 0o600 }); + expect(statSync(path).mode & 0o777).toBe(0o600); + + await withConfigSnapshot(path, async () => { + // Use chmodSync to actually loosen the mode — `writeFileSync`'s + // `mode` option is silently ignored when the file already exists. + writeFileSync(path, '{"x":1}\n', "utf8"); + chmodSync(path, 0o644); + expect(statSync(path).mode & 0o777).toBe(0o644); + }); + + expect(readFileSync(path, "utf8")).toBe("{}\n"); + expect(statSync(path).mode & 0o777).toBe(0o600); + }); + + it("works when the parent directory has to be recreated to restore", async () => { + // Simulates an adapter that nukes the directory mid-launch. Restore + // should still put the original bytes back at the original path. + const dir = join(sandbox, "nested", "deep"); + mkdirSync(dir, { recursive: true }); + const path = join(dir, "config.toml"); + writeFileSync(path, "keep me\n", "utf8"); + + await withConfigSnapshot(path, async () => { + rmSync(dir, { recursive: true, force: true }); + }); + + expect(readFileSync(path, "utf8")).toBe("keep me\n"); + }); +}); From 06191d034960c8fc8598370e9e4f5cfe9caa98bb Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 10:40:21 +0200 Subject: [PATCH 2/9] refactor(agents): rename `agents uninstall` to `agents remove` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration command never uninstalled the agent binary itself — it only removed Opper-owned config. "remove" matches the pairing with the implicit "add" that happens on first launch / configure, and avoids the suggestion that the agent gets uninstalled. Renames both the CLI subcommand and the menu option, plus the internal command function (agentsUninstallCommand → agentsRemoveCommand). README updated. Skills `uninstall` is left alone since that one really does remove files from disk. --- README.md | 4 ++-- src/cli/agents.ts | 8 ++++---- src/commands/agents.ts | 2 +- src/commands/menu/agents.ts | 4 ++-- test/commands/agents.test.ts | 10 +++++----- test/commands/menu.test.ts | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 7ac18e9..2eafd1a 100644 --- a/README.md +++ b/README.md @@ -87,10 +87,10 @@ The CLI also offers a per-agent submenu (`opper` → Agents → *agent* → Laun To remove an agent's Opper integration without uninstalling the agent itself: ```bash -opper agents uninstall claude-desktop # works for any registered adapter +opper agents remove claude-desktop # works for any registered adapter ``` -This is the non-interactive equivalent of the menu's "Uninstall" action. It clears Opper-owned config (e.g., flips Claude Desktop's `deploymentMode` back to `"1p"`, removes the `opper` provider block from OpenCode / Pi / OpenClaw, etc.) without touching anything you put there yourself. +This is the non-interactive equivalent of the menu's "Remove Opper integration" action. It clears Opper-owned config (e.g., flips Claude Desktop's `deploymentMode` back to `"1p"`, removes the `opper` provider block from OpenCode / Pi / OpenClaw, etc.) without touching anything you put there yourself. ## Ask — built-in support agent diff --git a/src/cli/agents.ts b/src/cli/agents.ts index 21bde46..a944836 100644 --- a/src/cli/agents.ts +++ b/src/cli/agents.ts @@ -1,4 +1,4 @@ -import { agentsListCommand, agentsUninstallCommand } from "../commands/agents.js"; +import { agentsListCommand, agentsRemoveCommand } from "../commands/agents.js"; import { launchCommand } from "../commands/launch.js"; import type { RegisterFn } from "./types.js"; @@ -40,12 +40,12 @@ const register: RegisterFn = (program, ctx) => { .action(agentsListCommand); agentsCmd - .command("uninstall ") + .command("remove ") .description( - "Remove the Opper integration from an agent's config (does not uninstall the agent itself)", + "Remove the Opper integration from an agent's config (the agent binary stays installed)", ) .action(async (name: string) => { - await agentsUninstallCommand(name); + await agentsRemoveCommand(name); }); program diff --git a/src/commands/agents.ts b/src/commands/agents.ts index 5920d39..4f250ee 100644 --- a/src/commands/agents.ts +++ b/src/commands/agents.ts @@ -87,7 +87,7 @@ export async function agentsListCommand(): Promise { } } -export async function agentsUninstallCommand(name: string): Promise { +export async function agentsRemoveCommand(name: string): Promise { const adapter = getAdapter(name); if (!adapter) { throw new OpperError( diff --git a/src/commands/menu/agents.ts b/src/commands/menu/agents.ts index f450673..943e33b 100644 --- a/src/commands/menu/agents.ts +++ b/src/commands/menu/agents.ts @@ -91,7 +91,7 @@ async function agentMenu(initial: AdapterStatus, opts: MenuOptions): Promise { }); }); -describe("agentsUninstallCommand", () => { +describe("agentsRemoveCommand", () => { beforeEach(() => { hermesUnconfigure.mockReset(); getAdapterMock.mockReset(); @@ -79,10 +79,10 @@ describe("agentsUninstallCommand", () => { }); const log = vi.spyOn(console, "log").mockImplementation(() => {}); try { - const { agentsUninstallCommand } = await import( + const { agentsRemoveCommand } = await import( "../../src/commands/agents.js" ); - await agentsUninstallCommand("hermes"); + await agentsRemoveCommand("hermes"); expect(hermesUnconfigure).toHaveBeenCalled(); const out = log.mock.calls.map((c) => String(c[0])).join("\n"); expect(out).toContain("Hermes Agent"); @@ -94,10 +94,10 @@ describe("agentsUninstallCommand", () => { it("throws AGENT_NOT_FOUND for unknown adapter names", async () => { getAdapterMock.mockReturnValue(null); - const { agentsUninstallCommand } = await import( + const { agentsRemoveCommand } = await import( "../../src/commands/agents.js" ); - await expect(agentsUninstallCommand("nope")).rejects.toMatchObject({ + await expect(agentsRemoveCommand("nope")).rejects.toMatchObject({ code: "AGENT_NOT_FOUND", }); }); diff --git a/test/commands/menu.test.ts b/test/commands/menu.test.ts index 420aa6a..adc035b 100644 --- a/test/commands/menu.test.ts +++ b/test/commands/menu.test.ts @@ -269,7 +269,7 @@ describe("menuCommand", () => { hermesIsConfigured.mockResolvedValue(true); answers.push(() => "agents"); answers.push(() => "agent:hermes"); - answers.push(() => "uninstall"); + answers.push(() => "remove"); answers.push(() => "back"); answers.push(() => "back"); answers.push(() => "quit"); From eb29302dc392ee7a503818da04c5dd296cac0471 Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 10:59:08 +0200 Subject: [PATCH 3/9] fix(launch): narrow snapshot to just the opper block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review flagged that the previous full-file snapshot/restore clobbered any edits the launched agent (or a concurrent user edit) made to other parts of the config during the session — themes, default model, MCP servers in opencode; sibling providers in pi/openclaw; user [settings] in codex. Replace `withConfigSnapshot(path, fn)` with `withJsonKey(path, keyPath, fn)` for the JSON adapters (pi, openclaw, opencode): captures only the value at the keyPath, runs fn, then re-reads the (possibly mutated) post-fn file and replaces only that key. Sibling keys edited mid-spawn are preserved. For codex (TOML sentinel-delimited) inline an equivalent narrow restore: capture the sentinel block contents, run, then strip the current block and reinsert the captured one — content outside the sentinels is preserved. Adds four regression tests (one per affected adapter) asserting that sibling edits made during spawnSync survive the restore. --- src/agents/codex.ts | 64 ++++++++-- src/agents/openclaw.ts | 8 +- src/agents/opencode.ts | 10 +- src/agents/pi.ts | 10 +- src/util/config-snapshot.ts | 126 ++++++++++++++----- test/agents/codex.test.ts | 30 +++++ test/agents/openclaw.test.ts | 31 ++++- test/agents/opencode.test.ts | 61 +++++++++- test/agents/pi.test.ts | 33 ++++- test/util/config-snapshot.test.ts | 193 +++++++++++++++++++++++------- 10 files changed, 474 insertions(+), 92 deletions(-) diff --git a/src/agents/codex.ts b/src/agents/codex.ts index b6e5ad8..36a2104 100644 --- a/src/agents/codex.ts +++ b/src/agents/codex.ts @@ -12,9 +12,10 @@ import type { OpperRouting, } from "./types.js"; +import { rm } from "node:fs/promises"; + import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { PICKER_MODELS } from "../config/models.js"; -import { withConfigSnapshot } from "../util/config-snapshot.js"; const SENTINEL_OPEN = "# >>> opper-cli >>>"; const SENTINEL_CLOSE = "# <<< opper-cli <<<"; @@ -58,6 +59,14 @@ function stripOpperBlock(text: string): string { return `${before}\n${after}`; } +function extractOpperBlock(text: string): string | null { + const startIdx = text.indexOf(SENTINEL_OPEN); + if (startIdx === -1) return null; + const endIdx = text.indexOf(SENTINEL_CLOSE, startIdx); + if (endIdx === -1) return null; + return text.slice(startIdx, endIdx + SENTINEL_CLOSE.length); +} + async function detect(): Promise { const path = await which("codex"); if (!path) return { installed: false }; @@ -141,12 +150,18 @@ function hasProfileArg(args: string[]): boolean { } async function spawn(args: string[], routing: OpperRouting): Promise { - // Snapshot config.toml so direct `codex` invocations after the launch - // don't inherit this session's URL. We rewrite the block with the - // session URL for the duration of spawn, then restore on exit. - return withConfigSnapshot(codexConfigPath(), async () => { - await writeOpperBlock(routing.baseUrl); + // Narrow snapshot: capture just the sentinel-delimited opper block (or + // its absence). On exit we restore that block on top of whatever the + // file looks like by then — anything outside the sentinels (user + // edits to [settings], theme, etc.) is preserved. + const cfg = codexConfigPath(); + const fileExistedBefore = existsSync(cfg); + const opperBlockBefore = fileExistedBefore + ? extractOpperBlock(readFileSync(cfg, "utf8")) + : null; + await writeOpperBlock(routing.baseUrl); + try { const env: NodeJS.ProcessEnv = { ...process.env, OPPER_API_KEY: routing.apiKey, @@ -156,7 +171,42 @@ async function spawn(args: string[], routing: OpperRouting): Promise { : ["--profile", DEFAULT_PROFILE, ...args]; const result = spawnSync("codex", finalArgs, { stdio: "inherit", env }); return result.status ?? -1; - }); + } finally { + await restoreOpperBlock(cfg, opperBlockBefore, fileExistedBefore); + } +} + +async function restoreOpperBlock( + cfg: string, + blockBefore: string | null, + fileExistedBefore: boolean, +): Promise { + try { + const current = existsSync(cfg) ? readFileSync(cfg, "utf8") : ""; + const stripped = stripOpperBlock(current); + let next: string; + if (blockBefore === null) { + next = stripped; + } else if (stripped.length === 0) { + next = `${blockBefore}\n`; + } else { + next = stripped.endsWith("\n") + ? `${stripped}${blockBefore}\n` + : `${stripped}\n${blockBefore}\n`; + } + if (!fileExistedBefore && next.trim().length === 0) { + await rm(cfg, { force: true }); + return; + } + await mkdir(dirname(cfg), { recursive: true }); + await writeFile(cfg, next, "utf8"); + } catch (err) { + process.stderr.write( + `opper: failed to restore ${cfg} after launch: ${ + err instanceof Error ? err.message : String(err) + }\n`, + ); + } } export const codex: AgentAdapter = { diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index 19a711e..690e230 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -9,7 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; -import { withConfigSnapshot } from "../util/config-snapshot.js"; +import { withJsonKey } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -159,7 +159,11 @@ async function spawn(args: string[], routing: OpperRouting): Promise { return result.status ?? -1; } - return withConfigSnapshot(modelsPath(), async () => { + // Snapshot just `providers.opper` so direct `openclaw` invocations + // after the launch don't inherit this session's URL — and so any + // sibling providers / top-level keys the user edits mid-spawn aren't + // clobbered on restore. + return withJsonKey(modelsPath(), ["providers", PROVIDER_KEY], async () => { await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); const result = spawnSync("openclaw", finalArgs, { stdio: "inherit" }); return result.status ?? -1; diff --git a/src/agents/opencode.ts b/src/agents/opencode.ts index 5730373..0b74635 100644 --- a/src/agents/opencode.ts +++ b/src/agents/opencode.ts @@ -9,7 +9,7 @@ import { } from "../setup/opencode.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { opencodeConfigPath } from "../util/editor-paths.js"; -import { withConfigSnapshot } from "../util/config-snapshot.js"; +import { withJsonKey } from "../util/config-snapshot.js"; import { brand } from "../ui/colors.js"; import type { AgentAdapter, @@ -153,11 +153,13 @@ async function spawn( } } - // User-scope: snapshot the global config so direct `opencode` + // User-scope: snapshot just `provider.opper` so direct `opencode` // invocations after the launch don't inherit this session's URL, // and so a launch on a machine with no prior opencode.json doesn't - // leave one behind. - return withConfigSnapshot(opencodeConfigPath("global"), async () => { + // leave one behind. OpenCode mutates this file during a session + // (theme, default model, MCP servers, …) — narrow restore keeps + // those edits and reverts only what we wrote. + return withJsonKey(opencodeConfigPath("global"), ["provider", "opper"], async () => { await configureOpenCode({ location: "global", overwrite: true }); // OpenCode reads `./opencode.json` if present and uses it instead of diff --git a/src/agents/pi.ts b/src/agents/pi.ts index 47f3680..0b9a0e6 100644 --- a/src/agents/pi.ts +++ b/src/agents/pi.ts @@ -9,7 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; -import { withConfigSnapshot } from "../util/config-snapshot.js"; +import { withJsonKey } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -121,9 +121,11 @@ async function unconfigure(): Promise { } async function spawn(args: string[], routing: OpperRouting): Promise { - // Snapshot models.json so direct `pi` invocations after the launch - // don't inherit this session's URL. - return withConfigSnapshot(piConfigPath(), async () => { + // Snapshot just `providers.opper` so direct `pi` invocations after + // the launch don't inherit this session's URL — and so any sibling + // providers (or other top-level keys) the user/agent edited mid-spawn + // survive intact. + return withJsonKey(piConfigPath(), ["providers", PROVIDER_KEY], async () => { await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); // pi's CLI requires *both* --provider and --model to resolve a non-default diff --git a/src/util/config-snapshot.ts b/src/util/config-snapshot.ts index abd85c6..9dea6ce 100644 --- a/src/util/config-snapshot.ts +++ b/src/util/config-snapshot.ts @@ -3,55 +3,127 @@ import { existsSync, readFileSync, statSync } from "node:fs"; import { chmod, mkdir, rm, writeFile } from "node:fs/promises"; /** - * Captures the contents (or absence) of `path`, runs `fn`, and restores - * the file to its pre-`fn` state in a finally block — including when the - * file didn't exist before (in which case "restore" means deleting it). + * Snapshot the value at `keyPath` in a JSON file (or its absence), run + * `fn`, then restore just that key to its pre-`fn` state. Other keys — + * including ones added or modified during `fn` (e.g. by the launched + * agent or a concurrent user edit) — are preserved. * * Used by adapters that bake a per-launch session URL into a persistent - * config file: snapshot ensures direct invocations of the agent (without - * `opper launch`) don't inherit the previous session's URL. + * config: snapshot ensures direct invocations of the agent (without + * `opper launch`) don't inherit the previous session's URL, while the + * narrow scope keeps unrelated mid-spawn config changes intact. * - * Restore failures are written to stderr but do not mask `fn`'s own error - * — the caller's error is always more informative than a follow-up I/O - * failure. + * If the file didn't exist before `fn` and is structurally empty after + * removing our key, it gets deleted on restore. + * + * Restore failures go to stderr but do not mask `fn`'s own error. */ -export async function withConfigSnapshot( +export async function withJsonKey( path: string, + keyPath: string[], fn: () => Promise, ): Promise { - const snapshot = capture(path); + if (keyPath.length === 0) throw new Error("keyPath must be non-empty"); + const fileExistedBefore = existsSync(path); + const beforeMode = fileExistedBefore + ? statSync(path).mode & 0o777 + : undefined; + const valueBefore = readKey(readJsonOrEmpty(path), keyPath); try { return await fn(); } finally { - await restore(path, snapshot); + await restore(path, keyPath, valueBefore, fileExistedBefore, beforeMode); + } +} + +function readJsonOrEmpty(path: string): Record { + if (!existsSync(path)) return {}; + try { + const parsed = JSON.parse(readFileSync(path, "utf8")); + return parsed && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record) + : {}; + } catch { + return {}; + } +} + +function readKey(obj: Record, keyPath: string[]): unknown { + let cur: unknown = obj; + for (const k of keyPath) { + if (cur === null || typeof cur !== "object") return undefined; + cur = (cur as Record)[k]; } + return cur; } -interface Snapshot { - existed: boolean; - bytes?: Buffer; - mode?: number; +function setKey( + obj: Record, + keyPath: string[], + value: unknown, +): void { + let cur = obj; + for (let i = 0; i < keyPath.length - 1; i++) { + const k = keyPath[i]!; + const next = cur[k]; + if (next === null || typeof next !== "object" || Array.isArray(next)) { + cur[k] = {}; + } + cur = cur[k] as Record; + } + const last = keyPath[keyPath.length - 1]!; + if (value === undefined) delete cur[last]; + else cur[last] = value; } -function capture(path: string): Snapshot { - if (!existsSync(path)) return { existed: false }; - const bytes = readFileSync(path); - const mode = statSync(path).mode & 0o777; - return { existed: true, bytes, mode }; +// Walk back up the path and drop intermediate objects that are now empty — +// keeps post-restore JSON clean when our key was the only inhabitant of +// `providers` / `provider`, etc. +function pruneEmptyAlongPath( + obj: Record, + keyPath: string[], +): void { + for (let depth = keyPath.length - 1; depth >= 1; depth--) { + let parent: Record = obj; + for (let j = 0; j < depth - 1; j++) { + const next = parent[keyPath[j]!]; + if (next === null || typeof next !== "object") return; + parent = next as Record; + } + const childKey = keyPath[depth - 1]!; + const child = parent[childKey]; + if ( + child !== null && + typeof child === "object" && + !Array.isArray(child) && + Object.keys(child as Record).length === 0 + ) { + delete parent[childKey]; + } + } } -async function restore(path: string, snap: Snapshot): Promise { +async function restore( + path: string, + keyPath: string[], + valueBefore: unknown, + fileExistedBefore: boolean, + beforeMode: number | undefined, +): Promise { try { - if (!snap.existed) { + const after = readJsonOrEmpty(path); + setKey(after, keyPath, valueBefore); + pruneEmptyAlongPath(after, keyPath); + + if (!fileExistedBefore && Object.keys(after).length === 0) { await rm(path, { force: true }); return; } + await mkdir(dirname(path), { recursive: true }); - // `writeFile`'s `mode` only applies when the file is being created. - // If the agent rewrote the file in place (still exists), we have to - // chmod explicitly to actually restore the original permissions. - await writeFile(path, snap.bytes!, { mode: snap.mode }); - if (snap.mode !== undefined) await chmod(path, snap.mode); + const text = JSON.stringify(after, null, 2) + "\n"; + await writeFile(path, text, beforeMode !== undefined ? { mode: beforeMode } : undefined); + if (beforeMode !== undefined) await chmod(path, beforeMode); } catch (err) { process.stderr.write( `opper: failed to restore ${path} after launch: ${ diff --git a/test/agents/codex.test.ts b/test/agents/codex.test.ts index db093f7..57348c3 100644 --- a/test/agents/codex.test.ts +++ b/test/agents/codex.test.ts @@ -271,4 +271,34 @@ describe("codex adapter", () => { expect(code).toBe(17); expect(readFileSync(cfgPath, "utf8")).toBe(before); }); + + it("spawn restore preserves user edits outside the sentinel block made mid-spawn", async () => { + // Anything outside the SENTINEL_OPEN/CLOSE markers is the user's + // own config (theme, settings, etc.). The narrow restore must + // not clobber edits made there during the session. + const cfgDir = join(sandbox, ".codex"); + const cfgPath = join(cfgDir, "config.toml"); + mkdirSync(cfgDir, { recursive: true }); + writeFileSync(cfgPath, "[settings]\ntheme = \"dark\"\n", "utf8"); + await codex.configure({}); + + spawnSyncMock.mockImplementation(() => { + // Simulate the user editing the [settings] block mid-session. + const cur = readFileSync(cfgPath, "utf8"); + writeFileSync(cfgPath, cur.replace('theme = "dark"', 'theme = "light"'), "utf8"); + return { status: 0 }; + }); + + await codex.spawn!([], { + baseUrl: SESSION_URL, + apiKey: "k", + model: "m", + compatShape: "openai", + }); + + const after = readFileSync(cfgPath, "utf8"); + expect(after).toContain('theme = "light"'); // sibling edit survived + expect(after).toContain('base_url = "https://api.opper.ai/v3/compat"'); // our block reverted + expect(after).not.toContain(SESSION_URL); + }); }); diff --git a/test/agents/openclaw.test.ts b/test/agents/openclaw.test.ts index c2f5ef7..5553741 100644 --- a/test/agents/openclaw.test.ts +++ b/test/agents/openclaw.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, readFileSync, existsSync } from "node:fs"; +import { mkdtempSync, rmSync, readFileSync, writeFileSync, existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -134,6 +134,35 @@ describe("openclaw adapter", () => { expect(readFileSync(cfgPath, "utf8")).toBe(before); }); + it("spawn restore preserves sibling providers / top-level edits made mid-spawn", async () => { + await openclaw.configure({ apiKey: "op_user_key" }); + const cfgPath = join( + sandbox, ".openclaw", "agents", "main", "agent", "models.json", + ); + + spawnSyncMock.mockImplementation(() => { + const cur = JSON.parse(readFileSync(cfgPath, "utf8")) as { + providers?: Record; + userKey?: string; + }; + cur.providers = cur.providers ?? {}; + cur.providers["custom"] = { baseUrl: "https://example.com" }; + cur.userKey = "preserved"; + writeFileSync(cfgPath, JSON.stringify(cur, null, 2) + "\n", "utf8"); + return { status: 0 }; + }); + + await openclaw.spawn!(["agent"], ROUTING); + + const after = JSON.parse(readFileSync(cfgPath, "utf8")) as { + providers?: Record; + userKey?: string; + }; + expect(after.providers?.custom).toEqual({ baseUrl: "https://example.com" }); + expect(after.userKey).toBe("preserved"); + expect(after.providers?.opper?.baseUrl).toBe("https://api.opper.ai/v3/compat"); + }); + it("spawn places the launch model at models[0] even when it isn't opus", async () => { spawnSyncMock.mockReturnValue({ status: 0 }); await openclaw.spawn!([], { ...ROUTING, model: "gpt-5.5" }); diff --git a/test/agents/opencode.test.ts b/test/agents/opencode.test.ts index 9eb3040..f093916 100644 --- a/test/agents/opencode.test.ts +++ b/test/agents/opencode.test.ts @@ -187,11 +187,11 @@ describe("opencode adapter", () => { }); spawnSyncMock.mockReturnValue({ status: 0 }); seedOpencodeConfig(sandbox); - const before = readFileSync(opencodeConfigPath(sandbox), "utf8"); + const before = JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8")); await opencode.spawn!([], ROUTING); - expect(readFileSync(opencodeConfigPath(sandbox), "utf8")).toBe(before); + expect(JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8"))).toEqual(before); }); it("spawn deletes any opencode.json it caused to be created when none existed before", async () => { @@ -231,12 +231,65 @@ describe("opencode adapter", () => { wrote: false, }); seedOpencodeConfig(sandbox); - const before = readFileSync(opencodeConfigPath(sandbox), "utf8"); + const before = JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8")); spawnSyncMock.mockReturnValue({ status: 17 }); const code = await opencode.spawn!([], ROUTING); expect(code).toBe(17); - expect(readFileSync(opencodeConfigPath(sandbox), "utf8")).toBe(before); + expect(JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8"))).toEqual(before); + }); + + it("user scope: restore preserves sibling edits OpenCode makes during the session", async () => { + // OpenCode mutates opencode.json during a session (theme, default + // model, MCP servers, …). Narrow restore must reset only + // provider.opper and leave the rest of the file alone. + configureOpenCodeMock.mockResolvedValue({ + path: opencodeConfigPath(sandbox), + wrote: false, + }); + seedOpencodeConfig(sandbox); + // Augment the seed with extra top-level keys that OpenCode would + // typically own. + const seedPath = opencodeConfigPath(sandbox); + const seeded = JSON.parse(readFileSync(seedPath, "utf8")) as { + provider: Record; + [k: string]: unknown; + }; + seeded.theme = "dark"; + seeded.model = "old-default"; + writeFileSync(seedPath, JSON.stringify(seeded, null, 2) + "\n", "utf8"); + + spawnSyncMock.mockImplementation(() => { + const cur = JSON.parse(readFileSync(seedPath, "utf8")) as { + provider: Record; + theme: string; + model: string; + mcpServers?: Record; + [k: string]: unknown; + }; + cur.theme = "light"; + cur.model = "new-default"; + cur.mcpServers = { fs: { command: "mcp-fs" } }; + writeFileSync(seedPath, JSON.stringify(cur, null, 2) + "\n", "utf8"); + return { status: 0 }; + }); + + await opencode.spawn!([], ROUTING); + + const after = JSON.parse(readFileSync(seedPath, "utf8")) as { + provider: { opper: { options: { baseURL: string } } }; + theme: string; + model: string; + mcpServers?: Record; + }; + // Sibling edits survive. + expect(after.theme).toBe("light"); + expect(after.model).toBe("new-default"); + expect(after.mcpServers).toEqual({ fs: { command: "mcp-fs" } }); + // Our provider is back to the pre-launch (compat) URL. + expect(after.provider.opper.options.baseURL).toBe( + "https://api.opper.ai/v3/compat", + ); }); it("spawn does not crash if the opencode.json doesn't exist yet (configureOpenCode was a no-op stub)", async () => { diff --git a/test/agents/pi.test.ts b/test/agents/pi.test.ts index f4650db..049d164 100644 --- a/test/agents/pi.test.ts +++ b/test/agents/pi.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, readFileSync, existsSync } from "node:fs"; +import { mkdtempSync, rmSync, readFileSync, writeFileSync, existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -129,6 +129,37 @@ describe("pi adapter", () => { expect(readFileSync(cfgPath, "utf8")).toBe(before); }); + it("spawn restore preserves sibling providers / top-level edits made mid-spawn", async () => { + // The user (or a concurrent edit) adds a non-opper provider while + // the launch is running. Narrow restore must reset only providers.opper + // and leave the new sibling alone. + await pi.configure({ apiKey: "op_user_key" }); + const cfgPath = join(sandbox, ".pi", "agent", "models.json"); + + spawnSyncMock.mockImplementation(() => { + const cur = JSON.parse(readFileSync(cfgPath, "utf8")) as { + providers?: Record; + somethingElse?: string; + }; + cur.providers = cur.providers ?? {}; + cur.providers["lmstudio"] = { baseUrl: "http://localhost:1234" }; + cur.somethingElse = "user added this"; + writeFileSync(cfgPath, JSON.stringify(cur, null, 2) + "\n", "utf8"); + return { status: 0 }; + }); + + await pi.spawn!([], ROUTING); + + const after = JSON.parse(readFileSync(cfgPath, "utf8")) as { + providers?: Record; + somethingElse?: string; + }; + expect(after.providers?.lmstudio).toEqual({ baseUrl: "http://localhost:1234" }); + expect(after.somethingElse).toBe("user added this"); + // Our opper provider is reverted to the pre-launch (compat) URL. + expect(after.providers?.opper?.baseUrl).toBe("https://api.opper.ai/v3/compat"); + }); + it("spawn places the launch model at models[0] even when it isn't opus", async () => { // Read mid-spawn — snapshot/restore wipes the config on exit when // there was no pre-launch config. diff --git a/test/util/config-snapshot.test.ts b/test/util/config-snapshot.test.ts index 8d07aec..3763057 100644 --- a/test/util/config-snapshot.test.ts +++ b/test/util/config-snapshot.test.ts @@ -11,9 +11,13 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { withConfigSnapshot } from "../../src/util/config-snapshot.js"; +import { withJsonKey } from "../../src/util/config-snapshot.js"; -describe("withConfigSnapshot", () => { +function readJson(path: string): Record { + return JSON.parse(readFileSync(path, "utf8")); +} + +describe("withJsonKey", () => { let sandbox: string; beforeEach(() => { @@ -23,87 +27,192 @@ describe("withConfigSnapshot", () => { rmSync(sandbox, { recursive: true, force: true }); }); - it("restores the original bytes when the file pre-existed", async () => { - const path = join(sandbox, "config.toml"); - writeFileSync(path, "original\n", "utf8"); + it("restores the captured value at the keyPath when it pre-existed", async () => { + const path = join(sandbox, "models.json"); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "compat" } } }, null, 2), + "utf8", + ); + + await withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify( + { providers: { opper: { baseUrl: "session-url" } } }, + null, + 2, + ), + "utf8", + ); + }); + + expect(readJson(path)).toEqual({ + providers: { opper: { baseUrl: "compat" } }, + }); + }); + + it("removes the key (and its now-empty parent) when it didn't exist before fn added it", async () => { + const path = join(sandbox, "models.json"); + writeFileSync(path, JSON.stringify({ other: "stuff" }, null, 2), "utf8"); + + await withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify( + { other: "stuff", providers: { opper: { baseUrl: "x" } } }, + null, + 2, + ), + "utf8", + ); + }); + + expect(readJson(path)).toEqual({ other: "stuff" }); + }); - const result = await withConfigSnapshot(path, async () => { - writeFileSync(path, "mutated\n", "utf8"); - return 42; + it("preserves sibling edits made during fn (the headline regression test)", async () => { + // The launched agent (or a concurrent user edit) writes to a + // sibling key — we must not clobber that on restore. + const path = join(sandbox, "models.json"); + writeFileSync( + path, + JSON.stringify( + { providers: { opper: { baseUrl: "compat" } }, theme: "dark" }, + null, + 2, + ), + "utf8", + ); + + await withJsonKey(path, ["providers", "opper"], async () => { + const cur = readJson(path); + // Agent rewrites our key (session URL) AND mutates a sibling. + (cur.providers as Record).opper = { + baseUrl: "session-url", + }; + (cur.providers as Record).ollama = { + baseUrl: "http://localhost:11434", + }; + cur.theme = "light"; + writeFileSync(path, JSON.stringify(cur, null, 2), "utf8"); }); - expect(result).toBe(42); - expect(readFileSync(path, "utf8")).toBe("original\n"); + const after = readJson(path); + expect(after.theme).toBe("light"); + expect((after.providers as Record).ollama).toEqual({ + baseUrl: "http://localhost:11434", + }); + expect((after.providers as Record).opper).toEqual({ + baseUrl: "compat", + }); }); - it("removes the file if it did not exist before", async () => { + it("deletes the file when it didn't exist before and is structurally empty after restore", async () => { const path = join(sandbox, "models.json"); expect(existsSync(path)).toBe(false); - await withConfigSnapshot(path, async () => { - writeFileSync(path, "{}\n", "utf8"); - expect(existsSync(path)).toBe(true); + await withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "x" } } }, null, 2), + "utf8", + ); }); expect(existsSync(path)).toBe(false); }); - it("restores even when fn throws, and rethrows the error", async () => { - const path = join(sandbox, "config.toml"); - writeFileSync(path, "before\n", "utf8"); + it("keeps the file when it didn't exist before but the agent added other content", async () => { + const path = join(sandbox, "models.json"); - await expect( - withConfigSnapshot(path, async () => { - writeFileSync(path, "during\n", "utf8"); - throw new Error("boom"); - }), - ).rejects.toThrow("boom"); + await withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify( + { + providers: { + opper: { baseUrl: "x" }, + ollama: { baseUrl: "y" }, + }, + }, + null, + 2, + ), + "utf8", + ); + }); - expect(readFileSync(path, "utf8")).toBe("before\n"); + expect(readJson(path)).toEqual({ + providers: { ollama: { baseUrl: "y" } }, + }); }); - it("removes a created file even when fn throws", async () => { + it("restores even when fn throws, and rethrows the error", async () => { const path = join(sandbox, "models.json"); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "compat" } } }, null, 2), + "utf8", + ); await expect( - withConfigSnapshot(path, async () => { - writeFileSync(path, "{}\n", "utf8"); + withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify( + { providers: { opper: { baseUrl: "session" } } }, + null, + 2, + ), + "utf8", + ); throw new Error("boom"); }), ).rejects.toThrow("boom"); - expect(existsSync(path)).toBe(false); + expect(readJson(path)).toEqual({ + providers: { opper: { baseUrl: "compat" } }, + }); }); it("preserves the original file mode", async () => { const path = join(sandbox, "secret.json"); - writeFileSync(path, "{}\n", { mode: 0o600 }); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "compat" } } }), + { mode: 0o600 }, + ); expect(statSync(path).mode & 0o777).toBe(0o600); - await withConfigSnapshot(path, async () => { - // Use chmodSync to actually loosen the mode — `writeFileSync`'s - // `mode` option is silently ignored when the file already exists. - writeFileSync(path, '{"x":1}\n', "utf8"); + await withJsonKey(path, ["providers", "opper"], async () => { + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "session" } } }), + "utf8", + ); chmodSync(path, 0o644); - expect(statSync(path).mode & 0o777).toBe(0o644); }); - expect(readFileSync(path, "utf8")).toBe("{}\n"); expect(statSync(path).mode & 0o777).toBe(0o600); }); it("works when the parent directory has to be recreated to restore", async () => { - // Simulates an adapter that nukes the directory mid-launch. Restore - // should still put the original bytes back at the original path. const dir = join(sandbox, "nested", "deep"); mkdirSync(dir, { recursive: true }); - const path = join(dir, "config.toml"); - writeFileSync(path, "keep me\n", "utf8"); - - await withConfigSnapshot(path, async () => { + const path = join(dir, "models.json"); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "keep" } } }), + "utf8", + ); + + await withJsonKey(path, ["providers", "opper"], async () => { rmSync(dir, { recursive: true, force: true }); }); - expect(readFileSync(path, "utf8")).toBe("keep me\n"); + expect(readJson(path)).toEqual({ + providers: { opper: { baseUrl: "keep" } }, + }); }); }); From 78fab576429615053628844865ae208f7571b5cf Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:08:47 +0200 Subject: [PATCH 4/9] fix(launch): detect openclaw daemon-start by subcommand, not arg count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `opper launch openclaw -- gateway start` (or `daemon start`) routes into a long-lived service the same way the default no-args path does — both detach a daemon that outlives spawnSync. The previous logic gated the daemon-safe behavior on `args.length === 0`, so an explicit `gateway start` fell into the snapshot/restore branch and clobbered or desynchronized the daemon's routing. Match `gateway|daemon start` directly instead. Adds tests pinning both explicit forms. --- src/agents/openclaw.ts | 11 +++++++++-- test/agents/openclaw.test.ts | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index 690e230..5c82b25 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -130,8 +130,15 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // opper launch openclaw -- agent --local -m "summarise ..." // opper launch openclaw -- gateway run # foreground if you // # really want it - const isDaemonStart = args.length === 0; - const finalArgs = isDaemonStart ? ["gateway", "start"] : args; + // Detect daemon launches by subcommand, not arg count: `opper launch + // openclaw` (no args, defaults to `gateway start`) AND `opper launch + // openclaw -- gateway start` / `-- daemon start` all detach a + // long-lived service that outlives spawnSync. + const finalArgs = args.length === 0 ? ["gateway", "start"] : args; + const isDaemonStart = + finalArgs.length >= 2 && + (finalArgs[0] === "gateway" || finalArgs[0] === "daemon") && + finalArgs[1] === "start"; // Snapshot/restore only fits one-shot synchronous invocations. For the // daemon path, `spawnSync` returns as soon as the gateway detaches — diff --git a/test/agents/openclaw.test.ts b/test/agents/openclaw.test.ts index 5553741..b3e9a59 100644 --- a/test/agents/openclaw.test.ts +++ b/test/agents/openclaw.test.ts @@ -196,6 +196,23 @@ describe("openclaw adapter", () => { expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); }); + it("spawn does NOT snapshot when the user explicitly passes `gateway start`", async () => { + // `opper launch openclaw -- gateway start` is the daemon path too + // — same semantics as the no-args default. We must not gate on + // arg-count alone. + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["gateway", "start"], ROUTING); + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + + it("spawn does NOT snapshot when the user explicitly passes `daemon start`", async () => { + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["daemon", "start"], ROUTING); + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + it("spawn forwards user-supplied args verbatim", async () => { spawnSyncMock.mockReturnValue({ status: 0 }); await openclaw.spawn!(["agent", "--local"], ROUTING); From 3660f7574b2db7cb069deb54646a8eed55b5c60b Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:24:41 +0200 Subject: [PATCH 5/9] fix(launch): cover all opper-owned opencode keys; fix project capture order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues codex flagged on the previous narrow-snapshot pass: 1. The opencode template writes BOTH `provider.opper` AND a top-level `model: "opper/..."`. Snapshotting only `provider.opper` left an orphaned `model` pointing at a removed provider after a fresh first launch — direct `opencode` runs after that would fail. 2. Project scope captured `restoreUrl` AFTER `configureOpenCode({ overwrite: true })` ran. With overwrite enabled, configureOpenCode replaces provider.opper with template values BEFORE the capture, so a hand-edited self-hosted baseURL was wiped before we could read it. Generalise the helper from `withJsonKey(path, keyPath, fn)` to `withJsonKeys(path, keyPaths[], fn)` and pass both Opper-owned keypaths from opencode user-scope. Move the readBaseUrl capture to before configureOpenCode in project-scope. Strengthens the project-scope custom-baseURL test to actually simulate `configureOpenCode`'s overwrite behavior, plus adds a test pinning that an orphaned `model: "opper/..."` is removed when no opper provider existed before. --- src/agents/openclaw.ts | 4 +- src/agents/opencode.ts | 74 +++++++++++++++------------- src/agents/pi.ts | 4 +- src/util/config-snapshot.ts | 37 ++++++++------ test/agents/opencode.test.ts | 81 +++++++++++++++++++++++++------ test/util/config-snapshot.test.ts | 79 ++++++++++++++++++++++++++---- 6 files changed, 202 insertions(+), 77 deletions(-) diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index 5c82b25..ee32017 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -9,7 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; -import { withJsonKey } from "../util/config-snapshot.js"; +import { withJsonKeys } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -170,7 +170,7 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // after the launch don't inherit this session's URL — and so any // sibling providers / top-level keys the user edits mid-spawn aren't // clobbered on restore. - return withJsonKey(modelsPath(), ["providers", PROVIDER_KEY], async () => { + return withJsonKeys(modelsPath(), [["providers", PROVIDER_KEY]], async () => { await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); const result = spawnSync("openclaw", finalArgs, { stdio: "inherit" }); return result.status ?? -1; diff --git a/src/agents/opencode.ts b/src/agents/opencode.ts index 0b74635..1be19f5 100644 --- a/src/agents/opencode.ts +++ b/src/agents/opencode.ts @@ -9,7 +9,7 @@ import { } from "../setup/opencode.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { opencodeConfigPath } from "../util/editor-paths.js"; -import { withJsonKey } from "../util/config-snapshot.js"; +import { withJsonKeys } from "../util/config-snapshot.js"; import { brand } from "../ui/colors.js"; import type { AgentAdapter, @@ -135,11 +135,13 @@ async function spawn( // the point — instead we apply the session URL only for the spawn // and reset baseURL afterwards. The opper provider block stays in // place across launches. - await configureOpenCode({ location: "local", overwrite: true }); - // Capture *after* configureOpenCode so a fresh project picks up the - // template's compat URL, while a hand-edited config (e.g. pointing - // at a self-hosted Opper) keeps that custom URL across launches. + // + // Capture restoreUrl *before* configureOpenCode runs, since + // `overwrite: true` replaces provider.opper with template values + // (compat URL) — capturing after would discard a hand-edited + // self-hosted baseURL. const restoreUrl = readBaseUrl("local") ?? OPPER_COMPAT_URL; + await configureOpenCode({ location: "local", overwrite: true }); await setSessionBaseUrl(routing.baseUrl, "local"); try { const env: NodeJS.ProcessEnv = { @@ -153,38 +155,42 @@ async function spawn( } } - // User-scope: snapshot just `provider.opper` so direct `opencode` - // invocations after the launch don't inherit this session's URL, - // and so a launch on a machine with no prior opencode.json doesn't - // leave one behind. OpenCode mutates this file during a session - // (theme, default model, MCP servers, …) — narrow restore keeps - // those edits and reverts only what we wrote. - return withJsonKey(opencodeConfigPath("global"), ["provider", "opper"], async () => { - await configureOpenCode({ location: "global", overwrite: true }); + // User-scope: snapshot the Opper-owned keys. The template writes + // both `provider.opper` AND a top-level `model: "opper/..."` — narrow + // restore must cover both, otherwise a fresh first launch leaves an + // orphaned `model` pointing at a removed provider. OpenCode mutates + // sibling keys (theme, MCP servers, …) during a session — those are + // outside our keyPaths and survive the restore. + return withJsonKeys( + opencodeConfigPath("global"), + [["provider", "opper"], ["model"]], + async () => { + await configureOpenCode({ location: "global", overwrite: true }); - // OpenCode reads `./opencode.json` if present and uses it instead of - // the user-level config. If one exists without an Opper provider, - // whatever we just wrote globally is dead weight — warn so the user - // can re-run with `--project`. - const projectPath = opencodeConfigPath("local"); - const state = readProjectConfigState(projectPath); - if (state.exists && !state.hasOpperProvider) { - process.stderr.write( - brand.dim( - `note: ${projectPath} will shadow the user-level Opper config. Re-run with \`--project\` to write the Opper provider there instead.\n`, - ), - ); - } + // OpenCode reads `./opencode.json` if present and uses it instead + // of the user-level config. If one exists without an Opper + // provider, whatever we just wrote globally is dead weight — warn + // so the user can re-run with `--project`. + const projectPath = opencodeConfigPath("local"); + const state = readProjectConfigState(projectPath); + if (state.exists && !state.hasOpperProvider) { + process.stderr.write( + brand.dim( + `note: ${projectPath} will shadow the user-level Opper config. Re-run with \`--project\` to write the Opper provider there instead.\n`, + ), + ); + } - await setSessionBaseUrl(routing.baseUrl, "global"); + await setSessionBaseUrl(routing.baseUrl, "global"); - const env: NodeJS.ProcessEnv = { - ...process.env, - OPPER_API_KEY: routing.apiKey, - }; - const result = spawnSync("opencode", args, { stdio: "inherit", env }); - return result.status ?? -1; - }); + const env: NodeJS.ProcessEnv = { + ...process.env, + OPPER_API_KEY: routing.apiKey, + }; + const result = spawnSync("opencode", args, { stdio: "inherit", env }); + return result.status ?? -1; + }, + ); } export const opencode: AgentAdapter = { diff --git a/src/agents/pi.ts b/src/agents/pi.ts index 0b9a0e6..b332b00 100644 --- a/src/agents/pi.ts +++ b/src/agents/pi.ts @@ -9,7 +9,7 @@ import { OpperError } from "../errors.js"; import { npmInstallGlobal } from "./npm-install.js"; import { OPPER_COMPAT_URL } from "../config/endpoints.js"; import { DEFAULT_MODELS, pickerModelsForLaunch } from "../config/models.js"; -import { withJsonKey } from "../util/config-snapshot.js"; +import { withJsonKeys } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -125,7 +125,7 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // the launch don't inherit this session's URL — and so any sibling // providers (or other top-level keys) the user/agent edited mid-spawn // survive intact. - return withJsonKey(piConfigPath(), ["providers", PROVIDER_KEY], async () => { + return withJsonKeys(piConfigPath(), [["providers", PROVIDER_KEY]], async () => { await setOpperProvider(routing.apiKey, routing.model, routing.baseUrl); // pi's CLI requires *both* --provider and --model to resolve a non-default diff --git a/src/util/config-snapshot.ts b/src/util/config-snapshot.ts index 9dea6ce..7fd293b 100644 --- a/src/util/config-snapshot.ts +++ b/src/util/config-snapshot.ts @@ -3,10 +3,11 @@ import { existsSync, readFileSync, statSync } from "node:fs"; import { chmod, mkdir, rm, writeFile } from "node:fs/promises"; /** - * Snapshot the value at `keyPath` in a JSON file (or its absence), run - * `fn`, then restore just that key to its pre-`fn` state. Other keys — - * including ones added or modified during `fn` (e.g. by the launched - * agent or a concurrent user edit) — are preserved. + * Snapshot the values at one or more `keyPaths` in a JSON file (or + * their absence), run `fn`, then restore just those keys to their + * pre-`fn` state. Other keys — including ones added or modified during + * `fn` (e.g. by the launched agent or a concurrent user edit) — are + * preserved. * * Used by adapters that bake a per-launch session URL into a persistent * config: snapshot ensures direct invocations of the agent (without @@ -14,25 +15,29 @@ import { chmod, mkdir, rm, writeFile } from "node:fs/promises"; * narrow scope keeps unrelated mid-spawn config changes intact. * * If the file didn't exist before `fn` and is structurally empty after - * removing our key, it gets deleted on restore. + * the captured keys are restored, it gets deleted. * * Restore failures go to stderr but do not mask `fn`'s own error. */ -export async function withJsonKey( +export async function withJsonKeys( path: string, - keyPath: string[], + keyPaths: string[][], fn: () => Promise, ): Promise { - if (keyPath.length === 0) throw new Error("keyPath must be non-empty"); + if (keyPaths.length === 0) throw new Error("keyPaths must be non-empty"); + for (const kp of keyPaths) { + if (kp.length === 0) throw new Error("each keyPath must be non-empty"); + } const fileExistedBefore = existsSync(path); const beforeMode = fileExistedBefore ? statSync(path).mode & 0o777 : undefined; - const valueBefore = readKey(readJsonOrEmpty(path), keyPath); + const before = readJsonOrEmpty(path); + const valuesBefore = keyPaths.map((kp) => readKey(before, kp)); try { return await fn(); } finally { - await restore(path, keyPath, valueBefore, fileExistedBefore, beforeMode); + await restore(path, keyPaths, valuesBefore, fileExistedBefore, beforeMode); } } @@ -105,15 +110,19 @@ function pruneEmptyAlongPath( async function restore( path: string, - keyPath: string[], - valueBefore: unknown, + keyPaths: string[][], + valuesBefore: unknown[], fileExistedBefore: boolean, beforeMode: number | undefined, ): Promise { try { const after = readJsonOrEmpty(path); - setKey(after, keyPath, valueBefore); - pruneEmptyAlongPath(after, keyPath); + for (let i = 0; i < keyPaths.length; i++) { + setKey(after, keyPaths[i]!, valuesBefore[i]); + } + for (const kp of keyPaths) { + pruneEmptyAlongPath(after, kp); + } if (!fileExistedBefore && Object.keys(after).length === 0) { await rm(path, { force: true }); diff --git a/test/agents/opencode.test.ts b/test/agents/opencode.test.ts index f093916..60231d6 100644 --- a/test/agents/opencode.test.ts +++ b/test/agents/opencode.test.ts @@ -239,24 +239,26 @@ describe("opencode adapter", () => { expect(JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8"))).toEqual(before); }); - it("user scope: restore preserves sibling edits OpenCode makes during the session", async () => { - // OpenCode mutates opencode.json during a session (theme, default - // model, MCP servers, …). Narrow restore must reset only - // provider.opper and leave the rest of the file alone. + it("user scope: restore preserves non-Opper-owned sibling edits, reverts Opper-owned keys", async () => { + // OpenCode mutates opencode.json during a session — themes, MCP + // servers, etc. Narrow restore must: + // - revert provider.opper AND top-level model (both Opper-owned; + // the template writes both, and an orphaned `model: "opper/X"` + // pointing at a removed provider would break direct opencode + // runs after the launch); + // - leave non-Opper-owned siblings (theme, mcpServers, …) alone. configureOpenCodeMock.mockResolvedValue({ path: opencodeConfigPath(sandbox), wrote: false, }); seedOpencodeConfig(sandbox); - // Augment the seed with extra top-level keys that OpenCode would - // typically own. const seedPath = opencodeConfigPath(sandbox); const seeded = JSON.parse(readFileSync(seedPath, "utf8")) as { provider: Record; [k: string]: unknown; }; seeded.theme = "dark"; - seeded.model = "old-default"; + seeded.model = "opper/claude-opus-4-7"; writeFileSync(seedPath, JSON.stringify(seeded, null, 2) + "\n", "utf8"); spawnSyncMock.mockImplementation(() => { @@ -268,7 +270,7 @@ describe("opencode adapter", () => { [k: string]: unknown; }; cur.theme = "light"; - cur.model = "new-default"; + cur.model = "opper/claude-haiku-4-5"; cur.mcpServers = { fs: { command: "mcp-fs" } }; writeFileSync(seedPath, JSON.stringify(cur, null, 2) + "\n", "utf8"); return { status: 0 }; @@ -282,16 +284,51 @@ describe("opencode adapter", () => { model: string; mcpServers?: Record; }; - // Sibling edits survive. + // Non-Opper-owned siblings survive. expect(after.theme).toBe("light"); - expect(after.model).toBe("new-default"); expect(after.mcpServers).toEqual({ fs: { command: "mcp-fs" } }); - // Our provider is back to the pre-launch (compat) URL. + // Opper-owned keys revert to pre-launch state. + expect(after.model).toBe("opper/claude-opus-4-7"); expect(after.provider.opper.options.baseURL).toBe( "https://api.opper.ai/v3/compat", ); }); + it("user scope: restore removes orphaned `model: opper/...` when no opper provider existed before", async () => { + // The exact regression Codex flagged: a fresh first launch with no + // prior opencode.json. The template writes provider.opper AND a + // top-level model: "opper/...". After restore, both must be gone — + // an orphaned `model` pointing at a removed provider would break + // direct `opencode` runs after the launch. + const cfg = opencodeConfigPath(sandbox); + expect(existsSync(cfg)).toBe(false); + configureOpenCodeMock.mockImplementation(async () => { + mkdirSync(join(sandbox, ".config", "opencode"), { recursive: true }); + writeFileSync( + cfg, + JSON.stringify({ + provider: { + opper: { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: "https://api.opper.ai/v3/compat", + apiKey: "{env:OPPER_API_KEY}", + }, + }, + }, + model: "opper/claude-opus-4-7", + }, null, 2) + "\n", + "utf8", + ); + return { path: cfg, wrote: true }; + }); + spawnSyncMock.mockReturnValue({ status: 0 }); + + await opencode.spawn!([], ROUTING); + + expect(existsSync(cfg)).toBe(false); + }); + it("spawn does not crash if the opencode.json doesn't exist yet (configureOpenCode was a no-op stub)", async () => { configureOpenCodeMock.mockResolvedValue({ path: opencodeConfigPath(sandbox), @@ -398,10 +435,24 @@ describe("opencode adapter", () => { }, null, 2), "utf8", ); - // configureOpenCode is a no-op when an opper provider already - // exists (see src/setup/opencode.ts:80-82). - configureOpenCodeMock.mockResolvedValue({ - path: projectCfg, wrote: false, reason: "exists" as const, + // Simulate `configureOpenCode({overwrite: true})` — it really + // does replace provider.opper with template values (compat URL), + // wiping the user's custom baseURL. The fix is to capture + // `restoreUrl` before this call. + configureOpenCodeMock.mockImplementation(async () => { + const cur = JSON.parse(readFileSync(projectCfg, "utf8")) as { + provider?: { opper?: { options?: { baseURL?: string } } }; + }; + cur.provider = cur.provider ?? {}; + cur.provider.opper = { + npm: "@ai-sdk/openai-compatible", + options: { + baseURL: "https://api.opper.ai/v3/compat", + apiKey: "{env:OPPER_API_KEY}", + }, + } as never; + writeFileSync(projectCfg, JSON.stringify(cur, null, 2), "utf8"); + return { path: projectCfg, wrote: true }; }); spawnSyncMock.mockReturnValue({ status: 0 }); diff --git a/test/util/config-snapshot.test.ts b/test/util/config-snapshot.test.ts index 3763057..57ce223 100644 --- a/test/util/config-snapshot.test.ts +++ b/test/util/config-snapshot.test.ts @@ -11,13 +11,13 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { withJsonKey } from "../../src/util/config-snapshot.js"; +import { withJsonKeys } from "../../src/util/config-snapshot.js"; function readJson(path: string): Record { return JSON.parse(readFileSync(path, "utf8")); } -describe("withJsonKey", () => { +describe("withJsonKeys", () => { let sandbox: string; beforeEach(() => { @@ -35,7 +35,7 @@ describe("withJsonKey", () => { "utf8", ); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify( @@ -56,7 +56,7 @@ describe("withJsonKey", () => { const path = join(sandbox, "models.json"); writeFileSync(path, JSON.stringify({ other: "stuff" }, null, 2), "utf8"); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify( @@ -85,7 +85,7 @@ describe("withJsonKey", () => { "utf8", ); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { const cur = readJson(path); // Agent rewrites our key (session URL) AND mutates a sibling. (cur.providers as Record).opper = { @@ -112,7 +112,7 @@ describe("withJsonKey", () => { const path = join(sandbox, "models.json"); expect(existsSync(path)).toBe(false); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify({ providers: { opper: { baseUrl: "x" } } }, null, 2), @@ -126,7 +126,7 @@ describe("withJsonKey", () => { it("keeps the file when it didn't exist before but the agent added other content", async () => { const path = join(sandbox, "models.json"); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify( @@ -157,7 +157,7 @@ describe("withJsonKey", () => { ); await expect( - withJsonKey(path, ["providers", "opper"], async () => { + withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify( @@ -185,7 +185,7 @@ describe("withJsonKey", () => { ); expect(statSync(path).mode & 0o777).toBe(0o600); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { writeFileSync( path, JSON.stringify({ providers: { opper: { baseUrl: "session" } } }), @@ -197,6 +197,65 @@ describe("withJsonKey", () => { expect(statSync(path).mode & 0o777).toBe(0o600); }); + it("restores multiple keyPaths independently", async () => { + // OpenCode case: snapshot both `provider.opper` (the Opper provider + // block) and the top-level `model` key — both are Opper-owned. + const path = join(sandbox, "opencode.json"); + writeFileSync( + path, + JSON.stringify({ + provider: { opper: { baseURL: "compat" } }, + model: "opper/old-default", + theme: "dark", + }, null, 2), + "utf8", + ); + + await withJsonKeys( + path, + [["provider", "opper"], ["model"]], + async () => { + writeFileSync( + path, + JSON.stringify({ + provider: { opper: { baseURL: "session-url" } }, + model: "opper/something-else", + theme: "light", // sibling user edit, must survive + }, null, 2), + "utf8", + ); + }, + ); + + const after = readJson(path); + expect(after.provider).toEqual({ opper: { baseURL: "compat" } }); + expect(after.model).toBe("opper/old-default"); + expect(after.theme).toBe("light"); + }); + + it("removes multiple keyPaths if they didn't exist before fn added them", async () => { + const path = join(sandbox, "opencode.json"); + writeFileSync(path, JSON.stringify({ theme: "dark" }, null, 2), "utf8"); + + await withJsonKeys( + path, + [["provider", "opper"], ["model"]], + async () => { + writeFileSync( + path, + JSON.stringify({ + theme: "dark", + provider: { opper: { baseURL: "x" } }, + model: "opper/y", + }, null, 2), + "utf8", + ); + }, + ); + + expect(readJson(path)).toEqual({ theme: "dark" }); + }); + it("works when the parent directory has to be recreated to restore", async () => { const dir = join(sandbox, "nested", "deep"); mkdirSync(dir, { recursive: true }); @@ -207,7 +266,7 @@ describe("withJsonKey", () => { "utf8", ); - await withJsonKey(path, ["providers", "opper"], async () => { + await withJsonKeys(path, [["providers", "opper"]], async () => { rmSync(dir, { recursive: true, force: true }); }); From 415f1d00d69a57c84763145c18ad8cd4dc0ee151 Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:31:23 +0200 Subject: [PATCH 6/9] fix(launch): tolerate unreadable config in snapshot capture path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codex spawn was reading config.toml outside any try/catch, so a permission error or transient I/O failure on a pre-existing-but- unreadable file would abort the launch — a regression vs. the prior behavior, where writeOpperBlock tolerated read failures and proceeded with an empty baseline. Wrap the readFileSync (capture of the opper sentinel block) in try/catch and fall back to a null block on failure. Same defensive guard around statSync in withJsonKeys (mode capture for pi/openclaw/opencode). Restore paths were already wrapped — only the capture side was exposed. --- src/agents/codex.ts | 15 ++++++++++++--- src/util/config-snapshot.ts | 11 ++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/agents/codex.ts b/src/agents/codex.ts index 36a2104..9676022 100644 --- a/src/agents/codex.ts +++ b/src/agents/codex.ts @@ -156,9 +156,18 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // edits to [settings], theme, etc.) is preserved. const cfg = codexConfigPath(); const fileExistedBefore = existsSync(cfg); - const opperBlockBefore = fileExistedBefore - ? extractOpperBlock(readFileSync(cfg, "utf8")) - : null; + // Tolerate read failures (perm, transient I/O) — same baseline as + // writeOpperBlock, which falls back to empty on read errors. A + // hard fail here would regress launch for users with unreadable + // configs. + let opperBlockBefore: string | null = null; + if (fileExistedBefore) { + try { + opperBlockBefore = extractOpperBlock(readFileSync(cfg, "utf8")); + } catch { + opperBlockBefore = null; + } + } await writeOpperBlock(routing.baseUrl); try { diff --git a/src/util/config-snapshot.ts b/src/util/config-snapshot.ts index 7fd293b..5d2267f 100644 --- a/src/util/config-snapshot.ts +++ b/src/util/config-snapshot.ts @@ -29,9 +29,14 @@ export async function withJsonKeys( if (kp.length === 0) throw new Error("each keyPath must be non-empty"); } const fileExistedBefore = existsSync(path); - const beforeMode = fileExistedBefore - ? statSync(path).mode & 0o777 - : undefined; + let beforeMode: number | undefined; + if (fileExistedBefore) { + try { + beforeMode = statSync(path).mode & 0o777; + } catch { + beforeMode = undefined; + } + } const before = readJsonOrEmpty(path); const valuesBefore = keyPaths.map((kp) => readKey(before, kp)); try { From 74a9b033dc06aa337bca1d9ed71b760be9141ac3 Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:37:52 +0200 Subject: [PATCH 7/9] fix(launch): scan for openclaw daemon-start anywhere in args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenClaw allows global flags before the subcommand (`openclaw [--dev] [--profile ] `), so daemon launches like `opper launch openclaw -- --profile dev gateway start` arrived with `args[0] === "--profile"`. The previous positional check missed those, falling into the snapshot/restore path — `spawnSync` returns when the daemon detaches and restore immediately desyncs models.json from the live gateway. Scan for the adjacent `gateway|daemon start` pair anywhere in args. Test pinning the leading-flags case. --- src/agents/openclaw.ts | 25 +++++++++++++++++-------- test/agents/openclaw.test.ts | 11 +++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index ee32017..ee3d683 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -118,6 +118,18 @@ async function unconfigure(): Promise { } } +function isDaemonInvocation(args: string[]): boolean { + for (let i = 0; i < args.length - 1; i++) { + if ( + (args[i] === "gateway" || args[i] === "daemon") && + args[i + 1] === "start" + ) { + return true; + } + } + return false; +} + async function spawn(args: string[], routing: OpperRouting): Promise { // OpenClaw is a gateway/daemon, not an interactive REPL. Default to // `gateway start` — installs/starts the background service via @@ -130,15 +142,12 @@ async function spawn(args: string[], routing: OpperRouting): Promise { // opper launch openclaw -- agent --local -m "summarise ..." // opper launch openclaw -- gateway run # foreground if you // # really want it - // Detect daemon launches by subcommand, not arg count: `opper launch - // openclaw` (no args, defaults to `gateway start`) AND `opper launch - // openclaw -- gateway start` / `-- daemon start` all detach a - // long-lived service that outlives spawnSync. + // Detect daemon launches by subcommand, not arg count. Scan for the + // adjacent pair `gateway start` / `daemon start` anywhere in args + // because OpenClaw allows global flags before the subcommand + // (e.g. `opper launch openclaw -- --profile dev gateway start`). const finalArgs = args.length === 0 ? ["gateway", "start"] : args; - const isDaemonStart = - finalArgs.length >= 2 && - (finalArgs[0] === "gateway" || finalArgs[0] === "daemon") && - finalArgs[1] === "start"; + const isDaemonStart = isDaemonInvocation(finalArgs); // Snapshot/restore only fits one-shot synchronous invocations. For the // daemon path, `spawnSync` returns as soon as the gateway detaches — diff --git a/test/agents/openclaw.test.ts b/test/agents/openclaw.test.ts index b3e9a59..04af576 100644 --- a/test/agents/openclaw.test.ts +++ b/test/agents/openclaw.test.ts @@ -213,6 +213,17 @@ describe("openclaw adapter", () => { expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); }); + it("spawn does NOT snapshot when daemon-start follows global flags", async () => { + // OpenClaw accepts global flags before the subcommand: + // `opper launch openclaw -- --profile dev gateway start` + // The daemon detection has to find the pair anywhere in args, not + // just at index 0. + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["--profile", "dev", "gateway", "start"], ROUTING); + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + it("spawn forwards user-supplied args verbatim", async () => { spawnSyncMock.mockReturnValue({ status: 0 }); await openclaw.spawn!(["agent", "--local"], ROUTING); From 1d3067c3edd43637c8e468f49e24f31f965f696d Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:43:56 +0200 Subject: [PATCH 8/9] fix(launch): treat openclaw `gateway|daemon restart` as daemon launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `restart` shares the daemon-lifecycle surface with `start` — both end with a long-lived service detached from spawnSync. The previous detection only matched `start`, so `opper launch openclaw -- gateway restart` fell into the snapshot path and reverted the just-written session URL while the restarted daemon was coming up. `stop` / `status` / `logs` are all synchronous and stay on the snapshot path. --- src/agents/openclaw.ts | 5 ++++- test/agents/openclaw.test.ts | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/agents/openclaw.ts b/src/agents/openclaw.ts index ee3d683..4029dbd 100644 --- a/src/agents/openclaw.ts +++ b/src/agents/openclaw.ts @@ -119,10 +119,13 @@ async function unconfigure(): Promise { } function isDaemonInvocation(args: string[]): boolean { + // `start` and `restart` both launch a long-lived service that + // outlives spawnSync. `stop` / `status` / `logs` return synchronously + // and are safe to snapshot/restore around. for (let i = 0; i < args.length - 1; i++) { if ( (args[i] === "gateway" || args[i] === "daemon") && - args[i + 1] === "start" + (args[i + 1] === "start" || args[i + 1] === "restart") ) { return true; } diff --git a/test/agents/openclaw.test.ts b/test/agents/openclaw.test.ts index 04af576..04064d6 100644 --- a/test/agents/openclaw.test.ts +++ b/test/agents/openclaw.test.ts @@ -224,6 +224,20 @@ describe("openclaw adapter", () => { expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); }); + it("spawn does NOT snapshot for `gateway restart` (same lifecycle as start)", async () => { + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["gateway", "restart"], ROUTING); + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + + it("spawn does NOT snapshot for `daemon restart`", async () => { + spawnSyncMock.mockReturnValue({ status: 0 }); + await openclaw.spawn!(["daemon", "restart"], ROUTING); + const models = readModels(sandbox); + expect(models.providers?.opper?.baseUrl).toBe(SESSION_URL); + }); + it("spawn forwards user-supplied args verbatim", async () => { spawnSyncMock.mockReturnValue({ status: 0 }); await openclaw.spawn!(["agent", "--local"], ROUTING); From f98173b861a51a6bedfa31ceabefe2437d53d57f Mon Sep 17 00:00:00 2001 From: Johnny Chadda Date: Fri, 8 May 2026 11:55:00 +0200 Subject: [PATCH 9/9] fix(launch): malformed-sentinel data loss + orphaned $schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues codex flagged on the previous pass: 1. Codex sentinel restore used indexOf-FIRST to strip the block. When the pre-launch config had a stale unclosed `# >>> opper-cli >>>` marker (e.g. a partial manual edit), the strip would cross-match that stale opener with our newly-appended closer and delete every line in between — silently losing user data. Add lastIndexOf-based `extractLastOpperBlock` and `stripLastOpperBlock`, used in the snapshot capture and restore paths. The legacy first-match helpers stay for unconfigure (separate concern). 2. The opencode template writes `$schema` alongside `provider.opper` and `model`. The narrow snapshot only covered the latter two, so a fresh first launch left a `{"$schema": "..."}` file behind. Add `["$schema"]` to the keypaths so the launch is truly ephemeral. Adds regression tests for both: codex pre-existing stale opener keeps unrelated user content intact; fresh opencode launch with template's three top-level keys leaves no file behind. --- src/agents/codex.ts | 35 ++++++++++++++++++++++++++++++++-- src/agents/opencode.ts | 10 ++++++---- test/agents/codex.test.ts | 37 ++++++++++++++++++++++++++++++++++++ test/agents/opencode.test.ts | 12 ++++++------ 4 files changed, 82 insertions(+), 12 deletions(-) diff --git a/src/agents/codex.ts b/src/agents/codex.ts index 9676022..20a26dc 100644 --- a/src/agents/codex.ts +++ b/src/agents/codex.ts @@ -67,6 +67,30 @@ function extractOpperBlock(text: string): string | null { return text.slice(startIdx, endIdx + SENTINEL_CLOSE.length); } +// Snapshot/restore variants that target the LAST opener-closer pair in +// the file. `writeOpperBlock` always appends our block at the end, so +// post-spawn the well-formed block is the last one. Using indexOf-first +// for restore would cross-match a stale unclosed opener with our new +// closer and strip user data between them. +function extractLastOpperBlock(text: string): string | null { + const startIdx = text.lastIndexOf(SENTINEL_OPEN); + if (startIdx === -1) return null; + const endIdx = text.indexOf(SENTINEL_CLOSE, startIdx); + if (endIdx === -1) return null; + return text.slice(startIdx, endIdx + SENTINEL_CLOSE.length); +} + +function stripLastOpperBlock(text: string): string { + const startIdx = text.lastIndexOf(SENTINEL_OPEN); + if (startIdx === -1) return text; + const endIdx = text.indexOf(SENTINEL_CLOSE, startIdx); + if (endIdx === -1) return text; + const before = text.slice(0, startIdx).replace(/\n$/, ""); + const after = text.slice(endIdx + SENTINEL_CLOSE.length).replace(/^\n/, ""); + if (before.length === 0) return after; + return `${before}\n${after}`; +} + async function detect(): Promise { const path = await which("codex"); if (!path) return { installed: false }; @@ -163,7 +187,10 @@ async function spawn(args: string[], routing: OpperRouting): Promise { let opperBlockBefore: string | null = null; if (fileExistedBefore) { try { - opperBlockBefore = extractOpperBlock(readFileSync(cfg, "utf8")); + // Use lastIndexOf so a stale unclosed opener earlier in the file + // doesn't cause us to capture (and later restore) unrelated user + // content as part of the "block". + opperBlockBefore = extractLastOpperBlock(readFileSync(cfg, "utf8")); } catch { opperBlockBefore = null; } @@ -192,7 +219,11 @@ async function restoreOpperBlock( ): Promise { try { const current = existsSync(cfg) ? readFileSync(cfg, "utf8") : ""; - const stripped = stripOpperBlock(current); + // Use lastIndexOf-based strip: writeOpperBlock appended our block + // at the end, so stripping the LAST opener-closer pair removes + // exactly what we wrote. indexOf-first would cross-match a stale + // unclosed opener with our new closer and erase user data between. + const stripped = stripLastOpperBlock(current); let next: string; if (blockBefore === null) { next = stripped; diff --git a/src/agents/opencode.ts b/src/agents/opencode.ts index 1be19f5..fe98fd0 100644 --- a/src/agents/opencode.ts +++ b/src/agents/opencode.ts @@ -156,14 +156,16 @@ async function spawn( } // User-scope: snapshot the Opper-owned keys. The template writes - // both `provider.opper` AND a top-level `model: "opper/..."` — narrow - // restore must cover both, otherwise a fresh first launch leaves an - // orphaned `model` pointing at a removed provider. OpenCode mutates + // `provider.opper`, a top-level `model: "opper/..."`, AND a top-level + // `$schema` — all three need narrow restore. Without `$schema`, a + // fresh first launch leaves a `{"$schema": "..."}` file behind even + // though it's meant to be ephemeral. Without `model`, an orphaned + // `model: "opper/..."` points at a removed provider. OpenCode mutates // sibling keys (theme, MCP servers, …) during a session — those are // outside our keyPaths and survive the restore. return withJsonKeys( opencodeConfigPath("global"), - [["provider", "opper"], ["model"]], + [["provider", "opper"], ["model"], ["$schema"]], async () => { await configureOpenCode({ location: "global", overwrite: true }); diff --git a/test/agents/codex.test.ts b/test/agents/codex.test.ts index 57348c3..83f9524 100644 --- a/test/agents/codex.test.ts +++ b/test/agents/codex.test.ts @@ -272,6 +272,43 @@ describe("codex adapter", () => { expect(readFileSync(cfgPath, "utf8")).toBe(before); }); + it("spawn restore does not destroy user data when the pre-launch config has a stale unclosed sentinel marker", async () => { + // Repro for the malformed-config case: a partial manual edit left + // a `# >>> opper-cli >>>` opener with no matching closer. Without + // lastIndexOf-based strip, our restore would treat the stale opener + // and our newly-written closer as a single block and erase everything + // between them — including unrelated user content. + const cfgDir = join(sandbox, ".codex"); + const cfgPath = join(cfgDir, "config.toml"); + mkdirSync(cfgDir, { recursive: true }); + writeFileSync( + cfgPath, + "# >>> opper-cli >>>\n# unclosed leftover from a partial edit\n[settings]\ntheme = \"dark\"\n", + "utf8", + ); + const before = readFileSync(cfgPath, "utf8"); + + spawnSyncMock.mockReturnValue({ status: 0 }); + await codex.spawn!([], { + baseUrl: SESSION_URL, + apiKey: "k", + model: "m", + compatShape: "openai", + }); + + // The user's [settings] section MUST still be there. + const after = readFileSync(cfgPath, "utf8"); + expect(after).toContain("[settings]"); + expect(after).toContain('theme = "dark"'); + // Our newly-written block has been removed (no SESSION_URL in the + // restored file). The stale unclosed opener stays in place — we + // never touched it. + expect(after).not.toContain(SESSION_URL); + // before may or may not equal after — the key invariant is that + // user content is preserved. + void before; + }); + it("spawn restore preserves user edits outside the sentinel block made mid-spawn", async () => { // Anything outside the SENTINEL_OPEN/CLOSE markers is the user's // own config (theme, settings, etc.). The narrow restore must diff --git a/test/agents/opencode.test.ts b/test/agents/opencode.test.ts index 60231d6..a473408 100644 --- a/test/agents/opencode.test.ts +++ b/test/agents/opencode.test.ts @@ -294,12 +294,11 @@ describe("opencode adapter", () => { ); }); - it("user scope: restore removes orphaned `model: opper/...` when no opper provider existed before", async () => { - // The exact regression Codex flagged: a fresh first launch with no - // prior opencode.json. The template writes provider.opper AND a - // top-level model: "opper/...". After restore, both must be gone — - // an orphaned `model` pointing at a removed provider would break - // direct `opencode` runs after the launch. + it("user scope: a fresh first launch leaves no opencode.json behind (covers $schema, provider.opper, model)", async () => { + // The template (data/opencode.json) writes THREE Opper-owned + // top-level keys: $schema, provider.opper, model. Narrow restore + // must cover all three — otherwise a fresh first launch leaves + // orphan state behind and the launch isn't truly ephemeral. const cfg = opencodeConfigPath(sandbox); expect(existsSync(cfg)).toBe(false); configureOpenCodeMock.mockImplementation(async () => { @@ -307,6 +306,7 @@ describe("opencode adapter", () => { writeFileSync( cfg, JSON.stringify({ + $schema: "https://opencode.ai/config.json", provider: { opper: { npm: "@ai-sdk/openai-compatible",