Skip to content

feat(rivetkit-agent-os): phase 3 process — 11 arms (exec/spawn/wait/kill/stop/list/all/tree/get/writeStdin/closeStdin) + ExecResultDto#5197

Open
abcxff wants to merge 1 commit into
stack/test-rivetkit-agent-os-partial-failure-driver-test-for-readfiles-batch-dto-option-content-option-error-pzzzpmtrfrom
stack/feat-rivetkit-agent-os-phase-3-process-11-arms-exec-spawn-wait-kill-stop-list-all-tree-get-writestdin-closestdin-execresultdto-unkrsszt
Open

feat(rivetkit-agent-os): phase 3 process — 11 arms (exec/spawn/wait/kill/stop/list/all/tree/get/writeStdin/closeStdin) + ExecResultDto#5197
abcxff wants to merge 1 commit into
stack/test-rivetkit-agent-os-partial-failure-driver-test-for-readfiles-batch-dto-option-content-option-error-pzzzpmtrfrom
stack/feat-rivetkit-agent-os-phase-3-process-11-arms-exec-spawn-wait-kill-stop-list-all-tree-get-writestdin-closestdin-execresultdto-unkrsszt

Conversation

@abcxff

@abcxff abcxff commented Jun 8, 2026

Copy link
Copy Markdown
Contributor
  • 18/24 driver tests green (spawn+waitProcess, listProcesses, killProcess across all 6 native cells).
  • 6 exec failures all from same root cause: agent-os sidecar's resolve_software uses node_modules/{package} which doesn't traverse pnpm's .pnpm hoisting. Phase 2.5 software mapping gap, not arm bugs.

…ill/stop/list/all/tree/get/writeStdin/closeStdin) + ExecResultDto

- 18/24 driver tests green (spawn+waitProcess, listProcesses, killProcess across all 6 native cells).
- 6 exec failures all from same root cause: agent-os sidecar's resolve_software uses node_modules/{package} which doesn't traverse pnpm's .pnpm hoisting. Phase 2.5 software mapping gap, not arm bugs.
@abcxff

abcxff commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/rivet

Check out this stack: forklift get https://github.com/rivet-dev/rivet/pull/5197
Pull/update this stack: forklift sync
Publish local edits: forklift submit
Merge when ready: forklift merge

@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Code Review - Phase 3 Process Arms

The implementation cleanly follows the filesystem.rs pattern. Here are the findings:

Correctness / Reliability

exec has no timeout: exec calls vm.exec(command, ExecOptions::default()) with default options. If ExecOptions has no deadline, a hung command blocks the actor indefinitely. Consider adding a configurable timeout arg or documenting that the caller is responsible.

listProcesses is dispatched as infallible: the arm calls list_processes and replies ok with no error path. Only safe if AgentOs::list_processes is genuinely infallible. Worth confirming at the agent_os_client level.

Style Issues

Import inside function body (process.rs, write_process_stdin): StdinInput is imported with a use statement inside the function body. CLAUDE.md says to always add imports at the top of the file. filesystem.rs:139 has the same pre-existing pattern but that does not justify continuing it.

processTree arm is missing outer braces unlike all other arms. Minor inconsistency.

Inline comment "// No args." in the listProcesses arm violates CLAUDE.md guidance to only comment when the WHY is non-obvious.

Known Gaps

6/24 driver tests fail due to pnpm hoisting - the sidecar resolve_software does not traverse pnpm .pnpm hoisting. Well-documented in the PR body and updated TS comment. No action needed here.

Minor Notes

ExecResultDto only renames exit_code to exitCode; stdout and stderr are fine as-is.
spawn being sync is acceptable for a brief fork/exec syscall.
map_err(anyhow::Error::from) is correct where the underlying type does not impl Intoanyhow::Error.

Summary

The two things worth addressing before merge are the exec timeout concern (functional) and the inline import (CLAUDE.md compliance). The missing braces on processTree and the no-args comment are trivial. The 6-test gap is pre-acknowledged and scoped to Phase 2.5, not these arms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant