codeant-ai-for-open-source[bot] commented on code in PR #36553:
URL: https://github.com/apache/superset/pull/36553#discussion_r2612988716
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -85,11 +89,18 @@ def decorator(func: F) -> F:
name=tool_name,
description=tool_description,
tags=tool_tags,
+ task=task,
)
mcp.add_tool(tool)
+ task_status = "background-task" if task else "sync"
Review Comment:
**Suggestion:** Accuracy bug in logging: the new log message uses `task` to
compute `task_status`, but if a compatibility fallback changed whether the tool
was actually registered as a background task, the log will be wrong; compute
`task_status` from the actual effective task registration (fallback to `task`
if no effective value is present). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Use the actual effective task state if available (set by
registration fallback), otherwise fall back to requested `task`.
_is_background = locals().get("effective_task", task)
task_status = "background-task" if _is_background else "sync"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If you implement the compatibility fallback that can change whether the tool
was actually registered as a task, the log should reflect the effective state,
not the requested `task` flag. The proposed lookup is fine, but prefer an
explicit `effective_task` variable set during registration instead of relying
on locals().
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/core/mcp/core_mcp_injection.py
**Line:** 96:96
**Comment:**
*Possible Bug: Accuracy bug in logging: the new log message uses `task`
to compute `task_status`, but if a compatibility fallback changed whether the
tool was actually registered as a background task, the log will be wrong;
compute `task_status` from the actual effective task registration (fallback to
`task` if no effective value is present).
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.
```
</details>
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -233,6 +247,7 @@ def generate_dashboard(
)
except Exception as e:
+ await ctx.error("Error creating dashboard: %s" % (str(e),))
logger.error("Error creating dashboard: %s", e)
Review Comment:
**Suggestion:** In the exception handler the code `await ctx.error(...)` is
called before local logging; if `ctx.error` itself raises, the local
`logger.error(...)` and the function's return will never run, masking the
original failure — log locally first, then attempt to send the context error
notification. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Ensure local logging always runs even if ctx.error fails
logger.error("Error creating dashboard: %s", e)
try:
await ctx.error("Error creating dashboard: %s" % (str(e),))
except Exception:
# Swallow context-reporting errors to avoid masking original
exception
logger.exception("Failed to report error to context")
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Logging locally before attempting to report to the external context is
safer: if ctx.error raises, you'll still have the local log of the original
failure. Wrapping the ctx.error call in a try/except and swallowing/reporting
any failure to report keeps the original error visible and avoids masking. This
is a defensive, practical change and fixes a real reliability concern in error
handling.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/generate_dashboard.py
**Line:** 250:251
**Comment:**
*Possible Bug: In the exception handler the code `await ctx.error(...)`
is called before local logging; if `ctx.error` itself raises, the local
`logger.error(...)` and the function's return will never run, masking the
original failure — log locally first, then attempt to send the context error
notification.
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.
```
</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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
