Repository navigation
feat(process-daemon): protocol v8 for shell commands - #61875
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change looks sound to me. I couldn't run it because there's no Rust toolchain in the review sandbox, so this is a code-only review.
I read the pipe/output-file path, wait/signal/set_metadata, and how unknown or invalid requests are told apart. File open happens before spawn and after all validation, with O_NOFOLLOW and the file closed before the exit is observable. wait covers disconnect, terminate and shutdown. Signalling is refused after reap, so there's no PID-reuse risk. Write and read failures now still publish the exit, with output_error set. The new tests are focused and mostly poll against deadlines instead of sleeping.
Public API: the opencode-pty IPC protocol goes 7 → 8. This is not backwards compatible, and that's intended (a v7 handoff is discarded).
create→create_pty/create_pipe(create_pipetakesoutput_pathandmetadata, and the child gets exactlyenv)listnow requiresmode- added
get,wait,signal,set_metadata TerminalInfo→ProcessInfo:title/user_title/foreground_process/cols/rowsmove underpty; addspipe(output_path,output_lines,output_error,metadata),started_at,exited_at- response fields
terminal/terminals→process/processes, andTerminals→Processes; adds aProcessresponse exitedaddsoutput_lines,output_error,exited_atfeaturesis now required and empty (rename/piperemoved)- unknown ops now get an error reply instead of a dropped connection
OpenCode's client in packages/core/src/persistent-pty/daemon.ts still speaks v7. It is unaffected until @opencode/pty is bumped, which the companion PR will need to handle together with the 0.5.0 release.
The Windows cfg paths haven't been compiled, as you noted, so a Windows CI run before release would be worth it.
There was a problem hiding this comment.
The two new commits look correct. I couldn't compile them because there's no Rust toolchain in the review sandbox, so this is a code-only review.
The macOS EPERM handling only treats EPERM as success once a non-reaping waitid(WNOHANG | WNOWAIT) shows the child has exited. That keeps the zombie in place until terminate, as before, and EPERM from a child that is still running is still reported. The CI smoke-test bump to protocol 8 matches the version change.
The public API is the same as in my previous review; these commits add nothing to the protocol 7 → 8 surface. Nothing new to raise.
3227c38 to
7dc3609
Compare
There was a problem hiding this comment.
This push drops 3227c38 ("expect protocol 8 from the smoke test"). The new head is its parent, so .github/workflows/check-process-daemon.yml (line 206) again expects opencode-pty $version (protocol 7). The binary now prints (protocol 8), so the Windows smoke step will fail. Please restore that commit, unless the workflow change is meant to land in a separate PR before this one.
The macOS EPERM fix is unchanged from what I reviewed last time. I couldn't run anything (no Rust toolchain in the sandbox). The public API is the same as in my previous review (opencode-pty protocol 7 → 8, not backwards compatible).
7dc3609 to
4b82d02
Compare
Bump the protocol to 8 and drop the compatibility code for older peers: no feature flags, required fields, a split create_pty/create_pipe, and List with a required mode. - ProcessInfo (was TerminalInfo) with pty/pipe groups and timestamps. - create_pipe replaces the environment, requires an absolute cwd, an output file (truncated, umask mode) and opaque metadata (1 MiB cap). - The output file is the only buffer: no replay for pipe processes. - output_lines counts newlines that reached the file. - get, wait (one Exited), signal (keeps the record) and set_metadata. - Unknown requests get an error instead of a dropped connection. - A failed pipe reader still publishes the exit.
There was a problem hiding this comment.
This push is a rebase onto a newer dev; the PR's packages/process-daemon changes are identical to what I reviewed last time.
My earlier CI concern is resolved: dev now matches the Windows smoke-step version string with (protocol \d+), so the protocol-8 binary passes and this PR doesn't need the workflow change. I couldn't run anything (no Rust toolchain in the sandbox). The public API is the same as in my previous review (opencode-pty protocol 7 → 8, not backwards compatible). Nothing new to raise.
Issue for this PR
Closes #
Supersedes #60701 (its three commits are included here, rebased).
Type of change
What does this PR do?
Protocol v8 for
opencode-pty, so OpenCode can run its local shell commands in the daemon (pipe mode) and keep them across a planned server restart. This is a breaking bump: a v8 server discards a v7 handoff and starts a fresh daemon, so persistent terminals start fresh once on upgrade.Pipe output file (from #60701, plus changes):
create_pipetakesoutput_path; the reader writes every chunk to it before forwarding. File is truncated on open, default mode (umask), symlinks refused, closed beforeexited.output_erroron the process and onexited.New requests:
get { id }: direct lookup.wait { id }: held open until the process exits, then oneexitedreply. Replies at once if it already exited;error "terminated"if it's terminated first.signal { id, signal }: SIGTERM/SIGINT/SIGKILL to the process group without removing it, sowaitstill gets a normal exit. Safe because the zombie stays unreaped untilterminate.set_metadata { id, metadata }: opaque JSON the server stores with the process (≤ 1 MiB).Daemon-recorded data:
output_lines(newlines actually written to the file, counted in the reader),started_at/exited_at.Cleanup enabled by the bump:
TerminalInfo→ProcessInfo, withpty/pipegroups for mode-specific fields.createsplit intocreate_ptyandcreate_pipe;listrequiresmode.featuresis required and empty); compatibility shims for older peers removed.error "unsupported request: <op>"instead of a dropped connection; bad fields getinvalid <op> request: ….Windows pipe mode is out of scope: on Windows,
create_pipeandsignalreturn errors.The OpenCode side is in #61876. This needs a 0.5.0 release before that one can merge.
How did you verify your code works?
On Linux:
cargo fmt --check,cargo clippy --locked --all-targets --all-features -- -D warnings, andcargo test --locked --lib --test service --test runtime --test ownership --test pipe --test rows --test ghostty-effects. New tests cover the v8 wire format,wait(single/multiple waiters, already exited, terminate and shutdown while waiting, client disconnect),signal(trapped SIGTERM then SIGKILL, background child, after exit, after terminate), output file mode/truncation/symlink, line counting (partial writes, batching, write errors), metadata and timestamps. The OpenCode client's tests also pass against this build.Not run: Windows clippy/tests (the
#[cfg(windows)]paths were updated by reading) and macOS.Checklist
Added by the opencode bot (AI-generated).
Public API: opencode-pty IPC protocol 7 → 8: create split into create_pty/create_pipe, adds get/wait/signal/set_metadata, TerminalInfo → ProcessInfo with pty/pipe groups (not backwards compatible; OpenCode's client must move with the 0.5.0 bump)