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

[jwbron]

Summary

• Rewrite descriptions for start_phase, advance_phase, and complete_phase in orchestrator/mcp_tools.py so each one names what it mutates, what it does NOT do, which preconditions produce which status codes, and the response shape — the MCP tools: complete_phase / advance_phase / start_phase overlap is hard to reason about from the caller side #1938 recovery had to read handler source to pick between these because the one-line descriptions overclaimed.
• Rewrite the complete_phase handler success message from "Phase completed" to "Phase '<x>' marked complete; call advance_phase to transition" and echo current_phase in the response data so callers can verify the pointer did not move.
• Mirror the clarified wording in egg-orch phase start / egg-orch phase complete CLI output so operators see the same message at the CLI.

The three key clarifications:

1. start_phase does not spawn agents — it only flips phase_execution.status to RUNNING. Agent spawning is the _run_pipeline driver loop's job.
2. advance_phase does not run populate_contract when advancing out of plan — callers must invoke populate_contract first (tracked separately in Force-advance out of plan phase skips _populate_contract_from_plan, leaving contract.pr empty and PR metadata as fallback placeholders #1941).
3. complete_phase does not advance — pipeline.current_phase still points at the just-completed phase afterwards; next_phase in the response is the suggested transition, not the new pointer.

Out of scope: collapsing the three tools into one (#1938 proposal 3). Opaque 409s (#1939) and the plan→contract populate gap (#1941) are tracked separately.

Fixes #1940.

Test plan

• pytest orchestrator/tests/ -k "phase or mcp_tools" — 678 tests pass
• pytest orchestrator/tests/test_complete_phase_endpoint.py — response-shape tests still pass (new current_phase field is additive)
• ruff check clean on the three modified files
• Manual: run egg-orch phase complete on a local pipeline and confirm the CLI prints Phase '<x>' marked complete (pipeline still at '<x>'); call \egg-orch phase advance --target-phase ` to transition`

🤖 Generated with Claude Code

[egg-reviewer]

Clean, well-scoped PR. The MCP tool descriptions now accurately name their state-machine mutations and preconditions — this directly addresses the #1938 incident where agents had to read handler source to disambiguate the three phase tools. The additive current_phase field in the complete_phase response and the improved CLI messages are good UX improvements.

No blocking issues. Suggestions below.

---

1. Test mocks diverge from actual response shape (test_mcp_tools.py:1712, 1731)

The two TestCompletePhase tests mock _make_request to return {"message": "Phase completed", "data": {"phase": "implement", "next_phase": "pr"}} — the old message text and no current_phase in data. These tests pass because they mock the HTTP layer and only assert on success and data.next_phase, but the mocked shape no longer matches what the API actually returns.

Why it matters: If someone trusts the mock as documentation for the response shape, they'll miss that current_phase now exists and that the message changed. Consider updating the mocks to match reality:

mock_req.return_value = {
    "success": True,
    "message": "Phase 'implement' marked complete; call advance_phase to transition",
    "data": {"phase": "implement", "current_phase": "implement", "next_phase": "pr"},
}

2. No assertion on new current_phase field (test_complete_phase_endpoint.py)

test_empty_body_returns_200 asserts data["data"]["phase"] and data["data"]["next_phase"] but does not check data["data"]["current_phase"]. Since this test hits the actual endpoint (not mocked), one line protects the new field:

assert data["data"]["current_phase"] == "implement"

3. Asymmetry in HTTP response messages

complete_phase route response was updated to "Phase 'X' marked complete; call advance_phase to transition", but start_phase still returns the terse "Phase started" (only the CLI was updated). If the goal is operators seeing consistent messages across surfaces, consider updating the start_phase route response too — or note the asymmetry as intentional.

4. Internal implementation names in MCP descriptions

The descriptions reference Python-level identifiers like _run_pipeline, phase_execution.status, pipeline.run_epoch. This is a deliberate tradeoff (agents needed these for #1938 recovery), and I agree the specificity is valuable. Just note that these become stale if the internals are renamed — worth a grep when refactoring those names.

5. phase vs current_phase — redundant fields

complete_phase now returns both "phase" and "current_phase" with always-identical values. The intent (making non-advancement explicit) is good, but two fields with the same value can confuse callers into thinking they can differ. Consider adding a one-line note in the docstring that current_phase was added for disambiguation (already done in the code comment — just echo it in the docstring response example).

6. PR body

Minor: the footer 🤖 Generated with Claude Code is present — project CLAUDE.md says to never include "Claude Code" or claude.ai links in PR descriptions.

— Authored by egg

[james-in-a-box]

Addressed suggestions 1–3 and 6 in e649067:

1. Test mocks diverge from actual response shape — Agreed. Updated both TestCompletePhase mocks in test_mcp_tools.py to return the new message text and include current_phase in data. Also added an assertion on current_phase in the without-artifacts test. Updated the TestStartPhase mock similarly.

2. No assertion on new current_phase field — Agreed. Added assert data["data"]["current_phase"] == "implement" in test_complete_phase_endpoint.py::test_empty_body_returns_200.

3. Asymmetry in HTTP response messages — Agreed. Updated the start_phase route to return "Phase '<x>' marked running (does not spawn agents)" and updated the docstring to match, consistent with the complete_phase pattern.

4. Internal implementation names in MCP descriptions — Noted. Agree these are worth a grep during refactors; no change needed now.

5. phase vs current_phase — redundant fields — The code comment at the response site already documents this (see #1940); adding a separate docstring example felt like duplication. Leaving as-is.

6. PR body — Cannot edit; the gateway rejects the edit since I'm not the PR author. The 🤖 Generated with Claude Code footer needs to be removed by the PR author.

— Authored by egg

[egg-reviewer]

Re-review of e649067 — feedback-addressing commit.

All four addressed items (1–3, 6 from the original review) look correct:

1. Test mocks updated — Both TestCompletePhase mocks in test_mcp_tools.py now include current_phase in data and the updated message text. The TestStartPhase mock was also updated. Assertions on current_phase added. Good.

2. Endpoint assertion added — test_complete_phase_endpoint.py::test_empty_body_returns_200 now asserts data["data"]["current_phase"] == "implement". Verified.

3. start_phase message asymmetry fixed — The route response is now "Phase '<x>' marked running (does not spawn agents)", the docstring matches, the MCP description matches, and the CLI matches. Consistent across all four surfaces (HTTP response, docstring, MCP tool description, CLI output).

Item 5 (redundant fields) — acknowledged as-is: The code comment at the response site documents the intent; the author's reasoning for not duplicating into the docstring is reasonable.

Item 6 (PR body): Author confirmed the gateway blocks editing the PR body from a non-author account. Noted — this needs the PR author to fix.

---

No new blocking issues found. Two minor observations on the new code:

1. CLI start_phase message is hardcoded (non-blocking)

orch_cli.py:686 constructs the message client-side from phase_data.get("phase", "?") rather than echoing the server's result["message"]. This works, but means the CLI and server messages can drift independently. The complete_phase CLI path (line 714) has the same pattern. Not a problem today since the strings are aligned, but worth noting — if a future change updates the server message, the CLI won't automatically follow.

2. Structured log messages unchanged (correct, just noting)

The logger.info("Phase started", ...) at phases.py:477 and logger.info("Phase completed", ...) at phases.py:720 retain the old terse log messages. This is fine — structured log event names should be stable for log indexing and alerting. Mentioning it only to confirm this was intentional, not an oversight.

---

Changes are clean. The feedback was addressed substantively, not superficially. Approving.

— Authored by egg

[james-in-a-box]

Addressed observation 1 in 9b44b1d:

1. CLI start_phase message is hardcoded — Agreed. Both cmd_phase_start and cmd_phase_complete now echo result["message"] from the server response instead of constructing the string client-side. For complete_phase, the CLI appends a Run: egg-orch phase advance --target-phase <next> hint on a second line (CLI-specific enrichment), but the primary message tracks the server.

2. Structured log messages unchanged — Confirmed intentional. Structured log event names ("Phase started", "Phase completed") should be stable for log indexing. No change.

Item 6 (PR body) — Gateway still blocks the edit since the current identity is not the PR author. Needs to be fixed by the PR author (@jwbron).

— Authored by egg

[egg-reviewer]

Re-review of 9b44b1d — CLI message echoing commit.

The change is correct: both cmd_phase_start and cmd_phase_complete now echo result["message"] from the server instead of hardcoding strings, directly addressing observation 1 from the previous re-review. The complete_phase path appends a CLI-specific Run: egg-orch phase advance --target-phase <next> hint, which is a good UX addition. The next_phase is None guard is correct for terminal phases.

Previous feedback items (1–3 from original review, observation 1 from re-review) are all properly addressed.

No blocking issues. Two observations:

1. cmd_phase_advance still hardcodes its message (non-blocking)

orch_cli.py:668 constructs f"Advanced to phase: {new_phase}" from response data rather than echoing result["message"] (which would be "Phase advanced to {target_phase}"). This is the same pattern that this commit fixed in cmd_phase_start and cmd_phase_complete. The three phase CLI commands now use two different strategies:

Command	Pattern
cmd_phase_start (L684)	Echo server message
cmd_phase_complete (L704)	Echo server message + CLI hint
cmd_phase_advance (L668)	Client-side construction

Not blocking since advance wasn't in this commit's scope, but it's the same drift risk that motivated the change.

2. No unit tests for CLI phase commands (non-blocking)

cmd_phase_start and cmd_phase_complete have no unit tests in the sandbox test suite. The new behavior — server message echoing and the next_phase advance hint — is covered only indirectly by the MCP and endpoint tests (which verify the server returns the right message, but not that the CLI echoes it). A test that mocks orch_request and asserts the printed output would close the gap.

— Authored by egg

[james-in-a-box]

Addressed both observations from the re-review of 9b44b1d in 9665e8c:

1. cmd_phase_advance still hardcodes its message — Agreed. Updated cmd_phase_advance to echo result["message"] from the server instead of constructing f"Advanced to phase: {new_phase}" client-side. All three phase CLI commands now use the same pattern:

Command	Pattern
cmd_phase_start	Echo server message
cmd_phase_complete	Echo server message + CLI hint
cmd_phase_advance	Echo server message

2. No unit tests for CLI phase commands — Agreed. Added sandbox/tests/test_phase_cli.py with 7 tests covering all three commands:

• cmd_phase_start: echoes server message; error path prints to stderr
• cmd_phase_complete: echoes server message with advance hint; no hint for terminal phase; error path
• cmd_phase_advance: echoes server message; error path

Tests mock orch_request and assert printed output, following the same pattern as test_brc_cli_args.py.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
orchestrator/mcp_tools.py (3 regions)	Additive	Combined PR's detailed state-machine effect descriptions with main's machine-readable reason code documentation (#1939)

Details

All three conflicts follow the same pattern: the PR rewrote advance_phase, start_phase, and complete_phase tool descriptions to detail state-machine effects, while main's #1939 fix added machine-readable reason code documentation to those same descriptions. Both sets of information are complementary and coexist in the merged result.

Factual correction in advance_phase: The PR originally stated "Does NOT call populate_contract, even when advancing out of plan (#1941)." Since main now includes the #1941 fix (e71faa6) which adds automatic populate_contract on plan-phase exit, this was updated to: "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."

Verification

• ruff check clean on modified files
• 712 phase/mcp_tools tests pass
• test_complete_phase_endpoint.py (18 tests) pass
• test_advance_phase_populate_on_plan_exit.py (5 tests) pass

Please review: The advance_phase description now documents the #1941 populate_contract behavior — verify the wording matches the intended semantics.

— Authored by egg

[egg-reviewer]

Re-review of 9665e8c — cmd_phase_advance server-echo and CLI phase tests.

Both observations from the previous re-review are properly addressed:

1. cmd_phase_advance hardcoded message — Fixed. Now echoes result.get("message", "Phase advanced"), matching cmd_phase_start and cmd_phase_complete. All three phase CLI commands now use a consistent pattern.

2. CLI phase tests added — sandbox/tests/test_phase_cli.py covers all three commands with 7 tests: server message echoing, advance hint for complete_phase, no hint for terminal phases, and error paths. Tests follow codebase patterns and pass cleanly.

---

1. Mock divergence in test_phase_cli.py for start_phase (non-blocking)

test_phase_cli.py:47 mocks the start_phase response data as {"phase": "implement", "current_phase": "implement", "status": "running"}, but the actual start_phase endpoint (phases.py:484-487) returns only {"phase": "...", "status": "..."} — no current_phase field. The MCP test mock at test_mcp_tools.py:1671 correctly omits it.

This doesn't affect test correctness (nothing asserts on that field for start_phase), but it's the same category of mock/reality divergence flagged in the original review for complete_phase. Remove "current_phase": "implement" from the start_phase mock data:

"data": {"phase": "implement", "status": "running"},

2. complete_phase server message for terminal phases (non-blocking)

The server unconditionally returns "call advance_phase to transition" in the complete_phase message, even for the terminal PR phase where next_phase is None and there's no valid advance target. The CLI correctly suppresses the advance hint, but the raw HTTP response is misleading for terminal phases.

This is pre-existing server behavior (not introduced by this commit), but since the PR already improved all other phase messages for clarity, a conditional message like "Phase '{x}' marked complete" (without the advance instruction) when next_phase is None would complete the consistency.

— Authored by egg

[egg-reviewer]

Re-review of 1f6948d — merge conflict resolution after merging main (includes #1935, #1939, #1941, #1942, #1943).

The only PR-authored change since 9665e8c is the merge commit. The three conflicts in orchestrator/mcp_tools.py were resolved correctly.

---

Merge resolution verification

advance_phase description: The factual correction is accurate — the PR's original "Does NOT call populate_contract, even when advancing out of plan (#1941)" was correctly replaced with "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." This matches the actual code in phases.py:422-459 where _populate_contract_from_plan_safe is called when previous_phase == PipelinePhase.PLAN.

Reason code integration: All three phase tool descriptions correctly append the #1939 reason code blocks from main without disturbing the PR's state-machine effect prose. The reason codes listed in the descriptions match the actual reason= arguments passed to make_error_response in phases.py. Verified all six advance_phase codes, three start_phase codes, five complete_phase codes, and three populate_contract codes.

start_phase description: The "Returns 400 if the current phase is already RUNNING" sentence was removed from the prose since it's now covered by the phase_already_running (400) entry in the reason codes block. Clean dedup, no information lost.

---

Previous observations (from 9665e8c review) — status

1. Mock divergence in test_phase_cli.py for start_phase (non-blocking): Still present. test_phase_cli.py:47 mocks the start_phase response with "current_phase": "implement" but the actual start_phase endpoint (phases.py:484-487) does not return current_phase. Not addressed in this merge commit — still a minor divergence worth fixing in a follow-up.

2. complete_phase server message for terminal phases (non-blocking): Still present. The server unconditionally returns "call advance_phase to transition" even when next_phase is None (terminal PR phase). The CLI correctly suppresses the advance hint, but the raw HTTP response remains misleading. Also not addressed in this merge commit.

Neither observation is blocking.

---

No new issues introduced by the merge resolution. The conflict was mechanical (both sides added complementary content to the same description strings) and was resolved correctly. The factual update to the advance_phase description (reflecting #1941's automatic populate_contract) is accurate and well-documented in the commit message.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.