Merge pull request #18 from closedloop-ai/campaign-prd-739-20260915-c3

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

Author: wongk

integrations/antigravity/skills/lemoncrow/SKILL.md

@@ -55,10 +55,10 @@ Global scope by default. Add `--workspace <dir>` only if the user names a specif
 
    Unknown key → run `lc settings show`, relay the valid keys.
 
-5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the broker by exact name — it returns a markdown panel, relay it verbatim:
+5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the `statusline_segment` tool by exact name, not through the `tool` broker (which refuses it) — it returns a markdown panel, relay it verbatim:
 
    ```json
-   {"name": "tool", "arguments": {"action": "call", "name": "statusline_segment", "arguments": {"format": "markdown"}}}
+   {"name": "statusline_segment", "arguments": {"format": "markdown"}}
    ```
 
    `format` accepts `markdown` (chat panel), `json` (raw report), `segment` (one statusline frame). Never recompute or restate the numbers.

integrations/claude/plugin/skills/lemoncrow/SKILL.md

@@ -55,10 +55,10 @@ Global scope by default. Add `--workspace <dir>` only if the user names a specif
 
    Unknown key → run `lc settings show`, relay the valid keys.
 
-5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the broker by exact name — it returns a markdown panel, relay it verbatim:
+5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the `statusline_segment` tool by exact name, not through the `tool` broker (which refuses it) — it returns a markdown panel, relay it verbatim:
 
    ```json
-   {"name": "tool", "arguments": {"action": "call", "name": "statusline_segment", "arguments": {"format": "markdown"}}}
+   {"name": "statusline_segment", "arguments": {"format": "markdown"}}
    ```
 
    `format` accepts `markdown` (chat panel), `json` (raw report), `segment` (one statusline frame). Never recompute or restate the numbers.

integrations/codex/plugin/skills/lemoncrow/SKILL.md

@@ -55,10 +55,10 @@ Global scope by default. Add `--workspace <dir>` only if the user names a specif
 
    Unknown key → run `lc settings show`, relay the valid keys.
 
-5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the broker by exact name — it returns a markdown panel, relay it verbatim:
+5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the `statusline_segment` tool by exact name, not through the `tool` broker (which refuses it) — it returns a markdown panel, relay it verbatim:
 
    ```json
-   {"name": "tool", "arguments": {"action": "call", "name": "statusline_segment", "arguments": {"format": "markdown"}}}
+   {"name": "statusline_segment", "arguments": {"format": "markdown"}}
    ```
 
    `format` accepts `markdown` (chat panel), `json` (raw report), `segment` (one statusline frame). Never recompute or restate the numbers.

integrations/skills/lemoncrow/SKILL.md

@@ -53,10 +53,10 @@ Global scope by default. Add `--workspace <dir>` only if the user names a specif
 
    Unknown key → run `lc settings show`, relay the valid keys.
 
-5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the broker by exact name — it returns a markdown panel, relay it verbatim:
+5. **"what are my savings?" / cost questions** — shell available (Claude Code, Codex CLI): run `lc usage` (add `optimize` for savings analysis, `optimize detail` for the per-operation breakdown) and relay it. No shell (chat-only host): call the `statusline_segment` tool by exact name, not through the `tool` broker (which refuses it) — it returns a markdown panel, relay it verbatim:
 
    ```json
-   {"name": "tool", "arguments": {"action": "call", "name": "statusline_segment", "arguments": {"format": "markdown"}}}
+   {"name": "statusline_segment", "arguments": {"format": "markdown"}}
    ```
 
    `format` accepts `markdown` (chat panel), `json` (raw report), `segment` (one statusline frame). Never recompute or restate the numbers.

src/lemoncrow/gateway/adapters/mcp/broker_policy.py

@@ -0,0 +1,82 @@
+"""What the MCP ``tool`` broker may run (PRD-739 FR4). Fork-only.
+
+The broker calls a registered tool by exact name, and the review agents that
+hold it read diffs written by PR authors. So it runs only tools that read:
+anything not on :data:`BROKER_READ_ONLY` is refused, and a tool registered later
+stays unreachable until it is classified (``test_cap_tools_list_gate`` fails
+until it is).
+
+Refused because the code shows a write, execute or network path:
+
+* ``scan`` runs the ast-grep binary in a subprocess.
+* ``context`` records the task on the session ledger.
+* ``statusline_segment`` rewrites the session's statusline sidecar in its
+  default ``segment`` format; ``markdown`` and ``json`` fold unfolded session
+  ledgers into the persisted savings aggregate. The lemoncrow skill calls it
+  directly by name instead.
+* ``search`` stores each query's results in the workspace search cache
+  (``smart_state.json``).
+* ``graph kind=index_docs`` writes the design-doc store; ``recall_docs`` embeds
+  its query through the configured embedder (the OpenAI one posts to the
+  network) and creates the store schema on connect; ``pr_risk`` folds each
+  changed file into the machine-wide semantic file index. ``enable`` only
+  switches ``index_docs`` indexing on, and is refused outright.
+
+Not counted as writes: the code-intel engine building and syncing the workspace
+index, and telemetry, which every engine-backed read triggers on its direct
+route too. ``read`` also folds the file it reads into the semantic file index;
+it stays because it is advertised, so the broker adds no route to that write.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Mapping
+from typing import Any
+
+BROKER_READ_ONLY: frozenset[str] = frozenset(
+    {
+        "blame",
+        "code_changes",
+        "code_coverage_check",
+        "code_query",
+        "code_search",
+        "graph",
+        "grep",
+        "orient",
+        "read",
+        "relations",
+    }
+)
+
+# `graph` runs only these kinds, which read the index or git history.
+GRAPH_READ_ONLY_KINDS: frozenset[str] = frozenset(
+    {
+        "blast_radius",
+        "centrality",
+        "commit_provenance",
+        "coupling",
+        "cycles",
+        "dead_code",
+        "design_gaps",
+        "topology",
+        "verify_design",
+    }
+)
+
+_ALTERNATIVES = "Read-only alternatives: read, code_search, relations, code_query."
+
+
+def broker_refusal(name: str, arguments: Mapping[str, Any]) -> str | None:
+    """Why the broker must not run *name* with *arguments*; ``None`` when it may."""
+    if name not in BROKER_READ_ONLY:
+        return f"{name!r} is not reachable through the broker, which runs read-only tools only. {_ALTERNATIVES}"
+    if name == "graph":
+        if "enable" in arguments:
+            return f"graph `enable` is not reachable through the broker: it switches on indexing. {_ALTERNATIVES}"
+        kind = arguments.get("kind", "blast_radius")
+        if not isinstance(kind, str) or kind not in GRAPH_READ_ONLY_KINDS:
+            return (
+                f"graph kind={kind!r} is not reachable through the broker, which runs only the kinds "
+                f"{', '.join(sorted(GRAPH_READ_ONLY_KINDS))}. {_ALTERNATIVES}"
+            )
+    return None

src/lemoncrow/gateway/adapters/mcp_server.py

@@ -80,6 +80,8 @@
 from lemoncrow.gateway.adapters.mcp.bash import (
     tool_bash as tool_bash,
 )
+from lemoncrow.gateway.adapters.mcp.broker_policy import BROKER_READ_ONLY as _BROKER_READ_ONLY
+from lemoncrow.gateway.adapters.mcp.broker_policy import broker_refusal as _broker_refusal
 from lemoncrow.gateway.adapters.mcp.deferral import (  # noqa: F401  (re-exported for back-compat)
     _defer_bash_enabled,
     _defer_web_fetch_enabled,
@@ -325,7 +327,7 @@ def _warm_pricing_table() -> None:
     }
 )
 
-# Tools the shadowed ``core/environment.py`` hides that we advertise anyway.
+# Tools upstream's ``core/environment.py`` hides that the fork advertises anyway.
 #
 # `relations` is the only *enumerative* symbol tool. Hidden, an agent under the
 # core profile sees exactly one code-intel tool -- `code_search` -- which ranks;
@@ -337,8 +339,9 @@ def _warm_pricing_table() -> None:
 # symbol, not a diff -- it would have to make the edit first and then ask what
 # broke, which is backwards.
 #
-# HIDDEN_LLM_TOOLS lives in a module that resolves from a compiled `.so` when an
-# engine is vendored, so the override belongs here, in live source.
+# Fork policy differs from upstream's HIDDEN_LLM_TOOLS. The override lives here,
+# in live source, rather than as an edit to core/environment.py, so upstream's
+# changes to that set do not conflict on every merge.
 _FORCE_VISIBLE_TOOLS: frozenset[str] = frozenset({"relations"})
 
 # --------------------------------------------------------------------------- #
@@ -10132,10 +10135,10 @@ def tool_statusline_segment(format: str = "segment") -> str:
       that render chat markdown and have no shell to run the CLI.
     - ``format="json"``: the raw savings report payload, JSON-encoded.
 
-    Hidden from tools/list (see HIDDEN_LLM_TOOLS) but reachable by exact name
-    through the `tool` broker (anything not currently advertised is, unless it
-    is in _BROKER_DENIED), which is how the lemoncrow skill answers "what are
-    my savings?" without a shell.
+    Hidden from tools/list (see HIDDEN_LLM_TOOLS) but callable by exact name,
+    which is how the lemoncrow skill answers "what are my savings?" without a
+    shell. The `tool` broker refuses it: every format writes (the sidecar, or the
+    savings aggregate).
     """
     fmt = (format or "segment").strip().lower()
     if fmt in {"markdown", "md", "json"}:
