Skip to content

feat(rivetkit-agent-os): phase 1a — rust crate skeleton + readFile arm + dispatcher e2e against real sidecar#5190

Open
abcxff wants to merge 1 commit into
stack/feat-rivetkit-json_compat_uint8_array-byte-payload-encoding-parity-with-ts-encode-decode-cross-language-tests-ukssumlnfrom
stack/feat-rivetkit-agent-os-phase-1a-rust-crate-skeleton-readfile-arm-dispatcher-e2e-against-real-sidecar-wrxkrssv
Open

feat(rivetkit-agent-os): phase 1a — rust crate skeleton + readFile arm + dispatcher e2e against real sidecar#5190
abcxff wants to merge 1 commit into
stack/feat-rivetkit-json_compat_uint8_array-byte-payload-encoding-parity-with-ts-encode-decode-cross-language-tests-ukssumlnfrom
stack/feat-rivetkit-agent-os-phase-1a-rust-crate-skeleton-readfile-arm-dispatcher-e2e-against-real-sidecar-wrxkrssv

Conversation

@abcxff

@abcxff abcxff commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@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/5190
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: feat(rivetkit-agent-os): phase 1a

Overview: This PR introduces a new rivetkit-agent-os Rust crate wrapping agent-os-client to wire a VM lifecycle into the RivetKit actor model. It adds AgentOsActor, AgentOsActorConfig, build_core_factory, a readFile action arm, and an E2E test gate against the real sidecar. The structure is clean and the lazy-boot design is well-reasoned.


Issues

1. unsafe { std::env::set_var() } is unsound in async tests

sidecar_available() in tests/dispatcher_e2e.rs mutates the environment inside unsafe while the comment claims "tests run single-process." Both #[tokio::test] functions run on Tokio threads that can be concurrent, and std::env::set_var races with any thread reading env vars. Use std::sync::OnceLock to probe-and-set exactly once without mutation races — no unsafe needed.

2. .context() preferred over anyhow! for wrapping errors

CLAUDE.md: "Prefer .context() over the anyhow! macro." The VM bring-up failure in run.rs wraps an existing error and should be:

.context("agent-os vm bring-up failed")?

instead of .map_err(|error| anyhow!("agent-os vm bring-up failed: {error}"))?.

3. Em dash in doc comment

src/actions/filesystem.rs uses an em dash in the first doc line. CLAUDE.md: "Do not use em dashes. Use periods to separate sentences instead." Change to /// Port of [AgentOs::read_file].

4. eprintln! in test code

Both skip paths use eprintln!("skipping: ..."). CLAUDE.md prohibits eprintln!/println! for logging. Drop the message (a silent skip is acceptable) or replace with tracing::info!.

5. Missing CLAUDE.md / AGENTS.md in new package

CLAUDE.md: "Every directory that has a CLAUDE.md must also have an AGENTS.md symlink." Neighbouring packages (rivetkit/, rivetkit-core/) both carry these files. rivetkit-agent-os/ has neither. Add a CLAUDE.md + AGENTS.md symlink, or explicitly note the omission is intentional.


Advisory

Dual serde_bare versions in Cargo.lock

The lock file now carries two distinct sources of serde_bare 0.5.0: the crates.io registry version (pulled by agent-os-client/agent-os-sidecar) and the rivet-dev git fork (used by the rest of the workspace). They are separate crates at the type-system level — any serde_bare-derived types that cross the agent-os-client/rivetkit boundary will fail to compile. Fine for phase 1a with no shared bare types yet, but worth flagging before later phases add cross-boundary wire types.

filesystem.rs is a zero-abstraction passthrough

pub async fn read_file(vm, path) { vm.read_file(path).await } adds nothing for the single existing action. CLAUDE.md: "Don't add abstractions beyond what the task requires." Call vm.read_file(&path).await directly in the dispatcher and remove the module until multiple filesystem actions justify the grouping.

Startup-ready signalled before VM is alive

build_core_factory signals startup_ready = Ok(()) immediately after wrap_start succeeds, well before VM bring-up (which is lazy on first action). A caller that waits on startup success may still get a VM-bring-up error on the first action. A comment near reply.send(Ok(())) noting that startup-ready reflects actor-loop health, not VM health, would prevent confusion.


Minor nit

The inline comment in actions/mod.rs explaining the Uint8Array wrapping duplicates the module-level //! doc. Remove it to avoid the two drifting apart.


Overall the structure is solid and the skippable real-sidecar test approach is the right call. The main blockers are the unsafe env-var race (issue 1) and missing CLAUDE.md/AGENTS.md (issue 5 per project convention). The rest are clean-up items.

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