CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 1m2s
CI & Build / integration (push) Successful in 1m0s
CI & Build / Python tests (push) Successful in 1m39s
CI & Build / Build & push image (push) Successful in 39s
Sessions predicted the ids their next creates would get and wrote them into
plan bodies and reference notes before the records existed. The database
never collides; the sequence is shared by every session and user, so any
concurrent create took the guessed numbers and the references pointed at
someone else's records.
- create_records (new MCP tool) and start_planning(body=, steps=) create
their records in ONE transaction: insert, flush for the real ids, rewrite
{{ref:N}} / {{ref:milestone}} placeholders as #id "title", commit. No
prediction, no waiting, no stub records left behind when a batch fails.
Ids need not be consecutive and nothing depends on it.
- Every MCP create/update of a note, task or milestone refuses a #N sitting
just above the highest assigned id (within 50): that can only be a guess.
Refusal, not warning. Numbers far above the max (PRs, forge issues) pass.
- notes.build_note splits validation out of create_note so the batch
validates records exactly as a single create does.
- writing-plans and using-scribe say to pass steps up front and never write
an unassigned id; plugin version minted.
Integration test runs six concurrent batches and checks each resolves its
placeholders to its own records, and that a failing batch writes nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
132 lines
5.6 KiB
Python
132 lines
5.6 KiB
Python
"""Record references written into prose: refusing guessed ids, resolving placeholders.
|
|
|
|
THE DEFECT THIS EXISTS FOR (#4016)
|
|
|
|
A session about to create several records guesses the ids they will get — the
|
|
last id it saw, plus one, plus two — and writes those guesses into a plan body
|
|
or a reference note before the records exist. The database never collides:
|
|
ids come from one Postgres sequence per table, handed out once each. But that
|
|
sequence is shared by every session and every user, so any concurrent create
|
|
takes a guessed number, and the reference silently points at someone else's
|
|
record. More users make it worse, never better.
|
|
|
|
Two halves, and they are not alternatives:
|
|
|
|
- `{{ref:N}}` PLACEHOLDERS (services/record_batch.py) remove the reason to
|
|
guess. A batch create inserts its records, receives their real ids and
|
|
rewrites the placeholders inside ONE transaction, so there is no moment at
|
|
which an id has to be predicted.
|
|
- REFUSING A GUESSED ID is the backstop for a session that guesses anyway. It
|
|
is a refusal, not a warning, because a warning is an instruction and the
|
|
operator's point was exactly that instructions are not always followed.
|
|
|
|
HOW A GUESS IS RECOGNISED
|
|
|
|
A guess is always just ABOVE the highest id that exists — that is the only
|
|
place "the next id" can be. So a `#N` is refused when it sits in
|
|
(max_id, max_id + GUESS_WINDOW]. Numbers below the max are real records (or
|
|
were), and numbers far above it are not Scribe ids at all — a PR, a forge
|
|
issue — so both pass. What this cannot catch, by construction: a guess that
|
|
another session has ALREADY used. That reference points at a real record, and
|
|
nothing in the text distinguishes it from a deliberate one. The placeholders
|
|
are the fix for that case; this is not.
|
|
|
|
WHERE IT IS ENFORCED
|
|
|
|
At the MCP door (the create/update tools), not in the services. The defect is
|
|
an agent predicting ids; a person typing `#123` in the web editor has read that
|
|
number off a record. Enforcing it in `notes.create_note` would also reach
|
|
restores and internal writers that legitimately carry numbers from elsewhere.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import re
|
|
|
|
from sqlalchemy import func, select
|
|
|
|
from scribe.models import async_session
|
|
from scribe.models.note import Note
|
|
|
|
# `#123` as a standalone token. The lookbehind keeps out `{` entities,
|
|
# URL fragments (`page#12`) and doubled `##`; `\b` after the digits keeps out a
|
|
# hex colour like `#123abc`.
|
|
REF_RE = re.compile(r"(?<![\w&#/])#(\d+)\b")
|
|
|
|
# How far above the highest existing id a number still reads as a guess. It
|
|
# only has to cover how far ahead a session predicts, which is the size of the
|
|
# batch it is planning — a plan's steps, a handful of cross-linked notes. 50 is
|
|
# generous for that and still narrow enough that an unrelated number (a PR, an
|
|
# issue on a forge) landing inside it is rare. Widening it trades exactly that.
|
|
GUESS_WINDOW = 50
|
|
|
|
# `{{ref:3}}` names the third record of the same batch; `{{ref:milestone}}` names
|
|
# the milestone a start_planning call creates alongside its steps.
|
|
PLACEHOLDER_RE = re.compile(r"\{\{\s*ref:\s*(\d+|milestone)\s*\}\}")
|
|
|
|
|
|
def cited_ids(*texts: str | None) -> set[int]:
|
|
"""Every `#N` written in these texts."""
|
|
found: set[int] = set()
|
|
for text in texts:
|
|
if text:
|
|
found.update(int(m) for m in REF_RE.findall(text))
|
|
return found
|
|
|
|
|
|
async def _max_note_id() -> int:
|
|
# Across ALL users and including trashed rows: the sequence is global, so
|
|
# "the next id" is global too. Only the number is read, never a row.
|
|
async with async_session() as session:
|
|
return (await session.execute(select(func.max(Note.id)))).scalar() or 0
|
|
|
|
|
|
async def guessed_ids(*texts: str | None) -> list[int]:
|
|
"""The `#N` references in these texts that can only be predictions."""
|
|
cited = cited_ids(*texts)
|
|
if not cited:
|
|
return []
|
|
ceiling = await _max_note_id()
|
|
return sorted(n for n in cited if ceiling < n <= ceiling + GUESS_WINDOW)
|
|
|
|
|
|
async def refuse_guessed_ids(*texts: str | None) -> None:
|
|
"""Raise if any text cites a note/task id that has not been assigned yet.
|
|
|
|
Raised before anything is written, so the caller's create or update does
|
|
not happen. The message names the fix, because the agent reading it is
|
|
mid-turn and can apply it immediately.
|
|
"""
|
|
guessed = await guessed_ids(*texts)
|
|
if not guessed:
|
|
return
|
|
listed = ", ".join(f"#{n}" for n in guessed)
|
|
raise ValueError(
|
|
f"{listed} does not exist yet — it reads as a guessed id, and a guess "
|
|
"is taken by whatever any other session creates next. Nothing was "
|
|
"written. An id exists only once a create call RETURNS it: create the "
|
|
"records first and cite the ids they come back with, or create them "
|
|
"together with create_records / start_planning(steps=...) and write "
|
|
"{{ref:N}} where the Nth record's id belongs. If this number is not a "
|
|
"Scribe record (a PR, an issue elsewhere), write it without the '#'."
|
|
)
|
|
|
|
|
|
def placeholder_keys(*texts: str | None) -> set[str]:
|
|
"""Every `{{ref:...}}` key in these texts: "1", "2", …, "milestone"."""
|
|
keys: set[str] = set()
|
|
for text in texts:
|
|
if text:
|
|
keys.update(PLACEHOLDER_RE.findall(text))
|
|
return keys
|
|
|
|
|
|
def resolve_placeholders(text: str | None, refs: dict[str, str]) -> str | None:
|
|
"""Rewrite `{{ref:...}}` using `refs` (key -> rendered reference).
|
|
|
|
Callers validate the keys first; an unknown key here is a bug in the
|
|
caller, so it raises rather than leaving a placeholder in a stored record.
|
|
"""
|
|
if not text:
|
|
return text
|
|
return PLACEHOLDER_RE.sub(lambda m: refs[m.group(1)], text)
|