You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/04/24 18:59:31 UTC
[PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
aminghadersohi opened a new pull request, #39634:
URL: https://github.com/apache/superset/pull/39634
## Summary
The MCP service logged ALL tool errors at `ERROR` level regardless of cause. When an LLM calls a tool with wrong parameters (a `ValidationError`) or tries a tool it lacks access to (`PermissionError`), it was logged at the same severity as a database outage. This creates noise in error monitoring and makes real system issues hard to spot.
**Changes:**
- Add `_is_user_error()` classification helper that checks against `_USER_ERROR_TYPES` tuple and `SupersetException.status` attribute
- **User errors (WARNING)**: `ValidationError`, `PermissionError`, `ValueError`, `FileNotFoundError`, `ToolError`, `CommandInvalidError`, `ObjectNotFoundError`, `ForbiddenError`, `SupersetSecurityException`, any `SupersetException` with `status < 500`
- **System errors (ERROR)**: `OperationalError`, `TimeoutError`, `ConnectionError`, generic `Exception`, `SupersetException` with `status >= 500`
- Add new handler branches for `SupersetException`, `CommandInvalidError`, `ObjectNotFoundError`, `ForbiddenError`, `ConnectionError` (previously all fell through to generic "Unexpected error" catch-all)
- Add `severity` field to event_logger payload for analytics
- Demote response-size-blocked log from ERROR to WARNING (user asked for too much data)
- Demote validation re-raise logs in `schema_utils.py` to WARNING (avoid double-logging)
- Demote catch-and-reraise helper in `mcp_core.py` to WARNING
## Testing
- Added parametrized unit tests for `_is_user_error()` covering all error types
- Added async integration tests for `GlobalErrorHandlerMiddleware` verifying correct log levels
- All existing middleware tests continue to pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4315617540
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39634?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `26 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.44%. Comparing base ([`dd18b2e`](https://app.codecov.io/gh/apache/superset/commit/dd18b2eb5402cadfcb13f5bb5c4f67ea6b8b8ad8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de4ff6c`](https://app.codecov.io/gh/apache/superset/commit/de4ff6c8376a6d4edf78482b52315024c579e05a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 51 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39634?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/middleware.py](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fmiddleware.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvbWlkZGxld2FyZS5weQ==) | 0.00% | [23 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/utils/schema\_utils.py](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&filepath=superset%2Fmcp_service%2Futils%2Fschema_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvdXRpbHMvc2NoZW1hX3V0aWxzLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/mcp\_core.py](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fmcp_core.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvbWNwX2NvcmUucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (dd18b2e) and HEAD (de4ff6c). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (dd18b2e) | HEAD (de4ff6c) |
>|------|------|------|
>|python|20|2|
>|presto|2|1|
>|hive|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39634 +/- ##
==========================================
- Coverage 64.54% 55.44% -9.11%
==========================================
Files 2559 2563 +4
Lines 133496 133793 +297
Branches 31028 31071 +43
==========================================
- Hits 86168 74180 -11988
- Misses 45836 58960 +13124
+ Partials 1492 653 -839
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.77% <0.00%> (-0.06%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.55% <0.00%> (-0.07%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.62% <0.00%> (-20.43%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39634/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39634?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3150817741
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -119,7 +119,7 @@ async def create_virtual_dataset(
error=str(messages),
)
except DatasetCreateFailedError as exc:
- await ctx.error("Virtual dataset creation failed: %s" % (str(exc),))
+ await ctx.warning("Virtual dataset creation failed: %s" % (str(exc),))
Review Comment:
**Suggestion:** `DatasetCreateFailedError` represents a server-side create failure (`CreateFailedError`, status 500), but this branch now logs it as a warning. That downgrades true system failures (for example DB/transaction failures) to user-level severity and can hide production incidents from error monitoring. Log this path at error level instead of warning. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β MCP dataset creation system failures logged as warnings not errors.
- β οΈ Production monitoring misses DatasetCreateFailedError incidents during outages.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Trigger the MCP `create_virtual_dataset` tool, which is implemented in
`superset/mcp_service/dataset/tool/create_virtual_dataset.py:43-131`. The tool calls
`CreateDatasetCommand(properties).run()` inside a `try` block at lines 71-86.
2. Cause `CreateDatasetCommand.run()` to raise `DatasetCreateFailedError`, as happens when
the transactional decorator wraps a database/SQLAlchemy failure. This is wired via
`@transaction(on_error=partial(on_error, reraise=DatasetCreateFailedError))` in
`superset/commands/dataset/create.py:42-47`, which delegates to
`superset/utils/decorators.py:210-23` to re-raise a `DatasetCreateFailedError` on SQL
errors.
3. In tests this behavior is reproduced directly in
`tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:44-52` where
`mock_command.run.side_effect = DatasetCreateFailedError()` and the tool is invoked via
`await client.call_tool("create_virtual_dataset", {"request": request.model_dump()})` at
lines 56-62, driving execution into the `except DatasetCreateFailedError as exc:` block in
`create_virtual_dataset.py:121-131`.
4. Observe that this `except DatasetCreateFailedError` block logs via `await
ctx.warning("Virtual dataset creation failed: %s" % (str(exc),))` at
`create_virtual_dataset.py:122` instead of `ctx.error`, even though
`DatasetCreateFailedError` inherits from `CreateFailedError` with `status = 500`
(`superset/commands/exceptions.py:49-51`) and is defined as such in
`superset/commands/dataset/exceptions.py:165-166`. In the HTTP API, the same exception is
logged at error level via `logger.error(..., exc_info=True)` in
`superset/datasets/api.py:30-37` and `646-652`, confirming it represents a server-side
failure. Therefore, system-level dataset creation failures in the MCP path are currently
downgraded to WARNING severity.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Ftool%2Fcreate_virtual_dataset.py%0A%2A%2ALine%3A%2A%2A%20122%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60DatasetCreateFailedError%60%20represents%20a%20server-side%20create%20failure%20%28%60CreateFailedError%60%2C%20status%20500%29%2C%20but%20this%20branch%20now%20logs%20it%20as%20a%20warning.%20That%20downgrades%20true%20system%20failures%20%28for%20example%20DB%2Ftransaction%20failures%29%20to%20user-level%20severity%20and%20can%20hide%20production%20incidents%20from%20error%20monitoring.%20Log%20this%20path%20at%20error%20level%20instead%20of%20warning.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20
is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Ftool%2Fcreate_virtual_dataset.py%0A%2A%2ALine%3A%2A%2A%20122%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60DatasetCreateFailedError%60%20represents%20a%20server-side%20create%20failure%20%28%60CreateFailedError%60%2C%20status%20500%29%2C%20but%20this%20branch%20now%20logs%20it%20as%20a%20warning.%20That%20downgrades%20true%20system%20failures%20%28for%20example%20DB%2Ftransaction%20failures%29%20to%20user-level%20severity%20and%20can%20hide%20production%
20incidents%20from%20error%20monitoring.%20Log%20this%20path%20at%20error%20level%20instead%20of%20warning.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/create_virtual_dataset.py
**Line:** 122:122
**Comment:**
*Logic Error: `DatasetCreateFailedError` represents a server-side create failure (`CreateFailedError`, status 500), but this branch now logs it as a warning. That downgrades true system failures (for example DB/transaction failures) to user-level severity and can hide production incidents from error monitoring. Log this path at error level instead of warning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=8d4df9906194e65f9e0405b2d0cf534332225bde4cb3b78c66f2efb4fc415e96&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=8d4df9906194e65f9e0405b2d0cf534332225bde4cb3b78c66f2efb4fc415e96&reaction=dislike'>π</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3150813716
##########
tests/unit_tests/mcp_service/test_middleware.py:
##########
@@ -441,3 +452,286 @@ async def test_string_response(self) -> None:
result = await middleware.on_call_tool(context, call_next)
assert result == response
+
+
+def _make_security_exception(msg: str = "access denied") -> SupersetSecurityException:
+ """Helper to construct SupersetSecurityException with a proper SupersetError."""
+ return SupersetSecurityException(
+ SupersetError(
+ message=msg,
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+class TestIsUserError:
+ """Test _is_user_error classification helper."""
+
+ @pytest.mark.parametrize(
+ "error,expected",
+ [
+ # User errors (WARNING) β expected in normal MCP operation
+ (ToolError("bad request"), True),
+ (PermissionError("access denied"), True),
+ (ObjectNotFoundError("Chart", "123"), True),
+ (ForbiddenError(), True),
+ (_make_security_exception(), True),
+ # System errors (ERROR) β unexpected failures
+ (ValueError("invalid param"), False),
+ (FileNotFoundError("not found"), False),
+ (RuntimeError("unexpected"), False),
+ (ConnectionError("connection refused"), False),
+ (TypeError("type mismatch"), False),
+ (KeyError("missing key"), False),
+ (Exception("generic"), False),
+ ],
+ ids=[
+ "ToolError",
+ "PermissionError",
+ "ObjectNotFoundError",
+ "ForbiddenError",
+ "SupersetSecurityException",
+ "ValueError",
+ "FileNotFoundError",
+ "RuntimeError",
+ "ConnectionError",
+ "TypeError",
+ "KeyError",
+ "Exception",
+ ],
+ )
+ def test_error_classification(self, error: Exception, expected: bool) -> None:
+ """Test that _is_user_error correctly classifies error types."""
+ assert _is_user_error(error) == expected
+
+ def test_validation_error(self) -> None:
+ """Test ValidationError is classified as user error."""
+ from pydantic import BaseModel
+
+ class TestModel(BaseModel):
+ name: str
Review Comment:
**Suggestion:** Add a short docstring to this newly introduced class so the new class definition complies with the docstring requirement for added Python classes. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
The added class `TestModel` has no class-level docstring. Since the suggestion is specifically about the repository's docstring requirement for newly introduced Python classes, this is a real violation in the current code.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Ftest_middleware.py%0A%2A%2ALine%3A%2A%2A%20512%3A513%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20short%20docstring%20to%20this%20newly%20introduced%20class%20so%20the%20new%20class%20definition%20complies%20with%20the%20docstring%20requirement%20for%20added%20Python%20classes.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20imp
lement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Ftest_middleware.py%0A%2A%2ALine%3A%2A%2A%20512%3A513%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20short%20docstring%20to%20this%20newly%20introduced%20class%20so%20the%20new%20class%20definition%20complies%20with%20the%20docstring%20requirement%20for%20added%20Python%20classes.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20commen
ts%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_middleware.py
**Line:** 512:513
**Comment:**
*Custom Rule: Add a short docstring to this newly introduced class so the new class definition complies with the docstring requirement for added Python classes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=394b8f35923b9849f1de4e1c6249f1381565ea9d8b89005cf54a869a9f5517a2&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=394b8f35923b9849f1de4e1c6249f1381565ea9d8b89005cf54a869a9f5517a2&reaction=dislike'>π</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4331639441
## Updated Review
Solid PR. The classification architecture is correct and the test coverage is good. Several issues were fixed in follow-up commits; two small nits remain.
---
## What works well
- **`_is_user_error()` design** is clean. Separating classification from handling is the right abstraction. The `SupersetException.status < 500` path elegantly handles the whole Superset exception hierarchy without enumerating every subclass.
- **Handler ordering** is correct. More-specific `SupersetException` subclasses (`ObjectNotFoundError`, `ForbiddenError`, etc.) are checked before the catch-all `SupersetException` branch. No subclass gets caught by the wrong branch.
- **`_FastMCPValidationFilter`** in `server.py` is the right way to handle third-party logging you can't control at the source.
- **Test depth** on the `SupersetException` status boundary (400/408/422/499/500/503) is thorough.
- **`_sanitize_params`** now applied to the token-blocked payload is a good catch.
- **`FileNotFoundError` as WARNING** is correct β it maps to 404, which is a user error.
---
## Fixed
**`raise ValueError` β `raise ToolError` throughout the tool/utility layer**
`chart_utils.py`, `sanitization.py`, `chart/schemas.py` (`parse_chart_config`), `server.py`, and `get_chart_sql.py` all had explicit user-error guards raising raw `ValueError`. These have been changed to `raise ToolError` directly, so `ValueError` in `_USER_ERROR_TYPES` is now a true safety net rather than load-bearing classification logic.
**Redundant `ConnectionError` subclasses**
`ConnectionRefusedError`, `ConnectionResetError`, and `BrokenPipeError` are all subclasses of `ConnectionError` in Python's stdlib. The `isinstance` tuple has been simplified to `isinstance(error, ConnectionError)`.
**Double-logging in `schema_utils.py`**
The pre-reraise `logger.warning("Failed to validate %s against %s", ...)` was removed. It added no context beyond what the middleware already captures.
---
## Remaining issues
**1. [nit] Log message embeds severity string redundantly β `middleware.py`**
```python
log_fn(
"MCP tool %s: tool=%s, ...",
"warning" if is_user else "error",
...
)
```
The log record's level is already set by `log_fn`. This produces `WARNING MCP tool warning: tool=...` which is redundant and creates two different message templates for the same event, making log search awkward. A single message format like `"MCP tool call failed:"` lets the log level carry the severity.
**2. [nit] `getattr(error, "status", 500)` fallback is dead code β `middleware.py`**
`SupersetException` defines `status = 500` as a class attribute, so `error.status` always resolves. The `500` default in `getattr` can never be reached.
---
## Question for the author
**`mcp_core.py._log_error`**: This logs at WARNING before re-raising, so every error through the three catch-and-reraise sites (lines 375, 549, 676) produces two WARNING lines β one here, one in the middleware. Does the per-site `Error in ClassName: ...` context add enough value to justify the double log, or should these be removed in favour of the middleware as the single source?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4317612627
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3141235287
##########
tests/unit_tests/mcp_service/test_middleware.py:
##########
@@ -441,3 +452,286 @@ async def test_string_response(self) -> None:
result = await middleware.on_call_tool(context, call_next)
assert result == response
+
+
+def _make_security_exception(msg: str = "access denied") -> SupersetSecurityException:
+ """Helper to construct SupersetSecurityException with a proper SupersetError."""
+ return SupersetSecurityException(
+ SupersetError(
+ message=msg,
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+class TestIsUserError:
+ """Test _is_user_error classification helper."""
+
+ @pytest.mark.parametrize(
+ "error,expected",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Parametrize decorator wrong type</b></div>
<div id="fix">
Change `pytest.mark.parametrize` first argument from string to tuple: `("error,expected",` instead of `"error,expected",`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
("error", "expected"),
````
</div>
</details>
</div>
<small><i>Code Review Run #934dac</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4331412343
Thanks for the thorough review @eschutho! Addressed all points in the latest commits:
**1. Double-logging** β Changed `mcp_core._log_error` and `schema_utils` pre-reraise logs from WARNING to DEBUG. The middleware is now the single source of truth for error classification.
**2. `ValueError` removed from `_USER_ERROR_TYPES`** β Agreed it's too broad. Removed. Tool-layer parameter validation is already covered by Pydantic `ValidationError`. Any tool-specific `ValueError` that's truly user-caused should be wrapped in `ToolError` at the raise site.
**3. `FileNotFoundError` removed** β No user-controlled path generates this in MCP tools. Removed from `_USER_ERROR_TYPES` so it correctly falls through to ERROR.
**4. Simplified `ConnectionError` handler** β Removed redundant subclasses. Good catch that `requests.exceptions.ConnectionError` inherits from `OSError` not Python's `ConnectionError` β those will fall through to the generic else branch, which is the right behavior (they should be ERROR).
**5. Removed redundant severity string** β Log message is now `"MCP tool call failed: ..."`, letting the log level carry the severity.
**6. Removed dead `getattr`** β Using `error.status` directly since `SupersetException` always defines it as a class attribute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4331352578
<!-- Bito Reply -->
The flagged issue is correctβthe newly introduced `TestModel` class lacks a class-level docstring, violating the repository's docstring requirement for added Python classes. To resolve this, add a short docstring right after the class definition.
**tests/unit_tests/mcp_service/test_middleware.py**
```
class TestModel(BaseModel):
"""Test model for validation error testing."""
name: str
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4331599392
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4332181641
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3150822881
##########
superset/mcp_service/middleware.py:
##########
@@ -95,6 +116,39 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+# Errors caused by the LLM/user β expected in normal MCP operation.
+# Agents send bad params, try tools they lack access to, request nonexistent
+# resources. These are 400-class errors and should be logged at WARNING.
+_USER_ERROR_TYPES = (
+ ToolError,
+ ValidationError,
+ PermissionError,
+ CommandInvalidError,
+ ObjectNotFoundError,
+ ForbiddenError,
+ SupersetSecurityException,
Review Comment:
**Suggestion:** `ValueError` and `FileNotFoundError` are not included in the user-error classification tuple, so these user-caused failures are still logged as system errors. This breaks the intended severity split and will keep noisy parameter/file mistakes at ERROR level; include these exception types in `_USER_ERROR_TYPES` (or handle them explicitly in `_is_user_error`) so they are treated as WARNING. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ ValueError-based parameter mistakes logged as system ERRORs.
- β οΈ FileNotFoundError tool failures inflate MCP error metrics.
- β οΈ Noise in monitoring obscures genuine system-level MCP issues.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Start Superset with the MCP service enabled so that `GlobalErrorHandlerMiddleware` in
`superset/mcp_service/middleware.py` is active for tool calls.
2. Trigger an MCP tool call via the LLM that raises a plain `ValueError` or
`FileNotFoundError` inside the tool implementation; this propagates to `_handle_error()`
at lines ~411β517 in `superset/mcp_service/middleware.py` as a bare `Exception` (no
wrapping into `ToolError`, `CommandInvalidError`, etc.).
3. Inside `_handle_error()`, the error is classified by `_is_user_error()` at line 429,
which only checks `_USER_ERROR_TYPES = (ToolError, ValidationError, PermissionError,
CommandInvalidError, ObjectNotFoundError, ForbiddenError, SupersetSecurityException)` and
`SupersetException.status`; `ValueError` and `FileNotFoundError` are not in this tuple and
are not `SupersetException`, so `_is_user_error()` returns `False`.
4. Because `_is_user_error()` returns `False`, `_handle_error()` selects `logger.error` as
`log_fn` and logs the failure at ERROR level and sets `severity` in the
`event_logger.log(... curated_payload=...)` block (lines ~441β453) to `"error"`, even
though the PR description states these should be treated as user-caused WARNING-level
issues.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmiddleware.py%0A%2A%2ALine%3A%2A%2A%20122%3A129%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60ValueError%60%20and%20%60FileNotFoundError%60%20are%20not%20included%20in%20the%20user-error%20classification%20tuple%2C%20so%20these%20user-caused%20failures%20are%20still%20logged%20as%20system%20errors.%20This%20breaks%20the%20intended%20severity%20split%20and%20will%20keep%20noisy%20parameter%2Ffile%20mistakes%20at%20ERROR%20level%3B%20include%20these%20exception%20types%20in%20%60_USER_ERROR_TYPES%60%20%28or%20handle%20them%20explicitly%20in%20%60_is_user_error%60%29%20so%20they%20are%20treated%20as%20WARNING.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20mak
e%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmiddleware.py%0A%2A%2ALine%3A%2A%2A%20122%3A129%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60ValueError%60%20and%20%60FileNotFoundError%60%20are%20not%20included%20in%20the%20user-error%20classification%20tuple%2C%20so%20these%20user-caused%20failures%20are%20still%20logged%20as%20system%20errors.%20This%20breaks%20the%20intended%20severity%20split%20and%20will%20keep%20noisy%20parameter%2Ffile%20mistakes%20at%20ERROR%20level%3B%20inclu
de%20these%20exception%20types%20in%20%60_USER_ERROR_TYPES%60%20%28or%20handle%20them%20explicitly%20in%20%60_is_user_error%60%29%20so%20they%20are%20treated%20as%20WARNING.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 122:129
**Comment:**
*Logic Error: `ValueError` and `FileNotFoundError` are not included in the user-error classification tuple, so these user-caused failures are still logged as system errors. This breaks the intended severity split and will keep noisy parameter/file mistakes at ERROR level; include these exception types in `_USER_ERROR_TYPES` (or handle them explicitly in `_is_user_error`) so they are treated as WARNING.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=5ee23fc33af4e5dd72e2cbcbf93c252e0b5b0c5d266d720bd85ed747a1c578af&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=5ee23fc33af4e5dd72e2cbcbf93c252e0b5b0c5d266d720bd85ed747a1c578af&reaction=dislike'>π</a>
##########
superset/mcp_service/server.py:
##########
@@ -74,6 +74,45 @@ def _suppress_third_party_warnings() -> None:
)
+class _FastMCPValidationFilter(logging.Filter):
+ """Downgrade FastMCP's user-error logs from ERROR to WARNING.
+
+ FastMCP's server.py logs ValidationError and ToolError at ERROR level
+ via logger.exception() before our GlobalErrorHandlerMiddleware sees it.
+ These are user errors (LLM sent bad params, access denied, not found)
+ and are expected in normal MCP operation β they should not pollute
+ ERROR-level logs in Datadog.
+
+ Patterns downgraded:
+ - "Error validating tool '...'" β Pydantic ValidationError (bad params)
+ - "Error calling tool '...'" when exc_info is a ToolError/FastMCPError
+ (our middleware converted the user error to ToolError before FastMCP
+ catches and re-logs it)
+ """
+
+ def filter(self, record: logging.LogRecord) -> bool:
+ if record.levelno != logging.ERROR:
+ return True
+ msg = record.getMessage()
+ if "Error validating tool" in msg:
+ record.levelno = logging.WARNING
+ record.levelname = "WARNING"
+ elif "Error calling tool" in msg and record.exc_info:
+ # Only downgrade if the exception is ToolError/FastMCPError
+ # (user errors), not generic Exception (system errors)
+ exc_type = record.exc_info[0] if record.exc_info else None
+ if exc_type is not None:
+ try:
+ from fastmcp.exceptions import FastMCPError
+
+ if issubclass(exc_type, FastMCPError):
+ record.levelno = logging.WARNING
+ record.levelname = "WARNING"
+ except ImportError:
+ pass
Review Comment:
**Suggestion:** This filter downgrades every `"Error calling tool"` log whose exception is a `FastMCPError`, but system failures are also wrapped into `ToolError`/`FastMCPError` by middleware. As a result, genuine infrastructure/internal failures can be incorrectly demoted to WARNING in FastMCP logs; restrict downgrade to explicitly user-caused error types/messages instead of all `FastMCPError` subclasses. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ System-level MCP tool failures downgraded to WARNING in FastMCP logs.
- β οΈ Datadog monitors on fastmcp.server may miss critical outages.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Start the MCP HTTP server by calling `run_server()` in
`superset/mcp_service/server.py:55-83`, which invokes `configure_logging(debug)` at
`superset/mcp_service/server.py:78`. Inside `configure_logging()`,
`_FastMCPValidationFilter` is attached to the `"fastmcp.server.server"` logger at
`superset/mcp_service/server.py:144-149`.
2. Trigger a system-level failure inside a tool, for example by causing a database outage
so that a tool invocation like `execute_sql` raises `sqlalchemy.exc.OperationalError`.
This exception is caught by `GlobalErrorHandlerMiddleware._handle_error()` in
`superset/mcp_service/middleware.py:390-88`, where the `(OperationalError, TimeoutError)`
branch at `superset/mcp_service/middleware.py:472-488` wraps it in a `ToolError` with a
"Database error in <tool>β¦" message and re-raises that `ToolError`.
3. The raised `ToolError` (which is imported from `fastmcp.exceptions` at
`superset/mcp_service/middleware.py:24` and is a subclass of `FastMCPError`, imported in
`superset/mcp_service/server.py:37`) propagates up to FastMCP's server layer. As
documented in the `_FastMCPValidationFilter` docstring at
`superset/mcp_service/server.py:8-21`, FastMCP's `server.py` logs failed tool calls with
messages like `"Error calling tool '...'"` using `logger.exception`, so the resulting log
record on logger `"fastmcp.server.server"` has level ERROR, message containing `"Error
calling tool"`, and `exc_info` whose first element is the `ToolError` type (a
`FastMCPError` subclass).
4. That log record passes through `_FastMCPValidationFilter.filter()` at
`superset/mcp_service/server.py:24-40`. The code at
`superset/mcp_service/server.py:100-112` checks `elif "Error calling tool" in msg and
record.exc_info:` and then `issubclass(exc_type, FastMCPError)`, which is True for
`ToolError`, so it unconditionally sets `record.levelno` and `record.levelname` to
WARNING. As a result, this genuine system/infrastructure failure (originating from
`OperationalError`) is downgraded from ERROR to WARNING in the `fastmcp.server.server`
logs, contrary to the intent of treating system errors as ERROR.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fserver.py%0A%2A%2ALine%3A%2A%2A%20100%3A112%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20filter%20downgrades%20every%20%60%22Error%20calling%20tool%22%60%20log%20whose%20exception%20is%20a%20%60FastMCPError%60%2C%20but%20system%20failures%20are%20also%20wrapped%20into%20%60ToolError%60%2F%60FastMCPError%60%20by%20middleware.%20As%20a%20result%2C%20genuine%20infrastructure%2Finternal%20failures%20can%20be%20incorrectly%20demoted%20to%20WARNING%20in%20FastMCP%20logs%3B%20restrict%20downgrade%20to%20explicitly%20user-caused%20error%20types%2Fmessages%20instead%20of%20all%20%60FastMCPError%60%20subclasses.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20mak
e%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fserver.py%0A%2A%2ALine%3A%2A%2A%20100%3A112%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20filter%20downgrades%20every%20%60%22Error%20calling%20tool%22%60%20log%20whose%20exception%20is%20a%20%60FastMCPError%60%2C%20but%20system%20failures%20are%20also%20wrapped%20into%20%60ToolError%60%2F%60FastMCPError%60%20by%20middleware.%20As%20a%20result%2C%20genuine%20infrastructure%2Finternal%20failures%20can%20be%20incorrectly%20demoted%20to
%20WARNING%20in%20FastMCP%20logs%3B%20restrict%20downgrade%20to%20explicitly%20user-caused%20error%20types%2Fmessages%20instead%20of%20all%20%60FastMCPError%60%20subclasses.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 100:112
**Comment:**
*Logic Error: This filter downgrades every `"Error calling tool"` log whose exception is a `FastMCPError`, but system failures are also wrapped into `ToolError`/`FastMCPError` by middleware. As a result, genuine infrastructure/internal failures can be incorrectly demoted to WARNING in FastMCP logs; restrict downgrade to explicitly user-caused error types/messages instead of all `FastMCPError` subclasses.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=883c8c4da087b8e10e4cdcdad6861faf5b4ef23da3645332539c478a76be8c97&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39634&comment_hash=883c8c4da087b8e10e4cdcdad6861faf5b4ef23da3645332539c478a76be8c97&reaction=dislike'>π</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #39634:
URL: https://github.com/apache/superset/pull/39634
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4315589171
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39634:
URL: https://github.com/apache/superset/pull/39634#issuecomment-4315614924
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | de4ff6c8376a6d4edf78482b52315024c579e05a |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69ebbd942cccd20008219ea7 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-39634--superset-docs-preview.netlify.app](https://deploy-preview-39634--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39634--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3150825074
##########
superset/mcp_service/middleware.py:
##########
@@ -95,6 +116,39 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+# Errors caused by the LLM/user β expected in normal MCP operation.
+# Agents send bad params, try tools they lack access to, request nonexistent
+# resources. These are 400-class errors and should be logged at WARNING.
+_USER_ERROR_TYPES = (
+ ToolError,
+ ValidationError,
+ PermissionError,
+ CommandInvalidError,
+ ObjectNotFoundError,
+ ForbiddenError,
+ SupersetSecurityException,
+)
+
+
+def _is_user_error(error: Exception) -> bool:
+ """Classify whether an error is user-caused (WARNING) or system-caused (ERROR).
+
+ User errors are expected in normal MCP operation β agents send bad params,
+ try tools they lack access to, request nonexistent resources. These are
+ 400-class errors and should be logged at WARNING.
+
+ System errors are unexpected β database down, unexpected exceptions,
+ infrastructure failures. These are 500-class and should be logged at ERROR.
+ """
+ if isinstance(error, _USER_ERROR_TYPES):
+ return True
+ # SupersetException and CommandException have a .status attribute.
+ # 4xx = user error, 5xx = system error.
+ if isinstance(error, SupersetException):
+ return error.status < 500
+ return False
Review Comment:
**π Architect Review β HIGH**
The user-error classifier in GlobalErrorHandlerMiddleware does not treat ValueError as a user error, so ValueError raised from MCP tools (for example the unsupported model_type ValueError in get_schema) is logged at ERROR severity and recorded with severity="error", even though the accompanying tests and docstrings expect ValueError-driven bad input to be classified as a user-caused warning.
**Suggestion:** Update _is_user_error (and its _USER_ERROR_TYPES tuple) so that ValueError is classified as a user error for logging and event_logger severity, and align the implementation with the existing GlobalErrorHandlerMiddleware tests that expect ValueError-based tool failures to log at WARNING with severity="warning".
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmiddleware.py%0A%2A%2ALine%3A%2A%2A%20122%3A149%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20The%20user-error%20classifier%20in%20GlobalErrorHandlerMiddleware%20does%20not%20treat%20ValueError%20as%20a%20user%20error%2C%20so%20ValueError%20raised%20from%20MCP%20tools%20%28for%20example%20the%20unsupported%20model_type%20ValueError%20in%20get_schema%29%20is%20logged%20at%20ERROR%20severity%20and%
20recorded%20with%20severity%3D%22error%22%2C%20even%20though%20the%20accompanying%20tests%20and%20docstrings%20expect%20ValueError-driven%20bad%20input%20to%20be%20classified%20as%20a%20user-caused%20warning.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20MUST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%20change%20as%20small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surroundin
g%20code%20or%20expand%20scope%20beyond%20the%20finding.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_serv
ice%2Fmiddleware.py%0A%2A%2ALine%3A%2A%2A%20122%3A149%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20The%20user-error%20classifier%20in%20GlobalErrorHandlerMiddleware%20does%20not%20treat%20ValueError%20as%20a%20user%20error%2C%20so%20ValueError%20raised%20from%20MCP%20tools%20%28for%20example%20the%20unsupported%20model_type%20ValueError%20in%20get_schema%29%20is%20logged%20at%20ERROR%20severity%20and%20recorded%20with%20severity%3D%22error%22%2C%20even%20though%20the%20accompanying%20tests%20and%20docstrings%20expect%20ValueError-driven%20bad%20input%20to%20be%20classified%20as%20a%20user-caused%20warning.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20M
UST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%20change%20as%20small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surrounding%20code%20or%20expand%20scope%20beyond%20the%20finding.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings β not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/middleware.py
**Line:** 122:149
**Comment:**
*HIGH: The user-error classifier in GlobalErrorHandlerMiddleware does not treat ValueError as a user error, so ValueError raised from MCP tools (for example the unsupported model_type ValueError in get_schema) is logged at ERROR severity and recorded with severity="error", even though the accompanying tests and docstrings expect ValueError-driven bad input to be classified as a user-caused warning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix β do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): classify user errors as WARNING, system errors as ERROR [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39634:
URL: https://github.com/apache/superset/pull/39634#discussion_r3151061100
##########
superset/mcp_service/mcp_core.py:
##########
@@ -68,7 +68,7 @@ def _log_error(self, error: Exception, context: str = "") -> None:
if context:
error_msg += f" ({context})"
error_msg += f": {str(error)}"
- self.logger.error(error_msg, exc_info=True)
+ self.logger.debug(error_msg, exc_info=True)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect error log level</b></div>
<div id="fix">
This change lowers error logging from warning to debug level, which would make exceptions invisible in production logs where debug is typically disabled. The _log_error method is used in core tool classes to log actual exceptions before re-raising them, so warning level is appropriate for reliability and debugging.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
self.logger.warning(error_msg, exc_info=True)
````
</div>
</details>
</div>
<small><i>Code Review Run #172127</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/middleware.py:
##########
@@ -426,18 +484,29 @@ async def _handle_error( # noqa: C901
f"Permission denied for {tool_name}: "
f"You don't have access to this resource."
) from error
- elif isinstance(error, FileNotFoundError):
- # File/resource not found errors
+ elif isinstance(error, (ObjectNotFoundError, CommandInvalidError)):
+ # Superset command: not found (404) or validation (422)
+ raise ToolError(f"Invalid request for {tool_name}: {str(error)}") from error
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Error Handling Regression</b></div>
<div id="fix">
Removing ValueError handling causes user input validation errors (e.g., empty SQL in dataset schemas) to be reported as 'Internal error' instead of 'Invalid parameter', misleading users and escalating log levels incorrectly. Since MCP tools raise ValueError for invalid parameters, this should be handled as a 4xx user error.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
raise ToolError(f"Invalid request for {tool_name}: {str(error)}") from error
elif isinstance(error, ValueError):
# Value/parameter errors
raise ToolError(
f"Invalid parameter in {tool_name}: {str(error)}"
) from error
````
</div>
</details>
</div>
<small><i>Code Review Run #172127</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org