From 77bb3729a37f4592bddc34441702cb60ed55c180 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:13:17 -0400 Subject: [PATCH 1/8] refactor(tests): per-model fakes, FakeMCP and session mocks come from tests/helpers (#2825, milestone 296 area 1, batch 2) Second pass over the tests/ ledger after bbee0d0. fake_record(**attrs) is the one MagicMock-with-real-attributes builder (to_dict mirrors them; the note-2109 hazard documented once); fake_note/fake_task/fake_snippet/ fake_project/fake_milestone/fake_system/fake_rulebook/fake_topic/fake_rule carry each model's ordinary defaults on top of it, replacing 14 per-file factories (two rulebook trios in tool-vs-service wordings, _fake_task, _fake_ms, _fake_project, _plan_note, _fake_snippet, two _snippet adapters now one-liners over fake_snippet). FakeMCP replaces the five closure-over-a-list registrar fakes (+ _Recorder); loc() and design_token_stub() replace the paired _loc / _token / _T stand-ins; every hand-built async_session mock (9 helper defs and 14 inline copies) now starts from make_mock_session(). Call sites rewritten by AST with each file's former defaults made explicit, so behaviour is unchanged. Co-Authored-By: Claude Fable 5 --- tests/helpers.py | 149 +++++++++++++++++--- tests/test_api_keys.py | 9 +- tests/test_design_cascade.py | 82 +++++------ tests/test_design_stylesheet.py | 49 +++---- tests/test_integration_snippet_locations.py | 13 +- tests/test_mcp_tool_design_systems.py | 32 ++--- tests/test_mcp_tool_milestones.py | 28 ++-- tests/test_mcp_tool_planning.py | 20 +-- tests/test_mcp_tool_processes.py | 15 +- tests/test_mcp_tool_projects.py | 49 ++----- tests/test_mcp_tool_rulebooks.py | 88 ++++-------- tests/test_mcp_tool_snippets.py | 46 ++---- tests/test_mcp_tool_systems.py | 19 +-- tests/test_mcp_tool_tags.py | 9 +- tests/test_mcp_tool_tasks.py | 54 +++---- tests/test_mcp_tool_trash.py | 14 +- tests/test_notes_description_field.py | 9 +- tests/test_recurrence.py | 25 +--- tests/test_retrieval_scopes.py | 6 +- tests/test_routes_design_systems.py | 12 +- tests/test_services_db_maintenance.py | 5 +- tests/test_services_dedup.py | 10 +- tests/test_services_project_summaries.py | 5 +- tests/test_services_rulebooks.py | 66 +-------- tests/test_services_supersession.py | 9 +- tests/test_shared_provenance.py | 9 +- tests/test_shared_write_access.py | 19 +-- tests/test_snippet_merge_provenance.py | 20 +-- tests/test_snippet_unmerge.py | 31 ++-- tests/test_trash_filtering.py | 5 +- tests/test_version_pinning_pin_unpin.py | 9 +- tests/test_version_pinning_prune.py | 8 +- 32 files changed, 375 insertions(+), 549 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index b0711b4..862c781 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -6,6 +6,8 @@ them; a module imports what it needs with ``from tests.helpers import ...``. """ from __future__ import annotations +from datetime import datetime, timezone +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock @@ -48,30 +50,135 @@ async def ensure_user(session, username: str, role: str = "user"): return user -def fake_note(**attrs) -> MagicMock: - """A MagicMock note with REAL values on every attribute the product reads - to label, scope, or render a record. +def fake_record(**attrs) -> MagicMock: + """A MagicMock record with REAL values on the attributes named, and a + ``to_dict()`` that mirrors them. The hazard this exists for (note 2109): an auto-created MagicMock attribute - is truthy and has a repr. The injected menu reads ``is_task`` / - ``task_kind`` / ``note_type`` for its kind marker, ``user_id`` to decide - whether a line needs a "shared by …" attribution, ``data`` for a snippet's - language tag, and ``deleted_at`` to spot trash — on a bare MagicMock every - record renders as another user's trashed task with a mock repr for a - language. Defaults below are the ORDINARY state (own note, live, no - structured data); override what the test is about. - - ``to_dict()`` returns the same values as a plain dict, so a tool that - repackages ``note.to_dict()`` sees keys that agree with the attributes. + is truthy and has a repr — so a bare MagicMock handed to the product reads + as trashed, shared, a task, and owned by a MagicMock. Name every attribute + the code under test will read; the per-model ``fake_*`` builders below + carry the ordinary defaults so a call site states only what the test is + about. ``created_at`` / ``updated_at`` are set as attributes but kept out + of ``to_dict()`` (no test serialises them, and the real models isoformat + them). """ - values = { + n = MagicMock() + for key, value in attrs.items(): + setattr(n, key, value) + n.to_dict.return_value = { + k: v for k, v in attrs.items() if k not in ("created_at", "updated_at") + } + return n + + +def _with_defaults(defaults: dict, attrs: dict) -> MagicMock: + values = dict(defaults) + values.update(attrs) + return fake_record(**values) + + +def _now(): + return datetime.now(timezone.utc) + + +def fake_note(**attrs) -> MagicMock: + """A stand-in Note: own (user_id=7, the caller `_bind_user` binds), live, + not a task, no structured data. The injected menu reads is_task / + task_kind / note_type for its kind marker, user_id for the "shared by …" + attribution, data for a snippet's language, deleted_at for trash.""" + return _with_defaults({ "id": 1, "title": "t", "body": "", "tags": [], "user_id": 7, "note_type": "note", "is_task": False, "task_kind": "work", "data": None, "deleted_at": None, - } - values.update(attrs) - n = MagicMock() - for key, value in values.items(): - setattr(n, key, value) - n.to_dict.return_value = dict(values) - return n + }, attrs) + + +def fake_task(**attrs) -> MagicMock: + """A stand-in task note — get_task reads parent_id, deleted_at, user_id.""" + return _with_defaults({ + "id": 1, "title": "t", "body": "", "status": "todo", "priority": "none", + "tags": [], "parent_id": None, "project_id": None, "is_task": True, + "task_kind": "work", "user_id": 7, "deleted_at": None, + }, attrs) + + +def fake_snippet(**attrs) -> MagicMock: + """A stand-in snippet note. ``data`` is explicitly None: snippet_fields + prefers `data` when truthy, and a MagicMock is truthy.""" + return _with_defaults({ + "id": 1, "title": "debounce — rate-limit a callback", + "body": "```js\nreturn 1\n```\n", "tags": ["js", "snippet"], + "note_type": "snippet", "is_task": False, "task_kind": "work", + "user_id": 7, "data": None, "deleted_at": None, + }, attrs) + + +def fake_project(**attrs) -> MagicMock: + """design_system_id is explicit: a truthy auto-attribute would route every + project through the design-system branch and out to a real database.""" + return _with_defaults({ + "id": 1, "title": "P", "description": "", "goal": "", "status": "active", + "color": None, "design_system_id": None, "user_id": 7, + }, attrs) + + +def fake_milestone(**attrs) -> MagicMock: + return _with_defaults({ + "id": 1, "project_id": 1, "title": "MS", "description": None, + "status": "active", "order_index": 0, + }, attrs) + + +def fake_system(**attrs) -> MagicMock: + return _with_defaults({"id": 1, "name": "Reader", "project_id": 5}, attrs) + + +def fake_rulebook(**attrs) -> MagicMock: + return _with_defaults({ + "id": 1, "owner_user_id": 7, "title": "FabledSword family", + "description": "", "created_at": _now(), "updated_at": _now(), + }, attrs) + + +def fake_topic(**attrs) -> MagicMock: + return _with_defaults({ + "id": 10, "rulebook_id": 1, "title": "git-workflow", "description": "", + "order_index": 0, "created_at": _now(), "updated_at": _now(), + }, attrs) + + +def fake_rule(**attrs) -> MagicMock: + return _with_defaults({ + "id": 1, "topic_id": 10, "title": "dev is home", + "statement": "Work directly on dev", "why": "", "how_to_apply": "", + "order_index": 0, "created_at": _now(), "updated_at": _now(), + }, attrs) + + +class FakeMCP: + """Stand-in for the FastMCP server a tool module's ``register(mcp)`` is + handed: records the ``name=`` of every ``@mcp.tool(...)`` registration in + ``names`` and leaves the function untouched, so a test can assert which + tools a module exposes.""" + + def __init__(self) -> None: + self.names: list[str] = [] + + def tool(self, name=None): + self.names.append(name) + return lambda fn: fn + + +def loc(path: str = "", repo: str = "", symbol: str = "") -> dict: + """One snippet location, in the shape the record stores.""" + return {"repo": repo, "path": path, "symbol": symbol} + + +def design_token_stub(name, value_by_mode, group_name=None, purpose=None, + order_index=0, supersedes=None) -> SimpleNamespace: + """A design-token row as the cascade / stylesheet code reads it.""" + return SimpleNamespace( + name=name, value_by_mode=value_by_mode, group_name=group_name, + purpose=purpose, order_index=order_index, supersedes=supersedes or [], + ) diff --git a/tests/test_api_keys.py b/tests/test_api_keys.py index dbe165b..7a0e312 100644 --- a/tests/test_api_keys.py +++ b/tests/test_api_keys.py @@ -13,6 +13,7 @@ from scribe.services.api_keys import ( revoke_api_key, lookup_key, ) +from tests.helpers import make_mock_session def test_generate_key_format(): @@ -45,9 +46,7 @@ async def test_create_api_key_returns_full_key(): mock_key_obj.to_dict.return_value = {"id": 1, "name": "test", "scope": "read", "key_prefix": "fmcp_xxx"} with patch("scribe.services.api_keys.async_session") as mock_session_ctx: - mock_session = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) + mock_session = make_mock_session() mock_session.add = MagicMock() mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock(side_effect=lambda obj: None) @@ -67,9 +66,7 @@ async def test_create_api_key_returns_full_key(): @pytest.mark.asyncio async def test_lookup_key_returns_none_for_unknown(): with patch("scribe.services.api_keys.async_session") as mock_session_ctx: - mock_session = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = None mock_session.execute = AsyncMock(return_value=mock_result) diff --git a/tests/test_design_cascade.py b/tests/test_design_cascade.py index 9083c33..167c1e8 100644 --- a/tests/test_design_cascade.py +++ b/tests/test_design_cascade.py @@ -7,6 +7,7 @@ own import-free module — see services/design_cascade.py. from types import SimpleNamespace from scribe.services.design_cascade import ancestry, resolve_tokens, would_cycle +from tests.helpers import design_token_stub # --- ancestry --------------------------------------------------------------- @@ -102,14 +103,6 @@ def test_the_guard_survives_a_hierarchy_that_is_already_corrupt(): # because resolve_tokens is pure and duck-typed — which is the whole reason it # lives here rather than inside the service. -def _token(name, value_by_mode, group_name=None, purpose=None, order_index=0, - supersedes=None): - return SimpleNamespace( - name=name, value_by_mode=value_by_mode, group_name=group_name, - purpose=purpose, order_index=order_index, supersedes=supersedes or [], - ) - - # A family (1) and an app inheriting from it (2) — the shape the model exists for. FAMILY, APP = 1, 2 PARENTS = {FAMILY: None, APP: FAMILY} @@ -124,7 +117,7 @@ def test_a_system_with_no_tokens_of_its_own_inherits_the_whole_family_set(): and the state every app system starts in.""" resolved = resolve_tokens( APP, PARENTS, - {FAMILY: [_token("--fs-obsidian", {"base": "#14171a"})], APP: []}, + {FAMILY: [design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"})], APP: []}, ) assert [t.name for t in resolved] == ["--fs-obsidian"] assert resolved[0].value_by_mode == {"base": "#14171a"} @@ -138,8 +131,8 @@ def test_the_deepest_system_wins_and_says_what_it_overrode(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], - APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + FAMILY: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#6b2118"})], + APP: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#5b4a8a"})], }, )) accent = resolved["--fs-accent"] @@ -158,8 +151,8 @@ def test_overriding_one_mode_leaves_the_others_inherited(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-accent", {"base": "#34a877", "dark": "#34a877"})], - APP: [_token("--fs-accent", {"base": "#15803d"})], + FAMILY: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#34a877", "dark": "#34a877"})], + APP: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#15803d"})], }, )) accent = resolved["--fs-accent"] @@ -172,7 +165,7 @@ def test_a_token_only_the_app_defines_is_not_an_override(): labelled both "overridden here" would misdescribe the first.""" resolved = _by_name(resolve_tokens( APP, PARENTS, - {FAMILY: [], APP: [_token("--fs-editor-caret", {"base": "#5b4a8a"})]}, + {FAMILY: [], APP: [design_token_stub(name="--fs-editor-caret", value_by_mode={"base": "#5b4a8a"})]}, )) caret = resolved["--fs-editor-caret"] assert caret.origin_by_mode == {"base": APP} @@ -183,8 +176,8 @@ def test_is_overridden_in_is_true_only_for_the_system_that_shadowed(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], - APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + FAMILY: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#6b2118"})], + APP: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#5b4a8a"})], }, )) accent = resolved["--fs-accent"] @@ -198,9 +191,9 @@ def test_three_levels_stack_nearest_first(): resolved = _by_name(resolve_tokens( 3, parents, { - 1: [_token("--fs-bg", {"base": "a"})], - 2: [_token("--fs-bg", {"base": "b"})], - 3: [_token("--fs-bg", {"base": "c"})], + 1: [design_token_stub(name="--fs-bg", value_by_mode={"base": "a"})], + 2: [design_token_stub(name="--fs-bg", value_by_mode={"base": "b"})], + 3: [design_token_stub(name="--fs-bg", value_by_mode={"base": "c"})], }, )) bg = resolved["--fs-bg"] @@ -214,8 +207,8 @@ def test_resolving_the_family_itself_ignores_its_children(): resolved = _by_name(resolve_tokens( FAMILY, PARENTS, { - FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], - APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + FAMILY: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#6b2118"})], + APP: [design_token_stub(name="--fs-accent", value_by_mode={"base": "#5b4a8a"})], }, )) assert resolved["--fs-accent"].value_by_mode == {"base": "#6b2118"} @@ -226,7 +219,7 @@ def test_value_for_falls_back_to_the_base_mode(): "dark" must yield that rather than nothing — the read rule the storage shape implies.""" resolved = _by_name(resolve_tokens( - FAMILY, PARENTS, {FAMILY: [_token("--fs-radius-md", {"base": "8px"})]}, + FAMILY, PARENTS, {FAMILY: [design_token_stub(name="--fs-radius-md", value_by_mode={"base": "8px"})]}, )) radius = resolved["--fs-radius-md"] assert radius.value_for("dark") == "8px" @@ -236,7 +229,7 @@ def test_value_for_falls_back_to_the_base_mode(): def test_value_for_prefers_an_explicit_mode_over_the_fallback(): resolved = _by_name(resolve_tokens( FAMILY, PARENTS, - {FAMILY: [_token("--fs-bg", {"base": "#f7f5ef", "dark": "#14171a"})]}, + {FAMILY: [design_token_stub(name="--fs-bg", value_by_mode={"base": "#f7f5ef", "dark": "#14171a"})]}, )) assert resolved["--fs-bg"].value_for("dark") == "#14171a" @@ -248,11 +241,8 @@ def test_metadata_is_inherited_when_the_override_leaves_it_blank(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token( - "--fs-obsidian", {"base": "#14171a"}, - group_name="surface", purpose="page bg, deepest surface", - )], - APP: [_token("--fs-obsidian", {"base": "#101317"})], + FAMILY: [design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"}, group_name="surface", purpose="page bg, deepest surface")], + APP: [design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#101317"})], }, )) obsidian = resolved["--fs-obsidian"] @@ -265,8 +255,8 @@ def test_an_override_that_states_metadata_wins_it_too(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-x", {"base": "a"}, purpose="family says")], - APP: [_token("--fs-x", {"base": "b"}, purpose="app says")], + FAMILY: [design_token_stub(name="--fs-x", value_by_mode={"base": "a"}, purpose="family says")], + APP: [design_token_stub(name="--fs-x", value_by_mode={"base": "b"}, purpose="app says")], }, )) assert resolved["--fs-x"].purpose == "app says" @@ -279,8 +269,8 @@ def test_an_override_at_default_order_keeps_the_familys_position(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-x", {"base": "a"}, order_index=7)], - APP: [_token("--fs-x", {"base": "b"})], + FAMILY: [design_token_stub(name="--fs-x", value_by_mode={"base": "a"}, order_index=7)], + APP: [design_token_stub(name="--fs-x", value_by_mode={"base": "b"})], }, )) assert resolved["--fs-x"].order_index == 7 @@ -290,10 +280,10 @@ def test_the_effective_set_is_ordered_by_group_then_position_with_ungrouped_last resolved = resolve_tokens( FAMILY, PARENTS, {FAMILY: [ - _token("--fs-z", {"base": "1"}), # ungrouped - _token("--fs-b", {"base": "2"}, group_name="text", order_index=1), - _token("--fs-a", {"base": "3"}, group_name="surface", order_index=2), - _token("--fs-c", {"base": "4"}, group_name="surface", order_index=1), + design_token_stub(name="--fs-z", value_by_mode={"base": "1"}), # ungrouped + design_token_stub(name="--fs-b", value_by_mode={"base": "2"}, group_name="text", order_index=1), + design_token_stub(name="--fs-a", value_by_mode={"base": "3"}, group_name="surface", order_index=2), + design_token_stub(name="--fs-c", value_by_mode={"base": "4"}, group_name="surface", order_index=1), ]}, ) assert [t.name for t in resolved] == ["--fs-c", "--fs-a", "--fs-b", "--fs-z"] @@ -306,7 +296,7 @@ def test_resolution_terminates_on_a_corrupt_hierarchy(): parents = {1: 2, 2: 1} resolved = _by_name(resolve_tokens( 1, parents, - {1: [_token("--fs-a", {"base": "one"})], 2: [_token("--fs-b", {"base": "two"})]}, + {1: [design_token_stub(name="--fs-a", value_by_mode={"base": "one"})], 2: [design_token_stub(name="--fs-b", value_by_mode={"base": "two"})]}, )) assert set(resolved) == {"--fs-a", "--fs-b"} # Each system contributes exactly once, not endlessly. @@ -326,8 +316,8 @@ def test_supersedes_is_inherited_when_the_override_is_silent_about_it(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-text", {"base": "#e8e4d8"}, supersedes=["#fff", "#ffffff"])], - APP: [_token("--fs-text", {"base": "#f0ece0"})], + FAMILY: [design_token_stub(name="--fs-text", value_by_mode={"base": "#e8e4d8"}, supersedes=["#fff", "#ffffff"])], + APP: [design_token_stub(name="--fs-text", value_by_mode={"base": "#f0ece0"})], }, )) text = resolved["--fs-text"] @@ -341,8 +331,8 @@ def test_an_override_that_states_its_own_supersedes_replaces_the_list(): resolved = _by_name(resolve_tokens( APP, PARENTS, { - FAMILY: [_token("--fs-text", {"base": "a"}, supersedes=["#fff", "#ffffff"])], - APP: [_token("--fs-text", {"base": "b"}, supersedes=["#fff"])], + FAMILY: [design_token_stub(name="--fs-text", value_by_mode={"base": "a"}, supersedes=["#fff", "#ffffff"])], + APP: [design_token_stub(name="--fs-text", value_by_mode={"base": "b"}, supersedes=["#fff"])], }, )) assert resolved["--fs-text"].supersedes == ("#fff",) @@ -352,7 +342,7 @@ def test_a_token_that_supersedes_nothing_resolves_to_an_empty_tuple(): """Most tokens replace nothing. That has to be an empty sequence rather than None, so no caller has to test for two kinds of nothing.""" resolved = _by_name(resolve_tokens( - FAMILY, PARENTS, {FAMILY: [_token("--fs-radius-md", {"base": "8px"})]}, + FAMILY, PARENTS, {FAMILY: [design_token_stub(name="--fs-radius-md", value_by_mode={"base": "8px"})]}, )) assert resolved["--fs-radius-md"].supersedes == () @@ -360,7 +350,7 @@ def test_a_token_that_supersedes_nothing_resolves_to_an_empty_tuple(): def test_supersedes_survives_serialisation_as_a_list(): resolved = _by_name(resolve_tokens( FAMILY, PARENTS, - {FAMILY: [_token("--fs-text", {"base": "#e8e4d8"}, supersedes=["#fff"])]}, + {FAMILY: [design_token_stub(name="--fs-text", value_by_mode={"base": "#e8e4d8"}, supersedes=["#fff"])]}, )) assert resolved["--fs-text"].to_dict()["supersedes"] == ["#fff"] @@ -372,7 +362,7 @@ def test_the_superseded_literal_need_not_match_the_tokens_own_value(): it was turned around.""" resolved = _by_name(resolve_tokens( FAMILY, PARENTS, - {FAMILY: [_token("--fs-text", {"base": "#e8e4d8"}, supersedes=["#fff"])]}, + {FAMILY: [design_token_stub(name="--fs-text", value_by_mode={"base": "#e8e4d8"}, supersedes=["#fff"])]}, )) text = resolved["--fs-text"] assert text.value_by_mode["base"] not in text.supersedes @@ -404,7 +394,7 @@ def test_rationale_cascades_like_purpose_and_is_a_different_question(): rationale="equals Moss, aligned by design", order_index=0, supersedes=[], )], - APP: [_token("--fs-success", {"base": "#3f5236"})], + APP: [design_token_stub(name="--fs-success", value_by_mode={"base": "#3f5236"})], }, )) token = resolved["--fs-success"] @@ -415,6 +405,6 @@ def test_rationale_cascades_like_purpose_and_is_a_different_question(): def test_a_token_without_a_rationale_resolves_to_none(): resolved = _by_name(resolve_tokens( - FAMILY, PARENTS, {FAMILY: [_token("--fs-x", {"base": "1px"})]}, + FAMILY, PARENTS, {FAMILY: [design_token_stub(name="--fs-x", value_by_mode={"base": "1px"})]}, )) assert resolved["--fs-x"].rationale is None diff --git a/tests/test_design_stylesheet.py b/tests/test_design_stylesheet.py index 245fc71..7f40cb7 100644 --- a/tests/test_design_stylesheet.py +++ b/tests/test_design_stylesheet.py @@ -17,13 +17,7 @@ from scribe.services.design_stylesheet import ( safe_value, selector_for_mode, ) - - -def _token(name, value_by_mode, group_name=None, purpose=None): - return SimpleNamespace( - name=name, value_by_mode=value_by_mode, - group_name=group_name, purpose=purpose, - ) +from tests.helpers import design_token_stub # --- safety ----------------------------------------------------------------- @@ -66,7 +60,7 @@ def test_a_rejected_value_is_dropped_not_cleaned_up(): """Rejecting beats stripping. A partially-sanitised value is one the operator never wrote, and the sheet's entire claim is that it IS the record — quietly rendering a different colour would break that claim invisibly.""" - css = render_stylesheet([_token("--fs-x", {"base": "red; } body { color: blue"})]) + css = render_stylesheet([design_token_stub(name="--fs-x", value_by_mode={"base": "red; } body { color: blue"})]) assert "body" not in css assert "value rejected" in css @@ -85,7 +79,7 @@ def test_a_malformed_token_name_is_dropped(): assert not is_valid_token_name("color: red") assert not is_valid_token_name("fs-obsidian") # no leading -- - css = render_stylesheet([_token("--bad name", {"base": "red"})]) + css = render_stylesheet([design_token_stub(name="--bad name", value_by_mode={"base": "red"})]) assert "bad name" not in css @@ -100,8 +94,8 @@ def test_the_sheet_declares_properties_and_styles_no_elements(): handful of values once per element and grow with the UI. Purpose tokens are stated once and reused; components are snippets that reference them.""" css = render_stylesheet([ - _token("--fs-obsidian", {"base": "#14171a"}, group_name="surface"), - _token("--fs-moss", {"base": "#4a5d3f"}, group_name="action"), + design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"}, group_name="surface"), + design_token_stub(name="--fs-moss", value_by_mode={"base": "#4a5d3f"}, group_name="action"), ]) assert "--fs-obsidian: #14171a;" in css # No element or class rules — the sheet has exactly one block here, and @@ -117,7 +111,7 @@ def test_the_sheet_declares_properties_and_styles_no_elements(): def test_the_header_says_what_the_sheet_is_for(): """A generated file with no explanation gets hand-edited, and then it has diverged from the record it claims to be.""" - css = render_stylesheet([_token("--fs-x", {"base": "1px"})], title="FabledSword") + css = render_stylesheet([design_token_stub(name="--fs-x", value_by_mode={"base": "1px"})], title="FabledSword") assert "FabledSword" in css assert "Generated" in css assert "snippets" in css @@ -129,7 +123,7 @@ def test_base_goes_on_the_root_selector_and_other_modes_layer_over_it(): """Matches the convention already in the codebase, and the one-way scoping #251 recorded: light on `:root`, dark layered on an attribute selector.""" css = render_stylesheet([ - _token("--fs-bg", {"base": "#f5f1e8", "dark": "#14171a"}), + design_token_stub(name="--fs-bg", value_by_mode={"base": "#f5f1e8", "dark": "#14171a"}), ]) assert ":root {" in css assert '[data-theme="dark"] {' in css @@ -141,8 +135,8 @@ def test_a_mode_block_contains_only_what_that_mode_declares(): Repeating every token in every block would make the sheet claim each mode redefines the whole system.""" css = render_stylesheet([ - _token("--fs-bg", {"base": "#f5f1e8", "dark": "#14171a"}), - _token("--fs-radius-md", {"base": "8px"}), + design_token_stub(name="--fs-bg", value_by_mode={"base": "#f5f1e8", "dark": "#14171a"}), + design_token_stub(name="--fs-radius-md", value_by_mode={"base": "8px"}), ]) dark_block = css.split('[data-theme="dark"] {')[1] assert "--fs-bg" in dark_block @@ -153,7 +147,7 @@ def test_the_root_selector_is_caller_chosen(): """A container-scoped preview cannot use `:root`. A generator that hardcoded it could not serve the preview surface at all.""" css = render_stylesheet( - [_token("--fs-x", {"base": "1px"})], root_selector="[data-preview]" + [design_token_stub(name="--fs-x", value_by_mode={"base": "1px"})], root_selector="[data-preview]" ) assert "[data-preview] {" in css assert ":root {" not in css @@ -163,8 +157,8 @@ def test_the_root_selector_is_caller_chosen(): def test_tokens_are_grouped_by_purpose_with_the_group_named(): css = render_stylesheet([ - _token("--fs-obsidian", {"base": "#14171a"}, group_name="surface"), - _token("--fs-radius-md", {"base": "8px"}, group_name="radius"), + design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"}, group_name="surface"), + design_token_stub(name="--fs-radius-md", value_by_mode={"base": "8px"}, group_name="radius"), ]) assert "/* surface */" in css assert "/* radius */" in css @@ -174,8 +168,7 @@ def test_a_purpose_becomes_an_inline_comment_on_the_base_layer_only(): """Repeating the same prose in every mode block is noise: the token means the same thing in dark mode.""" css = render_stylesheet([ - _token("--fs-obsidian", {"base": "#14171a", "dark": "#000000"}, - purpose="page bg, deepest surface"), + design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a", "dark": "#000000"}, purpose="page bg, deepest surface"), ]) assert css.count("page bg, deepest surface") == 1 @@ -185,8 +178,8 @@ def test_a_declared_token_with_no_value_appears_as_a_comment_not_a_silence(): that finding where the reader is already looking; dropping it would make the sheet look complete.""" css = render_stylesheet([ - _token("--fs-obsidian", {"base": "#14171a"}), - _token("--fs-radius-sm", {}), + design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"}), + design_token_stub(name="--fs-radius-sm", value_by_mode={}), ]) assert "--fs-radius-sm" in css assert "no value set yet" in css @@ -210,9 +203,9 @@ def test_two_tokens_sharing_a_value_are_reported_not_refused(): purpose — "Success = Moss, by design" — so this reports and lets a human decide which it is.""" dupes = duplicate_values([ - _token("--fs-moss", {"base": "#4A5D3F"}), - _token("--fs-success", {"base": "#4a5d3f"}), - _token("--fs-obsidian", {"base": "#14171a"}), + design_token_stub(name="--fs-moss", value_by_mode={"base": "#4A5D3F"}), + design_token_stub(name="--fs-success", value_by_mode={"base": "#4a5d3f"}), + design_token_stub(name="--fs-obsidian", value_by_mode={"base": "#14171a"}), ]) assert dupes == {"#4a5d3f": ["--fs-moss", "--fs-success"]} @@ -221,15 +214,15 @@ def test_tokens_that_agree_in_one_mode_but_differ_in_another_are_not_duplicates( """A near-miss is a different, weaker finding, and calling it a duplicate would send someone to merge two tokens that genuinely diverge.""" assert duplicate_values([ - _token("--fs-a", {"base": "#fff", "dark": "#000"}), - _token("--fs-b", {"base": "#fff", "dark": "#111"}), + design_token_stub(name="--fs-a", value_by_mode={"base": "#fff", "dark": "#000"}), + design_token_stub(name="--fs-b", value_by_mode={"base": "#fff", "dark": "#111"}), ]) == {"#fff": ["--fs-a", "--fs-b"]} def test_valueless_tokens_never_count_as_duplicates_of_each_other(): """Otherwise every unfilled token would collide with every other one and the report would be nothing but noise on a fresh import.""" - assert duplicate_values([_token("--fs-a", {}), _token("--fs-b", {})]) == {} + assert duplicate_values([design_token_stub(name="--fs-a", value_by_mode={}), design_token_stub(name="--fs-b", value_by_mode={})]) == {} # --- reading the sheet from the other side ---------------------------------- diff --git a/tests/test_integration_snippet_locations.py b/tests/test_integration_snippet_locations.py index 9db9f29..7f5a363 100644 --- a/tests/test_integration_snippet_locations.py +++ b/tests/test_integration_snippet_locations.py @@ -31,14 +31,11 @@ from scribe.services.snippets import ( list_snippets, snippet_fields, ) +from tests.helpers import loc pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] -def _loc(repo="", path="", symbol=""): - return {"repo": repo, "path": path, "symbol": symbol} - - @pytest_asyncio.fixture async def seeded(): """A user with four snippets covering the cases the filter has to separate. @@ -60,13 +57,13 @@ async def seeded(): data=compose_data(name=name, language="python", locations=locations), ) - nested = _snippet("nested", [_loc("Scribe", "frontend/src/lib/x.ts", "helper")]) - sibling = _snippet("sibling", [_loc("Scribe", "frontend/srcmap.ts", "other")]) + nested = _snippet("nested", [loc(repo="Scribe", path="frontend/src/lib/x.ts", symbol="helper")]) + sibling = _snippet("sibling", [loc(repo="Scribe", path="frontend/srcmap.ts", symbol="other")]) # Two locations, deliberately crossed: repo Scribe at src/a.py and repo # Portal at src/b.py. repo=Scribe + path=src/b.py must NOT match it. multi = _snippet( "multi", - [_loc("Scribe", "src/a.py", "alpha"), _loc("Portal", "src/b.py", "beta")], + [loc(repo="Scribe", path="src/a.py", symbol="alpha"), loc(repo="Portal", path="src/b.py", symbol="beta")], ) # No structured location at all — must never satisfy a location filter, # and must not error the query either. @@ -242,7 +239,7 @@ async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded): defaults to none_as_null=False) — a different state, covered by the next test. """ user_id, _ids = seeded - locations = [_loc("Legacy", "old/path/y.py", "legacy_helper")] + locations = [loc(repo="Legacy", path="old/path/y.py", symbol="legacy_helper")] async with async_session() as s: old = Note( user_id=user_id, diff --git a/tests/test_mcp_tool_design_systems.py b/tests/test_mcp_tool_design_systems.py index ffc61ff..d679470 100644 --- a/tests/test_mcp_tool_design_systems.py +++ b/tests/test_mcp_tool_design_systems.py @@ -5,26 +5,23 @@ calling convention meets the service's: an agent cannot omit an argument, so "leave unchanged", "clear" and "set" have to be encoded in the value. Getting that mapping wrong is silent — the call succeeds and changes the wrong thing. """ -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest from scribe.services.design_systems import DesignSystemCycle +from tests.helpers import design_token_stub, fake_record pytestmark = pytest.mark.usefixtures("_bind_user") -def _fake_system(): - s = MagicMock() - s.to_dict.return_value = {"id": 1, "title": "FabledSword", "parent_id": None} - return s +def _fake_design_system(): + return fake_record(id=1, title="FabledSword", parent_id=None) def _fake_token(): - t = MagicMock() - t.to_dict.return_value = {"id": 9, "name": "--fs-obsidian"} - return t + return fake_record(id=9, name="--fs-obsidian") # --- create ----------------------------------------------------------------- @@ -35,7 +32,7 @@ async def test_creating_without_a_parent_passes_none_not_zero(): system id — there is no system 0, so the create would fail an ACL check for a record that cannot exist.""" with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: - svc.create_design_system = AsyncMock(return_value=_fake_system()) + svc.create_design_system = AsyncMock(return_value=_fake_design_system()) from scribe.mcp.tools.design_systems import create_design_system await create_design_system(title="FabledSword") assert svc.create_design_system.await_args.kwargs["parent_id"] is None @@ -44,7 +41,7 @@ async def test_creating_without_a_parent_passes_none_not_zero(): @pytest.mark.asyncio async def test_creating_with_a_parent_passes_it_through(): with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: - svc.create_design_system = AsyncMock(return_value=_fake_system()) + svc.create_design_system = AsyncMock(return_value=_fake_design_system()) from scribe.mcp.tools.design_systems import create_design_system await create_design_system(title="Scribe", parent_id=4) assert svc.create_design_system.await_args.kwargs["parent_id"] == 4 @@ -65,7 +62,7 @@ async def test_create_raises_when_the_parent_is_not_writable(): async def test_update_with_parent_id_zero_leaves_the_parent_alone(): """The common case — renaming a system must not silently re-root it.""" with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: - svc.update_design_system = AsyncMock(return_value=_fake_system()) + svc.update_design_system = AsyncMock(return_value=_fake_design_system()) from scribe.mcp.tools.design_systems import update_design_system await update_design_system(design_system_id=1, title="Renamed") fields = svc.update_design_system.await_args.kwargs @@ -79,7 +76,7 @@ async def test_update_with_parent_id_minus_one_clears_it(): None, which is the value the service reads as "become a root" — where omitting the key means "leave alone".""" with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: - svc.update_design_system = AsyncMock(return_value=_fake_system()) + svc.update_design_system = AsyncMock(return_value=_fake_design_system()) from scribe.mcp.tools.design_systems import update_design_system await update_design_system(design_system_id=1, parent_id=-1) assert svc.update_design_system.await_args.kwargs["parent_id"] is None @@ -88,7 +85,7 @@ async def test_update_with_parent_id_minus_one_clears_it(): @pytest.mark.asyncio async def test_update_with_a_positive_parent_id_sets_it(): with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: - svc.update_design_system = AsyncMock(return_value=_fake_system()) + svc.update_design_system = AsyncMock(return_value=_fake_design_system()) from scribe.mcp.tools.design_systems import update_design_system await update_design_system(design_system_id=1, parent_id=4) assert svc.update_design_system.await_args.kwargs["parent_id"] == 4 @@ -161,16 +158,11 @@ async def test_resolve_returns_serialised_tokens_with_their_provenance(): resolution was built to preserve.""" from scribe.services.design_cascade import resolve_tokens - class _T: - def __init__(self, name, value_by_mode): - self.name, self.value_by_mode = name, value_by_mode - self.group_name = self.purpose = None - self.order_index = 0 resolved = resolve_tokens( 2, {1: None, 2: 1}, - {1: [_T("--fs-accent", {"base": "#6b2118"})], - 2: [_T("--fs-accent", {"base": "#5b4a8a"})]}, + {1: [design_token_stub("--fs-accent", {"base": "#6b2118"})], + 2: [design_token_stub("--fs-accent", {"base": "#5b4a8a"})]}, ) with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: svc.resolve_design_system = AsyncMock(return_value=resolved) diff --git a/tests/test_mcp_tool_milestones.py b/tests/test_mcp_tool_milestones.py index f4c1aef..907d0f1 100644 --- a/tests/test_mcp_tool_milestones.py +++ b/tests/test_mcp_tool_milestones.py @@ -6,20 +6,12 @@ import pytest from scribe.mcp.tools.milestones import ( list_milestones, get_milestone, create_milestone, update_milestone, ) +from tests.helpers import fake_milestone pytestmark = pytest.mark.usefixtures("_bind_user") -def _fake_ms(**overrides) -> MagicMock: - m = MagicMock() - base = {"id": 1, "project_id": 1, "title": "MS", "description": None, - "status": "active", "order_index": 0} - base.update(overrides) - m.to_dict.return_value = base - return m - - @pytest.mark.asyncio async def test_list_milestones_returns_dict_with_progress(): rows = [{"id": 1, "title": "MS1", "status": "active", "task_count": 2}] @@ -33,7 +25,7 @@ async def test_list_milestones_returns_dict_with_progress(): @pytest.mark.asyncio async def test_create_milestone_passes_through(): - m = _fake_ms(id=5) + m = fake_milestone(id=5) mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.create_milestone", mock): out = await create_milestone(project_id=1, title="new", description="d") @@ -45,7 +37,7 @@ async def test_create_milestone_passes_through(): @pytest.mark.asyncio async def test_create_milestone_empty_description_becomes_none(): - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.create_milestone", mock): await create_milestone(project_id=1, title="t", description="") @@ -55,7 +47,7 @@ async def test_create_milestone_empty_description_becomes_none(): @pytest.mark.asyncio async def test_create_milestone_passes_body_through(): """The milestone-as-plan body is forwarded to the service.""" - m = _fake_ms(id=5) + m = fake_milestone(id=5) mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.create_milestone", mock): await create_milestone(project_id=1, title="t", body="## Goal\n\nship") @@ -64,7 +56,7 @@ async def test_create_milestone_passes_body_through(): @pytest.mark.asyncio async def test_create_milestone_empty_body_becomes_none(): - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.create_milestone", mock): await create_milestone(project_id=1, title="t", body="") @@ -73,7 +65,7 @@ async def test_create_milestone_empty_body_becomes_none(): @pytest.mark.asyncio async def test_update_milestone_sends_body(): - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.update_milestone", mock): await update_milestone(project_id=1, milestone_id=5, body="new plan") @@ -82,7 +74,7 @@ async def test_update_milestone_sends_body(): @pytest.mark.asyncio async def test_get_milestone_returns_body_steps_and_rules(): - m = _fake_ms(id=5, project_id=3, body="## Goal") + m = fake_milestone(id=5, project_id=3, body="## Goal") step = MagicMock() step.to_dict.return_value = {"id": 9, "title": "step 1", "status": "todo"} applicable = {"rules": [{"id": 1, "title": "r"}], "truncated": False, @@ -112,7 +104,7 @@ async def test_get_milestone_raises_when_not_found(): @pytest.mark.asyncio async def test_update_milestone_only_sends_non_default_fields(): - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.update_milestone", mock): await update_milestone(project_id=1, milestone_id=5, status="done") @@ -124,7 +116,7 @@ async def test_update_milestone_only_sends_non_default_fields(): @pytest.mark.asyncio async def test_update_milestone_order_index_negative_is_omitted(): """order_index=-1 sentinel means leave unchanged.""" - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.update_milestone", mock): await update_milestone(project_id=1, milestone_id=5, order_index=-1) @@ -134,7 +126,7 @@ async def test_update_milestone_order_index_negative_is_omitted(): @pytest.mark.asyncio async def test_update_milestone_order_index_zero_is_explicit(): """order_index=0 is a real value (top of list), not a sentinel.""" - m = _fake_ms() + m = fake_milestone() mock = AsyncMock(return_value=m) with patch("scribe.mcp.tools.milestones.milestones_svc.update_milestone", mock): await update_milestone(project_id=1, milestone_id=5, order_index=0) diff --git a/tests/test_mcp_tool_planning.py b/tests/test_mcp_tool_planning.py index 7fdc617..26d4897 100644 --- a/tests/test_mcp_tool_planning.py +++ b/tests/test_mcp_tool_planning.py @@ -1,6 +1,7 @@ -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest +from tests.helpers import fake_task pytestmark = pytest.mark.usefixtures("_bind_user") @@ -18,25 +19,12 @@ async def test_start_planning_tool_delegates_to_service(): assert mock.call_args.kwargs == {"user_id": 7, "project_id": 3, "title": "Plan it"} -def _plan_note(task_kind: str): - note = MagicMock() - note.parent_id = None - note.project_id = 3 - note.id = 9 - # Real values — get_task reads deleted_at and compares user_id to the bound - # caller, and a MagicMock is truthy on both (note 2109). - note.user_id = 7 - note.deleted_at = None - note.to_dict.return_value = {"id": 9, "task_kind": task_kind, "project_id": 3} - return note - - @pytest.mark.asyncio async def test_get_task_augments_plan_with_rules(): applicable = {"rules": [{"id": 1, "title": "r"}], "truncated": False, "subscribed_rulebooks": [{"id": 2, "title": "rb"}]} with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", - AsyncMock(return_value=(_plan_note("plan"), "owner"))), \ + AsyncMock(return_value=(fake_task(task_kind="plan", id=9, project_id=3), "owner"))), \ patch("scribe.mcp.tools.tasks.rulebooks_svc.get_applicable_rules", AsyncMock(return_value=applicable)): from scribe.mcp.tools.tasks import get_task @@ -49,7 +37,7 @@ async def test_get_task_augments_plan_with_rules(): @pytest.mark.asyncio async def test_get_task_work_kind_has_no_rules(): with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", - AsyncMock(return_value=(_plan_note("work"), "owner"))), \ + AsyncMock(return_value=(fake_task(task_kind="work", id=9, project_id=3), "owner"))), \ patch("scribe.mcp.tools.tasks.rulebooks_svc.get_applicable_rules", AsyncMock()) as mock_rules: from scribe.mcp.tools.tasks import get_task diff --git a/tests/test_mcp_tool_processes.py b/tests/test_mcp_tool_processes.py index d8f73a5..49e26ff 100644 --- a/tests/test_mcp_tool_processes.py +++ b/tests/test_mcp_tool_processes.py @@ -2,7 +2,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note +from tests.helpers import FakeMCP, fake_note pytestmark = pytest.mark.usefixtures("_bind_user") @@ -172,18 +172,11 @@ def test_register_attaches_every_tool_in_the_module(): import inspect from scribe.mcp.tools import processes - names: list[str] = [] + mcp = FakeMCP() - class FakeMcp: - def tool(self, name): - names.append(name) - def deco(fn): - return fn - return deco - - processes.register(FakeMcp()) + processes.register(mcp) public = { name for name, obj in vars(processes).items() if inspect.iscoroutinefunction(obj) and not name.startswith("_") } - assert set(names) == public + assert set(mcp.names) == public diff --git a/tests/test_mcp_tool_projects.py b/tests/test_mcp_tool_projects.py index 94df878..a434ee4 100644 --- a/tests/test_mcp_tool_projects.py +++ b/tests/test_mcp_tool_projects.py @@ -7,6 +7,7 @@ from scribe.mcp.tools.projects import ( list_projects, get_project, create_project, update_project, enter_project, ) +from tests.helpers import FakeMCP, fake_project pytestmark = pytest.mark.usefixtures("_bind_user") @@ -56,22 +57,9 @@ def _no_bootstrap(): yield mock -def _fake_project(design_system_id=None, **overrides) -> MagicMock: - p = MagicMock() - base = {"id": 1, "title": "P", "description": "", "goal": "", - "status": "active", "color": None} - base.update(overrides) - p.to_dict.return_value = base - # Explicit, because a bare MagicMock hands back a truthy auto-attribute — - # which would route every project in this file through the design-system - # branch and out to a real database. - p.design_system_id = design_system_id - return p - - @pytest.mark.asyncio async def test_list_projects_wraps_in_dict(): - rows = [_fake_project(id=1), _fake_project(id=2)] + rows = [fake_project(id=1), fake_project(id=2)] with patch( "scribe.mcp.tools.projects.projects_svc.list_projects", AsyncMock(return_value=rows), @@ -82,7 +70,7 @@ async def test_list_projects_wraps_in_dict(): @pytest.mark.asyncio async def test_get_project_enriches_with_milestone_summary(): - p = _fake_project(id=5, title="found") + p = fake_project(id=5, title="found") milestone_summary = [{"id": 10, "title": "MS", "task_count": 3}] applicable_payload = { "rules": [], "truncated": False, "subscribed_rulebooks": [], @@ -107,7 +95,7 @@ async def test_get_project_includes_applicable_rules_and_subscribed_rulebooks(): """The augmented get_project response includes applicable_rules and subscribed_rulebooks pulled from services/rulebooks.get_applicable_rules. """ - p = _fake_project(id=3, title="Fabled Assistant") + p = fake_project(id=3, title="Fabled Assistant") milestone_summary = [] applicable_payload = { "rules": [ @@ -147,7 +135,7 @@ async def test_get_project_raises_when_not_found(): @pytest.mark.asyncio async def test_create_project_passes_color_empty_as_none(): - p = _fake_project() + p = fake_project() mock = AsyncMock(return_value=p) with patch("scribe.mcp.tools.projects.projects_svc.create_project", mock): await create_project(title="P", color="") @@ -156,7 +144,7 @@ async def test_create_project_passes_color_empty_as_none(): @pytest.mark.asyncio async def test_update_project_only_sends_non_default_fields(): - p = _fake_project() + p = fake_project() mock = AsyncMock(return_value=p) with patch("scribe.mcp.tools.projects.projects_svc.update_project", mock): await update_project(project_id=1, status="archived") @@ -179,7 +167,7 @@ async def test_update_project_raises_when_not_found(): async def test_enter_project_composes_full_context(): """enter_project pulls project + rules + milestone summary + open tasks + recent notes in one composed call.""" - p = _fake_project(id=5, title="P") + p = fake_project(id=5, title="P") applicable_payload = { "rules": [{"id": 1, "title": "r1", "statement": "s", "topic_title": "t", "rulebook_title": "rb"}], @@ -239,7 +227,7 @@ async def test_enter_project_surfaces_the_systems_vocabulary(): days after the feature landed — one System, nothing tagged since July 28 (#2546's audit). Trimmed to id/name/first-line: it rides on every session start, and the full charter is get_system's job.""" - p = _fake_project(id=5) + p = fake_project(id=5) sys1 = MagicMock() sys1.id = 3 sys1.name = "retrieval" @@ -294,7 +282,7 @@ async def test_enter_project_carries_the_bootstrap_ask_when_it_fires(): ask = "This project has 282 records and NO Systems modelled — ..." with contextlib.ExitStack() as stack: - for cm in _enter_project_stubs(_fake_project(id=5)): + for cm in _enter_project_stubs(fake_project(id=5)): stack.enter_context(cm) stack.enter_context(patch( "scribe.mcp.tools.projects.systems_tools.bootstrap_systems_ask", @@ -315,7 +303,7 @@ async def test_enter_project_never_asks_bootstrap_once_a_vocabulary_exists( sys1 = MagicMock() sys1.id = 3; sys1.name = "retrieval"; sys1.description = "" with contextlib.ExitStack() as stack: - for cm in _enter_project_stubs(_fake_project(id=5)): + for cm in _enter_project_stubs(fake_project(id=5)): stack.enter_context(cm) stack.enter_context(patch( "scribe.mcp.tools.projects.systems_svc.list_systems", @@ -336,7 +324,7 @@ async def test_enter_project_fires_the_coverage_seed_on_the_owner( enter stays fast.""" import contextlib - project = _fake_project(id=5) + project = fake_project(id=5) project.user_id = 42 # explicit: the OWNER, not the caller (ctx uid=7) with contextlib.ExitStack() as stack: for cm in _enter_project_stubs(project): @@ -357,7 +345,7 @@ async def test_enter_project_hands_back_the_design_system_when_the_project_has_o system binds the same way a rule does. Before this it was reachable only by an agent that already knew to call resolve_design_system — so the standards were present in the store and absent from the work.""" - p = _fake_project(id=5, design_system_id=9) + p = fake_project(id=5, design_system_id=9) design = {"id": 9, "title": "App kit", "guidance": [{"title": "House"}], "token_count": 95, "token_groups": ["surface"], "inherits_from": ["House"], "description": ""} @@ -399,14 +387,7 @@ async def test_enter_project_raises_when_project_not_found(): def test_enter_project_registered_in_register(): """register(mcp) registers enter_project alongside the existing tools.""" from scribe.mcp.tools.projects import register - registered: list[str] = [] + mcp = FakeMCP() - class FakeMCP: - def tool(self, name=None): - def decorator(fn): - registered.append(name) - return fn - return decorator - - register(FakeMCP()) - assert "enter_project" in registered + register(mcp) + assert "enter_project" in mcp.names diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 4ddb48a..004c7d2 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -1,41 +1,16 @@ """Tests for MCP rulebook tools — patches the service layer.""" -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest +from tests.helpers import FakeMCP, fake_rule, fake_rulebook, fake_topic pytestmark = pytest.mark.usefixtures("_bind_user") -def _fake_rulebook(id=1, title="t"): - rb = MagicMock() - rb.id = id - rb.title = title - rb.to_dict.return_value = {"id": id, "title": title} - return rb - - -def _fake_topic(id=10, title="git"): - t = MagicMock() - t.id = id - t.title = title - t.to_dict.return_value = {"id": id, "title": title} - return t - - -def _fake_rule(id=100, title="r", statement="s"): - r = MagicMock() - r.id = id - r.title = title - r.topic_id = 10 - r.statement = statement - r.to_dict.return_value = {"id": id, "title": title, "statement": statement} - return r - - @pytest.mark.asyncio async def test_list_rulebooks_wraps_in_dict(): - rows = [_fake_rulebook(id=1), _fake_rulebook(id=2)] + rows = [fake_rulebook(id=1, title="t"), fake_rulebook(id=2, title="t")] with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rulebooks", AsyncMock(return_value=rows), @@ -47,8 +22,8 @@ async def test_list_rulebooks_wraps_in_dict(): @pytest.mark.asyncio async def test_get_rulebook_includes_topics(): - rb = _fake_rulebook(id=1) - topics = [_fake_topic(id=10), _fake_topic(id=11)] + rb = fake_rulebook(id=1, title="t") + topics = [fake_topic(id=10, title="git"), fake_topic(id=11, title="git")] with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rulebook", AsyncMock(return_value=rb), @@ -75,7 +50,7 @@ async def test_get_rulebook_raises_when_not_found(): @pytest.mark.asyncio async def test_create_rule_passes_required_fields(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock = AsyncMock(return_value=rule) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", mock): from scribe.mcp.tools.rulebooks import create_rule @@ -109,7 +84,7 @@ async def test_create_rule_force_bypasses_duplicate_gate(): find_mock = AsyncMock() with patch("scribe.mcp.tools.rulebooks.dedup_svc.find_duplicate_rule", find_mock), \ patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", - AsyncMock(return_value=_fake_rule(id=5))): + AsyncMock(return_value=fake_rule(id=5, title="r", statement="s", topic_id=10))): from scribe.mcp.tools.rulebooks import create_rule out = await create_rule(topic_id=10, title="dev is home", statement="x", force=True) assert out["id"] == 5 @@ -118,7 +93,7 @@ async def test_create_rule_force_bypasses_duplicate_gate(): @pytest.mark.asyncio async def test_update_rule_only_sends_non_default_fields(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock = AsyncMock(return_value=rule) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule", mock): from scribe.mcp.tools.rulebooks import update_rule @@ -131,7 +106,7 @@ async def test_update_rule_only_sends_non_default_fields(): @pytest.mark.asyncio async def test_delete_rule_without_confirmed_returns_warning(): """delete_rule with confirmed=False returns a preview, not an action.""" - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", AsyncMock(return_value=rule), @@ -148,7 +123,7 @@ async def test_delete_rule_without_confirmed_returns_warning(): @pytest.mark.asyncio async def test_delete_rule_with_confirmed_soft_deletes(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock_delete = AsyncMock(return_value="batch-1") with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", @@ -191,27 +166,20 @@ async def test_unsubscribe_project_from_rulebook_calls_service(): def test_register_attaches_all_sixteen_tools(): """register(mcp) should call mcp.tool(name=...) for all 16 tools.""" from scribe.mcp.tools.rulebooks import register - registered: list[str] = [] + mcp = FakeMCP() - class FakeMCP: - def tool(self, name=None): - def decorator(fn): - registered.append(name) - return fn - return decorator - - register(FakeMCP()) - assert len(registered) == 22 + register(mcp) + assert len(mcp.names) == 22 # spot-check a few names - assert "list_rulebooks" in registered - assert "create_rule" in registered - assert "subscribe_project_to_rulebook" in registered - assert "list_always_on_rules" in registered - assert "create_project_rule" in registered - assert "suppress_rule_for_project" in registered - assert "unsuppress_rule_for_project" in registered - assert "suppress_topic_for_project" in registered - assert "unsuppress_topic_for_project" in registered + assert "list_rulebooks" in mcp.names + assert "create_rule" in mcp.names + assert "subscribe_project_to_rulebook" in mcp.names + assert "list_always_on_rules" in mcp.names + assert "create_project_rule" in mcp.names + assert "suppress_rule_for_project" in mcp.names + assert "unsuppress_rule_for_project" in mcp.names + assert "suppress_topic_for_project" in mcp.names + assert "unsuppress_topic_for_project" in mcp.names @pytest.mark.asyncio @@ -227,7 +195,7 @@ async def test_list_always_on_rules_returns_empty_when_no_always_on_rulebooks(): @pytest.mark.asyncio async def test_list_always_on_rules_projects_each_rule(): - rules = [_fake_rule(id=100), _fake_rule(id=101)] + rules = [fake_rule(id=100, title="r", statement="s", topic_id=10), fake_rule(id=101, title="r", statement="s", topic_id=10)] with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.list_always_on_rules", AsyncMock(return_value=rules), @@ -241,7 +209,7 @@ async def test_list_always_on_rules_projects_each_rule(): @pytest.mark.asyncio async def test_update_rulebook_forwards_always_on_when_set(): - rb = _fake_rulebook(id=1, title="t") + rb = fake_rulebook(id=1, title="t") mock = AsyncMock(return_value=rb) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rulebook", mock): from scribe.mcp.tools.rulebooks import update_rulebook @@ -254,7 +222,7 @@ async def test_update_rulebook_forwards_always_on_when_set(): @pytest.mark.asyncio async def test_update_rulebook_omits_always_on_when_none(): - rb = _fake_rulebook(id=1, title="t") + rb = fake_rulebook(id=1, title="t") mock = AsyncMock(return_value=rb) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rulebook", mock): from scribe.mcp.tools.rulebooks import update_rulebook @@ -266,7 +234,7 @@ async def test_update_rulebook_omits_always_on_when_none(): @pytest.mark.asyncio async def test_create_project_rule_passes_required_fields(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock = AsyncMock(return_value=rule) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock): from scribe.mcp.tools.rulebooks import create_project_rule @@ -284,7 +252,7 @@ async def test_create_project_rule_passes_required_fields(): @pytest.mark.asyncio async def test_create_project_rule_derives_title_from_statement(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock = AsyncMock(return_value=rule) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock): from scribe.mcp.tools.rulebooks import create_project_rule @@ -299,7 +267,7 @@ async def test_create_project_rule_derives_title_from_statement(): @pytest.mark.asyncio async def test_create_project_rule_uses_explicit_title_when_given(): - rule = _fake_rule() + rule = fake_rule(id=100, title="r", statement="s", topic_id=10) mock = AsyncMock(return_value=rule) with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_project_rule", mock): from scribe.mcp.tools.rulebooks import create_project_rule diff --git a/tests/test_mcp_tool_snippets.py b/tests/test_mcp_tool_snippets.py index 441d737..7773f14 100644 --- a/tests/test_mcp_tool_snippets.py +++ b/tests/test_mcp_tool_snippets.py @@ -2,32 +2,12 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from tests.helpers import FakeMCP, fake_snippet pytestmark = pytest.mark.usefixtures("_bind_user") -def _fake_snippet(user_id: int = 7): - n = MagicMock() - n.id = 1 - n.title = "debounce — rate-limit a callback" - n.body = "```js\nreturn 1\n```\n" - n.tags = ["js", "snippet"] - n.note_type = "snippet" - # Real int, matching the bound caller by default. The tools compare it to - # decide whether to attach a shared/owner marker; an auto-MagicMock would read - # as another user's record and send them off to look up a username. - n.user_id = user_id - # Explicitly None, not an auto-attribute: snippet_fields prefers `data` when - # truthy, and a MagicMock is truthy — every parsed field would come back as a - # MagicMock instead of a string. - n.data = None - n.to_dict.return_value = { - "id": 1, "title": n.title, "note_type": "snippet", "tags": n.tags, - } - return n - - @pytest.mark.asyncio async def test_create_snippet_requires_name_and_code(): from scribe.mcp.tools.snippets import create_snippet @@ -39,7 +19,7 @@ async def test_create_snippet_requires_name_and_code(): @pytest.mark.asyncio async def test_create_snippet_records_and_returns_parsed(): - created = _fake_snippet() + created = fake_snippet() with patch("scribe.services.dedup.find_duplicate_note", AsyncMock(return_value=None)), \ patch("scribe.services.snippets.create_snippet", AsyncMock(return_value=created)) as mock_create: @@ -88,7 +68,7 @@ async def test_update_snippet_missing_raises(): async def test_update_snippet_empty_string_clears_a_field(): # An omitted field must stay None ("leave alone"), but an explicit empty # string has to reach the service as "" so a stale field can be removed. - updated = _fake_snippet() + updated = fake_snippet() with patch("scribe.services.snippets.update_snippet", AsyncMock(return_value=updated)) as mock_update: from scribe.mcp.tools.snippets import update_snippet @@ -101,7 +81,7 @@ async def test_update_snippet_empty_string_clears_a_field(): @pytest.mark.asyncio async def test_update_snippet_project_id_conventions(): from scribe.services import snippets as snippets_svc - updated = _fake_snippet() + updated = fake_snippet() cases = {0: snippets_svc.UNSET, -1: None, 5: 5} for given, expected in cases.items(): with patch("scribe.services.snippets.update_snippet", @@ -115,7 +95,7 @@ async def test_update_snippet_project_id_conventions(): async def test_create_and_update_pass_locations_through(): locs = [{"repo": "a", "path": "a.py", "symbol": "f"}, {"repo": "b", "path": "b.py", "symbol": "g"}] - created = _fake_snippet() + created = fake_snippet() with patch("scribe.services.dedup.find_duplicate_note", AsyncMock(return_value=None)), \ patch("scribe.services.snippets.create_snippet", AsyncMock(return_value=created)) as mock_create: @@ -187,7 +167,7 @@ async def test_merge_snippets_requires_a_source(): @pytest.mark.asyncio async def test_merge_snippets_returns_survivor_and_merged_ids(): - survivor = _fake_snippet() + survivor = fake_snippet() with patch("scribe.services.snippets.merge_snippets", AsyncMock(return_value=(survivor, [2, 3]))) as mock_merge: from scribe.mcp.tools.snippets import merge_snippets @@ -209,18 +189,10 @@ async def test_merge_snippets_not_found_raises(): def test_register_attaches_all_tools(): from scribe.mcp.tools import snippets - names: list[str] = [] + mcp = FakeMCP() - class FakeMcp: - def tool(self, name): - names.append(name) - - def deco(fn): - return fn - return deco - - snippets.register(FakeMcp()) - assert set(names) == { + snippets.register(mcp) + assert set(mcp.names) == { "list_snippets", "create_snippet", "get_snippet", "update_snippet", "delete_snippet", "merge_snippets", "verify_snippet", "find_duplicate_snippets", "unmerge_snippet", diff --git a/tests/test_mcp_tool_systems.py b/tests/test_mcp_tool_systems.py index 25beb8f..45e3a32 100644 --- a/tests/test_mcp_tool_systems.py +++ b/tests/test_mcp_tool_systems.py @@ -2,21 +2,14 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note - - -def _fake_system(sid=1, name="Reader", project_id=5): - s = MagicMock() - s.to_dict.return_value = {"id": sid, "name": name, "project_id": project_id} - s.project_id = project_id - return s +from tests.helpers import fake_note, fake_system @pytest.mark.asyncio async def test_create_system_returns_dict(): with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ patch("scribe.mcp.tools.systems.systems_svc") as svc: - svc.create_system = AsyncMock(return_value=_fake_system(name="Reader")) + svc.create_system = AsyncMock(return_value=fake_system(name="Reader")) from scribe.mcp.tools.systems import create_system result = await create_system(project_id=5, name="Reader", description="pdf reader") assert result["name"] == "Reader" @@ -39,7 +32,7 @@ async def test_get_system_splits_records_by_kind(): note = MagicMock(); note.to_dict.return_value = {"id": 12}; note.task_kind = "work"; note.status = None with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ patch("scribe.mcp.tools.systems.systems_svc") as svc: - svc.get_system = AsyncMock(return_value=_fake_system(sid=3)) + svc.get_system = AsyncMock(return_value=fake_system(id=3)) svc.list_records_for_system = AsyncMock(return_value=[issue, work, note]) from scribe.mcp.tools.systems import get_system result = await get_system(system_id=3) @@ -164,7 +157,7 @@ async def test_populated_vocabulary_never_counts_records(): @pytest.mark.asyncio async def test_create_system_same_normalized_name_is_duplicate_gated(): - existing = _fake_system(sid=7, name="Scrape Pipeline") + existing = fake_system(id=7, name="Scrape Pipeline") existing.id = 7 existing.name = "Scrape Pipeline" with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ @@ -180,12 +173,12 @@ async def test_create_system_same_normalized_name_is_duplicate_gated(): @pytest.mark.asyncio async def test_create_system_distinct_name_passes_the_gate(): - other = _fake_system(sid=7, name="Workers") + other = fake_system(id=7, name="Workers") other.name = "Workers" with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ patch("scribe.mcp.tools.systems.systems_svc") as svc: svc.list_systems = AsyncMock(return_value=[other]) - svc.create_system = AsyncMock(return_value=_fake_system(sid=8, name="Exporter")) + svc.create_system = AsyncMock(return_value=fake_system(id=8, name="Exporter")) from scribe.mcp.tools.systems import create_system result = await create_system(project_id=5, name="Exporter") assert result["name"] == "Exporter" diff --git a/tests/test_mcp_tool_tags.py b/tests/test_mcp_tool_tags.py index ea95a36..dcb2231 100644 --- a/tests/test_mcp_tool_tags.py +++ b/tests/test_mcp_tool_tags.py @@ -8,6 +8,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.mcp.tools.tags import list_tags, _aggregate_tag_counts +from tests.helpers import make_mock_session pytestmark = pytest.mark.usefixtures("_bind_user") @@ -31,9 +32,7 @@ async def test_fable_list_tags_returns_sorted_by_count_desc(): """End-to-end: query returns three rows, top tag wins.""" mock_result = MagicMock() mock_result.all.return_value = [(["a"],), (["a", "b"],), (["a"],)] - mock_session = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) + mock_session = make_mock_session() mock_session.execute = AsyncMock(return_value=mock_result) mock_ctx = MagicMock(return_value=mock_session) with patch("scribe.mcp.tools.tags.async_session", mock_ctx): @@ -47,9 +46,7 @@ async def test_fable_list_tags_returns_sorted_by_count_desc(): async def test_fable_list_tags_clamps_limit(): mock_result = MagicMock() mock_result.all.return_value = [] - mock_session = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) + mock_session = make_mock_session() mock_session.execute = AsyncMock(return_value=mock_result) mock_ctx = MagicMock(return_value=mock_session) with patch("scribe.mcp.tools.tags.async_session", mock_ctx): diff --git a/tests/test_mcp_tool_tasks.py b/tests/test_mcp_tool_tasks.py index fa44350..47db67a 100644 --- a/tests/test_mcp_tool_tasks.py +++ b/tests/test_mcp_tool_tasks.py @@ -8,35 +8,15 @@ from scribe.mcp.tools.tasks import ( list_tasks, get_task, create_task, update_task, add_task_log, ) +from tests.helpers import fake_task pytestmark = pytest.mark.usefixtures("_bind_user") -def _fake_task(*, parent_id: int | None = None, user_id: int = 7, - **overrides) -> MagicMock: - n = MagicMock() - n.parent_id = parent_id - base = { - "id": 1, "title": "t", "body": "", "status": "todo", - "priority": "none", "tags": [], "parent_id": parent_id, - "is_task": True, - } - base.update(overrides) - n.to_dict.return_value = base - n.title = base["title"] - n.id = base["id"] - # Real values, not auto-attributes: get_task reads deleted_at and compares - # user_id against the bound caller for the shared/owner marker — a MagicMock - # is truthy on both (note 2109). - n.user_id = user_id - n.deleted_at = None - return n - - @pytest.mark.asyncio async def test_list_tasks_passes_is_task_true_and_repackages(): - rows = [_fake_task(id=1), _fake_task(id=2)] + rows = [fake_task(id=1), fake_task(id=2)] mock = AsyncMock(return_value=(rows, 2)) with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", mock): out = await list_tasks() @@ -63,7 +43,7 @@ async def test_list_tasks_empty_status_means_no_filter(): @pytest.mark.asyncio async def test_get_task_with_no_parent_returns_null_parent_title(): - fake = _fake_task(id=5, title="solo", parent_id=None) + fake = fake_task(id=5, title="solo", parent_id=None) with patch( "scribe.mcp.tools.tasks.notes_svc.get_note_for_user", AsyncMock(return_value=(fake, "owner")), @@ -77,8 +57,8 @@ async def test_get_task_with_no_parent_returns_null_parent_title(): @pytest.mark.asyncio async def test_get_task_enriches_with_parent_title(): """When parent_id is set, get_task fetches the parent and adds parent_title.""" - child = _fake_task(id=10, title="child", parent_id=5) - parent = _fake_task(id=5, title="parent of 10", parent_id=None) + child = fake_task(id=10, title="child", parent_id=5) + parent = fake_task(id=5, title="parent of 10", parent_id=None) # fetched twice: once for the child, once for the parent mock_get = AsyncMock(side_effect=[(child, "owner"), (parent, "owner")]) with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", mock_get): @@ -90,7 +70,7 @@ async def test_get_task_enriches_with_parent_title(): @pytest.mark.asyncio async def test_get_task_parent_missing_returns_null(): """If parent_id is set but the parent is gone (orphaned), parent_title is None.""" - child = _fake_task(id=10, parent_id=5) + child = fake_task(id=10, parent_id=5) mock_get = AsyncMock(side_effect=[(child, "owner"), None]) with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", mock_get): out = await get_task(task_id=10) @@ -101,7 +81,7 @@ async def test_get_task_parent_missing_returns_null(): async def test_get_task_opens_a_shared_task_and_says_whose_it_is(): """A task in a shared project opens in the web UI, so the agent path must not answer "not found" for the same id — and must say it isn't the caller's.""" - theirs = _fake_task(id=5, title="Their task", user_id=9) + theirs = fake_task(id=5, title="Their task", user_id=9) with patch( "scribe.mcp.tools.tasks.notes_svc.get_note_for_user", AsyncMock(return_value=(theirs, "viewer")), @@ -127,7 +107,7 @@ async def test_get_task_raises_when_not_found(): @pytest.mark.asyncio async def test_create_task_passes_status(): - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.create_note", mock): await create_task(title="do x", status="todo") @@ -155,7 +135,7 @@ async def test_create_task_force_bypasses_duplicate_gate(): find_mock = AsyncMock() with patch("scribe.mcp.tools.tasks.dedup_svc.find_duplicate_note", find_mock), \ patch("scribe.mcp.tools.tasks.notes_svc.create_note", - AsyncMock(return_value=_fake_task(id=9))): + AsyncMock(return_value=fake_task(id=9))): out = await create_task(title="dup", force=True) assert out["id"] == 9 find_mock.assert_not_called() # gate not even consulted @@ -173,7 +153,7 @@ async def test_create_task_rejects_retired_plan_kind(): @pytest.mark.asyncio async def test_create_task_priority_empty_becomes_none(): - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.create_note", mock): await create_task(title="x", priority="") @@ -182,7 +162,7 @@ async def test_create_task_priority_empty_becomes_none(): @pytest.mark.asyncio async def test_create_task_zero_id_sentinels_become_none(): - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.create_note", mock): await create_task(title="x", project_id=0, milestone_id=0, parent_id=0) @@ -193,7 +173,7 @@ async def test_create_task_zero_id_sentinels_become_none(): @pytest.mark.asyncio async def test_update_task_only_sends_non_default_fields(): - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, status="done") @@ -205,7 +185,7 @@ async def test_update_task_only_sends_non_default_fields(): @pytest.mark.asyncio async def test_update_task_empty_priority_is_omitted(): """Priority="" is "leave unchanged" — must not reach service as empty string.""" - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, status="done", priority="") @@ -225,7 +205,7 @@ async def test_update_task_raises_when_not_found(): @pytest.mark.asyncio async def test_update_task_milestone_zero_is_omitted(): """milestone_id=0 is 'leave unchanged' — must not reach the service.""" - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, milestone_id=0) @@ -234,7 +214,7 @@ async def test_update_task_milestone_zero_is_omitted(): @pytest.mark.asyncio async def test_update_task_milestone_positive_is_set(): - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, milestone_id=42) @@ -244,7 +224,7 @@ async def test_update_task_milestone_positive_is_set(): @pytest.mark.asyncio async def test_update_task_milestone_negative_one_clears(): """milestone_id=-1 clears the milestone (sets the column NULL).""" - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, milestone_id=-1) @@ -254,7 +234,7 @@ async def test_update_task_milestone_negative_one_clears(): @pytest.mark.asyncio async def test_update_task_clearing_project_also_clears_milestone(): """project_id=-1 clears the project and, with it, the milestone.""" - fake = _fake_task() + fake = fake_task() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): await update_task(task_id=1, project_id=-1) diff --git a/tests/test_mcp_tool_trash.py b/tests/test_mcp_tool_trash.py index 733442a..26fdc4c 100644 --- a/tests/test_mcp_tool_trash.py +++ b/tests/test_mcp_tool_trash.py @@ -2,6 +2,7 @@ from unittest.mock import AsyncMock, patch import pytest +from tests.helpers import FakeMCP pytestmark = pytest.mark.usefixtures("_bind_user") @@ -47,14 +48,7 @@ async def test_purge_trash_when_confirmed(): def test_register_attaches_three_tools(): from scribe.mcp.tools.trash import register - names: list[str] = [] + mcp = FakeMCP() - class FakeMCP: - def tool(self, name=None): - def deco(fn): - names.append(name) - return fn - return deco - - register(FakeMCP()) - assert set(names) == {"list_trash", "restore", "purge_trash"} + register(mcp) + assert set(mcp.names) == {"list_trash", "restore", "purge_trash"} diff --git a/tests/test_notes_description_field.py b/tests/test_notes_description_field.py index d519ce5..3884b8d 100644 --- a/tests/test_notes_description_field.py +++ b/tests/test_notes_description_field.py @@ -5,17 +5,16 @@ from `body` which (post-Task-as-Durable-Record) becomes the LLM-maintained consolidation summary. """ from unittest.mock import AsyncMock, MagicMock, patch +from tests.helpers import make_mock_session def _mock_session_for_update(mock_note): - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = mock_note mock_session.execute = AsyncMock(return_value=mock_result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) return mock_session @@ -56,12 +55,10 @@ async def test_create_note_forwards_description_to_model(): for k, v in kw.items(): setattr(self, k, v) - mock_session = AsyncMock() + mock_session = make_mock_session() mock_session.add = MagicMock() mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch( "scribe.services.notes.async_session", return_value=mock_session diff --git a/tests/test_recurrence.py b/tests/test_recurrence.py index d11d3f3..bb89bfc 100644 --- a/tests/test_recurrence.py +++ b/tests/test_recurrence.py @@ -12,14 +12,12 @@ async def test_update_note_sets_started_at_on_in_progress(): mock_note.started_at = None mock_note.recurrence_rule = None - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = mock_note mock_session.execute = AsyncMock(return_value=mock_result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch("scribe.services.notes.async_session", return_value=mock_session): from scribe.services.notes import update_note @@ -36,14 +34,12 @@ async def test_update_note_sets_completed_at_on_done(): mock_note.completed_at = None mock_note.recurrence_rule = None - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = mock_note mock_session.execute = AsyncMock(return_value=mock_result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch("scribe.services.notes.async_session", return_value=mock_session): from scribe.services.notes import update_note @@ -60,14 +56,12 @@ async def test_update_note_clears_timestamps_on_todo(): mock_note.completed_at = datetime(2026, 3, 15, tzinfo=timezone.utc) mock_note.recurrence_rule = None - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = mock_note mock_session.execute = AsyncMock(return_value=mock_result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch("scribe.services.notes.async_session", return_value=mock_session): from scribe.services.notes import update_note @@ -85,14 +79,12 @@ async def test_update_note_preserves_started_at_if_already_set(): mock_note.started_at = original_start mock_note.recurrence_rule = None - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.first.return_value = mock_note mock_session.execute = AsyncMock(return_value=mock_result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch("scribe.services.notes.async_session", return_value=mock_session): from scribe.services.notes import update_note @@ -104,6 +96,7 @@ async def test_update_note_preserves_started_at_if_already_set(): # ── Recurrence rule validation ──────────────────────────────────────────────── import pytest +from tests.helpers import make_mock_session def test_validate_interval_rule_valid(): @@ -233,14 +226,12 @@ async def test_spawn_recurring_tasks_creates_child(): mock_task.due_date = date(2026, 3, 1) mock_task.recurrence_rule = {"type": "interval", "every": 3, "unit": "month"} - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.all.return_value = [mock_task] mock_session.execute = AsyncMock(return_value=mock_result) mock_session.get = AsyncMock(return_value=mock_task) mock_session.commit = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) mock_child = MagicMock() mock_child.id = 2 @@ -273,13 +264,11 @@ async def test_list_notes_multi_status_builds_in_clause(): """list_notes with a list of statuses executes without error.""" from unittest.mock import AsyncMock, MagicMock, patch - mock_session = AsyncMock() + mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.all.return_value = [] mock_session.execute = AsyncMock(return_value=mock_result) mock_session.scalar = AsyncMock(return_value=0) - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch("scribe.services.notes.async_session", return_value=mock_session): from scribe.services.notes import list_notes diff --git a/tests/test_retrieval_scopes.py b/tests/test_retrieval_scopes.py index 2c2bfe0..fadbe2d 100644 --- a/tests/test_retrieval_scopes.py +++ b/tests/test_retrieval_scopes.py @@ -15,7 +15,7 @@ shared record would be findable by wording and invisible by meaning. from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note +from tests.helpers import fake_note, make_mock_session pytestmark = pytest.mark.usefixtures("_no_supersession") @@ -95,9 +95,7 @@ async def test_hybrid_search_halves_agree_on_scope(): patch.object(knowledge, "readable_notes_clause", MagicMock(return_value=(Note.user_id == 7))) as read_clause, \ patch.object(knowledge, "async_session") as sess: - session = AsyncMock() - session.__aenter__ = AsyncMock(return_value=session) - session.__aexit__ = AsyncMock(return_value=False) + session = make_mock_session() result = MagicMock() result.scalars.return_value.all.return_value = [] session.execute = AsyncMock(return_value=result) diff --git a/tests/test_routes_design_systems.py b/tests/test_routes_design_systems.py index 8ad2389..803e247 100644 --- a/tests/test_routes_design_systems.py +++ b/tests/test_routes_design_systems.py @@ -5,6 +5,7 @@ deliberate: relaxing one to a pattern would stop it catching the thing it was written for, which is a capability landing on one surface and not the other. """ import inspect +from tests.helpers import FakeMCP def test_design_systems_blueprint_registered(): @@ -107,17 +108,12 @@ def test_every_mcp_tool_in_the_module_is_registered(): else in the codebase would notice.""" from scribe.mcp.tools import design_systems as tools - registered = [] + mcp = FakeMCP() - class _Recorder: - def tool(self, name): - registered.append(name) - return lambda fn: fn - - tools.register(_Recorder()) + tools.register(mcp) public = { name for name, obj in vars(tools).items() if inspect.iscoroutinefunction(obj) and not name.startswith("_") } - assert set(registered) == public + assert set(mcp.names) == public diff --git a/tests/test_services_db_maintenance.py b/tests/test_services_db_maintenance.py index 8b5860e..33a6358 100644 --- a/tests/test_services_db_maintenance.py +++ b/tests/test_services_db_maintenance.py @@ -9,6 +9,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.services.db_maintenance import MAINTENANCE_TABLES, run_maintenance +from tests.helpers import make_mock_session def _mock_engine(exec_side_effect=None): @@ -79,9 +80,7 @@ async def test_one_table_failure_does_not_abort_the_rest(): def _health_session(db_bytes, rows): - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() size_res = MagicMock() size_res.scalar.return_value = db_bytes rows_res = MagicMock() diff --git a/tests/test_services_dedup.py b/tests/test_services_dedup.py index 4f0283b..873eaa1 100644 --- a/tests/test_services_dedup.py +++ b/tests/test_services_dedup.py @@ -9,14 +9,12 @@ from scribe.services.dedup import ( find_duplicate_note, find_duplicate_rule, ) -from tests.helpers import fake_note +from tests.helpers import fake_note, make_mock_session def _session_returning(note): """A mocked async_session() whose single execute() yields `note` (or None).""" - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() result = MagicMock() result.scalars.return_value.first.return_value = note s.execute = AsyncMock(return_value=result) @@ -150,9 +148,7 @@ def _session_sequence(results): a location query and then a code query, and the whole point is that they answer differently. """ - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() wrapped = [] for note in results: r = MagicMock() diff --git a/tests/test_services_project_summaries.py b/tests/test_services_project_summaries.py index 07dde57..745c615 100644 --- a/tests/test_services_project_summaries.py +++ b/tests/test_services_project_summaries.py @@ -17,14 +17,13 @@ project would pass a correctness test and reproduce the outage. from unittest.mock import AsyncMock, MagicMock, patch import pytest +from tests.helpers import make_mock_session def _session_factory(counter: list[int], results: list): """A session whose .execute() returns queued results, counting opens.""" def _make(): - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() counter[0] += 1 async def _execute(*_a, **_kw): diff --git a/tests/test_services_rulebooks.py b/tests/test_services_rulebooks.py index 4e4bfc5..c5871f6 100644 --- a/tests/test_services_rulebooks.py +++ b/tests/test_services_rulebooks.py @@ -3,25 +3,9 @@ Mirrors the pattern in tests/test_events_service.py. """ from unittest.mock import AsyncMock, MagicMock, patch -from datetime import datetime, timezone import pytest -from tests.helpers import make_mock_session - - -def _fake_rulebook(id=1, owner_user_id=7, title="FabledSword family", description=""): - rb = MagicMock() - rb.id = id - rb.owner_user_id = owner_user_id - rb.title = title - rb.description = description - rb.created_at = datetime.now(timezone.utc) - rb.updated_at = datetime.now(timezone.utc) - rb.to_dict.return_value = { - "id": id, "owner_user_id": owner_user_id, - "title": title, "description": description or "", - } - return rb +from tests.helpers import fake_rule, fake_rulebook, fake_topic, make_mock_session @pytest.mark.asyncio @@ -39,7 +23,7 @@ async def test_create_rulebook_stores_to_db(): @pytest.mark.asyncio async def test_list_rulebooks_returns_owned_only(): - rb = _fake_rulebook(id=1) + rb = fake_rulebook(id=1) mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.all.return_value = [rb] @@ -67,7 +51,7 @@ async def test_get_rulebook_returns_none_when_not_owner(): @pytest.mark.asyncio async def test_update_rulebook_only_sets_provided_fields(): - rb = _fake_rulebook(id=1, title="old") + rb = fake_rulebook(id=1, title="old") mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalar_one_or_none.return_value = rb @@ -81,7 +65,7 @@ async def test_update_rulebook_only_sets_provided_fields(): @pytest.mark.asyncio async def test_delete_rulebook_calls_delete(): - rb = _fake_rulebook(id=1) + rb = fake_rulebook(id=1) mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalar_one_or_none.return_value = rb @@ -96,22 +80,6 @@ async def test_delete_rulebook_calls_delete(): # ── Topic CRUD ─────────────────────────────────────────────────────────── -def _fake_topic(id=1, rulebook_id=1, title="git-workflow", description="", order_index=0): - t = MagicMock() - t.id = id - t.rulebook_id = rulebook_id - t.title = title - t.description = description - t.order_index = order_index - t.created_at = datetime.now(timezone.utc) - t.updated_at = datetime.now(timezone.utc) - t.to_dict.return_value = { - "id": id, "rulebook_id": rulebook_id, "title": title, - "description": description or "", "order_index": order_index, - } - return t - - @pytest.mark.asyncio async def test_create_topic_requires_owned_rulebook(): """create_topic raises ValueError if the rulebook isn't owned by user.""" @@ -130,8 +98,8 @@ async def test_create_topic_requires_owned_rulebook(): @pytest.mark.asyncio async def test_list_topics_returns_topics_for_owned_rulebook(): - rb = _fake_rulebook(id=1) - topic = _fake_topic(id=10, rulebook_id=1, title="git-workflow") + rb = fake_rulebook(id=1) + topic = fake_topic(id=10, rulebook_id=1, title="git-workflow") # Two execute calls: ownership check, then topic select. mock_session = make_mock_session() @@ -151,26 +119,6 @@ async def test_list_topics_returns_topics_for_owned_rulebook(): # ── Rule CRUD ─────────────────────────────────────────────────────────── -def _fake_rule(id=1, topic_id=10, title="dev is home", - statement="Work directly on dev", why="", how_to_apply=""): - r = MagicMock() - r.id = id - r.topic_id = topic_id - r.title = title - r.statement = statement - r.why = why - r.how_to_apply = how_to_apply - r.order_index = 0 - r.created_at = datetime.now(timezone.utc) - r.updated_at = datetime.now(timezone.utc) - r.to_dict.return_value = { - "id": id, "topic_id": topic_id, "title": title, - "statement": statement, "why": why or "", - "how_to_apply": how_to_apply or "", "order_index": 0, - } - return r - - @pytest.mark.asyncio async def test_create_rule_requires_owned_topic(): mock_session = make_mock_session() @@ -189,7 +137,7 @@ async def test_create_rule_requires_owned_topic(): @pytest.mark.asyncio async def test_list_rules_filters_by_topic_id(): """list_rules(topic_id=X) returns rules in that topic, ownership-scoped.""" - rule = _fake_rule(id=1, topic_id=10) + rule = fake_rule(id=1, topic_id=10) mock_session = make_mock_session() mock_result = MagicMock() mock_result.scalars.return_value.all.return_value = [rule] diff --git a/tests/test_services_supersession.py b/tests/test_services_supersession.py index 9c7d59c..11aca14 100644 --- a/tests/test_services_supersession.py +++ b/tests/test_services_supersession.py @@ -15,13 +15,12 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.services import supersession +from tests.helpers import make_mock_session def _session(scalars_sequence=None, get_returns=None): """A mocked async_session whose execute() yields successive scalar lists.""" - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() results = [] for scalars in scalars_sequence or []: @@ -156,9 +155,7 @@ async def test_get_relations_partitions_both_directions_from_one_query(): Note 5 supersedes 2 and 3, and is itself superseded by 9. All four rows come back from a single OR query and are partitioned by which column holds 5. """ - session = AsyncMock() - session.__aenter__ = AsyncMock(return_value=session) - session.__aexit__ = AsyncMock(return_value=False) + session = make_mock_session() result = MagicMock() result.all.return_value = [(5, 3), (5, 2), (9, 5)] # (superseder, superseded) session.execute = AsyncMock(return_value=result) diff --git a/tests/test_shared_provenance.py b/tests/test_shared_provenance.py index 08c7c1b..33334c4 100644 --- a/tests/test_shared_provenance.py +++ b/tests/test_shared_provenance.py @@ -9,14 +9,13 @@ passive surface Scribe has (each entry becomes an auto-surfacing skill file). from unittest.mock import AsyncMock, MagicMock, patch import pytest +from tests.helpers import make_mock_session def _session_returning_rows(rows): result = MagicMock() result.all.return_value = rows - session = AsyncMock() - session.__aenter__ = AsyncMock(return_value=session) - session.__aexit__ = AsyncMock(return_value=False) + session = make_mock_session() session.execute = AsyncMock(return_value=result) return session @@ -62,9 +61,7 @@ async def test_provenance_names_the_owner_and_permission(): from scribe.services.access import describe_provenance note = MagicMock(id=1, user_id=9) owner = MagicMock(username="alex") - session = AsyncMock() - session.__aenter__ = AsyncMock(return_value=session) - session.__aexit__ = AsyncMock(return_value=False) + session = make_mock_session() session.get = AsyncMock(return_value=owner) with patch("scribe.services.access.async_session") as cls, \ patch("scribe.services.access.get_note_permission", diff --git a/tests/test_shared_write_access.py b/tests/test_shared_write_access.py index 50c90bb..c0d689e 100644 --- a/tests/test_shared_write_access.py +++ b/tests/test_shared_write_access.py @@ -10,24 +10,17 @@ A record readable-but-not-writable raises PermissionError rather than reporting "not found" — the caller can plainly open it, so not-found would be a lie that sends them hunting for a missing id. """ -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest +from tests.helpers import fake_snippet def _snippet(id=1, owner=9): - n = MagicMock() - n.id = id - n.user_id = owner - n.title = "formatDuration — humanize a millisecond count" - n.body = "```ts\nexport const f = 1\n```\n" - n.tags = ["ts", "snippet"] - n.note_type = "snippet" - n.deleted_at = None - # Explicitly None — snippet_fields prefers `data` when truthy, and an - # auto-MagicMock attribute is truthy (see note 2109). - n.data = None - return n + return fake_snippet( + id=id, user_id=owner, title="formatDuration — humanize a millisecond count", + body="```ts\nexport const f = 1\n```\n", tags=["ts", "snippet"], + ) @pytest.mark.asyncio diff --git a/tests/test_snippet_merge_provenance.py b/tests/test_snippet_merge_provenance.py index b465f1b..d4ea2de 100644 --- a/tests/test_snippet_merge_provenance.py +++ b/tests/test_snippet_merge_provenance.py @@ -6,26 +6,20 @@ only in the trash — and only for someone who already knew to go looking. So th survivor carries `merged_from`, written to the body and the indexed mirror from one value, and ordinary edits must carry it forward rather than erase it. """ -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest from scribe.services import snippets as s +from tests.helpers import fake_snippet def _snippet(id=1, owner=9, body=None, data=None, tags=None): - n = MagicMock() - n.id = id - n.user_id = owner - n.title = "formatDuration — humanize a millisecond count" - n.body = body if body is not None else s.compose_body(code="x = 1", language="ts") - n.tags = tags if tags is not None else ["ts", "snippet"] - n.note_type = "snippet" - n.deleted_at = None - # Explicitly None — snippet_fields prefers `data` when truthy, and an - # auto-MagicMock attribute is truthy (note 2109). - n.data = data - return n + return fake_snippet( + id=id, user_id=owner, title="formatDuration — humanize a millisecond count", + body=body if body is not None else s.compose_body(code="x = 1", language="ts"), + tags=tags if tags is not None else ["ts", "snippet"], data=data, + ) async def _run_merge(target, sources, source_ids): diff --git a/tests/test_snippet_unmerge.py b/tests/test_snippet_unmerge.py index 1009504..d898c02 100644 --- a/tests/test_snippet_unmerge.py +++ b/tests/test_snippet_unmerge.py @@ -7,15 +7,12 @@ that source actually added. These tests pin that rule, and the refusal that guards the case where the attribution isn't there. """ from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest from scribe.services import snippets as s - - -def _loc(path, repo="r", symbol=""): - return {"repo": repo, "path": path, "symbol": symbol} +from tests.helpers import loc def _survivor(locations, merged_from, tags=None, owner=7): @@ -54,7 +51,7 @@ async def _run_unmerge(survivor, *, source_alive=None, restore=1): async def test_unmerge_strips_only_what_the_source_contributed(): """The survivor's own location survives; the source's is removed.""" - mine, theirs = _loc("mine.py"), _loc("theirs.py") + mine, theirs = loc(path="mine.py", repo="r"), loc(path="theirs.py", repo="r") survivor = _survivor( [mine, theirs], [{"id": 2, "locations": [theirs], "tags": ["helper"]}], @@ -70,7 +67,7 @@ async def test_a_location_the_survivor_also_owned_is_never_stripped(): """The central hazard the task named. If a source brought a location the survivor ALREADY had, merge attributes nothing to it — so reversing must leave that call site in place.""" - shared = _loc("shared.py") + shared = loc(path="shared.py", repo="r") survivor = _survivor([shared], [{"id": 2, "locations": [], "tags": ["t"]}]) kwargs = await _run_unmerge(survivor) assert kwargs["data"]["locations"] == [shared] @@ -78,8 +75,8 @@ async def test_a_location_the_survivor_also_owned_is_never_stripped(): async def test_the_reversed_entry_leaves_the_provenance_list(): survivor = _survivor( - [_loc("a.py"), _loc("b.py")], - [{"id": 2, "locations": [_loc("b.py")]}, {"id": 3, "locations": []}], + [loc(path="a.py", repo="r"), loc(path="b.py", repo="r")], + [{"id": 2, "locations": [loc(path="b.py", repo="r")]}, {"id": 3, "locations": []}], ) kwargs = await _run_unmerge(survivor) assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [3] @@ -89,7 +86,7 @@ async def test_the_reversed_entry_leaves_the_provenance_list(): async def test_unmerging_the_last_source_clears_the_provenance_line(): - survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": [], "tags": ["t"]}]) + survivor = _survivor([loc(path="a.py", repo="r")], [{"id": 2, "locations": [], "tags": ["t"]}]) kwargs = await _run_unmerge(survivor) assert "Merged from" not in kwargs["body"] @@ -101,8 +98,8 @@ async def test_an_already_restored_source_is_repaired_not_refused(): """The scenario that motivated the feature: the operator restored the source from the trash by hand, so both records claim its call sites and nothing ever stripped the survivor's copy. Un-merge must fix that, not reject it.""" - theirs = _loc("theirs.py") - survivor = _survivor([_loc("mine.py"), theirs], + theirs = loc(path="theirs.py", repo="r") + survivor = _survivor([loc(path="mine.py", repo="r"), theirs], [{"id": 2, "locations": [theirs]}]) alive = SimpleNamespace(id=2, user_id=7, note_type="snippet", deleted_at=None, title="g — x", tags=["snippet"], body="", data=None) @@ -110,13 +107,13 @@ async def test_an_already_restored_source_is_repaired_not_refused(): kwargs = await _run_unmerge(survivor, source_alive=alive) # Nothing to revive — it's already alive — but the subtraction still happens. revive.assert_not_called() - assert kwargs["data"]["locations"] == [_loc("mine.py")] + assert kwargs["data"]["locations"] == [loc(path="mine.py", repo="r")] async def test_a_purged_source_is_refused_and_the_survivor_is_untouched(): """If the source can't come back, stripping the survivor would lose the locations entirely — no record would claim them.""" - survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": [_loc("a.py")]}]) + survivor = _survivor([loc(path="a.py", repo="r")], [{"id": 2, "locations": [loc(path="a.py", repo="r")]}]) async def fake_get(_uid, sid): return survivor if sid == 1 else None @@ -139,7 +136,7 @@ async def test_an_entry_without_attribution_is_refused_not_guessed(): """Bare-id provenance comes from parsing the body, which can only hold ids. Subtracting a guess could strip call sites the survivor owns — so refuse and say what to do instead.""" - survivor = _survivor([_loc("a.py")], [2]) + survivor = _survivor([loc(path="a.py", repo="r")], [2]) async def fake_get(_uid, sid): return survivor if sid == 1 else None @@ -155,7 +152,7 @@ async def test_an_entry_without_attribution_is_refused_not_guessed(): async def test_unmerging_something_never_absorbed_is_refused(): - survivor = _survivor([_loc("a.py")], [{"id": 99, "locations": []}]) + survivor = _survivor([loc(path="a.py", repo="r")], [{"id": 99, "locations": []}]) with ( patch.object(s, "get_snippet", AsyncMock(return_value=survivor)), patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), @@ -166,7 +163,7 @@ async def test_unmerging_something_never_absorbed_is_refused(): async def test_unmerge_requires_write_access(): """Same rule as merge: a read-only share can see the record, not rearrange it.""" - survivor = _survivor([_loc("a.py")], [{"id": 2, "locations": []}]) + survivor = _survivor([loc(path="a.py", repo="r")], [{"id": 2, "locations": []}]) with ( patch.object(s, "get_snippet", AsyncMock(return_value=survivor)), patch("scribe.services.access.can_write_note", AsyncMock(return_value=False)), diff --git a/tests/test_trash_filtering.py b/tests/test_trash_filtering.py index 34861d2..fb5a3b3 100644 --- a/tests/test_trash_filtering.py +++ b/tests/test_trash_filtering.py @@ -7,12 +7,11 @@ compiled SQL of every statement passed to execute, then assert the from unittest.mock import AsyncMock, MagicMock, patch import pytest +from tests.helpers import make_mock_session def _capturing_session(captured: list[str]): - s = AsyncMock() - s.__aenter__ = AsyncMock(return_value=s) - s.__aexit__ = AsyncMock(return_value=False) + s = make_mock_session() s.commit = AsyncMock() s.scalar = AsyncMock(return_value=0) diff --git a/tests/test_version_pinning_pin_unpin.py b/tests/test_version_pinning_pin_unpin.py index 8fab843..f7d7efe 100644 --- a/tests/test_version_pinning_pin_unpin.py +++ b/tests/test_version_pinning_pin_unpin.py @@ -1,16 +1,15 @@ """Tests for manual pin / unpin on note versions.""" from unittest.mock import AsyncMock, MagicMock, patch +from tests.helpers import make_mock_session def _mock_session_for_version(mock_version): - mock_session = AsyncMock() + mock_session = make_mock_session() result = MagicMock() result.scalars.return_value.first.return_value = mock_version mock_session.execute = AsyncMock(return_value=result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) return mock_session @@ -92,14 +91,12 @@ async def test_pin_version_rejects_overlong_label(): async def test_pin_version_returns_none_when_not_found(): - mock_session = AsyncMock() + mock_session = make_mock_session() result = MagicMock() result.scalars.return_value.first.return_value = None mock_session.execute = AsyncMock(return_value=result) mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) with patch( "scribe.services.version_pinning.async_session", diff --git a/tests/test_version_pinning_prune.py b/tests/test_version_pinning_prune.py index a7fbe86..8787bdb 100644 --- a/tests/test_version_pinning_prune.py +++ b/tests/test_version_pinning_prune.py @@ -10,7 +10,7 @@ async def test_create_version_prune_sql_filters_to_unpinned(): """The DELETE statement issued by create_version's prune step must include `pin_kind IS NULL` in the inner SELECT so pinned versions aren't counted toward MAX_VERSIONS and can't be pruned by it.""" - mock_session = AsyncMock() + mock_session = make_mock_session() select_result = MagicMock() # No prior version → skips the throttle/dedupe early-return paths and # proceeds straight to insert + prune. @@ -18,8 +18,6 @@ async def test_create_version_prune_sql_filters_to_unpinned(): mock_session.add = MagicMock() mock_session.commit = AsyncMock() mock_session.refresh = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) captured_sql: list[str] = [] @@ -48,10 +46,8 @@ async def test_create_version_prune_sql_filters_to_unpinned(): async def test_prune_auto_pins_filters_to_auto_kind(): """prune_auto_pins must filter to pin_kind='auto' so manual pins and rolling rows aren't touched, and must bind MAX_AUTO_PINS as the OFFSET.""" - mock_session = AsyncMock() + mock_session = make_mock_session() mock_session.commit = AsyncMock() - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) captured_sql: list[str] = [] captured_params: list[dict] = [] From 848ce1592e2d0797238f82398d775edb29a04853 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:14:44 -0400 Subject: [PATCH 2/8] fix(tests): import make_mock_session in test_version_pinning_prune (batch-2 follow-up) Co-Authored-By: Claude Fable 5 --- tests/test_version_pinning_prune.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_version_pinning_prune.py b/tests/test_version_pinning_prune.py index 8787bdb..f0eb4b4 100644 --- a/tests/test_version_pinning_prune.py +++ b/tests/test_version_pinning_prune.py @@ -4,6 +4,7 @@ auto-pin bucket (pin_kind='auto'). Design: docs/superpowers/specs/2026-05-13-note-version-pinning-design.md """ from unittest.mock import AsyncMock, MagicMock, patch +from tests.helpers import make_mock_session async def test_create_version_prune_sql_filters_to_unpinned(): From b0eda3257515c2ddb5ab50c5cf4bba5545bab1ef Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:17:42 -0400 Subject: [PATCH 3/8] refactor(models): one iso() for every to_dict timestamp; mixins replace hand-rolled created_at/updated_at (#2827, milestone 296 area 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading all 28 models against each other: 54 `x.isoformat() if x else None` / `x.isoformat()` expressions in 23 to_dict methods, in two guarded/unguarded wordings, become iso() from models/base.py — uniform, and a row read before flush serialises as null instead of raising. Rulebook / RulebookTopic / Rule carried byte-identical copies of TimestampMixin's two columns; InvitationToken / PasswordResetToken / NoteUsageEvent carried CreatedAtMixin's — all six now use the mixin. AppLog and RetrievalLog keep their explicit created_at, commented: their composite index orders on `created_at.desc()`, which needs the column object in the class body. Schema-neutral (same column definitions) — no migration. Co-Authored-By: Claude Fable 5 --- src/scribe/models/api_key.py | 8 ++--- src/scribe/models/app_log.py | 6 +++- src/scribe/models/base.py | 12 ++++++- src/scribe/models/code_shape.py | 14 ++++---- src/scribe/models/design_system.py | 10 +++--- src/scribe/models/forge_connection.py | 6 ++-- src/scribe/models/group.py | 8 ++--- src/scribe/models/invitation.py | 6 ++-- src/scribe/models/milestone.py | 6 ++-- src/scribe/models/note.py | 18 ++++------- src/scribe/models/note_draft.py | 6 ++-- src/scribe/models/note_supersession.py | 4 +-- src/scribe/models/note_usage.py | 12 +++---- src/scribe/models/note_version.py | 4 +-- src/scribe/models/notification.py | 6 ++-- src/scribe/models/password_reset.py | 6 ++-- src/scribe/models/project.py | 6 ++-- src/scribe/models/repo_binding.py | 6 ++-- src/scribe/models/retrieval_log.py | 6 +++- src/scribe/models/rulebook.py | 44 ++++++-------------------- src/scribe/models/share.py | 10 +++--- src/scribe/models/system.py | 6 ++-- src/scribe/models/task_log.py | 6 ++-- src/scribe/models/user.py | 4 +-- 24 files changed, 101 insertions(+), 119 deletions(-) diff --git a/src/scribe/models/api_key.py b/src/scribe/models/api_key.py index 4093faf..181764a 100644 --- a/src/scribe/models/api_key.py +++ b/src/scribe/models/api_key.py @@ -4,7 +4,7 @@ from sqlalchemy import DateTime, ForeignKey, Index, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin +from scribe.models.base import CreatedAtMixin, iso class ApiKey(Base, CreatedAtMixin): @@ -36,7 +36,7 @@ class ApiKey(Base, CreatedAtMixin): "name": self.name, "key_prefix": self.key_prefix, "scope": self.scope, - "last_used_at": self.last_used_at.isoformat() if self.last_used_at else None, - "created_at": self.created_at.isoformat(), - "revoked_at": self.revoked_at.isoformat() if self.revoked_at else None, + "last_used_at": iso(self.last_used_at), + "created_at": iso(self.created_at), + "revoked_at": iso(self.revoked_at), } diff --git a/src/scribe/models/app_log.py b/src/scribe/models/app_log.py index 32c3910..a8b25b4 100644 --- a/src/scribe/models/app_log.py +++ b/src/scribe/models/app_log.py @@ -4,6 +4,7 @@ from sqlalchemy import DateTime, Float, Index, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base +from scribe.models.base import iso class AppLog(Base): @@ -20,6 +21,9 @@ class AppLog(Base): duration_ms: Mapped[float | None] = mapped_column(Float, nullable=True) ip_address: Mapped[str | None] = mapped_column(Text, nullable=True) details: Mapped[str | None] = mapped_column(Text, nullable=True) + # Declared here rather than via CreatedAtMixin on purpose: the composite + # index below orders on `created_at.desc()`, which needs the column object + # in this class body — a mixin's column is not in scope there. created_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) ) @@ -44,5 +48,5 @@ class AppLog(Base): "duration_ms": self.duration_ms, "ip_address": self.ip_address, "details": self.details, - "created_at": self.created_at.isoformat() if self.created_at else None, + "created_at": iso(self.created_at), } diff --git a/src/scribe/models/base.py b/src/scribe/models/base.py index 8715097..42980e0 100644 --- a/src/scribe/models/base.py +++ b/src/scribe/models/base.py @@ -1,9 +1,19 @@ -from datetime import datetime, timezone +from datetime import date, datetime, timezone from sqlalchemy import DateTime, Text from sqlalchemy.orm import Mapped, mapped_column +def iso(value: datetime | date | None) -> str | None: + """ISO-8601 for a payload, None for an unset column. + + Every model's to_dict serialises timestamps through this one helper so a + row read before flush (created_at still None) and a nullable column both + come out as null instead of raising on `.isoformat()`. + """ + return value.isoformat() if value else None + + class SoftDeleteMixin: """Recoverable-delete columns. NULL deleted_at = live row. deleted_batch_id groups rows soft-deleted in one operation so a cascade restores as a unit.""" diff --git a/src/scribe/models/code_shape.py b/src/scribe/models/code_shape.py index 7cb1967..5dc0ab5 100644 --- a/src/scribe/models/code_shape.py +++ b/src/scribe/models/code_shape.py @@ -13,7 +13,7 @@ from sqlalchemy import ( from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso # The classification vocabulary (note 2786). `unclassified` is the default and # THE todo state; every other status is a judgment, stamped with who made it. @@ -153,18 +153,18 @@ class CodeShape(Base, TimestampMixin): "snippet_id": self.snippet_id, "reason": self.reason, "classified_by": self.classified_by, - "classified_at": self.classified_at.isoformat() if self.classified_at else None, + "classified_at": iso(self.classified_at), "first_seen_commit": self.first_seen_commit, "last_seen_commit": self.last_seen_commit, - "vanished_at": self.vanished_at.isoformat() if self.vanished_at else None, + "vanished_at": iso(self.vanished_at), "signature": self.signature, "body_sha": self.body_sha, "proposal": self.proposal, "classified_sha": self.classified_sha, - "recheck_at": self.recheck_at.isoformat() if self.recheck_at else None, + "recheck_at": iso(self.recheck_at), "diverges_from": self.diverges_from, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } @@ -221,5 +221,5 @@ class CodeShapeEvent(Base): "classified_by": self.classified_by, "reason": self.reason, "commit": self.commit, - "at": self.at.isoformat(), + "at": iso(self.at), } diff --git a/src/scribe/models/design_system.py b/src/scribe/models/design_system.py index d2d8570..0b5c29f 100644 --- a/src/scribe/models/design_system.py +++ b/src/scribe/models/design_system.py @@ -20,7 +20,7 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import SoftDeleteMixin, TimestampMixin +from scribe.models.base import SoftDeleteMixin, TimestampMixin, iso class DesignSystem(Base, TimestampMixin, SoftDeleteMixin): @@ -56,8 +56,8 @@ class DesignSystem(Base, TimestampMixin, SoftDeleteMixin): "description": self.description or "", "guidance": self.guidance or "", "parent_id": self.parent_id, - "created_at": self.created_at.isoformat() if self.created_at else None, - "updated_at": self.updated_at.isoformat() if self.updated_at else None, + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } @@ -153,6 +153,6 @@ class DesignToken(Base, TimestampMixin, SoftDeleteMixin): "rationale": self.rationale, "supersedes": self.supersedes or [], "order_index": self.order_index, - "created_at": self.created_at.isoformat() if self.created_at else None, - "updated_at": self.updated_at.isoformat() if self.updated_at else None, + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/forge_connection.py b/src/scribe/models/forge_connection.py index 977f26b..9ecbdee 100644 --- a/src/scribe/models/forge_connection.py +++ b/src/scribe/models/forge_connection.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Integer, Text, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso class ForgeConnection(Base, TimestampMixin): @@ -40,6 +40,6 @@ class ForgeConnection(Base, TimestampMixin): "kind": self.kind, "base_url": self.base_url, "host": self.host, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/group.py b/src/scribe/models/group.py index 1a58834..c10a53d 100644 --- a/src/scribe/models/group.py +++ b/src/scribe/models/group.py @@ -4,7 +4,7 @@ from sqlalchemy import ForeignKey, Integer, Text, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column, relationship from scribe.models import Base -from scribe.models.base import CreatedAtMixin, TimestampMixin +from scribe.models.base import CreatedAtMixin, TimestampMixin, iso class Group(Base, TimestampMixin): @@ -27,8 +27,8 @@ class Group(Base, TimestampMixin): "name": self.name, "description": self.description, "created_by": self.created_by, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } @@ -53,5 +53,5 @@ class GroupMembership(Base, CreatedAtMixin): "group_id": self.group_id, "user_id": self.user_id, "role": self.role, - "created_at": self.created_at.isoformat(), + "created_at": iso(self.created_at), } diff --git a/src/scribe/models/invitation.py b/src/scribe/models/invitation.py index 432267b..55f3c2a 100644 --- a/src/scribe/models/invitation.py +++ b/src/scribe/models/invitation.py @@ -4,9 +4,10 @@ from sqlalchemy import Boolean, DateTime, ForeignKey, Index, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base +from scribe.models.base import CreatedAtMixin -class InvitationToken(Base): +class InvitationToken(Base, CreatedAtMixin): __tablename__ = "invitation_tokens" id: Mapped[int] = mapped_column(primary_key=True) @@ -15,9 +16,6 @@ class InvitationToken(Base): invited_by: Mapped[int] = mapped_column(ForeignKey("users.id", ondelete="CASCADE"), nullable=False) expires_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) used: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) __table_args__ = ( Index("ix_invitation_tokens_token_hash", "token_hash"), diff --git a/src/scribe/models/milestone.py b/src/scribe/models/milestone.py index a449204..72e7f39 100644 --- a/src/scribe/models/milestone.py +++ b/src/scribe/models/milestone.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin, SoftDeleteMixin +from scribe.models.base import SoftDeleteMixin, TimestampMixin, iso class Milestone(Base, TimestampMixin, SoftDeleteMixin): @@ -30,6 +30,6 @@ class Milestone(Base, TimestampMixin, SoftDeleteMixin): "body": self.body, "status": self.status, "order_index": self.order_index, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/note.py b/src/scribe/models/note.py index 2ba05f3..f09b16e 100644 --- a/src/scribe/models/note.py +++ b/src/scribe/models/note.py @@ -6,7 +6,7 @@ from sqlalchemy.dialects.postgresql import ARRAY, JSONB from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin, SoftDeleteMixin +from scribe.models.base import SoftDeleteMixin, TimestampMixin, iso class TaskStatus(str, enum.Enum): @@ -105,18 +105,14 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): "milestone_id": self.milestone_id, "status": self.status, "priority": self.priority, - "due_date": self.due_date.isoformat() if self.due_date else None, - "started_at": self.started_at.isoformat() if self.started_at else None, - "completed_at": self.completed_at.isoformat() if self.completed_at else None, + "due_date": iso(self.due_date), + "started_at": iso(self.started_at), + "completed_at": iso(self.completed_at), "recurrence_rule": self.recurrence_rule, - "recurrence_next_spawn_at": ( - self.recurrence_next_spawn_at.isoformat() - if self.recurrence_next_spawn_at - else None - ), + "recurrence_next_spawn_at": iso(self.recurrence_next_spawn_at), "is_task": self.is_task, "note_type": self.note_type or "note", "task_kind": self.task_kind, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/note_draft.py b/src/scribe/models/note_draft.py index 5ffbc1e..898cb80 100644 --- a/src/scribe/models/note_draft.py +++ b/src/scribe/models/note_draft.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso class NoteDraft(Base, TimestampMixin): @@ -25,6 +25,6 @@ class NoteDraft(Base, TimestampMixin): "original_body": self.original_body, "instruction": self.instruction, "scope": self.scope, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/note_supersession.py b/src/scribe/models/note_supersession.py index 127ccec..635f569 100644 --- a/src/scribe/models/note_supersession.py +++ b/src/scribe/models/note_supersession.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Index, Integer, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin +from scribe.models.base import CreatedAtMixin, iso class NoteSupersession(Base, CreatedAtMixin): @@ -66,5 +66,5 @@ class NoteSupersession(Base, CreatedAtMixin): "id": self.id, "superseder_id": self.superseder_id, "superseded_id": self.superseded_id, - "created_at": self.created_at.isoformat() if self.created_at else None, + "created_at": iso(self.created_at), } diff --git a/src/scribe/models/note_usage.py b/src/scribe/models/note_usage.py index 85eb5d4..433cccb 100644 --- a/src/scribe/models/note_usage.py +++ b/src/scribe/models/note_usage.py @@ -1,15 +1,14 @@ -from datetime import datetime, timezone - -from sqlalchemy import DateTime, Index, Integer, Text +from sqlalchemy import Index, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base +from scribe.models.base import CreatedAtMixin, iso SURFACED = "surfaced" PULLED = "pulled" -class NoteUsageEvent(Base): +class NoteUsageEvent(Base, CreatedAtMixin): """One row per time a note was SURFACED to the agent, or PULLED in full. Answers the question RetrievalLog cannot: not "what did the ranker return @@ -44,9 +43,6 @@ class NoteUsageEvent(Base): __tablename__ = "note_usage_events" id: Mapped[int] = mapped_column(primary_key=True) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) user_id: Mapped[int | None] = mapped_column(Integer, nullable=True) note_id: Mapped[int] = mapped_column(Integer, nullable=False) # 'surfaced' | 'pulled' @@ -81,7 +77,7 @@ class NoteUsageEvent(Base): def to_dict(self) -> dict: return { "id": self.id, - "created_at": self.created_at.isoformat() if self.created_at else None, + "created_at": iso(self.created_at), "user_id": self.user_id, "note_id": self.note_id, "event": self.event, diff --git a/src/scribe/models/note_version.py b/src/scribe/models/note_version.py index 86cd62f..04e395d 100644 --- a/src/scribe/models/note_version.py +++ b/src/scribe/models/note_version.py @@ -2,7 +2,7 @@ from sqlalchemy import ARRAY, ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin +from scribe.models.base import CreatedAtMixin, iso class NoteVersion(Base, CreatedAtMixin): @@ -26,7 +26,7 @@ class NoteVersion(Base, CreatedAtMixin): "tags": self.tags or [], "pin_kind": self.pin_kind, "pin_label": self.pin_label, - "created_at": self.created_at.isoformat(), + "created_at": iso(self.created_at), } if include_body: d["body"] = self.body diff --git a/src/scribe/models/notification.py b/src/scribe/models/notification.py index 05a5c35..ae455b1 100644 --- a/src/scribe/models/notification.py +++ b/src/scribe/models/notification.py @@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin +from scribe.models.base import CreatedAtMixin, iso class Notification(Base, CreatedAtMixin): @@ -26,6 +26,6 @@ class Notification(Base, CreatedAtMixin): "user_id": self.user_id, "type": self.type, "payload": self.payload, - "read_at": self.read_at.isoformat() if self.read_at else None, - "created_at": self.created_at.isoformat(), + "read_at": iso(self.read_at), + "created_at": iso(self.created_at), } diff --git a/src/scribe/models/password_reset.py b/src/scribe/models/password_reset.py index 70add83..811902c 100644 --- a/src/scribe/models/password_reset.py +++ b/src/scribe/models/password_reset.py @@ -4,9 +4,10 @@ from sqlalchemy import Boolean, DateTime, ForeignKey, Index, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base +from scribe.models.base import CreatedAtMixin -class PasswordResetToken(Base): +class PasswordResetToken(Base, CreatedAtMixin): __tablename__ = "password_reset_tokens" id: Mapped[int] = mapped_column(primary_key=True) @@ -14,9 +15,6 @@ class PasswordResetToken(Base): token_hash: Mapped[str] = mapped_column(Text, nullable=False, unique=True) expires_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) used: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) __table_args__ = ( Index("ix_password_reset_tokens_token_hash", "token_hash"), diff --git a/src/scribe/models/project.py b/src/scribe/models/project.py index 1d37c6e..3297106 100644 --- a/src/scribe/models/project.py +++ b/src/scribe/models/project.py @@ -2,7 +2,7 @@ import enum from sqlalchemy import BigInteger, ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin, SoftDeleteMixin +from scribe.models.base import SoftDeleteMixin, TimestampMixin, iso class ProjectStatus(str, enum.Enum): @@ -48,6 +48,6 @@ class Project(Base, TimestampMixin, SoftDeleteMixin): "color": self.color, "design_system_id": self.design_system_id, "forge_connection_id": self.forge_connection_id, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/repo_binding.py b/src/scribe/models/repo_binding.py index b5e8039..8686bcf 100644 --- a/src/scribe/models/repo_binding.py +++ b/src/scribe/models/repo_binding.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Integer, Text, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso class RepoBinding(Base, TimestampMixin): @@ -35,6 +35,6 @@ class RepoBinding(Base, TimestampMixin): "user_id": self.user_id, "project_id": self.project_id, "repo_key": self.repo_key, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/retrieval_log.py b/src/scribe/models/retrieval_log.py index 6630364..ba49df4 100644 --- a/src/scribe/models/retrieval_log.py +++ b/src/scribe/models/retrieval_log.py @@ -5,6 +5,7 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base +from scribe.models.base import iso class RetrievalLog(Base): @@ -23,6 +24,9 @@ class RetrievalLog(Base): __tablename__ = "retrieval_logs" id: Mapped[int] = mapped_column(primary_key=True) + # Declared here rather than via CreatedAtMixin on purpose: the composite + # index below orders on `created_at.desc()`, which needs the column object + # in this class body — a mixin's column is not in scope there. created_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) ) @@ -54,7 +58,7 @@ class RetrievalLog(Base): def to_dict(self) -> dict: return { "id": self.id, - "created_at": self.created_at.isoformat() if self.created_at else None, + "created_at": iso(self.created_at), "user_id": self.user_id, "source": self.source, "query": self.query, diff --git a/src/scribe/models/rulebook.py b/src/scribe/models/rulebook.py index 6b0813e..7a1cdf5 100644 --- a/src/scribe/models/rulebook.py +++ b/src/scribe/models/rulebook.py @@ -4,10 +4,10 @@ from sqlalchemy import BigInteger, Boolean, Column, DateTime, ForeignKey, Index, from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import SoftDeleteMixin +from scribe.models.base import SoftDeleteMixin, TimestampMixin, iso -class Rulebook(Base, SoftDeleteMixin): +class Rulebook(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "rulebooks" id: Mapped[int] = mapped_column(BigInteger, primary_key=True) @@ -19,14 +19,6 @@ class Rulebook(Base, SoftDeleteMixin): always_on: Mapped[bool] = mapped_column( Boolean, default=False, nullable=False, server_default="false" ) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) - updated_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), - default=lambda: datetime.now(timezone.utc), - onupdate=lambda: datetime.now(timezone.utc), - ) def to_dict(self) -> dict: return { @@ -35,12 +27,12 @@ class Rulebook(Base, SoftDeleteMixin): "title": self.title, "description": self.description or "", "always_on": self.always_on, - "created_at": self.created_at.isoformat() if self.created_at else None, - "updated_at": self.updated_at.isoformat() if self.updated_at else None, + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } -class RulebookTopic(Base, SoftDeleteMixin): +class RulebookTopic(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "rulebook_topics" # Partial unique: a title is unique among LIVE topics in a rulebook, so a # trashed topic doesn't block recreating/restoring the same title. @@ -58,14 +50,6 @@ class RulebookTopic(Base, SoftDeleteMixin): title: Mapped[str] = mapped_column(Text) description: Mapped[str | None] = mapped_column(Text, nullable=True) order_index: Mapped[int] = mapped_column(Integer, default=0) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) - updated_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), - default=lambda: datetime.now(timezone.utc), - onupdate=lambda: datetime.now(timezone.utc), - ) def to_dict(self) -> dict: return { @@ -74,12 +58,12 @@ class RulebookTopic(Base, SoftDeleteMixin): "title": self.title, "description": self.description or "", "order_index": self.order_index, - "created_at": self.created_at.isoformat() if self.created_at else None, - "updated_at": self.updated_at.isoformat() if self.updated_at else None, + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } -class Rule(Base, SoftDeleteMixin): +class Rule(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "rules" # Partial unique: title unique among LIVE rules in a topic (soft-deleted # rules don't block recreating/restoring the same title). @@ -109,14 +93,6 @@ class Rule(Base, SoftDeleteMixin): why: Mapped[str | None] = mapped_column(Text, nullable=True) how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) order_index: Mapped[int] = mapped_column(Integer, default=0) - created_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) - ) - updated_at: Mapped[datetime] = mapped_column( - DateTime(timezone=True), - default=lambda: datetime.now(timezone.utc), - onupdate=lambda: datetime.now(timezone.utc), - ) def to_dict(self) -> dict: return { @@ -128,8 +104,8 @@ class Rule(Base, SoftDeleteMixin): "why": self.why or "", "how_to_apply": self.how_to_apply or "", "order_index": self.order_index, - "created_at": self.created_at.isoformat() if self.created_at else None, - "updated_at": self.updated_at.isoformat() if self.updated_at else None, + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/share.py b/src/scribe/models/share.py index 522a728..038e6ee 100644 --- a/src/scribe/models/share.py +++ b/src/scribe/models/share.py @@ -2,7 +2,7 @@ from sqlalchemy import CheckConstraint, ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso class ProjectShare(Base, TimestampMixin): @@ -37,8 +37,8 @@ class ProjectShare(Base, TimestampMixin): "shared_with_group_id": self.shared_with_group_id, "permission": self.permission, "invited_by": self.invited_by, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } @@ -74,6 +74,6 @@ class NoteShare(Base, TimestampMixin): "shared_with_group_id": self.shared_with_group_id, "permission": self.permission, "invited_by": self.invited_by, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/system.py b/src/scribe/models/system.py index a47416e..1def12f 100644 --- a/src/scribe/models/system.py +++ b/src/scribe/models/system.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Index, Integer, Text, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin, TimestampMixin, SoftDeleteMixin +from scribe.models.base import CreatedAtMixin, SoftDeleteMixin, TimestampMixin, iso class System(Base, TimestampMixin, SoftDeleteMixin): @@ -44,8 +44,8 @@ class System(Base, TimestampMixin, SoftDeleteMixin): "color": self.color, "status": self.status, "order_index": self.order_index, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/task_log.py b/src/scribe/models/task_log.py index 0866ea8..2820613 100644 --- a/src/scribe/models/task_log.py +++ b/src/scribe/models/task_log.py @@ -2,7 +2,7 @@ from sqlalchemy import ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import TimestampMixin +from scribe.models.base import TimestampMixin, iso class TaskLog(Base, TimestampMixin): @@ -21,6 +21,6 @@ class TaskLog(Base, TimestampMixin): "user_id": self.user_id, "content": self.content, "duration_minutes": self.duration_minutes, - "created_at": self.created_at.isoformat(), - "updated_at": self.updated_at.isoformat(), + "created_at": iso(self.created_at), + "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/user.py b/src/scribe/models/user.py index 6a295cf..ea98372 100644 --- a/src/scribe/models/user.py +++ b/src/scribe/models/user.py @@ -2,7 +2,7 @@ from sqlalchemy import Index, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base -from scribe.models.base import CreatedAtMixin +from scribe.models.base import CreatedAtMixin, iso class User(Base, CreatedAtMixin): @@ -26,6 +26,6 @@ class User(Base, CreatedAtMixin): "username": self.username, "email": self.email, "role": self.role, - "created_at": self.created_at.isoformat(), + "created_at": iso(self.created_at), "has_password": self.password_hash is not None, } From c211e12b61761d6389472b5057d93c8070073bb4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:20:36 -0400 Subject: [PATCH 4/8] refactor(mcp): one rules_payload() for every surface that hands rules to an agent; drop the dead bearer resolver (#2828, milestone 296 area 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the 16 tool modules against each other: the six-key applicable-rules block (applicable_rules, applicable_rules_truncated, subscribed_rulebooks, project_rules, suppressed_rules, suppressed_topics) was hand-built in five places — enter_project, get_project, get_task (legacy plans), get_milestone (three of the six) and services/planning.start_planning. rulebooks_svc. rules_payload() is now the one place that names them; get_milestone gains the three it lacked, so every rules-carrying payload reads the same. list_rules / list_always_on_rules share _rule_summary. mcp/auth.resolve_bearer_to_user_id duplicated resolve_bearer's parsing and had no product caller (only its own tests) — removed; the tests now exercise resolve_bearer. Co-Authored-By: Claude Fable 5 --- src/scribe/mcp/auth.py | 14 -------------- src/scribe/mcp/tools/milestones.py | 4 +--- src/scribe/mcp/tools/projects.py | 14 ++------------ src/scribe/mcp/tools/rulebooks.py | 28 ++++++++-------------------- src/scribe/mcp/tools/tasks.py | 7 +------ src/scribe/services/planning.py | 7 +------ src/scribe/services/rulebooks.py | 19 +++++++++++++++++++ tests/test_mcp_auth.py | 24 +++++++++++++----------- 8 files changed, 45 insertions(+), 72 deletions(-) diff --git a/src/scribe/mcp/auth.py b/src/scribe/mcp/auth.py index 959d1f3..69872eb 100644 --- a/src/scribe/mcp/auth.py +++ b/src/scribe/mcp/auth.py @@ -4,20 +4,6 @@ from __future__ import annotations from scribe.services.api_keys import lookup_key -async def resolve_bearer_to_user_id(auth_header: str | None) -> int | None: - """Parse an `Authorization: Bearer ` header and return the user_id. - - Returns None if the header is missing, malformed, or the token is invalid - or revoked. The underlying lookup_key already updates last_used_at on hit. - """ - if not auth_header or not auth_header.startswith("Bearer "): - return None - raw_token = auth_header[len("Bearer "):].strip() - if not raw_token: - return None - api_key = await lookup_key(raw_token) - return api_key.user_id if api_key else None - async def resolve_bearer(auth_header: str | None) -> tuple[int, str] | None: """Resolve a Bearer token to (user_id, scope). diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 26463e6..5faf873 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -57,9 +57,7 @@ async def get_milestone(milestone_id: int) -> dict: return { "milestone": out, "steps": [t.to_dict() for t in steps], - "applicable_rules": applicable["rules"], - "subscribed_rulebooks": applicable["subscribed_rulebooks"], - "applicable_rules_truncated": applicable["truncated"], + **rulebooks_svc.rules_payload(applicable), } diff --git a/src/scribe/mcp/tools/projects.py b/src/scribe/mcp/tools/projects.py index 490ca52..f74b3ed 100644 --- a/src/scribe/mcp/tools/projects.py +++ b/src/scribe/mcp/tools/projects.py @@ -192,12 +192,7 @@ async def enter_project(project_id: int) -> dict: ], "design_system": design_system, "milestone_summary": milestone_summary, - "applicable_rules": applicable["rules"], - "project_rules": applicable.get("project_rules", []), - "suppressed_rules": applicable.get("suppressed_rules", []), - "suppressed_topics": applicable.get("suppressed_topics", []), - "subscribed_rulebooks": applicable["subscribed_rulebooks"], - "applicable_rules_truncated": applicable["truncated"], + **rulebooks_svc.rules_payload(applicable), "open_tasks": [ { "id": t.id, "title": t.title, "status": t.status, @@ -239,12 +234,7 @@ async def get_project(project_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=project_id, user_id=uid, ) - data["applicable_rules"] = applicable["rules"] - data["applicable_rules_truncated"] = applicable["truncated"] - data["subscribed_rulebooks"] = applicable["subscribed_rulebooks"] - data["project_rules"] = applicable.get("project_rules", []) - data["suppressed_rules"] = applicable.get("suppressed_rules", []) - data["suppressed_topics"] = applicable.get("suppressed_topics", []) + data.update(rulebooks_svc.rules_payload(applicable)) return data diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 1ddf1b3..15c89b7 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -193,6 +193,12 @@ async def delete_topic(topic_id: int, confirmed: bool = False) -> dict: # ── Rule CRUD ────────────────────────────────────────────────────────── +def _rule_summary(r) -> dict: + """The list-row shape for a rule: what an agent needs to APPLY it. The + full record (why, how_to_apply, timestamps) is get_rule's job.""" + return {"id": r.id, "title": r.title, "statement": r.statement, "topic_id": r.topic_id} + + async def list_rules( rulebook_id: int = 0, topic_id: int = 0, project_id: int = 0, ) -> dict: @@ -213,16 +219,7 @@ async def list_rules( topic_id=topic_id or None, project_id=project_id or None, ) - return { - "rules": [ - { - "id": r.id, "title": r.title, "statement": r.statement, - "topic_id": r.topic_id, - } - for r in rows - ], - "total": len(rows), - } + return {"rules": [_rule_summary(r) for r in rows], "total": len(rows)} async def list_always_on_rules() -> dict: @@ -235,16 +232,7 @@ async def list_always_on_rules() -> dict: """ uid = current_user_id() rules = await rulebooks_svc.list_always_on_rules(uid) - return { - "rules": [ - { - "id": r.id, "title": r.title, "statement": r.statement, - "topic_id": r.topic_id, - } - for r in rules - ], - "total": len(rules), - } + return {"rules": [_rule_summary(r) for r in rules], "total": len(rules)} async def get_rule(rule_id: int) -> dict: diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index bc1e6fa..1c30055 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -97,12 +97,7 @@ async def get_task(task_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=note.project_id, user_id=uid, ) - data["applicable_rules"] = applicable["rules"] - data["subscribed_rulebooks"] = applicable["subscribed_rulebooks"] - data["applicable_rules_truncated"] = applicable["truncated"] - data["project_rules"] = applicable.get("project_rules", []) - data["suppressed_rules"] = applicable.get("suppressed_rules", []) - data["suppressed_topics"] = applicable.get("suppressed_topics", []) + data.update(rulebooks_svc.rules_payload(applicable)) data.update(await access_svc.describe_provenance(uid, note)) # Same reasoning as get_note's record_pulled, and this is the tool where it # matters MOST: auto-inject ranks kind-blind over a corpus that is diff --git a/src/scribe/services/planning.py b/src/scribe/services/planning.py index c2634b6..4f58c97 100644 --- a/src/scribe/services/planning.py +++ b/src/scribe/services/planning.py @@ -60,12 +60,7 @@ async def start_planning(user_id: int, project_id: int, title: str) -> dict: return { "milestone": milestone.to_dict(), - "applicable_rules": applicable["rules"], - "subscribed_rulebooks": applicable["subscribed_rulebooks"], - "applicable_rules_truncated": applicable["truncated"], - "project_rules": applicable.get("project_rules", []), - "suppressed_rules": applicable.get("suppressed_rules", []), - "suppressed_topics": applicable.get("suppressed_topics", []), + **rulebooks_svc.rules_payload(applicable), "project_goal": getattr(project, "goal", "") or "", "open_task_count": open_count, } diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 18238c6..0517aad 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -779,3 +779,22 @@ async def get_applicable_rules( "truncated": truncated, "subscribed_rulebooks": subscribed_rulebooks, } + + +def rules_payload(applicable: dict) -> dict: + """The caller-facing shape of a get_applicable_rules() result. + + Every surface that hands rules to an agent (enter_project, get_project, + get_milestone, get_task for legacy plans, start_planning) carries the + same six keys under the same names — so a reader learns them once. One + place renames `rules` → `applicable_rules` and `truncated` → + `applicable_rules_truncated`; the tools merge this into their payloads. + """ + return { + "applicable_rules": applicable["rules"], + "applicable_rules_truncated": applicable["truncated"], + "subscribed_rulebooks": applicable["subscribed_rulebooks"], + "project_rules": applicable.get("project_rules", []), + "suppressed_rules": applicable.get("suppressed_rules", []), + "suppressed_topics": applicable.get("suppressed_topics", []), + } diff --git a/tests/test_mcp_auth.py b/tests/test_mcp_auth.py index 6c3a32c..d038107 100644 --- a/tests/test_mcp_auth.py +++ b/tests/test_mcp_auth.py @@ -3,20 +3,20 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from scribe.mcp.auth import resolve_bearer, resolve_bearer_to_user_id +from scribe.mcp.auth import resolve_bearer @pytest.mark.asyncio async def test_resolve_bearer_missing_header_returns_none(): - assert await resolve_bearer_to_user_id(None) is None + assert await resolve_bearer(None) is None @pytest.mark.asyncio async def test_resolve_bearer_malformed_header_returns_none(): - assert await resolve_bearer_to_user_id("Token abc") is None - assert await resolve_bearer_to_user_id("Bearer") is None - assert await resolve_bearer_to_user_id("Bearer ") is None - assert await resolve_bearer_to_user_id("") is None + assert await resolve_bearer("Token abc") is None + assert await resolve_bearer("Bearer") is None + assert await resolve_bearer("Bearer ") is None + assert await resolve_bearer("") is None @pytest.mark.asyncio @@ -25,19 +25,20 @@ async def test_resolve_bearer_unknown_token_returns_none(): "scribe.mcp.auth.lookup_key", AsyncMock(return_value=None), ): - assert await resolve_bearer_to_user_id("Bearer fmcp_doesnotexist") is None + assert await resolve_bearer("Bearer fmcp_doesnotexist") is None @pytest.mark.asyncio async def test_resolve_bearer_valid_token_returns_user_id(): fake_key = MagicMock() fake_key.user_id = 42 + fake_key.scope = "write" with patch( "scribe.mcp.auth.lookup_key", AsyncMock(return_value=fake_key), ): - uid = await resolve_bearer_to_user_id("Bearer fmcp_validkey") - assert uid == 42 + uid, scope = await resolve_bearer("Bearer fmcp_validkey") + assert (uid, scope) == (42, "write") @pytest.mark.asyncio @@ -45,13 +46,14 @@ async def test_resolve_bearer_calls_lookup_with_stripped_token(): """The Bearer prefix and any trailing whitespace must be stripped before lookup.""" fake_key = MagicMock() fake_key.user_id = 1 + fake_key.scope = "write" mock_lookup = AsyncMock(return_value=fake_key) with patch("scribe.mcp.auth.lookup_key", mock_lookup): - await resolve_bearer_to_user_id("Bearer fmcp_abc123 ") + await resolve_bearer("Bearer fmcp_abc123 ") mock_lookup.assert_awaited_once_with("fmcp_abc123") -# ── resolve_bearer (user_id + scope) ──────────────────────────────────── +# ── scope ─────────────────────────────────────────────────────────────── @pytest.mark.asyncio async def test_resolve_bearer_returns_user_id_and_scope(): From 64c641ce80dc8c491885c73ef99bed6d6db8ce1e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:23:47 -0400 Subject: [PATCH 5/8] refactor(routes): one supersession seam for REST and MCP; PUT/PATCH notes share a handler; shared mask/not-found/caller helpers (#2829, milestone 296 area 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the 28 route modules against each other and against the MCP tools: - routes/notes.py carried a PUT and a PATCH handler that were the same function minus the supersedes contract on one of them — one handler now serves both verbs, so both carry it. - The two _attach_supersession copies (REST + MCP) become supersession_svc.attach_relations(uid, note_id, data, hint=) — the seam the two surfaces must agree through; only the agent surface adds the one-sentence reading hint. - Three local _uid() wrappers over g.user.id → scribe.auth.get_current_user_id like every other module; design_systems' private _not_found → routes.utils. not_found; the four "********" literals → settings_svc.SECRET_MASK with the read/write contract written once. - routes/plugin.py: the project_id/repo resolution block and the comma-separated id parse were copied into three endpoints — _project_scope() and _int_list() now. Co-Authored-By: Claude Fable 5 --- src/scribe/mcp/tools/notes.py | 30 ++--------- src/scribe/mcp/tools/systems.py | 2 +- src/scribe/routes/admin.py | 12 ++--- src/scribe/routes/design_systems.py | 61 ++++++++++----------- src/scribe/routes/notes.py | 62 +++------------------ src/scribe/routes/plugin.py | 84 ++++++++++++----------------- src/scribe/routes/rulebooks.py | 53 +++++++++--------- src/scribe/routes/settings.py | 11 ++-- src/scribe/routes/trash.py | 13 ++--- src/scribe/services/settings.py | 7 +++ src/scribe/services/supersession.py | 32 +++++++++++ 11 files changed, 155 insertions(+), 212 deletions(-) diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index f4e2dc9..223ac18 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -62,30 +62,6 @@ async def list_notes( return {"notes": [n.to_dict() for n in rows], "total": total} -async def _attach_supersession(uid: int, note_id: int, data: dict) -> None: - """Add both directions of the supersession relation to a note payload. - - Both, because they answer different questions and only one of them is - obvious. `supersedes` is what the author claimed. `superseded_by` is what a - READER needs and what the note itself cannot know — a stale record handed - over without that marker gets acted on confidently, which is worse than - never surfacing it. - - Omitted entirely when empty, so an ordinary note's payload doesn't grow two - permanently-empty lists. A field that always says nothing trains readers to - skip fields, which is the lesson `consolidated_at` cost us (#2483). - """ - rel = await supersession_svc.get_relations(uid, note_id) - if rel["supersedes"]: - data["supersedes"] = rel["supersedes"] - if rel["superseded_by"]: - data["superseded_by"] = rel["superseded_by"] - data["superseded_note"] = ( - "A later note claims to bring this up to date — see superseded_by. " - "Read this as what was true when written, and check the newer one " - "before acting on it." - ) - async def get_note(note_id: int) -> dict: """Fetch the full content of a single Scribe note by its ID. @@ -113,7 +89,7 @@ async def get_note(note_id: int) -> dict: # snippets would leave those permanently at zero pulls and make them look # like dead weight next to snippets that merely had a counter (#2085). record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_note") - await _attach_supersession(uid, note_id, out) + await supersession_svc.attach_relations(uid, note_id, out, hint=True) await systems_tools.attach_systems( uid, getattr(note, "user_id", uid) or uid, out, note.id, note.project_id ) @@ -186,7 +162,7 @@ async def create_note( raise ValueError(str(exc)) from exc data = note.to_dict() await systems_tools.attach_systems(uid, uid, data, note.id, project_id or None) - await _attach_supersession(uid, note.id, data) + await supersession_svc.attach_relations(uid, note.id, data, hint=True) return data @@ -237,7 +213,7 @@ async def update_note( await systems_tools.attach_systems( uid, getattr(note, "user_id", uid) or uid, data, note_id, note.project_id ) - await _attach_supersession(uid, note_id, data) + await supersession_svc.attach_relations(uid, note_id, data, hint=True) return data diff --git a/src/scribe/mcp/tools/systems.py b/src/scribe/mcp/tools/systems.py index acaa9a0..2ccf049 100644 --- a/src/scribe/mcp/tools/systems.py +++ b/src/scribe/mcp/tools/systems.py @@ -134,7 +134,7 @@ async def attach_systems( tagged record shows its areas (the touching-a-System reflex needs the affiliation visible on read, not just settable on write), an untagged project record carries the question instead. Neither field is ever - attached empty (same reasoning as notes._attach_supersession / #2483 — a + attached empty (same reasoning as supersession_svc.attach_relations / #2483 — a field that always says nothing trains readers to skip fields). The hint goes only to the record's owner: tagging someone else's record in someone else's project is not the caller's call to make. Fail-open — decoration diff --git a/src/scribe/routes/admin.py b/src/scribe/routes/admin.py index ec5fcfe..9edf548 100644 --- a/src/scribe/routes/admin.py +++ b/src/scribe/routes/admin.py @@ -22,6 +22,7 @@ from scribe.services.email import SMTP_SETTING_KEYS, get_base_url, get_smtp_conf from scribe.services.logging import get_logs, get_log_stats, log_audit from scribe.services.notifications import send_invitation_email from scribe.services.settings import ( + SECRET_MASK, get_admin_setting, set_admin_setting, set_setting, @@ -116,7 +117,7 @@ async def get_smtp(): config = await get_smtp_config() # Mask password if config.get("smtp_password"): - config["smtp_password"] = "********" + config["smtp_password"] = SECRET_MASK return jsonify(config) @@ -130,7 +131,7 @@ async def update_smtp(): for key in SMTP_SETTING_KEYS: if key in data: # Skip password if it's the mask placeholder - if key == "smtp_password" and data[key] == "********": + if key == "smtp_password" and data[key] == SECRET_MASK: continue settings_to_save[key] = str(data[key]) @@ -157,9 +158,6 @@ async def test_smtp(): return jsonify({"error": str(e)}), 500 -_TOKEN_MASK = "********" - - # The forge CONFIG moved to per-user keyring rows (#2778, Settings → Git # forges); what stays admin is the webhook secret, because the push endpoint # is one URL per instance and authenticates deliveries, not users. @@ -178,7 +176,7 @@ async def get_forge_webhook_settings(): return jsonify({ # Secrets never leave the server — the smtp_password convention: # masked when set, empty when not. - "webhook_secret": _TOKEN_MASK if webhook_secret else "", + "webhook_secret": SECRET_MASK if webhook_secret else "", }) @@ -192,7 +190,7 @@ async def update_forge_webhook_settings(): webhook_secret = data.get("webhook_secret") # The mask coming back means "unchanged" — the form round-trips what GET # showed it, and storing the mask would silently break the integration. - if webhook_secret is not None and webhook_secret != _TOKEN_MASK: + if webhook_secret is not None and webhook_secret != SECRET_MASK: await set_admin_setting(FORGE_WEBHOOK_SECRET_KEY, str(webhook_secret)) # The secret is deliberately absent from the audit detail. await log_audit( diff --git a/src/scribe/routes/design_systems.py b/src/scribe/routes/design_systems.py index 1bb3628..ceff5ae 100644 --- a/src/scribe/routes/design_systems.py +++ b/src/scribe/routes/design_systems.py @@ -15,9 +15,10 @@ one and returns None for the other: those two IS the intent: distinguishing them would confirm the existence of records the caller may not see. """ -from quart import Blueprint, g, jsonify, request +from quart import Blueprint, jsonify, request -from scribe.auth import login_required +from scribe.auth import get_current_user_id, login_required +from scribe.routes.utils import not_found from scribe.services import design_systems as ds_svc from scribe.services.design_starter_roles import ( DEFAULT_TOKEN_PREFIX, @@ -28,12 +29,6 @@ from scribe.services.design_systems import DesignSystemCycle design_systems_bp = Blueprint("design_systems", __name__, url_prefix="/api") -def _uid() -> int: - return g.user.id - - -def _not_found(what: str = "design system"): - return jsonify({"error": f"{what} not found"}), 404 # ── Design systems ────────────────────────────────────────────────────── @@ -43,7 +38,7 @@ def _not_found(what: str = "design system"): async def list_design_systems(): """The caller's design systems. An empty list is the ordinary state for an install that has never made one, not an error.""" - rows = await ds_svc.list_design_systems(_uid()) + rows = await ds_svc.list_design_systems(get_current_user_id()) return jsonify({"design_systems": [s.to_dict() for s in rows]}) @@ -55,7 +50,7 @@ async def create_design_system(): if not title: return jsonify({"error": "title is required"}), 400 system = await ds_svc.create_design_system( - user_id=_uid(), + user_id=get_current_user_id(), title=title, description=data.get("description") or None, guidance=data.get("guidance") or None, @@ -84,9 +79,9 @@ async def list_starter_role_groups(): @design_systems_bp.get("/design-systems/") @login_required async def get_design_system(design_system_id: int): - system = await ds_svc.get_design_system(_uid(), design_system_id) + system = await ds_svc.get_design_system(get_current_user_id(), design_system_id) if system is None: - return _not_found() + return not_found("Design system") return jsonify(system.to_dict()) @@ -102,19 +97,19 @@ async def update_design_system(design_system_id: int): if "parent_id" in data: fields["parent_id"] = data["parent_id"] try: - system = await ds_svc.update_design_system(_uid(), design_system_id, **fields) + system = await ds_svc.update_design_system(get_current_user_id(), design_system_id, **fields) except DesignSystemCycle as exc: return jsonify({"error": str(exc)}), 400 if system is None: - return _not_found() + return not_found("Design system") return jsonify(system.to_dict()) @design_systems_bp.delete("/design-systems/") @login_required async def delete_design_system(design_system_id: int): - if not await ds_svc.delete_design_system(_uid(), design_system_id): - return _not_found() + if not await ds_svc.delete_design_system(get_current_user_id(), design_system_id): + return not_found("Design system") return "", 204 @@ -127,9 +122,9 @@ async def resolve_design_system(design_system_id: int): this returns what it ends up being. Both are real questions and answering only one would make the other a client-side computation. """ - resolved = await ds_svc.resolve_design_system(_uid(), design_system_id) + resolved = await ds_svc.resolve_design_system(get_current_user_id(), design_system_id) if resolved is None: - return _not_found() + return not_found("Design system") return jsonify({ "design_system_id": design_system_id, "tokens": [t.to_dict() for t in resolved], @@ -149,9 +144,9 @@ async def get_design_system_stylesheet(design_system_id: int): `:root`, so the generator takes it as a parameter. """ root = (request.args.get("root") or ":root").strip() or ":root" - result = await ds_svc.stylesheet_for_system(_uid(), design_system_id, root) + result = await ds_svc.stylesheet_for_system(get_current_user_id(), design_system_id, root) if result is None: - return _not_found() + return not_found("Design system") if request.args.get("format") == "css": return result["css"], 200, {"Content-Type": "text/css; charset=utf-8"} return jsonify(result) @@ -168,10 +163,10 @@ async def check_snippets_against_system(design_system_id: int): """ project_id = request.args.get("project_id", type=int) or 0 result = await ds_svc.check_snippets_against_system( - _uid(), design_system_id, project_id + get_current_user_id(), design_system_id, project_id ) if result is None: - return _not_found() + return not_found("Design system") return jsonify(result) @@ -181,9 +176,9 @@ async def check_snippets_against_system(design_system_id: int): @login_required async def list_design_tokens(design_system_id: int): """This system's OWN tokens — its override set, not its effective set.""" - if await ds_svc.get_design_system(_uid(), design_system_id) is None: - return _not_found() - rows = await ds_svc.list_tokens(_uid(), design_system_id) + if await ds_svc.get_design_system(get_current_user_id(), design_system_id) is None: + return not_found("Design system") + rows = await ds_svc.list_tokens(get_current_user_id(), design_system_id) return jsonify({"tokens": [t.to_dict() for t in rows]}) @@ -195,7 +190,7 @@ async def create_design_token(design_system_id: int): if not name: return jsonify({"error": "name is required"}), 400 token = await ds_svc.create_token( - user_id=_uid(), + user_id=get_current_user_id(), design_system_id=design_system_id, name=name, value_by_mode=data.get("value_by_mode"), @@ -206,7 +201,7 @@ async def create_design_token(design_system_id: int): order_index=data.get("order_index") or 0, ) if token is None: - return _not_found() + return not_found("Design system") return jsonify(token.to_dict()), 201 @@ -221,17 +216,17 @@ async def update_design_token(token_id: int): "supersedes", "order_index", ) } - token = await ds_svc.update_token(_uid(), token_id, **fields) + token = await ds_svc.update_token(get_current_user_id(), token_id, **fields) if token is None: - return _not_found("design token") + return not_found("Design token") return jsonify(token.to_dict()) @design_systems_bp.delete("/design-tokens/") @login_required async def delete_design_token(token_id: int): - if not await ds_svc.delete_token(_uid(), token_id): - return _not_found("design token") + if not await ds_svc.delete_token(get_current_user_id(), token_id): + return not_found("Design token") return "", 204 @@ -247,9 +242,9 @@ async def set_project_design_system(project_id: int): """ data = await request.get_json() or {} ok = await ds_svc.set_project_design_system( - _uid(), project_id, data.get("design_system_id") + get_current_user_id(), project_id, data.get("design_system_id") ) if not ok: - return _not_found("project or design system") + return not_found("Project or design system") return jsonify({"project_id": project_id, "design_system_id": data.get("design_system_id")}) diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index cb81507..17a13ad 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -26,22 +26,6 @@ from scribe.services import dedup as dedup_svc from scribe.services import supersession as supersession_svc from scribe.services.note_usage import record_pulled - -async def _attach_supersession(uid: int, note_id: int, data: dict) -> None: - """Both directions of the supersession relation on a note payload. - - Mirrors the MCP helper of the same name — the two surfaces must agree about - what a note's payload says, or the web UI and the agent would disagree about - whether a record is current. - - Omitted when empty: a field that always says nothing trains readers to skip - fields, which is what `consolidated_at` cost (#2483). - """ - rel = await supersession_svc.get_relations(uid, note_id) - if rel["supersedes"]: - data["supersedes"] = rel["supersedes"] - if rel["superseded_by"]: - data["superseded_by"] = rel["superseded_by"] from scribe.services.note_versions import list_versions, get_version logger = logging.getLogger(__name__) @@ -142,7 +126,7 @@ async def create_note_route(): # may not write the target. The note itself was created. return jsonify({"error": str(exc), "note": note.to_dict()}), 403 out = note.to_dict() - await _attach_supersession(uid, note.id, out) + await supersession_svc.attach_relations(uid, note.id, out) return jsonify(out), 201 @@ -241,13 +225,17 @@ async def get_note_route(note_id: int): # injected line useful?" is answered by agent pulls alone, and a human # clicking a link would inflate exactly the number #1038 and #2085 gate on. record_pulled(user_id=uid, note_id=note_id, source="rest_note") - await _attach_supersession(uid, note_id, data) + await supersession_svc.attach_relations(uid, note_id, data) return jsonify(data) -@notes_bp.route("/", methods=["PUT"]) +@notes_bp.route("/", methods=["PUT", "PATCH"]) @login_required async def update_note_route(note_id: int): + """Partial update — only the keys present in the payload change. PUT and + PATCH are the same handler on purpose: the form sends the field set it + edited, and the two verbs used to be two near-identical copies of this + function that drifted (one carried the supersedes contract, one did not).""" uid = get_current_user_id() # Share-aware: resolve through the ACL and write as the OWNER, so a shared # editor's save isn't rejected by the owner-scoped update service. @@ -290,44 +278,10 @@ async def update_note_route(note_id: int): except PermissionError as exc: return jsonify({"error": str(exc)}), 403 out = note.to_dict() - await _attach_supersession(uid, note_id, out) + await supersession_svc.attach_relations(uid, note_id, out) return jsonify(out) -@notes_bp.route("/", methods=["PATCH"]) -@login_required -async def patch_note_route(note_id: int): - uid = get_current_user_id() - result = await get_note_for_user(uid, note_id) - if result is None: - return not_found("Note") - note_obj, _ = result - if not await can_write_note(uid, note_id): - return jsonify({"error": "Permission denied"}), 403 - owner_uid = note_obj.user_id - data = await request.get_json() - fields = {} - for key in ("title", "body", "description", "parent_id", "project_id", "milestone_id", "status", "priority", "note_type"): - if key in data: - fields[key] = data[key] - if "due_date" in data: - if data["due_date"]: - result = parse_iso_date(data["due_date"], "due_date") - if isinstance(result, tuple): - return result - fields["due_date"] = result - else: - fields["due_date"] = None - if "tags" in data: - fields["tags"] = data["tags"] - try: - note = await update_note(owner_uid, note_id, **fields) - except ValueError as e: - return jsonify({"error": str(e)}), 400 - if note is None: - return not_found("Note") - return jsonify(note.to_dict()) - @notes_bp.route("/", methods=["DELETE"]) @login_required diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index c66d2e0..71e48b5 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -24,6 +24,35 @@ plugin_bp = Blueprint("plugin", __name__, url_prefix="/api/plugin") _MARKETPLACE_KEY = "plugin_marketplace_url" +def _int_list(raw: str | None) -> list[int]: + """A comma-separated id list from the query string; non-ints dropped.""" + return [int(p) for p in (raw or "").split(",") if p.strip().isdigit()] + + +async def _project_scope() -> tuple[int, str, str]: + """(project_id, repo, unbound_repo) from the request's `project_id` / + `repo` query args — the one resolution every plugin endpoint shares. + + An explicit project_id wins; otherwise the repo remote is resolved + through the caller's bindings, and a remote nobody bound comes back as + `unbound_repo` (normalised) so /context can say "bind this repo". + """ + try: + project_id = int(request.args.get("project_id", 0) or 0) + except (TypeError, ValueError): + project_id = 0 + repo = (request.args.get("repo") or "").strip() + unbound_repo = "" + if repo and not project_id: + resolved = await repo_bindings_svc.resolve_project(g.user.id, repo) + if resolved: + project_id = resolved + else: + unbound_repo = repo_bindings_svc.normalize_repo_key(repo) + return project_id, repo, unbound_repo + + + @plugin_bp.get("/context") @login_required async def session_context(): @@ -37,20 +66,7 @@ async def session_context(): project_id (optional int) — explicit override, mainly for manual/ad-hoc curl testing; takes precedence over `repo` when set. """ - try: - project_id = int(request.args.get("project_id", 0) or 0) - except (TypeError, ValueError): - project_id = 0 - - unbound_repo = "" - repo = (request.args.get("repo") or "").strip() - if repo and not project_id: - resolved = await repo_bindings_svc.resolve_project(g.user.id, repo) - if resolved: - project_id = resolved - else: - unbound_repo = repo_bindings_svc.normalize_repo_key(repo) - + project_id, _repo, unbound_repo = await _project_scope() result = await plugin_ctx_svc.build_session_context( g.user.id, project_id, unbound_repo=unbound_repo ) @@ -77,22 +93,8 @@ async def autoinject_retrieve(): session; skipped so each note injects at most once. """ q = (request.args.get("q") or "").strip() - try: - project_id = int(request.args.get("project_id", 0) or 0) - except (TypeError, ValueError): - project_id = 0 - - repo = (request.args.get("repo") or "").strip() - if repo and not project_id: - resolved = await repo_bindings_svc.resolve_project(g.user.id, repo) - if resolved: - project_id = resolved - - exclude_ids = [ - int(p) for p in (request.args.get("exclude_ids") or "").split(",") - if p.strip().isdigit() - ] - + project_id, _repo, _unbound = await _project_scope() + exclude_ids = _int_list(request.args.get("exclude_ids")) result = await plugin_ctx_svc.build_autoinject_hint( g.user.id, q, project_id=project_id, exclude_ids=exclude_ids ) @@ -139,25 +141,9 @@ async def write_path_prior_art(): """ path = (request.args.get("path") or "").strip() code = request.args.get("code") or "" - try: - project_id = int(request.args.get("project_id", 0) or 0) - except (TypeError, ValueError): - project_id = 0 - - repo = (request.args.get("repo") or "").strip() - if repo and not project_id: - resolved = await repo_bindings_svc.resolve_project(g.user.id, repo) - if resolved: - project_id = resolved - - exclude_ids = [ - int(p) for p in (request.args.get("exclude_ids") or "").split(",") - if p.strip().isdigit() - ] - exclude_sync_ids = [ - int(p) for p in (request.args.get("exclude_sync_ids") or "").split(",") - if p.strip().isdigit() - ] + project_id, repo, _unbound = await _project_scope() + exclude_ids = _int_list(request.args.get("exclude_ids")) + exclude_sync_ids = _int_list(request.args.get("exclude_sync_ids")) shapes = _parse_shapes(request.args.get("shapes") or "") api_key = getattr(g, "api_key", None) may_stamp = api_key is None or getattr(api_key, "scope", "") == "write" diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index 81617bc..4c340b0 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -1,29 +1,26 @@ """Rulebook / topic REST endpoints. -Wraps services/rulebooks.py. Standard Scribe auth: g.user.id is the +Wraps services/rulebooks.py. Standard Scribe auth: get_current_user_id() is the authenticated owner; the service enforces ownership scoping. """ from __future__ import annotations -from quart import Blueprint, g, jsonify, request +from quart import Blueprint, jsonify, request -from scribe.auth import login_required +from scribe.auth import get_current_user_id, login_required import scribe.services.rulebooks as rulebooks_svc from scribe.services.trash import delete as trash_delete rulebooks_bp = Blueprint("rulebooks", __name__, url_prefix="/api") -def _uid() -> int: - return g.user.id - # ── Rulebooks ─────────────────────────────────────────────────────────── @rulebooks_bp.get("/rulebooks") @login_required async def list_rulebooks(): - rows = await rulebooks_svc.list_rulebooks(_uid()) + rows = await rulebooks_svc.list_rulebooks(get_current_user_id()) return jsonify({"rulebooks": [rb.to_dict() for rb in rows]}) @@ -35,7 +32,7 @@ async def create_rulebook(): if not title: return jsonify({"error": "title is required"}), 400 rb = await rulebooks_svc.create_rulebook( - user_id=_uid(), + user_id=get_current_user_id(), title=title, description=data.get("description", ""), ) @@ -45,7 +42,7 @@ async def create_rulebook(): @rulebooks_bp.get("/rulebooks/") @login_required async def get_rulebook(rulebook_id: int): - rb = await rulebooks_svc.get_rulebook(rulebook_id, _uid()) + rb = await rulebooks_svc.get_rulebook(rulebook_id, get_current_user_id()) if rb is None: return jsonify({"error": "rulebook not found"}), 404 return jsonify(rb.to_dict()) @@ -56,7 +53,7 @@ async def get_rulebook(rulebook_id: int): async def update_rulebook(rulebook_id: int): data = await request.get_json() or {} fields = {k: v for k, v in data.items() if k in ("title", "description", "always_on")} - rb = await rulebooks_svc.update_rulebook(rulebook_id, _uid(), **fields) + rb = await rulebooks_svc.update_rulebook(rulebook_id, get_current_user_id(), **fields) if rb is None: return jsonify({"error": "rulebook not found"}), 404 return jsonify(rb.to_dict()) @@ -65,7 +62,7 @@ async def update_rulebook(rulebook_id: int): @rulebooks_bp.delete("/rulebooks/") @login_required async def delete_rulebook(rulebook_id: int): - await trash_delete(_uid(), "rulebook", rulebook_id) + await trash_delete(get_current_user_id(), "rulebook", rulebook_id) return "", 204 @@ -75,7 +72,7 @@ async def delete_rulebook(rulebook_id: int): @login_required async def list_topics(rulebook_id: int): try: - rows = await rulebooks_svc.list_topics(rulebook_id, _uid()) + rows = await rulebooks_svc.list_topics(rulebook_id, get_current_user_id()) except ValueError as exc: return jsonify({"error": str(exc)}), 404 return jsonify({"topics": [t.to_dict() for t in rows]}) @@ -91,7 +88,7 @@ async def create_topic(rulebook_id: int): try: topic = await rulebooks_svc.create_topic( rulebook_id=rulebook_id, - user_id=_uid(), + user_id=get_current_user_id(), title=title, description=data.get("description", ""), order_index=data.get("order_index", 0), @@ -109,7 +106,7 @@ async def update_topic(topic_id: int): k: v for k, v in data.items() if k in ("title", "description", "order_index") } - topic = await rulebooks_svc.update_topic(topic_id, _uid(), **fields) + topic = await rulebooks_svc.update_topic(topic_id, get_current_user_id(), **fields) if topic is None: return jsonify({"error": "topic not found"}), 404 return jsonify(topic.to_dict()) @@ -118,7 +115,7 @@ async def update_topic(topic_id: int): @rulebooks_bp.delete("/rulebook-topics/") @login_required async def delete_topic(topic_id: int): - if await trash_delete(_uid(), "topic", topic_id) is None: + if await trash_delete(get_current_user_id(), "topic", topic_id) is None: return jsonify({"error": "topic not found"}), 404 return "", 204 @@ -140,7 +137,7 @@ async def list_rules(): return jsonify({"error": "rulebook_id, topic_id, project_id must be integers"}), 400 rows = await rulebooks_svc.list_rules( - user_id=_uid(), + user_id=get_current_user_id(), rulebook_id=rulebook_id, topic_id=topic_id, project_id=project_id, @@ -159,7 +156,7 @@ async def create_rule(topic_id: int): try: rule = await rulebooks_svc.create_rule( topic_id=topic_id, - user_id=_uid(), + user_id=get_current_user_id(), title=title, statement=statement, why=data.get("why", ""), @@ -174,7 +171,7 @@ async def create_rule(topic_id: int): @rulebooks_bp.get("/rules/") @login_required async def get_rule(rule_id: int): - rule = await rulebooks_svc.get_rule(rule_id, _uid()) + rule = await rulebooks_svc.get_rule(rule_id, get_current_user_id()) if rule is None: return jsonify({"error": "rule not found"}), 404 return jsonify(rule.to_dict()) @@ -188,7 +185,7 @@ async def update_rule(rule_id: int): k: v for k, v in data.items() if k in ("title", "statement", "why", "how_to_apply", "order_index") } - rule = await rulebooks_svc.update_rule(rule_id, _uid(), **fields) + rule = await rulebooks_svc.update_rule(rule_id, get_current_user_id(), **fields) if rule is None: return jsonify({"error": "rule not found"}), 404 return jsonify(rule.to_dict()) @@ -197,7 +194,7 @@ async def update_rule(rule_id: int): @rulebooks_bp.delete("/rules/") @login_required async def delete_rule(rule_id: int): - if await trash_delete(_uid(), "rule", rule_id) is None: + if await trash_delete(get_current_user_id(), "rule", rule_id) is None: return jsonify({"error": "rule not found"}), 404 return "", 204 @@ -213,7 +210,7 @@ async def subscribe_project(project_id: int): return jsonify({"error": "rulebook_id is required"}), 400 try: await rulebooks_svc.subscribe_project( - project_id=project_id, rulebook_id=int(rulebook_id), user_id=_uid(), + project_id=project_id, rulebook_id=int(rulebook_id), user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -227,7 +224,7 @@ async def subscribe_project(project_id: int): async def unsubscribe_project(project_id: int, rulebook_id: int): try: await rulebooks_svc.unsubscribe_project( - project_id=project_id, rulebook_id=rulebook_id, user_id=_uid(), + project_id=project_id, rulebook_id=rulebook_id, user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -238,7 +235,7 @@ async def unsubscribe_project(project_id: int, rulebook_id: int): @login_required async def get_project_rules(project_id: int): result = await rulebooks_svc.get_applicable_rules( - project_id=project_id, user_id=_uid(), + project_id=project_id, user_id=get_current_user_id(), ) return jsonify(result) @@ -248,7 +245,7 @@ async def get_project_rules(project_id: int): async def suppress_project_rule(project_id: int, rule_id: int): try: await rulebooks_svc.suppress_rule_for_project( - project_id=project_id, rule_id=rule_id, user_id=_uid(), + project_id=project_id, rule_id=rule_id, user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -260,7 +257,7 @@ async def suppress_project_rule(project_id: int, rule_id: int): async def unsuppress_project_rule(project_id: int, rule_id: int): try: await rulebooks_svc.unsuppress_rule_for_project( - project_id=project_id, rule_id=rule_id, user_id=_uid(), + project_id=project_id, rule_id=rule_id, user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -272,7 +269,7 @@ async def unsuppress_project_rule(project_id: int, rule_id: int): async def suppress_project_topic(project_id: int, topic_id: int): try: await rulebooks_svc.suppress_topic_for_project( - project_id=project_id, topic_id=topic_id, user_id=_uid(), + project_id=project_id, topic_id=topic_id, user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -284,7 +281,7 @@ async def suppress_project_topic(project_id: int, topic_id: int): async def unsuppress_project_topic(project_id: int, topic_id: int): try: await rulebooks_svc.unsuppress_topic_for_project( - project_id=project_id, topic_id=topic_id, user_id=_uid(), + project_id=project_id, topic_id=topic_id, user_id=get_current_user_id(), ) except ValueError as exc: return jsonify({"error": str(exc)}), 404 @@ -303,7 +300,7 @@ async def create_project_rule(project_id: int): try: rule = await rulebooks_svc.create_project_rule( project_id=project_id, - user_id=_uid(), + user_id=get_current_user_id(), title=title, statement=statement, why=data.get("why", ""), diff --git a/src/scribe/routes/settings.py b/src/scribe/routes/settings.py index 35f31f2..ea3a4f0 100644 --- a/src/scribe/routes/settings.py +++ b/src/scribe/routes/settings.py @@ -9,7 +9,9 @@ from quart import Blueprint, jsonify, request from scribe.auth import login_required, get_current_user_id from scribe.config import Config -from scribe.services.settings import delete_setting, get_all_settings, get_setting, set_settings_batch +from scribe.services.settings import ( + SECRET_MASK, delete_setting, get_all_settings, get_setting, set_settings_batch, +) logger = logging.getLogger(__name__) @@ -22,12 +24,11 @@ settings_bp = Blueprint("settings", __name__, url_prefix="/api/settings") # rows live on the admin's own user_id, so the plain GET returned them raw. # (forge_token left with 0078: forge credentials are keyring rows now, #2778.) _SECRET_KEYS = frozenset({"smtp_password", "forge_webhook_secret"}) -_SECRET_MASK = "********" def _masked(settings: dict) -> dict: return { - k: (_SECRET_MASK if k in _SECRET_KEYS and v else v) + k: (SECRET_MASK if k in _SECRET_KEYS and v else v) for k, v in settings.items() } @@ -53,7 +54,7 @@ async def update_settings_route(): str_v = str(v) # A masked secret round-tripping through a client is "unchanged", not # a request to store the mask over the real credential. - if k in _SECRET_KEYS and str_v == _SECRET_MASK: + if k in _SECRET_KEYS and str_v == SECRET_MASK: continue if not str_v: await delete_setting(uid, k) @@ -127,7 +128,7 @@ async def update_forge_connection_route(connection_id: int): token = str(data.get("token", "")) # The mask coming back means "unchanged" — the form round-trips what the # list showed, and storing the mask would silently break the connection. - if token == _SECRET_MASK: + if token == SECRET_MASK: token = "" try: row = await update_connection( diff --git a/src/scribe/routes/trash.py b/src/scribe/routes/trash.py index 674194a..4e8b19b 100644 --- a/src/scribe/routes/trash.py +++ b/src/scribe/routes/trash.py @@ -1,33 +1,30 @@ """Trash REST API — list / restore / purge soft-deleted content by batch.""" from __future__ import annotations -from quart import Blueprint, g, jsonify +from quart import Blueprint, jsonify -from scribe.auth import login_required +from scribe.auth import get_current_user_id, login_required import scribe.services.trash as trash_svc trash_bp = Blueprint("trash", __name__, url_prefix="/api/trash") -def _uid() -> int: - return g.user.id - @trash_bp.get("") @login_required async def list_trash(): - return jsonify({"batches": await trash_svc.list_trash(_uid())}) + return jsonify({"batches": await trash_svc.list_trash(get_current_user_id())}) @trash_bp.post("//restore") @login_required async def restore_batch(batch_id: str): - n = await trash_svc.restore(_uid(), batch_id) + n = await trash_svc.restore(get_current_user_id(), batch_id) return jsonify({"restored": n}) @trash_bp.delete("/") @login_required async def purge_batch(batch_id: str): - n = await trash_svc.purge(_uid(), batch_id) + n = await trash_svc.purge(get_current_user_id(), batch_id) return jsonify({"purged": n}) diff --git a/src/scribe/services/settings.py b/src/scribe/services/settings.py index a914916..fb044aa 100644 --- a/src/scribe/services/settings.py +++ b/src/scribe/services/settings.py @@ -8,6 +8,13 @@ from scribe.models.user import User logger = logging.getLogger(__name__) +# What a stored credential looks like on the wire. Every surface that READS a +# secret (smtp_password, forge_webhook_secret, a forge token) returns this +# when one is set; every surface that WRITES one treats this value coming back +# as "unchanged", never as a request to store eight asterisks over the real +# credential. One constant so the read and write halves cannot disagree. +SECRET_MASK = "********" + async def get_admin_setting(key: str, default: str = "") -> str: """Read an instance-global setting (one stored on an admin account). diff --git a/src/scribe/services/supersession.py b/src/scribe/services/supersession.py index eb2fac1..9444b55 100644 --- a/src/scribe/services/supersession.py +++ b/src/scribe/services/supersession.py @@ -181,3 +181,35 @@ async def superseded_ids(note_ids: list[int]) -> set[int]: .where(NoteSupersession.superseded_id.in_(note_ids)) )).scalars().all() return {int(r) for r in rows} + + +SUPERSEDED_HINT = ( + "A later note claims to bring this up to date — see superseded_by. " + "Read this as what was true when written, and check the newer one " + "before acting on it." +) + + +async def attach_relations(user_id: int, note_id: int, data: dict, *, hint: bool = False) -> None: + """Add both directions of the supersession relation to a note payload. + + ONE seam for the REST and MCP surfaces, which must agree about what a + note's payload says — or the web UI and the agent would disagree about + whether a record is current. Both directions, because they answer + different questions and only one is obvious: `supersedes` is what the + author claimed; `superseded_by` is what a READER needs and what the note + itself cannot know — a stale record handed over without that marker gets + acted on confidently, which is worse than never surfacing it. + + Omitted entirely when empty, so an ordinary note's payload doesn't grow + two permanently-empty lists (#2483 — a field that always says nothing + trains readers to skip fields). `hint=True` (the agent surface) also + attaches `superseded_note`, the one-sentence reading instruction. + """ + rel = await get_relations(user_id, note_id) + if rel["supersedes"]: + data["supersedes"] = rel["supersedes"] + if rel["superseded_by"]: + data["superseded_by"] = rel["superseded_by"] + if hint: + data["superseded_note"] = SUPERSEDED_HINT From 92e38ff17be46f5c11a95044cb995dc44189ab2b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 11:26:01 -0400 Subject: [PATCH 6/8] test(routes): the prior-art contract guards read the handler plus its _project_scope helper (area 5 follow-up) Co-Authored-By: Claude Fable 5 --- tests/test_write_path_trigger.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index ed0ea75..fa9a905 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -764,7 +764,11 @@ def test_route_reads_every_arg_the_hook_sends(): import inspect from scribe.routes import plugin as routes - src = inspect.getsource(routes.write_path_prior_art) + # The handler plus the query-scope helper it delegates repo/project_id + # to — the contract is what the ROUTE MODULE reads, wherever it reads it. + src = inspect.getsource(routes.write_path_prior_art) + inspect.getsource( + routes._project_scope + ) for arg in ("path", "code", "repo", "project_id", "exclude_ids", "exclude_sync_ids", "shapes"): assert f'request.args.get("{arg}"' in src, f"route ignores {arg}" @@ -782,7 +786,9 @@ def test_route_resolves_repo_to_a_project_not_to_a_location_filter(): import inspect from scribe.routes import plugin as routes - src = inspect.getsource(routes.write_path_prior_art) + src = inspect.getsource(routes.write_path_prior_art) + inspect.getsource( + routes._project_scope + ) assert "resolve_project" in src assert "repo=repo" not in src From 7d48eb0b1bf7ff14e7a1242d8a954b58ffb34b1a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 12:34:28 -0400 Subject: [PATCH 7/8] =?UTF-8?q?refactor(services):=20one=20periodic-task?= =?UTF-8?q?=20shape,=20one=20APScheduler=20job=20shape,=20one=20token=20ha?= =?UTF-8?q?sh,=20one=20summary=20rule=20=E2=80=94=20the=20services=20pass?= =?UTF-8?q?=20of=20the=20shape=20audit=20(#2830,=20milestone=20296)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - background.start_periodic(interval, work, label=) replaces the three hand-rolled while-True/sleep/try loops in logging, auth and notifications. - services/scheduler.ScheduledJob replaces the four private BackgroundScheduler copies in recurrence/version_pinning/trash/db_maintenance schedulers; public start_/stop_/reschedule_ surfaces unchanged. - api_keys.hash_token is the one sha256 helper; auth.py used to inline it 5x. - auth.is_registration_open reads via settings.get_admin_setting; notification prefs read via settings.get_setting; _fire_share_email uses _get_user_email. - projects.get_project_summary / milestones.get_project_milestone_summary are now the one-id view of their batch siblings instead of a second copy of the queries; sharing.best_permission_by (was _deduplicate_by_permission) is the one rank-dedup, now also used by list_projects_for_user. - backup: the row builders for every section both exporters carry are named functions, so a column added to one export cannot silently miss the other. - iso() from models.base replaces the attr.isoformat()-if-attr-else-None idiom and db_maintenance._iso across services; backup keeps its explicit shape. - trash.py hoists the sql_delete/timedelta imports it re-imported per function. Co-Authored-By: Claude Fable 5 --- src/scribe/services/api_keys.py | 11 +- src/scribe/services/auth.py | 44 +- src/scribe/services/background.py | 20 + src/scribe/services/backup.py | 428 +++++++----------- src/scribe/services/dashboard.py | 3 +- src/scribe/services/db_maintenance.py | 9 +- .../services/db_maintenance_scheduler.py | 64 +-- src/scribe/services/dedup.py | 5 +- src/scribe/services/knowledge.py | 7 +- src/scribe/services/logging.py | 16 +- src/scribe/services/milestones.py | 12 +- src/scribe/services/note_usage.py | 5 +- src/scribe/services/notifications.py | 68 +-- src/scribe/services/projects.py | 68 +-- src/scribe/services/recurrence_scheduler.py | 39 +- src/scribe/services/scheduler.py | 78 ++++ src/scribe/services/shape_ledger.py | 7 +- src/scribe/services/sharing.py | 11 +- src/scribe/services/trash.py | 15 +- src/scribe/services/trash_scheduler.py | 78 +--- .../services/version_pinning_scheduler.py | 61 +-- tests/test_api_keys.py | 6 +- 22 files changed, 421 insertions(+), 634 deletions(-) create mode 100644 src/scribe/services/scheduler.py diff --git a/src/scribe/services/api_keys.py b/src/scribe/services/api_keys.py index c0ed030..d8b2021 100644 --- a/src/scribe/services/api_keys.py +++ b/src/scribe/services/api_keys.py @@ -13,8 +13,11 @@ def generate_key() -> str: return "fmcp_" + secrets.token_urlsafe(32) -def _hash_key(key: str) -> str: - return hashlib.sha256(key.encode()).hexdigest() +def hash_token(raw: str) -> str: + """The ONE fingerprint for every bearer secret stored by hash — API keys, + password-reset tokens, invitation tokens. Stored rows hold this, never + the raw value; a lookup hashes the presented token and compares.""" + return hashlib.sha256(raw.encode()).hexdigest() def _key_prefix(key: str) -> str: @@ -32,7 +35,7 @@ async def create_api_key( key = ApiKey( user_id=user_id, name=name, - key_hash=_hash_key(full_key), + key_hash=hash_token(full_key), key_prefix=_key_prefix(full_key), scope=scope, ) @@ -70,7 +73,7 @@ async def revoke_api_key(user_id: int, key_id: int) -> bool: async def lookup_key(raw_key: str) -> ApiKey | None: """Look up a non-revoked ApiKey by raw token value. Updates last_used_at.""" - key_hash = _hash_key(raw_key) + key_hash = hash_token(raw_key) async with async_session() as session: result = await session.execute( select(ApiKey).where( diff --git a/src/scribe/services/auth.py b/src/scribe/services/auth.py index cee92cd..9f551df 100644 --- a/src/scribe/services/auth.py +++ b/src/scribe/services/auth.py @@ -1,4 +1,3 @@ -import hashlib import logging import secrets from datetime import datetime, timedelta, timezone @@ -12,6 +11,8 @@ from scribe.models.invitation import InvitationToken from scribe.models.password_reset import PasswordResetToken from scribe.models.setting import Setting from scribe.models.user import User +from scribe.services.api_keys import hash_token +from scribe.services.settings import get_admin_setting logger = logging.getLogger(__name__) @@ -142,16 +143,7 @@ async def is_registration_open() -> bool: user_count = await get_user_count() if user_count == 0: return True - - async with async_session() as session: - # Find the admin user's registration_open setting - result = await session.execute( - select(Setting) - .join(User, Setting.user_id == User.id) - .where(User.role == "admin", Setting.key == "registration_open") - ) - setting = result.scalar_one_or_none() - return setting.value == "true" if setting else False + return await get_admin_setting("registration_open", "false") == "true" async def list_users() -> list[User]: @@ -211,7 +203,7 @@ async def get_user_by_email(email: str) -> User | None: async def create_password_reset_token(user_id: int) -> str: """Generate a password reset token. Returns the raw token (for the email link).""" raw_token = secrets.token_urlsafe(32) - token_hash = hashlib.sha256(raw_token.encode()).hexdigest() + token_hash = hash_token(raw_token) expires_at = datetime.now(timezone.utc) + timedelta(hours=1) async with async_session() as session: @@ -239,7 +231,7 @@ async def create_password_reset_token(user_id: int) -> str: async def reset_password_with_token(raw_token: str, new_password: str) -> int | None: """Validate a reset token and update the user's password. Returns user_id on success.""" - token_hash = hashlib.sha256(raw_token.encode()).hexdigest() + token_hash = hash_token(raw_token) async with async_session() as session: result = await session.execute( @@ -270,7 +262,7 @@ async def reset_password_with_token(raw_token: str, new_password: str) -> int | async def create_invitation(email: str, invited_by: int) -> str: """Generate an invitation token. Returns the raw token (for the email link).""" raw_token = secrets.token_urlsafe(32) - token_hash = hashlib.sha256(raw_token.encode()).hexdigest() + token_hash = hash_token(raw_token) expires_at = datetime.now(timezone.utc) + timedelta(days=7) async with async_session() as session: @@ -299,7 +291,7 @@ async def create_invitation(email: str, invited_by: int) -> str: async def validate_invitation_token(raw_token: str) -> InvitationToken | None: """Look up by hash, check not used/expired. Returns the token record with email.""" - token_hash = hashlib.sha256(raw_token.encode()).hexdigest() + token_hash = hash_token(raw_token) async with async_session() as session: result = await session.execute( @@ -319,7 +311,7 @@ async def validate_invitation_token(raw_token: str) -> InvitationToken | None: async def register_with_invitation(raw_token: str, username: str, password: str) -> User | None: """Validate token, create user with the invitation's email, mark token used.""" - token_hash = hashlib.sha256(raw_token.encode()).hexdigest() + token_hash = hash_token(raw_token) async with async_session() as session: result = await session.execute( @@ -398,20 +390,16 @@ async def purge_expired_auth_tokens(grace_days: int = 7) -> int: return removed -async def _auth_token_retention_loop() -> None: - import asyncio - while True: - await asyncio.sleep(86400) # daily - try: - removed = await purge_expired_auth_tokens() - if removed: - logger.info("Auth token retention: deleted %d expired token(s)", removed) - except Exception: - logger.exception("Error in auth token retention cleanup") +async def _auth_token_retention_tick() -> None: + removed = await purge_expired_auth_tokens() + if removed: + logger.info("Auth token retention: deleted %d expired token(s)", removed) def start_auth_token_retention_loop() -> None: global _auth_retention_task - import asyncio if _auth_retention_task is None or _auth_retention_task.done(): - _auth_retention_task = asyncio.create_task(_auth_token_retention_loop()) + from scribe.services.background import start_periodic + _auth_retention_task = start_periodic( + 86400, _auth_token_retention_tick, label="auth_token_retention", # daily + ) diff --git a/src/scribe/services/background.py b/src/scribe/services/background.py index 9f6d4ee..a81fa5d 100644 --- a/src/scribe/services/background.py +++ b/src/scribe/services/background.py @@ -45,6 +45,26 @@ def spawn(coro: Coroutine, *, site: str) -> None: task.add_done_callback(_done) +def start_periodic(interval_s: float, work, *, label: str) -> asyncio.Task: + """A forever loop that sleeps ``interval_s`` then awaits ``work()``, logging + (never raising) when a tick fails — the one shape the hourly/daily + retention sweeps share (log retention, notification sweep, auth-token + purge). Sleeps FIRST so startup isn't a sweep; holds a strong reference + like spawn() so the loop cannot be garbage-collected mid-flight.""" + async def _loop() -> None: + while True: + await asyncio.sleep(interval_s) + try: + await work() + except Exception: + logger.exception("periodic task %s failed", label) + + task = asyncio.get_running_loop().create_task(_loop(), name=f"periodic-{label}") + _pending.add(task) + task.add_done_callback(_pending.discard) + return task + + async def drain() -> None: """Await everything in flight — for tests that need the writes landed.""" while _pending: diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 5929c23..d1ec08c 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -189,6 +189,142 @@ def _repo_binding_rows(rows) -> list[dict]: ] +# Row builders for the sections both exporters carry. Pure, like the join-table +# helpers above; the full and per-user exports used to restate every one of +# these comprehensions side by side, and a column added to one and not the +# other is a backup that silently drops it (#2293's shape, one layer down). + +def _user_rows(rows) -> list[dict]: + return [ + { + "id": u.id, "username": u.username, "email": u.email, + "password_hash": u.password_hash, "oauth_sub": u.oauth_sub, + "role": u.role, "session_version": u.session_version, + "created_at": u.created_at.isoformat(), + } + for u in rows + ] + + +def _project_rows(rows) -> list[dict]: + return [ + { + "id": p.id, "user_id": p.user_id, "title": p.title, + "description": p.description, "goal": p.goal, "status": p.status, + "color": p.color, + "created_at": p.created_at.isoformat(), + "updated_at": p.updated_at.isoformat(), + } + for p in rows + ] + + +def _milestone_rows(rows) -> list[dict]: + return [ + { + "id": m.id, "user_id": m.user_id, "project_id": m.project_id, + "title": m.title, "description": m.description, "status": m.status, + "order_index": m.order_index, + "created_at": m.created_at.isoformat(), + "updated_at": m.updated_at.isoformat(), + } + for m in rows + ] + + +def _note_rows(rows) -> list[dict]: + return [ + { + "id": n.id, "user_id": n.user_id, "title": n.title, "body": n.body, + "tags": n.tags or [], "parent_id": n.parent_id, + "project_id": n.project_id, "milestone_id": n.milestone_id, + "status": n.status, "priority": n.priority, + "due_date": n.due_date.isoformat() if n.due_date else None, + "created_at": n.created_at.isoformat(), + "updated_at": n.updated_at.isoformat(), + } + for n in rows + ] + + +def _task_log_rows(rows) -> list[dict]: + return [ + { + "id": tl.id, "user_id": tl.user_id, "task_id": tl.task_id, + "content": tl.content, "duration_minutes": tl.duration_minutes, + "created_at": tl.created_at.isoformat(), + "updated_at": tl.updated_at.isoformat(), + } + for tl in rows + ] + + +def _note_draft_rows(rows) -> list[dict]: + return [ + { + "id": nd.id, "user_id": nd.user_id, "note_id": nd.note_id, + "proposed_body": nd.proposed_body, "original_body": nd.original_body, + "instruction": nd.instruction, "scope": nd.scope, + "created_at": nd.created_at.isoformat(), + "updated_at": nd.updated_at.isoformat(), + } + for nd in rows + ] + + +def _note_version_rows(rows) -> list[dict]: + return [ + { + "id": nv.id, "user_id": nv.user_id, "note_id": nv.note_id, + "title": nv.title, "body": nv.body, "tags": nv.tags or [], + "pin_kind": nv.pin_kind, "pin_label": nv.pin_label, + "created_at": nv.created_at.isoformat(), + } + for nv in rows + ] + + +def _setting_rows(rows) -> list[dict]: + return [{"user_id": s.user_id, "key": s.key, "value": s.value} for s in rows] + + +def _rulebook_rows(rows) -> list[dict]: + return [ + { + "id": rb.id, "owner_user_id": rb.owner_user_id, "title": rb.title, + "description": rb.description, "always_on": rb.always_on, + "created_at": rb.created_at.isoformat(), + "updated_at": rb.updated_at.isoformat(), + } + for rb in rows + ] + + +def _topic_rows(rows) -> list[dict]: + return [ + { + "id": t.id, "rulebook_id": t.rulebook_id, "title": t.title, + "description": t.description, "order_index": t.order_index, + "created_at": t.created_at.isoformat(), + "updated_at": t.updated_at.isoformat(), + } + for t in rows + ] + + +def _rule_rows(rows) -> list[dict]: + return [ + { + "id": r.id, "topic_id": r.topic_id, "project_id": r.project_id, + "title": r.title, "statement": r.statement, "why": r.why, + "how_to_apply": r.how_to_apply, "order_index": r.order_index, + "created_at": r.created_at.isoformat(), + "updated_at": r.updated_at.isoformat(), + } + for r in rows + ] + + # --------------------------------------------------------------------------- # Export # --------------------------------------------------------------------------- @@ -247,148 +383,17 @@ async def export_full_backup() -> dict: "Store it securely and restrict access." ), "_not_included": _NOT_INCLUDED, - "users": [ - { - "id": u.id, - "username": u.username, - "email": u.email, - "password_hash": u.password_hash, - "oauth_sub": u.oauth_sub, - "role": u.role, - "session_version": u.session_version, - "created_at": u.created_at.isoformat(), - } - for u in users - ], - "projects": [ - { - "id": p.id, - "user_id": p.user_id, - "title": p.title, - "description": p.description, - "goal": p.goal, - "status": p.status, - "color": p.color, - "created_at": p.created_at.isoformat(), - "updated_at": p.updated_at.isoformat(), - } - for p in projects - ], - "milestones": [ - { - "id": m.id, - "user_id": m.user_id, - "project_id": m.project_id, - "title": m.title, - "description": m.description, - "status": m.status, - "order_index": m.order_index, - "created_at": m.created_at.isoformat(), - "updated_at": m.updated_at.isoformat(), - } - for m in milestones - ], - "notes": [ - { - "id": n.id, - "user_id": n.user_id, - "title": n.title, - "body": n.body, - "tags": n.tags or [], - "parent_id": n.parent_id, - "project_id": n.project_id, - "milestone_id": n.milestone_id, - "status": n.status, - "priority": n.priority, - "due_date": n.due_date.isoformat() if n.due_date else None, - "created_at": n.created_at.isoformat(), - "updated_at": n.updated_at.isoformat(), - } - for n in notes - ], - "task_logs": [ - { - "id": tl.id, - "user_id": tl.user_id, - "task_id": tl.task_id, - "content": tl.content, - "duration_minutes": tl.duration_minutes, - "created_at": tl.created_at.isoformat(), - "updated_at": tl.updated_at.isoformat(), - } - for tl in task_logs - ], - "note_drafts": [ - { - "id": nd.id, - "user_id": nd.user_id, - "note_id": nd.note_id, - "proposed_body": nd.proposed_body, - "original_body": nd.original_body, - "instruction": nd.instruction, - "scope": nd.scope, - "created_at": nd.created_at.isoformat(), - "updated_at": nd.updated_at.isoformat(), - } - for nd in note_drafts - ], - "note_versions": [ - { - "id": nv.id, - "user_id": nv.user_id, - "note_id": nv.note_id, - "title": nv.title, - "body": nv.body, - "tags": nv.tags or [], - "pin_kind": nv.pin_kind, - "pin_label": nv.pin_label, - "created_at": nv.created_at.isoformat(), - } - for nv in note_versions - ], - "settings": [ - {"user_id": s.user_id, "key": s.key, "value": s.value} - for s in settings - ], - "rulebooks": [ - { - "id": rb.id, - "owner_user_id": rb.owner_user_id, - "title": rb.title, - "description": rb.description, - "always_on": rb.always_on, - "created_at": rb.created_at.isoformat(), - "updated_at": rb.updated_at.isoformat(), - } - for rb in rulebooks - ], - "rulebook_topics": [ - { - "id": t.id, - "rulebook_id": t.rulebook_id, - "title": t.title, - "description": t.description, - "order_index": t.order_index, - "created_at": t.created_at.isoformat(), - "updated_at": t.updated_at.isoformat(), - } - for t in topics - ], - "rules": [ - { - "id": r.id, - "topic_id": r.topic_id, - "project_id": r.project_id, - "title": r.title, - "statement": r.statement, - "why": r.why, - "how_to_apply": r.how_to_apply, - "order_index": r.order_index, - "created_at": r.created_at.isoformat(), - "updated_at": r.updated_at.isoformat(), - } - for r in rules - ], + "users": _user_rows(users), + "projects": _project_rows(projects), + "milestones": _milestone_rows(milestones), + "notes": _note_rows(notes), + "task_logs": _task_log_rows(task_logs), + "note_drafts": _note_draft_rows(note_drafts), + "note_versions": _note_version_rows(note_versions), + "settings": _setting_rows(settings), + "rulebooks": _rulebook_rows(rulebooks), + "rulebook_topics": _topic_rows(topics), + "rules": _rule_rows(rules), "rulebook_subscriptions": _subscription_rows(subscriptions), "rule_suppressions": _rule_suppression_rows(rule_suppressions), "topic_suppressions": _topic_suppression_rows(topic_suppressions), @@ -526,135 +531,16 @@ async def export_user_backup(user_id: int) -> dict: "role": user.role, "created_at": user.created_at.isoformat(), } if user else None, - "projects": [ - { - "id": p.id, - "user_id": p.user_id, - "title": p.title, - "description": p.description, - "goal": p.goal, - "status": p.status, - "color": p.color, - "created_at": p.created_at.isoformat(), - "updated_at": p.updated_at.isoformat(), - } - for p in projects - ], - "milestones": [ - { - "id": m.id, - "user_id": m.user_id, - "project_id": m.project_id, - "title": m.title, - "description": m.description, - "status": m.status, - "order_index": m.order_index, - "created_at": m.created_at.isoformat(), - "updated_at": m.updated_at.isoformat(), - } - for m in milestones - ], - "notes": [ - { - "id": n.id, - "user_id": n.user_id, - "title": n.title, - "body": n.body, - "tags": n.tags or [], - "parent_id": n.parent_id, - "project_id": n.project_id, - "milestone_id": n.milestone_id, - "status": n.status, - "priority": n.priority, - "due_date": n.due_date.isoformat() if n.due_date else None, - "created_at": n.created_at.isoformat(), - "updated_at": n.updated_at.isoformat(), - } - for n in notes - ], - "task_logs": [ - { - "id": tl.id, - "user_id": tl.user_id, - "task_id": tl.task_id, - "content": tl.content, - "duration_minutes": tl.duration_minutes, - "created_at": tl.created_at.isoformat(), - "updated_at": tl.updated_at.isoformat(), - } - for tl in task_logs - ], - "note_drafts": [ - { - "id": nd.id, - "user_id": nd.user_id, - "note_id": nd.note_id, - "proposed_body": nd.proposed_body, - "original_body": nd.original_body, - "instruction": nd.instruction, - "scope": nd.scope, - "created_at": nd.created_at.isoformat(), - "updated_at": nd.updated_at.isoformat(), - } - for nd in note_drafts - ], - "note_versions": [ - { - "id": nv.id, - "user_id": nv.user_id, - "note_id": nv.note_id, - "title": nv.title, - "body": nv.body, - "tags": nv.tags or [], - "pin_kind": nv.pin_kind, - "pin_label": nv.pin_label, - "created_at": nv.created_at.isoformat(), - } - for nv in note_versions - ], - "settings": [ - {"user_id": s.user_id, "key": s.key, "value": s.value} - for s in settings - ], - "rulebooks": [ - { - "id": rb.id, - "owner_user_id": rb.owner_user_id, - "title": rb.title, - "description": rb.description, - "always_on": rb.always_on, - "created_at": rb.created_at.isoformat(), - "updated_at": rb.updated_at.isoformat(), - } - for rb in rulebooks - ], - "rulebook_topics": [ - { - "id": t.id, - "rulebook_id": t.rulebook_id, - "title": t.title, - "description": t.description, - "order_index": t.order_index, - "created_at": t.created_at.isoformat(), - "updated_at": t.updated_at.isoformat(), - } - for t in topics - ], - "rules": [ - { - "id": r.id, - "topic_id": r.topic_id, - "project_id": r.project_id, - "title": r.title, - "statement": r.statement, - "why": r.why, - "how_to_apply": r.how_to_apply, - "order_index": r.order_index, - "created_at": r.created_at.isoformat(), - "updated_at": r.updated_at.isoformat(), - } - for r in rules - ], + "projects": _project_rows(projects), + "milestones": _milestone_rows(milestones), + "notes": _note_rows(notes), + "task_logs": _task_log_rows(task_logs), + "note_drafts": _note_draft_rows(note_drafts), + "note_versions": _note_version_rows(note_versions), + "settings": _setting_rows(settings), + "rulebooks": _rulebook_rows(rulebooks), + "rulebook_topics": _topic_rows(topics), + "rules": _rule_rows(rules), "rulebook_subscriptions": _subscription_rows(subscriptions), "rule_suppressions": _rule_suppression_rows(rule_suppressions), "topic_suppressions": _topic_suppression_rows(topic_suppressions), diff --git a/src/scribe/services/dashboard.py b/src/scribe/services/dashboard.py index 77c2d6f..d6942dd 100644 --- a/src/scribe/services/dashboard.py +++ b/src/scribe/services/dashboard.py @@ -15,6 +15,7 @@ from scribe.models import async_session from scribe.models.note import Note from scribe.models.project import Project from scribe.models.milestone import Milestone +from scribe.models.base import iso from scribe.services import milestones as milestones_svc logger = logging.getLogger(__name__) @@ -173,7 +174,7 @@ async def _recently_completed(user_id: int) -> list[dict]: .order_by(Note.completed_at.desc()).limit(RECENT_DONE_LIMIT) )).all() return [{"id": n.id, "title": n.title, "project_title": ptitle, - "completed_at": n.completed_at.isoformat()} for n, ptitle in rows] + "completed_at": iso(n.completed_at)} for n, ptitle in rows] async def _week_stats(user_id: int) -> dict: diff --git a/src/scribe/services/db_maintenance.py b/src/scribe/services/db_maintenance.py index b01277c..349ea7e 100644 --- a/src/scribe/services/db_maintenance.py +++ b/src/scribe/services/db_maintenance.py @@ -21,6 +21,7 @@ from datetime import datetime, timezone from sqlalchemy import text from scribe.models import async_session, engine +from scribe.models.base import iso from scribe.services.settings import get_admin_setting, set_admin_setting logger = logging.getLogger(__name__) @@ -128,10 +129,6 @@ _HEALTH_SQL = text(""" """) -def _iso(value) -> str | None: - return value.isoformat() if value is not None else None - - async def get_table_health() -> dict: """Per-table health from Postgres statistics + the total database size. @@ -156,8 +153,8 @@ async def get_table_health() -> dict: "dead_pct": float(r["dead_pct"] or 0), "total_bytes": int(r["total_bytes"] or 0), "mod_since_analyze": int(r["mod_since_analyze"] or 0), - "last_vacuum": _iso(r["last_vacuum"]), - "last_analyze": _iso(r["last_analyze"]), + "last_vacuum": iso(r["last_vacuum"]), + "last_analyze": iso(r["last_analyze"]), } for r in rows ] diff --git a/src/scribe/services/db_maintenance_scheduler.py b/src/scribe/services/db_maintenance_scheduler.py index 83cc6ff..35ff69a 100644 --- a/src/scribe/services/db_maintenance_scheduler.py +++ b/src/scribe/services/db_maintenance_scheduler.py @@ -1,9 +1,8 @@ """Daily APScheduler cron for basic DB maintenance (targeted VACUUM ANALYZE). -Mirrors trash_scheduler.py: a single global BackgroundScheduler job bridges -into the asyncio loop to run the async maintenance. Scheduled for 04:00 UTC by -default — after the 03:30 trash purge — so it collects the dead tuples that -night's delete sweeps leave behind. +One ScheduledJob (services/scheduler.py). Scheduled for 04:00 UTC by default +— after the 03:30 trash purge — so it collects the dead tuples that night's +delete sweeps leave behind. Two things are operator-tunable from the admin Settings card: - db_maintenance_enabled ("true"/"false") — checked at fire time, so toggling @@ -16,9 +15,9 @@ from __future__ import annotations import asyncio import logging -from apscheduler.schedulers.background import BackgroundScheduler from apscheduler.triggers.cron import CronTrigger +from scribe.services.scheduler import ScheduledJob from scribe.services.settings import get_admin_setting logger = logging.getLogger(__name__) @@ -26,9 +25,6 @@ logger = logging.getLogger(__name__) _JOB_ID = "db_maintenance_vacuum" _DEFAULT_HOUR = 4 -_scheduler: BackgroundScheduler | None = None -_loop: asyncio.AbstractEventLoop | None = None - async def get_maintenance_hour() -> int: """The configured run-hour (UTC, 0–23), clamped; default 04:00.""" @@ -45,23 +41,20 @@ async def is_maintenance_enabled() -> bool: return (await get_admin_setting("db_maintenance_enabled", "true")) != "false" -def _run_maintenance_threadsafe() -> None: - """APScheduler invokes this from a worker thread; bridge into the loop.""" - if _loop is None: - logger.warning("db maintenance scheduler: no loop registered") +async def _run_maintenance() -> None: + if not await is_maintenance_enabled(): + logger.debug("db maintenance: disabled, skipping scheduled run") return + from scribe.services.db_maintenance import run_maintenance + await run_maintenance() - async def _runner(): - try: - if not await is_maintenance_enabled(): - logger.debug("db maintenance: disabled, skipping scheduled run") - return - from scribe.services.db_maintenance import run_maintenance - await run_maintenance() - except Exception: - logger.exception("db maintenance run failed") - asyncio.run_coroutine_threadsafe(_runner(), _loop) +_JOB = ScheduledJob(_JOB_ID, _run_maintenance, label="DB maintenance") + + +def _trigger(hour: int) -> CronTrigger: + hour = hour if 0 <= hour <= 23 else _DEFAULT_HOUR + return CronTrigger(hour=hour, minute=0, timezone="UTC") def start_db_maintenance_scheduler( @@ -72,36 +65,15 @@ def start_db_maintenance_scheduler( in rather than read here so we never block the event loop at startup. The job's enabled-gate is re-checked at every fire, so only the hour is needed up front.""" - global _scheduler, _loop - if _scheduler is not None: - return - _loop = loop hour = hour if 0 <= hour <= 23 else _DEFAULT_HOUR - _scheduler = BackgroundScheduler() - _scheduler.add_job( - _run_maintenance_threadsafe, - trigger=CronTrigger(hour=hour, minute=0, timezone="UTC"), - id=_JOB_ID, - replace_existing=True, - ) - _scheduler.start() - logger.info("DB maintenance scheduler started (daily %02d:00 UTC)", hour) + _JOB.start(loop, _trigger(hour), describe=f"daily {hour:02d}:00 UTC") def reschedule_db_maintenance(hour: int) -> None: """Move the live job to a new UTC hour (called when the admin changes it).""" - if _scheduler is None: - return hour = hour if 0 <= hour <= 23 else _DEFAULT_HOUR - _scheduler.reschedule_job( - _JOB_ID, trigger=CronTrigger(hour=hour, minute=0, timezone="UTC") - ) - logger.info("DB maintenance scheduler rescheduled to %02d:00 UTC", hour) + _JOB.reschedule(_trigger(hour), describe=f"{hour:02d}:00 UTC") def stop_db_maintenance_scheduler() -> None: - global _scheduler - if _scheduler is not None: - _scheduler.shutdown(wait=False) - _scheduler = None - logger.info("DB maintenance scheduler stopped") + _JOB.stop() diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index 103ad96..72c2c41 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -32,6 +32,7 @@ from scribe.models import async_session from scribe.models.embedding import NoteEmbedding from scribe.models.note import Note from scribe.models.rulebook import Rule +from scribe.models.base import iso from scribe.services import embeddings as embeddings_svc # Imported rather than redeclared: no service imports this module (the create # gate is called from the routes/tools layer), so there is no cycle to dodge, @@ -592,8 +593,8 @@ async def find_duplicate_records( titles[int(i)] = t records[int(i)] = d or {} meta[int(i)] = { - "created_at": created.isoformat() if created else None, - "updated_at": updated.isoformat() if updated else None, + "created_at": iso(created), + "updated_at": iso(updated), "task_kind": task_kind, } except Exception: diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index 10b644c..e0a74f2 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -22,6 +22,7 @@ from sqlalchemy import and_, func, or_, select from scribe.models import async_session from scribe.models.note import Note +from scribe.models.base import iso from scribe.services.access import browsable_notes_clause, readable_notes_clause logger = logging.getLogger(__name__) @@ -211,8 +212,8 @@ def _note_to_item(note: Note) -> dict: # These lists now include records shared with the caller, so the client # needs the owner to tell "mine" from "someone else's" in a mixed list. "user_id": note.user_id, - "created_at": note.created_at.isoformat(), - "updated_at": note.updated_at.isoformat(), + "created_at": iso(note.created_at), + "updated_at": iso(note.updated_at), } # Drift verdict (#2086), when one has been recorded. Included here rather # than decorated on by the snippet layer because `current` is derivable from @@ -249,7 +250,7 @@ def _note_to_item(note: Note) -> dict: item["task_kind"] = note.task_kind item["status"] = note.status item["priority"] = note.priority - item["due_date"] = note.due_date.isoformat() if note.due_date else None + item["due_date"] = iso(note.due_date) return item diff --git a/src/scribe/services/logging.py b/src/scribe/services/logging.py index 3123920..1f560e9 100644 --- a/src/scribe/services/logging.py +++ b/src/scribe/services/logging.py @@ -194,18 +194,14 @@ async def delete_old_logs(retention_days: int) -> int: return result.rowcount -async def _retention_loop() -> None: - while True: - await asyncio.sleep(3600) # hourly - try: - deleted = await delete_old_logs(Config.LOG_RETENTION_DAYS) - if deleted: - logger.info("Log retention: deleted %d old log entries", deleted) - except Exception: - logger.exception("Error in log retention cleanup") +async def _retention_tick() -> None: + deleted = await delete_old_logs(Config.LOG_RETENTION_DAYS) + if deleted: + logger.info("Log retention: deleted %d old log entries", deleted) def start_log_retention_loop() -> None: global _retention_task if _retention_task is None or _retention_task.done(): - _retention_task = asyncio.create_task(_retention_loop()) + from scribe.services.background import start_periodic + _retention_task = start_periodic(3600, _retention_tick, label="log_retention") # hourly diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index 679aae5..5de7b59 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -235,12 +235,6 @@ async def get_project_milestone_summaries( async def get_project_milestone_summary(user_id: int, project_id: int) -> list[dict]: - """Return ordered list of milestones with their progress stats.""" - milestones = await list_milestones(user_id, project_id) - result = [] - for m in milestones: - progress = await get_milestone_progress(m.id) - entry = m.to_dict() - entry.update(progress) - result.append(entry) - return result + """Ordered milestones with progress — the one-project view of + get_project_milestone_summaries (two queries, not N+1).""" + return (await get_project_milestone_summaries(user_id, [project_id])).get(project_id, []) diff --git a/src/scribe/services/note_usage.py b/src/scribe/services/note_usage.py index 6355c7d..cf67d5c 100644 --- a/src/scribe/services/note_usage.py +++ b/src/scribe/services/note_usage.py @@ -36,6 +36,7 @@ from sqlalchemy import case, func, select from scribe.models import async_session from scribe.models.note_usage import PULLED, SURFACED, NoteUsageEvent +from scribe.models.base import iso logger = logging.getLogger(__name__) @@ -232,13 +233,13 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: slot["ambient_count"] = int(n) elif event == SURFACED: slot["surfaced_count"] = int(n) - slot["last_surfaced_at"] = last_at.isoformat() if last_at else None + slot["last_surfaced_at"] = iso(last_at) elif event == PULLED: # Pulls are pulls regardless of what surfaced the record — the # question a pull answers ("did anyone ever open this?") doesn't # depend on how it was found. slot["pull_count"] = slot["pull_count"] + int(n) - latest = last_at.isoformat() if last_at else None + latest = iso(last_at) if latest and (slot["last_pulled_at"] or "") < latest: slot["last_pulled_at"] = latest return out diff --git a/src/scribe/services/notifications.py b/src/scribe/services/notifications.py index cd171d6..00fb0cd 100644 --- a/src/scribe/services/notifications.py +++ b/src/scribe/services/notifications.py @@ -3,17 +3,20 @@ import asyncio import json import logging -from datetime import date, datetime, time, timezone +from datetime import date, datetime, time, timedelta, timezone -from sqlalchemy import func, select, text +from sqlalchemy import delete as sa_delete, func, select, text +from sqlalchemy import update as sa_update from scribe.models import async_session from scribe.models.app_log import AppLog from scribe.models.note import Note -from scribe.models.setting import Setting +from scribe.models.notification import Notification from scribe.models.user import User +from scribe.models.base import iso from scribe.services.email import _email_html, is_smtp_configured, send_email from scribe.services.logging import log_audit +from scribe.services.settings import get_setting logger = logging.getLogger(__name__) @@ -29,13 +32,7 @@ SECURITY_EVENT_LABELS = { async def _get_user_notification_pref(user_id: int, key: str) -> bool: """Check if a user has a notification preference enabled (default True).""" - async with async_session() as session: - result = await session.execute( - select(Setting).where(Setting.user_id == user_id, Setting.key == key) - ) - setting = result.scalar_one_or_none() - # Default to enabled - return setting.value != "false" if setting else True + return await get_setting(user_id, key, "true") != "false" async def _get_user_email(user_id: int) -> str | None: @@ -222,7 +219,7 @@ async def check_due_tasks() -> None: for task in user_tasks: overdue = task.due_date < today if task.due_date else False date_color = "#ef4444" if overdue else "#374151" - date_label = f'{task.due_date.isoformat()}' if task.due_date else "" + date_label = f'{iso(task.due_date)}' if task.due_date else "" overdue_badge = ' (overdue)' if overdue else "" task_rows += ( f'' @@ -261,13 +258,10 @@ _NOTIFICATION_RETENTION_DAYS = 30 async def purge_old_read_notifications(retention_days: int = _NOTIFICATION_RETENTION_DAYS) -> int: """Delete already-read in-app notifications older than retention_days.""" - from datetime import timedelta - from sqlalchemy import delete - from scribe.models.notification import Notification cutoff = datetime.now(timezone.utc) - timedelta(days=retention_days) async with async_session() as session: result = await session.execute( - delete(Notification).where( + sa_delete(Notification).where( Notification.read_at.isnot(None), Notification.read_at < cutoff, ) @@ -276,25 +270,21 @@ async def purge_old_read_notifications(retention_days: int = _NOTIFICATION_RETEN return result.rowcount or 0 -async def _notification_loop() -> None: - while True: - await asyncio.sleep(3600) # hourly - try: - await check_due_tasks() - except Exception: - logger.exception("Error in notification loop") - try: - removed = await purge_old_read_notifications() - if removed: - logger.info("Notification retention: deleted %d read notification(s)", removed) - except Exception: - logger.exception("Error in notification retention cleanup") +async def _notification_tick() -> None: + try: + await check_due_tasks() + except Exception: + logger.exception("Error in notification loop") + removed = await purge_old_read_notifications() + if removed: + logger.info("Notification retention: deleted %d read notification(s)", removed) def start_notification_loop() -> None: global _notification_task if _notification_task is None or _notification_task.done(): - _notification_task = asyncio.create_task(_notification_loop()) + from scribe.services.background import start_periodic + _notification_task = start_periodic(3600, _notification_tick, label="notifications") # hourly # --------------------------------------------------------------------------- @@ -303,7 +293,6 @@ def start_notification_loop() -> None: async def create_in_app_notification(user_id: int, notif_type: str, payload: dict): """Create an in-app Notification record.""" - from scribe.models.notification import Notification async with async_session() as session: n = Notification(user_id=user_id, type=notif_type, payload=payload) session.add(n) @@ -316,11 +305,10 @@ async def _fire_share_email(user_id: int, subject: str, body_text: str) -> None: try: if not await is_smtp_configured(): return - async with async_session() as session: - user = await session.get(User, user_id) - if user and user.email: + email = await _get_user_email(user_id) + if email: html = _email_html(subject, f"

{body_text.replace(chr(10), '
')}

") - await send_email(user.email, subject, html) + await send_email(email, subject, html) except Exception: logger.exception("Share email notification failed for user %d", user_id) @@ -427,7 +415,6 @@ async def notify_group_added( async def list_in_app_notifications(user_id: int, unread_only: bool = True) -> list[dict]: - from scribe.models.notification import Notification async with async_session() as session: q = select(Notification).where(Notification.user_id == user_id) if unread_only: @@ -438,7 +425,6 @@ async def list_in_app_notifications(user_id: int, unread_only: bool = True) -> l async def unread_notification_count(user_id: int) -> int: - from scribe.models.notification import Notification async with async_session() as session: result = await session.execute( select(func.count()).where( @@ -450,8 +436,6 @@ async def unread_notification_count(user_id: int) -> int: async def mark_notification_read(user_id: int, notification_id: int) -> bool: - from scribe.models.notification import Notification - from datetime import timezone as tz async with async_session() as session: n = (await session.execute( select(Notification).where( @@ -461,21 +445,17 @@ async def mark_notification_read(user_id: int, notification_id: int) -> bool: )).scalar_one_or_none() if not n: return False - from datetime import datetime - n.read_at = datetime.now(tz.utc) + n.read_at = datetime.now(timezone.utc) await session.commit() return True async def mark_all_notifications_read(user_id: int) -> int: - from scribe.models.notification import Notification - from datetime import datetime, timezone as tz - from sqlalchemy import update as sa_update async with async_session() as session: result = await session.execute( sa_update(Notification) .where(Notification.user_id == user_id, Notification.read_at.is_(None)) - .values(read_at=datetime.now(tz.utc)) + .values(read_at=datetime.now(timezone.utc)) .returning(Notification.id) ) await session.commit() diff --git a/src/scribe/services/projects.py b/src/scribe/services/projects.py index cfae26e..afdfafd 100644 --- a/src/scribe/services/projects.py +++ b/src/scribe/services/projects.py @@ -208,54 +208,9 @@ async def get_project_summaries( async def get_project_summary(user_id: int, project_id: int) -> dict: - """Return task counts by status, note count, and last activity.""" - async with async_session() as session: - # Task counts by status - task_rows = await session.execute( - select(Note.status, func.count(Note.id)) - .where( - Note.user_id == user_id, - Note.project_id == project_id, - Note.status.isnot(None), - Note.deleted_at.is_(None), - ) - .group_by(Note.status) - ) - # Initialise all three lifecycle keys to 0 so consumers can sum them - # safely without `?? 0` guards. Frontend interface declares all three - # as required; rendering `undefined + N` yields NaN. - task_counts: dict[str, int] = {"todo": 0, "in_progress": 0, "done": 0} - for status, count in task_rows.fetchall(): - task_counts[status] = count - - # Note count (non-tasks) - note_count_result = await session.scalar( - select(func.count(Note.id)).where( - Note.user_id == user_id, - Note.project_id == project_id, - Note.status.is_(None), - Note.deleted_at.is_(None), - ) - ) - note_count = note_count_result or 0 - - # Last activity - last_activity_result = await session.scalar( - select(func.max(Note.updated_at)).where( - Note.user_id == user_id, - Note.project_id == project_id, - ) - ) - - from scribe.services.milestones import get_project_milestone_summary - milestone_summary = await get_project_milestone_summary(user_id, project_id) - - return { - "task_counts": task_counts, - "note_count": note_count, - "last_activity": last_activity_result.isoformat() if last_activity_result else None, - "milestone_summary": milestone_summary, - } + """Return task counts by status, note count, and last activity — the + one-project view of get_project_summaries (one rule, not two copies).""" + return (await get_project_summaries(user_id, [project_id]))[project_id] # --------------------------------------------------------------------------- @@ -279,8 +234,6 @@ async def list_projects_for_user(user_id: int, status: str | None = None) -> lis """Owned projects + shared projects, each dict has 'permission' field.""" from scribe.models.group import GroupMembership from scribe.models.share import ProjectShare - from scribe.services.access import PERMISSION_RANK - owned = await list_projects(user_id, status) owned_ids = {p.id for p in owned} @@ -307,13 +260,14 @@ async def list_projects_for_user(user_id: int, status: str | None = None) -> lis ) )).scalars().all() - seen: dict[int, str] = {} - for share in list(shared_direct) + list(shared_group): - if share.project_id in owned_ids: - continue - prev = seen.get(share.project_id) - if prev is None or PERMISSION_RANK[share.permission] > PERMISSION_RANK[prev]: - seen[share.project_id] = share.permission + from scribe.services.sharing import best_permission_by + seen = { + pid: perm + for pid, perm in best_permission_by( + list(shared_direct) + list(shared_group), "project_id" + ).items() + if pid not in owned_ids + } for pid, perm in seen.items(): if status: diff --git a/src/scribe/services/recurrence_scheduler.py b/src/scribe/services/recurrence_scheduler.py index 082751a..127569e 100644 --- a/src/scribe/services/recurrence_scheduler.py +++ b/src/scribe/services/recurrence_scheduler.py @@ -4,7 +4,7 @@ Every 15 minutes, creates the next occurrence of any recurring task whose spawn time has arrived — draining `recurrence_next_spawn_at`, which is armed on task completion. Without this job, recurring tasks would never recur. -Uses the BackgroundScheduler pattern shared with the other *_scheduler modules. +One ScheduledJob (services/scheduler.py), like the other *_scheduler modules. (Formerly event_scheduler.py, which also ran event reminders + CalDAV sync; those were removed when the calendar surface was retired.) """ @@ -13,52 +13,27 @@ from __future__ import annotations import asyncio import logging -from apscheduler.schedulers.background import BackgroundScheduler from apscheduler.triggers.interval import IntervalTrigger -logger = logging.getLogger(__name__) +from scribe.services.scheduler import ScheduledJob -_scheduler: BackgroundScheduler | None = None -_loop: asyncio.AbstractEventLoop | None = None +logger = logging.getLogger(__name__) async def _run_recurrence_spawn() -> None: from scribe.services.recurrence import spawn_recurring_tasks # noqa: PLC0415 - try: - await spawn_recurring_tasks() - except Exception: - logger.warning("Recurring-task spawn job failed", exc_info=True) + await spawn_recurring_tasks() -def _run_recurrence_spawn_threadsafe(loop: asyncio.AbstractEventLoop) -> None: - asyncio.run_coroutine_threadsafe(_run_recurrence_spawn(), loop) +_JOB = ScheduledJob("recurrence_spawn", _run_recurrence_spawn, label="Recurring-task spawn") def start_recurrence_scheduler(loop: asyncio.AbstractEventLoop) -> None: - global _scheduler, _loop - if _scheduler is not None: - return - _loop = loop - _scheduler = BackgroundScheduler() - # Spawn the next occurrence of due recurring tasks every 15 minutes. # Without this job, recurrence_next_spawn_at is armed on completion but # never drained, so recurring tasks never recur. - _scheduler.add_job( - _run_recurrence_spawn_threadsafe, - trigger=IntervalTrigger(minutes=15), - args=[loop], - id="recurrence_spawn", - replace_existing=True, - ) - - _scheduler.start() - logger.info("Recurrence scheduler started (recurring-task spawn every 15m)") + _JOB.start(loop, IntervalTrigger(minutes=15), describe="recurring-task spawn every 15m") def stop_recurrence_scheduler() -> None: - global _scheduler - if _scheduler is not None: - _scheduler.shutdown(wait=False) - _scheduler = None - logger.info("Recurrence scheduler stopped") + _JOB.stop() diff --git a/src/scribe/services/scheduler.py b/src/scribe/services/scheduler.py new file mode 100644 index 0000000..1489930 --- /dev/null +++ b/src/scribe/services/scheduler.py @@ -0,0 +1,78 @@ +"""One APScheduler job bridged into the asyncio loop — the shape the four +*_scheduler modules (recurrence spawn, auto-pin scan, trash purge, DB +maintenance) each used to carry a private copy of. + +APScheduler's BackgroundScheduler fires from a worker thread; the work is +async and must run on the app's loop, so the fire is bridged with +``run_coroutine_threadsafe``. Each job is a module-level singleton: start is +idempotent, stop shuts the scheduler down, and a job whose trigger the +operator can change (the maintenance hour) reschedules the live job instead +of restarting. +""" +from __future__ import annotations + +import asyncio +import logging +from collections.abc import Awaitable, Callable + +from apscheduler.schedulers.background import BackgroundScheduler + +logger = logging.getLogger(__name__) + + +class ScheduledJob: + """A named APScheduler job that awaits ``work()`` on the asyncio loop. + + ``work`` is an async callable; exceptions it raises are logged under + ``label`` and never propagate into APScheduler's thread. + """ + + def __init__(self, job_id: str, work: Callable[[], Awaitable[None]], *, label: str) -> None: + self.job_id = job_id + self._work = work + self.label = label + self._scheduler: BackgroundScheduler | None = None + self._loop: asyncio.AbstractEventLoop | None = None + + @property + def running(self) -> bool: + return self._scheduler is not None + + def _fire(self) -> None: + """APScheduler invokes this from its worker thread; bridge into the loop.""" + if self._loop is None: + logger.warning("%s scheduler: no loop registered", self.label) + return + + async def _runner() -> None: + try: + await self._work() + except Exception: + logger.exception("%s run failed", self.label) + + asyncio.run_coroutine_threadsafe(_runner(), self._loop) + + def start(self, loop: asyncio.AbstractEventLoop, trigger, *, describe: str = "") -> None: + """Start the job on ``trigger``. Idempotent — a second start is a no-op.""" + if self._scheduler is not None: + return + self._loop = loop + self._scheduler = BackgroundScheduler() + self._scheduler.add_job( + self._fire, trigger=trigger, id=self.job_id, replace_existing=True, + ) + self._scheduler.start() + logger.info("%s scheduler started%s", self.label, f" ({describe})" if describe else "") + + def reschedule(self, trigger, *, describe: str = "") -> None: + """Move the live job to a new trigger; a no-op when not running.""" + if self._scheduler is None: + return + self._scheduler.reschedule_job(self.job_id, trigger=trigger) + logger.info("%s scheduler rescheduled%s", self.label, f" to {describe}" if describe else "") + + def stop(self) -> None: + if self._scheduler is not None: + self._scheduler.shutdown(wait=False) + self._scheduler = None + logger.info("%s scheduler stopped", self.label) diff --git a/src/scribe/services/shape_ledger.py b/src/scribe/services/shape_ledger.py index bbe89c7..deadb38 100644 --- a/src/scribe/services/shape_ledger.py +++ b/src/scribe/services/shape_ledger.py @@ -31,6 +31,7 @@ from sqlalchemy import select from scribe.models import async_session from scribe.models.code_shape import CodeShape, CodeShapeEvent +from scribe.models.base import iso logger = logging.getLogger(__name__) @@ -1318,9 +1319,9 @@ async def shape_history( "classified_by": r.classified_by, "reason": r.reason, "first_seen_commit": r.first_seen_commit, "last_seen_commit": r.last_seen_commit, - "first_seen_at": r.created_at.isoformat() if r.created_at else None, - "vanished_at": r.vanished_at.isoformat() if r.vanished_at else None, - "recheck_at": r.recheck_at.isoformat() if r.recheck_at else None, + "first_seen_at": iso(r.created_at), + "vanished_at": iso(r.vanished_at), + "recheck_at": iso(r.recheck_at), "diverges_from": r.diverges_from, } for r in rows diff --git a/src/scribe/services/sharing.py b/src/scribe/services/sharing.py index 036c43c..3081e4e 100644 --- a/src/scribe/services/sharing.py +++ b/src/scribe/services/sharing.py @@ -10,6 +10,7 @@ from scribe.models.note import Note from scribe.models.project import Project from scribe.models.share import NoteShare, ProjectShare from scribe.models.user import User +from scribe.models.base import iso logger = logging.getLogger(__name__) @@ -31,7 +32,7 @@ async def _enrich_shares(session, shares) -> list[dict]: return result -def _deduplicate_by_permission(shares, id_attr: str) -> dict[int, str]: +def best_permission_by(shares, id_attr: str) -> dict[int, str]: """Return {resource_id: best_permission} keeping the highest-ranked permission per resource.""" from scribe.services.access import PERMISSION_RANK seen: dict[int, str] = {} @@ -210,7 +211,7 @@ async def list_shared_with_me(user_id: int) -> dict: ) )).scalars().all() - seen_projects = _deduplicate_by_permission(list(proj_direct) + list(proj_group), "project_id") + seen_projects = best_permission_by(list(proj_direct) + list(proj_group), "project_id") projects = [] for pid, perm in seen_projects.items(): @@ -223,7 +224,7 @@ async def list_shared_with_me(user_id: int) -> dict: "description": proj.description, "status": proj.status, "color": proj.color, - "updated_at": proj.updated_at.isoformat(), + "updated_at": iso(proj.updated_at), "owner_username": owner.username if owner else None, "permission": perm, }) @@ -241,7 +242,7 @@ async def list_shared_with_me(user_id: int) -> dict: ) )).scalars().all() - seen_notes = _deduplicate_by_permission(list(note_direct) + list(note_group), "note_id") + seen_notes = best_permission_by(list(note_direct) + list(note_group), "note_id") notes = [] for nid, perm in seen_notes.items(): @@ -253,7 +254,7 @@ async def list_shared_with_me(user_id: int) -> dict: "title": note.title, "is_task": note.is_task, "project_id": note.project_id, - "updated_at": note.updated_at.isoformat(), + "updated_at": iso(note.updated_at), "owner_username": owner.username if owner else None, "permission": perm, }) diff --git a/src/scribe/services/trash.py b/src/scribe/services/trash.py index 8c171a8..03d4654 100644 --- a/src/scribe/services/trash.py +++ b/src/scribe/services/trash.py @@ -8,15 +8,16 @@ trashed rows via `alive()`. from __future__ import annotations import uuid -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone -from sqlalchemy import or_, select, update +from sqlalchemy import delete as sql_delete, or_, select, update from scribe.models import async_session from scribe.models.note import Note from scribe.models.project import Project from scribe.models.milestone import Milestone from scribe.models.rulebook import Rulebook, RulebookTopic, Rule +from scribe.models.base import iso # entity_type -> Model. Used to resolve which table a trash op targets. _MODEL_FOR = { @@ -87,16 +88,15 @@ async def _cascade(session, user_id: int, etype: str, eid: int, batch: str, now) # FK CASCADE would handle a full DELETE on the project row, but the # soft-delete path keeps the project row alive; this guarantees the # rows are gone whether or not the project ever gets purged. - from sqlalchemy import delete as _sql_delete from scribe.models.rulebook import ( project_rule_suppressions, project_topic_suppressions, ) await session.execute( - _sql_delete(project_rule_suppressions) + sql_delete(project_rule_suppressions) .where(project_rule_suppressions.c.project_id == eid) ) await session.execute( - _sql_delete(project_topic_suppressions) + sql_delete(project_topic_suppressions) .where(project_topic_suppressions.c.project_id == eid) ) await _set(session, Project, [Project.user_id == user_id, Project.id == eid], batch, now) @@ -213,7 +213,6 @@ async def restore_entity(user_id: int, entity_type: str, entity_id: int) -> int async def purge(user_id: int, batch_id: str) -> int: """Hard-delete every row in the batch. Irreversible.""" - from sqlalchemy import delete as sql_delete n = 0 async with async_session() as session: for model in _ALL: @@ -241,7 +240,7 @@ async def list_trash(user_id: int) -> list[dict]: grp = batches.setdefault( r.deleted_batch_id, {"batch_id": r.deleted_batch_id, - "deleted_at": r.deleted_at.isoformat() if r.deleted_at else None, + "deleted_at": iso(r.deleted_at), "items": []}, ) grp["items"].append({ @@ -265,8 +264,6 @@ async def purge_expired(user_id: int, retention_days: int) -> int: user's short window prematurely destroy another's data. retention_days <= 0 disables auto-purge (returns 0 without touching anything). """ - from datetime import timedelta - from sqlalchemy import delete as sql_delete if retention_days <= 0: return 0 cutoff = datetime.now(timezone.utc) - timedelta(days=retention_days) diff --git a/src/scribe/services/trash_scheduler.py b/src/scribe/services/trash_scheduler.py index 3d5a5fa..63cbf51 100644 --- a/src/scribe/services/trash_scheduler.py +++ b/src/scribe/services/trash_scheduler.py @@ -1,80 +1,50 @@ """Daily APScheduler cron that purges expired trash. -Mirrors version_pinning_scheduler.py: a single global BackgroundScheduler job -at 03:30 UTC bridges into the asyncio loop to run the async purge. Iterates -every user and applies that user's own `trash_retention_days` setting; 0 -disables auto-purge for that user. +A single job at 03:30 UTC (services/scheduler.py). Iterates every user and +applies that user's own `trash_retention_days` setting; 0 disables auto-purge +for that user. """ from __future__ import annotations import asyncio import logging -from apscheduler.schedulers.background import BackgroundScheduler from apscheduler.triggers.cron import CronTrigger +from sqlalchemy import select +from scribe.models import async_session +from scribe.models.user import User from scribe.services import trash as trash_svc +from scribe.services.scheduler import ScheduledJob from scribe.services.settings import get_setting logger = logging.getLogger(__name__) -_scheduler: BackgroundScheduler | None = None -_loop: asyncio.AbstractEventLoop | None = None +async def _run_purge() -> None: + async with async_session() as session: + user_ids = (await session.execute(select(User.id))).scalars().all() -def _run_purge_threadsafe() -> None: - """APScheduler invokes this from a worker thread; bridge into the loop.""" - if _loop is None: - logger.warning("trash scheduler: no loop registered") - return - - async def _runner(): + purged = 0 + for uid in user_ids: + raw = await get_setting(uid, "trash_retention_days", "90") try: - from sqlalchemy import select + days = int(raw) + except (TypeError, ValueError): + days = 90 + purged += await trash_svc.purge_expired(uid, days) + if purged: + logger.info("trash purge: removed %d expired row(s)", purged) + else: + logger.debug("trash purge: nothing expired") - from scribe.models import async_session - from scribe.models.user import User - async with async_session() as session: - user_ids = (await session.execute(select(User.id))).scalars().all() - - purged = 0 - for uid in user_ids: - raw = await get_setting(uid, "trash_retention_days", "90") - try: - days = int(raw) - except (TypeError, ValueError): - days = 90 - purged += await trash_svc.purge_expired(uid, days) - if purged: - logger.info("trash purge: removed %d expired row(s)", purged) - else: - logger.debug("trash purge: nothing expired") - except Exception: - logger.exception("trash purge run failed") - - asyncio.run_coroutine_threadsafe(_runner(), _loop) +_JOB = ScheduledJob("trash_retention_purge", _run_purge, label="Trash retention") def start_trash_scheduler(loop: asyncio.AbstractEventLoop) -> None: - global _scheduler, _loop - if _scheduler is not None: - return - _loop = loop - _scheduler = BackgroundScheduler() - _scheduler.add_job( - _run_purge_threadsafe, - trigger=CronTrigger(hour=3, minute=30, timezone="UTC"), - id="trash_retention_purge", - replace_existing=True, - ) - _scheduler.start() - logger.info("Trash retention scheduler started (daily 03:30 UTC)") + _JOB.start(loop, CronTrigger(hour=3, minute=30, timezone="UTC"), describe="daily 03:30 UTC") def stop_trash_scheduler() -> None: - global _scheduler - if _scheduler is not None: - _scheduler.shutdown(wait=False) - _scheduler = None - logger.info("Trash retention scheduler stopped") + _JOB.stop() diff --git a/src/scribe/services/version_pinning_scheduler.py b/src/scribe/services/version_pinning_scheduler.py index 7189a86..a12f607 100644 --- a/src/scribe/services/version_pinning_scheduler.py +++ b/src/scribe/services/version_pinning_scheduler.py @@ -5,68 +5,39 @@ system promotes stable note versions before they get aged out of the rolling cap. Off-hours by design — the scan is cheap but not time- critical and doesn't need to interrupt regular activity. -Mirrors the BackgroundScheduler + threadsafe-async-call pattern used by -journal_scheduler.py. +One ScheduledJob (services/scheduler.py), like the other *_scheduler modules. """ from __future__ import annotations import asyncio import logging -from apscheduler.schedulers.background import BackgroundScheduler from apscheduler.triggers.cron import CronTrigger +from scribe.services.scheduler import ScheduledJob from scribe.services.version_pinning import scan_all_users_for_auto_pins logger = logging.getLogger(__name__) -_scheduler: BackgroundScheduler | None = None -_loop: asyncio.AbstractEventLoop | None = None + +async def _run_scan() -> None: + results = await scan_all_users_for_auto_pins() + total = sum(results.values()) + if total > 0: + logger.info( + "auto-pin scan: pinned %d version(s) across %d user(s)", + total, len(results), + ) + else: + logger.debug("auto-pin scan: no new pins") -def _run_scan_threadsafe() -> None: - """APScheduler invokes this from a worker thread; bridge into the - asyncio loop so the scan can await its DB operations.""" - if _loop is None: - logger.warning("version_pinning scheduler: no loop registered") - return - - async def _runner(): - try: - results = await scan_all_users_for_auto_pins() - total = sum(results.values()) - if total > 0: - logger.info( - "auto-pin scan: pinned %d version(s) across %d user(s)", - total, len(results), - ) - else: - logger.debug("auto-pin scan: no new pins") - except Exception: - logger.exception("auto-pin scan run failed") - - asyncio.run_coroutine_threadsafe(_runner(), _loop) +_JOB = ScheduledJob("version_pinning_auto_scan", _run_scan, label="Version pinning") def start_version_pinning_scheduler(loop: asyncio.AbstractEventLoop) -> None: - global _scheduler, _loop - if _scheduler is not None: - return - _loop = loop - _scheduler = BackgroundScheduler() - _scheduler.add_job( - _run_scan_threadsafe, - trigger=CronTrigger(hour=3, minute=0, timezone="UTC"), - id="version_pinning_auto_scan", - replace_existing=True, - ) - _scheduler.start() - logger.info("Version pinning scheduler started (daily 03:00 UTC)") + _JOB.start(loop, CronTrigger(hour=3, minute=0, timezone="UTC"), describe="daily 03:00 UTC") def stop_version_pinning_scheduler() -> None: - global _scheduler - if _scheduler is not None: - _scheduler.shutdown(wait=False) - _scheduler = None - logger.info("Version pinning scheduler stopped") + _JOB.stop() diff --git a/tests/test_api_keys.py b/tests/test_api_keys.py index 7a0e312..5708bdf 100644 --- a/tests/test_api_keys.py +++ b/tests/test_api_keys.py @@ -5,7 +5,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.services.api_keys import ( - _hash_key, + hash_token, _key_prefix, generate_key, create_api_key, @@ -29,7 +29,7 @@ def test_generate_key_uniqueness(): def test_hash_key_is_sha256(): key = "fmcp_testkey" - h = _hash_key(key) + h = hash_token(key) expected = hashlib.sha256(key.encode()).hexdigest() assert h == expected @@ -79,7 +79,7 @@ async def test_lookup_key_returns_none_for_unknown(): def test_hash_key_deterministic(): key = "fmcp_some_test_key_value" - assert _hash_key(key) == _hash_key(key) + assert hash_token(key) == hash_token(key) @pytest.mark.asyncio From 2a6c55dacb456a8f40c8e7ba781b00adbf55a392 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 21 Aug 2026 12:41:18 -0400 Subject: [PATCH 8/8] =?UTF-8?q?refactor(frontend):=20auth-shared.css,=20ap?= =?UTF-8?q?iErrorMessage,=20one=20date=20helper=20per=20shape,=20modal=20c?= =?UTF-8?q?anon=20in=20components.css=20=E2=80=94=20the=20frontend=20pass?= =?UTF-8?q?=20of=20the=20shape=20audit=20(#2831=20#2832,=20milestone=20296?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - assets/auth-shared.css: the five auth views carried byte-identical scoped copies of the page/card/brand/footer/field/input/error rules (~60 lines each); they now load one stylesheet the way the editors load editor-shared.css. .closed-msg/.error-block/.success-msg (identical bodies) are one .auth-note; the form rules are scoped under .auth-card so nothing leaks into the rest of the app. - api/client.apiErrorMessage(e, fallback): the one place the {"error"} envelope is unpacked; replaces ten six-line `"body" in e` catch blocks. - utils/dateFormat: fmtDate / fmtStamp / fmtLogStamp replace eight local formatDate/formatTime copies (three byte-identical pairs); the file’s old Calendar/Home helpers had no callers and are gone. useRelativeTime gains relativeTimeOrDate for the two workspace panels’ identical variant. - components.css now owns the .modal-* shape (overlay/card/title/message/ actions/btn/primary/danger). It was copied into four views and lived in editor-shared.css, which ConfirmDialog — styleless, teleported to — silently depended on: opened from SnippetDetailView before any editor view had loaded, it rendered unstyled. Views keep only their own overrides. Co-Authored-By: Claude Fable 5 --- frontend/src/api/client.ts | 14 +++ frontend/src/assets/auth-shared.css | 115 ++++++++++++++++++ frontend/src/assets/components.css | 76 ++++++++++++ frontend/src/assets/editor-shared.css | 47 ------- frontend/src/components/HistoryPanel.vue | 11 +- frontend/src/components/SystemsSection.vue | 25 ---- frontend/src/components/TaskLogSection.vue | 10 +- .../src/components/WorkspaceNoteEditor.vue | 17 +-- .../src/components/WorkspaceTaskPanel.vue | 19 +-- frontend/src/composables/useRelativeTime.ts | 12 ++ frontend/src/utils/dateFormat.ts | 81 ++++-------- frontend/src/views/ForgotPasswordView.vue | 92 +------------- frontend/src/views/LoginView.vue | 81 +----------- frontend/src/views/LogsView.vue | 14 +-- frontend/src/views/ProjectListView.vue | 36 ------ frontend/src/views/ProjectView.vue | 25 ---- frontend/src/views/RegisterInviteView.vue | 114 +---------------- frontend/src/views/RegisterView.vue | 109 +---------------- frontend/src/views/ResetPasswordView.vue | 113 +---------------- frontend/src/views/SettingsView.vue | 46 ++----- frontend/src/views/SnippetListView.vue | 36 ------ frontend/src/views/UserManagementView.vue | 31 ++--- 22 files changed, 290 insertions(+), 834 deletions(-) create mode 100644 frontend/src/assets/auth-shared.css diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 93ffbea..286d247 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -38,6 +38,20 @@ async function handleResponse(res: Response, path: string): Promise { return res.json() as Promise; } +/** + * The server's `{"error": "..."}` message from a failed call, or `fallback` + * when the failure carried none (network error, non-JSON body). The one place + * the error envelope is unpacked on the client — views used to restate this + * as a six-line `"body" in e` branch at every catch site. + */ +export function apiErrorMessage(e: unknown, fallback: string): string { + if (e && typeof e === "object" && "body" in e) { + const body = (e as { body?: { error?: unknown } }).body; + if (body && typeof body.error === "string" && body.error) return body.error; + } + return fallback; +} + export async function apiGet(path: string): Promise { const res = await fetch(path); return handleResponse(res, path); diff --git a/frontend/src/assets/auth-shared.css b/frontend/src/assets/auth-shared.css new file mode 100644 index 0000000..fe507dd --- /dev/null +++ b/frontend/src/assets/auth-shared.css @@ -0,0 +1,115 @@ +/* ── Auth surface (Login / Register / RegisterInvite / ForgotPassword / ResetPassword) ── + The five auth views used to carry byte-identical copies of these rules in + their scoped blocks (2026-08 shape audit). Loaded per view with + diff --git a/frontend/src/components/TaskLogSection.vue b/frontend/src/components/TaskLogSection.vue index 57c917b..dc8ed2e 100644 --- a/frontend/src/components/TaskLogSection.vue +++ b/frontend/src/components/TaskLogSection.vue @@ -3,6 +3,7 @@ import { ref, onMounted } from "vue"; import { apiGet, apiPost, apiPatch, apiDelete } from "@/api/client"; import { renderMarkdown } from "@/utils/markdown"; import type { TaskLog } from "@/types/task"; +import { fmtStamp } from "@/utils/dateFormat"; const props = defineProps<{ taskId: number }>(); @@ -15,13 +16,6 @@ const editingId = ref(null); const editContent = ref(""); const editDuration = ref(""); -function formatDate(iso: string): string { - const d = new Date(iso); - const datePart = d.toLocaleDateString(undefined, { month: "short", day: "numeric", year: "numeric" }); - const timePart = d.toLocaleTimeString(undefined, { hour: "2-digit", minute: "2-digit" }); - return `${datePart}, ${timePart}`; -} - function formatDuration(minutes: number): string { if (minutes < 60) return `${minutes} min`; const h = Math.floor(minutes / 60); @@ -128,7 +122,7 @@ onMounted(loadLogs);