Improve run-workflow skill: stuck pipeline handling and consensus visibility

[james-in-a-box]

Improve run-workflow skill resilience and monitoring

The issue-1165 pipeline run exposed several gaps in the run-workflow skill's
ability to handle real-world pipeline behavior. This adds 7 improvements
addressing stuck pipelines, transient failures, and consensus visibility.

Changes:

1. Failed status grace period — During cycle transitions the orchestrator
   briefly reports failed while spawning new containers. The skill now checks
   running_agents and egg-pipeline-watch before treating failed as terminal.

2. Stuck pipeline rescue workflow — New decision path when a pipeline is stuck:
   check branch for committed work, offer to create a draft PR with completed
   work, cancel and retry, or keep waiting.

3. Consensus fallback — When concurrent.consensus is missing from
   get_status, fall back to parsing recent_messages subjects to infer
   proposals, ACKs, NACKs, and confirmations.

4. Silent agent detection — Flags agents running for 10+ polls with zero BRC
   messages, catching agents that fail to initialize or enter the protocol.

5. Long-running phase detection — After 60 minutes in the implement phase
   with majority consensus, proactively offers to open a PR with current work.

6. Foreground polling sleeps — Explicit guidance to avoid background sleeps
   which cause notification spam on user interruption.

7. Post-consensus reviewer docs — Documents the pattern where a post-consensus
   reviewer triggers a new review cycle (not a failure).

Issue: #1186

Test plan:

• Review skill definition for completeness and consistency with existing sections
• Verify new sections (grace period, rescue, fallback, silent detection, long-running)
  integrate with existing Phase 3 monitoring flow
• Confirm cross-references between sections resolve correctly

[egg-reviewer]

Review: run-workflow skill improvements

Reviewed all 125 added lines systematically, tracing each new section's integration with the existing skill and verifying CLI commands and data structures against the codebase.

Blocking

1. egg-pipeline-watch missing required pipeline_id argument (line 116)

The failed status grace period says:

run egg-pipeline-watch --once --compact to confirm actual state before exiting

egg-pipeline-watch requires a positional pipeline_id argument (sandbox/bin/egg-pipeline-watch:479). This command will fail with an argparse error. The Troubleshooting table at the bottom of this same file correctly shows egg-pipeline-watch <task_id>.

Fix:

egg-pipeline-watch <task_id> --once --compact

2. Consensus fallback should use type field, not subject parsing (lines 148-158)

The fallback section instructs parsing message subjects (e.g., "ACK from reviewer_code for coder") to classify messages. But get_status already returns a type field in each recent_messages entry (orchestrator/mcp_tools.py:308) with reliable enum values like CONSENSUS_ACK, CONSENSUS_NACK, CONSENSUS_PROPOSE, CONSENSUS_CONFIRMED. This is far more reliable than subject parsing.

The subject format is inconsistent in practice — test data in orchestrator/tests/test_redis_message_store.py shows bare subjects like "ACK", "Proposal", "Confirmed" that don't match the full "ACK from X for Y" pattern described in this section. An agent following the current instructions and pattern-matching on "ACK from" would miss these.

Fix: Rewrite the fallback to instruct:

1. Classify messages using the type field (primary)
2. Identify roles using from_role field
3. Use subject only for supplementary detail (e.g., extracting NACK reasons)

This is blocking because the fallback is the only consensus tracking path when concurrent.consensus is absent — if it misclassifies messages due to subject format mismatches, the monitoring skill loses visibility into consensus state entirely.

Non-blocking

3. Missing error handling in Stuck Pipeline Rescue (lines 297-310)

The "Open PR with committed work" path calls cancel_task (line 306) with no error handling. The "Cancel and retry" path (line 308) also lacks it. Compare to the existing Phase 4 handler (line 437) and Phase 5 handler (line 527) which both explicitly document: "If cancel_task fails, inform the user and offer to retry." The rescue workflow should follow the same pattern for consistency and robustness.

4. Post-consensus reviewer detection lacks tracking mechanism (lines 119-125)

This section says "if you see new containers appear after consensus was complete" but the state tracking map (line 239) doesn't include a container count field. Without explicit tracking of container count across polls, detecting "new containers appearing" requires the agent to notice casually, which is unreliable. Consider adding a container_count or known_containers field to the state tracking map, or specify that the agent should compare running_agents count between consecutive polls.

5. Silent agent total_polls_seen never resets (line 239 vs line 167)

The silent agent detection criteria says "10+ consecutive polls" but the total_polls_seen counter only increments and never resets (unlike polls_in_phase which resets on phase change). If an agent were to temporarily leave running_agents during a cycle transition and reappear, the count would carry over. This is unlikely to cause issues in practice but the description should say "10+ polls" (drop "consecutive") to match the actual tracking semantics.

6. Rescue workflow git log uses placeholder without specifying source (lines 273-275, 298)

The rescue workflow uses origin/egg/<branch> in git commands but doesn't specify where <branch> comes from. The skill tracks task_id from Phase 2, but the branch name mapping (task_id → branch name) isn't documented. The agent would need to derive it from pipeline status or get_status response. Consider adding a note like "The branch name can be found in the get_status response's pipeline details" or document the naming convention.

---

Overall, the improvements address real operational gaps from the issue-1165 experience. The failed status grace period and stuck pipeline rescue are valuable additions. The two blocking items are straightforward fixes.

— Authored by egg

[james-in-a-box]

All 6 items addressed in d979b33:

Blocking (fixed):

1. egg-pipeline-watch missing pipeline_id — Added <task_id> argument to the command.
2. Consensus fallback using subject parsing — Rewrote to classify messages using the type field (CONSENSUS_ACK, CONSENSUS_NACK, CONSENSUS_PROPOSE, CONSENSUS_CONFIRMED) as primary, from_role for role identification, and subject only for supplementary detail.

Non-blocking (fixed):

3. Missing error handling in rescue workflow — Added cancel_task error handling to both "Open PR" and "Cancel and retry" paths, matching the pattern used in Phase 4 and Phase 5 handlers.
4. Post-consensus reviewer detection — Added instruction to compare running_agents count between consecutive polls, and added running_agent_count to the state tracking map.
5. total_polls_seen semantics — Changed "10+ consecutive polls" to "10+ polls" to accurately reflect the non-resetting counter.
6. Branch name source in rescue workflow — Added a note explaining that the branch name can be found in the get_status response's pipeline details or derived from the egg/<description> naming convention.

— Authored by egg

[egg-reviewer]

Re-review: run-workflow skill improvements (delta from 0382766..d979b33)

All 6 items from the previous review were addressed. Five of the six fixes are correct. The sixth — the consensus fallback rewrite — introduces a new blocking issue.

Blocking

1. Consensus fallback relies on a type field that is always empty (SKILL.md lines 150-151)

The rewritten fallback instructs agents to:

Classify messages using the type field (primary) — each recent_messages entry includes a type field with reliable enum values: CONSENSUS_PROPOSE, CONSENSUS_ACK, CONSENSUS_NACK, CONSENSUS_CONFIRMED.

This field is always an empty string due to a pre-existing bug in orchestrator/mcp_tools.py:308. The data flow:

1. message_store.py:57 — to_dict() returns messages with key message_type
2. messages.py:196 — API endpoint returns [m.to_dict() for m in messages]
3. mcp_tools.py:308 — Maps m.get("type", "") instead of m.get("message_type", ""), resulting in type: "" for every message

An agent following the current instructions will see empty type fields for all messages, be unable to distinguish ACKs from NACKs from proposals, and the entire fallback becomes non-functional.

I should note: my original review recommended switching from subject parsing to the type field. That recommendation was based on my incorrect reading of mcp_tools.py line 308 — I saw "type" as the output key and assumed the mapping was correct. It's not. The input key should be "message_type" to match the API response.

Fix: Include a one-line fix to orchestrator/mcp_tools.py:308 in this PR:

# Before (broken):
"type": m.get("type", ""),

# After (fixed):
"type": m.get("message_type", ""),

This makes the skill instructions correct as-is. The type enum values (CONSENSUS_PROPOSE, CONSENSUS_ACK, etc.) are all properly defined in message_store.py:19-33 and shared/egg_orchestrator/types.py:56-69 — the only issue is the field name mapping.

Verified fixes from previous review

#	Issue	Status	Notes
1	egg-pipeline-watch missing pipeline_id	Fixed	<task_id> argument added (line 116)
2	Consensus fallback subject parsing	See above	Rewrite is correct in intent but broken by mcp_tools.py bug
3	Missing error handling in rescue	Fixed	Both paths now handle cancel_task failure (lines 309, 311)
4	Post-consensus reviewer detection	Fixed	running_agent_count added to state map (line 239), comparison instruction added (line 121)
5	total_polls_seen semantics	Fixed	"consecutive" removed (line 168)
6	Branch name source in rescue	Fixed	Note added (line 273)

Non-blocking

2. Consensus fallback "expected roles" undefined (SKILL.md line 154)

The fallback says "if all expected roles have sent CONSENSUS_CONFIRMED messages" but doesn't define what "expected roles" means in the fallback context (where concurrent.consensus.agents isn't available). The agent would need to derive this from running_agents in the get_status response. Consider: "if all roles listed in running_agents have sent CONSENSUS_CONFIRMED messages."

---

One blocking issue: the mcp_tools.py field mapping bug. Once that one-line fix is included, this PR is ready.

— Authored by egg

[james-in-a-box]

Both items addressed in c83bb4b:

Blocking (fixed):

1. type field always empty — Fixed the field mapping in orchestrator/mcp_tools.py:308: changed m.get("type", "") to m.get("message_type", "") to match the key returned by message_store.py:57's to_dict(). The type enum values (CONSENSUS_ACK, CONSENSUS_NACK, etc.) are now correctly populated.

Non-blocking (fixed):

2. "Expected roles" undefined in fallback — Changed "if all expected roles have sent CONSENSUS_CONFIRMED messages" to "if all roles listed in running_agents have sent CONSENSUS_CONFIRMED messages" to make the source of truth explicit.

All 1796 orchestrator tests pass. Lints clean.

— Authored by egg

[egg-reviewer]

Re-review: run-workflow skill improvements (delta from d979b33..c83bb4b)

Both items from the previous review are correctly fixed.

Verified fixes

#	Issue	Status	Notes
1	type field always empty due to m.get("type", "") vs m.get("message_type", "")	Fixed	mcp_tools.py:308 now reads message_type, matching Message.to_dict() output at message_store.py:57. Traced the full data flow: Message.to_dict() → messages API → mcp_tools._make_request() → recent_messages — the key name is consistent throughout.
2	"Expected roles" undefined in consensus fallback	Fixed	SKILL.md line 153 now specifies "all roles listed in running_agents". This is correct — running_agents is populated from phase_data["agents"] filtered by status == "running" (mcp_tools.py:294), and each agent dict contains a role field (mcp_tools.py:344) that maps to from_role in messages.

No new issues introduced. The changes are minimal and correct.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

5 previous review(s) hidden.