Skip to content

fix(acp): accept SDK elicitation requests - #11294

Open
shivamhwp wants to merge 1 commit into
mainfrom
fix/acp-elicitation-compat
Open

shivamhwp wants to merge 1 commit into
mainfrom
fix/acp-elicitation-compat

Conversation

@shivamhwp

@shivamhwp shivamhwp commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto main, which includes the device-ticket correction from #11304. This PR changes only ACP behavior.

ACP agents using elicitation/create receive "Method not found" even when T3 has a registered question handler. The shared client now accepts that SDK method and returns its flat action and content response. It also routes elicitation/complete to the existing completion handler.

The legacy session/elicitation methods retain their nested response format. Compatibility lives beside the RPC definitions because the generated schema is pinned to v0.11.3. Providers still supply their own question handler; missing handlers and malformed requests return protocol errors.

Closes #9048. This fixes the SDK method-name and response-shape mismatch reported in that issue. #10893 carries an Oh My Pi-specific workaround; adding that provider is separate. The adapter-facing generated types remain compatible, and this change covers session-scoped form and URL requests.

Validation: 44 focused client, agent, protocol and server-runtime tests pass, including flat accept/decline/cancel results, metadata, both notification names, malformed requests and legacy exchanges. A separate exchange with the official @agentclientprotocol/sdk 1.4.0 passes: its AgentSideConnection.createElicitation receives the accepted answer, and completeElicitation reaches T3's completion handler. Package and server typechecks and scoped lint pass. No live Oh My Pi process or native client UI was exercised.

Model: GPT-6. Harness: Codex in T3 Code.

Rebased on main 20363c32c9bfdbf49c2716ef11d1f18483fcc01b. Scoped validation rerun after rebasing; existing native and human review limits remain.

@cursor

cursor Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.6 KiB +19 B (+0.1%) 15.1 KiB
Codex Thread snapshot wire 7.0 KiB 7.0 KiB −2 B (−0.0%) 7.3 KiB
Codex Live turn WebSocket wire 6.5 KiB 6.5 KiB +21 B (+0.3%) 7.8 KiB
Codex Live turn WebSocket decoded 57.0 KiB 57.0 KiB 0 B (0.0%) 66.4 KiB
Codex Live turn messages 8 8 0 (0.0%) 21
Claude Total thread wire 13.6 KiB 13.6 KiB −30 B (−0.2%) 15.1 KiB
Claude Thread snapshot wire 7.1 KiB 7.1 KiB −6 B (−0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.6 KiB 6.5 KiB −24 B (−0.4%) 7.8 KiB
Claude Live turn WebSocket decoded 57.9 KiB 57.8 KiB −44 B (−0.1%) 66.4 KiB
Claude Live turn messages 10 9 −1 (−10.0%) 21

Baseline: e628683 · PR result: bdcfe7f · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: a26dddfd-9f91-474f-b7e0-17c5be56e48c

📥 Commits

Reviewing files that changed from the base of the PR and between e1c94f7 and 44f78f2.

📒 Files selected for processing (4)
  • packages/effect-acp/src/client.test.ts
  • packages/effect-acp/src/client.ts
  • packages/effect-acp/src/protocol.ts
  • packages/effect-acp/src/rpc.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Adds "elicitation/create" RPC support and accepts "elicitation/complete" notifications. The client shares elicitation handling across method aliases and validates payloads, response shapes, URL mode, malformed requests, and missing handlers.

Changes

Elicitation support

Layer / File(s) Summary
Elicitation RPC contract
packages/effect-acp/src/rpc.ts
Defines and registers CreateElicitationRpc with compatible request, success, and error schemas.
Client request routing
packages/effect-acp/src/client.ts
Decodes "elicitation/create" requests, maps invalid payloads to protocol errors, and invokes the shared elicitation handler.
Notification aliases and validation
packages/effect-acp/src/protocol.ts, packages/effect-acp/src/client.test.ts
Accepts both completion notification methods and tests response shapes, URL mode, validation failures, and missing handlers.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant CreateElicitationRpc
  participant decodeElicitationRequest
  participant coreHandlers.elicitation
  CreateElicitationRpc->>decodeElicitationRequest: Decode request payload
  decodeElicitationRequest-->>CreateElicitationRpc: Return decoded request
  CreateElicitationRpc->>coreHandlers.elicitation: Handle elicitation
  coreHandlers.elicitation-->>CreateElicitationRpc: Return action and optional _meta
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 44f78

No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: enabling the ACP client to accept SDK elicitation requests.
Description check ✅ Passed The description explains what changed, why it changed, compatibility behavior, validation results, and testing limits. It does not use the template headings or include the checklist, but the required …
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/acp-elicitation-compat

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

@shivamhwp
shivamhwp changed the base branch from main to fix/client-device-ticket-http-group September 11, 2026 20:30
@shivamhwp
shivamhwp force-pushed the fix/acp-elicitation-compat branch from 44f78f2 to 0838f78 Compare September 11, 2026 20:30
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 11, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 226856b

Macroscope's review found this PR approvable — This is a localized ACP interoperability fix that routes SDK elicitation aliases through existing handlers while preserving legacy exchanges. The new wire translations and notification handling are covered by focused tests, with no product-default, deployment, security, billing, or static-analysis changes.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

No code changes detected at bdcfe7f. Prior analysis still applies.

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

@shivamhwp
shivamhwp force-pushed the fix/acp-elicitation-compat branch from 0838f78 to 226856b Compare September 13, 2026 15:35
@shivamhwp
shivamhwp changed the base branch from fix/client-device-ticket-http-group to main September 13, 2026 15:35
@cursor

cursor Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@macroscopeapp
macroscopeapp Bot dismissed their stale review September 13, 2026 15:35

Dismissing prior approval to re-evaluate 226856b

@github-actions github-actions Bot added the 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. label Sep 13, 2026
@shivamhwp
shivamhwp force-pushed the fix/acp-elicitation-compat branch from 226856b to bdcfe7f Compare September 13, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(effect-acp): elicitation method name and response shape drift from official ACP SDK

1 participant