feat: automatic ACTOR retry on Monitor failure

[azalio]

Summary

• Add monitor_failed() and wave_monitor_failed() to map_orchestrator.py — when Monitor returns valid=false, the orchestrator automatically switches phase back to ACTOR, increments retry count, and persists feedback to a file
• This fixes the deadlock where Monitor detects issues but workflow-gate.py blocks edits during MONITOR phase
• Update map-efficient.md to use the new orchestrator commands instead of pseudocode for retry logic

Design

ACTOR (writes code) → MONITOR (finds bug, valid=false)
                           ↓
                      orchestrator calls monitor_failed()
                           ↓ phase → ACTOR, retry_count++
                      ACTOR (reads monitor_feedback.md, fixes) → MONITOR (re-checks)
                           ↓ valid=true → next phase

Monitor remains strictly read-only. The orchestrator (not Monitor) handles state transitions.

CLI interface

# Sequential mode
python3 .map/scripts/map_orchestrator.py monitor_failed "feedback text"

# Wave/parallel mode
python3 .map/scripts/map_orchestrator.py wave_monitor_failed ST-001 "feedback text"

Test plan

• Unit test: monitor_failed() sets phase to ACTOR, increments retry_count, requeues steps
• Unit test: wave_monitor_failed() resets subtask phase, increments per-subtask retry count
• Unit test: max_retries returns escalation status
• Template sync tests pass (24/24)
• Full test suite passes (818/818)

[Copilot]

Pull request overview

Adds orchestrator-driven retry handling when Monitor returns valid=false, so the workflow can safely transition back into an editing-allowed phase (ACTOR) and persist Monitor feedback for the next retry, avoiding Monitor-phase deadlocks with workflow-gate.py.

Changes:

• Add monitor_failed() and wave_monitor_failed() commands to map_orchestrator.py (both template + .map copy) to increment retry counters, reset phases, and write monitor_feedback*.md.
• Expose the new commands via the orchestrator CLI main() command choices/handlers.
• Update map-efficient.md (both template + .claude copy) to reference the new orchestrator commands instead of pseudocode retry logic.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.

File	Description
src/mapify_cli/templates/map/scripts/map_orchestrator.py	Adds retry handlers + CLI wiring for sequential and wave modes.
.map/scripts/map_orchestrator.py	Mirrors the orchestrator retry handlers + CLI wiring for local workflow execution.
src/mapify_cli/templates/commands/map-efficient.md	Documents using monitor_failed / wave_monitor_failed in the Monitor failure loop.
.claude/commands/map-efficient.md	Mirrors the map-efficient documentation update for Claude command usage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The max-retry check is off by one: with max_retries=5 this escalates on the 6th failure (because it uses >). Also, in the max_retries branch the state’s phase isn’t reset to an editing phase, so workflow-gate can remain stuck in MONITOR even though you’re escalating. Consider using >= and explicitly resetting current_step_id/current_step_phase/pending_steps (or a dedicated escalation phase) before returning.

[Copilot]

Re-queueing from step 2.3 leaves any prior 2.3 entries in completed_steps. This can inflate check_circuit_breaker()’s tool_count (it uses len(completed_steps)) and may trip the breaker early after a few retries. Consider removing 2.3 (and 2.4 if present) from completed_steps when re-queuing, or making completed_steps a set/unique list.

Suggested change

	# When re-queuing from 2.3/2.4, ensure these steps are no longer marked completed.
	# This prevents len(completed_steps)-based logic (e.g., circuit breakers) from
	# counting multiple retries of the same phase as additional completed tools.
	state.completed_steps = [
	step_id for step_id in state.completed_steps
	if step_id not in ("2.3", "2.4")
	]

[Copilot]

CLI parsing for monitor_failed only uses args.task_or_step, so unquoted multi-word feedback will be truncated and the remainder discarded into extra_args. For consistency with wave_monitor_failed, consider joining extra_args into the feedback text as well.

Suggested change

	feedback = args.task_or_step or ""
	parts = []
	if args.task_or_step:
	parts.append(args.task_or_step)
	if getattr(args, "extra_args", None):
	parts.extend(args.extra_args)
	feedback = " ".join(parts) if parts else ""

[Copilot]

This snippet is fenced as bash but mixes non-bash pseudocode (e.g., if monitor_output["valid"] == false: and later if RETRY_STATUS == "max_retries":). If this is intended to be runnable shell, please convert the conditionals to valid bash and show how MONITOR_FEEDBACK_TEXT is sourced/escaped; otherwise consider changing the fence/language label to reflect that it’s pseudocode.

[Copilot]

Same off-by-one issue here: current_retries > max_retries escalates on the (max_retries+1)th failure. Also, the max_retries branch doesn’t reset any phase to an editing phase. Given workflow-gate checks wave subtask_phases against {ACTOR, APPLY, TEST_WRITER} (not step IDs), consider also setting current_step_phase/current_subtask_id (or updating the gate/representation) so retries actually unblock edits.

[Copilot]

