Replace implicit consensus READY with agent restart

[james-in-a-box]

Replace implicit consensus READY with agent restart

The consensus wrapper was blindly auto-signaling READY when an agent
exited cleanly without participating in consensus. This masked the
real problem — the agent didn't follow the consensus protocol — and
could lead to premature phase completion with incomplete work.

Key design change: Agents must explicitly participate in consensus.
The wrapper never auto-signals READY on behalf of an agent.

Instead of faking consensus, the wrapper now restarts Claude inside
the same container with a recovery prompt that:

• Explains the agent was restarted because it exited without signaling
• Instructs it to poll for messages, assess state, and either signal
  READY or continue working
• Warns that repeated exits will exhaust restart attempts

Restarts are capped at 2 (configurable via max_restarts param).
After exhausting restarts the wrapper exits with code 1, triggering
the orchestrator's agent failure path (HITL decision with
retry/abort/continue options). No passive waiting — if the agent can't
follow the protocol after 3 attempts, escalate.

Also removes the orchestrator-level implicit READY registration in
the concurrent phase poll loop (pipelines.py). Updates the docs from
PR #1099 to reflect the new behavior.

Issue: #1081

Test plan:

• pytest orchestrator/tests/test_consensus_wrapper.py — 21 tests for wrapper restart behavior
• pytest orchestrator/tests/test_concurrent_integration.py — 22 tests including updated no-implicit-READY tests
• pytest orchestrator/tests/test_coordinator_routes_functional.py — 58 existing tests still pass

Authored-by: egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
orchestrator/routes/pipelines.py	Semantic (superset)	Took main's rename _mark_pipeline_records_cancelled → _mark_pipeline_records_terminated and dynamic error message f"Pipeline {pipeline.status.value}". Main's version is a superset — it handles both cancelled and failed states. Removed PR's redundant local import (already imported at module level).
orchestrator/tests/test_coordinator_routes_functional.py	Additive (stale header)	Removed PR's # Dependency validation tests section header — the tests it referred to were removed in commit a97d400.
orchestrator/tests/test_pipelines_api.py	Additive + Semantic (superset)	Took main's class rename TestCancelPipelineSyncsState → TestTerminatedPipelineSyncsState, added main's explanatory comment on assertion, and included main's new test_failed_pipeline_uses_correct_error_message test.

Verification: All 109 tests pass across the 4 test files from the PR's test plan:

• test_pipelines_api.py — 8 passed
• test_coordinator_routes_functional.py — 58 passed
• test_consensus_wrapper.py — 21 passed
• test_concurrent_integration.py — 22 passed

Lint checks also pass.

No concerns: All conflicts were cases where main generalized the PR's cancel-specific code to handle both cancelled and failed terminal states. The changes are complementary and the merge preserves both contributions.

— Authored by egg

[egg-reviewer]

Review Summary

The core design change — replacing implicit consensus READY with agent restart — is sound. Agents should explicitly participate in consensus, and silently faking READY on their behalf was masking protocol violations. The restart-with-recovery-prompt approach is well-designed: the agent gets context about what happened, instructions on what to do, and a bounded number of chances before escalation. The Dockerfile fix for shared/egg_agent/ is a necessary addition (the orchestrator already imports build_agent_command from it in ~11 places). The _mark_pipeline_records_cancelled helper is a good addition — cancellation should update state records. Tests are comprehensive.

Two issues need attention before merge.

---

Blocking

1. Undocumented removal of spawn-time dependency validation (coordinator.py)

The PR removes the entire get_role_definition() dependency check block from spawn_agent() (lines 227–261 removed) without mentioning it in the PR description. This was explicitly added in PR #1085 as a safety check: spawning reviewer_code would fail if its dependencies (e.g., coder) hadn't completed yet. The corresponding tests (TestSpawnDependencyValidation) are also deleted.

The commit message says "agents should run concurrently and communicate via the messaging system" — this rationale makes sense for the concurrent execution model, but:

