fix(mcp): two rule reads reach a read-only key, and every tool must now be classified (#3191)

rules_due_for_verification (the rule staleness sweep) and rule_history (what a
rule used to say) are pure reads, and a read-scoped API key was refused both:
neither is in _READ_ONLY_TOOLS, and the completeness test that should have
caught it only looked at tools whose NAMES start like a read (get_, list_,
search…). Neither does.

- Both join _READ_ONLY_TOOLS; the comment that pointed at this issue now says
  why they sat unlisted.
- _WRITE_TOOLS declares every writing tool by name. Nothing reads it at
  runtime — default-deny already refuses an unlisted tool — it exists so the
  classification is total.
- test_every_registered_tool_is_classified_exactly_once takes its candidates
  from what build_mcp_server() actually mounts, requires each in exactly one
  of the three sets, and still flags a classified name that is no tool. The
  decision stays explicit; only the candidate set widened.
- test_the_completeness_check_can_fail drops a real tool from its set and
  asserts it is noticed (rule 167).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy

tests/test_mcp_auth.py

@@ -96,66 +96,70 @@ def test_body_calls_write_tool_classifies_correctly():
     assert _body_calls_write_tool(b"not json") is False
 
 
-def test_every_read_shaped_tool_is_explicitly_classified():
-    """A read-shaped tool must be classified, not left to default-deny.
+def _registered_tool_names() -> set[str]:
+    """What the server actually mounts — not a glob of function names, which
+    would count helpers and miss anything registered another way."""
+    from scribe.mcp.server import build_mcp_server
 
-    `_READ_ONLY_TOOLS` is hand-maintained, and default-deny means a getter
+    return {tool.name for tool in build_mcp_server()._tool_manager.list_tools()}
+
+
+def test_every_registered_tool_is_classified_exactly_once():
+    """Every tool must be declared a read, a write, or a read-shaped write.
+
+    `_READ_ONLY_TOOLS` is hand-maintained, and default-deny means a read
     omitted from it fails CLOSED — safe, but silent. That is how a read key
-    ended up able to `get_note` and not `get_snippet`, both pure reads of the
-    same table, while design systems were unreachable entirely (#2496). The
-    `find_duplicate_snippets` entry was the tell: someone classified the report
-    and missed the getters beside it.
+    ended up able to `get_note` and not `get_snippet` (#2496), and how
+    `rules_due_for_verification` and `rule_history` sat denied (#3191).
 
-    This is the same shape as #2476 (record_pulled on three of four getters) —
-    a hand-written enumeration that missed the members added after it. The fix
-    there and here is the same: derive the CANDIDATES, keep the DECISION
-    explicit. Deriving the decision itself would be worse than a stale list —
-    it would make a security boundary follow a naming convention, so any future
-    `get_*` grants itself access.
-
-    So: every tool whose name reads like a read must appear in one of the two
-    sets. Adding a getter then forces a choice at review time.
+    The fix keeps the DECISION explicit and derives the CANDIDATES. Deriving
+    the decision would make a security boundary follow a naming convention, so
+    any future `get_*` grants itself access. This test used to derive
+    candidates from names too — tools starting `get_`, `list_`, `search`… — and
+    that narrowing is exactly what let the two #3191 reads through: neither
+    name looked like a read. So the candidates are now EVERY registered tool.
     """
-    import ast
-    import pathlib
-
-    from scribe.mcp.server import _DELIBERATELY_WRITE_SCOPED, _READ_ONLY_TOOLS
-
-    tools_dir = (pathlib.Path(__file__).resolve().parents[1]
-                 / "src" / "scribe" / "mcp" / "tools")
-    read_shaped = {
-        node.name
-        for path in tools_dir.glob("*.py") if path.name != "__init__.py"
-        for node in ast.parse(path.read_text()).body
-        if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef))
-        and node.name.startswith(("get_", "list_", "search", "resolve_",
-                                  "check_", "find_"))
-    }
-    assert read_shaped, "found no read-shaped tools — the tools package moved"
-
-    unclassified = sorted(read_shaped - _READ_ONLY_TOOLS
-                          - _DELIBERATELY_WRITE_SCOPED)
-    assert not unclassified, (
-        f"these read-shaped tools are classified by neither set: {unclassified}. "
-        f"They currently fail closed for read-only keys, silently. Add each to "
-        f"_READ_ONLY_TOOLS if it mutates nothing, or to "
-        f"_DELIBERATELY_WRITE_SCOPED with a comment saying what it writes."
+    from scribe.mcp.server import (
+        _DELIBERATELY_WRITE_SCOPED, _READ_ONLY_TOOLS, _WRITE_TOOLS,
     )
 
-    # The reverse: a name in either set that no longer exists is a rename or a
-    # deletion, and a stale grant is worth surfacing even though it grants
-    # access to nothing. `enter_project` is the one read tool without a read
-    # prefix, so it is checked against the full tool set, not `read_shaped`.
-    all_tools = {
-        node.name
-        for path in tools_dir.glob("*.py") if path.name != "__init__.py"
-        for node in ast.parse(path.read_text()).body
-        if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef))
-        and not node.name.startswith("_") and node.name != "register"
+    registered = _registered_tool_names()
+    assert len(registered) > 50, "found almost no tools — registration moved"
+
+    sets = {
+        "_READ_ONLY_TOOLS": _READ_ONLY_TOOLS,
+        "_WRITE_TOOLS": _WRITE_TOOLS,
+        "_DELIBERATELY_WRITE_SCOPED": _DELIBERATELY_WRITE_SCOPED,
     }
-    phantom = sorted((_READ_ONLY_TOOLS | _DELIBERATELY_WRITE_SCOPED) - all_tools)
+    unclassified = sorted(registered - set().union(*sets.values()))
+    assert not unclassified, (
+        f"these tools are classified by no set: {unclassified}. A read key is "
+        f"silently denied them. Add each to _READ_ONLY_TOOLS if it mutates "
+        f"nothing, otherwise to _WRITE_TOOLS."
+    )
+    names = list(sets)
+    for i, first in enumerate(names):
+        for second in names[i + 1:]:
+            both = sorted(sets[first] & sets[second])
+            assert not both, f"in both {first} and {second}: {both}"
+
+    # The reverse: a classified name that is not a tool is a rename or a
+    # deletion, and a stale grant is worth surfacing even though it grants
+    # access to nothing.
+    phantom = sorted(set().union(*sets.values()) - registered)
     assert not phantom, (
         f"these names are classified but are not tools: {phantom}. They were "
         f"renamed or removed — drop them, and check whatever replaced them got "
         f"classified."
     )
+
+
+def test_the_completeness_check_can_fail():
+    """A guard that cannot fail is indistinguishable from one that is broken
+    (rule 167). Drop a real tool from its set and the check must notice."""
+    from scribe.mcp.server import _READ_ONLY_TOOLS, _WRITE_TOOLS
+
+    registered = _registered_tool_names()
+    assert "rule_history" in _READ_ONLY_TOOLS
+    without = (_READ_ONLY_TOOLS - {"rule_history"}) | _WRITE_TOOLS
+    assert "rule_history" in registered - without