fix(systems): a record can be filed under a System from whichever door wrote it (#4249)

A System tag is how `list_system_records` gathers an area's pile, so a
record that cannot be tagged is reachable by search and by nothing else.
`update_lesson` did not take `system_ids` while `create_lesson` did, which
made tagging available exactly once — at the moment of least information.
A lesson is usually written at the end of a piece of work, which is
precisely when that argument gets dropped, and after that the record could
never be filed at all.

Filling in the rest of the table found three more gaps, and the issue's own
generalisation was wrong. It read as "the REST door can do something the
MCP door cannot", on three instances in a row. In fact:

  * `create_preference` is the exact MIRROR of the lesson bug — update
    takes `system_ids`, create does not. The same capability missing from
    the opposite end of the same lifecycle.
  * `routes/notes.py` handled `system_ids` NOWHERE, while the MCP door
    handled both ends. That runs the opposite way round from the premise.
  * `create_process` / `update_process` took it at neither door, though a
    process is a note and has always been taggable in the data model.

The real pattern is that whichever door nobody exercised for a kind is the
one that never grew the parameter — which is a better statement of #4248
than the one recorded there, and is not something a reviewer reliably
notices, because each door is only ever read on its own.

Milestones are NOT a fifth gap. `RecordSystem.note_id` is a ForeignKey to
`notes.id` and milestones are their own table, so they cannot be tagged at
any door by construction. Pinned in the test so the next pass does not
re-open it.

So the guard is the point, not the four parameters. `update_lesson` alone
would have left the shape that produced it intact. The new test asserts the
TABLE — every kind taggable anywhere is taggable everywhere it is written —
and keys the registry-coverage check on the SIGNATURE rather than on a
grep, so a module that only names the argument in prose is not swept in and
no hand-kept skip list can go stale. A fifth kind fails there rather than
shipping half-wired, the same reasoning test_derived_mirror_generic_door.py
records for derived mirrors (#3734).

One inconsistency found and deliberately not changed here: for the same
operation `routes/tasks.py` scopes `set_record_systems` by the caller while
`routes/lessons.py` and `routes/snippets.py` scope it by the owner. The new
notes code follows tasks.py and says why in a comment (#47 — an editor-share
holder should tag from what they can see rather than inherit the owner's
reach). Recorded in #4249 rather than fixed as a drive-by.

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

tests/test_system_tagging_door_parity.py

@@ -0,0 +1,201 @@
+"""Both doors can file a record under a System, at create AND at update (#4249).
+
+WHAT THIS IS ABOUT. A System tag is how `list_system_records` gathers an
+area's whole pile. A record that cannot be tagged is reachable by search and
+by nothing else — so a missing `system_ids` parameter is not a cosmetic API
+gap, it is a record that quietly never joins the area it belongs to.
+
+THE DEFECT IS THE ASYMMETRY, NOT ANY ONE PARAMETER. When #4249 was filed it
+read as "the REST door can do something the MCP door cannot", on three
+instances in a row. Filling in the whole table showed that was the wrong
+generalisation:
+
+    kind          MCP create  MCP update  REST create  REST update
+    lesson            yes        NO           yes         yes
+    preference         NO        yes           -           -
+    note              yes        yes          NO          NO
+    process            NO         NO          NO          NO
+
+`preference` was the exact mirror of `lesson` — the same capability missing
+from the opposite end of the same lifecycle — and `note` ran the opposite way
+round from the issue's own premise. The real pattern is that **whichever door
+nobody exercised for a kind is the one that never grew the parameter**, and
+that is not a thing a human reviewer reliably notices, because each door is
+only ever read on its own.
+
+So this file does not assert that any particular function takes the argument.
+It asserts the SHAPE OF THE TABLE: every kind that can be System-tagged
+anywhere can be System-tagged everywhere it is written. A fifth kind added
+next month fails here rather than shipping half-wired — the same reasoning
+test_derived_mirror_generic_door.py records for derived mirrors (#3734), where
+testing one constant let the second kind arrive with none of the protection.
+
+NOT A GAP — MILESTONES. `RecordSystem.note_id` is a ForeignKey to `notes.id`
+and milestones live in their own table, so milestones cannot be System-tagged
+at any door, by construction. They are absent from the registry deliberately:
+they are not an empty cell, they are not a cell. Tagging them would be a
+schema change, not a parameter. The same is true of rulebooks and topics.
+"""
+from __future__ import annotations
+
+import ast
+import pathlib
+
+import pytest
+
+ROOT = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe"
+
+# (kind, mcp module, mcp create fn, mcp update fn, rest module, rest handlers)
+#
+# `None` for a REST module means that kind has no dedicated REST door — its web
+# writes go through another kind's route. It is NOT a licence to skip the MCP
+# half.
+REGISTRY = [
+    ("lesson",     "lessons.py",   "create_lesson",     "update_lesson",     "lessons.py"),
+    ("note",       "notes.py",     "create_note",       "update_note",       "notes.py"),
+    ("snippet",    "snippets.py",  "create_snippet",    "update_snippet",    "snippets.py"),
+    ("task",       "tasks.py",     "create_task",       "update_task",       "tasks.py"),
+    ("process",    "processes.py", "create_process",    "update_process",    None),
+    ("rule",       "rulebooks.py", "create_rule",       "update_rule",       "rulebooks.py"),
+    ("preference", "rulebooks.py", "create_preference", "update_preference", None),
+]
+
+PARAM = "system_ids"
+
+
+def _tree(path: pathlib.Path) -> ast.Module:
+    return ast.parse(path.read_text())
+
+
+def _params(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> set[str]:
+    a = fn.args
+    return {x.arg for x in a.posonlyargs + a.args + a.kwonlyargs}
+
+
+def _find(tree: ast.Module, name: str):
+    for node in tree.body:
+        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
+            return node
+    return None
+
+
+def _mcp(module: str) -> ast.Module:
+    return _tree(ROOT / "mcp" / "tools" / module)
+
+
+def _rest_source(module: str) -> str:
+    return (ROOT / "routes" / module).read_text()
+
+
+# ── the MCP door ──────────────────────────────────────────────────────────
+
+
+@pytest.mark.parametrize("kind,mcp_mod,create,update,_rest", REGISTRY)
+def test_the_mcp_door_takes_system_ids_at_both_ends(kind, mcp_mod, create, update, _rest):
+    """Create AND update. Tagging available only at create is tagging at the
+    moment of LEAST information — a lesson written at the end of a piece of
+    work is exactly when the argument gets dropped, and before #4249 there was
+    then no way to file it ever again."""
+    tree = _mcp(mcp_mod)
+    for fn_name in (create, update):
+        fn = _find(tree, fn_name)
+        assert fn is not None, f"{mcp_mod}::{fn_name} not found — registry is stale"
+        assert PARAM in _params(fn), (
+            f"{fn_name} does not accept {PARAM}. A {kind} written through this "
+            f"door cannot be filed under a System, so no area-scoped read will "
+            f"ever surface it."
+        )
+
+
+# ── the REST door ─────────────────────────────────────────────────────────
+
+
+@pytest.mark.parametrize("kind,_m,_c,_u,rest_mod", [r for r in REGISTRY if r[4]])
+def test_the_rest_door_handles_system_ids(kind, _m, _c, _u, rest_mod):
+    """Asserted on the module rather than per-handler: these routes read an
+    untyped `data` dict, so there is no signature to inspect and the honest
+    check is that the module reaches `set_record_systems` (or the rule
+    equivalent) at all.
+
+    `note` is the case this catches. routes/notes.py handled `system_ids`
+    NOWHERE — not in create, not in update — while the MCP door handled both,
+    which is the issue's own premise running backwards.
+    """
+    src = _rest_source(rest_mod)
+    assert PARAM in src, (
+        f"routes/{rest_mod} never reads {PARAM}, so the web UI cannot file a "
+        f"{kind} under a System even though the agent door can."
+    )
+    assert "set_record_systems" in src or "rule_detail" in src, (
+        f"routes/{rest_mod} mentions {PARAM} but never applies it"
+    )
+
+
+# ── the property, not the list ────────────────────────────────────────────
+
+
+def test_every_tool_module_that_tags_is_in_the_registry():
+    """The guard that makes the two tests above keep working.
+
+    Without this, the registry is a list somebody has to remember to extend,
+    and #4249 IS the failure of somebody remembering. Any MCP tool module that
+    ACCEPTS `system_ids` must be registered — so a new kind that wires up
+    tagging is dragged into the parity checks by its own first use of the
+    parameter, rather than being half-wired quietly.
+
+    Keyed on the SIGNATURE, not on the text. A grep would also hit
+    `projects.py`, `search.py` and `systems.py`, which only name the argument
+    in prose while explaining it to the reader — and the fix for that is a
+    structural check, not a hand-kept skip list that would itself go stale
+    (rule 167: a guard asserts on structure).
+    """
+    registered = {r[1] for r in REGISTRY}
+    tagging = set()
+    for p in (ROOT / "mcp" / "tools").glob("*.py"):
+        for node in ast.parse(p.read_text()).body:
+            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                if PARAM in _params(node):
+                    tagging.add(p.name)
+                    break
+    missing = tagging - registered
+    assert not missing, (
+        f"MCP tool modules handle {PARAM} but are not covered by the parity "
+        f"registry: {sorted(missing)}. Add them, or say in the registry why "
+        f"the kind is not System-taggable."
+    )
+
+
+def test_the_registry_names_functions_that_exist():
+    """A registry that drifts from the code is worse than none — it reports
+    green while checking nothing. Pinned separately so a rename fails loudly
+    here instead of silently skipping a kind."""
+    missing = []
+    for kind, mcp_mod, create, update, _rest in REGISTRY:
+        tree = _mcp(mcp_mod)
+        for fn_name in (create, update):
+            if _find(tree, fn_name) is None:
+                missing.append(f"{mcp_mod}::{fn_name}")
+    assert not missing, f"registry names functions that no longer exist: {missing}"
+
+
+def test_milestones_are_absent_on_purpose():
+    """The one 'gap' that is not a gap, pinned so it is not re-opened.
+
+    `RecordSystem.note_id` is a ForeignKey to `notes.id`. Milestones are their
+    own table, so no amount of parameter-adding would let them be tagged — and
+    a reader filling in this table WILL notice milestones missing and go
+    looking. This says: yes, deliberately, and here is the reason.
+    """
+    src = (ROOT / "models" / "system.py").read_text()
+    tree = ast.parse(src)
+    cls = next(
+        n for n in tree.body
+        if isinstance(n, ast.ClassDef) and n.name == "RecordSystem"
+    )
+    seg = ast.get_source_segment(src, cls) or ""
+    assert 'ForeignKey("notes.id"' in seg, (
+        "RecordSystem no longer keys on notes.id — if it grew a generic "
+        "record reference, milestones may now be taggable and this test, the "
+        "registry above and #4249's table all need revisiting."
+    )
+    assert "milestone" not in {r[0] for r in REGISTRY}