1. The removal is scope creep — the PR title and description focus on consensus restart behavior, not dependency validation changes. This should be a separate PR or at minimum documented in the description.
2. It creates an enforcement gap: shared/egg_contracts/orchestration.py still defines can_agent_run() which checks dependencies, and agent_roles.py still declares dependencies for tester→coder, documenter→coder, integrator→[coder, tester], etc. The coordinator endpoint no longer enforces these, but the data model still declares them. A coordinator agent (which is just an LLM following a prompt) could now spawn agents out of order with no guardrail. The orchestration layer's can_agent_run() operates at a different level and is not called from the spawn endpoint.
3. If the intent is to allow concurrent spawning, the dependency definitions should also be updated (or documented as "soft dependencies for ordering hints, not hard constraints"), so the codebase is internally consistent.

Request: Either restore the dependency check with a bypass flag for concurrent mode, or split this into a separate PR with proper justification and update the dependency declarations to match the new behavior.

---

Non-blocking

2. Stale {role} placeholder in _RECOVERY_PROMPT docstring

Line 22: # Placeholders: {restart_number}, {max_restarts}, {role} — but {role} does not appear anywhere in the _RECOVERY_PROMPT string. This is misleading. Remove {role} from the comment, or add the agent's role to the recovery prompt (which would actually be useful context for the restarted agent — it doesn't currently know what role it is).

3. Wrapper doesn't check if agent already signaled READY before restarting

After the initial run, the wrapper checks whether consensus is complete but not whether this agent already signaled READY. If the agent signaled READY but then exited (e.g., context exhaustion), the wrapper will restart it unnecessarily. The restarted agent will poll, discover it already signaled READY, and just stay alive — so this isn't incorrect, just wasteful. Consider adding an egg-orch signal readiness --json check before restarting to skip the restart if the agent already signaled READY.

4. _mark_pipeline_records_cancelled error leaves stale response data

If _mark_pipeline_records_cancelled (line 699) throws, the pipeline variable isn't reassigned, and the response at line 711 returns the pre-cleanup pipeline object (still showing running containers/agents). The state is eventually consistent on next load, but the immediate API response is misleading. Consider returning the pipeline from store.load_pipeline() in the error path, or documenting this as expected behavior.

5. Coordinator spawn comment is stale

Line 305-307 in coordinator.py: the comment still says "keeps the container alive polling for consensus if Claude exits". This no longer describes the behavior — the wrapper now restarts Claude, not polls. Update to match the new semantics.

— Authored by egg

[jwbron]

@james-in-a-box pull main into this branch

[egg-reviewer]

Re-Review (post-merge with main)

The only change since the last review is commit d9a4966 — a merge with origin/main resolving conflicts in pipelines.py, test_coordinator_routes_functional.py, and test_pipelines_api.py.

Merge conflict resolution: Clean

The merge correctly:

• Took main's rename _mark_pipeline_records_cancelled → _mark_pipeline_records_terminated and dynamic error message f"Pipeline {pipeline.status.value}"
• Moved AgentExecutionStatus and ContainerStatus imports to module level (removing the redundant local import that was inside the old function)
• Integrated main's new test_failed_pipeline_uses_correct_error_message test and class rename TestTerminatedPipelineSyncsState
• Removed the stale # Dependency validation tests section header from test_coordinator_routes_functional.py

No issues introduced by the merge itself.

---

Previous blocking issue: Not addressed

Blocking #1 (Dependency validation removal) remains open. The PR still removes the spawn-time dependency check from coordinator.py without any of the requested remediation:

1. The PR description still doesn't mention it. The description says "Replace implicit consensus READY with agent restart" — the dependency enforcement removal is an unrelated change bundled silently.
2. Internal inconsistency persists. shared/egg_contracts/agent_roles.py declares 15+ dependency edges (tester→coder, documenter→coder, integrator→[coder, tester], etc.). shared/egg_contracts/orchestration.py exports can_agent_run() and get_runnable_agents() which check these dependencies. Both are tested and exported from __init__.py. But the only enforcement point — the coordinator spawn endpoint — no longer calls them. The declarations and utility functions are dead code from the coordinator's perspective.
3. No bypass flag, no documentation update, no separate PR. The original review offered two acceptable paths: (a) restore the check with a concurrent-mode bypass, or (b) split it out with proper justification and update the dependency declarations. Neither was done.

The concurrent execution rationale is valid — agents need to start in parallel. But removing the enforcement without updating the data model creates a codebase where dependencies=[AgentRole.CODER] on the tester role is a lie. Either these declarations should be removed/marked as advisory, or the enforcement should be conditional on execution mode. Do one or the other, not neither.

---

Previous non-blocking issues: None addressed

All four non-blocking suggestions from the first review remain:

• #2: Stale {role} placeholder in the _RECOVERY_PROMPT comment (line 22 of consensus_wrapper.py). Still says # Placeholders: {restart_number}, {max_restarts}, {role} but {role} is unused in the prompt.
• #3: No READY check before restart. Still just wasteful, not incorrect.
• #4: Stale response data in the _mark_pipeline_records_terminated error path (line 700-705 of pipelines.py). If the function throws, the response returns the pre-cleanup pipeline object. Same issue, just with the renamed function.
• #5: Stale coordinator spawn comment (lines 305-307 of coordinator.py). Still says "keeps the container alive polling for consensus if Claude exits" — should say "restarts the agent".

— Authored by egg

[james-in-a-box]

Feedback Response

Blocking #1 — Dependency validation removal

Disagree. coordinator.py has no diff against main in this PR — the dependency validation removal was merged to main via PR #1100 ("Fix orchestrator startup crash, spawn blocking, and cancel state sync"), not this PR. Running gh pr diff 1101 confirms coordinator.py is not in the changeset. The reviewer's first review (at commit 620a816) predated the merge with main, so the observation was valid at that time, but after the merge (d9a4966), the change is no longer attributable to this PR.

The consistency concern is valid though — agent_roles.py declares dependencies that the coordinator no longer enforces. This should be addressed in a dedicated follow-up PR that either marks them as advisory ordering hints or adds a conditional enforcement path for non-concurrent mode.

Non-blocking #2 — Stale {role} placeholder

Agreed. Removed {role} from the _RECOVERY_PROMPT comment since it's not used in the prompt string.

Non-blocking #3 — READY check before restart

Agreed. Added a pre-restart consensus and readiness check. The wrapper now:

1. Checks if consensus is already complete (exits immediately if so)
2. Checks if this agent already signaled READY via EGG_AGENT_ROLE env var — if so, enters a polling loop instead of restarting

Updated the test_clean_exit_triggers_restart test to use a stateful mock egg-orch that returns is_complete=false initially and true after the restart, so the new check doesn't short-circuit the restart test path.

Non-blocking #4 — Stale response data in error path

Agreed. Added store.load_pipeline() fallback in the exception handler so the API response reflects current state even when _mark_pipeline_records_terminated throws.

Non-blocking #5 — Stale coordinator spawn comment

Agreed. Updated comment from "keeps the container alive polling for consensus" to "restarts the agent with a recovery prompt if Claude exits before the orchestrator confirms consensus".

All 110 tests pass across the 4 test files. Lint is clean.

— Authored by egg

[egg-reviewer]

Re-Review Summary

The new commit addresses all four non-blocking suggestions from the previous review. The dependency validation concern (previous blocking #1) is confirmed resolved — the removal is in main via PR #1100, not in this PR's diff.

However, the new code introduced to address feedback #3 (pre-restart READY check) exposes a pre-existing bug in the consensus wrapper that makes all consensus/readiness detection non-functional.

---

Blocking

1. All consensus checks query the wrong endpoint — detection is non-functional

The wrapper uses egg-orch message status --json to check both consensus completion and per-agent readiness state (both in the new pre-restart check AND the pre-existing post-restart check). But message status returns only message bus statistics:

{"success": true, "data": {"total": 42, "by_type": {"PROGRESS": 15, ...}}}

It does not include consensus data. Consensus state (is_complete, per-agent state) is only available from the pipeline status endpoint (egg-orch pipeline status --json), nested under data.concurrent.consensus.

As a result:

• IS_COMPLETE will always be "False" — the path data.consensus.is_complete doesn't exist in the message status response, so the .get() chain falls through to the default.
• AGENT_STATE will always be "" — data.consensus.agents doesn't exist either.

This affects three code paths in the wrapper:

1. New pre-restart consensus check (lines 83–90): Will never detect consensus is complete → always falls through.
2. New pre-restart READY check (lines 93–117): Will never detect the agent is READY → always falls through to restart. The entire purpose of this code (avoiding wasteful restarts when the agent already signaled READY) is non-functional.
3. Pre-existing post-restart consensus check (lines 131–136): Will never detect consensus completion → wrapper always exhausts max restarts and exits with failure code 1.

The tests mask this because the mock egg-orch returns {"data": {"consensus": {"is_complete": true}}} — a JSON structure the real egg-orch message status never produces. The _make_mock_tools_with_delayed_consensus helper has the same issue: it returns a response structure that doesn't match the real command's output.

Fix: Replace egg-orch message status --json with egg-orch pipeline status --json and update the JSON extraction path from data.consensus.is_complete to data.concurrent.consensus.is_complete (and similarly for per-agent state: data.concurrent.consensus.agents.<role>.state). Update the test mocks to match the real response structure.

The pre-existing post-restart check (point 3) has the same bug and should be fixed in the same pass — the PR is already modifying this code.

---

Non-blocking

2. READY polling loop reuses MAX_RESTARTS as iteration count

In the READY polling loop (lines 106–117), the wait loop is bounded by MAX_RESTARTS:

while [ "$WAIT_COUNT" -lt "$MAX_RESTARTS" ]; do

MAX_RESTARTS semantically means "how many times to restart Claude" (default 2), but here it's repurposed as "how many poll cycles to wait for consensus." With default values (MAX_RESTARTS=2, POLL_INTERVAL=30s), the agent waits at most 60 seconds for consensus. This may be too short if other agents are slow. Consider a separate MAX_READY_POLLS constant or a longer timeout (the agent already signaled READY so there's minimal cost to waiting longer).

3. Test mock code duplication

_make_mock_tools_with_delayed_consensus duplicates the entire claude mock creation from _make_mock_tools. Extract the claude mock creation into a shared helper to reduce maintenance burden.

— Authored by egg

[james-in-a-box]

Blocking #1 — Wrong endpoint for consensus checks

Agreed. Replaced all three occurrences of egg-orch message status --json with egg-orch pipeline status --json and updated the JSON extraction path from data.consensus.* to data.concurrent.consensus.* to match the real API response structure. All test mocks updated accordingly.

Non-blocking #2 — READY polling loop reuses MAX_RESTARTS

Agreed. Added MAX_READY_POLL_CYCLES = 10 as a separate constant. With the default 30s poll interval, this gives 5 minutes for other agents to finish — much more reasonable than the 60 seconds from reusing MAX_RESTARTS=2.

Non-blocking #3 — Test mock code duplication

Agreed. Extracted claude mock creation into a shared _make_mock_claude() helper function. Both _make_mock_tools and _make_mock_tools_with_delayed_consensus now use it, and _make_failing_claude delegates to it with a non-zero exit code.

All 112 tests pass across the 4 test files. Lint and mypy clean.

— Authored by egg

[egg-reviewer]

Re-Review Summary

The new commit (b7776d0) cleanly addresses all three items from the previous review.

Previous Blocking #1 (Wrong endpoint): Fixed

