feat(rivetkit-napi): phase 1b — NapiActorFactory.fromAgentOs static constructor + writeFile arm + e2e factory round-trip

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

• feat(rivetkit-agent-os): phase 3 process — 11 arms (exec/spawn/wait/kill/stop/list/all/tree/get/writeStdin/closeStdin) + ExecResultDto #5197 unkrsszt · 2026-06-08 18:29:29
• test(rivetkit-agent-os): partial-failure driver test for readFiles batch DTO (Option<content>, Option<error>) #5196 pzzzpmtr · 2026-06-08 18:29:28
• feat(rivetkit-agent-os): phase 3 filesystem — mkdir/readdir/exists/move/deleteFile/readFiles/writeFiles/readdirRecursive arms + batch DTOs #5195 onyykwlk · 2026-06-08 18:29:27
• test(rivetkit-agent-os): config parsing tests + documented JS↔Rust software shape mismatch (phase 3 prep) #5194 ouyvrlxt · 2026-06-08 18:29:26
• feat(rivetkit-agent-os): phase 2 — stat action arm + structured-payload wrapper tests for cross-encoding parity #5193 slkvonmt · 2026-06-08 18:29:25
• feat(rivetkit): phase 1c — agentOs() native ladder + engine driver branch + driver-suite e2e green #5192 nxzrussy · 2026-06-08 18:29:24
• feat(rivetkit-napi): phase 1b — NapiActorFactory.fromAgentOs static constructor + writeFile arm + e2e factory round-trip #5191 mtlnunov · 2026-06-08 18:29:22 👈
• feat(rivetkit-agent-os): phase 1a — rust crate skeleton + readFile arm + dispatcher e2e against real sidecar #5190 wrxkrssv · 2026-06-08 18:29:21
• feat(rivetkit): JSON_COMPAT_UINT8_ARRAY byte-payload encoding parity with TS (encode + decode + cross-language tests) #5189 ukssumln · 2026-06-08 18:29:20
• chore: phase 0 prereqs (rivetkit workspace member, serde_bare align, wasm skip.agentOs default) #5188 qvnkvkxw · 2026-06-08 18:29:19
• docs: PLAN2 + TODOLIST + RIVETKIT_RUST_FIX (clean slate, post-wipe) #5187 yqrwozvn · 2026-06-08 18:29:18
• main

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

[claude]

Code Review — Phase 1b: NapiActorFactory.fromAgentOs + writeFile arm

Overview

This PR adds the writeFile arm to the agent-os action dispatcher, a NapiActorFactory.fromAgentOs static constructor, and end-to-end tests covering both the Rust dispatcher and the NAPI factory path. The overall structure is clean and the layering respects the NAPI-as-pure-bindings constraint.

---

Issues

unsafe { std::env::set_var } in async tests (correctness risk)

Both test files (tests/agent_os_factory.rs and tests/dispatcher_e2e.rs) mutate the environment inside Tokio test tasks:

unsafe {
    std::env::set_var("AGENT_OS_SIDECAR_BIN", candidate);
}

The comment says "tests run single-process" but Tokio's multi-threaded test runtime runs #[tokio::test] functions concurrently across threads. set_var is unsound under concurrent access (it's why Rust 1.81 made it unsafe). Either gate the mutation behind a parking_lot::Mutex (once global) or, simpler, just skip the auto-probe and require callers to set the env var explicitly.

eprintln! in test skip paths (CLAUDE.md)

Both sidecar-gated tests use eprintln! for the skip notice. CLAUDE.md says to never use eprintln!/println! even in test code. Prefer tracing::info! or, since these are test skip paths where the subscriber may not be initialized, at minimum use println! behind #[cfg(test)]. Alternatively, adopt the established skip pattern from the existing tests in rivetkit-core.

Triple-slash comment in TypeScript test file

The top of agent-os-napi-factory.test.ts uses /// JSDoc syntax for a module-level description, which implies API documentation. Module comments in test files should use //.

---

Observations

writeFile path goes through dispatcher without path sanitization

The writeFile arm accepts a caller-supplied path directly:

let args: Result<(String, serde_bytes::ByteBuf)> = action.decode_as();
match args {
    Ok((path, contents)) => filesystem::write_file(vm, &path, contents.into_vec()).await

Path validation presumably lives in the underlying vm.write_file, but there is no mention of it here and no test for path traversal. Worth confirming the sandboxed AgentOs client rejects ../-style paths before this arm ships.

AgentOsConfigJson derives Clone but only lives behind Arc

AgentOsConfigJson derives Clone but is immediately wrapped in Arc and never cloned directly — the Arc::clone inside the builder closure is the only copy operation. Dropping the Clone derive would make the intent clearer (this type is shared, not copied).

Empty CallbackBindings — new fields cause compile errors (good)

CallbackBindings::empty() lists every field explicitly. Adding a new callback field to CallbackBindings will produce a compile error here, which is the correct fail-loud behavior. Noting it's intentional.

Dispatcher comment style (minor)

The block comment on the writeFile arm in mod.rs uses a semicolon-joined run-on sentence. CLAUDE.md asks for "normal, complete sentences" without fragmented dash/paren structures. The comment is readable but could be tightened.

write_reply checked for non-empty but not for correct unit encoding

The NAPI e2e test asserts !write_reply.is_empty() but the comment says "we don't assert the exact unit encoding." A single CBOR null (0xF6) is one byte, so the assertion is effectively asserting the reply arrived at all. That is sufficient for this phase, but assert_eq!(write_reply, &[0xF6]) would pin the protocol shape.

---

Test Coverage

Good coverage overall:

• Rust dispatcher e2e: write then read round-trip + unknown action path
• NAPI factory Rust unit tests: construction, dispatches, unknown action, drain
• TypeScript tests: valid/empty/omitted config, unknown field rejection (deny_unknown_fields), malformed JSON, non-serializable field rejection

The TypeScript tests appropriately scope to Phase 1b (construction-only) and defer the full driver gate to Phase 1c.

---

Summary

The change is well-scoped and correctly layered. The main blocking concern is the unsafe set_var race in multi-threaded Tokio tests. The eprintln! and triple-slash nits are CLAUDE.md violations worth fixing before merge.