aminghadersohi commented on PR #39835: URL: https://github.com/apache/superset/pull/39835#issuecomment-4453276404
Thanks for the thorough analysis — the `get_instance_info` snippet approach is clean and the table of prose refs per tool is exactly the right way to scope the remaining work. **Option B — go for it.** A general line-level filter (`any(tool in line for tool in _disabled)`) is the right trade-off: Option A would fragment the instructions into ~60 constants across ~15 tools, which hurts readability. Option B closes the gap in ~5 lines. One thing worth adding alongside the implementation: a test for at least one prose-heavy tool (e.g., `execute_sql`) mirroring `test_disabling_get_instance_info_removes_all_prose_references` — just assert that the SQL workflow examples or the `execute_sql` request wrapper example are absent when `execute_sql` is disabled. **`remove_tool()` — keep it.** Your analysis is right. `disable()` hides from listing but may still allow direct invocation; `remove_tool()` removes the tool entirely. For an operator escape hatch designed to replace built-in tools with stricter versions, the hard-removal semantics are the correct choice. If FastMCP later documents that `disable()` enforces on execution too, switching can be a follow-up. The `local_provider` fix, the `set(MCP_DISABLED_TOOLS)` copy, and the dead-patch cleanup all look good — those are done. -- 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]
