Skip to content

docs: fix start_phase and complete_phase descriptions [doc-updater] - #1953

Merged
jwbron merged 4 commits into
mainfrom
egg/doc-update-phase-tool-descriptions
Apr 24, 2026
Merged

docs: fix start_phase and complete_phase descriptions [doc-updater]#1953
jwbron merged 4 commits into
mainfrom
egg/doc-update-phase-tool-descriptions

Conversation

@james-in-a-box

Copy link
Copy Markdown
Contributor

Update docs/reference/orchestrator-cli.md to correct factual errors
introduced when #1944 updated phase tool behavior without updating docs.

Changes:

  • start_phase entry: removed "spawns agents" claim — this endpoint only
    flips phase_execution.status to RUNNING; agent spawning is handled by
    the _run_pipeline loop. The comment in the recovery workflow example
    is updated to match.
  • complete_phase entry: clarified it does NOT advance the pipeline;
    operators must call advance_phase next. Added mention of the new
    current_phase field in the response (added in Fix #1940: make phase MCP tool descriptions name their state-machine effects #1944 to make the
    "no advance happened" guarantee explicit).

Triggered by: #1944

Test Plan

  • Automated: no automated doc tests; changes are textual corrections only
  • Manual: read the updated section in context to confirm it matches the
    implemented behavior in orchestrator/routes/phases.py

Authored-by: egg

…-cli ref

Correct two factual errors introduced when #1944 updated the server-side
behavior without updating the docs:

- start_phase does NOT spawn agents (spawning is driven by _run_pipeline
  loop); docs said "spawns agents"
- complete_phase does NOT advance the pipeline; add note that callers
  must invoke advance_phase next, and document the new current_phase
  field in the response

Authored-by: egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified all three changes against the implementation in orchestrator/routes/phases.py:

  • start_phase description: Confirmed — the handler (lines 510–584) sets phase_execution.status = PipelineStatus.RUNNING and returns. No agent spawning logic. The docstring at line 529 explicitly states "does NOT spawn agents."
  • complete_phase description: Confirmed — the handler (lines 662–842) sets phase_execution.status = PipelineStatus.COMPLETE, does not touch pipeline.current_phase, and returns both current_phase (unchanged) and next_phase (suggested transition) in the response (lines 814–822). Comment at line 817 references #1940.
  • Recovery workflow comment: Correctly updated to match.

All three changes are factually accurate.


Non-blocking suggestion: The same stale descriptions exist in docs/guides/sdlc-pipeline.md (lines 773–774):

- `start_phase` — start a phase that is in state but has no running containers
- `complete_phase` — manually mark a stuck phase as complete

