Files
FabledScribe/tests/test_services_notes_sweep.py
bvandeusen b51621fca7
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / TypeScript typecheck (push) Successful in 23s
CI & Build / integration (push) Successful in 25s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 24s
fix(tests): the sweep assertions read the WHERE clause, not the SELECT list (#3166)
select(Note) names every column, so searching the whole statement for
"notes.note_type" always finds the projection, and sql.index("notes.id") finds
the first column rather than the ORDER BY tiebreak. Both tests were asking the
wrong string.

The ordering test now asserts on the END of the statement, and the filter test
reads the WHERE clause — extracted by regex rather than split on a literal,
because the exact whitespace SQLAlchemy puts around WHERE is not something a
test should depend on.

The product is unchanged: the two assertions that mattered — NULLS FIRST
present, and no legal-carrier filter in the predicate — were both already
true.
2026-08-28 16:59:41 -04:00

243 lines
10 KiB
Python

"""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"]
def _where(sql: str) -> str:
"""Just the WHERE clause. `select(Note)` names every column, so searching
the whole statement for a column name always finds the SELECT list — which
is how one of these tests first failed for the wrong reason.
Matched by regex rather than split on a literal, because the exact
whitespace SQLAlchemy emits around WHERE is not something a test should
depend on.
"""
import re
m = re.search(r"\bWHERE\b(.*?)(?:\bORDER BY\b|$)", sql, re.S)
assert m, "no WHERE clause — the sweep must never select the whole table"
return m.group(1)
# ── 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.
Asserted on the END of the statement, not by searching it: `select(Note)`
names every column, so the first `notes.id` in the text is the SELECT
list, not the ORDER BY."""
sql = (await _sweep_sql()).rstrip()
assert sql.endswith("ORDER BY notes.verified_at ASC NULLS FIRST, 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."""
where = _where(await _sweep_sql())
assert "notes.status" not in where
assert "notes.note_type" not in where
# ── 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