fix: query_project rejected read-only tools excluded by the current context#1527
fix: query_project rejected read-only tools excluded by the current context#1527bor wants to merge 1 commit into
Conversation
…ontext The forwarding gate in QueryProjectTool.apply asserted tool.is_active(), which reflects the caller's active toolset (context + global config + modes + active project). A tool excluded merely by the current context -- e.g. search_for_pattern in the claude-code context, where the client has a native grep -- was therefore blocked, even though it is read-only and valid against the target project. Replace the is_active() gate with a check against the global Serena configuration only, keeping the read-only requirement. Context-level (client-cosmetic) exclusions no longer block forwarding; globally disabled tools still do. The shared ToolInclusionDefinition.disables_tool helper unifies fixed and incremental mode handling. Policy and safety gates now raise explicit ValueErrors rather than asserts so they cannot be stripped via python -O.
opcode81
left a comment
opcode81
left a comment
There was a problem hiding this comment.
Thanks for your PR, but I will need to reject this.
- As indicated in my comment, the new logic that is introduced does not make sense.
- There is no point in replacing AssertionErrors with ValueErrors.
- We do not need tests for this.
- The simplest approach is simply to remove the assertion that the tool must be active (noting that there are limitations regarding LLM awareness). You are welcome to propose a new PR with this one-line change plus changelog entry.
| # explicit ValueError (not assert): safety/policy gates must not be strippable via `python -O`. | ||
| if not tool.is_readonly(): | ||
| raise ValueError(f"Tool {tool_name} is not read-only and cannot be executed in another project.") | ||
| # the previous `is_active()` check conflated client-cosmetic context exclusions | ||
| # (e.g. search_for_pattern in the claude-code context, where the client has a native grep) | ||
| # with intentional global disabling. Forwarding a read-only tool to another project is | ||
| # valid even if the current context hides it; only the global config acts as kill-switch. | ||
| if self.agent.serena_config.disables_tool(tool_name): | ||
| raise ValueError(f"Tool {tool_name} is disabled in the global Serena configuration and cannot be executed.") |
There was a problem hiding this comment.
The introduction of the new method does not make sense: Whether or not SerenaConfig disables a tool is meaningless.
It can make sense to allow tools that are not currently enabled, but there is an important limitation: The LLM will be aware of their existence (and know how to use them) only if they are exposed by the MCP server.
So removing the is_active condition will have a practical effect only if the tool was previously exposed but not enabled.
In all other cases, you would need to manually inform the LLM about these tools.
There is no need to add any additional condition if we just want to soften the condition that needs to apply.
|
Thanks for the thorough review and the clear guidance, @opcode81 - much appreciated. Closing this PR; I'll open a fresh one with just the minimal change (drop the |
2 participants
Summary
QueryProjectTool.applygated forwarding ontool.is_active(), which reflects the caller's active toolset (context + global config + modes + active project). A read-only tool excluded merely by the current context — e.g.read_file,find_file,list_dir, orsearch_for_patternin CLI/IDE contexts such asclaude-code,codex,copilot-cli,vscode,junie,antigravity,jb-*,ide— was therefore blocked from being forwarded to another registered project, even though the tool is read-only and perfectly runnable against the target. The error users saw wasTool <name> is not active.Root cause
Tool.is_active()->agent.tool_is_active(name)-> membership inagent._active_tools, which is the base toolset reduced by context + global config + modes + project. Context-level exclusions (client-cosmetic, e.g. Claude Code has a native grep sosearch_for_patternis excluded for UX) were therefore conflated with "tool may not run at all." Forwarding is a different question — context-level exclusions should not block it.Corroboration: the existing server-side executor
ProjectServer._query_projectalready gates on onlyis_readonly(), so read-only is the intended gate; the over-strict check existed only on the caller side.Fix
assert tool.is_active()inQueryProjectTool.applywith a check against the global Serena configuration only, via a newToolInclusionDefinition.disables_tool(name)helper that unifiesexcluded_tools(incremental mode) andfixed_tools(fixed mode) handling.is_readonly()requirement.apply()(read-only, global-config, registration) now raise explicitValueErrors rather thanasserts, so they cannot be stripped bypython -O.After the fix:
Scope
Intentionally minimal — fixes only the reported behavior.
ProjectServer._query_projectis unchanged; it already gates onis_readonly(), which remains correct.Test plan
test/serena/test_query_project_tools.py— 4 in-process integration tests (no language server required;search_for_patternis non-symbolic + read-only, so_is_project_server_requiredreturnsFalse):mainwithAssertionError: Tool search_for_pattern is not active.)create_text_fileraisesValueErrormatching"read-only"serena_config.excluded_toolsblocks forwarding withValueErrormatching"global"project_nameraisesValueErrormatching"not registered"ValueError(notAssertionError) so a regression back toassertwould fail loudly.test/serena/config/test_serena_config.py::TestToolInclusionDefinitionDisablesTool— 3 unit tests covering default / incremental / fixed modes of the new helper.poe formatclean.poe type-checkclean.# Unreleased (main)/* Tools:.