feat(mcp): tool broker reaches read-only tools only (PLN-2027 PR 3)

[wongk]

Campaign chunk C3 of 7 (PRD-739 → PLN-2027), built one chunk at a time against main (which already includes C1 #15 and C2 #17).

C3 — Read-only broker; tool-surface cleanup (FR4, FR5)

Lands PLN-2027 PR 3.

• The tool broker now runs only tools on a read-only allow-list, kept in a new fork-only module src/lemoncrow/gateway/adapters/mcp/broker_policy.py so the upstream-owned mcp_server.py edit stays small.
• Refused through the broker: every tool that executes commands, writes files, writes memory, records review data, calls other tools or reaches the network (including web_fetch), plus statusline_segment and search (both write). graph is allowed per kind; index_docs, recall_docs, pr_risk and any call passing enable are refused.
• A refused call returns a JSON-RPC error naming read-only alternatives; the session continues. Hidden read-only tools (blame, grep, orient, read-only graph kinds) stay reachable.
• relations, code_query, code_coverage_check and code_changes stay advertised under both the core and full profiles.

Scope: a guardrail, not a security boundary

This PR narrows one route. It does not make review agents read-only, and it does not stop an agent that intends to write.

• Only the broker route is covered. The allow-list is checked inside the same server process that also offers bash and edit, running as the operator's OS user with full file and network access. Nothing below the server enforces it.
• The direct route stays open. LemonCrow's core profile advertises bash and edit to every agent. The code-review plugin's graph reviewer (3.10.0) removes only the built-in Bash, Edit and NotebookEdit tools and relies on its prompt to keep off MCP tools, and the PLN-1677 N1 hook has not shipped. A reviewer can call mcp__lc__bash or mcp__lc__edit directly, and where a Claude Code allow rule covers those tools, the auto-mode classifier never sees the call.
• Classifying tools by reading their code is fallible. Building this PR found three tools that could write despite looking read-only: statusline_segment, search, and code_changes (through an option-shaped base_ref). A bug in any allow-listed tool is a way out.
• What it does catch: an agent that reaches a writing tool through the broker by mistake.

Containing review agents is out of scope for the fork (PRD-739 v3): a review with LemonCrow connected is not contained, and a steered reviewer can still run commands or write files through tools it calls directly, Claude Code's own Write tool, or a defect in any tool.

Behaviour change to check

• Chat-only hosts reach statusline_segment (savings panel) by direct name now; the four lemoncrow SKILL.md copies say so. A host that only lets the model call tools listed in tools/list can no longer reach it, since it is hidden.
• "Writes files / changes the index or cache" is read as the tool's own writes. Engine index build and sync, telemetry.db, and read's semantic_file_index.json write are not counted, so engine-backed read tools stay broker-reachable.

Acceptance ledger

Criteria	Met	Met differently	Not met
31	22	9	0

Built in two attempts. A read-only audit of attempt 1 against the PRD-739 criteria found statusline_segment still broker-reachable although it writes a file; attempt 2 fixed that (and search) and added the four PRD-739 rows (User Story 4 ×3, FR5) to the ledger. The stale test_tools_list_hides_internal_workflow_tools failure the plan targets already passes on main after #16.

Deviations from the plan

• Plan: Replace _BROKER_DENIED (11363) in mcp_server.py with an allow-list, _BROKER_READ_ONLY.
  Did: Put the allow-list, the read-only graph kinds and the refusal function in a new fork-only module, src/lemoncrow/gateway/adapters/mcp/broker_policy.py. mcp_server.py imports them as _BROKER_READ_ONLY and _broker_refusal.
  Why: Constraint (PLN-2027 section 0): mcp_server.py is upstream-owned, so keep its edits small and put nothing there that can live in a fork-only module.
• Plan: graph: read-only kinds only; index_docs and any call passing enable are refused through the broker.
  Did: graph is allowed per kind through an allow-list of 9 kinds. Refused: index_docs, any call passing enable, and additionally recall_docs and pr_risk.
  Why: The campaign decision says to refuse any kind the code shows can write, execute or reach the network. recall_docs embeds its query through the configured embedder (the OpenAI backend posts at src/lemoncrow/infra/embeddings/openai_embedder.py:52), and opening its store runs CREATE TABLE IF NOT EXISTS (src/lemoncrow/pro/capabilities/code_health/doc_index.py:162-181). pr_risk calls summarize_file (src/lemoncrow/pro/capabilities/code_health/pr_risk.py:119 in _file_complexity, and :148), which writes the machine-wide semantic file index. PR 6 moves only the blast radius to open_file_graph, and _file_complexity still writes, so PR 6 alone does not make pr_risk broker-safe.
  Affects: PLN-2027 PR 6
• Plan: New test test_broker_refuses_execution_write_and_network_tools, parametrized over the deny class; and test_broker_deny_list_holds becomes the deny-class parametrization.
  Did: Wrote one test under the new name. It replaces test_broker_deny_list_holds and keeps that test's search-absence assertion.
  Why: Both plan items describe the same parametrization; two copies would be duplicate coverage.
• Plan: Fix the stale e2e test test_tools_list_hides_internal_workflow_tools; it currently fails on main.
  Did: No change.
  Why: It passes on base 2c4d1c2 after PR ci: get the fork workflows green #16 (1 passed), and the constraint says to record it as met on base rather than change code.
• Plan: Verification per PR: make lint, make typecheck from a fresh cache, and the full suite in a scratch worktree diffed against a main baseline run in an identical worktree.
  Did: Attempt 1 ran ruff, black and mypy --strict on the changed files, the orchestrator's scoped test list, and pytest -m 'not slow' once, reproducing every failure on origin/main. Attempt 2 ran the attempt-2 scope: the gate and e2e files, every test file naming statusline_segment or quoting search, ruff, black, mypy --strict and the mypyc safety test.
  Why: The chunk's verification scope excludes make typecheck over all of src and the full suite. Attempt 2 also excludes the not-slow run, since a one-entry allow-list change cannot fail differently outside the files it ran.
• Plan: PR 3 class table: Allow | read, grep, code_search, search, relations, code_query, code_changes, code_coverage_check, blame, orient, statusline_segment.
  Did: Took statusline_segment and search off BROKER_READ_ONLY (src/lemoncrow/gateway/adapters/mcp/broker_policy.py:36-49), recorded why in its docstring, and added both to the test deny class (tests/gateway/test_cap_tools_list_gate.py:267, :269). test_broker_calls_tools_that_are_hidden_under_the_core_profile now runs over blame, graph, grep and orient.
  Why: Both write, which PRD-739 User Story 4 AC (FR4) forbids through the broker. statusline_segment rewrites the session sidecar in its default segment format (src/lemoncrow/gateway/adapters/mcp_server.py:10160). Its markdown and json formats fold unfolded session ledgers into savings_aggregate.json (src/lemoncrow/core/capabilities/savings_summary.py:2837-2846, reached through build_savings_report), so refusing only format=segment would not fix it. search stores each query's results in the workspace search cache (src/lemoncrow/pro/capabilities/tool_supervision/smart_search.py:619, :677), and the query argument chooses that entry. The attempt-2 constraint says to fix any other writer the same way. Nothing in PLN-2027 PR 4-7 reaches either tool through the broker.
• Plan: Attempt-2 constraint: if something reaches statusline_segment through the broker, keep that caller working by its direct route and record it.
  Did: The four lemoncrow SKILL.md copies (integrations/skills/lemoncrow/SKILL.md:56-59; integrations/claude/plugin/skills/lemoncrow/SKILL.md, integrations/codex/plugin/skills/lemoncrow/SKILL.md and integrations/antigravity/skills/lemoncrow/SKILL.md, :58-61) now tell chat-only hosts to call statusline_segment directly by name, not through the broker. The tool_statusline_segment docstring (src/lemoncrow/gateway/adapters/mcp_server.py:10138-10141) now says the broker refuses it.
  Why: Those four files were the only caller that went through the broker. Hooks and statusline scripts read the sidecar file, and _mcp_profile.py calls the handler directly. The server runs a direct tools/call on a hidden tool: the dispatch looks up TOOLS with no visibility check (src/lemoncrow/gateway/adapters/mcp_server.py:12346), and a core-profile probe returned the 756-char markdown panel. Limit: neither profile advertises statusline_segment (HIDDEN_LLM_TOOLS), so a host that lets the model call only listed tools has no route to it. Advertising it would change the tool surface, and PR 3 does not decide that.
• Plan: PRD-739 User Story 4 AC: The broker refuses any tool that executes commands, writes files, changes the index or cache, writes memory, records review data, or calls other tools.
  Did: Applied the criterion to the writes a tool makes by its own function, and disclosed the rest instead of refusing it. Not counted: the engine building and syncing the workspace index (code_context, fts, intel and vectors sqlite), and telemetry.db, both seen from code_search, grep, blame, relations and code_query. Also not counted: read's write to semantic_file_index.json. read stays allowed.
  Why: Read literally, the criterion would refuse every engine-backed read, and even the broker call itself, whose dispatch records telemetry. No read-only broker would remain. The attempt-1 audit drew the same line for engine maintenance, because advertised tools trigger it on their direct routes. read is advertised under both profiles, so refusing it through the broker removes no route to its write. graph kind=pr_risk makes the same semantic-index write and stays refused, because graph is hidden and the broker would be a new route.

Review

/code-review:deep --base origin/main (code-review 3.10; review_root = this worktree at 916d594f, code intelligence on): APPROVED — verified 1, justified 0, rejected 0, pending 0, coverage gaps 0. No BLOCKING/HIGH.

Fixed

• bha_p0_f0 (MEDIUM, Security) src/lemoncrow/gateway/adapters/mcp/broker_policy.py:39 -- code_changes is allow-listed as read-only but base_ref reaches git merge-base / git diff as an option, so base_ref='--output=' writes an arbitrary file. Reproduced at 916d594 before fixing. Fixed in the tool itself (src/lemoncrow/infra/code_intel/change_impact.py): collect_changes refuses a base_ref starting with '-' by raising GitUnavailable, and both git calls that take the ref pass --end-of-options (installed git 2.50.1 supports it for merge-base and diff). code_changes stays on BROKER_READ_ONLY because the tool is fixed on both the broker and direct routes. Regression test test_option_shaped_base_ref_is_refused_and_writes_nothing fails without the fix and passes with it. (52e179df)

The injection predates this PR (it is in the fork's change_impact.py), but it let an allow-listed tool write through the broker, so it is fixed here. code_changes stays allow-listed because the tool itself now refuses option-shaped refs on every route.

Test pruning (/prune-tests): reviewed 3 new tests, cut 0.

Re-review gate: SKIP: 1 prod file(s) changed, none overlapping blocking_high, so no second review ran.

Verification

• uv run pytest -q tests/gateway/test_mcp_jsonrpc_e2e.py::test_tools_list_hides_internal_workflow_tools (on base 2c4d1c23, before any edit) — pass: 1 passed in 1.16s. PLN-2027 PR 3 item 4 is met on base.
• uv run pytest -q tests/gateway/test_cap_tools_list_gate.py tests/gateway/test_mcp_tool_handlers.py tests/gateway/test_p0_mcp_surfaces.py tests/gateway/test_agen — pass: 1 failed, 327 passed, 5 deselected in 41.71s. The failure, test_generated_agent_contexts.py::test_copilot_instructions_has_the_compact_managed_block, is 'FileNotFoundError: ... .github/copilot-instructions.md' (a gitignored generated file). It fails identically on origin/main 2c4d1c2 in a scratch w
• uv run pytest -q tests/gateway/test_mcp_jsonrpc_e2e.py — pass: 10 passed, 1 deselected in 3.17s.
• uv run pytest -q -p no:cacheprovider $(git grep -l -E '_broker_reachable|_tool_broker_handler|_TOOL_BROKER_SPEC|tool_statusline_segment' -- tests/) — pass: The grep matched only tests/gateway/test_cap_tools_list_gate.py; 51 passed in 1.20s.
• Counterfactuals: scratchpad driver cf_c3.py applies 9 single-line mutations to broker_policy.py or mcp_server.py, runs one isolated node each, and restores from — pass: Unmutated baseline: 8 passed. Every mutated run gave '1 failed' with the target assertion. Every file was byte-identical after restore.
• uv run ruff check src/lemoncrow/gateway/adapters/mcp/broker_policy.py src/lemoncrow/gateway/adapters/mcp_server.py tests/gateway/test_cap_tools_list_gate.py — pass: All checks passed!
• uv run black --check src/lemoncrow/gateway/adapters/mcp/broker_policy.py src/lemoncrow/gateway/adapters/mcp_server.py tests/gateway/test_cap_tools_list_gate.py — pass: 3 files would be left unchanged.
• uv run mypy --strict src/lemoncrow/gateway/adapters/mcp/broker_policy.py src/lemoncrow/gateway/adapters/mcp_server.py — pass: Success: no issues found in 2 source files. Clean, so no origin/main comparison was needed.
• uv run pytest -q tests/test_mypyc_compile_safety.py — pass: 3 passed in 2.76s.
• git diff --check; .githooks pre-commit (ruff --fix, black, mypy on staged files) — pass: diff-check clean. The hook passed on commit 32f759b and changed no files.
• uv run pytest -q -m "not slow" in the chunk worktree (no failure beyond origin/main) — pass: 9 failed, 6840 passed, 19 skipped, 155 deselected in 660.48s. It ran 60s past the 10-minute cap: the 600s wait returned still-running, and the kill arrived after pytest had exited, so the result is complete, not truncated. The 9 failures: 5 in test_daemon_ownership_races.py (AF_UNIX path too long);
• uv run pytest -q -p no:cacheprovider tests/gateway/test_cap_tools_list_gate.py tests/gateway/test_mcp_jsonrpc_e2e.py — pass: 61 passed, 1 deselected, 1 warning in 32.39s, on the tree committed as 916d594 (the pre-commit hook left all files unchanged).
• git grep -l 'statusline_segment' -- tests/, then uv run pytest -q -p no:cacheprovider on those files — pass: 6 files: tests/core/capabilities/test_usage_spend.py, tests/core/test_savings_summary_render.py, tests/gateway/test_cap_tools_list_gate.py, tests/gateway/test_phase_d3_d4.py, tests/gateway/test_statusline_script.py, tests/gateway/test_stop_hook_savings_rows.py. 109 passed, 1 warning in 42.75s.
• Changed allow-list entry search: git grep -l for the quoted tool name ('search' or "search") in tests/, then uv run pytest -q -p no:cacheprovider on the matchin — pass: 41 matches. The first run passed the 2 non-Python matches too (tests/benchmarks/context_quality/README.md, tests/fixtures/retrieval/ground_truth.jsonl), pytest stopped with 'ERROR: not found' and no tests ran. Rerun on the 39 .py files: 786 passed, 46 deselected, 20 warnings in 491.66s (under the 10
• uv run pytest -q -p no:cacheprovider tests/test_mypyc_compile_safety.py — pass: 3 passed in 10.52s.
• Counterfactuals on broker_policy.py: copy aside, apply one single-line mutation, run one isolated node, restore from the copy, compare shasum — pass: Unmutated baseline for [statusline_segment] and [search]: '2 passed'. Adding statusline_segment back to BROKER_READ_ONLY: [statusline_segment] '1 failed', 'Failed: DID NOT RAISE ToolArgumentError'. Adding search back: [search] '1 failed', same message. Removing orient: test_broker_calls_tools_that
• Write probes (scratchpad, not committed): every allow-listed tool through the broker handler, snapshotting a scratch LEMONCROW_ROOT, XDG_CACHE_HOME and a scratc — pass: Probe 1 ran in-process and resolved some tools to the real workspace, so it was superseded. Probe 2 ran a fresh process per call with a pinned workspace and one unfolded savings ledger; two inline runs followed. statusline_segment markdown (direct handler) created root/savings_aggregate.json. search
• Route probe through mcp_server._handle under LEMONCROW_MCP_TOOL_PROFILE=core with a scratch LEMONCROW_ROOT — pass: tools/list advertises tool and neither statusline_segment nor search. tool action=call for statusline_segment {format: markdown}, statusline_segment {} and search {query: foo} each returned error -32602 with 'Read-only alternatives: read, code_search, relations, code_query.' An empty broker search l
• git grep for callers of statusline_segment through the broker (hooks, statusline scripts, integrations/) — pass: Only the 4 lemoncrow SKILL.md copies called it through the tool broker; they now use the direct route. integrations/claude/plugin/hooks/stop.py and both statusline.sh scripts read or write the sidecar file, never through the broker. _mcp_profile.py calls the handler directly. After the change, git g
• git diff --check; .githooks pre-commit on 916d594f — pass: diff-check clean. The hook's ruff and black passes left all 3 files unchanged, and its mypy gave 'Success: no issues found in 2 source files'. 'All pre-commit checks passed successfully!'
• git status --porcelain in the chunk worktree and in the main checkout, after commit 916d594f — pass: Both printed nothing.
• Resolve: uv run pytest -q tests/infra/code_intel/test_change_impact.py tests/gateway/test_cap_tools_list_gate.py tests/gateway/test_completeness_contract.py — pass
• Resolve: uv run pytest -q tests/infra/code_intel/test_change_impact.py tests/infra/code_intel/test_completeness.py (files from git grep -l -E 'analyze_changes|collect_changes|_dif — pass
• Resolve: uv run ruff check src/lemoncrow/infra/code_intel/change_impact.py tests/infra/code_intel/test_change_impact.py — pass
• Resolve: uv run mypy --strict src/lemoncrow/infra/code_intel/change_impact.py — pass
• Resolve: uv run pytest -q tests/test_mypyc_compile_safety.py — pass
• Resolve: git diff --check — pass
• Resolve: regression test with the change_impact.py fix stashed: uv run pytest -q tests/infra/code_intel/test_change_impact.py -k option_shaped_base_ref — pass
• Resolve: pre-commit hook on commit 52e179df — pass

Still owed by later chunks

• C4 (PR 4): coverage matches the index; FR7–FR10 anchors and semantics.
• C5 (PR 5): keep operators on the fork build.
• C6 (PR 6): repo-correct PR risk and review-module warm-up. pr_risk stays refused through this broker because _file_complexity writes the machine-wide semantic index; PR 6 alone does not remove that write.
• C7 (PR 7): P2 spikes plus removal of the fork-only lc code export/import.
• After this merges: the plan §10 GA check (one /code-review:deep with LemonCrow reinstalled from main).

Artifacts: PRD-739 · PLN-2027