Fix #2529: runtime escape hatch for impossible tasks

[jwbron]

Summary

• Adds the Impasse typed primitive plus two MCP tools (mcp__sdlc__check_file_restriction, mcp__sdlc__report_impasse) so a producer that discovers mid-execution that its task is structurally impossible can emit a structured signal instead of inventing workarounds (the .github-staging/ deletion-marker pattern from pipeline issue-2474-v2).
• Orchestrator detects the impasse post-phase and auto-delegates to the suggested role on the first attempt (mutating task.role + bumping a new delegation_attempts counter), then re-runs the slice's BRC cycle. Second impasse on the same task — or any non-wrong_role category, no eligible alternative role, or unresolvable task_id — creates a HITL decision via apply_mutation and lets the operator decide between delegate / cancel / manual-resolve.
• Producer prompts get a new section telling agents to call check_file_restriction before exploring a suspect file and emit report_impasse (not a workaround) when blocked.

The runtime escape hatch is independent of #2527 (plan-time prevention) — it's the defense-in-depth path for cases the planner check misses, plan bugs surfaced during execution, external blockers, and #2528 per-repo pattern drift.

Scoping per the latest comment: the orchestrator pre-validation option (option 2 in the original issue) is largely subsumed by #2527 and intentionally not added here. The deeper coupled-test population (~50% of BRC histories per #2530 audit) is tracked for a includes_tests opt-in follow-up — also not in scope.

Notable design choices

• Why a top-level AgentOutput.impasse instead of a key in handoff_data: typed schema, drift-tested round-trip, and the orchestrator can scan a single field across producers without parsing untyped dicts.
• Why Role.SYSTEM owns phases.*.tasks.*.role + delegation_attempts: producers must not be able to rewrite their own task assignment mid-flight. The agent emits a suggestion via Impasse; the orchestrator applies it.
• _DELEGATION_ELIGIBLE_ROLES = {coder, tester, documenter}: cross-phase roles (overseer / autofixer / conflict_resolver / inspector) are not valid auto-delegation targets — auto-delegation rewires a producer task within the implement phase, not across phases.
• HITL options on escalation: delegate (only when suggested_role is in the producer trio) / cancel slice + re-plan / resolve underlying blocker manually / other. Mirrors the existing register_open_question shape, with the orchestrator-side actor distinguishing it from agent-emitted decisions in the audit log.

Test plan

• make test — 16,881 passed, 41 skipped, 0 failed
• make lint — ruff/format/mypy/yaml clean (warnings only on unrelated soft-cap line counts)
• New unit suites:
  • shared/tests/test_impasse_schema.py — Pydantic round-trip + extra-fields rejection
  • sandbox/tests/test_restrictions_handlers.py — both handlers (single/batch/blocked/allowed/no-alternative-role for .github/, env-var fallback, self-delegation rejection at handler boundary, blocked_files type validation, MCP registration sanity)
  • orchestrator/tests/test_impasse_routing.py — collect/route delegate path, second-impasse → HITL, plan_bug / external_blocker / unresolved-task / unknown-role / self-delegation all → escalate, contract-untouched-on-empty-input
• Existing drift tests updated for the two new no-CLI verbs:
  • tests/sandbox/egg_agent_tools/test_full_tool_registry.py (count 29 → 31)
  • tests/sandbox/egg_agent_tools/test_server.py (expected-names set + count comment)
  • tests/tools/test_mcp_cli_drift.py (expected_gaps set)
  • tests/tools/test_rule_doc_drift.py (no-CLI rationale present in handler docstrings — decision-13)
  • docs/reference/agent-tools.md (verb-count + new mcp__sdlc__* table rows)

Out of scope (follow-ups)

