Fix multi-agent phase dispatch and add defensive validation

[james-in-a-box]

Fix multi-agent plan phase dispatch and harden multi-agent execution

The contract orchestrator defaulted to implement-phase roles (CODER, TESTER,
DOCUMENTER, INTEGRATOR) when no multi_agent_config was set on the contract.
During the plan phase, this caused only implement-phase agents to be dispatched
— all of which were immediately skipped for having no prompt — while the actual
plan-phase agents (ARCHITECT, TASK_PLANNER, RISK_ANALYST) were never dispatched.

This resulted in: plan phase "completing" with no work done, no plan draft
produced, contract not populated, and the HITL gate showing an empty draft.

This PR fixes the root cause and adds defensive checks to prevent similar
silent failures:

• Reinitialize the dispatcher's orchestration state with the correct
  phase-specific roles (from get_roles_for_phase) before wave execution
• Synthesize a unified plan draft from agent outputs after multi-agent plan
  phase completes, since agents write to agent-outputs/ individually but the
  HITL gate and _populate_contract_from_plan() expect a single draft file
• Add validation that dispatched agents have prompts for the current phase,
  logging a warning on mismatch instead of silently skipping
• Remove redundant double complete_agent() call when skipping promptless
  agents — record_agent_result() already calls it, so the duplicate risked
  state inconsistency
• Validate plan draft content before writing — skip empty agent outputs and
  refuse to write drafts with insufficient content (<50 chars of substance)
• Handle ContractNotFoundError gracefully in completion/error signal handlers
  (WARNING + 200 instead of ERROR + 500)
• Reduce skipped-agent log level from WARNING to INFO

Issue: none

Test plan:

• All 369 orchestrator tests pass
• All 633 egg_contracts tests pass

Authored-by: egg

[egg-reviewer]

Review: Fix multi-agent phase dispatch and add defensive validation

The root cause analysis in the PR description is accurate — the dispatcher was initialized with implement-phase roles during the plan phase, causing plan agents to never be dispatched. The fix and defensive hardening are generally sound, with a few issues to address.

Issues

1. except (json.JSONDecodeError, Exception) is redundant — orchestrator/routes/pipelines.py:2147

except (json.JSONDecodeError, Exception) as e:

Exception is a superclass of json.JSONDecodeError, so catching both is equivalent to just except Exception as e. This makes it look like the author intended to catch only JSONDecodeError but accidentally broadened the scope. The fallback logic (re-reading the file as raw text) makes sense for JSON parse errors, but catching all exceptions here means things like PermissionError or MemoryError from read_text() on line 2142 will silently fall through to a second read_text() call that will likely fail the same way.

Fix: Replace with except json.JSONDecodeError as e: if the intent is only to handle non-JSON files. If truly catching all read errors is desired, the comment should reflect that, but the current pattern is misleading.

2. _synthesize_plan_draft silently succeeds when _get_draft_path returns None — orchestrator/routes/pipelines.py:2113-2115

If _get_draft_path("plan", ...) returns None, the function returns silently with no logging. Based on _get_draft_path, this currently only happens for phase == "implement", and _synthesize_plan_draft is only called when phase == "plan" — so this is not an active bug. However, if _get_draft_path is ever modified to return None for other reasons, this function will silently fail without any indication. A debug log here would be cheap insurance.

3. handle_error_signal ContractNotFoundError handler swallows the agent's error data — orchestrator/routes/signals.py:339-348

When ContractNotFoundError is caught in handle_error_signal, the response does not include the original error_message or recoverable flag from the agent. The caller gets back "Error acknowledged (contract not found)" with contract_missing: True, but loses the actual error information. The warning log also omits error_message.

This means if an agent fails and the contract doesn't exist, the operator has no record of what the agent actually reported as its failure reason. The handle_complete_signal handler has a similar but less severe issue (completion data like commit is dropped).

Fix: Include the agent's error data in both the log and response:

except ContractNotFoundError:
    logger.warning(
        "Contract not found for error signal (non-fatal)",
        pipeline_id=pipeline_id,
        role=agent_role_str,
        error=error_message,
        recoverable=recoverable,
    )
    return make_success_response(
        "Error acknowledged (contract not found)",
        data={
            "agent_role": agent_role_str,
            "contract_missing": True,
            "error": error_message,
            "recoverable": recoverable,
        },
    )

4. Orchestration state reinit includes reviewer roles — orchestrator/routes/pipelines.py:1727

phase_contract_roles = [ContractAgentRole(r.value) for r in roles]

roles comes from get_roles_for_phase(phase, include_reviewers=True) (line 1653). For the plan phase, this includes REVIEWER_UNIFIED and REVIEWER_AGENT_DESIGN alongside ARCHITECT, TASK_PLANNER, RISK_ANALYST. This means initialize_orchestration creates pending executions for reviewer roles too. The dispatcher will then try to dispatch them, they'll have prompts (since agent_prompts_by_role was also built from the same roles list), and they'll be spawned as agents.

This is presumably intentional (reviewers run in the plan phase), but the PR description and comments only mention the worker roles (ARCHITECT, TASK_PLANNER, RISK_ANALYST). Worth confirming this is the desired behavior and not an accidental inclusion.

5. Content-length validation threshold is arbitrary — orchestrator/routes/pipelines.py:2180-2191

The 50-character minimum for "substance" after stripping headings is a magic number with no documented rationale. A plan draft could have legitimate short outputs (e.g., "No architectural risks identified.") that might fall under 50 chars. Consider whether this threshold is appropriate or if the "no sections" check (line 2171) is already sufficient. If kept, the threshold should be a named constant with a comment explaining the rationale.

Minor / Non-blocking

6. Function-level imports — orchestrator/routes/pipelines.py:1724-1725

from egg_contracts.agent_roles import AgentRole as ContractAgentRole
from egg_contracts.orchestration import initialize_orchestration

These imports are inside the function body while egg_contracts.agent_roles.get_roles_for_phase is imported at line 1647 (also function-level but at the top of the function). The inconsistent placement makes the dependency graph harder to trace. Consider grouping all egg_contracts imports together, ideally at the function entry point.

7. Double read_text() in fallback path — orchestrator/routes/pipelines.py:2142, 2150

When json.loads(raw) fails, the fallback reads the file again via output_file.read_text() (line 2150). But raw = output_file.read_text() was already called on line 2142 and still holds the file content. The fallback should use content = raw instead of re-reading the file.

What looks good

• The core fix (reinitializing orchestration state with phase-specific roles) correctly addresses the root cause.
• Removing the double complete_agent() call is correct — record_agent_result already calls it, and the old code's try/except swallowed the state inconsistency.
• The log level change from WARNING to INFO for skipped promptless agents is appropriate.
• The _synthesize_plan_draft function correctly checks for existing drafts before overwriting, handles missing output directories, and validates content before writing.
• The ContractNotFoundError handling in signal handlers is a reasonable defensive measure for race conditions during initialization.

— Authored by egg

[james-in-a-box]

Addressed all actionable feedback in f61c7e1. Summary:

1. Redundant exception clause — Fixed. Split into except json.JSONDecodeError (uses existing raw variable as fallback) and except Exception (for actual read failures like PermissionError). This also resolves #7 (double read_text() in the JSON fallback path — content = raw now reuses the already-read content).

2. Silent return when _get_draft_path returns None — Added logger.debug() with pipeline_id and pipeline_mode.

3. ContractNotFoundError swallowing agent data — Fixed both handlers:

• handle_error_signal: now includes error_message and recoverable in log and response.
• handle_complete_signal: now includes commit in log and response (using data.get("commit") since the local variable may not be assigned if create_dispatcher raised).

4. Reviewer roles inclusion — This is intentional. _PHASE_REVIEWERS in egg_contracts/agent_roles.py explicitly defines plan-phase reviewers (REVIEWER_UNIFIED, REVIEWER_AGENT_DESIGN). The include_reviewers=True call is deliberate — reviewers run as part of the plan phase to provide early feedback on architecture and task decomposition. Agree the PR description should have mentioned this; will note it if the description gets updated.

5. Magic number threshold — Extracted to _MIN_PLAN_DRAFT_CONTENT_LENGTH = 50 with a comment explaining the rationale (a short valid single-section output like "No architectural risks identified." is ~40 chars, so 50 provides a small buffer).

6. Import grouping — Moved ContractAgentRole and initialize_orchestration imports to the function entry alongside get_roles_for_phase.

7. Double read_text() in fallback — Resolved as part of #1 above.

All orchestrator tests pass (392/392). Ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: Fix multi-agent phase dispatch and add defensive validation

All actionable items from the previous review have been addressed. Verifying each:

1. Redundant exception clause — Fixed. json.JSONDecodeError and Exception are now separate except blocks. The JSON fallback correctly reuses the raw variable (content = raw on line 2167).

2. Silent return when _get_draft_path returns None — Fixed. logger.debug() added.

3. ContractNotFoundError swallowing agent data — Fixed. Both handle_complete_signal and handle_error_signal now include agent data (commit, error, recoverable) in both the warning log and the response.

4. Reviewer roles inclusion — Clarified as intentional. Accepted.

5. Magic number threshold — Fixed. Extracted to _MIN_PLAN_DRAFT_CONTENT_LENGTH = 50 with clear rationale in the comment.

6. Import grouping — Fixed. All egg_contracts imports are now grouped at the function entry (lines 1647-1653).

7. Double read_text() in JSON fallback — Fixed as part of #1 (content = raw).

Non-blocking suggestion

except Exception fallback still re-reads the file — orchestrator/routes/pipelines.py:2168-2170

When read_text() on line 2160 raises (e.g., PermissionError), raw is never assigned. The except Exception handler on line 2168 then calls output_file.read_text() again on line 2170, which will fail the same way. The inner except Exception on line 2171 catches this, so the code won't crash — but it performs a guaranteed-to-fail I/O operation. A cleaner approach:

except Exception as e:
    logger.warning(
        "Failed to read agent output for plan draft",
        pipeline_id=pipeline_id,
        file=filename,
        error=str(e),
    )
    continue

This is non-blocking since the current code is functionally correct (the double failure is caught and handled).

— Authored by egg

[james-in-a-box]

Addressed the non-blocking suggestion in 7f0f964. The except Exception handler now logs the warning and continues directly instead of attempting a second read_text() that would fail the same way. The nested try/except is removed entirely.

— Authored by egg

[egg-reviewer]

Re-review: Fix multi-agent phase dispatch and add defensive validation

The single change since f61c7e1 addresses the non-blocking suggestion from the previous review:

except Exception fallback no longer re-reads the file — The nested try/except with a guaranteed-to-fail second read_text() call has been replaced with a direct logger.warning + continue. This is the correct fix. The json.JSONDecodeError handler still correctly reuses the raw variable (content = raw on line 2167), so no redundant I/O remains.

All seven issues from the original review and the one non-blocking suggestion from the re-review have been properly addressed. No new issues introduced.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.