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>

tests/test_create_tools_disambiguate.py

@@ -36,6 +36,7 @@ from tests.helpers import tool_doc as _doc
 _SURFACES = [
     ("scribe.mcp.tools.notes", "create_note"),
     ("scribe.mcp.tools.tasks", "create_task"),
+    ("scribe.mcp.tools.tasks", "create_records"),
     ("scribe.mcp.tools.tasks", "start_planning"),
     ("scribe.mcp.tools.snippets", "create_snippet"),
     ("scribe.mcp.tools.processes", "create_process"),

tests/test_integration_record_batch.py

@@ -0,0 +1,123 @@
+"""Real-Postgres proof that a batch create cannot be interfered with (#4016).
+
+What mocks cannot show: that the ids a placeholder is rewritten to are the ids
+the rows actually got, that two batches running at the same time each cite
+their OWN records, and that a failing batch leaves nothing behind. Those are
+properties of the sequence and the transaction, so they are measured against
+the real ones.
+"""
+import asyncio
+from unittest.mock import MagicMock, patch
+
+import pytest
+import pytest_asyncio
+from sqlalchemy import func, select
+
+from scribe.models import async_session
+from scribe.models.milestone import Milestone
+from scribe.models.note import Note
+from scribe.models.project import Project
+from scribe.services import record_refs
+from scribe.services.planning import start_planning
+from scribe.services.record_batch import BatchItem, create_batch
+from tests.helpers import ensure_user
+
+pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")]
+
+
+@pytest.fixture(autouse=True)
+def _no_embedding():
+    # embed_note detaches a task that loads the embedding model; this lane is
+    # about ids and transactions, and a background model load would outlive
+    # the test's event loop.
+    with patch("scribe.services.notes.embed_note", MagicMock()):
+        yield
+
+
+@pytest_asyncio.fixture
+async def seeded():
+    async with async_session() as s:
+        owner = await ensure_user(s, "record_batch_owner")
+        project = Project(user_id=owner.id, title="Batch target")
+        s.add(project)
+        await s.flush()
+        ids = {"owner": owner.id, "pid": project.id}
+        await s.commit()
+    return ids
+
+
+async def _count_in_project(pid: int) -> tuple[int, int]:
+    async with async_session() as s:
+        notes = (await s.execute(select(func.count(Note.id)).where(Note.project_id == pid))).scalar()
+        milestones = (await s.execute(
+            select(func.count(Milestone.id)).where(Milestone.project_id == pid)
+        )).scalar()
+    return notes, milestones
+
+
+async def test_a_plan_and_its_steps_cite_each_others_real_ids(seeded):
+    owner, pid = seeded["owner"], seeded["pid"]
+    out = await start_planning(
+        owner, pid, "Ship it",
+        body="First {{ref:1}}, then {{ref:2}}.",
+        steps=[
+            BatchItem(title="Step one", body="Part of {{ref:milestone}}; next is {{ref:2}}."),
+            BatchItem(title="Step two", body="Follows {{ref:1}}."),
+        ],
+    )
+    ms = out["milestone"]
+    one, two = out["steps"]
+    assert ms["body"] == f'First #{one["id"]} "Step one", then #{two["id"]} "Step two".'
+    assert one["body"] == f'Part of milestone {ms["id"]} "Ship it"; next is #{two["id"]} "Step two".'
+    assert two["body"] == f'Follows #{one["id"]} "Step one".'
+    assert one["milestone_id"] == two["milestone_id"] == ms["id"]
+    assert one["project_id"] == pid
+
+    # What was returned is what was stored — not a rewrite applied to the
+    # response alone.
+    async with async_session() as s:
+        stored = await s.get(Note, two["id"])
+        assert stored.body == f'Follows #{one["id"]} "Step one".'
+
+
+async def test_concurrent_batches_each_cite_their_own_records(seeded):
+    """The operator's scenario: several sessions creating at once. Each batch
+    must resolve {{ref:1}} to ITS first record, whatever the others take."""
+    owner, pid = seeded["owner"], seeded["pid"]
+
+    async def one_batch(tag: str):
+        return await create_batch(owner, [
+            BatchItem(title=f"{tag} first"),
+            BatchItem(title=f"{tag} second", body="points at {{ref:1}}"),
+        ], project_id=pid)
+
+    results = await asyncio.gather(*(one_batch(f"session-{i}") for i in range(6)))
+
+    all_ids = [n.id for _ms, notes in results for n in notes]
+    assert len(all_ids) == len(set(all_ids)), "the sequence handed out an id twice"
+    for _ms, (first, second) in results:
+        assert second.body == f'points at #{first.id} "{first.title}"'
+        async with async_session() as s:
+            assert (await s.get(Note, first.id)).title == first.title
+
+
+async def test_a_failing_batch_writes_nothing(seeded):
+    owner, pid = seeded["owner"], seeded["pid"]
+    before = await _count_in_project(pid)
+    with pytest.raises(ValueError, match="Invalid status"):
+        await start_planning(owner, pid, "Doomed", steps=[
+            BatchItem(title="fine"),
+            BatchItem(title="broken", status="someday"),
+        ])
+    assert await _count_in_project(pid) == before, "a partial batch (or its milestone) was left behind"
+
+
+async def test_a_guess_above_the_real_highest_id_is_refused(seeded):
+    owner, pid = seeded["owner"], seeded["pid"]
+    _ms, (made,) = await create_batch(owner, [BatchItem(title="anchor")], project_id=pid)
+    # `made` is the newest row, so anything just past it can only be a guess —
+    # unless a parallel test in the lane created more since, which only moves
+    # the ceiling up and keeps made.id itself a real id.
+    with pytest.raises(ValueError, match="does not exist yet"):
+        await record_refs.refuse_guessed_ids(f"#{made.id + record_refs.GUESS_WINDOW}")
+    await record_refs.refuse_guessed_ids(f"#{made.id}")

tests/test_mcp_tool_create_records.py

@@ -0,0 +1,170 @@
+"""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()

tests/test_mcp_tool_planning.py

@@ -16,7 +16,11 @@ async def test_start_planning_tool_delegates_to_service():
         from scribe.mcp.tools.tasks import start_planning
         out = await start_planning(project_id=3, title="Plan it")
     assert out["milestone"]["id"] == 5
-    assert mock.call_args.kwargs == {"user_id": 7, "project_id": 3, "title": "Plan it"}
+    # No body and no steps reach the service as None, so it seeds the template
+    # and takes the single-milestone path.
+    assert mock.call_args.kwargs == {
+        "user_id": 7, "project_id": 3, "title": "Plan it", "body": None, "steps": None,
+    }
 
 
 @pytest.mark.asyncio

tests/test_services_record_refs.py

@@ -0,0 +1,92 @@
+"""Guessed record ids are refused; `{{ref:N}}` placeholders resolve (#4016).
+
+The defect: a session predicts the ids its next creates will get and writes
+them into a body before the records exist; any other session's create takes
+those numbers. These pin the recogniser — what counts as a `#N`, where a guess
+can sit relative to the highest id — and the placeholder rewrite the batch
+create uses instead of guessing.
+"""
+from unittest.mock import AsyncMock, patch
+
+import pytest
+
+from scribe.services import record_refs
+from scribe.services.record_refs import (
+    GUESS_WINDOW,
+    cited_ids,
+    placeholder_keys,
+    resolve_placeholders,
+)
+
+
+@pytest.mark.parametrize(("text", "expected"), [
+    ("see #12 and (#34)", {12, 34}),
+    ("#7 at the start", {7}),
+    ("an entity { is not a reference", set()),
+    ("a fragment page#12 is not one", set()),
+    ("a colour #123abc is not one", set()),
+    ("path/#9 is not one", set()),
+    ("", set()),
+])
+def test_cited_ids_reads_standalone_hash_numbers_only(text, expected):
+    assert cited_ids(text) == expected
+
+
+def test_cited_ids_ignores_none_among_texts():
+    assert cited_ids(None, "#5", None) == {5}
+
+
+async def test_a_number_just_above_the_highest_id_is_a_guess():
+    with patch.object(record_refs, "_max_note_id", AsyncMock(return_value=100)):
+        assert await record_refs.guessed_ids("next is #101, then #102") == [101, 102]
+
+
+async def test_existing_ids_and_far_numbers_are_not_guesses():
+    """Below the max is a real record; far above it is not a Scribe id at all
+    (a PR, a forge issue) — neither may be refused."""
+    far = 100 + GUESS_WINDOW + 1
+    with patch.object(record_refs, "_max_note_id", AsyncMock(return_value=100)):
+        assert await record_refs.guessed_ids(f"#1 #100 #{far}") == []
+
+
+async def test_the_window_edge_is_still_a_guess():
+    edge = 100 + GUESS_WINDOW
+    with patch.object(record_refs, "_max_note_id", AsyncMock(return_value=100)):
+        assert await record_refs.guessed_ids(f"#{edge}") == [edge]
+
+
+async def test_text_without_references_never_queries():
+    """Most writes cite nothing; they must not pay a query for it."""
+    probe = AsyncMock(return_value=100)
+    with patch.object(record_refs, "_max_note_id", probe):
+        await record_refs.refuse_guessed_ids("no numbers here", None, "")
+    probe.assert_not_awaited()
+
+
+async def test_refusal_names_the_guess_and_the_fix():
+    with patch.object(record_refs, "_max_note_id", AsyncMock(return_value=100)):
+        with pytest.raises(ValueError) as exc:
+            await record_refs.refuse_guessed_ids("steps #101 and #103")
+    message = str(exc.value)
+    assert "#101, #103" in message
+    assert "create_records" in message and "{{ref:N}}" in message
+
+
+def test_placeholder_keys_tolerates_spacing():
+    assert placeholder_keys("{{ref:1}} {{ ref: 2 }} {{ref:milestone}}") == {"1", "2", "milestone"}
+
+
+def test_resolve_placeholders_rewrites_every_occurrence():
+    refs = {"1": '#40 "first"', "milestone": 'milestone 9 "plan"'}
+    out = resolve_placeholders("{{ref:1}} then {{ref:1}} in {{ref:milestone}}", refs)
+    assert out == '#40 "first" then #40 "first" in milestone 9 "plan"'
+
+
+def test_resolve_placeholders_passes_empty_through():
+    assert resolve_placeholders(None, {}) is None
+    assert resolve_placeholders("", {}) == ""
+
+
+def test_an_unknown_key_is_a_caller_bug_not_a_silent_leftover():
+    with pytest.raises(KeyError):
+        resolve_placeholders("{{ref:3}}", {"1": "#1"})