CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 38s
CI & Build / integration (push) Successful in 32s
CI & Build / Python tests (push) Failing after 49s
CI & Build / Build & push image (push) Skipped
The rules path's three lessons (#3096), inherited: EMPTY MEANS NULL. The sweep's whole signal is `verify_with IS NULL` = "this is a decision, there is nothing to go and check". A "" that is not NULL makes a norm look like a constraint nobody has verified — and never-checked sorts FIRST, so it would sit at the top of the sweep forever. CLEARING IS EXPLICIT. At the MCP door "" means "leave this alone", so an agent updating a body does not wipe a check it was never asked about — which leaves no value meaning "remove it". `clear` names the field, and naming it cannot happen by accident. The REST door, where a cleared form input arrives as "", reaches the same place through normalisation: two idioms, one outcome. THE STAMP CERTIFIES A CHECK, NOT A RECORD. Rewrite or clear `verify_with` and `verified_at` is dropped, so the note re-enters the sweep. A note wrongly listed as due costs one look; a note wrongly vouched for costs exactly what the sweep exists to catch. `verified_at` is also no longer settable through an ordinary edit — a stamp says somebody performed THIS check, and minting one from a write that ran no check is the one thing that would make the sweep lie. And one this path adds: not every record may carry a check. A task's decay is its status — a done issue records what happened rather than asserting something that can go false — and a snippet already has verify_snippet, which compares its recorded location and code against the repo and expires its own verdict. Both are refused with a message naming the alternative, never dropped silently (minted_kind's reasoning, #3129), and the gate lives at the service so the two doors cannot come to disagree. Written as an INVARIANT over the resulting record, not a filter on which fields were passed. That is what catches the sideways route — a checked note being turned into a task, a write that names no check at all and would sail past any per-field gate. The MCP docstrings carry the norm-vs-constraint test, because at that door the docstring IS the contract and a field described only as "how to verify this" gets filled in on every note. Step 5 does this properly across the instruction surfaces; this is the minimum that stops the field being misused on arrival. tests/helpers gains `drive_update_note` — the patch stack for driving update_note, written twice before this and now once. The note-shaped fakes gain the trio explicitly, for fake_note's own stated reason: an unset attribute is a truthy MagicMock, and a truthy verify_with reads as a check that is there.
173 lines
7.1 KiB
Python
173 lines
7.1 KiB
Python
"""A note's own check — verify_with / expires_when (milestone 317 step 2).
|
|
|
|
Three behaviours the rules path (#3096) had to get right and this one inherits:
|
|
empty means NULL, clearing is explicit, and rewriting the check drops the
|
|
stamp. Plus one this path adds: not every record may carry a check, and the
|
|
rule is an INVARIANT over the record rather than a filter on the write.
|
|
"""
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
from tests.helpers import drive_update_note as _update
|
|
from tests.helpers import fake_note, make_mock_session
|
|
|
|
|
|
def _checkable(**over):
|
|
"""A plain note — no status, no snippet type, no check. `fake_note` is a
|
|
MagicMock, so every one of these must be set explicitly: an unset
|
|
attribute is a truthy mock, which would look like a check that is there."""
|
|
base = dict(
|
|
status=None, note_type="note",
|
|
verify_with=None, expires_when=None, verified_at=None,
|
|
project_id=None,
|
|
)
|
|
base.update(over)
|
|
return fake_note(**base)
|
|
|
|
|
|
async def _create(**kwargs):
|
|
session = make_mock_session()
|
|
captured = {}
|
|
session.add = MagicMock(side_effect=lambda obj: captured.update(
|
|
verify_with=getattr(obj, "verify_with", "MISSING"),
|
|
expires_when=getattr(obj, "expires_when", "MISSING"),
|
|
))
|
|
with patch("scribe.services.notes.async_session") as cls, \
|
|
patch("scribe.services.notes.embed_note", MagicMock()), \
|
|
patch("scribe.services.notes._maybe_reactivate_project", AsyncMock()):
|
|
cls.return_value = session
|
|
from scribe.services.notes import create_note
|
|
await create_note(user_id=1, title="t", **kwargs)
|
|
return captured
|
|
|
|
|
|
# ── empty means empty ────────────────────────────────────────────────────────
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_empty_check_is_stored_as_null_not_as_a_blank():
|
|
"""The sweep's whole signal is `verify_with IS NULL` = "this is a decision,
|
|
there is nothing to check". A "" that is not NULL makes a norm look like a
|
|
constraint nobody has verified — and never-checked sorts FIRST, so it would
|
|
sit at the top of the sweep forever."""
|
|
assert (await _create(verify_with="", expires_when=""))["verify_with"] is None
|
|
|
|
note = _checkable(verify_with="curl the docs")
|
|
await _update(note, verify_with="")
|
|
assert note.verify_with is None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_check_is_stored_when_given():
|
|
captured = await _create(verify_with="curl the AMO docs", expires_when="AMO allows re-signing")
|
|
assert captured["verify_with"] == "curl the AMO docs"
|
|
assert captured["expires_when"] == "AMO allows re-signing"
|
|
|
|
|
|
# ── clearing is explicit ─────────────────────────────────────────────────────
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_clear_unsets_a_field_the_mcp_door_cannot_empty():
|
|
"""At the MCP door "" means "leave this alone", so an agent updating a body
|
|
does not wipe a check it was never asked about. That leaves no value
|
|
meaning "remove it" — hence naming the field, which cannot happen by
|
|
accident."""
|
|
note = _checkable(verify_with="a check", expires_when="a state")
|
|
await _update(note, clear=["verify_with"])
|
|
assert note.verify_with is None
|
|
assert note.expires_when == "a state", "clearing one must not clear the other"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_clear_ignores_a_field_that_is_not_clearable():
|
|
note = _checkable(title="keep me", verify_with="a check")
|
|
await _update(note, clear=["title"])
|
|
assert note.title == "keep me"
|
|
|
|
|
|
# ── the stamp certifies a check, not a record ────────────────────────────────
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_rewriting_the_check_drops_the_stamp():
|
|
note = _checkable(verify_with="the old check", verified_at="2026-01-01")
|
|
await _update(note, verify_with="a different check")
|
|
assert note.verified_at is None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_clearing_the_check_drops_the_stamp():
|
|
note = _checkable(verify_with="the old check", verified_at="2026-01-01")
|
|
await _update(note, clear=["verify_with"])
|
|
assert note.verified_at is None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_unchanged_check_keeps_its_stamp():
|
|
"""Only a CHANGE invalidates it — otherwise every unrelated edit would
|
|
re-enter the note into the sweep and the signal would mean nothing."""
|
|
note = _checkable(verify_with="the same check", verified_at="2026-01-01")
|
|
await _update(note, title="a new title", verify_with="the same check")
|
|
assert note.verified_at == "2026-01-01"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_stamp_cannot_be_set_through_an_ordinary_edit():
|
|
"""A stamp says somebody performed THIS check. Minting one from a write
|
|
that ran no check is the one thing that would make the sweep lie."""
|
|
note = _checkable(verify_with="a check", verified_at=None)
|
|
await _update(note, verified_at="2026-08-28")
|
|
assert note.verified_at is None
|
|
|
|
|
|
# ── the invariant: which records may carry a check ───────────────────────────
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_task_is_refused_a_check():
|
|
with pytest.raises(ValueError, match="task"):
|
|
await _create(status="todo", verify_with="a check")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_snippet_is_refused_and_told_where_to_go():
|
|
"""The message has to name the alternative, or the caller is left with a
|
|
refusal and no route."""
|
|
with pytest.raises(ValueError, match="verify_snippet"):
|
|
await _create(note_type="snippet", verify_with="a check")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_turning_a_checked_note_into_a_task_is_refused():
|
|
"""THE reason this is an invariant over the resulting record and not a
|
|
filter on which fields were passed. This write names no check at all, and
|
|
would sail past any per-field gate."""
|
|
note = _checkable(verify_with="a check")
|
|
with pytest.raises(ValueError, match="task"):
|
|
await _update(note, status="todo")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_unchecked_note_can_still_become_a_task():
|
|
"""The invariant must not make ordinary promotion impossible."""
|
|
note = _checkable()
|
|
await _update(note, status="todo")
|
|
assert note.status == "todo"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_clearing_the_check_in_the_same_write_lets_it_become_a_task():
|
|
"""The error tells the caller to clear the check first; doing both at once
|
|
has to actually work, or the advice is wrong."""
|
|
note = _checkable(verify_with="a check")
|
|
await _update(note, status="todo", clear=["verify_with"])
|
|
assert note.status == "todo"
|
|
assert note.verify_with is None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_ordinary_note_write_is_untouched_by_any_of_this():
|
|
"""The common case: no check, nothing to guard, nothing to reset."""
|
|
note = _checkable(title="before")
|
|
await _update(note, title="after")
|
|
assert note.title == "after"
|
|
assert note.verify_with is None
|
|
assert note.verified_at is None
|