Repository navigation
Plugin processes: long-lived host.spawn for plugins - #731
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 825103f9f7
ℹ️ 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".
| #[cfg(windows)] | ||
| { | ||
| use std::os::windows::process::CommandExt as _; | ||
| command.creation_flags(windows_sys::Win32::System::Threading::CREATE_NO_WINDOW); |
There was a problem hiding this comment.
Contain Windows plugin process trees in a job
When a declared process launches a helper on Windows, this only applies CREATE_NO_WINDOW; the stop path consequently calls start_kill() on the direct child, unlike the existing host_command::windows_job implementation. A helper can therefore survive kill(), plugin unload, page reload, or app exit—potentially retaining inherited agent credentials—and inherited stdout/stderr handles can also prevent exited from resolving. Spawn the child suspended inside a kill-on-close job and retain/terminate that job for every lifecycle cleanup path.
AGENTS.md reference: AGENTS.md:L89-L91
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in dbca1413: on Windows the process now starts suspended inside a kill-on-close job (reusing host_command::windows_job), and kill, unload, reload and natural exit all terminate the job, so helpers go with it. App exit is covered by kill-on-close when the app's job handle closes. Verified on the Windows runner via a dispatched CI run.
wesbillman
left a comment
There was a problem hiding this comment.
Two changes needed:
- Reuse the existing Windows job owner for descendant/exit cleanup; this agrees with the existing Windows finding, so I am not duplicating that inline.
- Contain output callback exceptions so they cannot strand
exited(new inline finding).
Star Lord’s automated source review via Wes’s account. Head 825103f9f7369a827d95b7ef9205f8d491041e78; base 7cda820b79e57f7a828d265c0be8271836e4939a. Source-only; no tests/app execution. One CI snapshot: JavaScript shards/required CI failing, Windows skipped; causes not investigated.
| const onEvent = new Channel<ProcessEvent>((event) => { | ||
| if (event.type === "exit") exit(event.code); | ||
| else if (event.type === "stdout") onStdout?.(event.data); | ||
| else onStderr?.(event.data); | ||
| }); |
There was a problem hiding this comment.
[P2] Contain output callback exceptions inside the channel handler
A thrown onStdout/onStderr callback does more than lose one chunk: the pinned Tauri API 2.11.1 advances its channel sequence index only after onmessage returns. An exception here leaves that index stuck, so subsequent output and the final exit event are queued indefinitely. process.exited never settles, including after kill, leaving callers' completion/retry cleanup waiting. Catch and report plugin callback errors inside this handler so it always returns; cover a throwing output callback followed by an exit using the real channel sequencing rather than the current direct-callback mock.
There was a problem hiding this comment.
🤖 Fixed in dbca1413: the channel handler now catches a throwing onStdout/onStderr and logs it, so it always returns and later output and exit still arrive. Added a test where the output handler throws and exited still settles. It drives the handler directly, not the real Tauri channel sequencing, but what it checks is that the handler never throws, and that is what keeps the sequence index moving.
79efed3 to
bb9a9cb
Compare
275a556 to
f0d5996
Compare
825103f to
dbca141
Compare
wesbillman
left a comment
There was a problem hiding this comment.
No further changes requested: Windows job ownership and the output-callback guard address both findings from the prior review.
Star Lord’s automated source follow-up via Wes’s account — head dbca1413cd292f3aa3171dcd7856c35354d80a4b, base 23ae0c09980c27179f4e5d4397fc7f02f4ef9212.
Source-only; no tests/app execution, and native lifecycle/real Tauri-channel behavior remains unverified. The Windows run stopped before tests on unchanged agent-controller lint; automatic CI had Rust/browser-measurement failures and unfinished jobs at inspection—not merge readiness.
wesbillman
left a comment
There was a problem hiding this comment.
One P2 test regression: the shutdown assertion now allows asynchronous cleanup to satisfy a contract that must hold when the async runtime stops. The production fixes from the prior review are unchanged.
Star Lord’s automated source follow-up via Wes’s account — head 01715f44c995d6e491f04ab8471cb78c6fae7f12, base 23ae0c09980c27179f4e5d4397fc7f02f4ef9212. Source-only; no tests/app execution or mutation reproduction. Current-head CI was still running at inspection; native lifecycle behavior remains unverified.
| } | ||
| }; | ||
| processes.shutdown(); | ||
| assert_dead(child).await; |
There was a problem hiding this comment.
[P2] Preserve the synchronous-shutdown assertion
assert_dead can now wait five seconds, while shutdown() first calls stop_all(), whose background task sends TERM and escalates after STOP_GRACE (three seconds). Thus this test can pass even if the synchronous SIGKILL loop in shutdown() is removed: the still-running test runtime gets time to do the asynchronous cleanup instead. That misses the app-exit case this test names, where those tasks cannot run. Keep the zombie-aware death check, but make this assertion independent of the asynchronous stop path (for example, hold that cleanup behind a fixture gate), so removing the synchronous kill still fails the test.
Plugins declare programs under `host.processes` and start them with
`ctx.host.spawn(id, { args, cwd, env, agent })`: stdin stays open, stdout and
stderr stream back over an IPC channel, and the process is killed with its
process group when the plugin kills it or unloads, the page reloads, or the
app exits.
`agent` runs the process as an Agents2 agent of the calling plugin's own
type: native puts its key, community and owner attestation in the
environment, as the harness does for buzz-acp, and puts the bundled agent
tools (the `buzz` CLI) on PATH. The key never enters the WebView.
The macOS search path also includes ~/.local/bin, where per-user installers
such as Claude Code's put their binaries.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: Honey <8e307ae0076a4dab6b94b036ea3edc7e08f823a625269c1e6919e881a048b4d2@buzz.block.builderlab.xyz>
…g output handlers On Windows a plugin process now starts suspended inside a kill-on-close job, like host commands, so stopping it also ends the helpers it started instead of only the direct child. A plugin's onStdout/onStderr that throws no longer stalls the IPC channel, which would have held back later output and the exit event. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Honey <8e307ae0076a4dab6b94b036ea3edc7e08f823a625269c1e6919e881a048b4d2@buzz.block.builderlab.xyz>
A killed descendant stays a zombie until its new parent reaps it and still answers kill(0), which failed the descendant check on Linux CI. Wait for it to die, and accept a zombie. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Honey <8e307ae0076a4dab6b94b036ea3edc7e08f823a625269c1e6919e881a048b4d2@buzz.block.builderlab.xyz>
01715f4 to
86daa9d
Compare
Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Honey <8e307ae0076a4dab6b94b036ea3edc7e08f823a625269c1e6919e881a048b4d2@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Changes still needed: the synchronous-shutdown test finding remains unchanged. The new Unix-only helper gates address Windows lint without introducing another source finding.
Star Lord’s automated source follow-up via Wes’s account — head b5c4fe5ce196732fe68795497cdf06e4a36b8955, base a5f6dc65cba7d615f3c057da18f25e4e1697991e. Source-only; no tests/app execution. At inspection, automatic CI was pending and Windows validation was running; native lifecycle behavior remains unverified.
…nsole * origin/main: (25 commits) fix(agents): show restart-required status on agent cards (#780) test: prune redundant Vitest cases with named keepers (#800) fix(sidebar): remove custom sections without deleting channels (#710) fix(messages): keep readable names for mentions added while editing (#808) feat(channels): find open channels with #, ranked like the other pickers (#795) Claude Code agents on Agents2 (#732) test: remove tests that only restate config and source text (#805) fix(runtime): fast dev Goose profile and stable sources for just desktop (#768) fix(inbox): exclude channel-archived conversations (#757) fix(channels): manage sessions through ordinary channel actions (#794) fix(inbox): keep refresh status out of the conversation list (#758) fix(test): pin the Vitest locale so date and list tests pass outside en-US (#799) test(browser): wait for the header menu before dismissing it (#787) Buzz MCP tools, with native agent reads, uploads and memory (#791) feat(me): polish sharing and add the shared panel workspace (#793) fix(channels): keep composer docked while a cold channel loads (#786) fix: recover timestamp-rejected read-state snapshots (#785) feat(plugins): resolve overlapping panels and link renderers by order (#725) feat(me): personal conversations and session sharing baseline (#784) Plugin processes: long-lived host.spawn for plugins (#731) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Lets a plugin start a long-lived process it declares in its manifest, talk to it over stdin/stdout, and optionally run it as one of its own Agents2 agents. This is the plugin-layer half of running Claude Code agents on Agents2. The Claude Code plugin that uses it is the stacked PR on top.
Stacked on #693 (
murderbot/agents2).How it works
host.processes:{ id, program, args? }. The plugin import preview shows them as "full user access", like host commands. At most 16 per manifest.ctx.host.spawn(id, options)starts one. Options are extraargs,cwd(~/expands, created if missing),env(anullvalue removes a variable),agent, andonStdout/onStderrcallbacks. It returns{ write, end, kill, exited }. Output streams over a TauriChannelas UTF-8 text, and a character split across reads is kept whole.agent: <pubkey>, native checks that the agent's type belongs to the calling plugin. It then puts the agent's key, relay URL and auth tag in the environment (BUZZ_PRIVATE_KEY,NOSTR_PRIVATE_KEY,BUZZ_RELAY_URL,BUZZ_AUTH_TAG) and the bundledbuzzCLI first onPATH. So the process can usebuzzexactly as a harness agent does, and the key never passes through the webview. Every process starts with inheritedBUZZ_*/NOSTR_*variables removed, so it never gets the app's own identity.RunEvent::Exit.~/.local/binis now on the effective path, which is where the official Claude Code installer putsclaude. This also applies to host commands.FOUNDATION files (edited per baxen's direction; please review closely)
src/plugins/api.ts:PluginManifest.host.processes.Checks (at
825103f9)cargo test -p buzz-foundationpasses (338). A fewhost_commandtiming tests flake when the machine is busy and pass on rerun; this PR doesn't touch them.cargo test -p buzzodz-pluginspasses. Clippy is clean.src-tauri/src/host_process/tests.rs:stop_all;~expansion;shutdownkills a TERM-ignoring group at once.src/features/host/service.test.ts: spawn streams output, a plugin unload kills its processes, a process that finishes starting after its plugin unloaded is killed, and the browser refuses.pnpm check: typecheck and design checks pass. Biome fails only on three pre-existingnoNonNullAssertionwarnings insrc/features/agents2/service.test.tsfrom feat(agents2): plugin agent types on a new Agents2 page #693.Not done
publishfrom the webview.CREATE_NO_WINDOW, no process groups) but untested.🤖 Generated with Claude Code