Fix #1940: make phase MCP tool descriptions name their state-machine effects

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"

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,
             },
         )

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)

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",

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

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