Fix #2581: auto-ACK pure producers when slice has no tasks for their role

orchestrator/approval_matrix.py

@@ -232,6 +232,77 @@ def is_context_change_nack(
             return False
         return not bool(prev_refs & new_refs)
 
+    def seed_auto_ack_for_empty_pure_producers(self, producers_with_tasks: set[str]) -> list[str]:
+        """Pre-seed proposal + critical-reviewer ACKs for pure producers
+        whose role has no tasks in this slice.
+
+        Prevents a BRC deadlock (#2581) where a producer-only slice (e.g.
+        tester-only or documenter-only) leaves CODER with no work but
+        still spawned: CODER's critical reviewers (REVIEWER_CODE et al.)
+        have nothing to review and may NACK indefinitely, since the
+        protocol requires every critical reviewer to ACK at the latest
+        version.
+
+        Behavior, per producer ``P`` in the graph not present in
+        ``producers_with_tasks``:
+
+        * Skip ``P`` if ``graph.is_dual_role(P)`` — a dual-role producer
+          (e.g. TESTER also reviews CODER) must always run so it can
+          discharge its reviewer responsibilities for the *other*
+          producers; auto-ACKing it as a producer is fine in principle,
+          but right now no role besides TESTER is dual-role, and skipping
+          here keeps the rule trivially aligned with the "tester always
+          runs" intent.
+        * Otherwise call :meth:`record_proposal` to bump ``P``'s
+          ``proposal_version`` (returns the new version ``v``), then
+          record an ACK at version ``v`` from **every** critical reviewer
+          of ``P``. On the first invocation ``v == 1``; if the seeder is
+          ever invoked again on the same matrix (e.g. a retry path) ``v``
+          increments, and the new round of seeded ACKs lands at the new
+          version.
+
+        **Dual-role reviewer pre-ACK — known failure mode.** This method
+        seeds an ACK from **every** critical reviewer of ``P``, including
+        dual-role reviewers like TESTER reviewing CODER. The reviewer's
+        own producer work has not yet run, so its ACK of ``P`` is a
+        *starting state*, not a final verdict: if its own work later
+        uncovers a need for ``P`` to produce something, it can NACK at
+        version ``v``, which overrides the seeded ACK and forces ``P`` to
+        re-propose at version ``v+1`` via the normal flow. Operationally
+        this means if a dual-role reviewer's container crashes, deadlocks,
+        or otherwise fails to revisit ``P`` before its own work is done,
+        the seeded ACK becomes the final word — a false-positive ACK that
+        a future reader of the matrix will see as "TESTER ACKed CODER"
+        without TESTER ever having reviewed anything. Issue #2581's
+        proposed design left dual-role reviewers PENDING (silence reads
+        as "not done"); we trade that defensive default for "consensus
+        reachable when the dual-role reviewer never gets to it," because
+        the alternative is exactly the deadlock this seed exists to
+        prevent. Operators inspecting a stalled slice should treat
+        seeded TESTER→CODER ACKs as advisory rather than authoritative.
+
+        The producer container is still spawned by the caller — this only
+        pre-seeds the matrix. The caller is responsible for telling the
+        empty pure producer (via its prompt) to skip its propose step;
+        otherwise the agent's real propose at version ``v+1`` invalidates
+        the seeded ACKs and the deadlock recurs.
+
+        Returns the list of producer roles that were auto-ACKed (mostly
+        useful for logging / tests).
+        """
+        auto_acked: list[str] = []
+        # ``empty_pure_producers`` is the single source of truth for "this
+        # role is a pure producer with no tasks in this slice" (#2581).
+        # ``_run_concurrent_phase`` uses the same helper to compute the
+        # prompt-level shortcut flag — keeping the prompt and the matrix
+        # state in lockstep.
+        for producer in sorted(self._graph.empty_pure_producers(producers_with_tasks)):
+            version = self.record_proposal(producer)
+            for reviewer in self._graph.critical_reviewers_for(producer):
+                self.record_ack(reviewer, producer, version=version)
+            auto_acked.append(producer)
+        return auto_acked
+
     def is_fully_acked(self, producer: str) -> bool:
         """Check if all critical reviewers have ACKed the producer's latest proposal.
 

orchestrator/concurrent_executor.py

@@ -142,6 +142,7 @@ def __init__(
         review_graph: ReviewGraph | None = None,
         roles: list[AgentRole] | None = None,
         slice_id: str | None = None,
+        producer_roles_with_tasks: set[str] | None = None,
     ) -> None:
         """Initialise the executor.
 
@@ -167,13 +168,25 @@ def __init__(
                 ``egg/issue-N/{slice_id}/{role}/work`` so commits across
                 slices stay isolated. ``None`` preserves the pre-slicing
                 pipeline-scoped semantics.
+            producer_roles_with_tasks: Set of producer role names that
+                actually have at least one task in the slice's plan
+                (#2581). When provided, ``spawn_all`` calls
+                ``tracker.seed_auto_ack_for_empty_pure_producers`` after
+                registering agents so pure producers (CODER, DOCUMENTER)
+                with no tasks don't deadlock BRC consensus waiting for
+                reviewers to ACK an empty proposal. ``None`` (the
+                default) preserves the prior unconditional-roster
+                behavior; callers that don't know the slice's task list
+                yet (CUSTOM-mode, BABYSIT, prompt-mode pipelines) leave
+                it unset.
         """
         self.pipeline = pipeline
         self.spawn_fn = spawn_fn
         self.max_concurrent = max_concurrent
         self._review_graph = review_graph
         self._roles_override = roles
         self._slice_id = slice_id
+        self._producer_roles_with_tasks = producer_roles_with_tasks
         self._failure_times: list[datetime] = []
         self._lock = threading.Lock()
 
@@ -397,6 +410,28 @@ def spawn_all(
         for role in roles:
             tracker.register_agent(role.value)
 
+        # Pre-seed BRC consensus for pure producers that have no tasks in
+        # this slice (#2581). Without this, e.g. a tester-only or
+        # documenter-only slice still spawns CODER, which proposes an
+        # empty artifact list — and CODER's pure reviewers can NACK
+        # forever ("no code to review"), spiraling until
+        # ``max_revision_rounds``. The seeder records an empty proposal +
+        # synthetic ACKs from pure reviewers; dual-role reviewers (TESTER
+        # reviewing CODER) are intentionally left to run and decide.
+        # Skipped when callers don't supply the task-role set (CUSTOM-mode,
+        # BABYSIT, prompt-mode), preserving the prior behavior.
+        if self._producer_roles_with_tasks is not None:
+            auto_acked = tracker.seed_auto_ack_for_empty_pure_producers(
+                self._producer_roles_with_tasks
+            )
+            if auto_acked:
+                logger.info(
+                    "Pre-seeded BRC auto-ACK for empty pure producers",
+                    pipeline_id=self.pipeline.id,
+                    slice_id=self._slice_id,
+                    auto_acked=auto_acked,
+                )
+
         return self._spawn_roles(roles, agent_prompts or {})
 
     def spawn_specific_roles(

orchestrator/peer_consensus.py

@@ -186,6 +186,16 @@ def register_agent(self, role: str) -> None:
             if self.graph.is_reviewer(role):
                 self._reviewer_phases[role] = ConsensusPhase.WORKING
 
+    def seed_auto_ack_for_empty_pure_producers(self, producers_with_tasks: set[str]) -> list[str]:
+        """Pre-seed the matrix for pure producers absent from the slice's task list.
+
+        Thin lock-holding wrapper around
+        ``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers`` (#2581).
+        See that method's docstring for the dual-role / pure-reviewer rules.
+        """
+        with self._lock:
+            return self.matrix.seed_auto_ack_for_empty_pure_producers(producers_with_tasks)
+
     def release_nudge(self, role: str, version: int) -> None:
         """Roll back a nudge memo entry recorded by ``_collect_newly_ready_producers``.
 

orchestrator/review_graph.py

@@ -147,6 +147,50 @@ def all_roles(self) -> set[str]:
         """Get all roles participating in the graph."""
         return self._producer_roles | self._reviewer_roles
 
+    def producer_roles(self) -> set[str]:
+        """Get all roles that act as producers in the graph.
+
+        Returned as a snapshot copy so callers can mutate or iterate without
+        risking concurrent modification of the internal set.
+        """
+        return set(self._producer_roles)
+
+    def reviewer_roles(self) -> set[str]:
+        """Get all roles that act as reviewers in the graph.
+
+        Returned as a snapshot copy so callers can mutate or iterate without
+        risking concurrent modification of the internal set.
+        """
+        return set(self._reviewer_roles)
+
+    def empty_pure_producers(self, producers_with_tasks: set[str]) -> set[str]:
+        """Producer roles that are pure-producers AND absent from
+        ``producers_with_tasks`` (#2581).
+
+        Single source of truth for the empty-pure-producer predicate
+        used by both the matrix-level seed
+        (``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers``)
+        and the prompt-level shortcut flag
+        (``_run_concurrent_phase`` → ``is_pre_seeded_empty_producer``).
+        Without one helper, the two call sites can drift — e.g. a future
+        change adding a third skip condition would have to be applied to
+        both, and the prompt could appear without the matrix being seeded
+        (or vice versa).
+
+        A role qualifies as an empty pure producer iff:
+
+        * it is a producer in this graph (``is_producer``),
+        * it does not appear in ``producers_with_tasks``,
+        * it is not also a reviewer (``is_dual_role`` is False) — dual-role
+          producers (currently only TESTER) always run so they can
+          discharge their reviewer responsibilities.
+        """
+        return {
+            p
+            for p in self._producer_roles
+            if p not in producers_with_tasks and not self.is_dual_role(p)
+        }
+
     def to_dict(self) -> dict[str, Any]:
         """Serialize the graph."""
         return {