diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index d2128ed..3175676 100644 --- a/src/scribe/mcp/server.py +++ b/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 -# tool must appear here or in _DELIBERATELY_WRITE_SCOPED below, so adding one -# forces a decision instead of silently denying it). +# the point; staleness is the cost, and test_mcp_auth covers that: EVERY +# registered tool must appear in exactly one of _READ_ONLY_TOOLS, _WRITE_TOOLS or +# _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 - # position and is NOT listed — see #3191. + # prompted this decision. "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 diff --git a/tests/test_mcp_auth.py b/tests/test_mcp_auth.py index d038107..f8eca0a 100644 --- a/tests/test_mcp_auth.py +++ b/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