fix(launch): snapshot agent config to stop session-URL leak; rename agents uninstall → remove#14
fix(launch): snapshot agent config to stop session-URL leak; rename agents uninstall → remove#14
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe06b486cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…session URL 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.
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.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb29302dc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78fab57642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… order
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3660f7574b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 415f1d00d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
OpenClaw allows global flags before the subcommand (`openclaw [--dev] [--profile <name>] <command>`), 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74a9b033dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d3067c3ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
codex/pi/openclaw/opencode(withoutopper launch) used to inherit the previous launch's/v3/session/<sid>/<tags>URL because we were rewriting persistent config files at spawn and never putting them back.withConfigSnapshot(path, fn)(captures bytes + mode or absence, restores infinally, even on non-zero exits and thrown errors) and wraps each affected adapter's rewrite +spawnSyncin it. Hermes and Claude Code already weren't leaking — Hermes runs against an isolatedHERMES_HOME, Claude Code routes via env vars.opper agents uninstall <name>→opper agents remove <name>(the menu option too). The integration command never uninstalled the agent binary — "remove" matches what it actually does and pairs with the implicit "add" that happens on first launch / configure. Skillsuninstallis unrelated and stays.Special cases
gateway startskips the snapshot: the daemon outlivesspawnSyncand ownsmodels.jsonfrom then on, so restoring would either break a running gateway or be cosmetic. Pass-through args (e.g.agent --local) still get the snapshot. Documented in-code.--projectuses a narrower revert —baseURLonly, captured afterconfigureOpenCoderuns — so a checked-in project config keeps its opper provider across launches, and a hand-edited self-hostedbaseURLsurvives unchanged.writeFile'smodeoption is silently ignored when the file already exists, so the helper now follows the write with an explicitchmod— and the test useschmodSyncmid-run instead ofwriteFileSync(... mode)so it actually exercises the code path.Mental model
opper launch <agent>is now ephemeral — it never alters persistent state. If there's no opper block, none gets left behind; if there's a custom one (self-hosted Opper, mixed-provider config, etc.), it survives. The persistent-setup surface lives entirely inopper agents(menu + CLI) — users own whatever's in their agent's config file.A user-visible side effect: anyone who was relying on
opper launchleaving an opper block behind now has to set it up explicitly via the menu oropper agents. Worth a release-note line.Test plan