This snippet is fenced as bash but mixes non-bash pseudocode (e.g., if monitor_output["valid"] == false: and later if RETRY_STATUS == "max_retries":). If this is intended to be runnable shell, please convert the conditionals to valid bash and show how MONITOR_FEEDBACK_TEXT is sourced/escaped; otherwise consider changing the fence/language label to reflect that it’s pseudocode.

[Copilot]

The max-retry check is off by one: with max_retries=5 this escalates on the 6th failure (because it uses >). Also, in the max_retries branch the state’s phase isn’t reset to an editing phase, so workflow-gate can remain stuck in MONITOR even though you’re escalating. Consider using >= and explicitly resetting current_step_id/current_step_phase/pending_steps (or a dedicated escalation phase) before returning.

[Copilot]

Re-queueing from step 2.3 leaves any prior 2.3 entries in completed_steps. This can inflate check_circuit_breaker()’s tool_count (it uses len(completed_steps)) and may trip the breaker early after a few retries. Consider removing 2.3 (and 2.4 if present) from completed_steps when re-queuing, or making completed_steps a set/unique list.

[Copilot]

CLI parsing for monitor_failed only uses args.task_or_step, so unquoted multi-word feedback will be truncated and the remainder discarded into extra_args. For consistency with wave_monitor_failed, consider joining extra_args into the feedback text as well.

Suggested change

	feedback = args.task_or_step or ""
	parts = []
	if args.task_or_step:
	parts.append(args.task_or_step)
	if args.extra_args:
	parts.extend(args.extra_args)
	feedback = " ".join(parts)

[Copilot]

No unit tests were added for these new retry handlers. The repo already has pytest coverage for other orchestrator commands (see tests/test_map_orchestrator.py), so adding tests for monitor_failed()/wave_monitor_failed() (retry increment, requeue/reset behavior, and max_retries semantics) would help prevent regressions.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

wave_monitor_failed() interpolates subtask_id into the feedback filename. Since subtask_id is user-controlled via the CLI, a value containing path separators / .. could lead to path traversal and writing outside .map/<branch>/. Validate/sanitize subtask_id (e.g., enforce ^ST-\d+$ and/or strip path separators) before using it in a path component.

[Copilot]

In the max_retries early-return path, monitor_failed() saves state but leaves current_step_phase as MONITOR. That reintroduces the original deadlock (workflow-gate blocks edits during MONITOR) at exactly the point where the user needs to decide next actions. Consider still resetting the phase/pending steps to an editing phase (e.g., ACTOR + requeue 2.3/2.4) while returning status="max_retries", or otherwise explicitly unlocking edits as part of the escalation path.

Suggested change

	if escalation is not None:
	if escalation is not None:
	# Even on max-retries escalation, move state back to an editing phase
	# so that workflow-gate does not keep the branch locked in MONITOR.
	state.pending_steps = ["2.3", "2.4"]
	state.current_step_id = "2.3"
	state.current_step_phase = "ACTOR"

[Copilot]

The escalation message says "retry limit reached (N attempts)", but escalation happens when current_retries > max_retries (i.e., after exceeding the limit). Consider adjusting the wording to "retry limit exceeded" (or include the current attempt number) to avoid off-by-one confusion in user-facing output.

Suggested change

	f"Monitor retry limit reached ({state.max_retries} attempts). "
	"Escalate to user."
	f"Monitor retry limit exceeded: "
	f"{state.retry_count} attempts (max {state.max_retries}). "
	"Escalating to user."

[Copilot]

wave_monitor_failed() interpolates subtask_id into the feedback filename. Since subtask_id is user-controlled via the CLI, a value containing path separators / .. could lead to path traversal and writing outside .map/<branch>/. Validate/sanitize subtask_id (e.g., enforce ^ST-\d+$ and/or strip path separators) before using it in a path component.

[Copilot]

wave_monitor_failed() increments retry counts and mutates subtask_phases without checking that wave execution is configured, that subtask_id exists in the current wave, or that the subtask is currently in MONITOR (2.4). As written, calling this command at the wrong time (or with an unknown subtask_id) can silently create/overwrite state and corrupt wave progress. Add guards (e.g., require execution_waves configured, subtask_id in current wave, and current phase == "2.4") and return a structured error otherwise.

Suggested change

	# Guard: wave execution must be configured
	execution_waves = getattr(state, "execution_waves", None)
	if not execution_waves:
	return {
	"status": "error",
	"error": "wave_execution_not_configured",
	"message": (
	"Monitor failure handling requires execution_waves to be "
	"configured in step_state.json."
	),
	"subtask_id": subtask_id,
	}
	# Guard: current wave index must be valid
	current_wave_index = getattr(state, "current_wave_index", None)
	if not isinstance(current_wave_index, int) or not (
	0 <= current_wave_index < len(execution_waves)
	):
	return {
	"status": "error",
	"error": "invalid_wave_state",
	"message": (
	"Current wave index is not valid; cannot process monitor failure "
	"for subtask."
	),
	"subtask_id": subtask_id,
	}
	current_wave = execution_waves[current_wave_index]
	# Derive the list of subtasks for the current wave in a tolerant way
	if isinstance(current_wave, dict):
	wave_subtasks = current_wave.get("subtasks")
	else:
	wave_subtasks = current_wave
	if not wave_subtasks or subtask_id not in wave_subtasks:
	return {
	"status": "error",
	"error": "unknown_subtask_for_current_wave",
	"message": (
	f"Subtask {subtask_id} is not part of the current execution wave; "
	"cannot mark monitor as failed."
	),
	"subtask_id": subtask_id,
	}
	# Guard: subtask must currently be in MONITOR phase (2.4)
	current_phase = state.subtask_phases.get(subtask_id)
	if current_phase != "2.4":
	return {
	"status": "error",
	"error": "invalid_phase_for_monitor_failed",
	"message": (
	f"Cannot mark monitor failed for {subtask_id} because its current "
	f"phase is {current_phase!r}, expected '2.4' (MONITOR)."
	),
	"subtask_id": subtask_id,
	"current_phase": current_phase,
	"expected_phase": "2.4",
	}

