Files
FabledScribe/tests/test_mcp_tool_create_records.py
T
bvandeusenandClaude Opus 5 441a1ac31d
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
fix(#4016): records that cite each other are created together, and a guessed id is refused
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>
2026-09-14 08:33:56 -04:00

171 lines
7.9 KiB
Python

"""The batch create door, and the guessed-id refusal at every create/update door (#4016).
What the unit lane can pin: the door refuses a guessed id BEFORE the duplicate
gate or any write, a batch is refused whole, and a malformed record fails with
a message naming it. That the ids really come back resolved and atomic needs
Postgres — see test_integration_record_batch.py.
"""
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from scribe.services import record_refs
from scribe.services.record_batch import BatchItem, BatchMilestone, check_batch
pytestmark = pytest.mark.usefixtures("_bind_user")
def _max(n: int):
return patch.object(record_refs, "_max_note_id", AsyncMock(return_value=n))
# ── the refusal reaches every door ─────────────────────────────────────────
@pytest.mark.parametrize(("module", "name", "kwargs"), [
("scribe.mcp.tools.tasks", "create_task", {"title": "t", "body": "after #101"}),
("scribe.mcp.tools.tasks", "update_task", {"task_id": 5, "body": "see #101"}),
("scribe.mcp.tools.notes", "create_note", {"title": "n", "body": "index: #101"}),
("scribe.mcp.tools.notes", "update_note", {"note_id": 5, "body": "index: #101"}),
("scribe.mcp.tools.milestones", "create_milestone", {"project_id": 1, "title": "m", "body": "#101"}),
("scribe.mcp.tools.milestones", "update_milestone", {"project_id": 1, "milestone_id": 2, "body": "#101"}),
("scribe.mcp.tools.tasks", "start_planning", {"project_id": 1, "title": "p", "body": "step #101"}),
("scribe.mcp.tools.tasks", "create_records", {"records": [{"title": "a", "body": "#101"}]}),
])
async def test_every_writing_door_refuses_a_guessed_id_before_writing(module, name, kwargs):
import importlib
mod = importlib.import_module(module)
tool = getattr(mod, name)
# Every service a door could reach is a MagicMock that must stay untouched:
# the refusal has to land before the duplicate gate and before the write.
with _max(100), \
patch("scribe.services.dedup.find_duplicate_note", AsyncMock()) as dedup, \
patch("scribe.services.notes.create_note", AsyncMock()) as create, \
patch("scribe.services.notes.update_note", AsyncMock()) as update, \
patch("scribe.services.milestones.create_milestone", AsyncMock()) as ms_create, \
patch("scribe.services.milestones.update_milestone", AsyncMock()) as ms_update, \
patch("scribe.services.record_batch.create_batch", AsyncMock()) as batch, \
patch("scribe.services.planning.start_planning", AsyncMock()) as plan:
with pytest.raises(ValueError, match="#101"):
await tool(**kwargs)
for mock in (dedup, create, update, ms_create, ms_update, batch, plan):
mock.assert_not_awaited()
async def test_a_real_id_passes_the_door():
from scribe.mcp.tools.tasks import create_task
fake = MagicMock(id=90, project_id=None)
fake.to_dict.return_value = {"id": 90}
with _max(100), \
patch("scribe.mcp.tools.tasks.dedup_svc.find_duplicate_note", AsyncMock(return_value=None)), \
patch("scribe.mcp.tools.tasks.notes_svc.create_note", AsyncMock(return_value=fake)), \
patch("scribe.mcp.tools.tasks.systems_tools.attach_systems", AsyncMock()):
out = await create_task(title="t", body="follows #42")
assert out["id"] == 90
# ── create_records ─────────────────────────────────────────────────────────
async def test_unknown_record_fields_are_refused_not_dropped():
from scribe.mcp.tools.tasks import create_records
with pytest.raises(ValueError, match=r"record 2 has unknown field\(s\) \['milestone'\]"):
await create_records(records=[{"title": "a"}, {"title": "b", "milestone": 4}])
async def test_a_record_type_must_be_task_or_note():
from scribe.mcp.tools.tasks import create_records
with pytest.raises(ValueError, match="type must be 'task' or 'note'"):
await create_records(records=[{"title": "a", "type": "rule"}])
async def test_one_duplicate_blocks_the_whole_batch():
from scribe.mcp.tools.tasks import create_records
dup = MagicMock(id=33, title="existing", similarity=1.0, reason="title")
gate = AsyncMock(side_effect=[None, dup])
with patch("scribe.mcp.tools.tasks.dedup_svc.find_duplicate_note", gate), \
patch("scribe.mcp.tools.tasks.batch_svc.create_batch", AsyncMock()) as batch:
out = await create_records(records=[{"title": "fresh"}, {"title": "existing"}], project_id=3)
assert out["duplicate"] is True and out["record"] == 2 and out["existing_id"] == 33
assert "Nothing in the batch was created" in out["message"]
batch.assert_not_awaited()
async def test_create_records_returns_ids_in_order():
from scribe.mcp.tools.tasks import create_records
notes = []
for nid in (51, 53):
n = MagicMock(id=nid)
n.to_dict.return_value = {"id": nid}
notes.append(n)
with patch("scribe.mcp.tools.tasks.dedup_svc.find_duplicate_note", AsyncMock(return_value=None)), \
patch("scribe.mcp.tools.tasks.batch_svc.create_batch", AsyncMock(return_value=(None, notes))) as batch:
out = await create_records(
records=[{"title": "a"}, {"title": "b", "type": "note", "body": "after {{ref:1}}"}],
milestone_id=8,
)
assert out["ids"] == [51, 53]
items = batch.call_args.args[1]
assert [i.is_task for i in items] == [True, False]
assert batch.call_args.kwargs["milestone_id"] == 8
async def test_start_planning_hands_its_steps_to_the_service():
from scribe.mcp.tools.tasks import start_planning
with patch("scribe.mcp.tools.tasks.dedup_svc.find_duplicate_note", AsyncMock(return_value=None)), \
patch("scribe.mcp.tools.tasks.planning_svc.start_planning",
AsyncMock(return_value={"milestone": {"id": 1}})) as svc:
await start_planning(project_id=3, title="Plan", body="see {{ref:1}}",
steps=[{"title": "Step 1", "kind": "spike"}])
kwargs = svc.call_args.kwargs
assert kwargs["body"] == "see {{ref:1}}"
assert [s.title for s in kwargs["steps"]] == ["Step 1"]
assert kwargs["steps"][0].task_kind == "spike"
# ── check_batch: refused whole, before any write ───────────────────────────
def test_a_placeholder_past_the_end_is_refused():
with pytest.raises(ValueError, match=r"\{\{ref:3\}\} names no record"):
check_batch([BatchItem(title="a"), BatchItem(title="b", body="{{ref:3}}")])
def test_the_milestone_placeholder_needs_a_milestone():
with pytest.raises(ValueError, match=r"\{\{ref:milestone\}\}"):
check_batch([BatchItem(title="a", body="in {{ref:milestone}}")])
def test_the_milestone_placeholder_is_valid_with_one():
check_batch([BatchItem(title="a", body="in {{ref:milestone}}")],
BatchMilestone(title="m", body="first {{ref:1}}"))
def test_every_record_needs_a_title():
with pytest.raises(ValueError, match="record 2 has no title"):
check_batch([BatchItem(title="a"), BatchItem(title=" ")])
def test_a_batch_has_a_ceiling():
from scribe.services.record_batch import MAX_BATCH
with pytest.raises(ValueError, match="at most"):
check_batch([BatchItem(title=f"t{i}") for i in range(MAX_BATCH + 1)])
async def test_start_planning_refuses_placeholders_with_no_steps():
from scribe.services.planning import start_planning
with patch("scribe.services.planning.projects_svc.get_project", AsyncMock(return_value=MagicMock())), \
patch("scribe.services.planning.milestones_svc.create_milestone", AsyncMock()) as create:
with pytest.raises(ValueError, match="no steps were given"):
await start_planning(user_id=7, project_id=3, title="p", body="see {{ref:1}}")
create.assert_not_awaited()