@@ -11374,28 +11377,26 @@ def tool_compact(
     "search for those. Use search once for a rare capability, then call its exact name."
 )
 
-# Tools the broker must never reach, whatever the advertised surface says.
-# `tool` itself would recurse; the other four spawn subagents, proxy arbitrary
-# external servers, run arbitrary SQL, or rewrite the tree en masse -- reaching
-# any of those through a generic escape hatch is not a fallback, it is a
-# footgun. Everything else that is merely unadvertised is fair game: the broker
-# exists precisely so a hidden tool is still reachable by exact name.
-_BROKER_DENIED = frozenset({"agent", "codemod", "mcp", "sql", "tool", "workflow"})
+# The broker reaches read-only tools only (PRD-739 FR4). Agents that hold it read
+# diffs written by PR authors, so a shell, a writer, an outbound fetch or another
+# agent must not be one call away. _BROKER_READ_ONLY and the per-call refusal
+# (graph is allowed kind by kind) live in the fork-only broker_policy module.
 
 
 def _broker_reachable(tool_name: str, spec: dict[str, Any]) -> bool:
     """True when the broker may search for, and call, *tool_name*.
 
-    The guard is "is it advertised right now", not "is it in the core profile".
-    The two are not the same question, and conflating them is what limited this
-    broker to a single reachable tool: `relations` and `grep` are in both
-    ``_CORE_MCP_TOOLS`` and ``HIDDEN_LLM_TOOLS``, so the old code refused them
-    as "already exposed" while nothing ever advertised them.
+    Only allow-listed (read-only) tools qualify. Among those, the guard is "is it
+    advertised right now", not "is it in the core profile". The two are not the
+    same question, and conflating them is what limited this broker to a single
+    reachable tool: `relations` and `grep` are in both ``_CORE_MCP_TOOLS`` and
+    ``HIDDEN_LLM_TOOLS``, so the old code refused them as "already exposed"
+    while nothing ever advertised them.
 
     search and call share this predicate on purpose -- search must never return
     a tool that call would then refuse.
     """
-    if tool_name in _BROKER_DENIED:
+    if tool_name not in _BROKER_READ_ONLY:
         return False
     return not _tool_advertised_now(tool_name, spec)
 
@@ -11427,16 +11428,18 @@ def _tool_broker_handler(args: dict[str, Any]) -> dict[str, Any] | Any:
         target = str(args.get("name") or "").strip()
         if not target:
             raise _ToolArgumentError("tool call requires an exact name")
-        # Deny first: the answer must not depend on whether the name happens to
-        # be registered. `tool` itself is not in TOOLS at all.
-        if target in _BROKER_DENIED:
-            raise _ToolArgumentError(f"{target!r} is not reachable through the broker")
-        call_spec = TOOLS.get(target)
-        if call_spec is None:
+        # An unregistered name is a typo. A registered name off the read-only
+        # allow-list -- or `tool` itself, which is not in TOOLS -- is refused
+        # before anything runs, with read-only alternatives in the message.
+        if target not in TOOLS and target != "tool":
             raise _ToolArgumentError(f"unknown tool: {target}")
         arguments = args.get("arguments") or {}
         if not isinstance(arguments, dict):
             raise _ToolArgumentError("tool arguments must be an object")
+        refusal = _broker_refusal(target, arguments)
+        if refusal is not None:
+            raise _ToolArgumentError(refusal)
+        call_spec = TOOLS[target]
         handler = cast(Callable[[dict[str, Any]], Any], call_spec["handler"])
         result = handler(arguments)
         # An advertised tool used to be refused here as "call it directly" --

src/lemoncrow/infra/code_intel/change_impact.py

@@ -75,7 +75,7 @@
 
 
 class GitUnavailable(RuntimeError):
-    """*repo_root* is not a git worktree, or git could not be run there."""
+    """*repo_root* is not a git worktree, git could not be run there, or the ref is unusable."""
 
 
 @dataclass(frozen=True)
@@ -258,7 +258,7 @@ def _diff_ref(repo_root: Path, base_ref: str) -> str:
     independently. Falls back to *base_ref* itself when there is no common
     ancestor (unrelated histories, or a ref that is not a commit).
     """
-    merge_base = _git(repo_root, "merge-base", base_ref, "HEAD")
+    merge_base = _git(repo_root, "merge-base", "--end-of-options", base_ref, "HEAD")
     if merge_base.returncode == 0 and merge_base.stdout.strip():
         return merge_base.stdout.strip()
     return base_ref
@@ -331,13 +331,21 @@ def flush() -> None:
 def collect_changes(
     repo_root: Path, base_ref: str = "HEAD", paths: list[str] | None = None
 ) -> tuple[str, list[FileChange]]:
-    """Run the diff and parse it. Returns ``(diff_ref, changes)``."""
+    """Run the diff and parse it. Returns ``(diff_ref, changes)``.
+
+    *base_ref* is caller-supplied, so one that starts with ``-`` is refused
+    before git sees it: git would parse it as an option (``--output=<file>``
+    makes ``git diff`` write anywhere). ``--end-of-options`` backs that up on
+    every git call that takes the ref.
+    """
+    if base_ref.startswith("-"):
+        raise GitUnavailable(f"base_ref {base_ref!r} is not a ref: it starts with '-'")
     if not (repo_root / ".git").exists():
         probe = _git(repo_root, "rev-parse", "--git-dir")
         if probe.returncode != 0:
             raise GitUnavailable(f"{repo_root} is not a git worktree")
     diff_ref = _diff_ref(repo_root, base_ref)
-    args = ["diff", "--unified=0", "--no-color", "--find-renames", diff_ref]
+    args = ["diff", "--unified=0", "--no-color", "--find-renames", "--end-of-options", diff_ref]
     if paths:
         args.extend(["--", *paths])
     result = _git(repo_root, *args)