[prashantrakheja]

What

• Widen the mcp dependency ceiling from <2 to <3 — the SDK now supports mcp 2.x as well as 1.x.
• Guard the AttributeError: 'NoneType' object has no attribute 'isError' crash: when the gateway returns a malformed/empty result, surface a clean AgentGatewayServerError to the caller instead of letting the opaque crash escape.

Breaking changes

None. The mcp<3 widening is permissive; existing mcp 1.x users are unaffected.

[prashantrakheja]

PR Review: #418: feat(agentgateway): support mcp 2.x and guard the malformed-result crash

Author: prashantrakheja Branch: fix/agw-mcp-malformed-result-guard → main
Verdict: ⚠️ Needs Minor Work
Summary: Correct, well-tested, well-logged fix that widens the mcp ceiling to <3 and guards the opaque 'NoneType' … isError crash; only process/doc items (AI disclosure, PR template completeness) are flagged, no code blockers.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	⚠️	Body is a concise What/Breaking-changes; omits the repo template's Type-of-Change box, How-to-Test steps, and 9-item checklist.
A2	Conventional Commits	✅	Title feat(agentgateway): … is valid; Commit Validation CI job passes.
A3	Issue linked	⚠️	No Closes #N. Non-blocking (not all PRs track an issue).
A4	AI-generated code disclosure	⚠️	Change is AI-assisted but body does not disclose it or reference the SAP GenAI contribution guideline (CONTRIBUTING.md §"Contributing with AI-generated code").

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅	No credentials, internal URLs, tenant IDs, or customer names in the diff.
B2	No sensitive data in PR body	✅	Body is generic; no GUIDs, tenant IDs, or internal references.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅	Code Quality, Commit Validation, Build, REUSE, Version Bump all green; test/CodeQL jobs re-running at review time.
C2	Version bump	✅	pyproject.toml version held at 0.61.0 (manual bump reverted); Check Version Bump passes.
C3	Type hints	✅	New helpers make_mcp_error(code: int, message: str) -> "McpError" and `find_malformed_result_error(exc: BaseException) -> "AttributeError
C4	No hardcoded values	✅	JSON-RPC codes passed through from the caught error; no magic literals introduced.
C5	Import organization	✅	New symbols imported at module top (_customer.py:36-37). The one in-function from mcp.types import ErrorData (_compat.py:33) is a deliberate mcp-1.x-only fallback inside the version shim.
C6	Naming conventions	✅	find_malformed_result_error, make_mcp_error snake_case; private module _compat.py.
C7	No unused code	✅	All new helpers are referenced; test_lob.py removes a duplicate ConsumptionLevel import (net cleanup).
C8	No unjustified new dependencies	✅	No new dependency; only widens existing mcp ceiling <2 → <3. Locked mcp (1.29.1) satisfies both, so uv.lock changes only the recorded specifier (+1/-1).
C9	Proto code freshness	➖	No .proto changes.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	➖	No new public config/behavior surface.
D2	Public API hygiene	✅	New helpers live in private _compat.py; nothing added to __all__.
D3	Breaking changes marked	✅	None; mcp<3 widening is permissive and the body states so.
D4	Pagination & tenant filtering	➖	No list/query operations added.
D5	Telemetry instrumentation	➖	No new client method or module.
D6	Multi-tenancy support	➖	No new service module; tenancy surface unchanged.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅	Both changed src/ files have matching tests: test_compat.py (+44) covers find_malformed_result_error bare/grouped/nested/no-match; test_customer.py (+139) covers the AttributeError→AgentGatewayServerError mapping and re-raise of unrelated AttributeErrors; test_agw_client.py (+56) pins the bare-error surfacing (asserts no NoneType/isError leak).
E2	Documentation quality	✅	New helpers carry thorough Google-style docstrings explaining the cross-major rationale; no public API signature changed.
E2a	Multi-tenancy documentation	➖	No new/modified user-guide.md.
E3	Module structure compliance	➖	No new module.

---

❌ Blocking Issues

None.

⚠️ Non-Blocking Suggestions

• [A4]: Add an AI-generated-code disclosure to the PR body referencing the SAP GenAI contribution guideline.
• [A1]: Consider expanding the body to the repo PR template (Type of Change, How to Test, checklist) for reviewer completeness.
• [A3]: Link a tracking issue with Closes #N if one exists for the deployed 'NoneType' … isError crash.

✅ Things Done Well

• Error-handling order is correct and defensive: protocol McpError → malformed/None-result AttributeError → generic re-raise, each matched structurally (walks BaseExceptionGroup trees) rather than by fragile string checks alone.
• Every branch logs with structured, self-contained context (tool name, URL, server name, error type + message) — a reader can reconstruct the failure from logs alone.
• The guard is validated to surface a BARE AgentGatewayServerError with no NoneType/isError string leaking to the LLM — directly meeting the "errors cleanly surface, agent loop doesn't crash" bar.
• Dependency widening is minimal and honest: ceiling-only change, lock diff is a single specifier line, no resolution churn.