diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 4186d3d..0e507ac 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/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) diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index 23f12fc..1246406 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/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() diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 650d5fb..35a61bc 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/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( diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index e4ad0ad..5824d6b 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/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( diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index cd384d1..e7e19d6 100644 --- a/src/scribe/routes/lessons.py +++ b/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() diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index 58b3779..92ed94b 100644 --- a/src/scribe/routes/notes.py +++ b/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) diff --git a/src/scribe/routes/snippets.py b/src/scribe/routes/snippets.py index 5f9978f..ff28699 100644 --- a/src/scribe/routes/snippets.py +++ b/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) diff --git a/tests/test_system_tagging_door_parity.py b/tests/test_system_tagging_door_parity.py new file mode 100644 index 0000000..80e9966 --- /dev/null +++ b/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}