All three consensus check paths now correctly use egg-orch pipeline status --json instead of egg-orch message status --json. The JSON extraction path is updated to data.concurrent.consensus.is_complete (and data.concurrent.consensus.agents.<role>.state for per-agent state), which matches the real API response structure from the get_pipeline_status() endpoint in pipelines.py. Test mocks are updated to return the correct response structure.

Previous Non-blocking #2 (READY polling reuses MAX_RESTARTS): Fixed

Added MAX_READY_POLL_CYCLES = 10 as a separate constant with a clear comment explaining the 5-minute timeout. The READY polling loop uses MAX_READY_POLLS instead of MAX_RESTARTS. The constant is parameterized through build_consensus_wrapped_command(max_ready_polls=...).

Previous Non-blocking #3 (Test mock duplication): Fixed

Extracted _make_mock_claude() as a module-level helper. _make_mock_tools, _make_mock_tools_with_delayed_consensus, and _make_failing_claude all delegate to it. Clean refactor, equivalent behavior.

Previously fixed items verified

• Non-blocking #4 (stale response in error path): Confirmed store.load_pipeline() fallback exists in the _mark_pipeline_records_terminated exception handler.
• Non-blocking #5 (stale coordinator spawn comment): Confirmed updated to "restarts the agent with a recovery prompt if Claude exits before the orchestrator confirms consensus".

---

Non-blocking

1. No behavioral test for the READY polling path

The READY check path (agent already signaled READY → skip restart → poll for consensus) has string-match tests (test_contains_ready_check_before_restart, test_ready_polling_uses_separate_constant, test_default_max_ready_polls) but no behavioral test that actually exercises the path in a subprocess. A test that sets EGG_AGENT_ROLE in the environment and has the mock return per-agent READY state would cover this. Not blocking since the logic is straightforward and the JSON path was validated against the real API structure.

— Authored by egg

[james-in-a-box]

Non-blocking #1 — No behavioral test for READY polling path

Agreed. Added test_ready_agent_skips_restart_and_polls — a behavioral test that runs the wrapper in a subprocess with EGG_AGENT_ROLE=coder set. The mock egg-orch returns per-agent READY state (agents.coder.state = "READY") with is_complete=false initially, then is_complete=true after 3 status calls. The test verifies:

• Claude is called exactly once (no restart)
• Wrapper detects "already signaled READY" and enters the polling loop
• Consensus is eventually detected and wrapper exits cleanly (code 0)
• No "Restarting" messages appear in output

Also added agent_role parameter to _run_wrapper_command and a _make_mock_tools_with_agent_ready_state helper that builds the mock with per-agent state in the response JSON.

All 113 tests pass. Lint clean.

[egg-reviewer]

Re-Review Summary

One new commit since the previous (approving) review: 8acea45 — "Add behavioral test for READY polling path". This addresses the single remaining non-blocking suggestion.

Previous non-blocking #1 (No behavioral test for READY polling path): Fixed

The new test (test_ready_agent_skips_restart_and_polls) correctly exercises the full READY polling path in a subprocess:

• Environment setup: _run_wrapper_command gains an agent_role parameter that sets EGG_AGENT_ROLE in the subprocess environment, correctly popping it when None to prevent leakage from the outer environment.
• Mock structure: _make_mock_tools_with_agent_ready_state returns per-agent READY state in the JSON response using the correct nested path (data.concurrent.consensus.agents.<role>.state), matching the real egg-orch pipeline status response structure. Uses consensus_after counter to gate consensus completion.
• Assertions: Verifies exit code 0, "already signaled READY" message, "Consensus reached" message, single Claude invocation (no restart), and no "Restarting" messages. Complete coverage of the path.
• Timing: With EGG_MESSAGE_POLL_INTERVAL=1 and consensus_after=3, the test completes in ~2-3 seconds well within the 30-second timeout.

No issues found. The JSON string construction with concatenation instead of f-strings is pragmatic given the brace-heavy context of shell-embedded Python.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

14 previous review(s) hidden.