Skip to content

feat(agent-os): add acpTimeoutMs passthrough test fixture and tests#4552

Open
brittianwarner wants to merge 2 commits intomainfrom
feat/acp-timeout-config
Open

feat(agent-os): add acpTimeoutMs passthrough test fixture and tests#4552
brittianwarner wants to merge 2 commits intomainfrom
feat/acp-timeout-config

Conversation

@brittianwarner
Copy link
Copy Markdown

@brittianwarner brittianwarner commented Apr 4, 2026

Summary

  • Adds a test fixture actor (agentOsTimeoutTestActor) configured with acpTimeoutMs: 300_000 in options to verify the option passes through the actor layer to AgentOs.create() without errors.
  • Adds 2 driver tests (filesystem round-trip and exec) using the timeout-configured actor.
  • No actor layer code changes needed. The existing code already forwards AgentOsOptions transparently via spread in buildVmOptions() and CreateSessionOptions directly in createSession().

Blocked on

Bump @rivet-dev/agent-os-core once the version with acpTimeoutMs in AgentOsOptions and CreateSessionOptions is published, then remove the as AgentOsOptions type assertion in the fixture.

Context

The upstream @rivet-dev/agent-os-core is getting an activity-aware ACP timeout feature:

  • Default timeout bumped from 120s to 900s (15 min)
  • New acpTimeoutMs field on both AgentOsOptions (VM-level default) and CreateSessionOptions (per-session override)
  • Activity-aware reset: timer resets on every inbound JSON-RPC message

The actor layer in this repo already passes both option types through transparently, so no structural code changes are needed here. These tests confirm the option is accepted and operations work with it set.

Add test fixture and driver tests verifying that the acpTimeoutMs option
passes through the actor config to AgentOs.create() without errors.

The actor layer already forwards AgentOsOptions transparently via spread,
so no code changes are needed. These tests confirm the option is accepted
and basic operations work with it set.

Blocked: bump @rivet-dev/agent-os-core once the version with acpTimeoutMs
in AgentOsOptions and CreateSessionOptions is published, then remove the
type assertion in the fixture.
@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 4, 2026

This PR was not deployed automatically as @brittianwarner does not have access to the Railway project.

In order to get automatic PR deploys, please add @brittianwarner to your workspace on Railway.

Copy link
Copy Markdown
Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Copy Markdown
Author

brittianwarner commented Apr 4, 2026

@claude
Copy link
Copy Markdown

claude bot commented Apr 4, 2026

PR Review: feat(agent-os): add acpTimeoutMs passthrough test fixture and tests

Overall this is a clean, well-scoped PR with a clear purpose. A few things to look at.


Concerns

Type assertion silences broader type errors

The as AgentOsOptions assertion on the entire options object suppresses type errors beyond just the missing acpTimeoutMs field. A narrower suppression would be safer. For example, using // @ts-expect-error on the specific line with a comment explaining why would be more targeted. As written, a typo elsewhere in options would go unchecked until the TODO is resolved.

exec("echo timeout-path") - shell-string vs. command+args

The new test calls actor.exec("echo timeout-path") as a single string, while other tests use actor.spawn("node", ["/tmp/exit42.js"]) with separate command + args. If exec treats its argument as a literal program name, this would try to run a binary named echo timeout-path (with the space). Worth verifying the exec API accepts a full shell string.

Tests verify actor works, not timeout is forwarded

The two new tests confirm the VM boots and exec runs, but do not assert that acpTimeoutMs reaches AgentOs.create() or createSession(). Since this is a passthrough-confirmation PR, a comment clarifying the behavioral contract being tested would help.


Minor

  • The import-sorting diff in registry.ts is a formatting-only change mixed into a functional PR, making the diff harder to scan. Worth confirming all reorders are purely alphabetical.
  • The single-quote to double-quote changes in actor-agent-os.ts are unrelated to the feature. Fine to keep if the formatter enforces it.

Positives

  • TODO comment referencing the PR number is good practice for tracking the temporary type assertion.
  • useRealTimers: true is appropriate for timeout-related tests.
  • Test timeouts (60s) are consistent with the rest of the suite.
  • PR description clearly documents the upstream dependency and what is blocked.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 4, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4552

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4552

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4552

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4552

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4552

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4552

@rivetkit/sqlite-vfs

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sqlite-vfs@4552

@rivetkit/traces

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/traces@4552

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4552

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4552

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4552

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4552

commit: fae941b

- Reference PR #4552 in TODO comment for tracking
- Rename test descriptions to be more precise about what is asserted
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