Skip to content

fix(acp): bound extension requests with per-request timeout option - #14138

Open
kvnloo wants to merge 1 commit into
pingdotgg:mainfrom
kvnloo:muse/acp-ext-request-timeout
Open

kvnloo wants to merge 1 commit into
pingdotgg:mainfrom
kvnloo:muse/acp-ext-request-timeout

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Problem

The ACP patched protocol's sendRequest awaited its response Deferred with no deadline. A wedged ACP agent that stays alive but never answers an extension request (e.g. cursor/list_available_models during Cursor model discovery) hung the caller forever — termination only rescued the dead-process case. This is the ACP twin of the unbounded codex app-server transport request fixed in #4.

Fix

Mirrors the #4 shape:

  • request() accepts an optional timeout (AcpPatchedRequestOptions), default unbounded (existing behavior).
  • On expiry the pending entry is dropped (a late response is ignored by resolveExtPending) and the request fails with the new AcpRequestTimeoutError (added to the AcpError union).
  • AcpSessionRuntime.request() passes the option through; Cursor model discovery is now bounded at 30s.

session/prompt and the typed agent RPC path are untouched — turn-length prompts stay unbounded by design.

Validation

  • New protocol.test.ts tests (TestClock, deterministic): timeout fires with AcpRequestTimeoutError (method, requestId, message), late response ignored, next request routes by id; plus a default-unbounded control.
  • Red-on-base: timeout test hangs on base (15s vitest timeout FAIL). Green with fix: 29/29 (protocol.test.ts + errors.test.ts).
  • tsc --noEmit clean on packages/effect-acp; touched server files verified against the new types (isolated check).
  • vp fmt clean.

Files

  • packages/effect-acp/src/protocol.ts — AcpPatchedRequestOptions, sendRequest timeout
  • packages/effect-acp/src/errors.ts — AcpRequestTimeoutError + union
  • packages/effect-acp/src/protocol.test.ts — 2 tests
  • apps/server/src/provider/acp/AcpSessionRuntime.ts — option pass-through
  • apps/server/src/provider/Layers/CursorProvider.ts — 30s bound on model discovery

Authored with AI assistance (Muse, Meta's Muse Spark) under the contributor's direction.

…tion

The ACP patched protocol's sendRequest awaited its response Deferred with
no deadline. A wedged ACP agent that stays alive but never answers an
extension request (e.g. cursor/list_available_models during Cursor model
discovery) hung the caller forever; termination only rescued the
dead-process case.

Mirrors the codex app-server transport fix: request() now accepts an
optional timeout (default unbounded, existing behavior). On expiry the
pending entry is dropped (late responses are ignored by resolveExtPending)
and the request fails with AcpRequestTimeoutError. The runtime's request()
passes the option through, and Cursor model discovery is now bounded at
30s.

Tests: protocol.test.ts gains a TestClock test (timeout fires, late
response ignored, next request routes by id) plus a default-unbounded
control. Red-on-base verified (hangs, 15s vitest timeout); 29/29 green
with the fix.

Authored with AI assistance (Muse, Meta's Muse Spark) under the contributor's direction.
@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: ea61c190-6285-4bcc-a7d9-0eb9bfa42055

📥 Commits

Reviewing files that changed from the base of the PR and between ed57bed and 5cfffb8.

📒 Files selected for processing (5)
  • apps/server/src/provider/Layers/CursorProvider.ts
  • apps/server/src/provider/acp/AcpSessionRuntime.ts
  • packages/effect-acp/src/errors.ts
  • packages/effect-acp/src/protocol.test.ts
  • packages/effect-acp/src/protocol.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 28, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 28, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR adds tested, backward-compatible timeout plumbing, but changes Cursor model discovery from unbounded waiting to a 30-second failure deadline. That is a product-level default behavior change affecting an existing path and warrants deliberate review.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 1, 2026 16:00

Dismissing prior approval to re-evaluate 5cfffb8

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

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants