feat(providers): standardize ACP providers - #6461
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new/changed Effect services in this PR (AcpRegistryCatalog, AcpRegistryRuntimeCoordinator, the ACP Registry driver/probe, and their call sites). Four convention issues, all in ACP Registry code introduced here.
Posted via Macroscope — Effect Service Conventions
Address the Macroscope review round on pingdotgg#6461 (fail-closed archive listings, linear download buffering, normalized probe fields, inline service shapes, restored accent badges, honest uninstall errors) and extend the generic ACP runtime with the capabilities live testing surfaced: - advertise and implement the ACP client fs and terminal capabilities, including embedded terminal output in tool calls - map session config options and modes onto provider option descriptors; collapse legacy model-effort variants onto base models and drive agent collaboration modes from T3's own Plan/Build toggle - compose runnable terminal-auth commands and env-var guidance for advertised authentication methods - cache successful discovery probes, raise the probe timeout for cold npx starts, and soften probe failures to warnings - replace stale ACP model inventories on completed probes instead of retaining them forever through the snapshot cache - implement checkpoint rollback for ACP threads with reset semantics - bridge T3's MCP endpoint over stdio (t3 acp-mcp-bridge) so every ACP agent receives the t3-code orchestration toolkit; live testing proved injected http servers never connected - fall back to native image loading for registry icons (the official CDN sends no CORS headers) and degrade rejected config-option values instead of failing the session open Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a23c9c5 to
44704f5
Compare
StiensWout
changed the title
44704f5 to
ac3b3a7
Compare
There was a problem hiding this comment.
Two Effect service convention issues in the new ACP service code. Everything else (inline Context.Service interfaces, AcpRegistryCatalog["Service"] / AcpRegistryRuntimeCoordinator["Service"] references, removal of the makeAcpRegistryResolver alias, environment-acquired catalog/coordinator in the driver) matches the conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Effect service conventions: two findings in the new ACP code. The earlier review round's items (standalone *Shape interfaces, the makeAcpRegistryResolver compatibility alias, and the detail-copies-cause.message wrapper in ws.ts) are resolved on this head.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This XXL PR introduces a new ACP Registry provider, third-party provisioning, authenticated MCP bridging, protocol compatibility, session lifecycle, terminal, settings, and orchestration behavior across server, web, mobile, contracts, and a shared package. The breadth and security-sensitive runtime effects, together with unresolved substantive protocol and orchestration concerns, require human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
Reviewed the ACP service/error changes against the Effect service conventions.
One new finding this round (duplicated private schema predicate in ws.ts). Two previously flagged violations are still present and unchanged, so I have not re-commented on them:
apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts:4667-4676— single known tagged failure recovered withEffect.catch+ manual_tagcheck andEffect.fail(error)re-raise; convention isEffect.catchTags({ AcpRequestError: ... }).apps/server/src/provider/Drivers/AcpRegistryDriver.ts:537— wrapperdetailis built fromcause.messagewhilecauseis already preserved; use the stable phrase form used by the sibling drivers.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One finding in the new ACP MCP stdio bridge: the error wrapper flattens the underlying failure into a message string instead of preserving it as cause.
Posted via Macroscope — Effect Service Conventions
519c42a to
4c55679
Compare
[gpt-5.6-sol] RESPONDING ON BEHALF OF WOUT:Generic ACP proofPi and Google Antigravity are both configured through the same official ACP Registry and orchestrator V2 path. The live Antigravity prepare streamed and installed the 543,411,011-byte Google-hosted archive; no agent-specific integration code was added. Latest commit: d54c3cb |
There was a problem hiding this comment.
UI consistency review: two new settings surfaces reconstruct shared primitives locally. Earlier findings on this PR (ScrollArea usage, radius tokens, DialogPanel, registry icon forwarding to the command palette and provider card) look resolved.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Effect service conventions: one finding on the ACP Registry session-management path. Everything else in the ACP Registry service surface (inline Context.Service interfaces, exported make/layer, exported isAcpRegistryError, Effect.catchTags, snapshot error attributes) matches the conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Two Effect service convention issues in the new ACP Registry code. Everything else in the changed service scope (the AcpRegistryCatalog / AcpRegistryRuntimeCoordinator definitions, their make/layer exports, and the driver wiring) reads as conformant.
Posted via Macroscope — Effect Service Conventions
[gpt-5.6-sol] RESPONDING ON BEHALF OF WOUT:Macroscope UI Consistency is a stale-mirror false positive on head 8813201. The check is attached to 8813201, but its own activity/output says it reviewed only |
|
Global-package boundary proof on
This complements the uploaded Pi + Antigravity proof above. Package-based Registry agents now install globally during prepare and ACP launches the exposed command directly, with no |
|
Macroscope UI Consistency is a stale false positive on |
| type: "diff" as const, | ||
| path: change.path, | ||
| ...(change.operation === "add" ? { oldText: null } : {}), | ||
| newText: patch, |
There was a problem hiding this comment.
🟡 Medium src/client.ts:336
V2 diff updates expose unified patch syntax as newText, so consumers treat the patch itself as the complete file contents and render incorrect file changes. This also repeats the same whole patch for every entry in changes; derive per-file replacement content or avoid mapping unsupported patch data into newText.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/effect-acp/src/client.ts around line 336:
V2 `diff` updates expose unified patch syntax as `newText`, so consumers treat the patch itself as the complete file contents and render incorrect file changes. This also repeats the same whole patch for every entry in `changes`; derive per-file replacement content or avoid mapping unsupported patch data into `newText`.
| ); | ||
| } | ||
| if (notification.update.sessionUpdate === "config_option_update") { | ||
| yield* Ref.set(configOptionsRef, notification.update.configOptions); |
There was a problem hiding this comment.
🟡 Medium acp/AcpSessionRuntime.ts:1636
A failed session/load leaves configOptionsRef containing the candidate session's config_option_update values, so subsequent setConfigOption calls validate against or short-circuit using the wrong session's configuration. The gate branch updates this shared ref before the load outcome is known; snapshot and restore the active session's options on failure, or isolate candidate-session updates until adoptSession succeeds.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 1636:
A failed `session/load` leaves `configOptionsRef` containing the candidate session's `config_option_update` values, so subsequent `setConfigOption` calls validate against or short-circuit using the wrong session's configuration. The gate branch updates this shared ref before the load outcome is known; snapshot and restore the active session's options on failure, or isolate candidate-session updates until `adoptSession` succeeds.
| if (update.sessionUpdate === "terminal_output_chunk") { | ||
| return { | ||
| ...current, | ||
| output: `${current.output}${Buffer.from(update.data, "base64").toString("utf8")}`.slice( |
There was a problem hiding this comment.
🟡 Medium acp/AcpRuntimeModel.ts:79
applyAcpAgentTerminalUpdate permanently corrupts terminal output when a multibyte UTF-8 character is split across terminal_output_chunk updates: each chunk is decoded independently, producing replacement characters instead of the original character. Preserve undecoded bytes between chunks or use a streaming UTF-8 decoder for the terminal state.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/AcpRuntimeModel.ts around line 79:
`applyAcpAgentTerminalUpdate` permanently corrupts terminal output when a multibyte UTF-8 character is split across `terminal_output_chunk` updates: each chunk is decoded independently, producing replacement characters instead of the original character. Preserve undecoded bytes between chunks or use a streaming UTF-8 decoder for the terminal state.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 30466f3. Configure here.
| update.sessionUpdate === "user_message" ? "user" : "assistant", | ||
| text, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
Subagent messages blocked by gate
High Severity
projectSubagentNotification now projects ACP v2 agent_message upserts for child sessions, but the foreign-session gate still admits only agent_message_chunk. Authoritative child-session messages never reach that handler, so subagent assistant text from v2 agents is dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 30466f3. Configure here.
| : modeChoices.find((choice) => choice === "code" || choice === "default"); | ||
| if (requestedMode !== undefined && modeOption.currentValue !== requestedMode) { | ||
| yield* runtime.setConfigOption(modeOption.id, requestedMode); | ||
| } |
There was a problem hiding this comment.
Build forces code mode
Medium Severity
The new config mode mapping treats non-plan interaction as code or default only. That disagrees with the existing modes-API path, which leaves the agent default alone for Build. Agents that advertise mode as a config option are switched out of ask on every Build open, changing their native permission behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 30466f3. Configure here.
| update.sessionUpdate === "user_message" ? "user" : "assistant", | ||
| text, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
Replay ignores plan updates
Medium Severity
During session load/replay, only legacy plan clears the loading message boundary. ACP v2 plan_update and plan_removed fall through, so later history chunks can keep concatenating onto the previous role instead of starting a new history item after a plan boundary.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 30466f3. Configure here.
7 participants
Generic ACP support on orchestrator v2 still depended on provider-specific assumptions. Registry agents could install and launch, but protocol generations, request identity, MCP access, authentication, session replay, and client presentation did not share one reliable boundary.
This PR makes ACP Registry agents first-class providers through the generic orchestrator v2 path.
What changed
npxanduvxpackages globally so their normal commands are available in fresh server terminals. Registry binaries stay in the managed, checksum-aware cache.Protocol compatibility
effect-acpnow generates and pins both protocol surfaces:schema-v2.0.0-alpha.3schema-v1.21.0Initialization advertises both generations and selects the wire implementation from the response shape. This is required in practice:
pi-acp@0.0.33returns the v1 shape with protocol value1, while Google Antigravity returns the v1 shape with protocol value2. After negotiation, v1 and v2 use separate method and lifecycle handlers behind one provider-neutral client.ACP v2 adds accepted-prompt plus idle-state completion, authoritative message upserts, usage and session metadata, rich plan updates, config changes, compaction, and agent-owned terminal state. V1 Registry agents keep their existing prompt response, session load, client filesystem, and client terminal behavior.
Runtime and security boundary
Live proof
Pi through generic ACP
Fresh global install of
pi-acp@0.0.33, provider creation, model discovery, completed T3 turn, and global command verification:Google Antigravity through generic ACP
Registry discovery, binary download, provider creation, live ACP handshake, and Google authentication boundary:
The Antigravity test host was deliberately cleared of Google credentials. The proof therefore ends at the correctly advertised
Log in with Googlestate instead of claiming an authenticated model turn.Full parity matrix and prioritized generic ACP gaps
Verification
effect-acpassertions passed.effect-acpand server TypeScript checks passed.git diff --checkpassed.Authentication credentials remain owned by each agent and user. T3 does not store or automate generic provider credentials.
Built with GPT-5.6 Sol in T3 Code through the Codex harness.
Note
High Risk
Touches ACP protocol, authenticated MCP bridging, binary download/provisioning, and orchestration auth scopes. A regression can break agent sessions, leak credentials, or install untrusted binaries.
Overview
Adds first-class ACP Registry providers (search, prepare/install, session import, logout) and standardizes the orchestrator-v2 ACP boundary so generic agents no longer depend on per-provider hacks.
Registry discovery is read-scoped; prepare, uninstall, URL auth, import, and logout require operate. Icons are allowlisted to the official Registry CDN. Mobile catalogs, composers, and thread rows now show registry identity with a cached icon and generic ACP fallback.
ACP sessions inject an authenticated stdio MCP bridge (
t3 acp-mcp-bridge/acp-mcp-call) so agents that only speak stdio MCP still reach T3 tools.bin.tsfast-paths those commands before the full CLI graph loads. The mock agent andeffect-acpmove to protocol v2 (configId,tool_call_update/plan_update,state_updateidle, elicitation/permission subject shapes).Also warms checkpoint object hashing in the background so the first capture does not block first-message latency, and appends managed ACP binaries to the integrated terminal PATH.
Reviewed by Cursor Bugbot for commit 30466f3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add
acpRegistryprovider driver with catalog, v2 protocol, and session managementacpRegistrybuilt-in driver backed by anAcpRegistryCatalogservice supporting search, prepare, inspect, resolve, and managed-binary uninstall with size-bounded, checksum-verified downloads and global package install receiptseffect-acpto ACP protocol v2 (schema-v2.0.0-alpha.3) with a v1.21 compatibility layer; the client negotiates generation on initialize and normalizes all subsequent RPCs and notificationsAcpRegistryRuntimeCoordinatorto arbitrate foreground vs. background probes per agent and stream live available commands, session configuration, and URL-auth actions into provider snapshotssearchAcpRegistry,prepareAcpRegistryAgent,uninstallAcpRegistryManagedBinary,acceptAcpRegistryUrlAuth,listAcpRegistrySessions,importAcpRegistrySession,logoutAcpRegistry) and atomicProviderInstanceMutation(create/upsert/remove) with transactional settings persistence and secret rollback in serverSettings.ts\n- Ships web and mobile UI for registry search, identity wizard, ACP icon rendering with caching, session import/logout, URL-based auth acceptance, and context-window cost display; addsisProviderInstanceTextGenerationCapablegating across settings and model selection so non-text-generation instances are hiddenacp-mcp-bridgeandacp-mcp-callCLI fast paths in bin.ts to proxy T3's MCP endpoint over stdio for ACP agents without loading the full CLIAcpSessionRuntimeremovessetSessionModel; callers in GrokTextGeneration.ts and GrokAcpSupport.ts no longer set a base model.AcpClienthandler signatures now require anAcpRequestContextargument on all core and extension handlers.session/loadis replaced bysession/resumewithreplayFromsemantics. Out-of-tree consumers ofeffect-acpclient/agent APIs will break.Macroscope summarized 30466f3.