feat(notes): a note's check is writable through both doors, and empty means empty (#3164, milestone 317 step 2)
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
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.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import logging
|
||||
import re
|
||||
from collections.abc import Iterable
|
||||
from datetime import date, datetime, timezone
|
||||
|
||||
from sqlalchemy import func, or_, select, text
|
||||
@@ -17,6 +18,45 @@ logger = logging.getLogger(__name__)
|
||||
_PARSED_FROM_BODY = frozenset({"title", "body", "tags"})
|
||||
|
||||
|
||||
# Text fields where EMPTY MEANS NULL (milestone 317). The sweep's whole signal
|
||||
# is `verify_with IS NULL` = "this is a decision, there is nothing to go and
|
||||
# check". An empty string that is not NULL makes a norm look like a constraint
|
||||
# nobody has verified, forever — and it would sit at the top of the sweep,
|
||||
# since never-checked sorts first. Sibling of rulebooks.NULLABLE_RULE_TEXT.
|
||||
NULLABLE_NOTE_TEXT = ("verify_with", "expires_when")
|
||||
|
||||
|
||||
def guard_check_fields(status: str | None, note_type: str | None) -> None:
|
||||
"""Raise unless a record in this shape may carry verify_with/expires_when.
|
||||
|
||||
Stated as an INVARIANT over the resulting record rather than a filter on
|
||||
which fields a caller passed, so it also catches the sideways route: a
|
||||
checked note being turned into a task, which no per-field gate would see.
|
||||
|
||||
Raises rather than dropping silently, for minted_kind's reason (#3129) — a
|
||||
silently-corrected write is the defect that reasoning exists to end, and a
|
||||
caller putting a check on the wrong record has an idea an error corrects
|
||||
and a default hides. Lives at the service, not either door, so REST and MCP
|
||||
cannot come to disagree about it.
|
||||
"""
|
||||
if status is not None:
|
||||
raise ValueError(
|
||||
"a task cannot carry verify_with/expires_when: a task's decay is "
|
||||
"its status, and a done issue records what happened rather than "
|
||||
"asserting something that can later go false. Put the check on the "
|
||||
"note the fact lives in, or clear the check before making this a "
|
||||
"task."
|
||||
)
|
||||
if note_type == "snippet":
|
||||
raise ValueError(
|
||||
"a snippet already has a check: verify_snippet(), which compares "
|
||||
"the recorded location and code against the repo and expires its "
|
||||
"own verdict when the code moves. verify_with/expires_when are the "
|
||||
"free-text form, for prose notes that assert a fact about "
|
||||
"something outside the repo."
|
||||
)
|
||||
|
||||
|
||||
def embed_note(note) -> None:
|
||||
"""Refresh a note's embedding, fire-and-forget.
|
||||
|
||||
@@ -108,7 +148,16 @@ async def create_note(
|
||||
task_kind: str = "work",
|
||||
arose_from_id: int | None = None,
|
||||
data: dict | None = None,
|
||||
verify_with: str | None = None,
|
||||
expires_when: str | None = None,
|
||||
) -> Note:
|
||||
# Empty means empty (NULLABLE_NOTE_TEXT), then the invariant. Both run
|
||||
# before anything is written, so an illegal shape never reaches the table.
|
||||
verify_with = verify_with or None
|
||||
expires_when = expires_when or None
|
||||
if verify_with or expires_when:
|
||||
guard_check_fields(status, note_type)
|
||||
|
||||
# Validate status/priority here so the MCP create_task path (which passes
|
||||
# them straight through) can't persist an out-of-enum value that the REST
|
||||
# route would have rejected — there's no DB CHECK on notes.status.
|
||||
@@ -152,6 +201,8 @@ async def create_note(
|
||||
task_kind=task_kind,
|
||||
arose_from_id=arose_from_id,
|
||||
data=data,
|
||||
verify_with=verify_with,
|
||||
expires_when=expires_when,
|
||||
)
|
||||
session.add(note)
|
||||
await session.commit()
|
||||
@@ -404,7 +455,20 @@ def minted_kind(kind: str) -> str:
|
||||
raise ValueError(f"kind must be one of {MINTABLE_KINDS}, got {kind!r}")
|
||||
|
||||
|
||||
async def update_note(user_id: int, note_id: int, **fields: object) -> Note | None:
|
||||
async def update_note(
|
||||
user_id: int, note_id: int, clear: Iterable[str] = (), **fields: object,
|
||||
) -> Note | None:
|
||||
"""Partial update. `clear` names fields to UNSET; **fields carries values.
|
||||
|
||||
Clearing is explicit and separate because a nullable field cannot be
|
||||
emptied by passing it: the MCP door reads "" as "leave this alone", so an
|
||||
agent filling two fields does not wipe the others, and a note that stops
|
||||
being a constraint genuinely needs its check removed. Naming the field is
|
||||
the one form that cannot happen by accident. The REST door, where a
|
||||
cleared form input arrives as "", reaches the same place through the
|
||||
NULLABLE_NOTE_TEXT normalisation below — two idioms, one outcome.
|
||||
(Same shape as rulebooks.update_rule, milestone 312 step 2.)
|
||||
"""
|
||||
async with async_session() as session:
|
||||
result = await session.execute(
|
||||
select(Note).where(Note.id == note_id, Note.user_id == user_id)
|
||||
@@ -416,6 +480,10 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No
|
||||
old_body = note.body
|
||||
old_title = note.title
|
||||
old_tags = list(note.tags or [])
|
||||
check_before = note.verify_with
|
||||
for key in clear:
|
||||
if key in NULLABLE_NOTE_TEXT:
|
||||
setattr(note, key, None)
|
||||
for key, value in fields.items():
|
||||
if not hasattr(note, key):
|
||||
continue
|
||||
@@ -445,7 +513,27 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No
|
||||
)
|
||||
elif key == "tags" and isinstance(value, list):
|
||||
value = _normalize_tags(value)
|
||||
elif key in NULLABLE_NOTE_TEXT:
|
||||
value = value or None
|
||||
elif key == "verified_at":
|
||||
# Not settable here. A stamp says somebody performed THIS
|
||||
# check, so it is written by the verification path and by a
|
||||
# restore, never by an ordinary edit that could mint one for a
|
||||
# check nobody ran.
|
||||
continue
|
||||
setattr(note, key, value)
|
||||
# The invariant, over the RESULTING record rather than over what was
|
||||
# passed — which is what catches a checked note being turned into a
|
||||
# task. Raised before commit, so nothing is persisted.
|
||||
if note.verify_with or note.expires_when:
|
||||
guard_check_fields(note.status, note.note_type)
|
||||
# A stamp certifies A CHECK, not a record. Rewrite or remove the check
|
||||
# and the old stamp certifies something that no longer exists, so it is
|
||||
# dropped and the note re-enters the sweep. The safe direction: a note
|
||||
# wrongly listed as due costs one look; a note wrongly vouched for
|
||||
# costs exactly what the sweep exists to catch.
|
||||
if note.verify_with != check_before:
|
||||
note.verified_at = None
|
||||
# A snippet's `data` is DERIVED from its body — so a write that moves
|
||||
# the body through this generic door must move the mirror with it
|
||||
# (#3128). Without this, PATCH /api/notes/<snippet_id> {body} left the
|
||||
|
||||
Reference in New Issue
Block a user