[Copilot]

This retry-handling snippet mixes bash-style variable assignments (RETRY_RESULT=$(...)) with Python-style conditionals (if RETRY_STATUS == "max_retries":). Because this prompt is meant to be followed verbatim, the mixed syntax is likely to confuse users/agents and can lead to incorrect command execution. Consider rewriting this block in one consistent pseudo-language (or make it valid bash with [ ... ] conditionals) and keep it consistently fenced.

[Copilot]

This retry-handling block mixes bash-style constructs (VAR=$(...), if echo ...; then) with Python-style conditionals (if RETRY_STATUS == "max_retries":) and pseudo-DSL like AskUserQuestion(...). Because map-efficient.md is an operational playbook prompt, the mixed syntax is likely to confuse users/agents. Consider rewriting the block in a single consistent pseudo-language (or make it valid bash throughout) and fence it accordingly.

[Copilot]

wave_monitor_failed() increments retry counts and mutates subtask_phases without checking that wave execution is configured, that subtask_id exists in the current wave, or that the subtask is currently in MONITOR (2.4). As written, calling this command at the wrong time (or with an unknown subtask_id) can silently create/overwrite state and corrupt wave progress. Add guards (e.g., require execution_waves configured, subtask_id in current wave, and current phase == "2.4") and return a structured error otherwise.

Suggested change

	# Ensure wave-based execution is configured and we have a current wave
	execution_waves = getattr(state, "execution_waves", None)
	current_wave_index = getattr(state, "current_wave_index", None)
	if not execution_waves or current_wave_index is None or not (
	0 <= current_wave_index < len(execution_waves)
	):
	return {
	"status": "error",
	"error": "wave_execution_not_configured",
	"subtask_id": subtask_id,
	"message": (
	"Wave-based execution is not properly configured; "
	"cannot process monitor failure for this subtask."
	),
	}
	# Verify the subtask belongs to the current wave
	current_wave = execution_waves[current_wave_index]
	if isinstance(current_wave, dict):
	current_wave_subtasks = list(current_wave.keys())
	elif isinstance(current_wave, list):
	current_wave_subtasks = current_wave
	else:
	current_wave_subtasks = []
	if subtask_id not in current_wave_subtasks:
	return {
	"status": "error",
	"error": "subtask_not_in_current_wave",
	"subtask_id": subtask_id,
	"message": (
	f"Subtask {subtask_id} is not part of the current wave; "
	"cannot process monitor failure."
	),
	}
	# Verify the subtask is currently in MONITOR phase ("2.4")
	current_phase = state.subtask_phases.get(subtask_id)
	if current_phase != "2.4":
	return {
	"status": "error",
	"error": "invalid_subtask_phase",
	"subtask_id": subtask_id,
	"expected_phase": "2.4",
	"actual_phase": current_phase,
	"message": (
	"wave_monitor_failed can only be called when the subtask "
	"is in MONITOR phase (2.4)."
	),
	}

[Copilot]

In the max_retries early-return path, monitor_failed() saves state but leaves current_step_phase as MONITOR. That reintroduces the original deadlock (workflow-gate blocks edits during MONITOR) at exactly the point where the user needs to decide next actions. Consider still resetting the phase/pending steps to an editing phase (e.g., ACTOR + requeue 2.3/2.4) while returning status="max_retries", or otherwise explicitly unlocking edits as part of the escalation path.

Suggested change

	if escalation is not None:
	if escalation is not None:
	# On max retries, we still want to unlock edits and move the workflow
	# out of MONITOR, otherwise the workflow-gate would deadlock exactly
	# when the user needs to intervene. Reset to an editing phase and
	# persist that state while returning status="max_retries".
	state.pending_steps = ["2.3", "2.4"]
	state.current_step_id = "2.3"
	state.current_step_phase = "ACTOR"

[Copilot]

The escalation message says "retry limit reached (N attempts)", but escalation happens when current_retries > max_retries (i.e., after exceeding the limit). Consider adjusting the wording to "retry limit exceeded" (or include the current attempt number) to avoid off-by-one confusion in user-facing output.

Suggested change

	f"Monitor retry limit reached ({state.max_retries} attempts). "
	"Escalate to user."
	f"Monitor retry limit exceeded after {state.retry_count} attempts "
	f"(max {state.max_retries}). Escalate to user."