fix(packaged): keep agent daemon-CLI invocations out of the single-instance gate - #7388
fix(packaged): keep agent daemon-CLI invocations out of the single-instance gate#7388lorenzozanee wants to merge 1 commit into
Conversation
…stance gate Agent tool calls that exec the packaged binary ($env:OD_BIN tools ..., export ...) entered main() like any GUI launch: past the --headless fast path nothing distinguished them, so they ran inspectExistingDesktopForLauncher() and, whenever the running desktop reported windowVisible:false, killed and restarted it as a headless owner. Each agent artifact/export call could therefore restart the app the user was using — restarts landed within ~0.2s of every OD_BIN call, and on 0.19.2 the same path escalated into an EPIPE hang. Classify these invocations at the packaged entry instead: when argv[1] is the configured daemon CLI entry (or matches its bundled basename, daemon-cli.mjs/cli.js), re-spawn process.execPath as Node with ELECTRON_RUN_AS_NODE=1, inherited stdio, and the child exit code, so the daemon CLI runs without touching the launcher gate. Environment alone never routes: agents legitimately launching the desktop inherit run-scoped OD_TOOL_TOKEN/ELECTRON_RUN_AS_NODE, so neither may decide. Also pass ELECTRON_RUN_AS_NODE through the Codex shell-environment include list next to OD_NODE_BIN, so node-mode children survive that boundary too.
|
Thanks @lorenzozanee — the regression write-up is very clear, and the split between the packaged launcher gate and the daemon-CLI respawn path is easy to follow. I’ve queued the reviewer routing and PR checks on our side. |
lefarcen
added
size/M
lefarcen
added
the
needs-validation
|
🧪 Queued for QA validation — this PR changes a user-facing runtime path, so we’re marking it for a manual QA pass before merge. Nothing needed from you right now; we’ll update here once it reaches the validation step. Thanks for the contribution! 🙏 |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@lorenzozanee, I reviewed all six changed files and the scoped tests. The argv-only predicate and Codex environment allowlist are thoughtfully covered, but the new subprocess boundary has two correctness issues in packaged runtime lifecycle (macOS helper selection and signal exit propagation), so I am requesting changes before merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // single-instance gate. Re-spawn in Electron-as-Node mode (same pattern as | ||
| // the packaged sidecar spawn env) with inherited stdio and exit code. | ||
| if (isAgentToolInvocation(process.argv, { daemonCliEntry: config.daemonCliEntry })) { | ||
| const child = spawn(process.execPath, process.argv.slice(1), { |
There was a problem hiding this comment.
Use the packaged Electron-as-Node command resolver here instead of process.execPath directly. apps/packaged/src/sidecars.ts already exports resolvePackagedElectronNodeCommand() and deliberately selects the hidden App Helper executable on macOS (the existing Dock-identity fix) before setting ELECTRON_RUN_AS_NODE. This new path bypasses that contract, so macOS agent calls can start the main app executable and recreate the Dock/GUI identity or focus side effect that this PR is intended to remove. Await that resolver (or extract a shared command helper) before spawning, and add a macOS helper-selection assertion for this agent path.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| stdio: "inherit", | ||
| windowsHide: true, | ||
| }); | ||
| child.on("exit", (code) => app.exit(code ?? 0)); |
There was a problem hiding this comment.
Preserve a non-zero result for signal-terminated or failed child processes. Node reports code=null when the CLI child is killed by SIGINT or SIGTERM, but code ?? 0 maps that cancellation to success, so an interrupted agent tools call can be reported as a successful invocation. Use the close event (or inspect signal) and map code === null or spawn errors to a non-zero exit while preserving numeric codes.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Hey @lorenzozanee — following up here: the current blockers are already covered in @nettee's review/comments on this head, specifically the macOS helper-command path and the exit/signal propagation on the respawned child process. Once those are addressed, a re-review on the same two points should unblock the PR. 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
3 participants
Fixes #7154
Why
The built-in agent runs live-artifact tool calls by exec'ing the packaged binary itself (
$env:OD_BIN tools …,export …). Those launches booted the packaged Electron entry and fell intoinspectExistingDesktopForLauncher()after the--headlessfast path. When the running desktop reportedwindowVisible: false, the gate classified it as a headless owner and killed + restarted it (action=restart reason=headless-owner) — each artifact/export call could restart the app mid-use; with the window visible it merely focus-flashed. On 0.19.2 the same restart path escalated into an EPIPE hang. Root cause: nothing distinguishes argv-shaped daemon-CLI invocations from a user double-launch at the entry.What users will see
Agent live-artifact register/verify/export calls no longer restart or focus-flash the running desktop. The invocations still execute against the daemon; GUI launches, deeplinks, file-association opens, and updater/launcher handoffs behave exactly as before.
Surface area
daemon-cli.mjs/cli.js) re-spawn as Node (ELECTRON_RUN_AS_NODE=1) instead of entering the launcher gate; environment alone never routesScreenshots
Not applicable — packaged process routing only, no UI.
Bug fix verification
apps/packaged/tests/agent-tool-invocation.test.ts(13 cases): red on base (module absent), green on branch.apps/daemon/tests/runtimes/codex-resume-args.test.ts(ELECTRON_RUN_AS_NODEin compiledinclude_only): red on base, green on branch.apps/packaged/tests/launcher-after-quit.test.ts(windowVisible:true→[STATUS×3, SHOW], SHUTDOWN provably absent): coverage hardening; passed on base too, not claimed as regression proof.index.tsimports electron, unrunnable under node vitest) — please eyeball that the guard sits between the headless fast path andinspectExistingDesktopForLauncher(index.ts:133 vs :169) and that the child env setsELECTRON_RUN_AS_NODE: "1". Known residual edges, accepted: a file literally nameddaemon-cli.mjspassed to the desktop exe routes as CLI when no entry path is baked into the build config, and--headlessinvocations keep taking the existing fast path before this guard.Validation
pnpm --filter @open-design/packaged exec vitest run— 23 files / 336 tests passcodex-resume-args9/9pnpm guardclean