diff --git a/orchestrator/mcp_tools.py b/orchestrator/mcp_tools.py index ca5b1c7c08..91a111207d 100644 --- a/orchestrator/mcp_tools.py +++ b/orchestrator/mcp_tools.py @@ -707,9 +707,25 @@ def _is_timeout_error(exc: BaseException) -> bool: { "name": "advance_phase", "description": ( - "Advance a pipeline to a target phase. When force=true, stops all " - "running containers before advancing to prevent SIGTERM cascading " - "into the new phase.\n\n" + "Transition a pipeline from its current phase to target_phase. " + "Mutates: marks the current phase_execution COMPLETE with a " + "completed_at timestamp, sets pipeline.current_phase = target_phase, " + "marks the target phase_execution RUNNING with started_at/" + "work_started_at timestamps, sets pipeline.status = RUNNING, bumps " + "pipeline.run_epoch, and launches a fresh _run_pipeline driver " + "thread that will spawn agents for the new phase. When advancing " + "out of plan, automatically runs populate_contract to write the " + "SDLC contract from the plan draft (#1941); failures warn and " + "continue so the advance hammer is not blocked. Preconditions " + "(force=false): target_phase must be a valid transition from the " + "current phase (else 400); the current phase_execution.status must " + "be COMPLETE or PENDING (else 400 — not 409); PHASE_COMPLETE " + "health checks must not return FAIL_PIPELINE (else 409, with " + "health_results in details). When force=true, skips transition " + "validation, the phase-status check, and health-check gating, " + "and first stops any running containers for the pipeline so their " + "SIGTERM does not cascade into the new phase. Response data " + "includes previous_phase and current_phase.\n\n" "Error responses include a machine-readable `reason` code (#1939). " "Note: reason codes are only visible to direct HTTP callers; the " "MCP handler layer does not yet surface them.\n" @@ -738,7 +754,7 @@ def _is_timeout_error(exc: BaseException) -> bool: }, "force": { "type": "boolean", - "description": "Skip validation and force the transition. Also stops running containers before advancing.", + "description": "Skip transition validation, phase-status check, and health-check gating. Also stops running pipeline containers before advancing so their SIGTERM does not cascade.", "default": False, }, }, @@ -748,7 +764,17 @@ def _is_timeout_error(exc: BaseException) -> bool: { "name": "start_phase", "description": ( - "Start execution of the current phase for a pipeline.\n\n" + "Flip the current phase's execution status to RUNNING. Mutates: " + "sets phase_execution.status = RUNNING on pipeline.current_phase, " + "stamps started_at and work_started_at, and sets pipeline.status = " + "RUNNING. Does NOT spawn agents — agent spawning is driven by the " + "_run_pipeline loop when it observes a RUNNING phase, which is " + "already active for pipelines created through the normal submit " + "path. Does NOT transition to the next phase — only affects " + "pipeline.current_phase. Intended for operator recovery when a " + "phase needs to be re-marked RUNNING (e.g. after a crash); not " + "the way to move a completed phase forward — use advance_phase " + "for that.\n\n" "Error responses include a machine-readable `reason` code (#1939). " "Note: reason codes are only visible to direct HTTP callers; the " "MCP handler layer does not yet surface them.\n" @@ -771,10 +797,20 @@ def _is_timeout_error(exc: BaseException) -> bool: { "name": "complete_phase", "description": ( - "Mark the current phase as complete for a pipeline, with optional " - "artifacts. Returns 409 when the phase still has unresolved HITL " - "decisions; pass force=true to abandon them (the abandoned ids are " - "recorded in the phase's artifacts for audit).\n\n" + "Mark the current phase's execution as COMPLETE. Mutates: sets " + "phase_execution.status = COMPLETE and stamps completed_at on " + "pipeline.current_phase, optionally stores artifacts on that " + "phase_execution, persists BRC history for the phase, and clears " + "ephemeral inter-agent messaging and consensus state. Does NOT " + "advance the pipeline — pipeline.current_phase still points at " + "the just-completed phase afterwards; callers must invoke " + "advance_phase to move forward. The next_phase field in the " + "response data names the canonical next transition, not the new " + "current_phase — current_phase is also echoed so callers can " + "confirm it has not moved. Returns 409 when the phase still has " + "unresolved HITL decisions; pass force=true to abandon them " + "(abandoned ids are recorded in the phase's artifacts for " + "audit).\n\n" "Error responses include a machine-readable `reason` code (#1939). " "Note: reason codes are only visible to direct HTTP callers; the " "MCP handler layer does not yet surface them.\n" diff --git a/orchestrator/routes/phases.py b/orchestrator/routes/phases.py index faa89f291f..309f396453 100644 --- a/orchestrator/routes/phases.py +++ b/orchestrator/routes/phases.py @@ -519,12 +519,17 @@ def start_phase(pipeline_id: str) -> tuple[Response, int]: Response: { "success": true, - "message": "Phase started", + "message": "Phase 'implement' marked running (does not spawn agents)", "data": { "phase": "implement", "status": "running" } } + + Note: this endpoint only flips phase_execution.status to RUNNING. It + does NOT spawn agents — agent spawning is driven by the _run_pipeline + loop. Intended for operator recovery; not the way to advance a + completed phase — use advance_phase for that. """ try: store, pipeline = get_state_store_for_pipeline(pipeline_id) @@ -552,7 +557,7 @@ def start_phase(pipeline_id: str) -> tuple[Response, int]: ) return make_success_response( - "Phase started", + f"Phase '{pipeline.current_phase.value}' marked running (does not spawn agents)", data={ "phase": pipeline.current_phase.value, "status": phase_execution.status.value, @@ -673,12 +678,18 @@ def complete_phase(pipeline_id: str) -> tuple[Response, int]: Response: { "success": true, - "message": "Phase completed", + "message": "Phase 'implement' marked complete; call advance_phase to transition", "data": { "phase": "implement", + "current_phase": "implement", "next_phase": "pr" } } + + Note: this endpoint only flips phase_execution.status to COMPLETE. It + does NOT advance pipeline.current_phase — callers must call + /phase (advance_phase) next. The ``next_phase`` field is the + suggested next transition, not the new current_phase. """ # silent=True: Content-Type: application/json with an empty body would # otherwise raise BadRequest(400), which breaks callers that omit @@ -796,9 +807,17 @@ def complete_phase(pipeline_id: str) -> tuple[Response, int]: ) return make_success_response( - "Phase completed", + ( + f"Phase '{pipeline.current_phase.value}' marked complete; " + "call advance_phase to transition" + ), data={ "phase": pipeline.current_phase.value, + # Echo current_phase to make it explicit that this endpoint + # did NOT advance the pipeline — the pointer is unchanged. + # next_phase is the *suggested* transition, not the new + # current_phase. See #1940. + "current_phase": pipeline.current_phase.value, "next_phase": next_phase.value if next_phase else None, }, ) diff --git a/orchestrator/tests/test_complete_phase_endpoint.py b/orchestrator/tests/test_complete_phase_endpoint.py index 8187c16942..42b8e53272 100644 --- a/orchestrator/tests/test_complete_phase_endpoint.py +++ b/orchestrator/tests/test_complete_phase_endpoint.py @@ -70,6 +70,7 @@ def test_empty_body_returns_200(self, mock_get_store, _mock_clear, client): data = json.loads(resp.data) assert data["success"] is True assert data["data"]["phase"] == "implement" + assert data["data"]["current_phase"] == "implement" assert data["data"]["next_phase"] == "pr" phase_exec = pipeline.get_phase_execution(PipelinePhase.IMPLEMENT) diff --git a/orchestrator/tests/test_mcp_tools.py b/orchestrator/tests/test_mcp_tools.py index fa3fd551d3..2c8f113489 100644 --- a/orchestrator/tests/test_mcp_tools.py +++ b/orchestrator/tests/test_mcp_tools.py @@ -1667,7 +1667,7 @@ def test_start_success(self, handler): with patch.object(handler, "_make_request") as mock_req: mock_req.return_value = { "success": True, - "message": "Phase started", + "message": "Phase 'implement' marked running (does not spawn agents)", "data": {"phase": "implement", "status": "running"}, } result = handler.handle_tool_call("start_phase", {"task_id": "issue-42"}) @@ -1709,8 +1709,8 @@ def test_complete_without_artifacts(self, handler): with patch.object(handler, "_make_request") as mock_req: mock_req.return_value = { "success": True, - "message": "Phase completed", - "data": {"phase": "implement", "next_phase": "pr"}, + "message": "Phase 'implement' marked complete; call advance_phase to transition", + "data": {"phase": "implement", "current_phase": "implement", "next_phase": "pr"}, } result = handler.handle_tool_call("complete_phase", {"task_id": "issue-42"}) @@ -1720,6 +1720,7 @@ def test_complete_without_artifacts(self, handler): data=None, ) assert result["success"] is True + assert result["data"]["current_phase"] == "implement" assert result["data"]["next_phase"] == "pr" def test_complete_with_artifacts(self, handler): @@ -1728,8 +1729,8 @@ def test_complete_with_artifacts(self, handler): with patch.object(handler, "_make_request") as mock_req: mock_req.return_value = { "success": True, - "message": "Phase completed", - "data": {"phase": "implement", "next_phase": "pr"}, + "message": "Phase 'implement' marked complete; call advance_phase to transition", + "data": {"phase": "implement", "current_phase": "implement", "next_phase": "pr"}, } result = handler.handle_tool_call( "complete_phase", diff --git a/sandbox/egg_lib/orch_cli.py b/sandbox/egg_lib/orch_cli.py index eaa4a76d26..2a3bad0f3e 100755 --- a/sandbox/egg_lib/orch_cli.py +++ b/sandbox/egg_lib/orch_cli.py @@ -663,9 +663,7 @@ def cmd_phase_advance(args: argparse.Namespace) -> int: return 0 if result.get("success"): - phase_data = result.get("data", {}) - new_phase = phase_data.get("current_phase", phase_data.get("phase", "?")) - print(f"Advanced to phase: {new_phase}") + print(result.get("message", "Phase advanced")) return 0 print(f"Error: {result.get('message')}", file=sys.stderr) return 1 @@ -681,7 +679,7 @@ def cmd_phase_start(args: argparse.Namespace) -> int: return 0 if result.get("success"): - print("Phase started") + print(result.get("message", "Phase started")) return 0 print(f"Error: {result.get('message')}", file=sys.stderr) return 1 @@ -701,7 +699,12 @@ def cmd_phase_complete(args: argparse.Namespace) -> int: return 0 if result.get("success"): - print("Phase completed") + msg = result.get("message", "Phase completed") + phase_data = result.get("data", {}) + next_phase = phase_data.get("next_phase") + if next_phase: + msg += f"\nRun: egg-orch phase advance --target-phase {next_phase}" + print(msg) return 0 print(f"Error: {result.get('message')}", file=sys.stderr) return 1 diff --git a/sandbox/tests/test_phase_cli.py b/sandbox/tests/test_phase_cli.py new file mode 100644 index 0000000000..41af0b9d2d --- /dev/null +++ b/sandbox/tests/test_phase_cli.py @@ -0,0 +1,141 @@ +""" +Tests for CLI phase commands: ``cmd_phase_start``, ``cmd_phase_complete``, +``cmd_phase_advance``. + +Verifies that each command echoes the server's ``result["message"]`` rather +than constructing strings client-side, and that ``cmd_phase_complete`` appends +a CLI-specific advance hint when ``next_phase`` is present. +""" + +import argparse +import sys +from pathlib import Path +from unittest.mock import patch + +_sandbox_path = str(Path(__file__).parent.parent) +if _sandbox_path not in sys.path: + sys.path.insert(0, _sandbox_path) + +from egg_lib.orch_cli import cmd_phase_advance, cmd_phase_complete, cmd_phase_start + + +def _make_phase_args(**overrides: object) -> argparse.Namespace: + """Build a minimal ``argparse.Namespace`` for phase commands.""" + defaults = { + "pipeline_id": "pipe-1", + "json": False, + "target_phase": "plan", + "reason": None, + } + defaults.update(overrides) + return argparse.Namespace(**defaults) + + +# --------------------------------------------------------------------------- +# cmd_phase_start +# --------------------------------------------------------------------------- + + +class TestCmdPhaseStart: + """cmd_phase_start echoes server message.""" + + @patch("egg_lib.orch_cli.orch_request") + def test_echoes_server_message(self, mock_request, capsys): + mock_request.return_value = { + "success": True, + "message": "Phase 'implement' marked running (does not spawn agents)", + "data": {"phase": "implement", "current_phase": "implement", "status": "running"}, + } + rc = cmd_phase_start(_make_phase_args()) + assert rc == 0 + out = capsys.readouterr().out + assert "Phase 'implement' marked running (does not spawn agents)" in out + + @patch("egg_lib.orch_cli.orch_request") + def test_error_prints_to_stderr(self, mock_request, capsys): + mock_request.return_value = { + "success": False, + "message": "Phase already running", + } + rc = cmd_phase_start(_make_phase_args()) + assert rc == 1 + err = capsys.readouterr().err + assert "Phase already running" in err + + +# --------------------------------------------------------------------------- +# cmd_phase_complete +# --------------------------------------------------------------------------- + + +class TestCmdPhaseComplete: + """cmd_phase_complete echoes server message and appends advance hint.""" + + @patch("egg_lib.orch_cli.orch_request") + def test_echoes_server_message_with_advance_hint(self, mock_request, capsys): + mock_request.return_value = { + "success": True, + "message": "Phase 'implement' marked complete; call advance_phase to transition", + "data": {"phase": "implement", "current_phase": "implement", "next_phase": "pr"}, + } + rc = cmd_phase_complete(_make_phase_args()) + assert rc == 0 + out = capsys.readouterr().out + assert "Phase 'implement' marked complete; call advance_phase to transition" in out + assert "Run: egg-orch phase advance --target-phase pr" in out + + @patch("egg_lib.orch_cli.orch_request") + def test_no_advance_hint_for_terminal_phase(self, mock_request, capsys): + mock_request.return_value = { + "success": True, + "message": "Phase 'pr' marked complete; call advance_phase to transition", + "data": {"phase": "pr", "current_phase": "pr", "next_phase": None}, + } + rc = cmd_phase_complete(_make_phase_args()) + assert rc == 0 + out = capsys.readouterr().out + assert "Phase 'pr' marked complete" in out + assert "Run: egg-orch phase advance" not in out + + @patch("egg_lib.orch_cli.orch_request") + def test_error_prints_to_stderr(self, mock_request, capsys): + mock_request.return_value = { + "success": False, + "message": "Phase not running", + } + rc = cmd_phase_complete(_make_phase_args()) + assert rc == 1 + err = capsys.readouterr().err + assert "Phase not running" in err + + +# --------------------------------------------------------------------------- +# cmd_phase_advance +# --------------------------------------------------------------------------- + + +class TestCmdPhaseAdvance: + """cmd_phase_advance echoes server message.""" + + @patch("egg_lib.orch_cli.orch_request") + def test_echoes_server_message(self, mock_request, capsys): + mock_request.return_value = { + "success": True, + "message": "Phase advanced to plan", + "data": {"previous_phase": "refine", "current_phase": "plan"}, + } + rc = cmd_phase_advance(_make_phase_args(target_phase="plan")) + assert rc == 0 + out = capsys.readouterr().out + assert "Phase advanced to plan" in out + + @patch("egg_lib.orch_cli.orch_request") + def test_error_prints_to_stderr(self, mock_request, capsys): + mock_request.return_value = { + "success": False, + "message": "Invalid target phase", + } + rc = cmd_phase_advance(_make_phase_args(target_phase="invalid")) + assert rc == 1 + err = capsys.readouterr().err + assert "Invalid target phase" in err