feat(rivetkit): phase 1c — agentOs() native ladder + engine driver branch + driver-suite e2e green

[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/5192
Pull/update this stack: forklift sync
Publish local edits: forklift submit
Merge when ready: forklift merge

[claude]

Code Review — Phase 1c: agentOs() native ladder + engine driver branch

Overview

This PR migrates the agentOs() actor from a pure JS implementation (~1,500 lines across cron/db/filesystem/network/preview/process/session/shell modules) to a Rust-backed factory via a new nativeFactoryBuilder marker on ActorDefinition. The ladder: agentOs() sets nativeFactoryBuilder, buildRegistryWithRuntime dispatches on it, and EngineActorDriver stubs out the JS actor lifecycle for the native path.

---

Issues

handler.actorStartPromise?.resolve() uses optional chaining in a lifecycle path

actor-driver.ts line ~1654:

handler.actorStartPromise?.resolve();

Per CLAUDE.md's Fail-By-Default Runtime section, optional chaining is only acceptable for best-effort diagnostics and cleanup paths. Actor start is a required lifecycle step. If actorStartPromise is absent when it shouldn't be, the actor silently hangs instead of surfacing an explicit error. The corresponding native/static branches (isStaticActorDefinition, buildNativeFactory) also use optional chaining here — if that's already accepted, this is consistent. If it isn't, both should be fixed together.

NATIVE_NAPI_ACTOR_STUB is an underspecified stub

const NATIVE_NAPI_ACTOR_STUB = {
    onStop: async (_reason: string) => {},
    debugForceCrash: async () => {},
} as unknown as AnyActorInstance;

The double cast through unknown hides any future AnyActorInstance methods added to the interface. If the engine driver ever routes an onRequest, onConnect, or similar call through the JS actor layer for a native-factory actor, this will throw at runtime instead of at compile time. Worth auditing every callsite of handler.actor.* to confirm only onStop/debugForceCrash are reachable after setting the stub.

toolCallbacks ?? undefined is redundant

napi-runtime.ts:

const factory = this.#bindings.NapiActorFactory.fromAgentOs(
    options,
    toolCallbacks ?? undefined,
);

toolCallbacks is already typed as Record<string, unknown> | undefined | null. Passing toolCallbacks ?? undefined converts null → undefined but the call site always passes undefined. Either remove the ?? undefined or accept null at the NAPI boundary and let Rust handle it.

AgentOsToolCallbacks mixes undefined and null

export type AgentOsToolCallbacks = Record<string, unknown> | undefined | null;

The tri-valued type signals uncertainty. Since this is explicitly "always undefined" until Phase 5, just type it undefined now and broaden it when Phase 5 lands. An opaque unknown placeholder type would also be cleaner than an all-null/undefined union.

Phase/task references in source comments

Several comments reference phased implementation context that belongs in the PR description, not in the code. Per CLAUDE.md: "Don't reference the current task, fix, or callers". Examples:

• // Phase 1c minimum: empty config. Future phases thread software, permissions, mounts, etc. through here.
• // Reserved for Phase 5 (toolkit dispatch); currently always undefined.
• // Phase 3+ adds...

These rot immediately once future phases land and the comments aren't updated.

Multi-line comment blocks

CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." Several additions are multi-line JSDoc blocks (the NATIVE_NAPI_ACTOR_STUB header, the NapiAgentOsOptions and AgentOsToolCallbacks JSDoc, the nativeFactoryBuilder property docs on ActorDefinition). The intent is clear but the form conflicts with the convention.

buildConfigJson ignores its argument

function buildConfigJson<TConnParams>(
    _parsed: AgentOsActorConfig<TConnParams>,
): string {
    return "{}";
}

The _parsed parameter with leading-underscore signals intentional non-use — fine for now. But the function call site passes parsed as a named argument, which will make future reviewers wonder if the omission was intentional. A bare function buildConfigJson(): string { return "{}"; } is clearer until the body actually uses config, or add a one-line comment on the first field that will be threaded through.

---

Positive Notes

• WriteFileContent + JsonCompatUint8Array deserialization is clean and handles the three wire shapes correctly (CBOR text, CBOR bytes, ["$Uint8Array", base64]). The error messages on malformed base64 and bad tags are explicit.
• The dispatch in buildRegistryWithRuntime is clean — one branch, no other code touched.
• The runtime guard in buildNativeFactoryBuilder is appropriately explicit:

  if (runtime.kind !== "napi") { throw new Error(...) }
  if (!runtime.createAgentOsFactory) { throw new Error(...) }

  Fail-by-default at registration time, not at first action dispatch.
• Deleting ~1,500 lines of JS action builders while preserving the public agentOs() signature is the right direction.

---

Questions

1. Are onStop and debugForceCrash truly the only AnyActorInstance methods the engine driver calls post-start for a native factory actor? A quick grep or a compile-time exhaustiveness check on the stub would be worth adding to CI.
2. The removed JS agentOs() actions (sessions, processes, filesystem, shell, cron, preview) are not yet re-implemented in Rust. Is this branch expected to break callers of those actions until a later phase, or is there a compatibility shim somewhere?