feat(notes): the sweep — which notes assert a fact nobody has confirmed (#3166, milestone 317 step 3)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 28s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 46s
CI & Build / Build & push image (push) Skipped

The read half. `notes_due_for_verification` + `mark_note_verified` + the MCP
pair, 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
getting this wrong would not error — it would silently invert the one signal
the sweep exists to carry, which is why it has a test of its own.

A SIBLING of rules_due_for_verification, not a shared implementation, and this
milestone is a deliberate self-application of note 3163: the row could have
been shared, the QUERY could not. That sweep scopes by rulebook ownership XOR
project ownership because rules have no sharing ACL at all; a note scopes by
the note ACL — browse, not read, so a record shared one-to-one never arrives
in a passive surface unasked (decision 2094).

What genuinely IS common moved to services/verification.py: how a stamp reads,
how old it is, and the three states `last_verified` distinguishes — None ("a
decision, the question does not apply"), "never" ("a fact nobody has
confirmed"), a date. Rulebooks now imports it rather than defining it, so this
is a consolidation and not a third copy.

A failed check writes NOTHING, carried over from 312: there is no "verified
false" state, because a note whose check failed is not in a special condition
— it is WRONG, and recording the failure as a flag would let it sit there
being false with the sweep quietly satisfied that somebody had looked.

Two decisions worth naming:

The sweep does NOT filter to non-task, non-snippet records even though the
write path permits a check on nothing else. Such a row would be in an ILLEGAL
state and this is the one surface that could say so; hiding it to match the
invariant would make the sweep agree with a database it had stopped
describing.

A negative `older_than_days` raises instead of meaning "everything" — silently
answering a different question is the failure shape this guards.

`notes_due_for_verification` is classified read-only in server.py, spelled out
because its name matches none of the prefixes the completeness test derives
from. `rules_due_for_verification` is in the same position and is NOT listed,
so it fails closed for read keys today — filed as #3191 rather than fixed
here, since widening an auth boundary on a tool I did not write is the
operator's call.
This commit is contained in:
2026-08-28 16:39:50 -04:00
parent 4736a0a0ba
commit 8489206224
6 changed files with 537 additions and 21 deletions
+222
View File
@@ -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