Skip to content

fix(server): provider session failures show the provider's reason - #14969

Closed
Derpedyea wants to merge 1 commit into
pingdotgg:mainfrom
Derpedyea:fix/provider-failure-reason
Closed

Derpedyea wants to merge 1 commit into
pingdotgg:mainfrom
Derpedyea:fix/provider-failure-reason

Conversation

@Derpedyea

@Derpedyea Derpedyea commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

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.message returns only the JSON-RPC message ("Internal error") and ignores data, where ACP SDKs put the real reason.
  • The start-failure item reads one level below the session error. That level is the adapter's wrapper, which only names the session id.

Repro: Factory Droid 0.233.0 through the ACP registry. Its session/new answers {"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.message now appends the reason ACP SDKs carry in data: { 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: toProtocolError still sends errorMessage and data separately.
  • Session-open and thread-load failures read through ProviderSessionOpenError and adapter errors to the first error underneath, for every provider. ProviderAdapterProtocolError stops the walk because it carries its own detail. This also surfaces the stderr excerpt AcpProcessExitedError already captures when an agent crashes on start.
  • ACP prompt failures get the same text. The registry and Grok hooks read .message instead of errorMessage. makeProviderFailure treats AcpRequestError as 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 latest main rebase. The 2 new tests fail with the source changes reverted, and passed 20/20 repeated runs.
  • Adjacent ACP suites (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 on main. After the latest rebase, GrokAdapterV2 and AcpRegistryAdapterV2 re-run: 24 passed.
  • tsc --noEmit in apps/server, packages/provider-core, packages/provider-grok and packages/effect-acp: 0 errors. vp run build:bundle in apps/server: builds. vp lint on the changed files: 2 pre-existing warnings in packages/provider-grok/src/server/adapter.ts, none on changed lines.
  • Captured Droid 0.233.0's real responses over stdio to get the error shapes. Checked how @agentclientprotocol/sdk 1.7.0 (TypeScript), agent-client-protocol 2.2.0 (Rust) and agent-client-protocol 0.12.1 (Python) encode data for uncaught agent errors.
  • Not checked: an end-to-end run in a live client.

Built and verified with Opus 5.5 in Claude Code.

@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 Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 72d1b59

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.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7e42f6a0-1e8b-41c3-acd5-e853d9e5cd22
📥 Commits

Reviewing files that changed from the base of the PR and between abdf84d and 432b080.

📒 Files selected for processing (1)
  • packages/effect-acp/src/errors.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

ACP request errors now include supported protocol details in their messages. Server provider failure paths use these messages and unwrap eligible nested adapter errors when reporting final-attempt startup failures.

Changes

ACP Error Reporting

Layer / File(s) Summary
ACP protocol error details
packages/effect-acp/src/errors.ts, packages/effect-acp/src/errors.test.ts
AcpRequestError.message now includes supported detail text and avoids repeating the protocol message. Tests cover object details and a string that repeats the message.
Server provider failure reporting
apps/server/src/orchestration-v2/ProviderFailure.ts, apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts, apps/server/src/orchestration-v2/ProviderTurnStartService.ts, apps/server/src/orchestration-v2/ProviderTurnStartService.test.ts
Failure handling recognizes ACP request messages. Final-attempt startup failures unwrap eligible nested adapter errors. The test checks that a nested ACP internal error appears in the failed turn item.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 432b0

The change surfaces supported ACP details in provider failure messages without an established regression. No concrete merge-blocking risk was found.

Architecture Summary

Architecture risk: 🔵 Low · up to 432b0

The change affects 2 systems.

Changed systems: apps/server, packages/effect-acp

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 5 changed files map to changed impact.
  • observed — packages/effect-acp (library) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.ts: acpRegistryPromptFailure now uses cause.message for ACP request errors instead of cause.errorMessage; the existing error classification and code handling remain unchanged.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts: For ACP request errors without an XAiPromptFailureText cause, the failure message now comes from cause.message instead of cause.errorMessage; the specialized X.AI failure text remains preferred.
  • observed — Modified behavior in apps/server/src/orchestration-v2/ProviderFailure.ts: causeMessage now treats AcpRequestError as a recognized cause and extracts its message field; previously, this cause had no dedicated matching case.
  • observed — Modified behavior in apps/server/src/orchestration-v2/ProviderTurnStartService.test.ts: Adds the AcpRequestError import used to construct the nested ACP failure in the new test.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 8 files.
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 and concisely describes the main change: showing the provider's reason for session failures.
Description check Passed The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure, implementation, scope justification, test results, known pre-existing fai…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

Dismissing prior approval to re-evaluate 432b080

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@Derpedyea

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Derpedyea
Derpedyea force-pushed the fix/provider-failure-reason branch from 432b080 to abf62c2 Compare October 3, 2026 03:27
@Derpedyea

Copy link
Copy Markdown
Contributor Author

@coderabbitai Review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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