Merge pull request 'fix(systems): System tagging works from whichever door wrote the record, as whoever wrote it (#4249)' (#178) from dev into main

src/scribe/mcp/tools/lessons.py

@@ -229,6 +229,7 @@ async def update_lesson(
     insight: str = "",
     learned_from: list[int] | None = None,
     tags: list[str] | None = None,
+    system_ids: list[int] | None = None,
 ) -> dict:
     """Update a lesson. Empty fields are left unchanged.
 
@@ -249,6 +250,15 @@ async def update_lesson(
         learned_from: Replace the source ids. None leaves unchanged; pass the
             FULL list, including the ones already there.
         tags: Replace tags. None leaves unchanged.
+        system_ids: Replace the Systems this lesson is filed under. None
+            leaves unchanged; pass the FULL list, and `[]` to clear.
+
+            Here because `create_lesson` took it and this did not, so a lesson
+            written without one could never be filed afterwards — and the end
+            of a piece of work, when a lesson is usually written, is exactly
+            when that argument gets dropped (#4249). A System tag is how
+            `list_system_records` gathers an area's pile, so an untagged
+            lesson is reachable by search and by nothing else.
     """
     uid = current_user_id()
     note = await lessons_svc.update_lesson(
@@ -261,6 +271,12 @@ async def update_lesson(
     )
     if note is None:
         raise ValueError(f"lesson {lesson_id} not found")
+    # `is not None` rather than truthiness, so `[]` CLEARS the associations.
+    # `set_record_systems` is replace-semantics; treating [] as "no change"
+    # would make the one call that unfiles a lesson silently do nothing.
+    if system_ids is not None:
+        await systems_svc.set_record_systems(uid, lesson_id, system_ids)
+        note = await lessons_svc.get_lesson(uid, lesson_id) or note
     return _to_dict(note)
 
 

src/scribe/mcp/tools/processes.py

@@ -11,6 +11,7 @@ from scribe.services import access as access_svc
 from scribe.services import dedup as dedup_svc
 from scribe.services import knowledge as knowledge_svc
 from scribe.services import notes as notes_svc
+from scribe.services import systems as systems_svc
 from scribe.services import trash as trash_svc
 from scribe.services.note_usage import record_pulled
 
@@ -50,7 +51,8 @@ async def list_processes(
 
 
 async def create_process(
-    title: str, body: str, tags: list[str] | None = None, force: bool = False,
+    title: str, body: str, tags: list[str] | None = None,
+    system_ids: list[int] | None = None, force: bool = False,
 ) -> dict:
     """Create a stored process (a reusable saved prompt).
 
@@ -73,6 +75,9 @@ async def create_process(
         title: Process name, e.g. "Drift Audit" (required).
         body: The full prompt to run later (markdown). Required.
         tags: Plain-string tags, no # prefix.
+        system_ids: Systems (subsystems/areas) to file this process under, so
+            an area-scoped read finds it. A process is a note, so it has always
+            been taggable in the data model; neither door offered it (#4249).
         force: Bypass the near-duplicate gate. By default, if a title- or
             meaning-similar process already exists, creation is BLOCKED and the
             existing one's id is returned so you update it instead. Set true
@@ -99,6 +104,8 @@ async def create_process(
     note = await notes_svc.create_note(
         uid, title=title.strip(), body=body, note_type="process", tags=tags,
     )
+    if system_ids:
+        await systems_svc.set_record_systems(uid, note.id, system_ids)
     return note.to_dict()
 
 
@@ -153,10 +160,14 @@ async def get_process(name_or_id: str, project_id: int = 0) -> dict:
 
 
 async def update_process(process_id: int, title: str = "", body: str = "",
-                         tags: list[str] | None = None) -> dict:
+                         tags: list[str] | None = None,
+                         system_ids: list[int] | None = None) -> dict:
     """Update a stored process. Only provided fields change — empty title/body
     leave that field unchanged; pass tags to replace the tag set.
 
+    `system_ids` replaces the Systems this process is filed under: None leaves
+    them alone, a list (including `[]`) replaces them.
+
     Editing another user's process requires an editor or admin share from them; a
     read-only share is not enough and says so rather than claiming not-found.
     """
@@ -179,6 +190,12 @@ async def update_process(process_id: int, title: str = "", body: str = "",
         fields["tags"] = tags
     # As the owner — update_note is owner-scoped and the write is authorised above.
     updated = await notes_svc.update_note(note.user_id, process_id, **fields)
+    # The CALLER, not the owner. `update_note` above is owner-scoped because
+    # the service demands it; `set_record_systems` is not — it runs its own
+    # share-aware check and links only Systems the acting user can read, so
+    # passing the owner would bypass the check and borrow their reach (#47).
+    if system_ids is not None:
+        await systems_svc.set_record_systems(uid, process_id, system_ids)
     if updated is None:
         raise ValueError(f"process {process_id} not found")
     out = updated.to_dict()

src/scribe/mcp/tools/rulebooks.py

@@ -712,7 +712,8 @@ async def update_rule(
 async def create_preference(
     topic_id: int, title: str, statement: str, when_to_apply: str,
     arose_from_id: int, why: str = "", how_to_apply: str = "",
-    order_index: int = 0, force: bool = False,
+    order_index: int = 0, system_ids: list[int] | None = None,
+    force: bool = False,
 ) -> dict:
     """Record how the operator wants work done. No approval loop — write it.
 
@@ -777,6 +778,10 @@ async def create_preference(
         statement: How the operator wants it done, in their terms.
         when_to_apply: The moment it applies. Required; see above.
         arose_from_id: The task or note that taught this. Required; see above.
+        system_ids: Ids from list_canonical_systems — the global AREAS this
+            preference is about, which is what lets it reach a session working
+            in that area. `update_preference` took this and create did not, so
+            a preference could only be filed after the fact (#4249).
         force: Bypass the near-duplicate gate. For a genuinely distinct
             preference, not for one that is "mostly" different — a mostly
             different preference is an update.
@@ -804,7 +809,7 @@ async def create_preference(
         kind="preference", arose_from_id=arose_from_id,
         why=why, how_to_apply=how_to_apply, order_index=order_index,
     )
-    return await rulebooks_svc.rule_detail(uid, rule, None)
+    return await rulebooks_svc.rule_detail(uid, rule, system_ids)
 
 
 async def update_preference(

src/scribe/mcp/tools/snippets.py

@@ -483,7 +483,10 @@ async def update_snippet(
     if note is None:
         raise ValueError(f"snippet {snippet_id} not found")
     if system_ids is not None:
-        await systems_svc.set_record_systems(note.user_id, snippet_id, system_ids)
+        # The CALLER, not the owner (#4249). `update_snippet` above already
+        # raised PermissionError if this user may not write, so the tagging
+        # runs with the actor's own System visibility rather than the owner's.
+        await systems_svc.set_record_systems(uid, snippet_id, system_ids)
     data = snippets_svc.snippet_to_dict(note)
     data.update(await access_svc.describe_provenance(uid, note))
     await systems_tools.attach_systems(

src/scribe/routes/lessons.py

@@ -240,9 +240,13 @@ async def update_lesson_route(lesson_id: int):
     if updated is None:
         return not_found("Lesson")
     if data.get("system_ids") is not None:
-        await systems_svc.set_record_systems(
-            owner_uid, lesson_id, data["system_ids"]
-        )
+        # The CALLER, not owner_uid (#4249). `set_record_systems` runs its own
+        # `can_write_note` and links only Systems the acting user can read;
+        # handing it the owner makes that check trivially pass and filters by
+        # the owner's visibility instead. The caller's write permission is
+        # already established above, so this neither loosens nor tightens who
+        # may edit — it decides WHOSE reach the tagging uses (#47).
+        await systems_svc.set_record_systems(uid, lesson_id, data["system_ids"])
     out = lessons_svc.lesson_to_dict(updated)
     out["systems"] = [
         s.to_dict()

src/scribe/routes/notes.py

@@ -27,6 +27,7 @@ from scribe.services.notes import (
 from scribe.services.note_drafts import upsert_draft, get_draft, delete_draft
 from scribe.services import dedup as dedup_svc
 from scribe.services import supersession as supersession_svc
+from scribe.services import systems as systems_svc
 from scribe.services.note_usage import record_pulled
 
 from scribe.services.note_versions import list_versions, get_version
@@ -130,6 +131,12 @@ async def create_note_route():
             # 403, not 400: the request is well-formed and the caller simply
             # may not write the target. The note itself was created.
             return jsonify({"error": str(exc), "note": note.to_dict()}), 403
+    # #4249: this door could not file a note to a System at all, while the
+    # MCP door could — the reverse of the asymmetry the same issue records for
+    # lessons. The pattern is not that one door is richer; it is that the door
+    # nobody exercised for a kind is the one that never grew the parameter.
+    if data.get("system_ids") is not None:
+        await systems_svc.set_record_systems(uid, note.id, data["system_ids"])
     out = note.to_dict()
     await supersession_svc.attach_relations(uid, note.id, out)
     return jsonify(out), 201
@@ -289,6 +296,16 @@ async def update_note_route(note_id: int):
             await supersession_svc.set_supersedes(uid, note_id, data["supersedes"] or [])
         except PermissionError as exc:
             return jsonify({"error": str(exc)}), 403
+    # Set-semantics like the above: present-and-empty clears, absent leaves
+    # alone. Scoped by the CALLER rather than owner_uid, for the same reason
+    # the supersedes call is (#47) — `set_record_systems` links only Systems
+    # the acting user can read, and an editor-share holder should tag from
+    # what they can see rather than inherit the owner's reach. This matches
+    # routes/tasks.py; routes/lessons.py and routes/snippets.py pass the owner
+    # for the same operation, which is a real inconsistency, recorded in #4249
+    # rather than changed here.
+    if "system_ids" in data:
+        await systems_svc.set_record_systems(uid, note_id, data["system_ids"] or [])
     out = note.to_dict()
     await supersession_svc.attach_relations(uid, note_id, out)
     return jsonify(out)

src/scribe/routes/snippets.py

@@ -208,7 +208,8 @@ async def update_snippet_route(snippet_id: int):
     if updated is None:
         return not_found("Snippet")
     if data.get("system_ids") is not None:
-        await systems_svc.set_record_systems(owner_uid, snippet_id, data["system_ids"])
+        # The CALLER, not owner_uid — see routes/lessons.py for the why (#4249).
+        await systems_svc.set_record_systems(uid, snippet_id, data["system_ids"])
     out = snippets_svc.snippet_to_dict(updated)
     out["systems"] = [
         s.to_dict() for s in await systems_svc.list_record_systems(owner_uid, snippet_id)

tests/test_system_tagging_door_parity.py

@@ -0,0 +1,255 @@
+"""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_every_tagging_write_acts_as_the_caller():
+    """`set_record_systems` is handed the ACTING user, never the record's owner.
+
+    THE ARGUMENT. `set_record_systems` is not a dumb setter — it runs its own
+    `can_write_note` and then links only Systems the given user can READ.
+    Handing it `note.user_id` therefore does two things at once, both quiet:
+    the access check becomes trivially true (an owner can always write their
+    own record), and the System filter uses the owner's visibility instead of
+    the actor's. On a single-user install those are invisible. With a share
+    they are an editor acting with the owner's reach — the shape #47 exists to
+    prevent, and the same reasoning routes/notes.py already applies to
+    `set_supersedes`.
+
+    It is NOT a permission loosening either way: every call site establishes
+    the caller's write access first (a route's `can_write_note`, or a service
+    that raises PermissionError). What this decides is WHOSE reach the tagging
+    runs with, which is exactly the kind of difference that survives review
+    because each call site reads fine on its own.
+
+    Four of twenty sites passed the owner before #4249 — two routes, two MCP
+    tools, one of them written earlier in the very session that unified them.
+    That is the tell that this needs a guard rather than care.
+    """
+    offenders = []
+    for path in ROOT.rglob("*.py"):
+        src = path.read_text()
+        if "set_record_systems" not in src:
+            continue
+        tree = ast.parse(src)
+        for node in ast.walk(tree):
+            if not isinstance(node, ast.Call):
+                continue
+            fn = node.func
+            name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None)
+            if name != "set_record_systems" or not node.args:
+                continue
+            first = node.args[0]
+            # The acting user arrives as a bare local — `uid` in the tools and
+            # routes, `user_id` in the service's own signature. An attribute
+            # access (`note.user_id`, `target.user_id`) is a record's owner,
+            # and `owner_uid` is the same thing already unpacked.
+            ok = isinstance(first, ast.Name) and first.id in {"uid", "user_id"}
+            if not ok:
+                shown = ast.unparse(first) if hasattr(ast, "unparse") else "?"
+                rel = path.relative_to(ROOT.parent.parent)
+                offenders.append(f"{rel}:{node.lineno} passes {shown!r}")
+    assert not offenders, (
+        "set_record_systems must be called with the acting user, not the "
+        "record's owner — passing the owner bypasses its own access check and "
+        "borrows the owner's System visibility (#47, #4249):\n  "
+        + "\n  ".join(offenders)
+    )
+
+
+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}