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/agents/codex.ts b/src/agents/codex.ts index 8038849..20a26dc 100644 --- a/src/agents/codex.ts +++ b/src/agents/codex.ts @@ -12,6 +12,8 @@ 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"; @@ -57,6 +59,38 @@ 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); +} + +// 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 }; @@ -140,19 +174,79 @@ 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. + // 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); + // 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 { + // 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; + } + } + await writeOpperBlock(routing.baseUrl); + try { + 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; + } finally { + await restoreOpperBlock(cfg, opperBlockBefore, fileExistedBefore); + } +} - 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; +async function restoreOpperBlock( + cfg: string, + blockBefore: string | null, + fileExistedBefore: boolean, +): Promise { + try { + const current = existsSync(cfg) ? readFileSync(cfg, "utf8") : ""; + // 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; + } 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 d979726..4029dbd 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 { withJsonKeys } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -117,11 +118,22 @@ 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); +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] === "restart") + ) { + 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 // launchd/systemd, returns quickly, and the gateway keeps serving @@ -133,18 +145,48 @@ 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", - ); + // 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 = isDaemonInvocation(finalArgs); + + // 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; + + // 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 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; + }); } export const openclaw: AgentAdapter = { diff --git a/src/agents/opencode.ts b/src/agents/opencode.ts index 60fb3ad..fe98fd0 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 { withJsonKeys } from "../util/config-snapshot.js"; import { brand } from "../ui/colors.js"; import type { AgentAdapter, @@ -101,47 +103,96 @@ 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. + // + // 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 }); - } else { - 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`, - ), - ); + 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"); } } - // Rewrite the baseURL to the per-session URL on every launch so - // generations land on the right session. - await setSessionBaseUrl(routing.baseUrl, location); + // User-scope: snapshot the Opper-owned keys. The template writes + // `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"], ["$schema"]], + async () => { + await configureOpenCode({ location: "global", overwrite: true }); - const env: NodeJS.ProcessEnv = { - ...process.env, - OPPER_API_KEY: routing.apiKey, - }; - const result = spawnSync("opencode", args, { stdio: "inherit", env }); - return result.status ?? -1; + // 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"); + + 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..b332b00 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 { withJsonKeys } from "../util/config-snapshot.js"; import type { AgentAdapter, ConfigureOptions, @@ -120,21 +121,25 @@ 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 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 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 + // 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/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( + path: string, + keyPaths: string[][], + fn: () => Promise, +): Promise { + 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); + 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 { + return await fn(); + } finally { + await restore(path, keyPaths, valuesBefore, 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; +} + +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; +} + +// 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, + keyPaths: string[][], + valuesBefore: unknown[], + fileExistedBefore: boolean, + beforeMode: number | undefined, +): Promise { + try { + const after = readJsonOrEmpty(path); + 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 }); + return; + } + + await mkdir(dirname(path), { recursive: true }); + 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: ${ + err instanceof Error ? err.message : String(err) + }\n`, + ); + } +} diff --git a/test/agents/codex.test.ts b/test/agents/codex.test.ts index 7f25d1b..83f9524 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,130 @@ 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); + }); + + 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 + // 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 873f2f4..04064d6 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"; @@ -77,22 +77,90 @@ 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", + ); + 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", ); - expect(raw).not.toContain('"baseUrl": "https://api.opper.ai/v3/compat"'); + 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 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 () => { @@ -117,6 +185,59 @@ 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 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 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 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); diff --git a/test/agents/opencode.test.ts b/test/agents/opencode.test.ts index 30a65b5..a473408 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,178 @@ 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 = JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8")); + + await opencode.spawn!([], ROUTING); + + 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 () => { + // 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 = JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8")); + + spawnSyncMock.mockReturnValue({ status: 17 }); + const code = await opencode.spawn!([], ROUTING); + expect(code).toBe(17); + expect(JSON.parse(readFileSync(opencodeConfigPath(sandbox), "utf8"))).toEqual(before); + }); + + 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); + const seedPath = opencodeConfigPath(sandbox); + const seeded = JSON.parse(readFileSync(seedPath, "utf8")) as { + provider: Record; + [k: string]: unknown; + }; + seeded.theme = "dark"; + seeded.model = "opper/claude-opus-4-7"; + 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 = "opper/claude-haiku-4-5"; + 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; + }; + // Non-Opper-owned siblings survive. + expect(after.theme).toBe("light"); + expect(after.mcpServers).toEqual({ fs: { command: "mcp-fs" } }); + // 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: 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 () => { + mkdirSync(join(sandbox, ".config", "opencode"), { recursive: true }); + writeFileSync( + cfg, + JSON.stringify({ + $schema: "https://opencode.ai/config.json", + 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 () => { @@ -205,6 +359,153 @@ 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", + ); + // 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 }); + + 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..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"; @@ -76,31 +76,103 @@ 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 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. + 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 +181,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/commands/agents.test.ts b/test/commands/agents.test.ts index 97b1a3c..d851729 100644 --- a/test/commands/agents.test.ts +++ b/test/commands/agents.test.ts @@ -61,7 +61,7 @@ describe("agentsListCommand", () => { }); }); -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"); diff --git a/test/util/config-snapshot.test.ts b/test/util/config-snapshot.test.ts new file mode 100644 index 0000000..57ce223 --- /dev/null +++ b/test/util/config-snapshot.test.ts @@ -0,0 +1,277 @@ +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 { withJsonKeys } from "../../src/util/config-snapshot.js"; + +function readJson(path: string): Record { + return JSON.parse(readFileSync(path, "utf8")); +} + +describe("withJsonKeys", () => { + let sandbox: string; + + beforeEach(() => { + sandbox = mkdtempSync(join(tmpdir(), "opper-snap-")); + }); + afterEach(() => { + rmSync(sandbox, { recursive: true, force: true }); + }); + + 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 withJsonKeys(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 withJsonKeys(path, [["providers", "opper"]], async () => { + writeFileSync( + path, + JSON.stringify( + { other: "stuff", providers: { opper: { baseUrl: "x" } } }, + null, + 2, + ), + "utf8", + ); + }); + + expect(readJson(path)).toEqual({ other: "stuff" }); + }); + + 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 withJsonKeys(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"); + }); + + 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("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 withJsonKeys(path, [["providers", "opper"]], async () => { + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "x" } } }, null, 2), + "utf8", + ); + }); + + expect(existsSync(path)).toBe(false); + }); + + it("keeps the file when it didn't exist before but the agent added other content", async () => { + const path = join(sandbox, "models.json"); + + await withJsonKeys(path, [["providers", "opper"]], async () => { + writeFileSync( + path, + JSON.stringify( + { + providers: { + opper: { baseUrl: "x" }, + ollama: { baseUrl: "y" }, + }, + }, + null, + 2, + ), + "utf8", + ); + }); + + expect(readJson(path)).toEqual({ + providers: { ollama: { baseUrl: "y" } }, + }); + }); + + 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( + withJsonKeys(path, [["providers", "opper"]], async () => { + writeFileSync( + path, + JSON.stringify( + { providers: { opper: { baseUrl: "session" } } }, + null, + 2, + ), + "utf8", + ); + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + + expect(readJson(path)).toEqual({ + providers: { opper: { baseUrl: "compat" } }, + }); + }); + + it("preserves the original file mode", async () => { + const path = join(sandbox, "secret.json"); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "compat" } } }), + { mode: 0o600 }, + ); + expect(statSync(path).mode & 0o777).toBe(0o600); + + await withJsonKeys(path, [["providers", "opper"]], async () => { + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "session" } } }), + "utf8", + ); + chmodSync(path, 0o644); + }); + + 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 }); + const path = join(dir, "models.json"); + writeFileSync( + path, + JSON.stringify({ providers: { opper: { baseUrl: "keep" } } }), + "utf8", + ); + + await withJsonKeys(path, [["providers", "opper"]], async () => { + rmSync(dir, { recursive: true, force: true }); + }); + + expect(readJson(path)).toEqual({ + providers: { opper: { baseUrl: "keep" } }, + }); + }); +});