fix(tools): activate --allowedTools/--disallowedTools (were a silent no-op)#739
Merged
Conversation
…no-op) Both the headless (--print) and agent-server tool-filter helpers backed --allowedTools / --disallowedTools by calling a non-existent registry.unregister() inside a bare try/except. The AttributeError was swallowed, so the filter never removed anything: the flags were parsed and plumbed all the way to _filter_registry, but the model still saw the full tool pool (schemas are emitted from registry.list_tools()). Point both call sites at the real ToolRegistry.remove_tool(), which already exists and is used for live MCP tool swaps. Now the flags actually narrow the pool the model sees — matching Claude Code's harbor adapter, whose equivalent flags work. Both paths only ever REMOVE tools, so activating them can only narrow the toolset (no new access). - src/entrypoints/headless.py, src/server/agent_server.py: unregister -> remove_tool - src/tool_system/registry.py: refresh the stale "never actually run" note - tests/test_headless_tool_filter.py: lock in denylist/allowlist filtering, registry has remove_tool (not unregister), idempotence, agent-server parity Surfaced while comparing clawcodex's eval tool set (46 tools) against Claude Code's (~15): the flag needed to trim it was dead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-safe Follow-up to activating the filter. Two edges surfaced in review: 1. Alias input did nothing. list_tools() yields only primary names, so --disallowedTools KillShell (alias of TaskStop) matched no primary name and left TaskStop both in the schema AND dispatch-reachable via the alias. Add ToolRegistry.canonicalize_names(), which resolves each requested name (primary or alias) to its tool's primary name via get(); both call sites canonicalize the allow/deny sets up front (before either filter runs, so aliases still resolve). remove_tool() then drops the tool and its aliases, closing both the schema and dispatch gaps. 2. A stray "" in the request set would become a match-nothing allowlist that removes every tool. The CLI's _split_csv already drops blanks, but canonicalize_names now also skips them so the helper is safe for any caller. Tests: alias denylist removes from schema + get(); alias allowlist keeps the real tool; blank input can't wipe the pool. 61 related tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omment Address critic follow-ups on the tool-filter activation: - remove_tool() now resolves by primary name OR alias and drops the tool from _tools plus EVERY _by_name key pointing at it. Removing by an alias used to leave the canonical key behind, so the tool stayed dispatch-reachable via a sibling key. Not triggered by any current caller (the filter passes canonical names, MCP passes full names), but remove_tool now backs two subsystems — this closes the latent execution leak. Test added. - Fix the flag spelling in the docstrings/comments I added: clawcodex registers kebab-case --allowed-tools/--disallowed-tools (cli.py:495/501), not the camelCase Claude Code form. - mcp_runtime.py: refresh the sibling stale "registry.unregister" comment. Note (unchanged, documented): these flags do name-based tool filtering only — they do NOT parse Claude Code's scoped permission rules (e.g. Bash(rm:*)) and add no deny-rule. The agent-server call sites are correct but currently inert (nothing populates ServerConfig.allowed_tools yet); the reachable path today is headless -p. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ericleepi314
added a commit
that referenced
this pull request
Jul 26, 2026
…d a headless-only tool (#747) Four defects found by diffing two full terminal-bench 2.1 runs at identical model/effort/dataset (claude-opus-5, xhigh): clawcodex 0.742 vs the official Claude Code harness 0.843. Of the 13 tasks Claude Code solved and clawcodex did not, 3 were crashes and 3 were trials that did nothing at all — none of it model capability. All four fixes are general robustness. Nothing here is conditional on an eval: no env checks, no branches, no wording aimed at a grader. ## An empty assistant turn ended the run as "completed" Three trials — break-filter-js-from-html, crack-7z-hash, vulnerable-secret — produced ONE output token in ~3 seconds, returned an empty result, reported `subtype: success`, and scored 0. Claude Code solved all three. No text and no tool calls is a degenerate response, not an answer. The continuation nudge could not catch it because it gates on the text being truthy, so an empty turn fell straight through to `Terminal(reason="completed")`. Empty turns are now re-prompted, bounded by the same MAX_CONTINUATION_NUDGES cap as every other nudge. Carve-out: a model that speaks through SendUserMessage (documented as the primary visible output channel) legitimately ends with empty assistant text, and agent_loop_compat already treats the outbox as the response text. The existing parity test caught that case. ## A transport blip killed a 24-minute trial regex-chess died on `peer closed connection without sending complete message body (incomplete chunked read)` seconds after a passing 1500-position fuzz run. Transport drops now take the same bounded retry the idle watchdog already takes, and the ORIGINAL exception is re-raised on exhaustion so the log names the real failure instead of an idle timeout. Gated two ways. Status-bearing errors (401/529/400) and aborts fail fast — retrying a revoked token just burns the budget on guaranteed 401s. And the retry is skipped once any text has been handed to the caller: a re-attempt replays the response from scratch, so retrying after partial output would emit those chunks twice, which is exactly what query.py:1635 forbids ("never after partial output"). That check sits above this layer and cannot see a retry taken here, so the gate has to live here too. ## AskUserQuestion was reachable in --print fix-git called it twice, then ended with "let me explain what I found" instead of finishing; the verifier found the files untouched. It is not in the initial toolset — the model reached it through ToolSearch. Nothing on the headless surface can answer it (StreamJsonReader parses only `user` messages; there is no can_use_tool protocol), so it is now unregistered rather than left registered with a stubbed responder. This is parity, not special-casing: the TS reference's AskUserQuestionTool already reports itself disabled when nobody is at the keyboard. Removal happens before the allow/deny filters so --allowed-tools cannot resurrect it, and it propagates to subagents, which share the registry instance. Test doubles gained `remove_tool` rather than the production call gaining a getattr guard — a silent no-op there is the bug class that left --allowed-tools dead for months (#739). ## The idle-timeout message was written for a grader It appended "Connection timed out" with a comment saying the phrasing was deliberate so eval harnesses would classify the failure as retryable. That optimizes for the scoreboard rather than the agent, and it was also untrue — an idle stream is not a connection timeout, so it pointed debuggers at the wrong subsystem. Removed; the test now asserts the honest wording and that the old clause stays gone. ## Diagnosed, deliberately NOT fixed Two trials died on `401 OAuth access token has been revoked` — not expiry (one died 4.7 minutes in against a 2h runway) but refresh rotation: containers hold a refresh-token-free copy, so anything rotating the host credential revokes the in-flight token. An in-adapter retry would hand clawcodex attempts the official harness would not give another agent, so this is documented in the adapter with a recommendation to use an API key for scored runs. Tests: 8841 passed. Every fix is revert-sensitive — each line was deleted and the intended test confirmed failing. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
--allowed-tools/--disallowed-toolsare parsed and plumbed into_filter_registry(...)in both the headless (--print) path (src/entrypoints/headless.py) and the agent-server / TUI path (src/server/agent_server.py) — but_filter_registrycalledregistry.unregister(name), a method that does not exist onToolRegistry(onlyremove_tooldoes). The resultingAttributeErrorwas swallowed by a bareexcept Exception: continue, so the filter was a silent no-op: the flags removed nothing and the model still saw the full tool pool (schemas are emitted fromregistry.list_tools(), consumed atsrc/query/agent_loop_compat.py:608). The registry even documented this dead path in a comment, deferring the fix.Fix (3 commits)
_filter_registrycall sites at the realToolRegistry.remove_tool()(already battle-tested by the live MCP tool-swap atagent_server.py:3695). The flags now actually narrow the pool the model sees. Both filters only ever remove tools → activating them can only narrow the toolset, never grant access. Default behavior (no flags) is untouched: the filter only runs whenallowed_tools/disallowed_toolsis non-empty.list_tools()yields only primary names, so--disallowed-tools KillShell(alias ofTaskStop) matched nothing and left the tool schema-present and dispatch-reachable. AddedToolRegistry.canonicalize_names(); both call sites resolve the allow/deny sets (primary or alias → primary) up front. Blank names are skipped so a stray""can't become a match-nothing allowlist.remove_tool+ doc nits —remove_toolnow resolves by primary-or-alias and clears every_by_namekey pointing at the tool (closes a latent dispatch leak for future alias-passing callers). Fixed a sibling staleunregistercomment inmcp_runtime.py; corrected flag spelling to kebab-case in docstrings.Scope / caveats
Bash(rm:*)) and add no deny-rule.ServerConfig.allowed_toolsyet); the reachable path today is headless-p.Why it matters
Found while comparing the clawcodex terminal-bench eval harness to Claude Code's. clawcodex exposes 46 model-facing tools in headless mode (~4,500 schema tokens/request) vs Claude Code's ~15 default; many (AskUserQuestion, EnterPlanMode, Cron*, ScheduleWakeup, Monitor, SendMessage, Team*, Workflow, Clipboard*) are non-functional in a one-shot headless container. The flag needed to trim that set was dead. This PR unblocks a focused eval toolset (a ~13-tool terminal set is ~2,200 tokens — ~2,300 tokens/request saved), to be enabled + A/B-validated separately.
Tests
tests/test_headless_tool_filter.py(10 tests): denylist/allowlist remove fromlist_tools(), alias-form denylist clears schema + dispatch, alias-form allowlist keeps the real tool,remove_tool-by-alias clears the canonical key, blank-input safety, idempotence, registry hasremove_toolnotunregister, agent-server parity. Reviewed by thecriticsubagent (APPROVE; alias gap it raised is fixed in commit 2). Related suites green:test_ch15_mcp_list_changed_round4,test_agent_toolkit_filtering,test_headless_cli,test_mcp_config_full(80 passed).🤖 Generated with Claude Code