docs: document #2777 plan preflight validator + hard-required context PR opener [doc-updater]

[james-in-a-box]

Update documentation to reflect changes from PR #2888 (issue #2777 slice-1):

• docs/guides/sdlc-pipeline.md: The plan populate section now notes that advance_phase at plan→implement runs a structural pre-flight check before populate. Checks for yaml-tasks, pr.title, pr.description, pr.test_plan, and pr.manual_steps; returns 422 preflight_invalid_plan with missing_fields list on failure. force=true bypasses the validator.
• docs/architecture/sdlc-pipeline.md: Updated the context-PR parenthetical (Context PR silently not created for pipelines (regression of #2548 / PR #2578) #2593/#2744) to note that #2777 replaced the soft-fail advance_phase opener with _open_context_pr_at_implement_start — a hard-required opener that runs before the state lock, so gateway failures now surface as 422 instead of silently stranding the slice stack.

Triggered by: #2888

Test Plan

• Automated: doc-only change, no code changes
• Manual: verify the updated paragraphs accurately describe the code in orchestrator/routes/phases.py (advance_phase plan→implement block) and shared/egg_contracts/plan_parser.py (validate_plan_preflight)

Authored-by: egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
docs/guides/sdlc-pipeline.md	Additive (auto-merged)	Git cleanly merged the PR's new "Plan pre-flight validation at plan→implement (#2777)" paragraph with main's slice-3 doc updates — no manual intervention needed.
docs/architecture/sdlc-pipeline.md	Semantic (superset)	Took main's side. The PR added a sentence to the legacy #2593/#2744 soft-fail paragraph, but main's slice-3 refactor (ca16f5f) deleted that entire paragraph and replaced it with a new "Context-PR mechanism (#2777 collapse)" paragraph plus the new "Schema 1.2 (#2777)" paragraph. Main's new paragraph already states the context PR is "up-front at the plan→implement boundary, hard-required and idempotent" — which is exactly what the PR's sentence was trying to add. Re-introducing the legacy paragraph would have contradicted main's newer "the multi-step soft-fail open path were deleted (#2777)" framing.

Please review: the docs/architecture/sdlc-pipeline.md resolution drops the PR's explicit mention of _open_context_pr_at_implement_start and its "runs before the state-lock-protected phase mutation" detail. If you want that specific implementation note preserved, it should be added to main's "Context-PR mechanism (#2777 collapse)" paragraph as a follow-up rather than reverting to the legacy framing.

Verification:

• tests/docs/test_context_pr_doc_terminology.py — 30 passed, 9 xpassed
• make lint — no errors (only pre-existing soft-cap warnings)

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. Doc-only change describing orchestrator pre-flight validation behavior — no impact on prompts, agent workflows, or LLM invocation paths.

— Authored by egg

[egg-reviewer]

Review

Verified the new paragraph against orchestrator/routes/phases.py (lines 376–514) and shared/egg_contracts/plan_parser.py::validate_plan_preflight (lines 1736–1820). Most of the claims (gate on not force, runs before populate, 422 with reason: preflight_invalid_plan + missing_fields, accumulates all errors, parseable yaml-tasks requirement, pr.title / pr.description / pr.test_plan non-empty) match the implementation exactly.

Blocking

pr.manual_steps is not a non-empty check — it is a key-presence check. The new paragraph says:

non-empty pr.title, pr.description, pr.test_plan, and pr.manual_steps fields

But validate_plan_preflight deliberately treats pr.manual_steps differently from the other three:

# plan_parser.py:1808–1817
# (e) manual_steps: an empty string IS allowed (contract default),
# so we only reject when the key is ABSENT from the parsed YAML
# entirely.
...
if "manual_steps" not in raw_pr_block:
    missing.append("pr.manual_steps")

The validator docstring (plan_parser.py:1755–1757) is also explicit: "pr.manual_steps — key missing entirely (empty string is allowed; the contract field defaults to "")".

This isn't a nit — the new paragraph contradicts the existing guidance four lines above it in the same file (docs/guides/sdlc-pipeline.md:855):

The manual_steps field lists any pre- or post-merge actions (migrations, config changes, deployments); use an empty string if none.

Operators reading the page top-to-bottom now get two different rules for the same field. The practical failure mode: an operator who follows the existing "use an empty string if none" guidance will hit a 422 only if they remove the key entirely; one who reads the new paragraph will think empty-string is invalid and add padding text. Either path is wrong relative to the validator.

Suggested fix — split the rule so it matches the validator:

The check verifies that the plan contains a parseable yaml-tasks block, non-empty pr.title / pr.description / pr.test_plan fields, and a present pr.manual_steps key (an empty string is allowed — the contract default — but the key must exist in the YAML).

Non-blocking

• The PR body advertises an update to docs/architecture/sdlc-pipeline.md too, but the merge commit (32c9713) dropped that hunk because main's slice-3 refactor already replaced the paragraph being extended. The merge message explains this clearly, so no action needed — flagging only so the PR description and final diff don't diverge for the merger.
• The new paragraph documents the operator-facing 422 surface but doesn't mention the three 500 / reason: preflight_unavailable branches in phases.py (import failure, OSError on worktree probe, OSError on plan read), nor the silent skip when the draft path is absent or not declared. These are infra/edge cases rather than operator-facing semantics, so probably fine to omit — but if you want the section to be a complete reference for "what advance_phase plan→implement can return", they're worth a parenthetical.

— Authored by egg

[james-in-a-box]

Review Response

Addressed both items from the egg-reviewer feedback in commit d540793.

Blocking

• pr.manual_steps is a key-presence check, not non-empty — fixed-in-PR (commit d540793). You're right, and the contradiction with line 854 (use an empty string if none) was the worst part. Took your suggested wording verbatim: the paragraph now groups pr.title / pr.description / pr.test_plan as non-empty checks and documents pr.manual_steps as a present-key check with the contract default called out explicitly. Verified against plan_parser.py:1802-1817 and the validator docstring at plan_parser.py:1755-1757.

Non-blocking

• PR body advertises a docs/architecture/sdlc-pipeline.md hunk dropped by the merge — disagree (no code change needed; merge commit message already explains why the hunk was dropped, as you noted). Flagging only so the merger sees the same reasoning you did — the PR body will read as stale to anyone who didn't look at 32c9713's message, but updating the PR description for a merged-away hunk doesn't change the diff. Happy to edit the PR body if you'd prefer it match the final diff verbatim.
• Missing 500 / preflight_unavailable branches and silent-skip cases — fixed-in-PR (commit d540793). Added a final sentence covering both: the import-failure / OSError paths surface as 500 with reason: preflight_unavailable (so operators retry rather than mistake infra trouble for a passing check), and the no-draft-path / draft-absent cases skip silently and let populate handle the gap. Kept it to one sentence so the operator-facing 422 surface stays the headline of the paragraph.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns on the delta. The only change since my prior review is a one-line refinement in docs/guides/sdlc-pipeline.md clarifying pr.manual_steps as a key-presence check and documenting the 500 / preflight_unavailable and silent-skip branches — still operator-facing orchestrator validation behavior, no impact on prompts, agent workflows, or LLM invocation paths.

— Authored by egg

[egg-reviewer]

Re-review

Re-verified the delta between 32c97136 and d540793 against shared/egg_contracts/plan_parser.py::validate_plan_preflight (lines 1736–1820) and orchestrator/routes/phases.py lines 390–514.

Blocking issue from prior review — fixed

The pr.manual_steps semantics correction landed verbatim. The paragraph at docs/guides/sdlc-pipeline.md:873 now reads:

non-empty pr.title / pr.description / pr.test_plan fields, and a present pr.manual_steps key (an empty string is allowed — the contract default — but the key must exist in the YAML)

This matches the validator (plan_parser.py:1790–1817) — the first three are stripped non-empty checks via _normalize_optional_string while pr.manual_steps does a key-presence check on result.raw_yaml["pr"]. The contradiction with the "use an empty string if none" guidance four lines above (line 854) is resolved.

Non-blocking completeness — incorporated cleanly

The added sentence covering 500 / preflight_unavailable and silent-skip branches is accurate:

• ImportError → 500 / preflight_unavailable: matches phases.py:406–417 (resolve_worktree_path / _get_draft_path import) and phases.py:487–497 (plan_parser import).
• OSError → 500 / preflight_unavailable: matches phases.py:431–442 (worktree probe / draft-path resolution) and phases.py:470–480 (plan_path.read_text()).
• Undeclared draft path → silent skip: matches phases.py:444–454 (_draft_rel is None branch, info-log only).
• Absent draft file → silent skip: matches phases.py:455–465 ((_validator_worktree / _draft_rel).exists() is False, info-log only).

The phrase "dependency import errors, worktree probe / draft-read OSErrors" cleanly covers all four 500-returning branches without over-specifying. The semicolon-joined silent-skip clause correctly attributes the gap to populate's downstream handling.

No new concerns

No new claims in the delta that contradict the implementation. The PR body's stale reference to docs/architecture/sdlc-pipeline.md is still present, but as noted in the prior review the merge commit (32c9713) message explains why that hunk was dropped, and the author confirmed they will edit if requested — no blocker.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.