fix(tasks): reconcile acceptance_criteria_ids at the update chokepoint (#682)

Every post-create rewrite of acceptance_criteria (task PATCH route,
prompter update_live_draft / _patch_batch_child / update_live_batch)
routes through TaskService.update()'s generic field loop, which
overwrote the criteria without touching acceptance_criteria_ids —
leaving ids mismatched or empty, and an empty id list silently
disabled the parent-coverage gate entirely.

- New pure _reconcile_ac_ids: one id per new criterion; text-unchanged
  criteria keep their id (children and findings reference criteria by
  id or exact text — a blanket re-mint would orphan every live
  reference), new/reworded text mints fresh, dropped criteria drop
  theirs. create() now stamps through the same helper (explicitly
  supplied ids still win).
- update() derives acceptance_criteria_ids whenever acceptance_criteria
  is rewritten without an explicit id list.
- The parent-coverage gate self-heals a criteria-bearing row whose ids
  are empty/out-of-length (re-stamp in place) instead of returning
  early and silently waiving coverage for the whole subtree.

Co-authored-by: Renn F <rennf93@users.noreply.github.com>

roboco/services/task.py

@@ -740,6 +740,35 @@ def supersede_marker_line(task: Any) -> str:
     return markers.get_external_pr_supersede(task) or ""
 
 
+def _reconcile_ac_ids(
+    *,
+    old_criteria: list[str],
+    old_ids: list[str],
+    new_criteria: list[str],
+) -> list[str]:
+    """Stable per-criterion ids (migration 036) across an AC rewrite.
+
+    One id per element of ``new_criteria``. A criterion whose TEXT is
+    unchanged keeps its existing id — ``covers_parent_criteria``, the
+    findings ledger's ``criterion`` matcher, and ``parent_ac_refs`` all
+    reference a criterion by stable id or exact text, so a blanket re-mint on
+    every edit would silently orphan every existing reference and open
+    finding. A new or reworded criterion mints a fresh id; a dropped one
+    drops its id. Same routine serves a genuine caller-driven rewrite (``old``
+    is the row's pre-update state) and a legacy self-heal (``old`` and `new`
+    are the same current row, reconciling ids that drifted out of sync before
+    every write routed through this helper). Duplicate text is matched in
+    encounter order (first ``old`` occurrence to first ``new`` occurrence).
+    """
+    pool: dict[str, list[str]] = {}
+    for ac_text, cid in zip(old_criteria, old_ids, strict=False):
+        pool.setdefault(ac_text, []).append(cid)
+    return [
+        pool[ac_text].pop(0) if pool.get(ac_text) else uuid4().hex
+        for ac_text in new_criteria
+    ]
+
+
 class TaskService(BaseService):
     """
     Service for managing tasks.
@@ -1264,9 +1293,9 @@ class TaskService(BaseService):
 
         # Stable per-criterion ids (1:1 with acceptance_criteria) so children can
         # reference specific parent criteria; generated here when not supplied.
-        ac_ids = req.acceptance_criteria_ids or [
-            uuid4().hex for _ in (req.acceptance_criteria or [])
-        ]
+        ac_ids = req.acceptance_criteria_ids or _reconcile_ac_ids(
+            old_criteria=[], old_ids=[], new_criteria=req.acceptance_criteria or []
+        )
         task = TaskTable(
             title=req.title,
             description=req.description,
@@ -2901,11 +2930,26 @@ class TaskService(BaseService):
         task_id: UUID,
         **updates: Any,
     ) -> TaskTable | None:
-        """Update a task."""
+        """Update a task.
+
+        A rewritten ``acceptance_criteria`` re-stamps ``acceptance_criteria_ids``
+        1:1 unless the caller already supplied its own ids explicitly — every
+        writer of this generic setter (the PATCH route, the board-redraft
+        patches) otherwise left the old ids in place, mismatched or empty
+        against the new criteria list (see ``_reconcile_ac_ids``).
+        """
         task = await self.get(task_id)
         if not task:
             return None
 
+        new_criteria = updates.get("acceptance_criteria")
+        if new_criteria is not None and "acceptance_criteria_ids" not in updates:
+            updates["acceptance_criteria_ids"] = _reconcile_ac_ids(
+                old_criteria=list(task.acceptance_criteria or []),
+                old_ids=list(task.acceptance_criteria_ids or []),
+                new_criteria=new_criteria,
+            )
+
         for key, value in updates.items():
             if hasattr(task, key) and value is not None:
                 setattr(task, key, value)
@@ -9650,19 +9694,37 @@ class TaskService(BaseService):
         statuses = result.scalars().all()
         return all(s in terminal for s in statuses)
 
+    async def _self_heal_ac_ids(self, parent: TaskTable) -> None:
+        """Re-stamp ``acceptance_criteria_ids`` in place when it's empty or out
+        of length with ``acceptance_criteria`` -- a legacy row from before every
+        AC rewrite reconciled ids (``TaskService.update``), or any other drift.
+        No-op when already 1:1. Reconciling against the row's own current
+        criteria means any id a child already references by matching TEXT
+        survives; the parent-coverage gate is live again instead of skipped
+        forever (``_parent_ac_ref_sets``).
+        """
+        if len(parent.acceptance_criteria_ids or []) == len(parent.acceptance_criteria):
+            return
+        parent.acceptance_criteria_ids = _reconcile_ac_ids(
+            old_criteria=parent.acceptance_criteria,
+            old_ids=list(parent.acceptance_criteria_ids or []),
+            new_criteria=parent.acceptance_criteria,
+        )
+        await self.session.flush()
+
     async def _parent_ac_ref_sets(
         self, task_id: UUID
     ) -> tuple[TaskTable, set[str], set[str], bool, set[str]] | None:
         """Load a parent and its children's parent-AC-ref coverage sets.
 
         Shared core of the three AC-coverage primitives. Returns ``None`` when
-        the parent is missing or has no stable criterion ids (nothing to cover).
-        Otherwise ``(parent, claimed, verified, any_declared, root_owned)``
-        where ``claimed`` is the union of parent_ac_refs over all non-cancelled
-        children, ``verified`` the union over COMPLETED children only, and
-        ``any_declared`` whether *any* child declared a ref at all (the
-        safe-by-construction inertness signal — a cancelled-only declaration
-        still counts as "coverage tracking is active here").
+        the parent is missing or has no acceptance criteria at all (nothing to
+        cover). Otherwise ``(parent, claimed, verified, any_declared,
+        root_owned)`` where ``claimed`` is the union of parent_ac_refs over all
+        non-cancelled children, ``verified`` the union over COMPLETED children
+        only, and ``any_declared`` whether *any* child declared a ref at all
+        (the safe-by-construction inertness signal — a cancelled-only
+        declaration still counts as "coverage tracking is active here").
 
         ``root_owned`` is the parent's OWN ``parent_ac_refs`` (declared on
         itself via ``declare_coverage(task_id=<own root>, ...)``) — criteria
@@ -9671,10 +9733,16 @@ class TaskService(BaseService):
         folded into both ``claimed`` and ``verified``: there is no child
         status to gate on, the work happens at/after the root's own
         submit/supersede by construction.
+
+        A parent whose ``acceptance_criteria_ids`` is empty or out of length
+        with ``acceptance_criteria`` (a legacy row from before every AC
+        rewrite reconciled ids — or any other drift) self-heals via
+        ``_self_heal_ac_ids`` rather than silently disabling coverage.
         """
         parent = await self.get(task_id)
-        if not parent or not parent.acceptance_criteria_ids:
+        if not parent or not parent.acceptance_criteria:
             return None
+        await self._self_heal_ac_ids(parent)
         result = await self.session.execute(
             select(TaskTable.status, TaskTable.parent_ac_refs).where(
                 TaskTable.parent_task_id == task_id

tests/integration/test_prompter_redraft.py

@@ -207,6 +207,22 @@ async def test_update_live_draft_main_pm_updates_and_hands_off(
     assert task.team == Team.MAIN_PM
 
 
+@pytest.mark.asyncio
+async def test_update_live_draft_reconciles_acceptance_criteria_ids(
+    redraft_setup: dict,
+) -> None:
+    # The wiping path: update_live_draft patches acceptance_criteria via the
+    # generic TaskService.update(), which used to leave acceptance_criteria_ids
+    # stale/mismatched against the revised criteria (the fe-pm delegate-loop
+    # incident). Must come out 1:1 with the new criteria after a redraft.
+    _N = 2
+    task = redraft_setup["mk"](True)
+    await redraft_setup["db"].flush()
+    await redraft_setup["svc"].update_live_draft(task.id, _DRAFT, route="main_pm")
+    assert len(task.acceptance_criteria_ids) == len(task.acceptance_criteria) == _N
+    assert len(set(task.acceptance_criteria_ids)) == _N
+
+
 @pytest.mark.asyncio
 async def test_update_live_draft_reboard_resets_flag(redraft_setup: dict) -> None:
     task = redraft_setup["mk"](True)

tests/integration/test_task_service_basics.py

@@ -514,6 +514,40 @@ async def test_update_returns_none_for_missing(task_setup: dict) -> None:
     assert await svc.update(uuid4(), title="x") is None
 
 
+@pytest.mark.asyncio
+async def test_update_acceptance_criteria_restamps_ids_1to1(
+    task_setup: dict,
+) -> None:
+    # fe-pm delegate-loop root cause: TaskService.update() rewrote
+    # acceptance_criteria without touching acceptance_criteria_ids, leaving a
+    # stale/mismatched id list. A criterion whose TEXT is unchanged must keep
+    # its id (children/findings reference it by id or exact text); a new one
+    # mints a fresh id; a dropped one drops its id.
+    _N = 3
+    svc = task_setup["svc"]
+    task = await svc.create(_req(task_setup, acceptance_criteria=["a", "b", "c"]))
+    old_ids = list(task.acceptance_criteria_ids)
+    updated = await svc.update(task.id, acceptance_criteria=["a", "c", "d"])
+    assert updated is not None
+    assert len(updated.acceptance_criteria_ids) == _N
+    assert updated.acceptance_criteria_ids[0] == old_ids[0]  # "a" unchanged
+    assert updated.acceptance_criteria_ids[1] == old_ids[2]  # "c" unchanged
+    assert updated.acceptance_criteria_ids[2] not in old_ids  # "d" is new
+    assert len(set(updated.acceptance_criteria_ids)) == _N
+
+
+@pytest.mark.asyncio
+async def test_update_without_acceptance_criteria_leaves_ids_untouched(
+    task_setup: dict,
+) -> None:
+    svc = task_setup["svc"]
+    task = await svc.create(_req(task_setup, acceptance_criteria=["a", "b"]))
+    old_ids = list(task.acceptance_criteria_ids)
+    updated = await svc.update(task.id, title="renamed")
+    assert updated is not None
+    assert list(updated.acceptance_criteria_ids) == old_ids
+
+
 @pytest.mark.asyncio
 async def test_add_progress(task_setup: dict) -> None:
     svc = task_setup["svc"]

tests/integration/test_task_service_transitions.py

@@ -1694,6 +1694,46 @@ async def test_production_replay_root_with_child_and_root_owned_coverage(
     assert await svc.uncovered_parent_acceptance_criteria(parent.id) == []
 
 
+@pytest.mark.asyncio
+async def test_parent_ac_edit_preserves_ids_children_still_resolve(
+    task_setup: dict, db_session: AsyncSession
+) -> None:
+    """A board-redraft-style parent AC edit that drops one criterion and adds
+    another must keep the id of every criterion whose TEXT is unchanged, so a
+    child's existing parent_ac_refs -- by id OR by exact text -- still
+    resolves after the rewrite instead of orphaning."""
+    svc = task_setup["svc"]
+    parent = await svc.create(
+        _req(task_setup, acceptance_criteria=["crit a", "crit b", "crit x"])
+    )
+    id_a, _id_b, id_x = parent.acceptance_criteria_ids
+    child_by_id = await svc.create(
+        _req(task_setup, parent_task_id=parent.id, parent_ac_refs=[id_a])
+    )
+    child_by_id.status = TaskStatus.COMPLETED
+    child_by_text = await svc.create(
+        _req(task_setup, parent_task_id=parent.id, parent_ac_refs=["crit x"])
+    )
+    child_by_text.status = TaskStatus.COMPLETED
+    await db_session.flush()
+
+    # "crit b" dropped, "crit new" added, "crit a"/"crit x" kept (reordered).
+    await svc.update(
+        parent.id,
+        acceptance_criteria=["crit x", "crit a", "crit new"],
+    )
+    await db_session.flush()
+
+    refreshed = await svc.get(parent.id)
+    assert refreshed is not None
+    assert set(refreshed.acceptance_criteria_ids[:2]) == {id_a, id_x}
+    assert refreshed.acceptance_criteria_ids[2] not in {id_a, id_x}
+
+    # Both the id-based and text-based refs still resolve -- only the
+    # brand-new criterion is uncovered.
+    assert await svc.uncovered_parent_acceptance_criteria(parent.id) == ["crit new"]
+
+
 # ---------------------------------------------------------------------------
 # _unblock_dependents
 # ---------------------------------------------------------------------------

tests/unit/services/test_task.py

@@ -35,6 +35,7 @@ from roboco.services.task import (
     GatewayAgentView,
     TaskService,
     _ceo_reject_finding_texts,
+    _reconcile_ac_ids,
     get_task_service,
 )
 from sqlalchemy import select
@@ -1214,12 +1215,49 @@ async def test_create_generates_ac_ids_and_carries_parent_ac_refs() -> None:
     assert list(task.parent_ac_refs) == ["parent-ac-1", "parent-ac-2"]
 
 
+def test_reconcile_ac_ids_preserves_new_and_drops() -> None:
+    # Unchanged text keeps its id (position may shift); a reworded/new entry
+    # mints a fresh one; a dropped criterion's id disappears with it.
+    _N = 3
+    ids = _reconcile_ac_ids(
+        old_criteria=["a", "b", "c"],
+        old_ids=["id-a", "id-b", "id-c"],
+        new_criteria=["a", "c", "d"],  # b dropped, a/c kept (reordered), d new
+    )
+    assert ids[0] == "id-a"
+    assert ids[1] == "id-c"
+    assert ids[2] not in {"id-a", "id-b", "id-c"}
+    assert len(ids) == len(set(ids)) == _N
+
+
+def test_reconcile_ac_ids_mints_fresh_when_nothing_to_preserve() -> None:
+    _N = 2
+    ids = _reconcile_ac_ids(old_criteria=[], old_ids=[], new_criteria=["x", "y"])
+    assert len(ids) == len(set(ids)) == _N
+
+
+def test_reconcile_ac_ids_duplicate_text_matched_in_order() -> None:
+    ids = _reconcile_ac_ids(
+        old_criteria=["dup", "dup"],
+        old_ids=["id-1", "id-2"],
+        new_criteria=["dup", "dup", "dup"],
+    )
+    assert ids[:2] == ["id-1", "id-2"]
+    assert ids[2] not in {"id-1", "id-2"}
+
+
 def _svc_with_children(parent: object, child_rows: list[tuple]) -> TaskService:
     """TaskService whose get() returns `parent` and whose execute() yields the
