Sign up
View stargazersSign up

fix(provider-acp-registry): surface agent error message on sign-in failure#30295

merged1
main
GitHub fork: fix/acp-signin-error-message
opened8d agobyIiScabrous
2 files changed
View on GitHubTwo-way sync enabledPR last imported 4d ago

Summary

When a local or registry ACP agent responds to authenticate or session verification with a JSON-RPC error, the UI previously only showed the generic message (The ACP agent could not complete sign-in. or The provider could not create a session after sign-in.), completely hiding the actionable cause from AcpRequestError.

This PR surfaces the agent's error message from cause on failure:

  • Unwraps nested Cause objects using Cause.squash.
  • Appends the trimmed agent errorMessage from validated AcpRequestError instances to the failure detail.
  • Handles clean punctuation and avoids duplicate periods or trailing spaces.
  • Clamps excessively long agent error messages (proxy/HTML pages) to 500 characters to prevent UI breaks.
  • Retains standard safe detail messages if cause is not an AcpRequestError or is empty.

Closes #30218

Checks

24 successful · 13 skipped
All checks passed
aed7b39

Added 1 commit

  • fix(provider-acp-registry): surface agent error message on sign-in failureaed7b39
MmacroscopeappBot8d ago
Synced from GitHub

Approvability

Verdict: Approved at aed7b39

Macroscope's review found this PR approvable — ACP setup failures now include the agent's error message, trimmed and length-limited, instead of only generic text. Authentication behavior and credential handling remain unchanged.

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

CcoderabbitaiBot8d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

ACP authentication failures now include a trimmed agent error message when the cause is an AcpRequestError. Messages longer than 500 characters are truncated. The original cause remains in ProviderSetupError. Tests cover authentication and verification failures, nested causes, and message formatting.

Changes: ACP authentication error details

Layer / File(s)Summary
Format and verify ACP failure details
packages/provider-acp-registry/src/server/auth.ts, packages/provider-acp-registry/src/server/auth.test.ts		formatFailureDetail appends a trimmed ACP error message to the failure detail and truncates messages longer than 500 characters. The failure path retains the original cause. Tests cover message formatting and sign-in failure cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related issues

  • pingdotgg/t3code#17508 reports hidden ACP authentication error messages. This change addresses the ACP Registry path, but not the Antigravity-specific path.

Possibly related PRs

  • pingdotgg/t3code#19661 changed the same auth module’s failure details for a recognized browser sign-in error.

Suggested reviewers: juliusmarminge, macroscopeapp

Merge Risk: External image Minimal · up to aed7b

ACP error details reach the sign-in UI. No merge-blocking issue was identified.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description covers the problem (generic sign-in messages hide the AcpRequestError cause), the change (Cause.squash unwrapping, trimmed errorMessage appended, punctuation cleanup, 500-character cap…Add a Verification section. List the focused tests run (for example, the auth.test.ts cases for formatFailureDetail and the sign-in failure paths) and the observed results. State anything not checked. Add a Scope and approval section that e…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title names the primary change: ACP sign-in failures now surface the agent's error message. It follows conventional commit form, names the affected package, and matches the summary, the linked iss…
Linked Issues checkPassedIssue #30218 requires ACP agent messages for authentication failures and post-sign-in verification failures. auth.ts passes each operation cause to formatFailureDetail. The formatter unwraps `Caus…
Out of Scope Changes checkPassedThe changes are limited to ACP authentication failure formatting and automated tests in packages/provider-acp-registry/src/server. The exported formatter supports direct unit testing of the linked b…
Full details: Title check

Explanation

The title names the primary change: ACP sign-in failures now surface the agent's error message. It follows conventional commit form, names the affected package, and matches the summary, the linked issue, and the code changes in auth.ts and auth.test.ts.

Full details: Linked Issues check

Explanation

Issue #30218 requires ACP agent messages for authentication failures and post-sign-in verification failures. auth.ts passes each operation cause to formatFailureDetail. The formatter unwraps Cause, validates AcpRequestError, trims and appends errorMessage, and preserves the generic detail for unsupported or empty causes. It limits long messages to 500 characters. Tests cover authentication, terminal verification, agent-auth verification, nested causes, empty messages, punctuation, and truncation.

Full details: Out of Scope Changes check

Explanation

The changes are limited to ACP authentication failure formatting and automated tests in packages/provider-acp-registry/src/server. The exported formatter supports direct unit testing of the linked behavior. No unrelated product behavior or unrelated files changed.

Full details: Description check

Explanation

The description covers the problem (generic sign-in messages hide the AcpRequestError cause), the change (Cause.squash unwrapping, trimmed errorMessage appended, punctuation cleanup, 500-character cap, fallback to safe detail), and a closing issue reference. It does not follow the template's Scope and approval section, which asks for a linked triaged issue or maintainer approval. Its Verification section is missing, so it does not state which tests were run or what they showed. The required template sections for Verification and Scope are therefore incomplete.

Resolution

Add a Verification section. List the focused tests run (for example, the auth.test.ts cases for formatFailureDetail and the sign-in failure paths) and the observed results. State anything not checked. Add a Scope and approval section that explains the link to issue #30218 and the maintainer approval, or the reason the fix qualifies without one. Add the Problem and Change headings from the template, and keep the template's reference to the conventional commit title.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 30724e2e9ae1c2f5da495b590e22e41e425b8db5 and aed7b39952892856b224425251411a652e5fe8b3.

External image Files selected for processing: 2
  • packages/provider-acp-registry/src/server/auth.test.ts
  • packages/provider-acp-registry/src/server/auth.ts

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

Mmaria7d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified head aed7b39952892856b224425251411a652e5fe8b3 against github/main at 15707a84b0, using separate checkouts and one dev server at a time. drove the web app with repo playwright and google chrome on this linux vm. no source changes.

added the real gemini cli 0.66.0 through settings → providers. with gemini-api-key selected and no key configured, the real agent accepted authenticate and rejected session/new with "Gemini API key is missing or not configured."

  • main displayed only "The provider could not create a session after sign-in."
  • this head appended the agent's error. retry produced the same detail and kept the provider unauthenticated.
  • selecting vertex-ai completed real acp authentication/session creation and showed "Signed in." this checks session setup, not external inference.
  • added a project pointing at this git repo through the app. codex gpt-6.1-sol completed a first turn and a follow-up through the web client and local proxy.

main's unbounded gemini process stalled during cleanup after returning its error. both comparison runs therefore used the same real executable through a 30-second timeout launcher. no protocol responses were injected. the original agent error arrived before cleanup.

blacksmith passed the focused auth suite (8 tests) and vp run check:changed cefb544af3c98224874419198c0359b652ca3c94: format, lint, knip, 10 package typechecks, and 9 tests. the shallow runner needed a depth-2 fetch of the pr head. all 30 reported ci checks were successful.

short runtime transcript

main hides the real gemini session error

pr displays the real gemini session error

retry sign-in returns the actionable error

successful acp session setup with vertex-ai

two real codex turns complete in the client

Not verified

real agent authenticate json-rpc rejection and terminal-login failure were not reproduced; the focused tests cover those failure branches. successful external gemini inference, native desktop/mobile, and relay/tunnel connections were not exercised. the unbounded gemini cleanup stall remains outside this pr's message-formatting change.