diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 93bc5e0..f371fbf 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -120,6 +120,13 @@ _READ_ONLY_TOOLS = frozenset({ # prefix, so the completeness test below cannot derive it — the same # reason `enter_project` is spelled out above. "retrieval_telemetry", + # The note staleness sweep (milestone 317). A pure read — mark_note_verified + # is the write, and it is deliberately NOT here. Spelled out for + # retrieval_telemetry's reason: `notes_due_for_verification` matches none of + # the prefixes the completeness test derives from, so nothing would have + # prompted this decision. `rules_due_for_verification` is in the same + # position and is NOT listed — see #3191. + "notes_due_for_verification", }) # Read-SHAPED tools that must NOT be reachable with a read key — a getter that diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index c50c2eb..287490e 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -320,6 +320,96 @@ async def delete_note(note_id: int) -> dict: "message": f"Note {note_id} moved to trash. Restore with restore('{batch}')."} +async def notes_due_for_verification( + older_than_days: int = 0, project_id: int = 0, never_only: bool = False, +) -> dict: + """Which notes assert a FACT that nobody has confirmed lately. + + A corpus of notes holds two kinds of thing. Most are DECISIONS or records + of what happened — they have no truth value and cannot rot. A few assert a + fact about someone else's software: what a signing service does on a + duplicate upload, how a forge numbers its CI runs, what an updater + compares. Those go false silently, with nobody present, and a + cross-project reference note keeps being read as current by every project + that cites it. + + This lists the second kind, oldest verification first, NEVER-CHECKED AT + THE TOP — a note nobody has ever confirmed is a claim with no evidence + behind it at all. Each row carries `verify_with` in full, because you are + about to go and run it, plus `expires_when` and `days_since_verified`. + + Reach for it when curating, when a note's claim just contradicted what you + observed, or periodically. Then, per row: run the check, and call + mark_note_verified with what you found. + + Notes with no `verify_with` never appear, and that is correct — they are + decisions, and there is nothing to go and check. Do not "fix" their + absence by giving them checks: this list is only worth reading while + everything on it genuinely can go false. + + Args: + older_than_days: only notes last verified longer ago than this. + Never-checked notes always qualify — they are the most overdue + thing there is. 0 = no age filter. + project_id: narrow to one project. 0 = every project. Unlike the rules + sweep, this filter is safe: a note belongs to at most one project + outright, with none of the subscription and always-on paths that + would make a project filter UNDER-report a rule. + never_only: only notes nobody has ever verified. + """ + uid = current_user_id() + notes = await notes_svc.notes_due_for_verification( + uid, + older_than_days=older_than_days, + project_id=project_id or None, + never_only=never_only, + ) + return { + "notes": [notes_svc.verification_row(n) for n in notes], + "total": len(notes), + } + + +async def mark_note_verified(note_id: int, still_true: bool = True) -> dict: + """Record that you ran a note's check — and what it said. + + Call this AFTER actually running the note's `verify_with`, never on the + strength of the claim sounding plausible. A stamp nobody earned is worse + than no stamp: it moves the note to the bottom of the sweep and buys the + claim another long silence. + + `still_true=False` writes NOTHING. A note whose check failed is not in a + special state to be recorded — it is WRONG, and the honest next moves are + to correct it, supersede it, or find out why. So it stays at the top of + the sweep until someone deals with it, and the response tells you what the + note said would end it. + + Args: + note_id: the note whose check you ran. + still_true: True if the check passed. False if the fact it asserts is + no longer true — say so, that is the outcome worth having. + """ + uid = current_user_id() + note = await notes_svc.mark_note_verified(note_id, uid, still_true) + if note is None: + raise ValueError( + f"note {note_id} not found, not writable by you, or carries no " + f"verify_with (nothing to verify is not the same as verified)" + ) + data = notes_svc.verification_row(note) + data["verified"] = bool(still_true) + if not still_true: + data["next"] = ( + "This note is no longer true and is still being read as current " + "by anything that cites it. Correct it with update_note, write " + "the replacement with create_note(supersedes=[...]), or clear its " + "check with update_note(clear=[\"verify_with\"]) if it has stopped " + "asserting a fact at all. It stays at the top of " + "notes_due_for_verification until one of those happens." + ) + return data + + def register(mcp) -> None: for fn in ( list_notes, @@ -328,5 +418,7 @@ def register(mcp) -> None: update_note, find_duplicate_records, delete_note, + notes_due_for_verification, + mark_note_verified, ): mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index c2a3720..db8d61d 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -599,6 +599,154 @@ async def update_note( # permanent deletion. Both are reachable; neither is spelled `delete_note`. +# ── The sweep (milestone 317 step 3) ───────────────────────────────────────── +# +# A SIBLING of rulebooks.rules_due_for_verification, not a shared +# implementation, and deliberately so (note 3163). The row could have been +# shared; the QUERY cannot. That sweep scopes by rulebook ownership XOR project +# ownership because rules have no sharing ACL at all — no rule_shares, no +# can_read_rule. A note scopes by the note ACL, which is a different question +# with a different answer. What IS common — how a stamp reads, how old it is — +# lives in services/verification.py and is imported by both. + +async def notes_due_for_verification( + user_id: int, + older_than_days: int = 0, + project_id: int | None = None, + never_only: bool = False, +) -> list[Note]: + """Notes that carry a check, oldest verification first, never-checked top. + + THE QUERY THE COLUMNS EXIST FOR. `verify_with` and `expires_when` are + storage; this is what turns them into something that gets acted on. + Without it, note decay is caught only when a human reads the note and + disagrees — which is the case where the note was already believed. + + Ordered `verified_at` ASC **NULLS FIRST**: never-checked outranks + checked-long-ago, because a note nobody has ever confirmed is a claim with + no evidence behind it at all. Postgres sorts NULLs LAST on ASC by default, + so this is explicit — and getting it wrong would not error, it would + silently invert the one signal the sweep exists to carry. + + Notes with no `verify_with` never appear. Not an omission: they are + decisions, there is nothing to go and check, and listing them would dilute + the result until nobody reads it. + + Scoped with `browsable_notes_clause`, NOT the read scope (decision note + 2094): a sweep is a passive surface, and a record shared one-to-one must + not arrive in one unasked. + + Deliberately NOT filtered to non-task, non-snippet records even though the + write path (step 2) permits a check on nothing else. A row in that state + would be a row in an ILLEGAL state, and this is the one surface that could + tell somebody about it. Hiding it here to match the invariant would make + the sweep agree with a database it had stopped describing. + + Args: + user_id: whose notes. + older_than_days: only notes last verified longer ago than this. + Never-checked notes always qualify — they are the most overdue + thing there is. 0 = no age filter. Negative raises: it would mean + "everything", which is a different question than the one asked, + answered silently. + project_id: narrow to one project. None = every project. + never_only: only notes that have never been verified. + """ + from datetime import timedelta + + from scribe.services.access import browsable_notes_clause + + if older_than_days < 0: + raise ValueError( + f"older_than_days must be >= 0, got {older_than_days}. A negative " + f"window silently means 'everything', which is not what any caller " + f"of a staleness sweep is asking." + ) + + async with async_session() as session: + # One statement, not a fetch-then-filter: the ordering below is the + # database's, so it cannot disagree with itself across two halves. + stmt = ( + select(Note) + .where( + browsable_notes_clause(user_id), + Note.deleted_at.is_(None), + Note.verify_with.is_not(None), + ) + ) + if project_id is not None: + stmt = stmt.where(Note.project_id == project_id) + if never_only: + stmt = stmt.where(Note.verified_at.is_(None)) + elif older_than_days > 0: + cutoff = datetime.now(timezone.utc) - timedelta(days=older_than_days) + stmt = stmt.where( + or_(Note.verified_at.is_(None), Note.verified_at < cutoff) + ) + stmt = stmt.order_by(Note.verified_at.asc().nullsfirst(), Note.id) + return list((await session.execute(stmt)).scalars().all()) + + +def verification_row(note: Note) -> dict: + """One row of the sweep — the CHECK in full, unlike a listing. + + The opposite call from a browse: here the caller is about to go and run the + check, so the text they need IS the payload rather than the bloat. + """ + from scribe.services.verification import ( + days_since_verified, + last_verified_label, + ) + + return { + "id": note.id, + "title": note.title, + "project_id": note.project_id, + "verify_with": note.verify_with or "", + "expires_when": note.expires_when or "", + "last_verified": last_verified_label(note), + "days_since_verified": days_since_verified(note), + } + + +async def mark_note_verified( + note_id: int, user_id: int, still_true: bool = True, +) -> Note | None: + """Stamp a note as verified — or, when the check FAILED, refuse to. + + The asymmetry is the design: passing writes a stamp, failing writes + nothing. There is no "verified false" state, because a note whose check + failed is not a note in a special condition — it is a note that is WRONG, + and the honest resolutions are to correct it, supersede it, or find out + why. Recording the failure as a flag would let it sit there being false + with the sweep quietly satisfied that somebody had looked. + + So a failed check leaves `verified_at` untouched and the note stays at the + top of the sweep until someone actually deals with it. + + Write access, not read (rules 47/78): stamping is a mutation, and an + editor-share holder may make it while a viewer may not. + + Returns None when the note is not found, not writable, or carries no + `verify_with` — nothing to verify is a different answer from verified. + """ + from scribe.services.access import can_write_note + + async with async_session() as session: + note = (await session.execute( + select(Note).where(Note.id == note_id, Note.deleted_at.is_(None)) + )).scalars().first() + if note is None or not note.verify_with: + return None + if not await can_write_note(user_id, note_id): + return None + if still_true: + note.verified_at = datetime.now(timezone.utc) + await session.commit() + await session.refresh(note) + return note + + async def get_all_tags(user_id: int, q: str | None = None) -> list[str]: async with async_session() as session: if q: diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 536754e..bfc8488 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -16,6 +16,10 @@ from sqlalchemy import and_, delete as sql_delete, insert, or_, select from scribe.models import async_session from scribe.models.system import System from scribe.models.rulebook import Rulebook +from scribe.services.verification import ( + days_since_verified as _days_since_verified, + last_verified_label as _last_verified_label, +) logger = logging.getLogger(__name__) @@ -311,19 +315,12 @@ def _valid_tier(tier: str) -> str: return tier if tier in TIERS else "always_on" -def last_verified_label(rule: Rule) -> str | None: - """How long ago the rule's check passed — None when it carries no check. - - One helper because two surfaces need the same answer and the brief-dict - lesson in rule_brief's docstring is what happens otherwise: three copies - that had already drifted. `None` means "this rule is a decision, the - question does not apply"; "never" means "it is a fact and nobody has - confirmed it" — a distinction worth keeping, because the second is the - one worth acting on. - """ - if not rule.verify_with: - return None - return rule.verified_at.date().isoformat() if rule.verified_at else "never" +# Re-exported, not redefined. Notes gained the same trio in milestone 317 and +# this reading of it is genuinely common, so it moved to services/verification +# — the DRY win note 3163 names, as against sharing the QUERY, which the two +# record types cannot (a rule scopes by rulebook ownership, a note by the note +# ACL). Kept importable from here because callers already reach for it here. +last_verified_label = _last_verified_label def rule_brief(rule: Rule, **extra) -> dict: @@ -1455,14 +1452,7 @@ def verification_row(rule: Rule) -> dict: because "2026-06-14" and "74 days" prompt different reactions and only one of them is the question being asked. """ - from datetime import datetime, timezone - - days = None - if rule.verified_at is not None: - stamp = rule.verified_at - if stamp.tzinfo is None: - stamp = stamp.replace(tzinfo=timezone.utc) - days = (datetime.now(timezone.utc) - stamp).days + days = _days_since_verified(rule) return { "id": rule.id, "title": rule.title, diff --git a/src/scribe/services/verification.py b/src/scribe/services/verification.py new file mode 100644 index 0000000..865e627 --- /dev/null +++ b/src/scribe/services/verification.py @@ -0,0 +1,57 @@ +"""What a record's own check MEANS — shared by rules and notes. + +Two record types carry `verify_with` / `expires_when` / `verified_at`: rules +(milestone 312) and notes (milestone 317). What they share is BEHAVIOUR — how +a stamp is read, how old it is, what "never" means — not storage and not the +query. Note 3163 is the rule this file is an instance of: `semantic_search_*` +could never have been shared between them because a rule scopes by rulebook +ownership and a note scopes by the note ACL, so the two sweeps are siblings. +These functions are the part that genuinely is common, factored so it exists +once rather than twice. + +Everything here is pure, sync and DUCK-TYPED: it reads `verify_with` and +`verified_at` off whatever it is handed. That is deliberate. A shared base +class or a protocol would tie two SQLAlchemy models together to share four +lines of date arithmetic — the DRY costume, in the same note's words. +""" +from datetime import datetime, timezone + + +def last_verified_label(record) -> str | None: + """How long ago this record's check passed — None when it carries none. + + Three states, and the distinction between the last two is the whole point: + + - `None` — this record is a DECISION. There is nothing to go and check, + and the question does not apply. Most records. + - "never" — it asserts a fact and NOBODY HAS EVER CONFIRMED IT. The one + worth acting on, and why the sweep sorts these first. + - a date — somebody checked, then. + + Callers attach this to a payload only when it is not None, so "no key" and + "never verified" do not become two states every client has to tell apart. + """ + if not getattr(record, "verify_with", None): + return None + stamp = getattr(record, "verified_at", None) + return stamp.date().isoformat() if stamp else "never" + + +def days_since_verified(record) -> int | None: + """Whole days since the check last passed; None if it never has. + + Computed rather than left to the reader, because "2026-06-14" and "74 + days" prompt different reactions and only one of them is the question + being asked. + + Naive stamps are read as UTC. Postgres hands these back tz-aware, but a + restore, a fixture or a sqlite-backed test may not, and subtracting an + aware datetime from a naive one raises — which would make the sweep fail + on exactly the rows it exists to surface. + """ + stamp = getattr(record, "verified_at", None) + if stamp is None: + return None + if stamp.tzinfo is None: + stamp = stamp.replace(tzinfo=timezone.utc) + return (datetime.now(timezone.utc) - stamp).days diff --git a/tests/test_services_notes_sweep.py b/tests/test_services_notes_sweep.py new file mode 100644 index 0000000..64ee239 --- /dev/null +++ b/tests/test_services_notes_sweep.py @@ -0,0 +1,222 @@ +"""The note staleness sweep (milestone 317 step 3). + +The query is a sibling of the rules sweep, not a shared implementation — a +rule scopes by rulebook ownership, a note by the note ACL, so only the +BEHAVIOUR is common (services/verification). These pin the parts that would +fail silently rather than loudly: the ordering, which rows are eligible, and +the asymmetry of a failed check. +""" +from datetime import datetime, timedelta, timezone +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from tests.helpers import compiled_sql, fake_note, make_mock_session + + +async def _sweep_sql(**kwargs): + """The statement the sweep builds, as SQL text. + + No database: the SHAPE of the query is what is under test, and the + ordering clause in particular cannot be checked any other way without one. + The await happens INSIDE the patch — a coroutine created in the block and + awaited outside it would run against the real session. + """ + captured = {} + session = make_mock_session() + + async def _execute(stmt): + try: + captured["sql"] = compiled_sql(stmt) + except Exception: + # Literal-rendering a datetime bind is dialect-dependent and can + # raise. Only the tests asserting on a literal value (user_id, + # project_id) need that form, and none of those build a cutoff. + captured["sql"] = str(stmt) + result = MagicMock() + result.scalars.return_value.all.return_value = [] + return result + + session.execute = _execute + with patch("scribe.services.notes.async_session") as cls: + cls.return_value = session + from scribe.services.notes import notes_due_for_verification + await notes_due_for_verification(user_id=7, **kwargs) + return captured["sql"] + + +# ── the ordering, which is the whole signal ────────────────────────────────── + +@pytest.mark.asyncio +async def test_never_checked_sorts_first_not_last(): + """THE thing most likely to be got wrong, and it would not error. + + Postgres sorts NULLs LAST on ASC by default, so the obvious `ORDER BY + verified_at ASC` sinks every never-checked note below every checked one — + exactly inverting the signal the sweep exists to carry. A note nobody has + ever confirmed is a claim with no evidence behind it at all. + """ + sql = await _sweep_sql() + assert "ORDER BY notes.verified_at ASC NULLS FIRST" in sql + + +@pytest.mark.asyncio +async def test_the_order_is_total(): + """A tiebreak, so two notes verified in the same transaction do not swap + places between calls and make a page boundary lie.""" + sql = await _sweep_sql() + assert sql.index("NULLS FIRST") < sql.index("notes.id") + + +# ── which rows are eligible ────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_only_notes_that_carry_a_check_appear(): + """A note with no `verify_with` is not overdue — it is a decision, and + listing it would dilute the result until nobody reads it.""" + assert "notes.verify_with IS NOT NULL" in await _sweep_sql() + + +@pytest.mark.asyncio +async def test_trashed_notes_are_excluded(): + assert "notes.deleted_at IS NULL" in await _sweep_sql() + + +@pytest.mark.asyncio +async def test_the_scope_is_browse_not_read(): + """Decision note 2094: a sweep is a PASSIVE surface, so a record shared + one-to-one must not arrive in one unasked. `note_shares` is the tell — + its presence would mean the read scope leaked in.""" + sql = await _sweep_sql() + assert "notes.user_id = 7" in sql + assert "project_shares" in sql + assert "note_shares" not in sql + + +@pytest.mark.asyncio +async def test_a_task_carrying_a_check_is_NOT_filtered_out(): + """Deliberate. The write path permits a check on nothing but a plain note, + so such a row would be in an ILLEGAL state — and this is the one surface + that could tell somebody. Hiding it to match the invariant would make the + sweep agree with a database it had stopped describing.""" + sql = await _sweep_sql() + assert "notes.status IS NULL" not in sql + assert "notes.note_type" not in sql + + +# ── the filters ────────────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_never_only_narrows_to_the_unexamined(): + assert "notes.verified_at IS NULL" in await _sweep_sql(never_only=True) + + +@pytest.mark.asyncio +async def test_an_age_window_still_includes_the_never_checked(): + """They are the most overdue thing there is; a window that excluded them + would answer the opposite of the question.""" + sql = await _sweep_sql(older_than_days=30) + assert "notes.verified_at IS NULL" in sql + assert "notes.verified_at <" in sql + + +@pytest.mark.asyncio +async def test_never_only_wins_over_an_age_window(): + """Both together is a caller contradicting themselves; the narrower one is + the safe reading, and it must not emit a cutoff as well.""" + sql = await _sweep_sql(never_only=True, older_than_days=30) + assert "notes.verified_at <" not in sql + + +@pytest.mark.asyncio +async def test_a_project_filter_narrows(): + assert "notes.project_id = 4" in await _sweep_sql(project_id=4) + + +@pytest.mark.asyncio +async def test_a_negative_window_raises_rather_than_meaning_everything(): + """Silently answering a different question is the failure this guards.""" + with pytest.raises(ValueError, match="older_than_days"): + await _sweep_sql(older_than_days=-1) + + +# ── the stamp ──────────────────────────────────────────────────────────────── + +async def _mark(note, still_true=True, writable=True): + session = make_mock_session() + result = MagicMock() + result.scalars.return_value.first.return_value = note + session.execute = AsyncMock(return_value=result) + with patch("scribe.services.notes.async_session") as cls, \ + patch("scribe.services.access.can_write_note", + AsyncMock(return_value=writable)): + cls.return_value = session + from scribe.services.notes import mark_note_verified + return await mark_note_verified(note_id=1, user_id=7, + still_true=still_true), session + + +@pytest.mark.asyncio +async def test_a_passing_check_stamps_the_note(): + note = fake_note(verify_with="curl the docs", verified_at=None) + out, session = await _mark(note) + assert out is note + assert note.verified_at is not None + session.commit.assert_awaited() + + +@pytest.mark.asyncio +async def test_a_failing_check_writes_nothing(): + """The asymmetry IS the design. There is no "verified false" state, + because a note whose check failed is not in a special condition — it is + WRONG. Recording the failure as a flag would let it sit there being false + with the sweep quietly satisfied that somebody had looked.""" + note = fake_note(verify_with="curl the docs", verified_at=None) + out, session = await _mark(note, still_true=False) + assert out is note + assert note.verified_at is None, "a failed check must leave the stamp alone" + session.commit.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_a_note_with_no_check_cannot_be_verified(): + """Nothing to verify is a different answer from verified.""" + note = fake_note(verify_with=None) + out, _ = await _mark(note) + assert out is None + + +@pytest.mark.asyncio +async def test_a_reader_cannot_stamp(): + """Stamping is a mutation (rules 47/78) — an editor-share holder may make + it, a viewer may not.""" + note = fake_note(verify_with="curl the docs", verified_at=None) + out, _ = await _mark(note, writable=False) + assert out is None + assert note.verified_at is None + + +# ── the row ────────────────────────────────────────────────────────────────── + +def test_the_row_carries_the_check_in_full(): + """The opposite call from a listing: the caller is about to go and run it, + so the text IS the payload rather than the bloat.""" + note = fake_note( + id=9, title="Versioning", project_id=2, + verify_with="curl the AMO docs", expires_when="AMO allows re-signing", + verified_at=datetime.now(timezone.utc) - timedelta(days=74), + ) + from scribe.services.notes import verification_row + row = verification_row(note) + assert row["verify_with"] == "curl the AMO docs" + assert row["expires_when"] == "AMO allows re-signing" + assert row["days_since_verified"] == 74 + + +def test_a_never_checked_row_says_never_not_none(): + """None means "this is a decision, the question does not apply"; "never" + means "it asserts a fact and nobody has confirmed it". The second is the + one worth acting on, and collapsing them loses the sweep's point.""" + from scribe.services.notes import verification_row + row = verification_row(fake_note(verify_with="a check", verified_at=None)) + assert row["last_verified"] == "never" + assert row["days_since_verified"] is None