Skip to content

feat(pi): add ACP bridge for editors - #5797

Draft
eersnington wants to merge 16 commits into
docs/headless-pi-actorfrom
feat/pi-acp-integration
Draft

eersnington wants to merge 16 commits into
docs/headless-pi-actorfrom
feat/pi-acp-integration

Conversation

@eersnington

@eersnington eersnington commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Adds rivet-pi acp, which connects ACP editors such as Zed to Pi actors, and serveAcp() from @rivet-dev/pi/acp for apps that build their own bridge.

npx @rivet-dev/pi acp --actor agent --user me --credentials credentials
  • Each editor session is the Pi actor with key [user, sessionId]. Reopening a session replays its messages. The editor's model picker uses getAvailableModels and setModel.
  • --credentials <actor> offers a login in the editor. The editor runs Pi's login in a terminal, and the command calls save(provider, credential) on that actor with key [user].
  • Docs: an Editors section on the Pi page, and the Zed setup in the credentials example README.

Security:

  • rivet-pi acp uses RIVET_TOKEN, which reaches every actor. The docs say to use it only for yourself, and to build a serveAcp() command with actor-scoped JWTs for other users.

This is part 3 of 3 in a stack:

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review

Solid addition: wires ACP editors (Zed, etc.) to Pi actors via a bridge, with unusually thorough test coverage of the tricky concurrency/reconnection edge cases. A couple of things worth addressing before merge, plus a few nits.

Findings

1. (Medium) newSession() creates the actor (and, when a sandbox provider is configured, provisions its sandbox) before checking whether the user has any login credentials — integrations/pi/src/acp/agent.ts:208-221

```ts
async newSession(_params: NewSessionRequest) {
const session = await this.#open(randomUUID(), true); // creates the actor + calls getSession()
const configOptions = await this.#configOptions(session);
const login = this.#options.login;
if (login && !configOptions.some((option) => option.id === "model")) {
await this.#close(session.id); // only disposes the local connection
...
throw RequestError.authRequired(...);
}
...
}
```

`#open(..., true)` calls `options.actor(sessionId, true)` (typically `getOrCreate`) and then `handle.getSession()`. `getSession` is a `read()` action (`integrations/pi/src/actions.ts:118-121, 221`) that goes through `ensurePiSession()` (`runtime.ts`), which connects/provisions the actor's sandbox when a `SandboxProvider` is configured, unconditionally, before any credential check happens. Only after that does `newSession` look at whether a `model` config option exists to decide whether to throw `authRequired`.

On the "no credentials yet" branch, `#close()` only disposes the local `PiAgentConnection` (`agent.ts:307-312`); there's no `destroy()` on `PiAgentHandle`/`PiAgentConnection`, so the actor itself (and any sandbox it provisioned) is never cleaned up. Editors can call `session/new` repeatedly before a user finishes logging in (app startup, "New Chat" clicks, etc.), so this can leave behind a permanently orphaned actor (with a live sandbox, when one is configured) per failed attempt. Worth checking model/credential availability before creating the full session, or destroying the actor on the auth-required path.

2. (Low-medium) #open() never releases the handle from options.actor() when getSession() times out or errors — agent.ts:279-305

If `withTimeout(handle.getSession(), timeoutMs)` throws, the function just throws a `RequestError` without touching `handle`. For the scoped-JWT pattern demonstrated in the PR's own test ("an editor that reaches each conversation with a scoped token..."), each `actor()` call builds a brand-new `createClient(...)`. If that client's `getSession()` never resolves (engine unreachable, bad token, etc.) and times out, the freshly created client is never disposed, its background reconnect loop (which, per the `openTimeoutMs` doc comment, retries forever) keeps running with no way for the caller to reclaim it. Worth exposing a way to dispose the handle on the failure path, or documenting that `options.actor()` implementations must handle their own cleanup on this path.

Nits

  • `integrations/pi/tests/acp.test.ts`, the "a new session fails with an error instead of hanging when the engine is unreachable" test creates a `client` via `createClient()` at a closed port but never disposes it, unlike the other tests that use `proxiedClient`/`c.onTestFinished(() => client.dispose())`. Could leave a background reconnect loop running for the rest of the suite.
  • Double blank line at the top of `acp.test.ts` (after the `tcp-proxy` import, before `let mockModel`).

Positive notes

  • Test coverage is strong for this kind of bridging code: cancel-before-start vs. cancel-mid-run races, connection loss between vs. during a turn, retry-then-succeed vs. terminal model failure, session replay, model/thinking-level switching with allowlist enforcement, and a full scoped-JWT-per-actor rotation scenario, all against real actors via `setupTest`, no mocking, matching repo testing conventions.
  • Exhaustive `switch`/`satisfies never` matching over the ACP and Pi event unions follows the repo's "no `_ =>` fallthrough on discriminated unions" convention.
  • Security posture is reasonable: the `RIVET_TOKEN`-reaches-every-actor caveat is explicitly documented with `serveAcp()` as the scoped alternative, and the terminal-login command construction (`shellQuote`) only builds a display string / a command handed to the ACP client's own terminal integration, not something executed via a shell here.

@eersnington
eersnington force-pushed the feat/pi-acp-integration branch 2 times, most recently from 66a2099 to d9aba94 Compare September 25, 2026 17:13
@eersnington
eersnington force-pushed the docs/headless-pi-actor branch from e5040f4 to 89a5bd7 Compare September 25, 2026 18:16
@eersnington
eersnington force-pushed the feat/pi-acp-integration branch 2 times, most recently from 0a12f50 to 3a6264d Compare September 25, 2026 20:22
@eersnington
eersnington force-pushed the docs/headless-pi-actor branch from 89a5bd7 to dc9d5b1 Compare September 25, 2026 20:22
@eersnington
eersnington force-pushed the docs/headless-pi-actor branch from dc9d5b1 to 3e2a18d Compare September 25, 2026 20:32
@eersnington
eersnington force-pushed the feat/pi-acp-integration branch from 3a6264d to 926f6e9 Compare September 25, 2026 20:32
@eersnington
eersnington force-pushed the docs/headless-pi-actor branch 3 times, most recently from 3f92a6f to b2d96c4 Compare September 26, 2026 00:04

This branch has not been deployed

No deployments
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