Enforce spawn dependency ordering and fix coordinator bugs

[james-in-a-box]

Enforce agent spawn ordering and fix coordinator bugs

During the issue-1059 pipeline run, reviewers consistently finished
before the primary agents they were meant to review, provide_input
returned 405 on every HITL resolution, and concurrent agents diverged
onto separate branches. These failures stem from missing guardrails in
the coordinator spawn path and a wrong URL in the MCP tool handler.

This PR adds dependency validation and contract checks to the spawn
endpoint (extending the phase-advance contract guard from #1072 to
cover spawns), fixes the provide_input URL, appends consensus
protocol instructions to agent prompts, and switches
ConcurrentPhaseExecutor to a shared pipeline branch.

Issue: #1081

Test plan:

• pytest orchestrator/tests/test_coordinator_routes_functional.py — 75 pass, including 4 new tests for dependency and contract enforcement at spawn time
• pytest orchestrator/tests/test_concurrent_integration.py — 2 new tests for shared branch behavior
• pytest orchestrator/tests/test_coordinator_mcp_functional.py — test_provide_input verifies /resolve suffix

Authored-by: egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[egg-reviewer]

Review: Enforce spawn dependency ordering and fix coordinator bugs

Reviewed all 7 changed files (281 additions, 13 deletions). The changes are well-scoped and address real failures observed in issue-1059. The MCP URL fix, contract enforcement at spawn time, and dependency validation are all correct. Tests are comprehensive.

MCP URL fix (mcp_tools.py)

Correct fix. The decision resolution endpoint is POST /api/v1/pipelines/{id}/decisions/{id}/resolve. The old URL without /resolve maps to a GET-only handler, which explains the 405s seen in production. Test updated to match.

Contract enforcement at spawn (`routes/coordinator.py:212-227)

Correct extension of the existing phase-advance guard. Blocks spawns in IMPLEMENT/PR phases when contract_synced is false. Consistent with the phase-advance contract enforcement from #1072. Default value of contract_synced=True in the Pipeline model means existing tests pass without modification unless explicitly testing this path — which the new tests do correctly.

Dependency validation (routes/coordinator.py:229-259)

The implementation is correct and handles cross-phase dependencies properly. agents_spawned persists across phases in CoordinatorState, so plan-phase agents (TASK_PLANNER, RISK_ANALYST) remain visible during implement phase. This allows reviewer roles with cross-phase dependencies to correctly check against the full spawn history.

The ordering of checks — contract enforcement before dependency validation before phase-role validation — is logical: fail fast on the cheapest checks first.

One note on the exception handler (lines 254-259): except (ValueError, KeyError) silently allows spawn when get_role_definition fails. The debug-level log is appropriate for forward compatibility with roles not yet registered in egg_contracts, but consider whether logger.warning would be more appropriate here since this bypasses a safety check. If a role definition is misconfigured and raises unexpectedly, the dependency check is silently skipped.

Shared branch (concurrent_executor.py:95-100)

The switch from per-role branches (egg/issue-{N}/{role}) to a shared pipeline branch is the correct direction — divergent branches were causing the integration failures described in the issue. The dependency ordering added in this same PR mitigates the primary risk by ensuring agents that write to the same areas run sequentially.

Advisory: Agents that CAN run in parallel on the shared branch (e.g., TESTER+DOCUMENTER in wave 2, REVIEWER_CODE+REVIEWER_CONTRACT in wave 4) may hit non-fast-forward push failures when pushing concurrently. The gateway has no push serialization or retry mechanism. In practice this is low-risk since these role pairs write to disjoint file sets, but it's worth documenting. If an agent's push fails due to a concurrent push from its sibling, the agent has no built-in recovery path.

Consensus protocol prompt (routes/coordinator.py:338-344)

The stay-alive instructions are appended unconditionally to every coordinator-spawned agent. This is fine for orchestrated pipelines where the coordinator is always active, though it's slightly redundant with the concurrent mode section already in CLAUDE.md. The directness of injecting it into the prompt should improve agent compliance, which was the problem.

Test coverage

Good test additions:

• TestSpawnDependencyValidation (2 tests): Correctly verifies blocking when deps are incomplete and allowing when deps are satisfied.
• TestSpawnContractEnforcement (2 tests): Correctly verifies blocking in implement phase without contract and allowing in refine phase.
• TestGetWorktreeBranch (2 tests): Verifies shared branch behavior.
• Existing tests updated to satisfy new dependency checks — this is correct and shows the new validation integrates properly with the existing test suite.

Minor observation on test_get_worktree_branch_fallback: setting pipeline.branch = "" works (empty string is falsy) but None is the more realistic falsy value given the model type str | None. The fallback also produces "egg/issue-999" which is identical to the cleared pipeline branch value, making it harder to confirm the fallback path is actually computing the branch rather than using a cached value. Consider using a distinct issue_number to make the test more discriminating.

Summary

No blocking issues. The fixes address the three failures described in the issue (reviewer ordering, provide_input 405, branch divergence) with correct implementations and good test coverage.

— Authored by egg

[james-in-a-box]

Agreed. Both suggestions addressed in c078126:

1. Logger level: Changed logger.debug → logger.warning in the dependency check exception handler (coordinator.py:256). The reviewer is right that bypassing a safety check warrants warning-level visibility.

2. Fallback test: Updated test_get_worktree_branch_fallback to use pipeline.branch = None (more realistic falsy value matching the str | None type) and a distinct issue_number = 777 so the asserted branch (egg/issue-777) differs from the default pipeline branch, proving the fallback path actually computes the value.

— Authored by egg

[egg-reviewer]

Re-review: c078126 (feedback addressing commit)

Reviewed the delta since 5ec697e. One commit: c078126 Address review feedback: raise log level, improve fallback test.

Changes verified

1. Logger level (coordinator.py:257): logger.debug → logger.warning in the dependency check exception handler. Correct. The surrounding comment also updated to explain why warning level is appropriate. No other safety-check bypass paths remain at debug level — the remaining logger.debug at line 288 (phase-role mapping fallback) is a distinct case where the phase legitimately has no role definitions, not a dependency bypass.

2. Fallback test (test_concurrent_integration.py): pipeline.branch = "" → pipeline.branch = None (matches str | None model type). Added pipeline.issue_number = 777 so the asserted branch (egg/issue-777) is distinct from the helper's default (egg/issue-999), proving the fallback actually computes from issue_number rather than using a cached or coincidental value. Both changes make the test more discriminating.

Verdict

Both suggestions from the previous review are properly addressed. No new issues introduced. The delta is minimal and correct.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.