diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index b5730ef..2f5533e 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "2026.09.11.2026", + "version": "2026.09.14.1233", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 7206511..14523e5 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -161,6 +161,13 @@ Two constraints on *how* that's achieved: context than the operator has now: commit messages, task bodies, and any record that cites another. + **An id exists only once a create call returns it.** Never write the id you + expect a record to get — every session and user draws from one sequence, + so the number goes to whoever creates next, and Scribe refuses a body that + cites an id not yet assigned. Records that must cite each other are created + together — `create_records` or `start_planning(steps=...)` — with + `{{ref:N}}` where the Nth record's id belongs. + 9. **State updates in place; chronicles don't.** A dev-log records what *happened* — write it once, never rewrite it. A durable finding (how a subsystem works, a measured number) lives in that System's **reference diff --git a/plugin/skills/writing-plans/SKILL.md b/plugin/skills/writing-plans/SKILL.md index 67bf6f7..c2d3969 100644 --- a/plugin/skills/writing-plans/SKILL.md +++ b/plugin/skills/writing-plans/SKILL.md @@ -34,13 +34,26 @@ already done. It creates a **milestone** (the plan container) seeded with a design template and returns the milestone id plus the project's applicable rules. The plan lives in that milestone: -- The **design/intent** goes in the milestone `body` — edit it with +- The **design/intent** goes in the milestone `body` — pass it as + `start_planning(..., body=...)`, or edit it later with `update_milestone(milestone_id, body=...)`. -- Each **step** is its own task under the milestone — create it with - `create_task(milestone_id=)` and track it with status + - `add_task_log`. Steps are first-class tasks, **not** checkboxes in the body. +- Each **step** is its own task under the milestone. When you know the steps, + pass them in the same call — `start_planning(..., steps=[{title, body}, …])` + — and the milestone and every step are created together. Add steps later + with `create_records(milestone_id=, records=[…])`, or + `create_task` for one. Track each with status + `add_task_log`. Steps are + first-class tasks, **not** checkboxes in the body. - Read the whole plan back with `get_milestone` (body + its step-tasks). +**Never write an id you have not been given.** A plan body that says "see +#4012" before #4012 exists is a guess, and other sessions and users are +creating records from the same sequence at the same time — the number goes to +whoever creates next, and the plan then points at their record. Scribe refuses +a body citing an id that has not been assigned. Where the plan and its steps +need to cite each other, write a placeholder instead: `{{ref:N}}` is the Nth +step in the list, `{{ref:milestone}}` is the milestone. They are replaced with +the real id and title as the records are created. + **Do not** write plans or specs to local `.md` files — the milestone is the record, not a file on disk. (The old `kind=plan` task is retired; `start_planning` no longer creates one.) diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index cde1687..97f73a3 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -16,6 +16,7 @@ from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import trash as trash_svc +from scribe.services.record_refs import refuse_guessed_ids async def list_milestones(project_id: int) -> dict: @@ -83,6 +84,7 @@ async def create_milestone( status: active (default) or done. """ uid = current_user_id() + await refuse_guessed_ids(title, description, body) milestone = await milestones_svc.create_milestone( uid, project_id=project_id, @@ -127,6 +129,7 @@ async def update_milestone( fields["status"] = status if order_index >= 0: fields["order_index"] = order_index + await refuse_guessed_ids(title, description, body) milestone = await milestones_svc.update_milestone(uid, milestone_id, **fields) if milestone is None: raise ValueError(f"milestone {milestone_id} not found") diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index aa5195c..f4df26a 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -22,6 +22,7 @@ from scribe.services import supersession as supersession_svc from scribe.services import systems as systems_svc from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled +from scribe.services.record_refs import refuse_guessed_ids async def list_notes( @@ -169,6 +170,11 @@ async def create_note( instead (no duplicate bloat / no stale RAG copies). Set true only when you're sure this is a genuinely distinct note. + AN ID EXISTS ONLY ONCE A CREATE RETURNS IT. A body citing a `#N` that has + not been assigned yet is refused — every session and user draws from one + sequence, so an expected id is taken by whoever creates next. Records that + must cite each other: create_records, with {{ref:N}} placeholders. + Returns the created note object including its assigned id, OR — when a near-duplicate is found and force is false — {"duplicate": true, "existing_id": ..., "message": ...} and nothing is created. A tagged @@ -177,6 +183,7 @@ async def create_note( create the missing System, or deliberately leave it untagged. """ uid = current_user_id() + await refuse_guessed_ids(title, body) if not force: dup = await dedup_svc.find_duplicate_note( uid, title, body, project_id=project_id or None, @@ -269,6 +276,7 @@ async def update_note( fields["verify_with"] = verify_with if expires_when: fields["expires_when"] = expires_when + await refuse_guessed_ids(title, body) note = await notes_svc.update_note( uid, note_id, clear=clear or (), **fields ) diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 32eef56..885b4ce 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -29,11 +29,13 @@ from scribe.services import notes as notes_svc # guard into a MagicMock that approves anything. from scribe.services.notes import minted_kind from scribe.services import planning as planning_svc +from scribe.services import record_batch as batch_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import systems as systems_svc from scribe.services import task_logs as task_logs_svc from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled +from scribe.services.record_refs import refuse_guessed_ids async def list_tasks( @@ -174,6 +176,13 @@ async def create_task( BLOCKED and the existing task's id is returned so you update it instead. Set true only for a genuinely distinct task. + AN ID EXISTS ONLY ONCE A CREATE RETURNS IT. Never write the id you expect + a record to get: every session and user draws from one sequence, so the + number is taken by whoever creates next. A body citing a `#N` that has not + been assigned yet is refused. Creating several records that cite each + other? Use create_records (or start_planning(steps=...)) and write + {{ref:N}} — the real ids are filled in as they are created. + Returns the created task, OR — when a near-duplicate is found and force is false — {"duplicate": true, "existing_id": ..., "message": ...} (nothing created). A tagged record shows its `systems`; created untagged in a @@ -189,6 +198,7 @@ async def create_task( "milestone + seeds the design), then create each step as its own " "task with create_task(milestone_id=)." ) + await refuse_guessed_ids(title, body) if not force: dup = await dedup_svc.find_duplicate_note( uid, title, body, project_id=project_id or None, @@ -279,6 +289,7 @@ async def update_task( fields["arose_from_id"] = arose_from_id if kind: fields["task_kind"] = minted_kind(kind) + await refuse_guessed_ids(title, body) note = await notes_svc.update_note(uid, task_id, **fields) if note is None: raise ValueError(f"task {task_id} not found") @@ -324,7 +335,117 @@ async def add_task_log(task_id: int, content: str) -> dict: return data -async def start_planning(project_id: int, title: str) -> dict: +_ITEM_KEYS = {"title", "body", "type", "status", "priority", "kind", "tags", "system_ids"} + + +def _batch_items(records: list[dict], *, what: str = "record") -> list[batch_svc.BatchItem]: + """Parse the door's plain dicts into BatchItems, refusing unknown keys. + + Strict for the same reason StrictArgsFastMCP is (#2709): a misspelt key + silently dropped — `milestone` for a per-record milestone, `desc` for body — + creates a record that looks right and is missing what the caller sent. + """ + items: list[batch_svc.BatchItem] = [] + for i, raw in enumerate(records or [], start=1): + if not isinstance(raw, dict): + raise ValueError(f"{what} {i} must be an object with at least a title") + unknown = set(raw) - _ITEM_KEYS + if unknown: + raise ValueError( + f"{what} {i} has unknown field(s) {sorted(unknown)}; " + f"allowed: {sorted(_ITEM_KEYS)}" + ) + rtype = raw.get("type") or "task" + if rtype not in ("task", "note"): + raise ValueError(f"{what} {i}: type must be 'task' or 'note', got {rtype!r}") + items.append(batch_svc.BatchItem( + title=raw.get("title") or "", + body=raw.get("body") or "", + is_task=rtype == "task", + status=raw.get("status") or "todo", + priority=raw.get("priority") or None, + task_kind=raw.get("kind") or "work", + tags=list(raw.get("tags") or []), + system_ids=list(raw.get("system_ids") or []), + )) + return items + + +async def _first_duplicate(uid: int, items: list, project_id: int | None) -> dict | None: + """The duplicate gate over a whole batch — the first hit blocks all of it.""" + for i, item in enumerate(items, start=1): + dup = await dedup_svc.find_duplicate_note( + uid, item.title, item.body, project_id=project_id, + is_task=item.is_task, note_type="note", + ) + if dup is not None: + payload = dedup_svc.duplicate_response(dup, "task" if item.is_task else "note") + payload["record"] = i + payload["message"] = f"Record {i} of the batch: {payload['message']} Nothing in the batch was created." + return payload + return None + + +async def create_records( + records: list[dict], + project_id: int = 0, + milestone_id: int = 0, + force: bool = False, +) -> dict: + """Create several tasks and/or notes in ONE call, so they can cite each other. + + Reach for this whenever records you are about to create need to reference + one another — a set of tasks that name their siblings, a reference note + listing the tasks it indexes. Writing the ids you EXPECT them to get is + always wrong: every session and user draws from one sequence, so another + create takes those numbers, and a body citing an unassigned `#N` is refused. + A plan (a milestone plus its steps) is start_planning(steps=...), which + uses the same mechanism. One record with nothing to cite is create_task or + create_note. + + In any record's body write {{ref:N}} where the Nth record's id belongs + (1-based, in the order listed). It is replaced with `# ""` + once the ids exist. Everything is created in one transaction: a bad + placeholder, an invalid field or a duplicate creates NOTHING. + + Args: + records: The records, in order. Each is an object with `title` + (required) and optionally `body`, `type` ('task', the default, or + 'note'), and for tasks `status`, `priority` and `kind` + ('work' | 'issue' | 'spike'); plus `tags` and `system_ids`. + project_id: The project every record belongs to (0 = none, or taken + from milestone_id). + milestone_id: File every record under this existing milestone (0 = none). + force: Bypass the near-duplicate gate for the whole batch. By default + the first record that near-duplicates an existing one BLOCKS the + batch, and its existing id comes back so you can update it instead. + + Returns {"ids": [...], "records": [...]} in the order given — the ids + cite-able from here on — OR a duplicate payload naming which `record` + matched, with nothing created. The ids are the real ones and are not + necessarily consecutive: other sessions keep creating meanwhile, and + nothing here depends on the numbers being adjacent. + """ + uid = current_user_id() + items = _batch_items(records) + await refuse_guessed_ids(*[t for item in items for t in (item.title, item.body)]) + if not force: + dup = await _first_duplicate(uid, items, project_id or None) + if dup is not None: + return dup + _ms, notes = await batch_svc.create_batch( + uid, items, project_id=project_id or None, milestone_id=milestone_id or None, + ) + return {"ids": [n.id for n in notes], "records": [n.to_dict() for n in notes]} + + +async def start_planning( + project_id: int, + title: str, + body: str = "", + steps: list[dict] | None = None, + force: bool = False, +) -> dict: """Begin a plan in Scribe (the preferred home for plans — not a local .md file). Reach for this when the work has an ARC — several steps toward one goal, @@ -341,11 +462,17 @@ async def start_planning(project_id: int, title: str) -> dict: context. The milestone is the plan container — the individual steps live as first-class child tasks under it, not as checkboxes in the body. - Afterwards: - - Edit the plan/design with update_milestone(milestone_id, body=...). - - Create each step as its own task with create_task(milestone_id=<this id>); - track it with status + add_task_log. Do NOT put steps as checkboxes in the - milestone body. + PASS THE STEPS HERE when you already know them. The milestone and every + step are then created in one transaction, and the plan body and the steps + can cite each other with placeholders that become real ids: {{ref:N}} is + the Nth step (1-based) and {{ref:milestone}} is this milestone. Never write + the ids you expect records to get — other sessions and users draw from the + same sequence, and a body citing an unassigned `#N` is refused. + + Without steps: edit the design afterwards with update_milestone( + milestone_id, body=...), and add steps with create_records(milestone_id= + <this id>, ...) — or create_task for a single one. Track each with status + + add_task_log. Do NOT put steps as checkboxes in the milestone body. (kind=plan tasks are retired — use this instead. Existing historical plan-tasks remain readable but new planning goes through milestones.) @@ -353,10 +480,29 @@ async def start_planning(project_id: int, title: str) -> dict: Args: project_id: The project this plan is for. title: A short title for the plan/milestone. + body: The plan's design (markdown). Omit to seed the Goal/Approach/ + Verification template. + steps: The plan's step-tasks, in order — each an object with `title` + (required) and optionally `body`, `status`, `priority`, `kind` + ('work' | 'issue' | 'spike'), `tags`, `system_ids`. + force: Bypass the near-duplicate gate on the steps. By default a step + that near-duplicates an existing task blocks the whole plan, and + nothing — milestone included — is created. + + Returns the milestone, the project's applicable rules and brief context, + plus `steps` (the created tasks, in order) when steps were given — OR a + duplicate payload naming the `record` that matched, with nothing created. """ uid = current_user_id() + items = _batch_items(steps or [], what="step") + await refuse_guessed_ids(body, *[t for item in items for t in (item.title, item.body)]) + if items and not force: + dup = await _first_duplicate(uid, items, project_id or None) + if dup is not None: + return dup return await planning_svc.start_planning( user_id=uid, project_id=project_id, title=title, + body=body or None, steps=items or None, ) @@ -388,6 +534,7 @@ def register(mcp) -> None: list_tasks, get_task, create_task, + create_records, update_task, add_task_log, start_planning, diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 4deee9c..e1bc878 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -155,6 +155,77 @@ async def create_note( verify_with: str | None = None, expires_when: str | None = None, ) -> Note: + # Auto-populate project_id from milestone when not explicitly provided + if milestone_id is not None and project_id is None: + from scribe.models.milestone import Milestone + async with async_session() as lookup: + result = await lookup.execute( + select(Milestone).where(Milestone.id == milestone_id, Milestone.user_id == user_id) + ) + ms = result.scalars().first() + if ms is not None: + project_id = ms.project_id + + note = build_note( + user_id, + title=title, + body=body, + description=description, + tags=tags, + parent_id=parent_id, + project_id=project_id, + milestone_id=milestone_id, + status=status, + priority=priority, + due_date=due_date, + recurrence_rule=recurrence_rule, + note_type=note_type, + task_kind=task_kind, + arose_from_id=arose_from_id, + data=data, + verify_with=verify_with, + expires_when=expires_when, + ) + async with async_session() as session: + session.add(note) + await session.commit() + await session.refresh(note) + + embed_note(note) + + if project_id is not None: + await _maybe_reactivate_project(project_id) + + return note + + +def build_note( + user_id: int, + title: str = "", + body: str = "", + description: str | None = None, + tags: list[str] | None = None, + parent_id: int | None = None, + project_id: int | None = None, + milestone_id: int | None = None, + status: str | None = None, + priority: str | None = None, + due_date: date | None = None, + recurrence_rule: dict | None = None, + note_type: str = "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: + """A validated, UNSAVED Note — every check create_note makes, no write. + + Split out of create_note so a caller that must add several records in one + transaction (services/record_batch.py) validates them exactly as a single + create would, instead of carrying a second copy of the rules that drifts. + Raises before anything is built, so nothing illegal reaches a session. + """ # 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 @@ -176,48 +247,26 @@ async def create_note( except ValueError: raise ValueError(f"Invalid priority: {priority!r}. Must be one of: {[p.value for p in TaskPriority]}") - # Auto-populate project_id from milestone when not explicitly provided - if milestone_id is not None and project_id is None: - from scribe.models.milestone import Milestone - async with async_session() as lookup: - result = await lookup.execute( - select(Milestone).where(Milestone.id == milestone_id, Milestone.user_id == user_id) - ) - ms = result.scalars().first() - if ms is not None: - project_id = ms.project_id - - async with async_session() as session: - note = Note( - user_id=user_id, - title=title, - body=body, - description=description, - tags=_normalize_tags(tags or []), - parent_id=parent_id, - project_id=project_id, - milestone_id=milestone_id, - status=status, - priority=priority, - due_date=due_date, - recurrence_rule=recurrence_rule, - note_type=note_type, - 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() - await session.refresh(note) - - embed_note(note) - - if project_id is not None: - await _maybe_reactivate_project(project_id) - - return note + return Note( + user_id=user_id, + title=title, + body=body, + description=description, + tags=_normalize_tags(tags or []), + parent_id=parent_id, + project_id=project_id, + milestone_id=milestone_id, + status=status, + priority=priority, + due_date=due_date, + recurrence_rule=recurrence_rule, + note_type=note_type, + task_kind=task_kind, + arose_from_id=arose_from_id, + data=data, + verify_with=verify_with, + expires_when=expires_when, + ) async def get_note(user_id: int, note_id: int) -> Note | None: diff --git a/src/scribe/services/planning.py b/src/scribe/services/planning.py index d7addeb..229427e 100644 --- a/src/scribe/services/planning.py +++ b/src/scribe/services/planning.py @@ -6,13 +6,19 @@ The milestone IS the plan: its `body` holds the design/intent (Goal/Approach/ Verification), and the individual steps live as first-class child tasks (milestone_id) rather than checkboxes crammed into one body. The legacy kind=plan task is retired going forward — start_planning never creates one. + +Steps passed up front are created in the SAME transaction as the milestone +(services/record_batch.py), so the plan body and the steps can cite each other +through `{{ref:N}}` placeholders instead of predicted ids (#4016). """ from __future__ import annotations from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc from scribe.services import projects as projects_svc +from scribe.services import record_batch as batch_svc from scribe.services import rulebooks as rulebooks_svc +from scribe.services.record_refs import placeholder_keys # The plan body template — design only. Steps are NOT checkboxes here; each # step becomes its own child task under this milestone (status, work-logs, @@ -25,10 +31,19 @@ PLAN_TEMPLATE = """## Goal """ -async def start_planning(user_id: int, project_id: int, title: str) -> dict: +async def start_planning( + user_id: int, + project_id: int, + title: str, + body: str | None = None, + steps: list[batch_svc.BatchItem] | None = None, +) -> dict: """Create a milestone seeded as a plan container and return it with the project's applicable rules + brief context. + `body` replaces the seeded template; `steps` creates the step-tasks with + the milestone, atomically. + Returns: { "milestone": <milestone dict>, @@ -37,19 +52,35 @@ async def start_planning(user_id: int, project_id: int, title: str) -> dict: "applicable_rules_truncated": bool, "project_goal": str, "open_task_count": int, + "steps": [<task dict>, ...], # only when steps were given } """ project = await projects_svc.get_project(user_id, project_id) if project is None: raise ValueError(f"project {project_id} not found") - milestone = await milestones_svc.create_milestone( - user_id, - project_id=project_id, - title=title, - body=PLAN_TEMPLATE, - status="active", - ) + plan_body = body or PLAN_TEMPLATE + step_notes = [] + if steps: + milestone, step_notes = await batch_svc.create_batch( + user_id, steps, project_id=project_id, + milestone=batch_svc.BatchMilestone(title=title, body=plan_body), + ) + else: + # A placeholder with no batch behind it has nothing to resolve to, and + # storing `{{ref:1}}` verbatim would be a reference to nothing. + if placeholder_keys(plan_body): + raise ValueError( + "the plan body uses {{ref:...}} placeholders but no steps were " + "given — pass steps=[...] so they have records to name" + ) + milestone = await milestones_svc.create_milestone( + user_id, + project_id=project_id, + title=title, + body=plan_body, + status="active", + ) applicable = await rulebooks_svc.get_applicable_rules( project_id=project_id, user_id=user_id, @@ -58,9 +89,12 @@ async def start_planning(user_id: int, project_id: int, title: str) -> dict: user_id, is_task=True, status="todo", project_id=project_id, limit=1, ) - return { + result = { "milestone": milestone.to_dict(), **rulebooks_svc.rules_payload(applicable, user_id=user_id, source="start_planning"), "project_goal": getattr(project, "goal", "") or "", "open_task_count": open_count, } + if steps: + result["steps"] = [n.to_dict() for n in step_notes] + return result diff --git a/src/scribe/services/record_batch.py b/src/scribe/services/record_batch.py new file mode 100644 index 0000000..1600836 --- /dev/null +++ b/src/scribe/services/record_batch.py @@ -0,0 +1,193 @@ +"""Create several records at once, so they can cite each other without guessing ids. + +Why this exists is in services/record_refs.py (#4016): a session that needs +records to reference one another used to create them one by one and predict +the ids of the ones not yet made — and any concurrent create, from any session +or any user, took those numbers. + +Here the whole batch is ONE transaction: insert every record, flush so the +sequence assigns the real ids, rewrite each `{{ref:N}}` with the id and title +it names, commit. Other sessions keep creating throughout and cannot interfere +— the sequence never hands out the same number twice, so another session's +insert just takes a different one. The ids a batch receives are therefore NOT +guaranteed consecutive, and nothing here needs them to be: the placeholders +are filled with whatever ids came back. Forcing consecutive ids would take a +table lock that stalls every user's writes; nobody needs that. + +All or nothing: a bad placeholder, an invalid status, or a failure mid-insert +leaves no partial batch behind — which is also why there are no "reserved" +stub records to clean up after a session that dies. +""" +from __future__ import annotations + +from dataclasses import dataclass, field + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.milestone import Milestone +from scribe.models.note import Note +from scribe.services import access as access_svc +from scribe.services import notes as notes_svc +from scribe.services import systems as systems_svc +from scribe.services.record_refs import placeholder_keys, resolve_placeholders + +# A batch is a plan's steps or a handful of linked records, and every item runs +# the near-duplicate gate (an embedding search) before the transaction. 50 is +# far past any real plan and keeps one call from becoming a bulk import, which +# is a different job with a different door. +MAX_BATCH = 50 + + +@dataclass +class BatchItem: + """One record in a batch. `is_task` False makes it a plain note.""" + + title: str + body: str = "" + is_task: bool = True + status: str = "todo" + priority: str | None = None + task_kind: str = "work" + tags: list[str] = field(default_factory=list) + system_ids: list[int] = field(default_factory=list) + + +@dataclass +class BatchMilestone: + """The milestone start_planning creates in the same transaction as its steps.""" + + title: str + body: str + description: str | None = None + + +def check_batch(items: list[BatchItem], milestone: BatchMilestone | None = None) -> None: + """Refuse a batch that cannot be written in full, before any write. + + Every placeholder must name something the batch will actually create; + otherwise the rewrite has nothing to put there, and the alternatives — + leaving `{{ref:9}}` in a stored body, or dropping it — both store a + reference that points nowhere. + """ + if not items and milestone is None: + raise ValueError("a batch needs at least one record") + if len(items) > MAX_BATCH: + raise ValueError(f"a batch holds at most {MAX_BATCH} records; got {len(items)}") + for i, item in enumerate(items, start=1): + if not (item.title or "").strip(): + raise ValueError(f"record {i} has no title") + valid = {str(i) for i in range(1, len(items) + 1)} + if milestone is not None: + valid.add("milestone") + texts = [item.body for item in items] + ([milestone.body] if milestone else []) + unknown = placeholder_keys(*texts) - valid + if unknown: + named = ", ".join("{{ref:%s}}" % k for k in sorted(unknown)) + raise ValueError( + f"{named} names no record in this batch. {{{{ref:N}}}} is the Nth " + f"record listed (1 to {len(items)})" + + (", and {{ref:milestone}} is the milestone being created." if milestone else ".") + ) + + +async def create_batch( + user_id: int, + items: list[BatchItem], + *, + project_id: int | None = None, + milestone_id: int | None = None, + milestone: BatchMilestone | None = None, +) -> tuple[Milestone | None, list[Note]]: + """Create `items` (and optionally a new milestone they belong to) atomically. + + `milestone_id` files the items under an EXISTING milestone the caller owns; + `milestone` creates a new one alongside them and files them there. Passing + both is refused. Returns (the new milestone or None, the notes in input + order). Near-duplicate gating is the door's job, exactly as for a single + create. + """ + if milestone is not None and milestone_id: + raise ValueError("pass milestone_id (an existing milestone) or a new milestone, not both") + check_batch(items, milestone) + + if milestone_id: + async with async_session() as session: + existing = (await session.execute( + select(Milestone).where( + Milestone.id == milestone_id, Milestone.deleted_at.is_(None), + ) + )).scalars().first() + if existing is None: + raise ValueError(f"milestone {milestone_id} not found") + if project_id and project_id != existing.project_id: + raise ValueError( + f"milestone {milestone_id} belongs to project {existing.project_id}, not {project_id}" + ) + project_id = existing.project_id + if milestone is not None and not project_id: + raise ValueError("a new milestone needs a project_id") + # Share-aware (rule 78): a collaborator with write access to a shared + # project may plan in it; a bare owner filter would refuse them. + if project_id and not await access_svc.can_write_project(user_id, project_id): + raise ValueError(f"project {project_id} not found") + + async with async_session() as session: + # Validate every record before adding any: build_note raises on a bad + # status/priority, and raising here writes nothing. + notes = [ + notes_svc.build_note( + user_id, + title=item.title, + body=item.body, + tags=item.tags, + project_id=project_id, + milestone_id=milestone_id, + status=item.status if item.is_task else None, + priority=item.priority if item.is_task else None, + task_kind=notes_svc.minted_kind(item.task_kind) if item.is_task else "work", + ) + for item in items + ] + + new_ms = None + if milestone is not None: + new_ms = Milestone( + user_id=user_id, project_id=project_id, title=milestone.title, + description=milestone.description, body=milestone.body, status="active", + ) + session.add(new_ms) + await session.flush() + for note in notes: + note.milestone_id = new_ms.id + + session.add_all(notes) + # The flush is where the sequence assigns ids. Nothing is visible to + # any other session until the commit below, and if anything between + # here and there raises, the context manager rolls it all back. + await session.flush() + + refs = {str(i): f'#{n.id} "{n.title}"' for i, n in enumerate(notes, start=1)} + if new_ms is not None: + refs["milestone"] = f'milestone {new_ms.id} "{new_ms.title}"' + new_ms.body = resolve_placeholders(new_ms.body, refs) + for note in notes: + note.body = resolve_placeholders(note.body, refs) + + await session.commit() + for note in notes: + await session.refresh(note) + if new_ms is not None: + await session.refresh(new_ms) + + # After the commit, as a single create does: embedding and System tags are + # enrichment on records that now exist, and a failure in either must not + # un-create them. + for note, item in zip(notes, items): + notes_svc.embed_note(note) + if item.system_ids: + await systems_svc.set_record_systems(user_id, note.id, item.system_ids) + if project_id is not None: + await notes_svc._maybe_reactivate_project(project_id) + + return new_ms, notes diff --git a/src/scribe/services/record_refs.py b/src/scribe/services/record_refs.py new file mode 100644 index 0000000..fed360e --- /dev/null +++ b/src/scribe/services/record_refs.py @@ -0,0 +1,131 @@ +"""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) diff --git a/tests/test_create_tools_disambiguate.py b/tests/test_create_tools_disambiguate.py index 0089707..cf7ffee 100644 --- a/tests/test_create_tools_disambiguate.py +++ b/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"), diff --git a/tests/test_integration_record_batch.py b/tests/test_integration_record_batch.py new file mode 100644 index 0000000..788a7d2 --- /dev/null +++ b/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}") diff --git a/tests/test_mcp_tool_create_records.py b/tests/test_mcp_tool_create_records.py new file mode 100644 index 0000000..c7a9919 --- /dev/null +++ b/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() diff --git a/tests/test_mcp_tool_planning.py b/tests/test_mcp_tool_planning.py index 26d4897..951bdfc 100644 --- a/tests/test_mcp_tool_planning.py +++ b/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 diff --git a/tests/test_services_record_refs.py b/tests/test_services_record_refs.py new file mode 100644 index 0000000..92ae27a --- /dev/null +++ b/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"})