-    (status, parent_ac_refs) child rows the coverage primitive selects."""
+    (status, parent_ac_refs) child rows the coverage primitive selects.
+
+    ``flush`` is a real AsyncMock (not the unconfigured default) so the
+    empty/mismatched-ids self-heal in ``_parent_ac_ref_sets`` can await it.
+    """
     rows = MagicMock()
     rows.all.return_value = child_rows
-    svc = TaskService(MagicMock(execute=AsyncMock(return_value=rows)))
+    svc = TaskService(
+        MagicMock(execute=AsyncMock(return_value=rows), flush=AsyncMock())
+    )
     _bind(svc, "get", AsyncMock(return_value=parent))
     return svc
 
@@ -1362,12 +1400,48 @@ async def test_parent_ac_coverage_maps_claimed_and_verified() -> None:
 
 
 @pytest.mark.asyncio
-async def test_parent_ac_coverage_empty_without_ac_ids() -> None:
-    # No stable ids on the parent (e.g. created before the linkage) -> nothing to
-    # report; the digest stays absent rather than emitting bogus rows.
+async def test_parent_ac_coverage_self_heals_empty_ac_ids() -> None:
+    # fe-pm delegate-loop incident: a coordination root had real
+    # acceptance_criteria but zero acceptance_criteria_ids (an update rewrote
+    # criteria without reconciling ids) -> the digest used to stay [] forever,
+    # silently disabling the parent-coverage gate. It now self-heals by
+    # stamping fresh ids in place so the digest reports for real.
     parent = _build_task(acceptance_criteria=["a"], acceptance_criteria_ids=[])
     svc = _svc_with_children(parent, [(TaskStatus.IN_PROGRESS, ["id-a"])])
-    assert await svc.parent_ac_coverage(parent.id) == []
+    cov = await svc.parent_ac_coverage(parent.id)
+    assert len(cov) == 1
+    assert cov[0]["text"] == "a"
+    assert cov[0]["id"]  # freshly stamped, non-empty
+    assert list(parent.acceptance_criteria_ids) == [cov[0]["id"]]
+
+
+@pytest.mark.asyncio
+async def test_uncovered_parent_acs_self_heals_and_still_matches_text_refs() -> None:
+    # Same legacy shape as above, but proves the self-heal doesn't regress the
+    # id-or-text matching: a child that declared coverage by TEXT (the only
+    # option while the parent had no ids) still resolves through the
+    # freshly-stamped ids, and the gate is live again instead of permanently
+    # inert.
+    _N = 2
+    parent = _build_task(
+        acceptance_criteria=["crit a", "crit b"], acceptance_criteria_ids=[]
+    )
+    svc = _svc_with_children(parent, [(TaskStatus.COMPLETED, ["crit a"])])
+    assert await svc.uncovered_parent_acceptance_criteria(parent.id) == ["crit b"]
+    assert len(parent.acceptance_criteria_ids) == _N
+    assert len(set(parent.acceptance_criteria_ids)) == _N
+
+
+@pytest.mark.asyncio
+async def test_parent_ac_coverage_no_self_heal_when_ids_already_1to1() -> None:
+    # The common case (ids already match criteria 1:1) must not re-stamp —
+    # the self-heal only fires on an actual length mismatch.
+    parent = _build_task(
+        acceptance_criteria=["a", "b"], acceptance_criteria_ids=["id-a", "id-b"]
+    )
+    svc = _svc_with_children(parent, [(TaskStatus.COMPLETED, ["id-a", "id-b"])])
+    await svc.parent_ac_coverage(parent.id)
+    assert list(parent.acceptance_criteria_ids) == ["id-a", "id-b"]
 
 
 @pytest.mark.asyncio