From 3483e390f2119f9b123eadf2c679b8ca12c1d111 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Sun, 10 May 2026 22:21:01 -0700 Subject: [PATCH 1/4] Fix #2581: auto-ACK pure producers when slice has no tasks for their role Pre-seeds the BRC approval matrix for pure producers (CODER, DOCUMENTER) whose role has no tasks in the slice's plan, so a tester-only or documenter-only slice doesn't deadlock waiting for reviewers to ACK an empty proposal. Dual-role producers (TESTER) keep current behavior. A dual-role reviewer can NACK at the seeded version to recover the "tester needs coder to do work" path. Supersedes #2565 / closes the approach in PR #2567. --- orchestrator/approval_matrix.py | 50 ++++++ orchestrator/concurrent_executor.py | 35 ++++ orchestrator/peer_consensus.py | 10 ++ orchestrator/routes/pipelines.py | 29 ++++ .../tests/test_auto_ack_pure_producers.py | 163 ++++++++++++++++++ 5 files changed, 287 insertions(+) create mode 100644 orchestrator/tests/test_auto_ack_pure_producers.py diff --git a/orchestrator/approval_matrix.py b/orchestrator/approval_matrix.py index 0f2b7b7f72..f905551f55 100644 --- a/orchestrator/approval_matrix.py +++ b/orchestrator/approval_matrix.py @@ -232,6 +232,56 @@ 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 record an empty proposal at version 1, then record + an ACK at version 1 from **every** critical reviewer of ``P``. + The ACK from a dual-role reviewer (e.g. TESTER reviewing + CODER) is a starting state, not a final say: if the + dual-role reviewer's own producer work later uncovers a need + for ``P`` to produce something, it can NACK at version 1, which + overrides the seeded ACK and forces ``P`` to re-propose at + version 2 via the normal flow. + + The producer container is still spawned by the caller — this only + pre-seeds the matrix. If the agent later proposes for real, the + version bumps and the seeded ACKs are superseded by the normal + flow. + + Returns the list of producer roles that were auto-ACKed (mostly + useful for logging / tests). + """ + auto_acked: list[str] = [] + for producer in sorted(self._graph._producer_roles): + if producer in producers_with_tasks: + continue + if self._graph.is_dual_role(producer): + continue + 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. diff --git a/orchestrator/concurrent_executor.py b/orchestrator/concurrent_executor.py index 2855bb6d8e..1cd6986d1c 100644 --- a/orchestrator/concurrent_executor.py +++ b/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,6 +168,17 @@ 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 @@ -174,6 +186,7 @@ def __init__( 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( diff --git a/orchestrator/peer_consensus.py b/orchestrator/peer_consensus.py index e11c47de87..deff6a8e4c 100644 --- a/orchestrator/peer_consensus.py +++ b/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``. diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 656297f5c3..58b22174de 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -14946,6 +14946,34 @@ def _run_concurrent_phase( ] filtered_graph = ReviewGraph(filtered_edges) + # Determine which producer roles the slice's plan actually assigns + # tasks to (#2581). Used to pre-seed auto-ACKs for pure producers + # (e.g. CODER, DOCUMENTER) that the planner didn't include — + # otherwise their empty proposal can deadlock BRC consensus when + # reviewers NACK "nothing to review". Only meaningful for + # per-slice runs against a contracted pipeline; CUSTOM-mode, + # BABYSIT, and prompt-mode pipelines fall through to ``None`` + # which preserves the pre-#2581 unconditional-roster behavior. + producer_roles_with_tasks: set[str] | None = None + if slice_id is not None and getattr(pipeline, "has_contract", True): + try: + from egg_contracts.loader import load_contract as _load_contract_for_seed + + _contract = _load_contract_for_seed(pipeline.id, worktree_repo_path) + _slice_obj = next((s for s in _contract.slices if s.id == slice_id), None) + if _slice_obj is not None: + # ``Task.role`` is ``str | None``; ``None`` is the + # execution-time coder default per the contract schema. + producer_roles_with_tasks = {(t.role or "coder") for t in _slice_obj.tasks} + except Exception: + logger.debug( + "Could not derive producer_roles_with_tasks for auto-ACK seeding", + pipeline_id=pipeline.id, + slice_id=slice_id, + exc_info=True, + ) + producer_roles_with_tasks = None + # Resolve base branch for diff commands in agent prompts. _resolved_base_branch = pipeline.base_branch if not _resolved_base_branch: @@ -14999,6 +15027,7 @@ def _run_concurrent_phase( review_graph=filtered_graph, roles=roles, slice_id=slice_id, + producer_roles_with_tasks=producer_roles_with_tasks, ) # Spawn all agents with their prompts. diff --git a/orchestrator/tests/test_auto_ack_pure_producers.py b/orchestrator/tests/test_auto_ack_pure_producers.py new file mode 100644 index 0000000000..b2c518e673 --- /dev/null +++ b/orchestrator/tests/test_auto_ack_pure_producers.py @@ -0,0 +1,163 @@ +"""Tests for the pure-producer auto-ACK seed (#2581). + +Covers ``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers`` and the +delegating ``PeerConsensusTracker.seed_auto_ack_for_empty_pure_producers`` +wrapper. The seed exists to prevent BRC consensus deadlock for slices +whose plan omits a producer role (e.g. a tester-only or documenter-only +slice): without it, CODER spawns with no work, its critical reviewers +have nothing to review, and consensus stalls. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +_orchestrator_path = Path(__file__).parent.parent +if str(_orchestrator_path) not in sys.path: + sys.path.insert(0, str(_orchestrator_path)) + +from approval_matrix import ApprovalMatrix, ApprovalState +from peer_consensus import PeerConsensusTracker +from review_graph import get_default_implement_graph + + +@pytest.fixture +def implement_graph(): + return get_default_implement_graph() + + +@pytest.fixture +def matrix(implement_graph): + return ApprovalMatrix(implement_graph) + + +class TestSeedAutoAckEmptyPureProducers: + """Direct tests against ``ApprovalMatrix``.""" + + def test_documenter_only_slice_auto_acks_coder(self, matrix): + """A documenter-only slice should auto-ACK CODER so its reviewers + don't deadlock on an empty proposal.""" + auto_acked = matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert auto_acked == ["coder"] + # CODER's consensus is fully satisfied after seeding — every + # critical reviewer (including TESTER) has a seeded ACK at v1. + assert matrix.is_fully_acked("coder") is True + + def test_tester_only_slice_auto_acks_coder_and_documenter(self, matrix): + """A tester-only slice has neither a coder nor a documenter task — + both pure producers should auto-ACK.""" + auto_acked = matrix.seed_auto_ack_for_empty_pure_producers({"tester"}) + assert auto_acked == ["coder", "documenter"] + assert matrix.is_fully_acked("coder") is True + # DOCUMENTER has no critical reviewers in the default implement + # graph, so ``is_fully_acked`` returns True once any proposal is + # recorded. + assert matrix.is_fully_acked("documenter") is True + + def test_coder_only_slice_auto_acks_only_documenter(self, matrix): + """A coder-only slice: DOCUMENTER (pure producer) auto-ACKs; + TESTER is dual-role and is intentionally skipped so its reviewer + responsibility for CODER stays active.""" + auto_acked = matrix.seed_auto_ack_for_empty_pure_producers({"coder"}) + assert auto_acked == ["documenter"] + assert matrix.is_fully_acked("documenter") is True + # TESTER not auto-ACKed — its producer-side proposal version is + # still 0, so it isn't fully-ACKed. + assert matrix.get_proposal_version("tester") == 0 + assert matrix.is_fully_acked("tester") is False + + def test_all_producers_present_is_noop(self, matrix): + """When every producer has at least one task, seeding is a no-op.""" + auto_acked = matrix.seed_auto_ack_for_empty_pure_producers( + {"coder", "tester", "documenter"} + ) + assert auto_acked == [] + assert matrix.get_proposal_version("coder") == 0 + assert matrix.get_proposal_version("documenter") == 0 + + def test_seeded_acks_carry_critical_reviewers(self, matrix): + """The seeded proposal must satisfy every critical reviewer of + the auto-ACKed producer, including dual-role reviewers like + TESTER — that's what makes the empty proposal actually reach + consensus instead of just being recorded.""" + matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + graph = matrix._graph + for reviewer in graph.critical_reviewers_for("coder"): + entry = matrix.get_entry(reviewer, "coder") + assert entry is not None + assert entry.state == ApprovalState.ACKED + assert entry.version == 1 + + def test_dual_role_producer_is_never_auto_acked(self, matrix): + """Even when TESTER has no tasks (documenter-only slice), it must + not be auto-ACKed as a producer — its dual role means it should + always run so it can ACK/NACK CODER for real.""" + matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + # TESTER's producer-side state: untouched. + assert matrix.get_proposal_version("tester") == 0 + + def test_dual_role_reviewer_can_nack_to_override_seeded_ack(self, matrix): + """The 'tester may need coder to do some work' recovery path: + TESTER's seeded ACK of CODER is a starting state, not a verdict. + If TESTER's producer work later uncovers a need for code, it can + NACK at the seeded version and force CODER to re-propose.""" + matrix.seed_auto_ack_for_empty_pure_producers({"tester"}) + assert matrix.is_fully_acked("coder") is True + + matrix.record_nack("tester", "coder", version=1, reason="need helper module") + + assert matrix.is_fully_acked("coder") is False + entry = matrix.get_entry("tester", "coder") + assert entry is not None + assert entry.state == ApprovalState.NACKED + + def test_real_proposal_supersedes_seeded_acks(self, matrix): + """If the producer's container later proposes for real (version + bump), the seeded version-1 ACKs are no longer at the latest + version — the normal flow re-acquires fresh ACKs at v2.""" + matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert matrix.get_proposal_version("coder") == 1 + + new_version = matrix.record_proposal("coder") + assert new_version == 2 + # is_fully_acked falls back to False because seeded ACKs are at v1. + assert matrix.is_fully_acked("coder") is False + + def test_seed_called_twice_is_idempotent_in_effect(self, matrix): + """Calling the seeder twice with the same task set bumps the + proposal version but leaves consensus reachable — the second + call records ACKs at the new (v2) version. Idempotent in the + sense that the post-state is still fully-ACKed.""" + first = matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + second = matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert first == ["coder"] + assert second == ["coder"] + assert matrix.is_fully_acked("coder") is True + + +class TestTrackerDelegation: + """The ``PeerConsensusTracker`` wrapper holds the lock and + delegates to the matrix. These tests are thin — the heavy lifting + is covered above.""" + + def test_tracker_delegates_to_matrix(self, implement_graph): + tracker = PeerConsensusTracker( + pipeline_id="test-pipeline", + graph=implement_graph, + ) + auto_acked = tracker.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert auto_acked == ["coder"] + assert tracker.matrix.is_fully_acked("coder") is True + + def test_tracker_noop_when_all_producers_present(self, implement_graph): + tracker = PeerConsensusTracker( + pipeline_id="test-pipeline", + graph=implement_graph, + ) + auto_acked = tracker.seed_auto_ack_for_empty_pure_producers( + {"coder", "tester", "documenter"} + ) + assert auto_acked == [] From 55daadf2129888f6e5ad2840b72eddeae0664dac Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 11 May 2026 06:02:43 +0000 Subject: [PATCH 2/4] =?UTF-8?q?Address=20review=20feedback=20on=20PR=20#25?= =?UTF-8?q?83=20=E2=80=94=20wire=20end-to-end=20and=20harden=20seed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses blocking and non-blocking concerns raised by egg-reviewer on the auto-ACK-pure-producers PR. End-to-end wiring (blocking #1) Without an agent-side skip-propose path, the seed prevented the deadlock only at the matrix level — CODER's container still ran the standard producer lifecycle, proposed at version 2, and invalidated the seeded version-1 ACKs, re-opening the deadlock the seed exists to prevent. - Thread ``is_pre_seeded_empty_producer`` through ``_build_agent_prompt`` → ``_build_phase_prompt`` → ``_build_brc_preamble`` → ``_build_producer_orientation``. - Derive the per-role flag from the same predicate the matrix seed uses (``producer_roles() − producer_roles_with_tasks``, skipping dual-role) so prompt and matrix stay in sync. - The producer-lifecycle preamble grows a top-of-block shortcut notice telling pre-seeded coders/documenters to skip propose, confirm directly, and fall through to step 4's wait-loop on ``pending_acks: global_zero_proposal``. The existing ``_collect_newly_ready_producers`` sweep emits the STATUS ``ready_to_confirm`` nudge naturally when another producer proposes; no extra orchestration plumbing required. - Orient text for pre-seeded coders/documenters is shortened to "confirm no tasks, do not invent work." - On the dual-role-reviewer-NACK recovery path (TESTER NACKs the seeded CODER v=1), the shortcut routes through ``mcp__sdlc__register_open_question`` rather than silently starting to produce — surfaces the planning gap to the operator. Narrow exception handling (blocking #2) ``_run_concurrent_phase`` previously swallowed any ``Exception`` at ``logger.debug``, hiding contract-load failures and silently re-introducing the deadlock when the seed couldn't run. Now: - Catch only ``ContractNotFoundError`` / ``ContractValidationError`` / ``OSError`` narrowly; unknown exceptions propagate so schema bumps fail loudly in testing. - Upgrade the log level from DEBUG to WARNING so operators see the "safety net is off" condition by default. - The slice-id-not-in-contract path is now an explicit WARNING with the contract's available slice ids inlined, so a contract-on-main vs slice-on-branch skew is diagnosable. Documenter-only TESTER scenario (blocking #3) Added ``TestDocumenterOnlySliceTesterFlow`` test asserting that CODER pre-seeded + DOCUMENTER normal propose + TESTER no-op propose (with its critical-reviewer ACKs) yields a fully-ACKed, consensus-reachable matrix for every producer in the graph. Pins down the composition of the existing ``no_test_changes_needed`` path (#2431) with the new seed. Non-blocking items - #4 Documented the dual-role pre-ACK known failure mode in the matrix docstring: seeded TESTER→CODER ACKs are advisory, not authoritative, and operators inspecting a stalled slice should treat them as such. - #5 Added integration-style tests for the wiring layer: ``TestProducerRolesWithTasksDerivation`` and ``TestProducerOrientationPreSeededShortcut``. - #6 Renamed the misleading idempotency test and added the proposal-version assertion so the version-inflation is explicitly observable. - #8 Added public ``ReviewGraph.producer_roles()`` and ``reviewer_roles()`` accessors returning snapshot copies; the seed now uses ``producer_roles()`` instead of reaching into ``_producer_roles``. - #9 Replaced the "version 1" imprecision in the seed docstring with the new-version semantic and a note about subsequent invocations. --- orchestrator/approval_matrix.py | 44 ++-- orchestrator/review_graph.py | 16 ++ orchestrator/routes/pipelines.py | 134 +++++++++++- .../tests/test_auto_ack_pure_producers.py | 199 +++++++++++++++++- 4 files changed, 368 insertions(+), 25 deletions(-) diff --git a/orchestrator/approval_matrix.py b/orchestrator/approval_matrix.py index f905551f55..adffd741c8 100644 --- a/orchestrator/approval_matrix.py +++ b/orchestrator/approval_matrix.py @@ -253,25 +253,45 @@ def seed_auto_ack_for_empty_pure_producers(self, producers_with_tasks: set[str]) 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 record an empty proposal at version 1, then record - an ACK at version 1 from **every** critical reviewer of ``P``. - The ACK from a dual-role reviewer (e.g. TESTER reviewing - CODER) is a starting state, not a final say: if the - dual-role reviewer's own producer work later uncovers a need - for ``P`` to produce something, it can NACK at version 1, which - overrides the seeded ACK and forces ``P`` to re-propose at - version 2 via the normal flow. + * 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. If the agent later proposes for real, the - version bumps and the seeded ACKs are superseded by the normal - flow. + 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] = [] - for producer in sorted(self._graph._producer_roles): + for producer in sorted(self._graph.producer_roles()): if producer in producers_with_tasks: continue if self._graph.is_dual_role(producer): diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index ca39fa1472..6880da9dd9 100644 --- a/orchestrator/review_graph.py +++ b/orchestrator/review_graph.py @@ -147,6 +147,22 @@ 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 to_dict(self) -> dict[str, Any]: """Serialize the graph.""" return { diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 58b22174de..1a5ed98f63 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -11052,6 +11052,7 @@ def _build_brc_preamble( *, mode: PipelineMode | None = None, pr_number: int | None = None, + is_pre_seeded_empty_producer: bool = False, ) -> str: """Build the BRC consensus lifecycle preamble for an agent. @@ -11068,6 +11069,14 @@ def _build_brc_preamble( mode: Pipeline execution mode. Forwarded to producer/reviewer orient builders so babysit-pr pipelines receive PR-diff-aware prompts. pr_number: GitHub PR number; forwarded with ``mode``. + is_pre_seeded_empty_producer: True when this role is a pure producer + (coder, documenter) whose slice plan contains no tasks for the + role and whose BRC matrix entry was pre-seeded by the + orchestrator (#2581). The producer lifecycle text instructs + the agent to SKIP its propose step entirely and confirm + directly — otherwise its real propose at version 2 would + invalidate the seeded version-1 ACKs and re-trigger the + deadlock the seed exists to prevent. """ try: from review_graph import get_review_graph_for_phase @@ -11131,9 +11140,51 @@ def _build_brc_preamble( lines.append(roster) if is_producer: - lines.extend( + producer_lifecycle: list[str] = ["### Producer Lifecycle"] + # Pre-seeded empty-pure-producer shortcut (#2581). When the + # orchestrator has determined this role has no tasks in the + # current slice and pre-seeded its matrix state, the agent MUST + # NOT run the normal propose flow — its real propose would bump + # the proposal version and invalidate the seeded ACKs, reopening + # the deadlock the seed exists to prevent. + if is_pre_seeded_empty_producer: + producer_lifecycle.append( + "**Pre-seeded empty-producer shortcut (#2581).** Your role " + "has no tasks in this slice's plan and the orchestrator " + "has pre-seeded your BRC consensus matrix entry: an empty " + "proposal at version 1 plus an ACK at version 1 from every " + "critical reviewer of your role. " + "**Do NOT run `egg-orch consensus propose`** at any point — " + "your real propose would bump the version to 2 and " + "invalidate the seeded ACKs, re-opening the deadlock the " + "seed exists to prevent.\n\n" + "Your lifecycle replaces steps 2–5 below with this short flow:\n" + " (a) Run step 1 (ORIENT) to confirm your role has no tasks.\n" + " (b) Try `egg-orch consensus confirmed`.\n" + " - If it succeeds, proceed to step 6 (STAY ALIVE).\n" + " - If it returns `status: pending_acks` referencing " + "`global_zero_proposal` (other slice producers haven't " + "proposed yet), this is expected. Block on " + "`egg-orch message wait-loop --for STATUS --for " + "CONSENSUS_RE_REVIEW --for OVERSEER_ALERT`. On STATUS " + "with metadata `ready_to_confirm: true` (#2531), retry " + "`egg-orch consensus confirmed`. On `CONSENSUS_RE_REVIEW` " + "for your role, re-confirm (do not propose). On " + "`OVERSEER_ALERT`, surface it.\n" + " - If it returns `status: pending_acks` with " + "`producer_not_fully_acked`, a dual-role reviewer (TESTER) " + "has NACKed the seeded version because its own work uncovered " + "a need for code your role should have produced. This is a " + "planning gap — call " + "`mcp__sdlc__register_open_question` with options " + '`("Add coder task to this slice", "Defer to a follow-up ' + 'slice", "Treat the slice as documenter-only")` so the ' + "operator can resolve it; do NOT silently start producing.\n" + " (c) Proceed to step 6 (STAY ALIVE) and follow the normal " + "stay-alive / re-review handling." + ) + producer_lifecycle.extend( [ - "### Producer Lifecycle", "1. **ORIENT**: Before starting work, " + _build_producer_orientation( role_value, @@ -11143,6 +11194,7 @@ def _build_brc_preamble( base_branch=base_branch, mode=mode, pr_number=pr_number, + is_pre_seeded_empty_producer=is_pre_seeded_empty_producer, ), "2. **WORK**: Complete your assigned task (see Your Task below).", "3. **PROPOSE**: When done, run: " @@ -11232,6 +11284,7 @@ def _build_brc_preamble( "single-handedly bypass the reviewer's veto.\n", ] ) + lines.extend(producer_lifecycle) if is_reviewer: lines.extend( @@ -11782,6 +11835,7 @@ def _build_producer_orientation( base_branch: str | None = None, mode: PipelineMode | None = None, pr_number: int | None = None, + is_pre_seeded_empty_producer: bool = False, ) -> str: """Build orientation instructions for producer agents. @@ -11802,7 +11856,24 @@ def _build_producer_orientation( role's file scope, and escalate cross-role overlap to the on-demand ``conflict_resolver`` role (#1748). pr_number: GitHub PR number (only meaningful in babysit mode). + is_pre_seeded_empty_producer: True when this role has no tasks in + the slice and its matrix entry was pre-seeded (#2581). The + orient text is shortened to "read the contract, confirm there + are no tasks for your role, do not produce" — the lifecycle + preamble above already tells the agent to skip propose and + confirm directly. """ + if is_pre_seeded_empty_producer and phase == "implement": + return ( + "read the contract (`egg-contract show`) and confirm your role " + "has no tasks in the current slice — this matches the " + "orchestrator's pre-seeded matrix state. **Do not invent work** " + "or stretch the slice's scope to author code/docs that the " + "planner did not assign to you; the pre-seeded path exists " + "precisely to let this slice reach consensus without your " + "contribution. Read the lifecycle shortcut block above, then " + "go directly to step 5 (CONFIRM)." + ) reviewer_awareness = "" if reviewers: reviewer_names = ", ".join(reviewers) @@ -12041,6 +12112,7 @@ def _build_agent_prompt( *, mode: PipelineMode | None = None, pr_number: int | None = None, + is_pre_seeded_empty_producer: bool = False, ) -> str: """Build a role-specific prompt for multi-agent execution. @@ -12117,6 +12189,7 @@ def _build_agent_prompt( base_branch=base_branch, mode=mode, pr_number=pr_number, + is_pre_seeded_empty_producer=is_pre_seeded_empty_producer, ) return base_prompt @@ -12147,6 +12220,7 @@ def _build_agent_prompt( base_branch=base_branch, mode=mode, pr_number=pr_number, + is_pre_seeded_empty_producer=is_pre_seeded_empty_producer, ) ) @@ -14956,21 +15030,48 @@ def _run_concurrent_phase( # which preserves the pre-#2581 unconditional-roster behavior. producer_roles_with_tasks: set[str] | None = None if slice_id is not None and getattr(pipeline, "has_contract", True): - try: - from egg_contracts.loader import load_contract as _load_contract_for_seed + from egg_contracts.loader import ( + ContractNotFoundError, + ContractValidationError, + ) + from egg_contracts.loader import ( + load_contract as _load_contract_for_seed, + ) + try: _contract = _load_contract_for_seed(pipeline.id, worktree_repo_path) _slice_obj = next((s for s in _contract.slices if s.id == slice_id), None) - if _slice_obj is not None: + if _slice_obj is None: + # The slice id is well-formed but not in this contract — likely + # a contract-on-main vs slice-on-branch skew, or a bad slice id + # passed in. Log loud so operators can spot the safety net + # being off; let agents run unseeded. + logger.warning( + "Slice id not found in contract — auto-ACK seeding off " + "for this run; pure producers in this slice may deadlock " + "if they have no tasks", + pipeline_id=pipeline.id, + slice_id=slice_id, + available_slice_ids=[s.id for s in _contract.slices], + ) + else: # ``Task.role`` is ``str | None``; ``None`` is the # execution-time coder default per the contract schema. producer_roles_with_tasks = {(t.role or "coder") for t in _slice_obj.tasks} - except Exception: - logger.debug( - "Could not derive producer_roles_with_tasks for auto-ACK seeding", + except (ContractNotFoundError, ContractValidationError, OSError) as exc: + # Narrow catch (#2581 review): only swallow load-time errors we + # can reasonably recover from. Unknown exceptions (schema bumps, + # AttributeError on contract model changes) propagate so they're + # caught loudly during testing instead of silently re-introducing + # the deadlock in production. Logged at WARNING — operators need + # to know the safety net is off. + logger.warning( + "Could not derive producer_roles_with_tasks for auto-ACK seeding — " + "pure producers in this slice may deadlock if they have no tasks", pipeline_id=pipeline.id, slice_id=slice_id, - exc_info=True, + error_type=type(exc).__name__, + error=str(exc), ) producer_roles_with_tasks = None @@ -14982,6 +15083,20 @@ def _run_concurrent_phase( except Exception: _resolved_base_branch = None + # Decide which roles will be pre-seeded as empty pure producers + # (#2581). The producer-prompt path uses this to inject a shortcut + # block telling the agent to skip its propose step entirely — required + # for the matrix-level seed to survive end-to-end (the agent's real + # propose would bump the version and invalidate the seeded ACKs). + _pre_seeded_empty_producer_roles: set[str] = set() + if producer_roles_with_tasks is not None: + for _candidate in filtered_graph.producer_roles(): + if _candidate in producer_roles_with_tasks: + continue + if filtered_graph.is_dual_role(_candidate): + continue + _pre_seeded_empty_producer_roles.add(_candidate) + agent_prompts: dict[AgentRole, str] = {} for role in roles: prompt = _build_agent_prompt( @@ -15000,6 +15115,7 @@ def _run_concurrent_phase( network_mode=gateway_mode, mode=pipeline.mode, pr_number=getattr(pipeline, "pr_number", None), + is_pre_seeded_empty_producer=role.value in _pre_seeded_empty_producer_roles, ) agent_prompts[role] = prompt diff --git a/orchestrator/tests/test_auto_ack_pure_producers.py b/orchestrator/tests/test_auto_ack_pure_producers.py index b2c518e673..61299f4c95 100644 --- a/orchestrator/tests/test_auto_ack_pure_producers.py +++ b/orchestrator/tests/test_auto_ack_pure_producers.py @@ -126,17 +126,44 @@ def test_real_proposal_supersedes_seeded_acks(self, matrix): # is_fully_acked falls back to False because seeded ACKs are at v1. assert matrix.is_fully_acked("coder") is False - def test_seed_called_twice_is_idempotent_in_effect(self, matrix): + def test_seed_called_twice_keeps_consensus_reachable_but_bumps_version(self, matrix): """Calling the seeder twice with the same task set bumps the - proposal version but leaves consensus reachable — the second - call records ACKs at the new (v2) version. Idempotent in the - sense that the post-state is still fully-ACKed.""" + proposal version each time and re-records ACKs at the new + version. + + The seeder is NOT idempotent on the matrix — each call advances + ``proposal_version`` (v1 → v2 → …) and records fresh ACKs at + that version. The post-state is still fully-ACKed (consensus + reachable), but the version inflation matters: if a retry path + ever calls the seeder while a real propose / ACK is in flight, + seeded-at-v=N ACKs would invalidate any earlier verdicts at + v 0. + assert matrix.get_proposal_version("coder") == 1 + assert matrix.get_proposal_version("documenter") == documenter_v + assert matrix.get_proposal_version("tester") == tester_v + for producer in ("coder", "documenter", "tester"): + assert matrix.get_proposal_version(producer) > 0 + + +class TestProducerRolesWithTasksDerivation: + """Tests the contract → ``producer_roles_with_tasks`` derivation + done by ``_run_concurrent_phase`` (#2581). The derivation drives + both the seed (which producers to auto-ACK) and the prompt-level + pre-seeded flag (which producers should skip propose). + + These tests stub out the contract loader rather than executing the + full ``_run_concurrent_phase`` (which spins up containers etc.) — + the goal is to lock the derivation contract: ``Task.role or "coder"`` + is the canonical mapping, and load failures surface narrowly. + """ + + def test_derivation_uses_coder_default_for_taskless_role(self): + """A task with ``role=None`` is implicitly a coder task per the + contract schema's execution-time default; the derivation must + treat it as ``coder``.""" + + class _Task: + def __init__(self, role: str | None) -> None: + self.role = role + + tasks = [_Task(role=None), _Task(role="tester"), _Task(role="documenter")] + # Mirrors the in-function expression in _run_concurrent_phase. + derived = {(t.role or "coder") for t in tasks} + assert derived == {"coder", "tester", "documenter"} + + def test_seed_skipped_when_load_raises_narrow_exception(self, matrix): + """When the contract loader raises a narrow recoverable error + (ContractNotFoundError / ContractValidationError / OSError), + ``_run_concurrent_phase`` sets ``producer_roles_with_tasks`` + back to ``None`` and the seed is skipped. The matrix is then + unchanged from registration (proposal_version 0 for every + producer).""" + # Direct simulation: without calling the seeder, the matrix + # stays at v=0 for every producer. + assert matrix.get_proposal_version("coder") == 0 + assert matrix.get_proposal_version("documenter") == 0 + assert matrix.get_proposal_version("tester") == 0 + + def test_seed_skipped_when_slice_id_not_in_contract(self, matrix): + """When the slice id does not match any slice in the loaded + contract, ``_run_concurrent_phase`` falls back to + ``producer_roles_with_tasks=None`` and the seed is skipped — + producers run unseeded just like CUSTOM-mode pipelines. + + This is the new "available_slice_ids" log path (#2581 review): + a slice-id / contract-branch skew was previously logged at + DEBUG level, hiding the safety-net-off condition. The matrix + end state with the seed skipped is documented here so the + skip path remains observable from tests. + """ + # No seed call → matrix unchanged. + assert matrix.get_proposal_version("coder") == 0 + assert matrix.get_proposal_version("documenter") == 0 + assert matrix.get_proposal_version("tester") == 0 + + +class TestProducerOrientationPreSeededShortcut: + """Tests that the BRC preamble injects the empty-producer shortcut + block when ``is_pre_seeded_empty_producer=True`` (#2581). + + The shortcut tells CODER / DOCUMENTER to skip the propose step + entirely — without this, the agent's real propose at v=2 would + invalidate the seeded v=1 ACKs and re-trigger the deadlock. The + block is the end-to-end wire-up between the matrix-level seed and + the agent's runtime behaviour. + """ + + def test_shortcut_block_appears_for_pre_seeded_coder(self): + """A pre-seeded coder's BRC preamble must include the shortcut + block telling it to skip propose and confirm directly.""" + from routes.pipelines import _build_brc_preamble + + preamble = _build_brc_preamble( + role_value="coder", + phase="implement", + repo="jwbron/egg", + branch="main", + is_pre_seeded_empty_producer=True, + ) + assert "Pre-seeded empty-producer shortcut" in preamble + assert "Do NOT run `egg-orch consensus propose`" in preamble + # The shortcut should explicitly route through CONFIRM. + assert "consensus confirmed" in preamble + + def test_shortcut_block_absent_when_flag_false(self): + """The shortcut block must NOT appear when the agent has tasks + (the normal flow) — otherwise CODER with real tasks would skip + propose and break the slice.""" + from routes.pipelines import _build_brc_preamble + + preamble = _build_brc_preamble( + role_value="coder", + phase="implement", + repo="jwbron/egg", + branch="main", + is_pre_seeded_empty_producer=False, + ) + assert "Pre-seeded empty-producer shortcut" not in preamble + + def test_shortcut_block_appears_for_pre_seeded_documenter(self): + """Documenter pure-producer path is symmetric with coder.""" + from routes.pipelines import _build_brc_preamble + + preamble = _build_brc_preamble( + role_value="documenter", + phase="implement", + repo="jwbron/egg", + branch="main", + is_pre_seeded_empty_producer=True, + ) + assert "Pre-seeded empty-producer shortcut" in preamble From 913f080a0f5ed1f32fdc7a61911fe9b5a4f97ec4 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 11 May 2026 06:38:19 +0000 Subject: [PATCH 3/4] Replace stub tests with real protocol-level tests for #2581 seed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second-pass review of PR #2583: - Extracts ``routes.pipelines._derive_producer_roles_with_tasks`` so the contract-load + slice-lookup + narrow-exception logic can be unit-tested without spinning up a pipeline. The function is called from ``_run_concurrent_phase`` exactly as before; tests patch its module-level ``load_contract`` import. - Adds ``ReviewGraph.empty_pure_producers(producers_with_tasks)`` as the single source of truth for the empty-pure-producer predicate. Both ``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers`` and ``_run_concurrent_phase``'s prompt-flag computation now route through it, so the prompt flag and the matrix seed cannot drift. - Rewrites the three stub tests (each previously asserted only an untouched matrix state) into real ones that exercise the production code paths: * ``TestDeriveProducerRolesWithTasks`` — patches ``load_contract`` and parametrizes over each narrow exception type, the schema-bump propagation path, the slice-id-not-in-contract path, and the happy path. Verifies the WARNING is emitted with ``pipeline_id`` / ``error_type`` / ``available_slice_ids`` in the structured payload. * ``TestEmptyPureProducersPredicate`` — pins down the invariant that the matrix seed and the prompt-flag computation agree on the role set. - Replaces the matrix-only documenter-only "end-to-end" test with ``TestDocumenterOnlySliceEndToEnd``, a real-protocol test that drives ``PeerConsensusTracker`` through ``handle_propose`` / ``handle_ack`` / ``handle_nack`` / ``handle_confirmed`` and exercises ``check_propose_guard`` / ``check_confirm_guard`` / ``_collect_newly_ready_producers``. Covers: seeded CODER confirms via ``handle_confirmed`` after peers propose; confirm rejected with ``global_zero_proposal`` before peers propose; STATUS-nudge wake-up after the last peer's propose; dual-role TESTER NACK breaks the seeded ACKs and rejects confirm. - Widens the shortcut's wait-loop subscriptions to include ``CONSENSUS_ACK`` / ``CONSENSUS_NACK`` so a dual-role-reviewer NACK against the seeded version can wake the agent (the ``_collect_newly_ready_producers`` STATUS nudge no longer fires once ``is_fully_acked`` breaks). - Tightens the orient short-circuit text to defer to the lifecycle shortcut block instead of duplicating it. Net effect: 31 tests in ``test_auto_ack_pure_producers.py``, all passing. The matrix-level scaffolding, the contract-load derivation helper, and the end-to-end protocol flow each have their own real test coverage; no hand-built fixtures that bypass production code paths remain in the file. --- orchestrator/approval_matrix.py | 11 +- orchestrator/review_graph.py | 28 + orchestrator/routes/pipelines.py | 176 +++-- .../tests/test_auto_ack_pure_producers.py | 633 +++++++++++++++--- 4 files changed, 689 insertions(+), 159 deletions(-) diff --git a/orchestrator/approval_matrix.py b/orchestrator/approval_matrix.py index adffd741c8..87c5bb2ae6 100644 --- a/orchestrator/approval_matrix.py +++ b/orchestrator/approval_matrix.py @@ -291,11 +291,12 @@ def seed_auto_ack_for_empty_pure_producers(self, producers_with_tasks: set[str]) useful for logging / tests). """ auto_acked: list[str] = [] - for producer in sorted(self._graph.producer_roles()): - if producer in producers_with_tasks: - continue - if self._graph.is_dual_role(producer): - continue + # ``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) diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index 6880da9dd9..181d75fbdd 100644 --- a/orchestrator/review_graph.py +++ b/orchestrator/review_graph.py @@ -163,6 +163,34 @@ def reviewer_roles(self) -> set[str]: """ 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 { diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 1a5ed98f63..4b00994044 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -10095,6 +10095,84 @@ def _open_context_pr_for_pipeline( return context_branch +def _derive_producer_roles_with_tasks( + pipeline_id: str, + slice_id: str | None, + has_contract: bool, + worktree_repo_path: Path, +) -> set[str] | None: + """Compute which producer roles have tasks in this slice's plan (#2581). + + Drives both the matrix-level auto-ACK seed + (``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers``) and the + prompt-level shortcut flag (``is_pre_seeded_empty_producer``) used + by ``_build_brc_preamble`` / ``_build_producer_orientation``. + + Behavior: + + * Returns ``None`` when ``slice_id`` is ``None`` or the pipeline has + no contract (CUSTOM-mode / BABYSIT / prompt-mode pipelines) — + preserves pre-#2581 unconditional-roster behavior, no seed. + * Otherwise loads the contract, locates the slice, and returns + ``{(task.role or "coder") for task in slice.tasks}``. ``Task.role`` + is ``str | None`` and ``None`` is the execution-time coder default + per the contract schema. + * If the slice id is absent from the loaded contract, logs a WARNING + with ``available_slice_ids`` inlined and returns ``None`` — the + seed is skipped and pure producers in this slice will deadlock if + they have no tasks. Logged loud so operators can spot the safety + net being off. + * Narrowly catches ``ContractNotFoundError`` / + ``ContractValidationError`` / ``OSError`` from the loader — these + are recoverable load-time errors (missing branch checkout, malformed + contract, file IO failure). Logs a WARNING and returns ``None``. + Unknown exceptions (schema bumps, ``AttributeError`` on contract + model changes) propagate so they fail loudly during testing rather + than silently re-introducing the deadlock in production. + + Extracted from ``_run_concurrent_phase`` so this load+derive path can + be unit-tested without spinning up containers — the production + call site is ``_run_concurrent_phase`` and the unit tests in + ``test_auto_ack_pure_producers.py`` patch the ``load_contract`` + import via this module so the catch logic is exercised directly. + """ + if slice_id is None or not has_contract: + return None + + from egg_contracts.loader import ( + ContractNotFoundError, + ContractValidationError, + load_contract, + ) + + try: + _contract = load_contract(pipeline_id, worktree_repo_path) + except (ContractNotFoundError, ContractValidationError, OSError) as exc: + logger.warning( + "Could not derive producer_roles_with_tasks for auto-ACK seeding — " + "pure producers in this slice may deadlock if they have no tasks", + pipeline_id=pipeline_id, + slice_id=slice_id, + error_type=type(exc).__name__, + error=str(exc), + ) + return None + + _slice_obj = next((s for s in _contract.slices if s.id == slice_id), None) + if _slice_obj is None: + logger.warning( + "Slice id not found in contract — auto-ACK seeding off " + "for this run; pure producers in this slice may deadlock " + "if they have no tasks", + pipeline_id=pipeline_id, + slice_id=slice_id, + available_slice_ids=[s.id for s in _contract.slices], + ) + return None + + return {(t.role or "coder") for t in _slice_obj.tasks} + + def _resolve_slice_1_context_branch_from_contract( pipeline_id: str, worktree_repo_path: Path, @@ -11166,11 +11244,23 @@ def _build_brc_preamble( "`global_zero_proposal` (other slice producers haven't " "proposed yet), this is expected. Block on " "`egg-orch message wait-loop --for STATUS --for " - "CONSENSUS_RE_REVIEW --for OVERSEER_ALERT`. On STATUS " - "with metadata `ready_to_confirm: true` (#2531), retry " - "`egg-orch consensus confirmed`. On `CONSENSUS_RE_REVIEW` " - "for your role, re-confirm (do not propose). On " - "`OVERSEER_ALERT`, surface it.\n" + "CONSENSUS_RE_REVIEW --for CONSENSUS_ACK --for " + "CONSENSUS_NACK --for OVERSEER_ALERT`. The " + "`CONSENSUS_ACK` / `CONSENSUS_NACK` subscriptions cover " + "the dual-role-NACK-recovery scenario: if a dual-role " + "reviewer (TESTER) NACKs your seeded ACKs while you " + "wait, the NACK breaks `is_fully_acked` and " + "`_collect_newly_ready_producers` stops emitting the " + "STATUS nudge — without these subscriptions the wait " + "would hang. On STATUS with metadata " + "`ready_to_confirm: true` (#2531), retry " + "`egg-orch consensus confirmed`. On `CONSENSUS_ACK` / " + "`CONSENSUS_NACK` for your role, retry " + "`egg-orch consensus confirmed` so the orchestrator " + "tells you whether you can proceed (success) or you've " + "hit the `producer_not_fully_acked` branch below. " + "On `CONSENSUS_RE_REVIEW` for your role, re-confirm " + "(do not propose). On `OVERSEER_ALERT`, surface it.\n" " - If it returns `status: pending_acks` with " "`producer_not_fully_acked`, a dual-role reviewer (TESTER) " "has NACKed the seeded version because its own work uncovered " @@ -11871,8 +11961,8 @@ def _build_producer_orientation( "or stretch the slice's scope to author code/docs that the " "planner did not assign to you; the pre-seeded path exists " "precisely to let this slice reach consensus without your " - "contribution. Read the lifecycle shortcut block above, then " - "go directly to step 5 (CONFIRM)." + "contribution. Then follow the **Pre-seeded empty-producer " + "shortcut** block above, which replaces steps 2–5 below." ) reviewer_awareness = "" if reviewers: @@ -15024,56 +15114,15 @@ def _run_concurrent_phase( # tasks to (#2581). Used to pre-seed auto-ACKs for pure producers # (e.g. CODER, DOCUMENTER) that the planner didn't include — # otherwise their empty proposal can deadlock BRC consensus when - # reviewers NACK "nothing to review". Only meaningful for - # per-slice runs against a contracted pipeline; CUSTOM-mode, - # BABYSIT, and prompt-mode pipelines fall through to ``None`` + # reviewers NACK "nothing to review". CUSTOM-mode, BABYSIT, and + # prompt-mode pipelines (and contract-load failures) get ``None``, # which preserves the pre-#2581 unconditional-roster behavior. - producer_roles_with_tasks: set[str] | None = None - if slice_id is not None and getattr(pipeline, "has_contract", True): - from egg_contracts.loader import ( - ContractNotFoundError, - ContractValidationError, - ) - from egg_contracts.loader import ( - load_contract as _load_contract_for_seed, - ) - - try: - _contract = _load_contract_for_seed(pipeline.id, worktree_repo_path) - _slice_obj = next((s for s in _contract.slices if s.id == slice_id), None) - if _slice_obj is None: - # The slice id is well-formed but not in this contract — likely - # a contract-on-main vs slice-on-branch skew, or a bad slice id - # passed in. Log loud so operators can spot the safety net - # being off; let agents run unseeded. - logger.warning( - "Slice id not found in contract — auto-ACK seeding off " - "for this run; pure producers in this slice may deadlock " - "if they have no tasks", - pipeline_id=pipeline.id, - slice_id=slice_id, - available_slice_ids=[s.id for s in _contract.slices], - ) - else: - # ``Task.role`` is ``str | None``; ``None`` is the - # execution-time coder default per the contract schema. - producer_roles_with_tasks = {(t.role or "coder") for t in _slice_obj.tasks} - except (ContractNotFoundError, ContractValidationError, OSError) as exc: - # Narrow catch (#2581 review): only swallow load-time errors we - # can reasonably recover from. Unknown exceptions (schema bumps, - # AttributeError on contract model changes) propagate so they're - # caught loudly during testing instead of silently re-introducing - # the deadlock in production. Logged at WARNING — operators need - # to know the safety net is off. - logger.warning( - "Could not derive producer_roles_with_tasks for auto-ACK seeding — " - "pure producers in this slice may deadlock if they have no tasks", - pipeline_id=pipeline.id, - slice_id=slice_id, - error_type=type(exc).__name__, - error=str(exc), - ) - producer_roles_with_tasks = None + producer_roles_with_tasks = _derive_producer_roles_with_tasks( + pipeline.id, + slice_id, + getattr(pipeline, "has_contract", True), + worktree_repo_path, + ) # Resolve base branch for diff commands in agent prompts. _resolved_base_branch = pipeline.base_branch @@ -15088,14 +15137,15 @@ def _run_concurrent_phase( # block telling the agent to skip its propose step entirely — required # for the matrix-level seed to survive end-to-end (the agent's real # propose would bump the version and invalidate the seeded ACKs). - _pre_seeded_empty_producer_roles: set[str] = set() + # Same predicate as ``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers`` + # (both route through ``ReviewGraph.empty_pure_producers``) so the + # prompt flag and the matrix seed cannot drift. if producer_roles_with_tasks is not None: - for _candidate in filtered_graph.producer_roles(): - if _candidate in producer_roles_with_tasks: - continue - if filtered_graph.is_dual_role(_candidate): - continue - _pre_seeded_empty_producer_roles.add(_candidate) + _pre_seeded_empty_producer_roles = filtered_graph.empty_pure_producers( + producer_roles_with_tasks + ) + else: + _pre_seeded_empty_producer_roles = set() agent_prompts: dict[AgentRole, str] = {} for role in roles: diff --git a/orchestrator/tests/test_auto_ack_pure_producers.py b/orchestrator/tests/test_auto_ack_pure_producers.py index 61299f4c95..0be2dc5bab 100644 --- a/orchestrator/tests/test_auto_ack_pure_producers.py +++ b/orchestrator/tests/test_auto_ack_pure_producers.py @@ -190,111 +190,562 @@ def test_tracker_noop_when_all_producers_present(self, implement_graph): assert auto_acked == [] -class TestDocumenterOnlySliceTesterFlow: - """Verifies the documenter-only slice scenario reaches consensus - end-to-end at the matrix level: CODER is pre-seeded, DOCUMENTER - does a normal propose for its tasks, and TESTER (dual-role) does a - no-op propose with ``no_test_changes_needed=true`` whose ACKs from - its critical reviewers leave global consensus reachable. - - This is the scenario the PR review flagged as untested (#2581 - review issue 3). The existing TESTER ``no_test_changes_needed`` - path was added in #2431; this test pins down that it composes - correctly with the auto-ACK seed. +def _build_tracker(graph): + """Build a real ``PeerConsensusTracker`` with every role in the + graph registered. + + Uses RELAXED attestation strictness (the in-test default in the + rest of ``orchestrator/tests/``) so test payloads don't need to + populate every attestation field. The propose/ACK/confirm guards + that gate consensus are unaffected — those run regardless of + attestation strictness. + """ + from peer_consensus import AttestationStrictness + + tracker = PeerConsensusTracker( + pipeline_id="test-2581", + graph=graph, + cooldown_seconds=0, + attestation_strictness=AttestationStrictness.RELAXED, + ) + for role in graph.all_roles(): + tracker.register_agent(role) + return tracker + + +def _propose_payload(summary="proposed work for #2581 end-to-end test"): + return { + "summary": summary, + "artifacts": ["file.py"], + "commit_sha": "deadbeef", + } + + +def _ack_payload(version): + return { + "artifact_references": ["file.py"], + "ack_version": version, + } + + +class TestDocumenterOnlySliceEndToEnd: + """End-to-end documenter-only slice flow through a real + ``PeerConsensusTracker`` — the test the second review's blocking + issue #1 asked for. CODER pre-seeded; DOCUMENTER + TESTER propose + via ``handle_propose``; reviewers ACK via ``handle_ack``; the + seeded CODER confirms via ``handle_confirmed``. + + This goes through: + * ``check_propose_guard`` (the production gate that the seed + bypasses for CODER but DOCUMENTER / TESTER pass through). + * ``check_confirm_guard`` (the global-zero-proposal guard that + previously deadlocked tester-only / documenter-only slices). + * ``_collect_newly_ready_producers`` (the STATUS-nudge sweep + the shortcut depends on — it's called from + ``_handle_propose_inner`` and ``handle_ack``). + * ``handle_confirmed`` for the seeded CODER (the production path + the agent shortcut invokes via ``egg-orch consensus + confirmed``). """ - def test_documenter_only_slice_reaches_global_consensus(self, matrix): - """Documenter-only slice: CODER seeded; DOCUMENTER + TESTER - propose normally; TESTER's critical reviewers ACK its no-op - proposal; consensus is reachable for every role in the graph.""" - # Pre-seed CODER (no coder tasks in this slice). - matrix.seed_auto_ack_for_empty_pure_producers({"documenter"}) + def test_seeded_coder_confirms_via_handle_confirmed_after_peers_propose(self, implement_graph): + """The full happy path: documenter-only slice, every active + role in the graph reaches CONFIRMED through the production + propose / ack / confirm handlers. Demonstrates the shortcut's + runtime behavior end-to-end. + + Before #2581 the seeded CODER would have to call + ``handle_propose`` and bump to v=2, invalidating the seeded + v=1 ACKs and reopening the deadlock. After #2581 the agent + skips propose and goes straight to ``handle_confirmed``; + that's what this test exercises.""" + tracker = _build_tracker(implement_graph) + # Pre-seed CODER (no coder tasks). + seeded = tracker.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert seeded == ["coder"] + # The seed put CODER at v=1 with every critical reviewer ACKed. + assert tracker.matrix.is_fully_acked("coder") is True - # DOCUMENTER does a normal propose for its tasks. - documenter_v = matrix.record_proposal("documenter") - # DOCUMENTER has no critical reviewers — fully-ACKed once any - # proposal version is recorded. - assert matrix.is_fully_acked("documenter") is True + # DOCUMENTER does a normal propose via the production handler. + doc_result = tracker.handle_propose("documenter", _propose_payload()) + assert doc_result["status"] == "proposed" + assert doc_result["version"] == 1 + # No critical reviewers → fully-acked once proposed. + assert tracker.matrix.is_fully_acked("documenter") is True + + # TESTER (dual-role) does its no-op propose. At the matrix level + # this is a normal handle_propose — the no_test_changes_needed + # attestation is validated separately and isn't relevant to the + # consensus-reachability claim under test here. + tester_result = tracker.handle_propose("tester", _propose_payload()) + assert tester_result["status"] == "proposed" + tester_v = tester_result["version"] + + # Every critical reviewer of TESTER ACKs via handle_ack. + for reviewer in implement_graph.critical_reviewers_for("tester"): + ack_result = tracker.handle_ack(reviewer, "tester", _ack_payload(tester_v)) + assert ack_result["status"] == "acked" + assert tracker.matrix.is_fully_acked("tester") is True + + # The seeded CODER now calls handle_confirmed — the exact path + # the agent shortcut invokes. Before #2581 this would either + # never be reached (CODER proposed first and broke the seed) or + # be rejected with global_zero_proposal until every other + # producer proposed. + # + # This is the core claim of the fix end-to-end: a seeded + # empty-pure producer can confirm via the production + # ``handle_confirmed`` path *without* going through propose, + # once peers have proposed. handle_confirmed accepts CODER + # because the seed made it fully-acked at v=1 AND every + # producer in the graph has now proposed (so the global + # zero-proposal guard clears). + coder_confirm = tracker.handle_confirmed("coder") + assert coder_confirm["status"] == "confirmed" + # CODER is in the confirmed set after a successful confirm — + # the matrix-level proof that the seed survived end-to-end. + assert "coder" in tracker._confirmed + + def test_seeded_coder_confirm_rejected_before_peers_propose(self, implement_graph): + """The shortcut's expected-pending-acks path: the seeded CODER + calls ``handle_confirmed`` before any other producer has + proposed. ``check_confirm_guard`` rejects with + ``global_zero_proposal``, exactly what the shortcut text tells + the agent to expect — the agent then blocks on the wait-loop + until DOCUMENTER's propose triggers the STATUS nudge.""" + tracker = _build_tracker(implement_graph) + tracker.seed_auto_ack_for_empty_pure_producers({"documenter"}) + + # CODER tries to confirm without any peer having proposed. + result = tracker.handle_confirmed("coder") + # Production handler returns pending_acks with global_zero_proposal. + assert result["status"] == "pending_acks" + # The shortcut text references this exact field. + assert "zero_proposal_producers" in result + # DOCUMENTER and TESTER are the two producers that haven't + # proposed yet; they're surfaced in the response so the agent + # could log them. + assert set(result["zero_proposal_producers"]) >= {"documenter", "tester"} + + def test_seeded_coder_wakes_via_status_nudge_after_peer_proposes(self, implement_graph): + """The shortcut's STATUS-wakeup path. After CODER's first + ``handle_confirmed`` call is rejected with + ``global_zero_proposal``, DOCUMENTER's propose (or any other + peer's propose) calls ``_collect_newly_ready_producers`` — + which is what would emit the directed ``STATUS + ready_to_confirm`` nudge that the agent's wait-loop is blocked + on. This test pins that the seeded CODER appears in the + ``newly_ready`` list returned by ``handle_propose`` once the + last peer's propose makes the global-zero-proposal guard clear. + + ``newly_ready`` is the payload the orchestrator uses to drive + ``_emit_ready_to_confirm_nudges`` (the STATUS message that + wakes the agent). If this regresses, the shortcut's wait-loop + hangs. + """ + tracker = _build_tracker(implement_graph) + tracker.seed_auto_ack_for_empty_pure_producers({"documenter"}) + + # First peer proposes — DOCUMENTER. Global zero-proposal guard + # still blocks (TESTER hasn't proposed yet), so CODER isn't yet + # ready_to_confirm. + doc_result = tracker.handle_propose("documenter", _propose_payload()) + # newly_ready is the source of the directed STATUS nudge. + doc_ready_roles = {item["role"] for item in doc_result["newly_ready"]} + # CODER not yet ready: TESTER hasn't proposed. + assert "coder" not in doc_ready_roles + + # Second peer (TESTER) proposes. Now every producer has + # proposed; CODER's full-ACKed-at-v=1 state means + # check_confirm_guard finally accepts CODER and the sweep + # surfaces it. _emit_ready_to_confirm_nudges in production + # turns this into the STATUS message the agent's wait-loop + # blocks on. + tester_result = tracker.handle_propose("tester", _propose_payload()) + tester_ready_roles = {item["role"] for item in tester_result["newly_ready"]} + # CODER is now in the newly_ready list — the STATUS nudge fires. + assert "coder" in tester_ready_roles, ( + f"CODER should be ready_to_confirm after TESTER proposes; " + f"newly_ready was {tester_result['newly_ready']}" + ) - # TESTER (dual-role) does a no-op propose with - # `no_test_changes_needed=true`. At the matrix level this is a - # normal record_proposal — the attestation flag is validated - # elsewhere (orchestrator.attestation_schemas). - tester_v = matrix.record_proposal("tester") - # Every critical reviewer of TESTER ACKs the no-op proposal. - graph = matrix._graph - for reviewer in graph.critical_reviewers_for("tester"): - matrix.record_ack(reviewer, "tester", version=tester_v) + # And calling handle_confirmed for CODER now succeeds. + confirm_result = tracker.handle_confirmed("coder") + assert confirm_result["status"] in ("confirmed", "partially_confirmed") + + def test_dual_role_tester_nack_breaks_seeded_acks_and_rejects_confirm(self, implement_graph): + """The dual-role recovery scenario the second review's + shortcut docstring describes. After the seed, TESTER's + producer-side work uncovers a need for code that wasn't in the + slice plan. TESTER NACKs CODER at v=1 — the matrix records the + NACK at the seeded version, ``is_fully_acked("coder")`` drops + to False, and a subsequent CODER ``handle_confirmed`` is + rejected with ``producer_not_fully_acked``. The shortcut text + instructs the agent to call + ``mcp__sdlc__register_open_question`` at this point rather than + silently start producing.""" + tracker = _build_tracker(implement_graph) + tracker.seed_auto_ack_for_empty_pure_producers({"documenter"}) + assert tracker.matrix.is_fully_acked("coder") is True - # All three producers are now fully-ACKed at the matrix level. - assert matrix.is_fully_acked("coder") is True - assert matrix.is_fully_acked("documenter") is True - assert matrix.is_fully_acked("tester") is True + # DOCUMENTER + TESTER propose so the global zero-proposal guard + # is otherwise satisfied. + tracker.handle_propose("documenter", _propose_payload()) + tracker.handle_propose("tester", _propose_payload()) + + # TESTER (dual-role) NACKs CODER at the seeded version. + nack_payload = { + "ack_version": 1, + "nack_version": 1, + "artifact_references": ["file.py"], + "reason": ( + "no_test_changes_needed=false would be required — slice plan missing coder task" + ), + } + tracker.handle_nack("tester", "coder", nack_payload) + assert tracker.matrix.is_fully_acked("coder") is False + + # CODER's confirm is now rejected — the shortcut's "if it + # returns pending_acks with producer_not_fully_acked" branch. + result = tracker.handle_confirmed("coder") + assert result["status"] == "pending_acks" + assert "producer_not_fully_acked" in result["message"].lower() or ( + "fully ack" in result["message"].lower() + ) - # Global zero-proposal guard would clear: every producer has - # proposal_version > 0. - assert matrix.get_proposal_version("coder") == 1 - assert matrix.get_proposal_version("documenter") == documenter_v - assert matrix.get_proposal_version("tester") == tester_v - for producer in ("coder", "documenter", "tester"): - assert matrix.get_proposal_version(producer) > 0 - - -class TestProducerRolesWithTasksDerivation: - """Tests the contract → ``producer_roles_with_tasks`` derivation - done by ``_run_concurrent_phase`` (#2581). The derivation drives - both the seed (which producers to auto-ACK) and the prompt-level - pre-seeded flag (which producers should skip propose). - - These tests stub out the contract loader rather than executing the - full ``_run_concurrent_phase`` (which spins up containers etc.) — - the goal is to lock the derivation contract: ``Task.role or "coder"`` - is the canonical mapping, and load failures surface narrowly. + +class TestDeriveProducerRolesWithTasks: + """Exercises ``routes.pipelines._derive_producer_roles_with_tasks`` + end-to-end via the module's ``load_contract`` import (#2581). + + The previous test pass-through asserted only that an untouched + matrix had ``proposal_version == 0`` — the review (issue #1, second + pass) flagged that those tests never exercised the catch logic or + the slice-id-not-found branch. These tests patch the contract + loader directly so the production helper executes its real branches. """ - def test_derivation_uses_coder_default_for_taskless_role(self): - """A task with ``role=None`` is implicitly a coder task per the - contract schema's execution-time default; the derivation must - treat it as ``coder``.""" + def _patch_loader(self, monkeypatch, loader_fn): + """Patch the module-level ``load_contract`` import seen by + ``_derive_producer_roles_with_tasks`` so we exercise its real + try/except and slice-lookup logic without spinning up a + contract on disk. + + ``_derive_producer_roles_with_tasks`` performs an + ``from egg_contracts.loader import load_contract`` *inside* + the function. Patching the symbol on ``egg_contracts.loader`` + is the right seam — the import sees the patched symbol when + the helper runs. + """ + import egg_contracts.loader as loader_module + + monkeypatch.setattr(loader_module, "load_contract", loader_fn) + + def test_returns_none_when_slice_id_is_none(self): + """CUSTOM-mode / prompt-mode pipelines have no slice id — the + helper short-circuits with ``None`` and the loader is never + called. This is the pre-#2581 no-seed path.""" + from routes.pipelines import _derive_producer_roles_with_tasks + + # No loader patch — if the helper called load_contract here, the + # real implementation would fail because /tmp isn't a real repo. + result = _derive_producer_roles_with_tasks( + pipeline_id="pid", + slice_id=None, + has_contract=True, + worktree_repo_path=Path("/tmp/nonexistent"), + ) + assert result is None + + def test_returns_none_when_pipeline_has_no_contract(self): + """``has_contract=False`` (BABYSIT / CUSTOM-mode with no + contract draft) short-circuits the same way — the helper never + attempts to load.""" + from routes.pipelines import _derive_producer_roles_with_tasks + + result = _derive_producer_roles_with_tasks( + pipeline_id="pid", + slice_id="slice-1", + has_contract=False, + worktree_repo_path=Path("/tmp/nonexistent"), + ) + assert result is None + + def test_returns_role_set_from_loaded_contract(self, monkeypatch): + """Happy path: loader returns a contract whose slice has tasks + across all three producer roles. The helper returns the set of + roles (with ``Task.role=None`` mapped to ``coder`` per the + contract schema's execution-time default).""" + from routes.pipelines import _derive_producer_roles_with_tasks class _Task: - def __init__(self, role: str | None) -> None: + def __init__(self, role): self.role = role - tasks = [_Task(role=None), _Task(role="tester"), _Task(role="documenter")] - # Mirrors the in-function expression in _run_concurrent_phase. - derived = {(t.role or "coder") for t in tasks} - assert derived == {"coder", "tester", "documenter"} - - def test_seed_skipped_when_load_raises_narrow_exception(self, matrix): - """When the contract loader raises a narrow recoverable error - (ContractNotFoundError / ContractValidationError / OSError), - ``_run_concurrent_phase`` sets ``producer_roles_with_tasks`` - back to ``None`` and the seed is skipped. The matrix is then - unchanged from registration (proposal_version 0 for every - producer).""" - # Direct simulation: without calling the seeder, the matrix - # stays at v=0 for every producer. - assert matrix.get_proposal_version("coder") == 0 - assert matrix.get_proposal_version("documenter") == 0 - assert matrix.get_proposal_version("tester") == 0 + class _Slice: + def __init__(self, sid, tasks): + self.id = sid + self.tasks = tasks + + class _Contract: + def __init__(self, slices): + self.slices = slices - def test_seed_skipped_when_slice_id_not_in_contract(self, matrix): - """When the slice id does not match any slice in the loaded - contract, ``_run_concurrent_phase`` falls back to - ``producer_roles_with_tasks=None`` and the seed is skipped — - producers run unseeded just like CUSTOM-mode pipelines. - - This is the new "available_slice_ids" log path (#2581 review): - a slice-id / contract-branch skew was previously logged at - DEBUG level, hiding the safety-net-off condition. The matrix - end state with the seed skipped is documented here so the - skip path remains observable from tests. + # role=None → coder; explicit roles preserved. + slice_obj = _Slice( + "slice-1", + [_Task(role=None), _Task(role="tester"), _Task(role="documenter")], + ) + contract = _Contract([slice_obj]) + + captured: dict = {} + + def _fake_loader(pid, path): + captured["pid"] = pid + captured["path"] = path + return contract + + self._patch_loader(monkeypatch, _fake_loader) + result = _derive_producer_roles_with_tasks( + pipeline_id="pid-42", + slice_id="slice-1", + has_contract=True, + worktree_repo_path=Path("/tmp/wt"), + ) + assert result == {"coder", "tester", "documenter"} + # The loader was called with the pipeline id and worktree path. + assert captured["pid"] == "pid-42" + assert captured["path"] == Path("/tmp/wt") + + def test_documenter_only_slice_returns_documenter_only(self, monkeypatch): + """A documenter-only slice yields ``{"documenter"}`` — + downstream ``empty_pure_producers`` will pick up CODER as a + pure-producer empty-of-tasks role and the seed will fire.""" + from routes.pipelines import _derive_producer_roles_with_tasks + + class _Task: + def __init__(self, role): + self.role = role + + class _Slice: + def __init__(self, sid, tasks): + self.id = sid + self.tasks = tasks + + class _Contract: + def __init__(self, slices): + self.slices = slices + + contract = _Contract([_Slice("slice-1", [_Task(role="documenter")])]) + self._patch_loader(monkeypatch, lambda pid, path: contract) + + result = _derive_producer_roles_with_tasks( + pipeline_id="pid", + slice_id="slice-1", + has_contract=True, + worktree_repo_path=Path("/tmp/wt"), + ) + assert result == {"documenter"} + + @pytest.mark.parametrize( + "exc_factory", + [ + # The three narrow exception types the helper catches. + # ContractNotFoundError / ContractValidationError have + # their own __init__ signatures — construct them the way + # the real loader does. + lambda: __import__( + "egg_contracts.loader", fromlist=["ContractNotFoundError"] + ).ContractNotFoundError("pid-test", Path("/tmp/wt/.egg-state/contracts")), + lambda: __import__( + "egg_contracts.loader", fromlist=["ContractValidationError"] + ).ContractValidationError("pid-test", ["bad field x"]), + lambda: OSError("io broken"), + ], + ids=["ContractNotFoundError", "ContractValidationError", "OSError"], + ) + def test_returns_none_on_narrow_loader_exception(self, monkeypatch, exc_factory): + """Each of the three narrow exception types the helper + catches: the helper returns ``None`` and emits a structured + WARNING with the error_type / pipeline_id inlined. This is the + safety-net-off condition operators must see in default log + output — DEBUG-level fallbacks would hide it. + + Patches ``routes.pipelines.logger`` directly (the same pattern + ``test_slice_1_context_branch_base_resolution.py`` uses) since + the project's structlog logger writes through a module-level + ``logger`` object that intercept-tests are expected to mock. """ - # No seed call → matrix unchanged. - assert matrix.get_proposal_version("coder") == 0 - assert matrix.get_proposal_version("documenter") == 0 - assert matrix.get_proposal_version("tester") == 0 + from unittest.mock import MagicMock, patch + + from routes.pipelines import _derive_producer_roles_with_tasks + + def _raising_loader(pid, path): + raise exc_factory() + + self._patch_loader(monkeypatch, _raising_loader) + + with patch("routes.pipelines.logger") as mock_logger: + mock_logger.warning = MagicMock() + result = _derive_producer_roles_with_tasks( + pipeline_id="pid-42", + slice_id="slice-1", + has_contract=True, + worktree_repo_path=Path("/tmp/wt"), + ) + assert result is None + # Exactly one WARNING about the failed load. + assert mock_logger.warning.called, ( + "expected logger.warning called for the narrow-exception catch path" + ) + # The message contains the stable "Could not derive ..." prefix + # and the structured fields carry the error_type + pipeline_id + # so operators can grep for the safety-net-off signal. + msg_args = [call.args[0] for call in mock_logger.warning.call_args_list if call.args] + assert any("Could not derive producer_roles_with_tasks" in m for m in msg_args), ( + f"expected stable WARNING message, got: {msg_args}" + ) + # Inspect kwargs of the first matching call — error_type and + # pipeline_id are required for operators to diagnose the skew. + for call in mock_logger.warning.call_args_list: + if call.args and "Could not derive" in call.args[0]: + assert call.kwargs.get("pipeline_id") == "pid-42" + assert call.kwargs.get("slice_id") == "slice-1" + assert call.kwargs.get("error_type") is not None + break + else: + pytest.fail("expected matching 'Could not derive' warning call") + + def test_unknown_exception_propagates(self, monkeypatch): + """The catch is narrow on purpose — schema bumps or + ``AttributeError`` on contract model changes must propagate so + they fail loudly during testing rather than silently + re-introducing the deadlock in production. This is the exact + bare-``except Exception`` antipattern the previous review + called out; this test pins down that it cannot regress.""" + from routes.pipelines import _derive_producer_roles_with_tasks + + def _raising_loader(pid, path): + raise AttributeError("contract schema changed shape") + + self._patch_loader(monkeypatch, _raising_loader) + with pytest.raises(AttributeError, match="contract schema changed"): + _derive_producer_roles_with_tasks( + pipeline_id="pid", + slice_id="slice-1", + has_contract=True, + worktree_repo_path=Path("/tmp/wt"), + ) + + def test_returns_none_when_slice_id_not_in_contract(self, monkeypatch): + """Slice id well-formed but absent from the loaded contract — + likely a contract-on-main vs slice-on-branch skew or a stale + slice id passed in. The helper logs a WARNING with the + contract's available slice ids inlined (so operators can + diagnose the skew from the log line) and returns ``None``. + + Patches ``routes.pipelines.logger`` directly — same pattern as + the narrow-exception test above. + """ + from unittest.mock import MagicMock, patch + + from routes.pipelines import _derive_producer_roles_with_tasks + + class _Slice: + def __init__(self, sid): + self.id = sid + self.tasks: list = [] + + class _Contract: + def __init__(self, slices): + self.slices = slices + + contract = _Contract([_Slice("slice-1"), _Slice("slice-2")]) + self._patch_loader(monkeypatch, lambda pid, path: contract) + + with patch("routes.pipelines.logger") as mock_logger: + mock_logger.warning = MagicMock() + result = _derive_producer_roles_with_tasks( + pipeline_id="pid-99", + slice_id="slice-99", # not in the contract + has_contract=True, + worktree_repo_path=Path("/tmp/wt"), + ) + assert result is None + # The stable WARNING line includes the contract's available + # slice ids in the structured payload — operators can spot + # contract-vs-branch skew without digging through DEBUG logs. + for call in mock_logger.warning.call_args_list: + if call.args and "Slice id not found in contract" in call.args[0]: + assert call.kwargs.get("pipeline_id") == "pid-99" + assert call.kwargs.get("slice_id") == "slice-99" + # available_slice_ids carries the literal slice ids the + # contract knows about — required for the skew + # diagnosis log path. + assert call.kwargs.get("available_slice_ids") == ["slice-1", "slice-2"] + break + else: + pytest.fail( + "expected 'Slice id not found in contract' WARNING; " + f"got: {mock_logger.warning.call_args_list}" + ) + + +class TestEmptyPureProducersPredicate: + """Pins down ``ReviewGraph.empty_pure_producers`` (#2581) — the + single source of truth for the empty-pure-producer predicate used + by both the matrix seed and the prompt-level shortcut flag. + + The previous review (non-blocking #4, second pass) flagged that + the predicate was duplicated in two places: the matrix seeder's + inline loop and ``_run_concurrent_phase``'s + ``_pre_seeded_empty_producer_roles`` computation. If one drifts + (e.g. a future change adds a third skip condition), the prompt + flag and the matrix seed go out of sync. These tests pin the + helper down so both call sites can rely on identical semantics. + """ + + def test_predicate_skips_dual_role_producers(self, implement_graph): + """TESTER (dual-role) must NEVER appear in + ``empty_pure_producers`` regardless of whether it has tasks — + the seed depends on TESTER always running so it can ACK or + NACK CODER.""" + # No tasks for any producer — TESTER would otherwise look + # eligible but is excluded by the dual-role check. + result = implement_graph.empty_pure_producers(producers_with_tasks=set()) + assert "tester" not in result + # Pure producers with no tasks are eligible. + assert "coder" in result + assert "documenter" in result + + def test_predicate_skips_producers_with_tasks(self, implement_graph): + """A producer that has at least one task in the slice is not + empty and must not be auto-ACKed.""" + result = implement_graph.empty_pure_producers(producers_with_tasks={"coder", "documenter"}) + # Both pure producers have tasks → neither is empty-pure. + assert result == set() + + def test_matrix_seed_and_prompt_flag_agree(self, implement_graph): + """End-to-end of non-blocking #4: the matrix seeder and the + prompt-level shortcut flag MUST yield the same set of roles, + because they both route through ``empty_pure_producers``. If + one drifts the seed and prompt go out of sync and the agent + either skips propose without a seeded matrix entry (deadlock) + or proposes against a seeded matrix entry (version bump + deadlock — exactly what #2581 fixed).""" + matrix = ApprovalMatrix(implement_graph) + producers_with_tasks = {"documenter"} + + # Matrix seed says these roles got auto-ACKed: + auto_acked = matrix.seed_auto_ack_for_empty_pure_producers(producers_with_tasks) + + # The prompt-flag computation (mirrored from _run_concurrent_phase) + # uses the same helper: + prompt_flag_roles = implement_graph.empty_pure_producers(producers_with_tasks) + + # The two are the same set. If a future change breaks this + # invariant, the seed and prompt will silently disagree. + assert set(auto_acked) == prompt_flag_roles class TestProducerOrientationPreSeededShortcut: From 284dcf9b9b0e3653640203b1819d16a5bea0b8be Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 11 May 2026 06:57:02 +0000 Subject: [PATCH 4/4] Address third-review suggestions: drop dead branch, use public API, restore type hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three non-blocking suggestions from the third review on #2583: 1. Loose producer_not_fully_acked assertion — the first alternative ("producer_not_fully_acked" in result["message"].lower()) was dead because the guard-name literal lives in guard.details, not in the message. handle_confirmed returns message=guard.reason which for this branch is "Producer {role} cannot confirm: not fully ACKed. ...". Replaced the or-chain with a single "not fully ACKed" substring check and a comment pinning where the message comes from. 2. Private attribute access — replaced "coder" in tracker._confirmed with the public confirmed_roles property (returns frozenset of confirmed roles). Same observation, no private-attribute reach. 3. Type hint loss on _pre_seeded_empty_producer_roles — confirmed the if/else reassign was intentional and added an explicit set[str] declaration above the branches so mypy doesn't have to infer (and a future change to either branch can't silently produce a wider type). --- orchestrator/routes/pipelines.py | 1 + .../tests/test_auto_ack_pure_producers.py | 15 +++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 4b00994044..026b398b46 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -15140,6 +15140,7 @@ def _run_concurrent_phase( # Same predicate as ``ApprovalMatrix.seed_auto_ack_for_empty_pure_producers`` # (both route through ``ReviewGraph.empty_pure_producers``) so the # prompt flag and the matrix seed cannot drift. + _pre_seeded_empty_producer_roles: set[str] if producer_roles_with_tasks is not None: _pre_seeded_empty_producer_roles = filtered_graph.empty_pure_producers( producer_roles_with_tasks diff --git a/orchestrator/tests/test_auto_ack_pure_producers.py b/orchestrator/tests/test_auto_ack_pure_producers.py index 0be2dc5bab..d4d4a0f1e8 100644 --- a/orchestrator/tests/test_auto_ack_pure_producers.py +++ b/orchestrator/tests/test_auto_ack_pure_producers.py @@ -304,7 +304,9 @@ def test_seeded_coder_confirms_via_handle_confirmed_after_peers_propose(self, im assert coder_confirm["status"] == "confirmed" # CODER is in the confirmed set after a successful confirm — # the matrix-level proof that the seed survived end-to-end. - assert "coder" in tracker._confirmed + # Observed through the public ``confirmed_roles`` property + # rather than reaching into ``tracker._confirmed`` directly. + assert "coder" in tracker.confirmed_roles def test_seeded_coder_confirm_rejected_before_peers_propose(self, implement_graph): """The shortcut's expected-pending-acks path: the seeded CODER @@ -407,11 +409,16 @@ def test_dual_role_tester_nack_breaks_seeded_acks_and_rejects_confirm(self, impl # CODER's confirm is now rejected — the shortcut's "if it # returns pending_acks with producer_not_fully_acked" branch. + # ``handle_confirmed`` returns ``message=guard.reason`` (see + # ``peer_consensus.py``), and the producer-not-fully-acked + # reason string is + # ``f"Producer {agent_role} cannot confirm: not fully ACKed. ..."`` + # (``action_guards.py``). The ``producer_not_fully_acked`` + # guard-name literal lives in ``guard.details`` only, not the + # message — assert on the reason substring. result = tracker.handle_confirmed("coder") assert result["status"] == "pending_acks" - assert "producer_not_fully_acked" in result["message"].lower() or ( - "fully ack" in result["message"].lower() - ) + assert "not fully ACKed" in result["message"] class TestDeriveProducerRolesWithTasks: