SkinnyPigeon commented on PR #39835:
URL: https://github.com/apache/superset/pull/39835#issuecomment-4452762560
Hey @richardfogaca @aminghadersohi. Thanks both for taking a look. About the
second set of comments:
### Point 1 — Instruction/tool-registry drift
`get_instance_info` is now fully handled: all five prose references to it
(the "Feature Availability" block, the `get_instance_info returns
current_user.roles` Permission Awareness bullet, the `accessible_menus from
get_instance_info` bullet, the "start with get_instance_info" paragraph, and
the "When you first connect, call get_instance_info" paragraph) are omitted
when that tool is disabled — not just the bullet entry.
For every other tool the current implementation only removes the `-
tool_name: description` bullet line. I ran an analysis of the full instructions
string to map out all prose references beyond the bullet list:
| Tool | Prose refs | What's affected |
|------|-----------|-----------------|
| `execute_sql` | 6 | "To explore data with SQL" workflow steps + CRITICAL
RULES reference |
| `generate_chart` | 8 | "To create a chart" workflow,
`generate_explore_link vs generate_chart` section |
| `generate_explore_link` | 7 | Same as above |
| `add_chart_to_existing_dashboard` | 4 | "To add a chart to an existing
dashboard" + CRITICAL RULES |
| `generate_dashboard` | 4 | CRITICAL RULES + "To add a chart" workflow |
| `get_dataset_info` | 5 | "Using Saved Metrics vs Columns", workflow
examples |
| `list_charts` / `list_dashboards` / `list_datasets` / `list_databases` |
2–9 | Workflow examples, query examples |
| `get_chart_info` | 3 | Request Wrapper example, "To modify an existing
chart" workflow |
| `update_chart` | 5 | "To modify an existing chart" workflow, CRITICAL
RULES |
| `get_schema` | 2 | General tips section |
| `create_virtual_dataset`, `query_dataset`, `save_sql_query`,
`open_sql_lab_with_context` | 1–2 | Workflow steps |
Two options to close this gap:
**Option A — More `_SNIPPET_*` constants** (what was done for
`get_instance_info`): gate entire workflow sections behind a check on their key
tool. More explicit, but each tool's prose is scattered across multiple
sections (e.g. `execute_sql` appears in the workflow section _and_ in CRITICAL
RULES), making this hard to do cleanly without fragmenting the instructions
string further.
**Option B — General line filter** (extension of the existing bullet
filter): after building the f-string, remove any line that contains a disabled
tool name anywhere on the line. The existing filtering loop already iterates
every line; it just needs a second pass that checks `any(tool in line for tool
in _disabled)`. This handles all of the above in ~5 extra lines and is robust
to future additions to the instructions string. False positives are extremely
low given these names (`execute_sql`, `generate_chart`, etc.) are specific.
My preference is **Option B** — it closes the gap comprehensively and keeps
the instructions string readable. Happy to implement it if you agree.
---
### Point 2 — `local_provider.remove_tool` vs `mcp.disable()`
FastMCP 3.2.4 does expose a public visibility API:
```python
mcp.disable(names={"tool_name"}, components={"tool"})
```
This adds a visibility transform that marks the tool as disabled in the tool
listing. It is the cleaner, officially public API.
However, there is a meaningful behavioural difference worth considering:
- **`mcp.disable()`** — hides the tool from `tools/list` responses so
clients won't discover it during tool enumeration, but a client that already
knows the tool name _may_ still be able to call it directly (depending on
whether FastMCP enforces visibility on execution as well as listing).
- **`mcp.local_provider.remove_tool()`** — removes the tool entirely from
the provider registry; it cannot be called at all.
For an operator escape hatch intended to prevent misuse (e.g. replacing
`execute_sql` with a stricter extension), the `remove_tool` behaviour is
stronger. But if FastMCP enforces visibility on both listing _and_ execution
then `mcp.disable()` is equivalent and preferable as the public API.
I can switch to `mcp.disable()` — would you like me to verify whether
FastMCP enforces visibility on call execution as well as listing, and switch if
it does? Or would you prefer `remove_tool` be kept for the stronger guarantee?
--
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]
