Skip to content

feat(agent-os): use agent-os-client from crates.io and pass sidecar binary path from npm package#5234

Open
NathanFlurry wants to merge 1 commit into
stack/feat-rivetkit-agent-os-phase-3-process-11-arms-exec-spawn-wait-kill-stop-list-all-tree-get-writestdin-closestdin-execresultdto-unkrssztfrom
stack/feat-agent-os-use-agent-os-client-from-crates-io-and-pass-sidecar-binary-path-from-npm-package-xkllkonn
Open

feat(agent-os): use agent-os-client from crates.io and pass sidecar binary path from npm package#5234
NathanFlurry wants to merge 1 commit into
stack/feat-rivetkit-agent-os-phase-3-process-11-arms-exec-spawn-wait-kill-stop-list-all-tree-get-writestdin-closestdin-execresultdto-unkrssztfrom
stack/feat-agent-os-use-agent-os-client-from-crates-io-and-pass-sidecar-binary-path-from-npm-package-xkllkonn

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Jun 12, 2026

Copy link
Copy Markdown
Member

No description provided.

@NathanFlurry

Copy link
Copy Markdown
Member Author

Stack for rivet-dev/rivet

Current stack:

Dependencies:

Get stack: forklift get 5234
Push local edits: forklift submit
Merge when ready: forklift merge 5234

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

PR Review: feat(agent-os): use agent-os-client from crates.io and pass sidecar binary path from npm package

Blocker

Hardcoded absolute paths in Cargo.toml (two locations)

# rivetkit-rust/packages/rivetkit-agent-os/Cargo.toml
-agent-os-client = { path = "../../../ext/agent-os/crates/client" }
+agent-os-client = { path = "/home/nathan/a5/crates/client" }

# rivetkit-typescript/packages/rivetkit-napi/Cargo.toml
-agent-os-client = { path = "../../../ext/agent-os/crates/client" }
+agent-os-client = { path = "/home/nathan/a5/crates/client" }

The PR title says "use agent-os-client from crates.io" but both Cargo.toml files use a local absolute path to Nathan's home directory. This breaks the build for every other developer and all CI runners. The path should either use the original relative path (../../../ext/agent-os/crates/client) or a proper crates.io version specifier (agent-os-client = "0.2.0-rc.3").


Bug: createSignedPreviewUrl silently drops the TTL argument

The example in examples/agent-os-e2e/src/client.ts calls:

agent.createSignedPreviewUrl(8080, 60)

But the Rust dispatch arm only decodes a single (u16,):

// actions/mod.rs
"createSignedPreviewUrl" => {
    let args: Result<(u16,)> = action.decode_as();
    match args {
        Ok((port,)) => action.ok(&preview::create(previews, port)),

The second argument (TTL in seconds or minutes) is silently ignored and the hardcoded PREVIEW_TTL_MS = 60 * 60 * 1000 (one hour) is always used. Either the example should be updated to drop the second argument, or the handler should accept and honor it.


Concern: unsafe std::env::set_var is fragile

rivetkit-typescript/packages/rivetkit-napi/src/agent_os.rs:

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

The SAFETY comment says this runs once before any VM thread reads the var, which may be true today, but:

  1. parse_agent_os_options can be called more than once (e.g., multiple factory constructions in the same process). A second call could race with a VM thread already reading AGENT_OS_SIDECAR_BIN.
  2. std::env::set_var is unsound in multithreaded programs - Rust 1.81 formally deprecated it outside unsafe for exactly this reason. The NAPI worker thread pool means there is always more than one thread.

A safer pattern is to use a std::sync::OnceLock to ensure the write happens exactly once, and return an error if called again with a different path rather than silently overwriting.


Minor: PreviewStore leaks expired tokens that are never resolved

In preview.rs, expired entries are only pruned lazily when resolve() is called. A token that is issued but never proxied will accumulate in the HashMap for the lifetime of the actor. In a long-running actor with many createSignedPreviewUrl calls this is a gradual memory leak. Consider sweeping expired entries inside create() before inserting.


Minor: Duplicate header values are dropped in proxy_preview

In run.rs, HashMap<String, String> is used to collect response headers. HTTP responses can carry multiple values for the same header (the canonical case is Set-Cookie). Last writer wins, silently dropping earlier values. For a preview proxy this is probably fine in practice, but worth a comment acknowledging the limitation.


Observation: Prerelease version strings look like dev artifacts

Several packages are pinned to 0.0.0-claude-rust-review-and-runtime-fixes.5955345. The branch name in the version identifier suggests these were published from a development branch. Confirm these will be updated to stable versions before merge.


Positive notes

  • The DTO + mapping pattern in cron.rs, session.rs, network.rs, and preview.rs is clean and consistent with the existing filesystem.rs style.
  • The deleteFile optional-args fallback (try 2-arg, fall back to 1-arg) handles the ciborium fixed-tuple limitation correctly and the comment explains why.
  • The URL construction fix in examples/agent-os-e2e/src/client.ts (using a URL object to insert the preview path into the pathname) is correct; the comment explaining the gateway URL query-string hazard is helpful.
  • Removing the long TODO comment about software threading and replacing it with a working implementation is a meaningful improvement.

@NathanFlurry NathanFlurry force-pushed the stack/feat-agent-os-use-agent-os-client-from-crates-io-and-pass-sidecar-binary-path-from-npm-package-xkllkonn branch from 94738d3 to 3db3433 Compare June 12, 2026 23:05
@NathanFlurry NathanFlurry force-pushed the stack/feat-agent-os-use-agent-os-client-from-crates-io-and-pass-sidecar-binary-path-from-npm-package-xkllkonn branch from 3db3433 to 4cfbd95 Compare June 14, 2026 06:38
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