Skip to content

improve(mcp): unwrap ExceptionGroups so real connection errors are visible#203

Open
IsDon wants to merge 1 commit into
dnhkng:mainfrom
IsDon:improve/mcp-error-visibility
Open

improve(mcp): unwrap ExceptionGroups so real connection errors are visible#203
IsDon wants to merge 1 commit into
dnhkng:mainfrom
IsDon:improve/mcp-error-visibility

Conversation

@IsDon

@IsDon IsDon commented May 27, 2026

Copy link
Copy Markdown
Contributor

Usability Improvement for 3rd party integrations.

Errors raised inside streamablehttp_client / sse_client / stdio_client get wrapped in one or more ExceptionGroup layers by anyio's task group machinery. The session_runner exception handler only logs '{exc}', which renders as 'unhandled errors in a TaskGroup (1 sub-exception)' — the actual cause is hidden behind the wrapper, sometimes through multiple nested layers.

Add a small _leaf_exceptions() helper that recursively descends ExceptionGroup chains and returns the leaf-level error messages. Log them explicitly in the warning, and emit the full traceback (also at WARNING level so debug logging doesn't need to be enabled) so anyone hitting MCP connection failures can see the real cause without attaching a debugger.

No behaviour change — purely a logging improvement. Affects only the failure path.

Summary by CodeRabbit

  • Bug Fixes
    • Improved error logging to show detailed exception causes and full tracebacks for connection-related failures, increasing visibility for troubleshooting.
  • Tests
    • Added tests that validate extraction and ordering of underlying exception messages and enforcement of an unwrap depth cap.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8cf15088-aa89-441b-8acd-a66720920901

📥 Commits

Reviewing files that changed from the base of the PR and between 56f597a and 62f9528.

📒 Files selected for processing (2)
  • src/glados/mcp/manager.py
  • tests/test_mcp_error_visibility.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/glados/mcp/manager.py

📝 Walkthrough

Walkthrough

This PR adds a traceback import and a private _leaf_exceptions(...) helper to recursively unwrap nested ExceptionGroup instances, and updates the MCP session runner to log flattened leaf exception messages and the full formatted traceback at WARNING level. Tests exercise unwrapping order and the depth cap.

Changes

Exception logging enhancement

Layer / File(s) Summary
Exception unwrapping, logging, and tests
src/glados/mcp/manager.py, tests/test_mcp_error_visibility.py
Adds traceback import and _leaf_exceptions() to flatten ExceptionGroup trees with a max depth, replaces the session runner's single warning with logs that include the flattened leaf messages and the full formatted traceback, and adds tests validating order and depth-capping behavior.

🎯 3 (Moderate) | ⏱️ ~20 minutes

A rabbit nudged the tangled trace,
Unwrapped each leaf with careful grace,
Warnings now sing the full bright line,
Depths capped neat, each cause defined,
Logs hop clear — no error left behind. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly and precisely summarizes the main change: adding exception unwrapping for ExceptionGroups to improve error visibility in MCP.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

…sible

Errors raised inside streamable_http_client / sse_client / stdio_client get
wrapped in one or more ExceptionGroup layers by anyio's task-group
machinery. The _session_runner handler only logged '{exc}', which renders
as 'unhandled errors in a TaskGroup (1 sub-exception)' - the actual cause
is hidden behind the wrapper, sometimes through several nested layers.

Add a small _leaf_exceptions() helper that recursively descends
ExceptionGroup chains and returns the leaf-level error messages. Log them
explicitly in the warning, and emit the full traceback (also at WARNING
level so DEBUG logging doesn't need to be enabled) so anyone hitting an MCP
connection failure can see the real cause without attaching a debugger.

No behaviour change - purely a logging improvement; affects only the
failure path. I believe this is worthwhile given the nature of the project
for tinkering and MCP integration use-cases.

Changes:
- Add _leaf_exceptions(exc, depth, max_depth): recursively flattens
  ExceptionGroup trees to "<Type>: <message>" leaf strings, depth-bounded.
- _session_runner logs the unwrapped leaves plus the full traceback at
  WARNING.
- Docstring on _leaf_exceptions (Args/Returns + depth-cap behaviour).
- tests/test_mcp_error_visibility.py: flat exception, group flattening,
  nested-group flattening, and the depth cap.

Merge note: if landing both MCP fixes, merge the streamable_http_client
auth fix (dnhkng#202, dnhkng#202) first. It
touches the same import block and _session_runner, so there is a small
overlap with this branch, but the two resolve manually without much
trouble - this change is best applied on top of dnhkng#202.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@IsDon IsDon force-pushed the improve/mcp-error-visibility branch from 56f597a to 62f9528 Compare May 30, 2026 01:46
@dnhkng

dnhkng commented May 31, 2026

Copy link
Copy Markdown
Owner

@coderabbitai. check the branch conflict

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the mergeable state of PR `#203`
gh pr view 203 --repo dnhkng/GLaDOS --json mergeable,mergeStateStatus,baseRefName,headRefName,title

# Also check if there are any conflicting files
gh pr view 203 --repo dnhkng/GLaDOS --json files

Length of output: 697



@dnhkng PR #203 has a merge conflict — the branch improve/mcp-error-visibility cannot be cleanly merged into main.

Here's a summary:

Field Value
Mergeable ❌ CONFLICTING
Merge State DIRTY
Conflicting file src/glados/mcp/manager.py

The PR touches src/glados/mcp/manager.py (the only modified file), and main has diverged there since the branch was created.

To resolve, the contributor (@IsDon) should:

# Option 1: Rebase onto main (keeps history clean)
git fetch origin
git checkout improve/mcp-error-visibility
git rebase origin/main
# Resolve conflicts in src/glados/mcp/manager.py, then:
git rebase --continue
git push --force-with-lease origin improve/mcp-error-visibility

# Option 2: Merge main into the branch
git fetch origin
git checkout improve/mcp-error-visibility
git merge origin/main
# Resolve conflicts, then commit and push

The conflict is isolated to src/glados/mcp/manager.py — the new test file tests/test_mcp_error_visibility.py is freshly added so it should not conflict.

@dnhkng

dnhkng commented May 31, 2026

Copy link
Copy Markdown
Owner

@coderabbitai. Offer the fix for the merge conflict

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the current main version of the conflicting file
echo "=== MAIN version ==="
git show origin/main:src/glados/mcp/manager.py

echo ""
echo "=== PR branch version ==="
git show origin/improve/mcp-error-visibility:src/glados/mcp/manager.py

Length of output: 23286


🏁 Script executed:

#!/bin/bash
# Fetch the PR branch file content via GitHub API
gh api repos/dnhkng/GLaDOS/contents/src/glados/mcp/manager.py \
  --jq '.content' \
  -H "Accept: application/vnd.github.v3.raw" \
  --raw-field ref=improve/mcp-error-visibility \
  2>/dev/null || \
gh api "repos/dnhkng/GLaDOS/contents/src%2Fglados%2Fmcp%2Fmanager.py?ref=improve/mcp-error-visibility" \
  --jq '.content' | base64 -d

Length of output: 838


🏁 Script executed:

#!/bin/bash
# Get the raw diff for PR `#203`
gh pr diff 203 --repo dnhkng/GLaDOS

Length of output: 4606



@dnhkng Here is the full resolution. The conflict has two roots:

  1. Import line — main added AsyncIterator to from collections.abc import ...; the PR branch still had the old single-name import.
  2. _leaf_exceptions() insertion point — main inserted the _http_transport async context manager in exactly the same spot where the PR inserts _leaf_exceptions().

Resolved patch for src/glados/mcp/manager.py

Apply these two hunks after rebasing/merging main in:

--- a/src/glados/mcp/manager.py
+++ b/src/glados/mcp/manager.py
@@ -3,6 +3,7 @@
 import subprocess
 import threading
 import time
+import traceback
 from collections.abc import AsyncIterator, Iterable
 from concurrent.futures import TimeoutError as FuturesTimeoutError
 from dataclasses import dataclass
@@ -48,6 +49,37 @@ async def _http_transport(url: str, headers: dict[str, str]) -> AsyncIterator[A
             yield streams
 
 
+def _leaf_exceptions(exc: BaseException, depth: int = 0, max_depth: int = 8) -> list[str]:
+    """Recursively descend ExceptionGroup chains to surface leaf exceptions.
+
+    anyio task groups wrap failures in one or more ``ExceptionGroup`` layers,
+    so logging the top-level exception only shows "unhandled errors in a
+    TaskGroup". This flattens that tree to the underlying causes.
+
+    Args:
+        exc: The exception to unwrap. Any object exposing an ``exceptions``
+            attribute (e.g. ``ExceptionGroup``) is treated as a group and
+            descended; anything else is treated as a leaf.
+        depth: Current recursion depth (internal; callers use the default).
+        max_depth: Safety bound on recursion. Beyond it, descent stops and the
+            current node is reported with an "(unwrap depth exceeded)" prefix
+            rather than recursing further.
+
+    Returns:
+        A flat list of ``"<ExceptionType>: <message>"`` strings, one per leaf
+        exception (or one entry if ``exc`` is not a group).
+    """
+    if depth > max_depth:
+        return [f"(unwrap depth exceeded) {type(exc).__name__}: {exc}"]
+    subs = getattr(exc, "exceptions", None)
+    if subs:
+        out: list[str] = []
+        for sub in subs:
+            out.extend(_leaf_exceptions(sub, depth + 1, max_depth))
+        return out
+    return [f"{type(exc).__name__}: {exc}"]
+
+
 class MCPError(RuntimeError):
     pass
@@ -240,7 +272,15 @@ async def _session_runner(self, config: MCPServerConfig) -> None:
             except asyncio.CancelledError:
                 break
             except Exception as exc:
-                logger.warning(f"MCP: server '{config.name}' connection failed: {exc}")
+                # Recursively unwrap nested ExceptionGroups (Python 3.11+) so the
+                # actual leaf exception(s) are visible.
+                leaves = _leaf_exceptions(exc)
+                logger.warning(
+                    f"MCP: server '{config.name}' connection failed: leaves={leaves}"
+                )
+                # Full traceback to WARNING so we don't need DEBUG level enabled.
+                tb = "".join(traceback.format_exception(type(exc), exc, exc.__traceback__))
+                logger.warning(f"MCP: full traceback for '{config.name}':\n{tb}")
                 if self._observability_bus:
                     self._observability_bus.emit(
                         source="mcp",

What to tell @IsDon

git fetch origin
git checkout improve/mcp-error-visibility
git rebase origin/main

When the rebase stops on src/glados/mcp/manager.py, the two conflict blocks to fix manually are:

Block 1 — imports (keep AsyncIterator and add traceback):

import time
import traceback
from collections.abc import AsyncIterator, Iterable

Block 2 — function insertion (keep _http_transport from main, then add _leaf_exceptions right before class MCPError):

        async with streamable_http_client(url, http_client=client) as streams:
            yield streams


def _leaf_exceptions(exc: BaseException, depth: int = 0, max_depth: int = 8) -> list[str]:
    # ... (full body as in the PR)


class MCPError(RuntimeError):
    pass

Then:

git add src/glados/mcp/manager.py
git rebase --continue
git push --force-with-lease origin improve/mcp-error-visibility

The new test file tests/test_mcp_error_visibility.py is untouched and will rebase cleanly. 🐇

@dnhkng

dnhkng commented Jun 8, 2026

Copy link
Copy Markdown
Owner

@IsDon Can you check this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants