fix(agentgateway): log correlation-id on mcp tool call failure#196
Open
Lothnic wants to merge 2 commits into
Open
fix(agentgateway): log correlation-id on mcp tool call failure#196Lothnic wants to merge 2 commits into
Lothnic wants to merge 2 commits into
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR improves debuggability of AgentGateway MCP tool invocations by capturing and logging the x-correlation-id from Agent Gateway responses when a tool call fails, for both LoB and Customer agent flows.
Changes:
- Added
httpxresponse event hooks to capturex-correlation-idfrom response headers during MCP tool invocation. - Added error logging on tool call failures that includes the captured correlation id (or a fallback value).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/sap_cloud_sdk/agentgateway/_lob.py | Captures x-correlation-id via httpx response hooks during LoB MCP tool calls and logs it on failure. |
| src/sap_cloud_sdk/agentgateway/_customer.py | Captures x-correlation-id via httpx response hooks during Customer MCP tool calls and logs it on failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+386
to
+399
| correlation_id: str | None = None | ||
|
|
||
| async def _capture_correlation_id(response: httpx.Response) -> None: | ||
| nonlocal correlation_id | ||
| cid = response.headers.get("x-correlation-id") | ||
| if cid: | ||
| correlation_id = cid | ||
|
|
||
| try: | ||
| async with httpx.AsyncClient( | ||
| headers={ | ||
| "Authorization": f"Bearer {user_auth_token}", | ||
| "x-correlation-id": str(uuid.uuid4()), | ||
| }, |
Comment on lines
+416
to
+422
| except Exception: | ||
| logger.error( | ||
| "MCP tool call failed for '%s' (x-correlation-id: %s)", | ||
| tool.name, | ||
| correlation_id or "unknown", | ||
| ) | ||
| raise |
| return "" | ||
| first = result.content[0] | ||
| return str(getattr(first, "text", "")) | ||
| except Exception: |
Comment on lines
+554
to
+558
| correlation_id: str | None = None | ||
|
|
||
| async with httpx.AsyncClient( | ||
| headers={ | ||
| "Authorization": f"Bearer {auth_token}", | ||
| "x-correlation-id": str(uuid.uuid4()), | ||
| }, | ||
| timeout=timeout, | ||
| ) as http_client: | ||
| async with streamable_http_client(tool.url, http_client=http_client) as ( | ||
| read, | ||
| write, | ||
| _, | ||
| ): | ||
| async with ClientSession(read, write) as session: | ||
| await session.initialize() | ||
| result = await session.call_tool(tool.name, kwargs) | ||
| async def _capture_correlation_id(response: httpx.Response) -> None: | ||
| nonlocal correlation_id | ||
| cid = response.headers.get("x-correlation-id") |
|
|
||
| first = result.content[0] | ||
| return str(getattr(first, "text", "")) | ||
| except Exception: |
…and include the traceback for debugging. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
tiagoek
added a commit
that referenced
this pull request
Jul 6, 2026
Adds the SDK Module Review skill to this repository: ## What this PR adds **20 deterministic checks** (no LLM calls in CI): - secrets, license-spdx, disclosure, hardcode, telemetry, docs, bdd, patterns, versioning + BREAKING family, commits, errors-logging, testing-depth, http-hygiene, concurrency, deps-supply, deletion-hygiene, constants, binding-shape, quality-gate-parity, pr-size **Rule tiers:** SHADOW | FLAG | BLOCK | BLOCK_LOCKED, configured in `.claude/config/rules.yaml` **BLOCK_LOCKED rules cannot be suppressed:** - SEC-* (all secrets), HC-03 (SAP-internal URL leak), DIS-06 (internal artifactory), LIC-01/02 (SPDX headers), BND-02 (BTP token URL concat), BND-05 (binding logs credentials), BREAKING-* (all breaking-change declarations) **REUSE.toml aggregate exemption** — this repo uses REUSE.toml at root, so per-file SPDX rules are baseline-exempted automatically. **Cross-language BDD parity** — module-aliases.yaml seeded with `dms ↔ documentmanagement` (Java sibling). **4 PR signals posted per run:** 1. Inline comments per finding 2. Aggregated summary comment 3. GitHub check-run (Checks tab) 4. Status label (`sdk-review: ✅ passed` / `❌ blocked` / `⚠️ flagged`) **Idempotent re-runs:** HTML markers let subsequent runs replace prior artifacts rather than duplicate them. ## Structure - `.claude/scripts/`: 20 check-*.sh + orchestrate.sh + aggregate.sh - `.claude/scripts/lib/`: 12 shared libs (2 Python AST + 10 bash) - `.claude/config/`: rules.yaml, module-aliases.yaml, baseline.json - `.github/workflows/sdk-module-review.yml`: auto-trigger on every PR - `.github/workflows/sdk-skill-isolation-check.yml`: validates sub-PRs - `docs/PR-REVIEW.md`: user-facing docs - `docs/BRANCH-PROTECTION-SETUP.md`: admin guide for required-check config - `tests/sdk-review/`: 57 bats tests + 6 fixtures (placeholder secrets, not real credentials) - `CONTRIBUTING.md`: amended — sdk-module-review is required before review ## Validation - 57/57 bats tests passing - shellcheck clean on all bash scripts - E2E on 5 real PRs (#196, #191, #192, #185 in this repo + Java jv#9): zero false positives observed - Self-test on this PR: 0 BLOCK, 0 FLAG (skill correctly skips its own files) - Flagship BND-02 finding verified on Java new-module PR (token URL concat) ## Rollout Recommended SHADOW → FLAG → BLOCK progression, 2 weeks each. See `docs/BRANCH-PROTECTION-SETUP.md`. ## Note on test fixtures `tests/sdk-review/fixtures/secrets-*.diff` contain intentional placeholder strings that match secret regexes (e.g. `AKIATESTFIXTUREXXXXX`, `eyJTESTFIXTURE...`) so the check-secrets rule can be unit-tested. These are NOT real credentials. Ref: https://jira.tools.sap/browse/AFSDK-3937
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Log the x-correlation-id from AGW response headers when an MCP tool call fails, so users can search AGW logs for debugging. The correlation-id is captured via httpx event hooks in both the LoB and Customer agent flows.
Related Issue
Closes #195
Link to the issue
Type of Change
Please check the relevant option:
How to Test
Describe how reviewers can test your changes:
Checklist
Before submitting your PR, please review and check the following: