Repository navigation
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, self-contained error-reporting fix that preserves protocol behavior while surfacing provider reasons through existing redaction and length limits. Tests cover both nested session failures and ACP SDK error-detail formats, with no schema, deployment, or configuration changes. You can add or adjust custom eligibility rules. Learn more. |
Dismissing prior approval to re-evaluate 432b080
|
@coderabbitai review |
|
432b080 to
abf62c2
Compare
|
@coderabbitai Review |
|
abf62c2 to
aa00982
Compare
0bc3543 to
72d1b59
Compare
Dismissing prior approval to re-evaluate 72d1b59
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
72d1b59 to
3003ff3
Compare
Problem
When a provider session fails to open, the thread shows only T3's wrapper text, e.g.
Failed to open acpRegistry provider session provider-session:provider-instance:acpRegistry_factory_droid:thread:…. The provider's actual reason is dropped in two places:AcpRequestError.messagereturns only the JSON-RPCmessage("Internal error") and ignoresdata, where ACP SDKs put the real reason.Repro: Factory Droid 0.233.0 through the ACP registry. Its
session/newanswers{"code":-32603,"message":"Internal error","data":{"details":"Droid process exited unexpectedly (exit code 1)"}}(a Droid bug, Factory-AI/factory#9), and T3 shows the session id instead.Change
AcpRequestError.messagenow appends the reason ACP SDKs carry indata:{ details }from the TypeScript and Python SDKs, a plain string from the Rust SDK. It's skipped when it restates the message. The wire encoding is unchanged:toProtocolErrorstill sendserrorMessageanddataseparately.ProviderSessionOpenErrorand adapter errors to the first error underneath, for every provider.ProviderAdapterProtocolErrorstops the walk because it carries its own detail. This also surfaces the stderr excerptAcpProcessExitedErroralready captures when an agent crashes on start..messageinstead oferrorMessage.makeProviderFailuretreatsAcpRequestErroras the agent's own reason, so flavors without a hook (Antigravity) stop showing "Provider turn failed."All provider text still goes through the existing redaction and 4,096-character bound. No contract or client changes.
Out of scope: working around Droid's own crash (reported upstream).
Scope and approval
Small, focused fix for an obvious bug: T3 already receives the provider's error and discards it before it reaches the user. Nothing changes beyond showing that text.
Verification
vp test run packages/effect-acp/src/errors.test.ts packages/provider-core/src/server/failure.test.ts apps/server/src/orchestration-v2/ProviderTurnStartService.test.ts: 36 passed on the latestmainrebase. The 2 new tests fail with the source changes reverted, and passed 20/20 repeated runs.AcpAdapterV2,AcpRegistryAdapterV2,GrokAdapterV2,AntigravityAdapterV2,provider/acp/*, Antigravity auth,packages/effect-acp): 724 passed, 1 failed. The failure,AcpSessionRuntime.processTree"contains a packaged-runtime command without requiring cgroup delegation", is pre-existing: it fails the same way onmain. After the latest rebase,GrokAdapterV2andAcpRegistryAdapterV2re-run: 24 passed.tsc --noEmitinapps/server,packages/provider-core,packages/provider-grokandpackages/effect-acp: 0 errors.vp run build:bundleinapps/server: builds.vp linton the changed files: 2 pre-existing warnings inpackages/provider-grok/src/server/adapter.ts, none on changed lines.@agentclientprotocol/sdk1.7.0 (TypeScript),agent-client-protocol2.2.0 (Rust) andagent-client-protocol0.12.1 (Python) encodedatafor uncaught agent errors.Built and verified with Opus 5.5 in Claude Code.