test(e2e): skip the three Datadog MCP tool-call tests pending LIT-5052 - #35380
Conversation
All three send a `telemetry` object in the arguments to Datadog's search_datadog_logs tool. Datadog tightened that tool's input schema to reject unknown properties, so every call now fails validation with 'unexpected additional properties ["telemetry"]' before the behavior each test exists to prove is reached. `telemetry` was never a documented Datadog parameter; the tests relied on the server ignoring extra properties. The proxy transmitted exactly what the tests supplied and surfaced the upstream error faithfully, so this is test-side. The covers markers and registry rows stay put: the collector counts a cell as covered only when a test pytest would actually run declares it, so skipping hands all four cells back to the gap list where they belong.
Greptile SummaryThis PR temporarily disables three Datadog-backed MCP end-to-end tests.
Confidence Score: 4/5The PR appears safe to merge, but unnecessarily leaves four MCP coverage cells unexecuted when removing the rejected test-only argument would preserve the tests. The coverage registry honestly reports the skipped cells as gaps, but all three tests can retain their intended assertions without the unused Files Needing Attention: tests/e2e/mcp/test_mcp_datadog_e2e.py, tests/e2e/mcp/test_mcp_guardrail_e2e.py, tests/e2e/mcp/test_mcp_key_access_e2e.py
|
| Filename | Overview |
|---|---|
| tests/e2e/mcp/test_mcp_datadog_e2e.py | Skips the sole end-to-end coverage for API-key-authenticated MCP list-tools and call-tool success paths instead of removing the rejected argument. |
| tests/e2e/mcp/test_mcp_guardrail_e2e.py | Skips the MCP pre-call content-filter test, leaving its blocked and clean control paths unexecuted. |
| tests/e2e/mcp/test_mcp_key_access_e2e.py | Skips tool-call permission-denial coverage while leaving the separate list-tools denial test active. |
Reviews (1): Last reviewed commit: "test(e2e): skip the three Datadog MCP to..." | Re-trigger Greptile
| @pytest.mark.skip( | ||
| reason=( | ||
| "LIT-5052: this test sends a `telemetry` argument that Datadog's " | ||
| "search_datadog_logs tool now rejects, so every tool call fails validation with " | ||
| "'unexpected additional properties [\"telemetry\"]' before the round-trip " | ||
| "assertion is reached. `telemetry` was never a documented Datadog parameter; the " | ||
| "test relied on the server ignoring unknown properties. Unskip once the argument " | ||
| "is dropped." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Skips avoidable regression coverage
These unconditional skips remove the only executing e2e coverage for four MCP success, authorization, and guardrail contracts, even though telemetry is unused by their assertions and can be removed from the three tool calls. The coverage registry reports the resulting gaps honestly, but CI no longer detects regressions in these paths while the skips remain.
Rule Used: What: Flag any modifications to existing tests and... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
88ab22f
into
litellm_internal_staging
TLDR
High level flow for the user:
High level flow on a technical level:
telemetryargumentRelevant issues
Linear ticket
Refs LIT-5052
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This PR adds no product behavior, so there is nothing to curl a green result out of; what needs proving is that the upstream tool call is rejected and that the three tests can no longer reach their assertions. Evidence is from the stage e2e run on 2026-07-31 12:47 UTC, both namespaces, at stage image commit
16507f1174The client-side failure, byte-identical across all three tests and both suites:
The gateway logged the upstream rejection at each failure instant (13:33:59, 13:35:15, 13:35:22 UTC):
This is not a credential problem, not a Datadog outage, and not a proxy regression. Auth demonstrably works:
test_search_logs_finds_seeded_completionfailed at line 96, meaningdd_logs.poll_events_for_marker(...)had already returned data over the sameDD-API-KEY/DD-APPLICATION-KEYandclient.await_tool(...)had already listedsearch_datadog_logsover the authenticated MCP session. Onlytools/callargument validation failed. There is no 401, 403, 429 or 5xx frommcp.us5.datadoghq.comanywhere in the window. And nothing in the proxy injects the key;grep -rn "telemetry" litellm/proxy/_experimental/mcp_server/matches only two OpenTelemetry doc commentsThe argument is hardcoded in the tests themselves, at
tests/e2e/mcp/test_mcp_datadog_e2e.py:91,tests/e2e/mcp/test_mcp_guardrail_e2e.py:111andtests/e2e/mcp/test_mcp_key_access_e2e.py:73.telemetryhas never appeared in Datadog's documented parameter list forsearch_datadog_logs, so the tests were relying on the server silently ignoring unknown properties; Datadog's own docs note the MCP tools are "under significant development and are subject to change"After this PR, at commit
d62e5246de, all three skip with the reason attached:And the coverage collector hands the cells back rather than counting them covered:
make lint-e2e-basedpyrightreports0 errors, 0 warnings, 0 notesType
✅ Test
Changes
Adds a
pytest.mark.skipto the three MCP tests whose Datadog tool call carries the rejectedtelemetryargument, each with a reason naming LIT-5052 and describing which assertion is being starved. No product code changes and no other test is touched;test_list_tools_denied_without_permissionis deliberately left alone because it never calls the tool and still passesThe
@pytest.mark.coversmarkers and the registry rows are both left in place on purpose. The collector resolves skip state with pytest's own evaluator and counts a cell as covered only when a test that would actually run declares it, so skipping already returns all four cells to the gap list. Deleting the rows instead would shrink the denominator and make three P0 MCP cells disappear from the report rather than show up as the gap they now areThose four cells, uncovered until the skips lift:
mcp.list_tools.api_key.succeeds(P0),mcp.call_tool.api_key.succeeds(P0),mcp.call_tool.api_key.denied_without_permission(P0),guardrail.litellm_content_filter.pre_mcp_call.blocks(P1). The fix in LIT-5052 is to drop the argument, which is small; the skips are meant to be short-livedQA runbook
tests/e2e/mcp/test_mcp_datadog_e2e.py::TestDatadogMcpRoundTrip::test_search_logs_finds_seeded_completion - currently skipped; when unskipped it proves a chat completion seeded with a unique marker is findable through the real Datadog MCP server's search tool
/v1/mcp/serverwith the static DD-API-KEY / DD-APPLICATION-KEY headers, deferring its deletionsearch_datadog_logsto appear in the server's tool list, absorbing the registration lag across replicastelemetryargumenttests/e2e/mcp/test_mcp_guardrail_e2e.py::TestMcpToolCallGuardrail::test_content_filter_blocks_banned_keyword_in_tool_args - currently skipped; when unskipped it proves a content-filter guardrail set to pre_mcp_call blocks a banned keyword inside an MCP tool call's arguments while a clean call still passes
pre_mcp_callhook point with a banned keyword, deferring its deletiontelemetryargumenttests/e2e/mcp/test_mcp_key_access_e2e.py::TestMcpKeyWithoutAccessIsDenied::test_call_tool_denied_without_permission - currently skipped; when unskipped it proves a key without MCP permission cannot invoke a tool that a permitted key can
telemetryargumentFinal Attestation