fix(mcp): stop false-flagging successful responses with error_type:null as failures (#42580) - #42736
Conversation
…payloads (#42580) LoggingMiddleware._is_error_response substring-matches for the literal "error_type" key, but response schemas like ExecuteSqlResponse always serialize an "error_type" field (null on success), so every successful execute_sql call is misclassified as a failure and logged with success=False.
Code Review Agent Run #bda4efActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42736 +/- ##
==========================================
+ Coverage 65.57% 65.59% +0.01%
==========================================
Files 2818 2819 +1
Lines 160038 160168 +130
Branches 36557 36569 +12
==========================================
+ Hits 104942 105055 +113
- Misses 53051 53064 +13
- Partials 2045 2049 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ll as failures (#42580) _is_error_response did a raw substring search for '"error_type"' in the serialized tool response, but success schemas (ChartError-adjacent response models) declare error_type as an optional field defaulting to null, so every successful call's response contains that substring too -- logging it as a failure regardless of outcome. Parse the response as JSON and check error_type for a truthy value instead of merely being present, matching how the real error schemas (DashboardError, etc.) always populate it with a real type string on failure and never on success. Also corrects a pre-existing test fixture that modeled error_type nested under an "error" object -- no schema in the codebase produces that shape; error is always a plain message string with error_type as its sibling.
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #1f34e6Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified the success/failure determination behaves correctly across the null-vs-absent-vs-real distinction.
Predicate — old '"error_type"' in result.content[0].text (substring, shape-blind) → new bool(isinstance(payload, dict) and payload.get("error_type")) over the parsed JSON:
error_typeabsent →.getNone → success ✅error_type: null→ None → success ✅ (the #42580 bug)error_type: "validation_error"(real value) → truthy → failure ✅
No under-reporting of real failures. All error responses derive from MCPBaseError (common/error_schemas.py), which declares a required, top-level, non-empty error_type: str, so genuine errors still serialize a truthy top-level value and are still flagged. The raised-exception path (error_type = type(exc).__name__, success = False) and the separate isError encoding path in StructuredContentStripperMiddleware are untouched — no collateral damage. Success is derived from this single signal, consistent with the pre-existing design and non-conflicting with the error-path work in #42730.
Edge cases: error_type: "" → treated as success — acceptable, since the schema contract makes an empty discriminator a non-error; 0/False are unreachable given the str | None typing. Only top-level error_type is inspected, which matches the real serialized shape (the old test's nested "error": {"error_type": ...} fixture was an artifact of the substring check, correctly updated here).
Tests cover both directions, non-vacuously. Reverting only the production predicate makes test_success_response_with_null_error_type_not_detected_as_error fail (null→success), while test_on_call_tool_does_not_extract_id_on_failed_response guards real-error→failure against the actually-emitted curated_payload["success"]. Verified locally.
3 participants
SUMMARY
Fixes #42580: successful
execute_sql(and other tool) calls were logged as failures in the MCP audit log.Root cause.
LoggingMiddleware._is_error_responsedecided success/failure with a raw substring search,'"error_type"' in result.content[0].text. Success response schemas (the ones adjacent toChartError/DashboardError) declareerror_typeas an optional field defaulting tonull, for a uniform response shape — so every successful call's serialized JSON contains that substring too, and gets logged assuccess=False.Fix. Parse the response body as JSON and check whether
error_typeis actually truthy, not merely present. The real error schemas (e.g.DashboardError) always populateerror_typewith a concrete type string on failure and never on success, so this is a reliable discriminator where the substring check wasn't.Also corrected a pre-existing test fixture (
test_on_call_tool_does_not_extract_id_on_failed_response) that modelederror_typenested under an"error"object — no schema in the codebase actually produces that shape;erroris always a plain message string witherror_typeas its sibling field, not a container.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend audit-logging fix, no UI change.
TESTING INSTRUCTIONS
39 passed. Also ran the full
tests/unit_tests/mcp_service/suite (3134 tests): 1 pre-existing unrelated failure (test_tools_call_health_check_over_real_asgi_transport, a test-isolation FK-constraint/version-field issue, confirmed unrelated by running it in isolation both before and after this change).ADDITIONAL INFORMATION