Block MCP tools in plain Claude completions

[joshnroy]

Summary

• Add --disallowedTools "*" to the plain Claude completion client: --tools "" disables built-ins but does not block MCP tools.
• Add two independent Docker integration tests: prove the harmless MCP tool is invoked without the flag, and unavailable with it. Both use the same ordinary Python probe and MCP server files, with setup and assertions explicit in each test.
• Mark paid integration tests with pytest.mark.integration and exclude them from default pytest/CI runs. Run explicitly with python -m pytest tests/utils/test_llm.py -m integration -v -s.

Verification

• Live Docker tests: 2 passed on Claude Code 2.1.233. Control: tool exposed, server invocation recorded, random token returned. Patched: empty tool inventory, zero server invocations, no token returned.
• Local Claude 2.1.247 also reproduced the issue and verified the fix during investigation; only Docker coverage is retained.
• Final non-live regression suite: 80 passed, 2 integration tests deselected.
• Black and diff whitespace checks passed.
• Standalone pylint was not clean: the existing .pylintrc contains obsolete/unrecognized options in the installed pylint. Full CI has not yet run.

Scope and limitations

The tests use run_genplan_in_docker() with production authentication, filtered source mounts, image entrypoint, firewall, and privilege dropping. They replace only the robot-training driver with a diagnostic probe calling the real ClaudeCLIClient.

This verifies model tool isolation, not containment of generated policy execution or full experiment readiness. The flag affects all users of the shared plain Claude completion client. Campaign YAMLs and unrelated lockfile changes are intentionally excluded.

[merlerm]

LGTM, maybe we can get a @tomsilver Claude review to be safe

[tomsilver]

The core finding is real and the one-line fix is right: --tools "" only covers built-ins, so MCP tools stayed live in what's supposed to be a plain LLM call. The unit test guards the flag in CI, and the Docker control/treatment pair is a good way to demonstrate the behavior. Two substantive things I'd change before merge, plus tooling friction that will show up on the first run_ci_checks.sh.

Substantive

1. --disallowedTools "*" leans on undocumented wildcard semantics, and nothing in CI would notice if it broke.

claude --help on 2.1.247 documents the flag as "Comma or space-separated list of tool names to deny (e.g. Bash(git *) Edit)". A bare * matching every tool isn't documented anywhere; it's an empirical observation on 2.1.233/2.1.247. If a future CLI version stops treating it as match-all, the failure is silent: tools come back, no error, and test_cli_denies_all_tools still passes because it only asserts the flag was passed, not that it had effect. The test that would catch it is opt-in, paid, and Docker-gated, so it will effectively never run again after this PR.

2. Production still loads user/project settings; the probe doesn't, so the test validates a stricter config than the one that ships.

record_cli appends --strict-mcp-config, --mcp-config, --setting-sources "", and --no-session-persistence before invoking the CLI. Production complete() passes none of those. In the real path --setting-sources is unset, so user/project/local settings load: MCP servers configured in ~/.claude.json are still spawned as processes (deny only stops the model from calling them), and settings can still inject hooks and permission rules into a call that is meant to be a stateless completion. The machine-dependent part of the attack surface is the part the test doesn't cover.

Both points have the same fix. In complete() (src/robocode/utils/llm/cli_client.py:45):

"--strict-mcp-config",     # no --mcp-config given => zero MCP servers configured
"--setting-sources", "",   # no user/project/local settings, hooks, or permission rules

That makes the guarantee structural (servers are never configured) instead of dependent on deny-rule matching, and it makes the client behave the same on every machine. Keep --disallowedTools "*" as defense in depth. The test then exercises the real production arg list rather than an augmented one, and the probe only needs to add --mcp-config and --output-format stream-json. Worth confirming on one live run that --setting-sources "" doesn't disturb auth in the container: credentials aren't settings, so it shouldn't, but the sandbox mounts a throwaway ~/.claude.

3. The control test asserts on model behavior.

assert token in final["result"] fails if the model declines, paraphrases, or wraps the token. init["tools"] containing mcp__probe__probe_token plus a non-empty calls.txt already prove the tool was exposed and invoked. I'd drop the token assertion from the control and keep token not in ... in the deny case, which is cheap and meaningful.

Tooling friction (verified locally)

4. The _thread import moves depend on which isort you have. isort 9.0.0 accepts the new ordering (clean); isort 8.0.0 reverts both hunks back to the third-party block. CI's isort/isort-action@master pulls latest, so CI is happy and a local ./run_ci_checks.sh on an older isort is not. Pin it in the develop extra ("isort>=9") rather than letting the two fight.

5. run_autoformat.sh isn't clean on this branch. docformatter rewraps both new docstrings (wrap-descriptions = 88), e.g. in claude_mcp_probe.py:

-Both tests use this program. --without-deny-flag reproduces the old wrapper;
-otherwise all production restrictions remain. Each call has a $1 CLI budget.
+Both tests use this program. --without-deny-flag reproduces the old wrapper; otherwise
+all production restrictions remain. Each call has a $1 CLI budget.

Not a CI job, but it means the next person's autoformat produces unrelated diff noise. Same for the test_llm.py module docstring.

6. The addopts change is safe. I checked the marker interaction empirically: a CLI -m pylint overrides the ini -m 'not integration' (last -m wins), so pytest . --pylint -m pylint in the lint job is unaffected, and pytest tests/ correctly deselects the two integration tests. black, mypy, and pylint are clean on the new files.

Nits

• The two integration tests are ~30 duplicated lines differing by one flag and three assertions. @pytest.mark.parametrize("deny", [False, True]) over a shared _run_probe helper would read better. The PR body defends the duplication as explicitness, so take or leave it.
• check=kwargs.pop("check", True) in both record_cli and launch_probe flips subprocess's default for no reason: every call site in cli_client and docker_sandbox passes check explicitly, so the default is dead and the flip would surprise a future caller that relies on check=False. Just real_run(args, **kwargs).
• The probe hardcodes "model": "claude-opus-5", which will drift from experiments/conf/approach/completion/cli_claude.yaml. Read it from the config or a shared constant.
• $1 budget x 2 tests is ~$2 per integration run. Worth putting next to the run command in the README, not only in the PR body.
• write_text/read_text in _create_mcp_tool and _parse_claude_output omit encoding="utf-8", unlike the fixtures and the rest of the repo.
• cli_client.py's module docstring still says "with tools and system prompt stripped"; mention MCP now that that's the interesting part.