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

src/scribe/mcp/server.py

@@ -72,9 +72,10 @@ shared:true records are another user's suggestion, not settled practice.
 # what you hand to something you don't fully trust — a dashboard, a CI job, a
 # shared integration — and a boundary inferred from a naming convention grants
 # access to whatever a future author happens to call `get_*`. Enumerating it is
-# the point; staleness is the cost, and test_mcp_auth covers that (a read-shaped
+# the point; staleness is the cost, and test_mcp_auth covers that: EVERY
-# tool must appear here or in _DELIBERATELY_WRITE_SCOPED below, so adding one
+# registered tool must appear in exactly one of _READ_ONLY_TOOLS, _WRITE_TOOLS or
-# forces a decision instead of silently denying it).
+# _DELIBERATELY_WRITE_SCOPED below, so adding one forces a decision instead of
+# silently denying it — whatever the tool is called (#3191).
 #
 # Membership means "reads the operator's data and mutates none of it". Several
 # getters record a retrieval event via record_pulled; that is telemetry about
@@ -121,9 +122,51 @@ _READ_ONLY_TOOLS = frozenset({
     # is the write, and it is deliberately NOT here. Spelled out for
     # retrieval_telemetry's reason: `notes_due_for_verification` matches none of
     # the prefixes the completeness test derives from, so nothing would have
-    # prompted this decision. `rules_due_for_verification` is in the same
+    # prompted this decision.
-    # position and is NOT listed — see #3191.
     "notes_due_for_verification",
+    # Its rule twin and a rule's edit history (milestones 312 and 323). Both
+    # pure reads, and both sat unlisted — so a read key was refused them — for
+    # the same reason: no read prefix, back when the completeness test only
+    # looked at names that had one (#3191). rule_history records a pull the way
+    # the getters above do.
+    "rules_due_for_verification", "rule_history",
+})
+
+# Every tool that WRITES, by name. Nothing reads this set at runtime — a tool
+# absent from _READ_ONLY_TOOLS is already denied to a read key. It exists so the
+# classification is total: test_mcp_auth requires every registered tool to sit
+# in exactly one of the three sets, which is what makes forgetting impossible
+# rather than merely unlikely. Before #3191 the test only asked about tools whose
+# names looked like reads, and two reads with other names were denied for weeks.
+_WRITE_TOOLS = frozenset({
+    # notes, tasks, planning
+    "create_note", "update_note", "delete_note",
+    "create_task", "update_task", "delete_task", "add_task_log",
+    "create_records", "start_planning",
+    "create_milestone", "update_milestone", "delete_milestone",
+    "mark_note_verified",
+    # projects, Systems, repos
+    "create_project", "update_project", "delete_project", "decide_project_inception",
+    "create_system", "update_system", "delete_system", "map_system_to_canonical",
+    "bind_repo", "unbind_repo",
+    # snippets, processes, the shape ledger
+    "create_snippet", "update_snippet", "delete_snippet", "verify_snippet",
+    "merge_snippets", "unmerge_snippet",
+    "create_process", "update_process", "delete_process",
+    "classify_shapes", "classify_shapes_by_rule", "confirm_shape_proposals",
+    "refresh_pattern_coverage",
+    # design systems
+    "create_design_system", "update_design_system", "delete_design_system",
+    "create_design_token", "update_design_token", "delete_design_token",
+    "set_project_design_system",
+    # rules
+    "create_rulebook", "update_rulebook", "delete_rulebook",
+    "create_topic", "update_topic", "delete_topic",
+    "create_rule", "create_project_rule", "update_rule", "move_rule", "delete_rule",
+    "create_preference", "update_preference",
+    "relate_rules", "unrelate_rules", "mark_rule_verified",
+    # trash
+    "restore", "purge_trash",
 })
 
 # Read-SHAPED tools that must NOT be reachable with a read key — a getter that

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():
+def _registered_tool_names() -> set[str]:
-    """A read-shaped tool must be classified, not left to default-deny.
+    """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
+    ended up able to `get_note` and not `get_snippet` (#2496), and how
-    same table, while design systems were unreachable entirely (#2496). The
+    `rules_due_for_verification` and `rule_history` sat denied (#3191).
-    `find_duplicate_snippets` entry was the tell: someone classified the report
-    and missed the getters beside it.
 
-    This is the same shape as #2476 (record_pulled on three of four getters) —
+    The fix keeps the DECISION explicit and derives the CANDIDATES. Deriving
-    a hand-written enumeration that missed the members added after it. The fix
+    the decision would make a security boundary follow a naming convention, so
-    there and here is the same: derive the CANDIDATES, keep the DECISION
+    any future `get_*` grants itself access. This test used to derive
-    explicit. Deriving the decision itself would be worse than a stale list —
+    candidates from names too — tools starting `get_`, `list_`, `search`… — and
-    it would make a security boundary follow a naming convention, so any future
+    that narrowing is exactly what let the two #3191 reads through: neither
-    `get_*` grants itself access.
+    name looked like a read. So the candidates are now EVERY registered tool.
-
-    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.
     """
-    import ast
+    from scribe.mcp.server import (
-    import pathlib
+        _DELIBERATELY_WRITE_SCOPED, _READ_ONLY_TOOLS, _WRITE_TOOLS,
-
-    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."
     )
 
-    # The reverse: a name in either set that no longer exists is a rename or a
+    registered = _registered_tool_names()
-    # deletion, and a stale grant is worth surfacing even though it grants
+    assert len(registered) > 50, "found almost no tools — registration moved"
-    # 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`.
+    sets = {
-    all_tools = {
+        "_READ_ONLY_TOOLS": _READ_ONLY_TOOLS,
-        node.name
+        "_WRITE_TOOLS": _WRITE_TOOLS,
-        for path in tools_dir.glob("*.py") if path.name != "__init__.py"
+        "_DELIBERATELY_WRITE_SCOPED": _DELIBERATELY_WRITE_SCOPED,
-        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"
     }
-    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