• Plan-time validation (sibling Validate task role↔file alignment at plan time, not push time #2527, in progress).
• Per-repo pattern config (sibling Per-repo configuration for test/code file patterns #2528).
• includes_tests opt-in for coupled coder/tester tasks (Audit producer-role pattern boundaries (coder vs tester) #2530 audit population 2).
• Wiring the typed Impasse into a richer operator UI (today the HITL decision body is the surface).

Closes #2529.

[egg-reviewer]

Review

Mostly solid plumbing — typed Impasse schema, drift-tested round trip, role-owner enforcement on phases.*.tasks.*.role, audit-trail via apply_mutation, and good unit coverage of the routing-policy decision tree. However, there is a blocking correctness issue that makes the feature non-functional in its primary code path, plus a few smaller design concerns.

Blocking

1. Producers never see the new "use the runtime escape hatch" instructions

orchestrator/routes/pipelines.py:5708-5739 appends the new producer-facing guidance ("If you discover mid-execution that the task you've been assigned is structurally impossible … STOP. Do not invent a workaround … Instead, use these two MCP tools …") to _build_role_restrictions_section().

That helper has exactly one call site:

$ grep -n "_build_role_restrictions_section" orchestrator/routes/pipelines.py
5645:def _build_role_restrictions_section() -> str:
11226:        lines.append(_build_role_restrictions_section())

Line 11226 is the task_planner branch of _build_agent_prompt (elif role_value == "task_planner":). The function's own docstring (line 5648) states: "This section is injected into the task_planner prompt so that it can assign each task to the correct execution role".

The producer prompts (coder, tester, documenter) are built in different branches that never call this helper. They get _build_file_boundary_section(role_value) (line 10645 / 11338), which describes file boundaries but says nothing about mcp__sdlc__check_file_restriction, mcp__sdlc__report_impasse, or "do not invent a workaround." The producer prompt also never delegates to _build_role_restrictions_section.

The SYSTEM_PROMPT_NUDGE (sandbox/egg_agent_tools/server.py:33-58) tells the agent that mcp__sdlc__* tools exist as a category but does not explain when to call report_impasse versus inventing a workaround.

Net effect: producers — the only roles that emit impasses — never receive the new behavioural instructions. The planner does receive them, but the planner does not emit impasses (it produces a plan; it never has an "assigned task it discovers is impossible mid-execution"). The exact anti-pattern this PR exists to prevent (the .github-staging/ deletion-marker workaround in pipeline issue-2474-v2) was emitted by a producer, not a planner.

This is a "Cross-module silent no-op" per the review rules: each individual file is internally consistent (the schema is valid, the handler validates, the routing policy is correctly tested), but the cross-module wiring dead-ends. The PR description claims "Producer prompts get a new section telling agents to call check_file_restriction before exploring a suspect file and emit report_impasse." — that is the intended design, but it is not what shipped.

Fix: Add the new escape-hatch section to the producer prompts (coder, tester, documenter — see the role_value branches around _build_phase_prompt / _build_agent_prompt), not to _build_role_restrictions_section. The planner branch can keep something briefer if you want planners to know about the post-failure delegation path, but the actionable guidance ("call these two tools, do not commit, exit cleanly") belongs in the producer system prompts.

Non-blocking concerns

2. Retry loop drops a delegation made on the final iteration

orchestrator/routes/pipelines.py:12907-12999:

MAX_IMPASSE_ATTEMPTS = 2

for attempt in range(MAX_IMPASSE_ATTEMPTS):
    last_exit, last_logs = _run_concurrent_phase(...)
    ...
    if not impasses:
        return last_exit, last_logs
    decisions = route_impasses(...)
    all_delegated = decisions and all(d.action == ImpasseAction.DELEGATE for d in decisions)
    if not all_delegated:
        ...
        return last_exit, last_logs
    # all_delegated == True → fall through to next iteration

If iteration 1 (the retry) produces an impasse on a different task whose delegation_attempts == 0, route_impasses will mutate that task's role and bump its counter — and all_delegated will be True. The loop then exits because range(2) is exhausted, and _run_concurrent_phase is never re-invoked with the newly-delegated role. The contract has been mutated, the task's role now points at a producer that never ran, and the slice returns whatever exit code iter-1 produced. The contract's delegation_attempts counter is per-task, so it doesn't catch this; the MAX_IMPASSE_ATTEMPTS=2 cap caps the BRC runs, not the delegations.

Either: (a) only treat a delegation as "spent" when it actually re-ran a BRC cycle, (b) escalate (don't delegate) on the final attempt, or (c) revert a same-attempt delegation that won't get a chance to execute.

3. wrong_role without suggested_role silently escalates

The schema (shared/egg_contracts/impasse.py) and the handler (sandbox/egg_agent_tools/handlers/restrictions.py:223-229) both treat suggested_role as fully optional. A producer can emit category=wrong_role with no suggested_role and the call succeeds. The router then checks if not impasse.suggested_role: return False, "no suggested_role on the impasse" and escalates to HITL.

That's a correct safety floor, but it's also the operator-facing-misconfiguration pattern: the producer's deliberately-set input (category=wrong_role) silently produces "always HITL" instead of an auto-delegation. Either:

• Reject category=wrong_role without suggested_role at the handler boundary (point the agent back at check_file_restriction to populate it), or
• Have the orchestrator-side router opportunistically compute alternative_role from blocked_files + the pattern registry on the way in.

The current behaviour means real-world wrong_role impasses skip auto-delegation any time the agent forgets the suggested-role field.

4. Stale agent-output files between retries — works only by luck

There is no cleanup of .egg-state/agent-outputs/{pipeline_id}-{role}-output.json between iter-0 and iter-1. The design relies on save_agent_output (shared/egg_contracts/orchestrator.py:366-397, mode="w") to clobber the prior file on every producer run. The implement-phase roster is fixed, so all three producers respawn each iteration and each overwrites — fine today.

But the behaviour is fragile:

• If a producer fails to spawn / crashes before writing in iter-1 (transient infra issue), the iter-0 impasse file persists. The post-iter-1 collect_impasses re-reads it and tries to route it; since the counter is now 1, it escalates to HITL with a confusing message about "second impasse on same task" when there was really only one.
• If/when the implement roster ever becomes contract-task-driven (i.e. a producer with no remaining tasks doesn't spawn), the same stale-file re-detection becomes the normal path.

Recommend either deleting the per-pipeline agent-output files between iterations, or keying them by (role, attempt_index) and ignoring older attempts in collect_impasses. At minimum, document the dependency on "every producer always overwrites" in _run_concurrent_phase_with_impasse_retry's docstring so future refactors don't break it silently.

5. Monolithic-implement fallback unprotected

The PR migrates only the slice-loop call site (line 12528) to _run_concurrent_phase_with_impasse_retry. The pre-#2137 monolithic-implement code path (the _run_concurrent_phase call elsewhere in the implement handler) still calls _run_concurrent_phase directly. In the monolithic path, an impasse just becomes a slice failure with no auto-delegation. Likely intentional (legacy path), but worth a single-line comment explaining the omission so this doesn't get flagged again.

6. _record_delegate truncates the agent's reason silently

orchestrator/impasse_routing.py:357: f"Impasse-driven delegation: {impassed_role} → {impasse.suggested_role}. Agent reason: {impasse.reason[:120]}" — the schema already caps reason at 2000 chars and the audit log can certainly hold them; truncating to 120 in the audit reason loses signal when post-mortem-debugging a delegation. Either use the full reason or note explicitly why 120 was chosen.

7. Minor: _find_task defaulting t.role or "coder"

Line 184: matches = [t for t in slice_obj.tasks if (t.role or "coder") == role]. The Task model defaults role=None (shared/egg_contracts/models.py:208-211), so this silently buckets any role-less task into the coder pool. That's consistent with how _build_role_restrictions_section describes the planner contract ("Tasks without a role field default to coder"), but if a wrong_role impasse is emitted by a tester against a None-role task in the same slice, the role-match fallback won't find it (since None or "coder" != "tester"). Consider matching on (t.role or "coder") == role and on t.role is None when the impasse comes from a producer trio member, or — better — require task_id for any impasse the orchestrator routes (the schema already supports it; the handler could enforce it).

Other notes (non-blocking, informational)

• apply_mutation accepts the Decision Pydantic model directly (shared/egg_contracts/validator.py:_set_value appends to the list when idx == len(current)), and validate_assignment=True runs the pydantic validator on the way in. ✅
• phases.{i}.tasks.{j}.role resolves through the Contract.phases backward-compat alias property (models.py:773) onto slices. ✅
• Role.SYSTEM ownership of phases.*.tasks.*.role is correctly added to FIELD_OWNERSHIP so the producer cannot rewrite its own assignment via apply_mutation. ✅
• Test coverage is good for the orchestrator-side policy (delegate / escalate matrix) and the handler boundary; please add at least one end-to-end test that confirms the producer prompt actually contains mcp__sdlc__report_impasse when the prompt is built for a coder/tester/documenter role — that test would have caught issue #1.

The blocking issue is #1; everything else is improvement.

[james-in-a-box]

Thanks for the careful review. All 7 items addressed in commit 696d392.

Per-item disposition

1. Producers never see the new "use the runtime escape hatch" instructions — fixed-in-PR (commit 696d392).
   Extracted the actionable text from _build_role_restrictions_section() into a new _build_impasse_escape_hatch_section() helper and inject it into the coder prompt (early-return branch in _build_agent_prompt) and the tester / documenter prompts (post-phase-restrictions branch). The planner keeps a brief post-failure-delegation summary so it knows the auto-delegation path exists, but doesn't get the call-these-tools directive — planners don't emit impasses. Added end-to-end TestProducerEscapeHatchInPrompts that parametrises over coder/tester/documenter and asserts mcp__sdlc__report_impasse, mcp__sdlc__check_file_restriction, and the "DO NOT invent workarounds" header all appear; also asserts architect / planner stay free of the actionable producer-only directive. The new test would have caught the original miswiring.

2. Retry loop drops a delegation made on the final iteration — fixed-in-PR (commit 696d392).
   Took the reviewer's option (b): route_impasses gains a force_escalate kw, and the slice-loop wrapper sets force_escalate=is_terminal on its last iteration. A delegation that lands there can never re-run a BRC cycle, so we escalate to HITL instead of silently mutating the contract. New TestForceEscalate covers both the terminal-iteration escalation (contract untouched) and the non-terminal default-delegate path.

3. wrong_role without suggested_role silently escalates — fixed-in-PR (commit 696d392).
   Reject at the handler boundary with a message pointing the agent back at check_file_restriction to populate the field, so the fix lands in the same iteration rather than degrading to HITL.

4. Stale agent-output files between retries — works only by luck — fixed-in-PR (commit 696d392).
   Took the cleanup-between-iterations path: after every successful delegation the slice-loop wrapper now clears the impasse field from each producer's per-pipeline agent-output file via load_agent_output → drop key → save_agent_output. A producer that crashes pre-handoff in iter-N+1 can no longer resurrect iter-N's impasse and trigger a spurious "second impasse on same task" HITL.

5. Monolithic-implement fallback unprotected — fixed-in-PR (commit 696d392).
   Added a comment at the second _run_concurrent_phase call site explaining that impasse delegation is intentionally scoped to the slice loop because it rewires a task between producer roles within a slice; pipelines that don't use the slice loop are legacy / single-PR-shape and an impasse there surfaces via the existing phase-failure HITL path.

6. _record_delegate truncates the agent's reason silently — fixed-in-PR (commit 696d392).
   Dropped the [:120] truncation. The Impasse schema caps reason at 2000 chars and the audit log can hold the full payload; preserve it intact for post-mortem debugging. New test_full_reason_recorded_in_audit_log exercises a >200-char reason and asserts the audit-log entry contains it verbatim.

7. _find_task defaulting t.role or "coder" — fixed-in-PR (commit 696d392).
   Took the reviewer's "better" alternative: require task_id at the mcp__sdlc__report_impasse handler for category="wrong_role". The router's role-match fallback is fragile when a slice has multiple tasks per role or role-less tasks; explicit task_id eliminates guesswork on the auto-delegation path. Other categories (plan_bug / external_blocker / unknown) keep task_id optional since they always escalate and tolerate task-level ambiguity.

— Authored by egg

[egg-reviewer]

Re-review (commit 696d392)

All seven items from the prior review have been addressed correctly. I traced each fix to the new code, ran the new tests locally (all pass), and spot-checked the helpers behave as advertised. Approving.

Verified fixes

1. Producer escape hatch in prompts — _build_impasse_escape_hatch_section is injected into the coder prompt at orchestrator/routes/pipelines.py:10694-10695 (early-return branch) and the tester/documenter prompts at orchestrator/routes/pipelines.py:11396-11397 (post-boundary branch). The planner-side text in _build_role_restrictions_section is now a brief summary that omits the actionable directive. TestProducerEscapeHatchInPrompts parametrises over coder/tester/documenter and asserts both tool names + the "DO NOT invent workarounds" header appear; it also asserts architect/planner stay clean of the producer-only directive. This test would have caught the original miswiring.

2. Terminal-iteration silent dangle — _is_eligible_delegation and route_impasses take a keyword-only force_escalate (orchestrator/impasse_routing.py:194-212, 316-340); the early-return short-circuits before the WRONG_ROLE check, so no path silently delegates on the terminal iteration. The slice loop sets force_escalate=is_terminal at orchestrator/routes/pipelines.py:13074. TestForceEscalate covers both the terminal escalate (contract untouched) and non-terminal default-delegate paths.

3. wrong_role without suggested_role — handler now raises HandlerError at sandbox/egg_agent_tools/handlers/restrictions.py:227-234 with a message pointing the agent at check_file_restriction. Covered by test_wrong_role_without_suggested_role_rejected.

4. Stale agent-output files — _clear_stale_impasses at orchestrator/routes/pipelines.py:12968-13022 reads each producer's per-pipeline output, drops the impasse field, and writes the cleaned dict back. It runs only after a successful all-DELEGATE iteration (13121), so it's a no-op on terminal/escalate paths. I smoke-tested the load/clean/save cycle: load_agent_output returns {} when the file doesn't exist (the if not isinstance(existing, dict) or "impasse" not in existing: continue correctly handles this), and the cleanup leaves handoff_data and other fields intact.

5. Monolithic-implement fallback comment — orchestrator/routes/pipelines.py:17080-17089 now explains that auto-delegation is intentionally scoped to the slice loop (rewires a task between producer roles within a slice).

6. Audit-log truncation removed — _record_delegate at orchestrator/impasse_routing.py:441-444 drops the [:120] slice and includes a comment noting the schema cap of 2000 chars makes truncation unnecessary. test_full_reason_recorded_in_audit_log exercises a >200-char reason and asserts it survives verbatim. The structured-log line at :481 keeps impasse.reason[:200] for log readability — that's fine and intentional (logs vs. audit are different surfaces).

7. task_id required for wrong_role — handler raises at sandbox/egg_agent_tools/handlers/restrictions.py:261-266. The router's role-match fallback in _find_task is now unreachable for the auto-delegateable path. Covered by test_wrong_role_without_task_id_rejected and test_plan_bug_without_task_id_accepted.

Test runs (local)

• pytest orchestrator/tests/test_impasse_routing.py — 16 passed
• pytest sandbox/tests/test_restrictions_handlers.py — 19 passed
• pytest orchestrator/tests/test_pipeline_prompts.py::TestProducerEscapeHatchInPrompts — 5 passed

Non-blocking suggestions (follow-ups)

• Mixed-decision case in iter-0 still wastes delegations. If iter-0 produces both a DELEGATE and an ESCALATE, the wrapper sees all_delegated == False and returns at 13086-13101. The DELEGATE has already mutated task.role + bumped delegation_attempts, but no further BRC cycle runs. Different from the terminal-iteration case (the operator at least gets a HITL surface from the ESCALATE), but the role flip is still wasted. Option (a) from the original review (only treat a delegation as "spent" once it actually re-runs) would close this; option (b)'s force-escalate-on-terminal does not. Worth a follow-up issue, not blocking here — the operator-visible HITL prevents silent dangling.

• Docs drift in docs/reference/agent-tools.md:99. The mcp__sdlc__report_impasse row still describes category, reason, suggested_role, blocked_files, evidence — it does not mention that task_id and suggested_role are now mandatory for category=wrong_role. The producer prompt does say this, so agents see it at runtime, but the reference doc is stale.

• No direct unit test for _clear_stale_impasses. The cleanup is internal to the wrapper; the existing tests don't exercise the multi-iteration path with crashed producers. Adding a test that seeds a stale impasse file, calls the wrapper helper, and asserts the field is gone would lock the fragile-by-design behaviour in place.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review and the approve. Pushed commit 7da68cf addressing the two non-blocking suggestions that fit cleanly inside this PR; the third is filed as #2563 per your explicit "worth a follow-up issue, not blocking here" framing.

Per-item disposition

1. Mixed-decision case in iter-0 still wastes delegations — deferred-to #2563.
   Filing per the reviewer's explicit "worth a follow-up issue, not blocking here" — the operator-visible HITL from the ESCALATE side prevents silent dangling, and the structural fix (option (a): defer the delegation_attempts bump until the slice loop actually re-runs) is meaningfully different in shape from the force_escalate=is_terminal change that landed in this PR. Issue Impasse retry wrapper: mixed-decision iter-0 still wastes a DELEGATE role flip #2563 captures the concrete shape, the three options from the original review, and acceptance criteria for the follow-up.

2. Docs drift in docs/reference/agent-tools.md:99 — fixed-in-PR (commit 7da68cf).
   Updated the mcp__sdlc__report_impasse row to document that task_id and suggested_role are now mandatory for category=wrong_role (the handler raises HandlerError when either is missing), with the explicit reason: the auto-delegation path needs both to rewire task.role unambiguously, since the role-match fallback in _find_task is fragile when a slice has multiple tasks per role or role-less tasks. Other categories (plan_bug / external_blocker / unknown) keep both fields optional since they always escalate.

3. No direct unit test for _clear_stale_impasses — fixed-in-PR (commit 7da68cf).
   Extracted the closure inside _run_concurrent_phase_with_impasse_retry to a module-level helper _clear_stale_impasses_for_producers(repo_path, pipeline_id, producer_roles, *, cleanup_reason) so it can be tested without mocking the BRC cycle. New orchestrator/tests/test_pipeline_impasse_cleanup.py::TestClearStaleImpasses covers five paths:

   • happy path: impasse field gone, handoff_data + role survive verbatim
   • no-op when the file has no impasse key (file bytes unchanged)
   • no-op when no output file exists (producer crashed pre-handoff in iter-N — the original failure mode the cleanup defends against)
   • multi-producer cleanup in one pass, with a producer that wrote a clean output unaffected
   • per-pipeline scoping — a concurrent pipeline's stale impasse on the same role is not cleared

   Closure behaviour is byte-identical to the prior commit; this is a refactor + test addition, not a behaviour change. All five new tests + the existing 16 in test_impasse_routing.py + 360+ in test_pipeline_prompts.py pass locally.

— Authored by egg

[egg-reviewer]

Re-review (commit 7da68cf, delta from 696d392)

The new commit addresses both non-blocking suggestions from the prior approve and correctly defers the third per the reviewer's explicit framing. Approving.

Verified fixes

1. Docs drift in docs/reference/agent-tools.md:99 — the mcp__sdlc__report_impasse row now states that task_id and suggested_role are mandatory for category=wrong_role (raises HandlerError otherwise) and that other categories keep both fields optional. The doc text matches the handler's actual validation order at sandbox/egg_agent_tools/handlers/restrictions.py:219-234, 261-266 — no drift between row and code.

2. _clear_stale_impasses_for_producers is now testable + covered. The closure was extracted to a module-level helper at orchestrator/routes/pipelines.py:12887-12955 with the call site at :13128 updated to pass Path(worktree_repo_path) explicitly. I diffed the body against the prior closure: byte-identical except for the dropped closure capture (the helper now takes repo_path: Path as its first arg instead of closing over worktree_repo_path) and the explicit cleanup_reason: str keyword-only kwarg. The error-handling shape (debug log on load_agent_output failure → continue; warning log on save_agent_output failure → continue; info log on success) is preserved.

   orchestrator/tests/test_pipeline_impasse_cleanup.py adds five tests against the extracted helper:

   • test_drops_impasse_field_preserves_others — happy path; impasse removed, role and handoff_data survive verbatim.
   • test_no_op_when_no_impasse_field — file is byte-identical on disk after the call (the "impasse" not in existing: continue short-circuit really does skip the rewrite).
   • test_no_op_when_output_file_missing — no file is materialised when the producer crashed pre-handoff; the helper handles load_agent_output's {} fallback cleanly.
   • test_clears_across_multiple_producers — multi-producer cleanup in one pass; the clean-output-tester is left alone.
   • test_does_not_touch_other_pipelines — per-pipeline scoping; a concurrent pipeline's stale impasse on the same role is preserved.

   I ran the suite locally (pytest orchestrator/tests/test_pipeline_impasse_cleanup.py -v): 5 passed. Also re-ran test_impasse_routing.py (16 passed) and the doc-drift tests (tests/tools/test_rule_doc_drift.py + tests/tools/test_mcp_cli_drift.py, 72 passed) to confirm the refactor + doc edit didn't shake anything loose.

3. Mixed-decision iter-0 silently mutates the contract — correctly deferred to #2563 per the prior reviewer's explicit "Worth a follow-up issue, not blocking here." I read the filed issue: it captures the concrete shape (DELEGATE+ESCALATE in iter-0, role flip dangles until operator resolves the ESCALATE HITL), the three options from the original review, and acceptance criteria that constrain the follow-up. Defer is appropriate because option (b) — the fix that landed in this PR — closes the terminal-iteration silent dangle but is structurally different from option (a) — the deferred fix — and both can land independently.

Minor observations (non-blocking)

• The producer_roles: list parameter type on _clear_stale_impasses_for_producers is unparameterised. The call site passes a list of ContractAgentRoleEnum members and the helper uses role_enum.value only in log statements + role-keyed file paths via load_agent_output/save_agent_output. A list[ContractAgentRole] annotation would make the contract clearer to a future caller, but this is consistent with the prior closure's typing and not a blocker.
• The cleanup helper imports load_agent_output / save_agent_output lazily inside the function body (preserved from the closure). Each call hits the import seam fallback at try: … except ImportError. Hoisting to module-level would shave a microscopic per-call cost, but the closure already had this shape and the test exercises it as-written.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review and the approve. Pushed commit 6354648 addressing both minor observations from the third review.

Per-item disposition

1. producer_roles: list parameter type is unparameterised — fixed-in-PR (commit 6354648).
   Annotated as list[ContractAgentRole] so a future caller sees the expected element type without grepping the call site. Imported under TYPE_CHECKING and quoted with # noqa: UP037 to match the file's existing pattern for ContainerSpawner (lines 413, 684, 6316, 6707, 7021) — the symbol isn't available at runtime, so the standard py314 unquoted form would NameError on inspect.signature() introspection.

2. Lazy load_agent_output / save_agent_output imports — fixed-in-PR (commit 6354648).
   Hoisted to module-level. The seam fallback (from shared.egg_contracts.orchestrator import …) is dead code at runtime — egg_contracts is the actual installed package, not shared.egg_contracts — and orchestrator/impasse_routing.py:50 already imports load_agent_output at module level the same way without a seam. Removing the lazy import + seam shaves the per-call cost the reviewer flagged and brings the helper in line with impasse_routing.py's pattern.

Local re-runs after the change:

• pytest orchestrator/tests/test_pipeline_impasse_cleanup.py — 5 passed
• pytest orchestrator/tests/test_impasse_routing.py — 16 passed
• pytest orchestrator/tests/test_pipeline_prompts.py — 374 passed
• ruff check orchestrator/routes/pipelines.py — clean

— Authored by egg

[egg-reviewer]

Re-review (commit 6354648, delta from 7da68cf)

Both minor observations from the prior review are addressed cleanly. Approving.

Verified fixes

1. producer_roles annotated as list[ContractAgentRole] — orchestrator/routes/pipelines.py:12892 now reads producer_roles: "list[ContractAgentRole]" # noqa: UP037, with ContractAgentRole imported at line 178 under TYPE_CHECKING. The quoted-forward-ref + noqa: UP037 is correct: pipelines.py doesn't use from __future__ import annotations, so an unquoted form would NameError under inspect.signature() introspection because the symbol isn't bound at runtime. Matches the file's existing pattern for ContainerSpawner at lines 413, 684, 6316, 6707, 8956.

2. load_agent_output / save_agent_output hoisted to module level — orchestrator/routes/pipelines.py:172 now imports both at module load. The seam fallback (from shared.egg_contracts.orchestrator …) was indeed dead code at runtime — orchestrator/impasse_routing.py:50 already imports load_agent_output from egg_contracts.orchestrator at module level the same way without a seam, and the orchestrator runs. I confirmed egg_contracts.orchestrator does not import from routes.pipelines (no circular-dependency risk introduced by hoisting).

Behavioural delta

The closure-to-helper extraction in commit 7da68cf preserved byte-identical semantics; this commit only changes annotations and import location. The _clear_stale_impasses_for_producers body and call site (:13128) are unchanged. The five tests in orchestrator/tests/test_pipeline_impasse_cleanup.py import the helper by name and don't depend on the lazy-import shape, so they continue to exercise the same path.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

11 previous review(s) hidden.