From e2e1ea3667b13da1a2ad1803f79113f4de7d0134 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 16:02:16 -0400 Subject: [PATCH 1/2] fix(systems): a record can be filed under a System from whichever door wrote it (#4249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/lessons.py | 16 ++ src/scribe/mcp/tools/processes.py | 19 ++- src/scribe/mcp/tools/rulebooks.py | 9 +- src/scribe/routes/notes.py | 17 ++ tests/test_system_tagging_door_parity.py | 201 +++++++++++++++++++++++ 5 files changed, 258 insertions(+), 4 deletions(-) create mode 100644 tests/test_system_tagging_door_parity.py 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..b4d831c 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,10 @@ 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) + # Written as the OWNER, matching the update above: an editor-shared process + # keeps its owner's associations rather than sprouting a second set. + if system_ids is not None: + await systems_svc.set_record_systems(note.user_id, 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/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/tests/test_system_tagging_door_parity.py b/tests/test_system_tagging_door_parity.py new file mode 100644 index 0000000..8bb4fdd --- /dev/null +++ b/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} From 43ed7f4db749210f7a46eff2478b5cad4b88c990 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 16:09:41 -0400 Subject: [PATCH 2/2] fix(access): tag a record as the user doing it, not as its owner (#4249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `set_record_systems` is not a dumb setter. It runs its own `can_write_note` and then links only the Systems the given user can READ. Four of twenty call sites handed it the record's owner instead of the acting user, which did two quiet things at once: the access check became trivially true, since an owner can always write their own record, and the System filter used the owner's visibility rather than the actor's. On a single-user install neither is observable. With a share it is an editor acting with the owner's reach — the shape rule 47 exists to prevent, and the same reasoning routes/notes.py already spells out for `set_supersedes` two lines away. This is NOT a permission change. Every one of the four sites establishes the caller's write access first: routes/lessons.py and routes/snippets.py call `can_write_note(uid, …)`, mcp/tools/processes.py does the same, and mcp/tools/snippets.py reaches `set_record_systems` only after `update_snippet` has raised PermissionError if the caller may not write. So nobody gains or loses the ability to edit anything. What changes is whose reach the tagging runs with, which is exactly the kind of difference that survives review because every call site reads fine on its own. The four: routes/lessons.py:243 owner_uid -> uid routes/snippets.py:211 owner_uid -> uid mcp/tools/snippets.py:486 note.user_id -> uid mcp/tools/processes.py:196 note.user_id -> uid The last one was written earlier in this same session, an hour before the sweep that found it, with a comment confidently explaining why the owner was correct. That is the argument for the guard rather than for care: the unified stance was known and still got it wrong at the next opportunity. So the guard is the point again. `test_every_tagging_write_acts_as_the_caller` walks every `set_record_systems` call in src/ and asserts the first argument is a bare local named `uid` or `user_id` — an attribute access is a record's owner by construction. Verified against `git show HEAD:` as well as the working tree: clean now, four offenders on the code it replaces. Reads are deliberately untouched. `list_record_systems(owner_uid, …)` gates on reading the NOTE, which the caller can do anyway, so it returns the same list either way; it is a different operation and churning it would add noise without changing behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/processes.py | 8 ++-- src/scribe/mcp/tools/snippets.py | 5 ++- src/scribe/routes/lessons.py | 10 +++-- src/scribe/routes/snippets.py | 3 +- tests/test_system_tagging_door_parity.py | 54 ++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index b4d831c..1246406 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -190,10 +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) - # Written as the OWNER, matching the update above: an editor-shared process - # keeps its owner's associations rather than sprouting a second set. + # 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(note.user_id, process_id, system_ids) + 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/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/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 index 8bb4fdd..80e9966 100644 --- a/tests/test_system_tagging_door_parity.py +++ b/tests/test_system_tagging_door_parity.py @@ -178,6 +178,60 @@ def test_the_registry_names_functions_that_exist(): 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.