Since the whole point of this PR is correcting these misconceptions, it would be worth fixing these in the same pass. The start_phase line in particular still implies "starting" means spawning work, and complete_phase says "stuck phase" which implies it unblocks transitions (it doesn't — you need advance_phase for that).

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Fixed the stale start_phase and complete_phase descriptions in docs/guides/sdlc-pipeline.md (lines 773–774) to match the corrected behavior:

  • start_phase — mark a phase RUNNING (does not spawn agents)
  • complete_phase — mark a phase COMPLETE (does not advance the pipeline)

Pushed in 382269c.

— Authored by egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns. This is a documentation-only correction — no agent workflows, prompts, or LLM interactions are affected.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 382269c. Delta since last review: one commit fixing the stale start_phase / complete_phase descriptions in docs/guides/sdlc-pipeline.md.

Verification of previous feedback: The non-blocking suggestion from the prior review (fix stale descriptions at lines 773–774 of sdlc-pipeline.md) has been properly addressed. The new wording is consistent with the corrections already made in docs/reference/orchestrator-cli.md.

New commit review:

The change at docs/guides/sdlc-pipeline.md lines 773–774 is correct:

  • start_phase — now says "mark a phase RUNNING (does not spawn agents)" — matches the implementation at orchestrator/routes/phases.py:529 which only flips phase_execution.status to RUNNING.
  • complete_phase — now says "mark a phase COMPLETE (does not advance the pipeline)" — matches the implementation at orchestrator/routes/phases.py:693 which only flips status to COMPLETE and returns current_phase (unchanged) + next_phase (suggestion).

The remaining start_phase references in sdlc-pipeline.md (lines 1086+) refer to the config.start_phase pipeline parameter (which phase to begin execution at), not the MCP tool — correctly left untouched.

No other doc files (agent-tools.md, agent-recovery.md, etc.) contain stale references to these tools.

No issues found. Both commits are factually accurate documentation corrections.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main:

File Category Resolution
docs/reference/orchestrator-cli.md Additive Combined main's advance_phase auto-populate note (#1950) with PR's corrected start_phase and complete_phase descriptions
docs/guides/sdlc-pipeline.md Auto-merged cleanly (no conflict)

Details on orchestrator-cli.md conflict:
The phase MCP tools table had three rows modified on both sides:

Verification: make lint passed, 13142 tests passed (108 skipped).

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at fc42a1e. Delta since 382269c is a merge commit resolving conflicts with main plus minor wording fixes — all documentation-only. No agent-mode design concerns.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Merged origin/main into egg/doc-update-phase-tool-descriptions — no manual conflict resolution was needed.

The two new commits from main (#1957, #1937) auto-merged cleanly with this PR's doc changes. The only shared file (docs/reference/orchestrator-cli.md) had changes in non-overlapping sections:

File Status Details
docs/reference/orchestrator-cli.md Auto-merged Main updated the egg-orch push row (#1882); this PR updated start_phase/complete_phase rows — no overlap
docs/guides/sdlc-pipeline.md No conflict Only modified by this PR

Verification:

  • make lint — passed
  • make test — 13,328 passed, 120 skipped, 0 failures

— Authored by egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at fc42a1e. Delta since last review (382269c): one merge commit resolving conflicts with main.

Previous feedback verification: The non-blocking suggestion from the prior review cycle (fix stale start_phase/complete_phase descriptions in sdlc-pipeline.md) was addressed in 382269c and survives the merge intact at lines 773–774.

Merge conflict resolution review:

The conflict was in docs/reference/orchestrator-cli.md where main had added content from #1950 (advance_phase auto-populate note) and #1951 (phase lifecycle error reason codes) while the PR had corrected the start_phase and complete_phase descriptions. The resolution correctly:

  1. advance_phase row (line 230): Took main's version with the auto-populate note from #1950. Correct — the PR didn't modify this row.
  2. start_phase and complete_phase rows (lines 231–232): Kept the PR's corrected descriptions. Correct.
  3. Error reason codes section (lines 247–267): Brought in from main (#1951). Correct — no conflict with the PR's changes.
  4. advance_phase parameter description (line 239): Took main's version with the auto-populate detail. Correct.
  5. Recovery workflow step 3 comment (lines 281–282): Took main's updated comment from #1950 ("automatic when advancing from plan; needed for other phase transitions where the plan was set up externally"). Correct — more accurate than the PR's original version.
  6. Recovery workflow step 4 comment (line 287): Kept the PR's corrected version ("Mark the phase running (does not spawn agents — the _run_pipeline loop handles that)"). Correct — main still had the stale "Start the phase (spawn agents)" text here.

The sdlc-pipeline.md auto-merged cleanly, picking up main's addition at line 854 (advance_phase auto-populate note from #1950) alongside the PR's corrections at lines 773–774.

Verification against implementation: Confirmed start_phase handler (phases.py:529) only flips status to RUNNING, complete_phase handler (phases.py:689–692) only flips status to COMPLETE without advancing current_phase, and advance_phase auto-populates the contract when leaving plan phase (phases.py:428–450). All documentation matches.

Stale reference check: No remaining stale start_phase/complete_phase descriptions found in the docs directory. The config.start_phase references in sdlc-pipeline.md (lines 1086+) are the pipeline configuration parameter, not the MCP tool — correctly left untouched.

No issues found. The merge resolution is correct and preserves all PR fixes while cleanly incorporating main's additions.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 566c4c0. Delta since last review (fc42a1e): one merge commit bringing in main (primarily #1882 gateway auto-filter changes). No PR-authored changes, no conflicts — the doc corrections in orchestrator-cli.md and sdlc-pipeline.md survive intact.

No agent-mode design concerns. Documentation-only PR with no impact on agent workflows, prompts, or LLM interactions.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 566c4c0. Delta since fc42a1e: one merge commit bringing in main (includes #1957, #1937).

Merge verification: The merge auto-merged cleanly — no conflicts in the PR-touched files (orchestrator-cli.md, sdlc-pipeline.md). The PR's corrections at lines 231–232 and 287 of orchestrator-cli.md, and lines 773–774 of sdlc-pipeline.md, survive intact.

Implementation verification: Re-confirmed all three claims against orchestrator/routes/phases.py:

  • start_phase (lines 510–584): only flips phase_execution.status to RUNNING; docstring at line 529 explicitly states "does NOT spawn agents"
  • complete_phase (lines 662–842): only flips status to COMPLETE; does not touch pipeline.current_phase; response includes current_phase (unchanged) and next_phase (suggested transition) at lines 816–822
  • advance_phase (lines 410–459): auto-populates contract when leaving PLAN phase

Stale reference check: Remaining start_phase references in sdlc-pipeline.md (lines 1086+) are all the config.start_phase pipeline parameter, not the MCP tool — correctly untouched.

No issues found.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg review completed. View run logs

11 previous review(s) hidden.

@jwbron
jwbron merged commit 06dbdcc into main Apr 24, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant