From fd72b485d022975f7be98de9431b227d32bd95b7 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Sat, 5 Sep 2026 01:01:46 +0000 Subject: [PATCH] fix(projects): refuse checklist create/archive when the parent task is gone (tsk-s5pif2) task_checklist_items.task_id declares REFERENCES project_tasks(id), but ProjectTaskStore never issues PRAGMA foreign_keys = ON, so SQLite does not enforce it and a checklist item can outlive its task. Both create and archive then fell back to publishing their broker event under a topic that is not a project_id, and since project subscribers subscribe at project scope only, the event went to a channel nobody listens to and was silently lost. Resolve the parent task before either path mutates and raise ValueError("task not found: ") when it is missing, so a refusal leaves neither an orphan row nor a half-applied archive, and the publish topic is always the task's real project_id. --- ...sk-s5pif2-checklist-orphan-task-publish.md | 9 +++ docs/agent-coordination.md | 9 +++ tests/projects/test_task_store.py | 74 +++++++++++++++++++ tinyagentos/projects/task_store.py | 26 +++++-- 4 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 changelog.d/tsk-s5pif2-checklist-orphan-task-publish.md diff --git a/changelog.d/tsk-s5pif2-checklist-orphan-task-publish.md b/changelog.d/tsk-s5pif2-checklist-orphan-task-publish.md new file mode 100644 index 000000000..9a0e0c45c --- /dev/null +++ b/changelog.d/tsk-s5pif2-checklist-orphan-task-publish.md @@ -0,0 +1,9 @@ +### Fixed +- Checklist item create and archive now refuse a task that no longer exists + instead of publishing their broker event under a topic no project subscriber + listens to. `task_checklist_items.task_id` declares a foreign key but the + task store never enables `PRAGMA foreign_keys = ON`, so a checklist item can + outlive its task; the previous fallback resolved the publish topic to an + empty string and the `checklist.item.created` / `checklist.item.archived` + event was silently lost. Both paths now resolve the parent task before they + mutate, so a refusal leaves no orphan row and no half-applied archive. diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index c8915545c..c73de80e2 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -1174,6 +1174,15 @@ Route module `tinyagentos/routes/projects.py`. another project is existence-hiding rather than merely forbidden. - Creating an item logs `checklist.item.created` to the project activity feed with the actor, task id, item id and text. +- Both `checklist.item.created` and `checklist.item.archived` are published on + the broker under the task's **`project_id`**, because project subscribers + subscribe at project scope. The store therefore resolves the parent task + first and raises `ValueError: task not found: ` when it is gone — + create refuses before inserting the row, archive refuses before flipping + `archived`. `task_checklist_items.task_id` declares a foreign key but the + store never sets `PRAGMA foreign_keys = ON`, so an item can outlive its task; + without the guard the event went to a topic nobody listens to and was + silently lost. - Archiving is store-level only and refuses unless the item is both **verified** and **reported**; there is no archive route. - `DELETE` and per-item subpaths (`.../checklist-items/{item_id}`) stay diff --git a/tests/projects/test_task_store.py b/tests/projects/test_task_store.py index 5dad68960..0a5b681be 100644 --- a/tests/projects/test_task_store.py +++ b/tests/projects/test_task_store.py @@ -464,6 +464,80 @@ async def test_checklist_item_event_delivered_at_project_scope(store_with_broker assert checklist_events[0].payload["task_id"] == t["id"] +async def _delete_task_row(store, task_id: str) -> None: + """Drop a task row out from under its checklist items. + + ``task_checklist_items.task_id`` declares ``REFERENCES project_tasks(id)`` + but ProjectTaskStore never issues ``PRAGMA foreign_keys = ON``, so SQLite + does not enforce it and a live checklist item can outlive its task. + """ + await store._db.execute("DELETE FROM project_tasks WHERE id = ?", (task_id,)) + await store._db.commit() + + +@pytest.mark.asyncio +async def test_archive_checklist_item_refuses_when_task_is_gone(store_with_broker): + """Archiving an orphaned item must raise, not publish off-topic. + + The fallback resolved the publish topic to something that is not a + project_id, so ``checklist.item.archived`` landed on a channel no + project subscriber listens to and the mutation was silently lost. + """ + store, broker = store_with_broker + t = await store.create_task(project_id="proj-orphan", title="Objective", created_by="u") + item = await store.create_checklist_item(task_id=t["id"], text="step one", created_by="u") + await store.update_checklist_item(item_id=item["id"], verified=True, reported=True) + await _delete_task_row(store, t["id"]) + + # Captured rather than asserted with pytest.raises so the topic assertion + # below is the one that reports the defect, not an unreached line after it. + raised: ValueError | None = None + try: + await store.archive_checklist_item(item_id=item["id"]) + except ValueError as exc: + raised = exc + + # Nothing may reach a non-project channel: "" is the current fallback + # topic, the task_id is the pre-#2622 one. Both are dead letter boxes. + for dead_topic in ("", t["id"]): + queue = await broker.subscribe(dead_topic) + stray = [] + while not queue.empty(): + stray.append(queue.get_nowait().kind) + assert not stray, f"event published to dead topic {dead_topic!r}: {stray}" + + assert raised is not None and "task not found" in str(raised), ( + f"archive must refuse an item whose task is gone, raised: {raised!r}" + ) + # The refused archive must not have mutated the row either. + again = await store.get_checklist_item(item["id"]) + assert again["archived"] is False + + +@pytest.mark.asyncio +async def test_create_checklist_item_refuses_when_task_is_missing(store_with_broker): + """Same defect on the sibling create path — it must refuse, not orphan a row.""" + store, broker = store_with_broker + + raised: ValueError | None = None + try: + await store.create_checklist_item(task_id="tsk-ghost", text="step one", created_by="u") + except ValueError as exc: + raised = exc + + for dead_topic in ("", "tsk-ghost"): + queue = await broker.subscribe(dead_topic) + stray = [] + while not queue.empty(): + stray.append(queue.get_nowait().kind) + assert not stray, f"event published to dead topic {dead_topic!r}: {stray}" + + assert raised is not None and "task not found" in str(raised), ( + f"create must refuse a missing task, raised: {raised!r}" + ) + assert await store.list_checklist_items(task_id="tsk-ghost", include_archived=True) == [] + + @pytest.mark.asyncio async def test_checklist_item_created_by_persists(store): """Acceptance 1: Round-trip test: create_checklist_item(created_by="u") -> list_checklist_items returns created_by == "u". RED on origin/dev (column absent), green on the fix.""" diff --git a/tinyagentos/projects/task_store.py b/tinyagentos/projects/task_store.py index d8961a0d9..f28969f17 100644 --- a/tinyagentos/projects/task_store.py +++ b/tinyagentos/projects/task_store.py @@ -917,6 +917,16 @@ async def create_checklist_item( text: str, created_by: str, ) -> dict: + """Create a checklist item on a task. + + Raises ValueError if the task does not exist: the item's only route to + a project subscriber is the task's ``project_id``, so a missing task + leaves nothing to publish under. Resolved before the INSERT so a refusal + never leaves an orphan row behind. + """ + task = await self.get_task(task_id) + if task is None: + raise ValueError(f"task not found: {task_id}") cid = new_id("cki") now = time.time() await self._db.execute( @@ -932,9 +942,7 @@ async def create_checklist_item( row = await cur.fetchone() desc = cur.description item = _row_to_checklist_item(row, desc) - task = await self.get_task(task_id) - project_id = task["project_id"] if task is not None else "" - await self._publish(project_id, "checklist.item.created", {"id": item["id"], "text": item["text"], "task_id": task_id}) + await self._publish(task["project_id"], "checklist.item.created", {"id": item["id"], "text": item["text"], "task_id": task_id}) return item async def list_checklist_items( @@ -989,7 +997,10 @@ async def archive_checklist_item(self, item_id: str) -> dict: """Archive a checklist item. Only valid if verified=1 and reported=1. Raises ValueError if the item cannot be archived because it lacks - verification or a report. + verification or a report, or because its task is gone — the task + carries the ``project_id`` that ``checklist.item.archived`` is + published under, and project subscribers listen at project scope only. + Resolved before the UPDATE so a refusal leaves the item untouched. """ item = await self.get_checklist_item(item_id) if item is None: @@ -998,15 +1009,16 @@ async def archive_checklist_item(self, item_id: str) -> dict: raise ValueError("item cannot be archived: not verified") if item["reported"] != 1: raise ValueError("item cannot be archived: not reported") + task = await self.get_task(item["task_id"]) + if task is None: + raise ValueError(f"task not found: {item['task_id']}") now = time.time() await self._db.execute( "UPDATE task_checklist_items SET archived = 1, updated_at = ? WHERE id = ?", (now, item_id), ) await self._db.commit() - task = await self.get_task(item["task_id"]) - project_id = task["project_id"] if task is not None else "" - await self._publish(project_id, "checklist.item.archived", {"id": item_id, "task_id": item["task_id"], "archived": True}) + await self._publish(task["project_id"], "checklist.item.archived", {"id": item_id, "task_id": item["task_id"], "archived": True}) return await self.get_checklist_item(item_id) async def get_checklist_item(self, item_id: str) -> dict | None: