docs: update pipeline docs for PR-phase removal [doc-updater] - #2898
Conversation
Remove all references to the deprecated PR phase from README.md, docs/architecture/sdlc-pipeline.md, and docs/guides/sdlc-pipeline.md. IMPLEMENT is now the terminal phase. The context PR opens automatically at the plan→implement boundary via _open_context_pr_at_implement_start. Schema bumped to 1.2 (removes context_branch/context_title/context_description from PRMetadata; context_pr_number survives). Authored-by: egg
This comment has been minimized.
This comment has been minimized.
… in pipeline docs Both branches updated the same pipeline doc sections after #2777 PR-phase removal. Took main's more comprehensive language where they overlapped, but kept the PR's accurate description of the _migrate_schema_version_to_1_2 wrap-mode migration shim (the v1.1 paragraph on main incorrectly claimed v1.2 was a clean break with no shim; the code does have one).
Conflict Resolution SummaryResolved merge conflicts with
Please review: The two semantic resolutions where this PR's wording was kept over main's — specifically the Schema 1.2 paragraphs in both files. I verified the migration shim is real ( The — Authored by egg |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns. This is a documentation-only PR updating pipeline diagrams, phase tables, and schema migration notes to reflect the PR-phase removal from #2891 (#2777 slice 2/4). No agent prompts, workflows, output formats, or sandbox/gateway boundaries are touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Summary
Doc-only PR updating three files (README.md, docs/architecture/sdlc-pipeline.md, docs/guides/sdlc-pipeline.md) to reflect the PR-phase removal landed in #2891 / #2777.
I verified every substantive code-level claim in the diff against the checked-out branch (PipelinePhase.PR removed, schema bumped to 1.2, Contract._migrate_schema_version_to_1_2 exists as a model_validator(mode="wrap") shim, _open_context_pr_at_implement_start, create_slice_pr, GatewayClient.list_open_prs / _lookup_open_pr, _cleanup_agent_outputs_for_pr, egg/<pipeline_id>/work branch pattern, deleted _should_skip_pr_phase_auto_pr skip gate, PRMetadata field deletions). All twelve claims are accurate.
That said, the diff leaves three stale references in the very section it's rewriting, and three diagram regressions. Per the project's pre-existing-issues rule, these need to be fixed here since the PR is already in this area and its stated mission is to clean up the PR-phase removal docs.
Blocking — stale references inside the rewritten section
docs/guides/sdlc-pipeline.md:557 — Step 5 of "Implement and PR-Based Review" still says:
- Review feedback — The
reusable-review.ymlworkflow provides line-level code review comments on the draft PR
But the context PR opens with draft=False (default of GatewayClient.create_pr, see orchestrator/gateway_client.py:1533) — _open_context_pr_at_implement_start never passes draft=True. The PR rewrites step 1 ("Context PR opened") and removes step 7 ("PR finalization … the draft PR is marked ready for human merge") but leaves the adjacent step 5 inconsistent. This is exactly the cleanup the PR exists to perform.
docs/guides/sdlc-pipeline.md:50 — Same stale terminology earlier in the file:
Code reviews are performed by the existing PR review workflow (
reusable-review.yml), which provides line-level feedback on draft PRs created during the implement phase.
Two things are wrong: (a) the context PR is not a draft, (b) it is no longer "created during the implement phase" by the implementer — it's opened up-front by the orchestrator at the plan→implement boundary, before any agent runs.
docs/guides/sdlc-pipeline.md:547 — Dangling reference to a deleted concept:
This ensures agents always work with the latest codebase and conflicts are resolved before work begins, not at PR finalization time.
There is no "PR finalization time" anymore — that step was the one this PR just removed from the numbered list immediately below. Drop "not at PR finalization time" (the remaining sentence still works) or rephrase against the new topology.
Blocking — diagram regressions
docs/architecture/sdlc-pipeline.md:47-48 — Dangling 4th down-arrow. The original diagram had 4 phase boxes each with its own │ … ▼ flowing into Contract State. The new diagram has 3 boxes but kept all 4 arrows:
│ │ │ │ │ │
│ ▼ ▼ ▼ ▼ │
The 4th column at ~col 64 used to point down from the PR box (now deleted), so it now lands under the right half of the wide Implement box. Visually it reads as "the Implement box has two outputs," which it doesn't. Drop the 4th │ / ▼.
docs/architecture/sdlc-pipeline.md:45 — Off-by-one box edge. The Implement box's third inner row is 74 chars while the surrounding box lines (42, 43, 44, 46) are 75 chars — the closing │ │ is short one space, so the inner-box right edge drifts left by one column on that row only.
docs/guides/sdlc-pipeline.md:70-82 — Wobbling right border + missing arrowhead.
- Line lengths drift: lines 71–74 are 74 chars, lines 75–76 are 74, lines 77–80 are 73, line 82 is 74, while the rest are 75. The outer
│wobbles by 1–2 columns across those rows. - Line 70 shows
│ ▼ ╎ ▼ │ │— the Refine and Plan columns get a clean▼arrowhead into the REVIEW box, but the Implement column has just│(no arrowhead). The flow└── IMPLEMENT ──┘ → │ → ┌── REVIEW ──┐should be└── IMPLEMENT ──┘ → ▼ → ┌── REVIEW ──┐, matching the other two columns.
Non-blocking
README.md:100-101— "Human merges slice PRs" is technically incomplete: humans merge the context PR and each slice PR (the cascade is documented indocs/guides/concurrent-execution.md:949— context merges first, then slice-1, then slice-2…). Not wrong, but you could lift "Human merges context + slice PRs" without making the label longer.
What I did not re-check
I did not re-run the test suite per the review-conventions guidance. I read tests/docs/test_context_pr_doc_terminology.py (which the PR body cites as already updated upstream) — note that the test file pins terminology in three other files (docs/architecture/orchestrator.md, docs/reference/orchestrator-cli.md, docs/guides/concurrent-execution.md), not the three files this PR modifies, so its passing status does not validate the new wording here.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- docs/guides/sdlc-pipeline.md:50 — Replace stale "draft PRs created during the implement phase" with accurate description (context PR opened by orchestrator at plan→implement boundary). - docs/guides/sdlc-pipeline.md:547 — Drop dangling "not at PR finalization time" reference to a now-deleted phase. - docs/guides/sdlc-pipeline.md:557 — Step 5 now says "context PR" instead of "draft PR" (the context PR is opened with draft=False). - docs/guides/sdlc-pipeline.md:70-82 — Restore consistent 75-char width across diagram rows and add missing ▼ arrowhead on the IMPLEMENT→REVIEW column (matching the Refine/Plan columns). - docs/architecture/sdlc-pipeline.md:45 — Pad the third inner row of the Implement box to 75 chars (was 74; right edge drifted). - docs/architecture/sdlc-pipeline.md:47-48 — Drop the 4th down-arrow that previously fed from the now-removed PR box; the wide Implement box no longer reads as having two outputs. - README.md:100-101 — Clarify the human-merge label as "context + slice PRs" to reflect that both surfaces require human merge. Authored-by: egg
|
Addressed all six blocking items plus the non-blocking suggestion in 6855d38.
Verified all diagram row lengths post-fix with
The pre-existing mypy errors in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns in the re-review delta. The new commit (6855d38) only adjusts diagram column widths/arrowheads in two pipeline docs and refines "draft PR" → "context PR" wording — no agent prompts, output formats, workflows, or sandbox/gateway boundaries are touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit 6855d38f0f03eadb43a488e5bcd27462e8cfe9dd
Previous blockers — all six addressed correctly
Verified each fix against the working tree:
| Item | Status |
|---|---|
docs/guides/sdlc-pipeline.md:50 — stale "draft PRs created during the implement phase" |
✅ Rewritten to "the context PR opened by the orchestrator at the plan→implement boundary" |
docs/guides/sdlc-pipeline.md:547 — dangling "not at PR finalization time" |
✅ Clause dropped |
docs/guides/sdlc-pipeline.md:557 — Step 5 "draft PR" |
✅ Reads "context PR" |
docs/architecture/sdlc-pipeline.md:45 — Implement-box right edge 74 vs 75 |
✅ Padded to 75; confirmed all rows uniform with len(line) per-line check |
docs/architecture/sdlc-pipeline.md:47-48 — dangling 4th │ / ▼ |
✅ Removed; the wide Implement box now has a single output column |
docs/guides/sdlc-pipeline.md:70-82 — wobbling border + missing IMPLEMENT→REVIEW arrowhead |
✅ All rows now 75 chars; line 70 col 47 carries a ▼ matching the Refine/Plan columns |
README.md:100-101 (non-blocking) — "slice PRs" |
✅ Reads "context + slice PRs" |
New blocking finding — docs assert a code path that does not exist
Two passages added by this PR (commit 183ee9746…, present unchanged at HEAD) describe a cleanup operation that the orchestrator does not actually run:
1. docs/guides/sdlc-pipeline.md:968 (under "Context PR (opened at implement-phase start, #2777)"):
Agent-outputs cleanup: At implement-phase start the orchestrator removes
.egg-state/agent-outputs/from the branch via_cleanup_agent_outputs_for_pr(). These files are ephemeral coder→tester handoff artifacts that the tester has already consumed. Cleanup is best-effort — failures are logged but do not block PR creation.
2. docs/guides/sdlc-pipeline.md:1617 (under "Context-PR / Slice-PR State File Troubleshooting"):
Before opening the context PR at the plan→implement boundary, the orchestrator runs several operations: agent-outputs cleanup, BRC history writing, and a final push. Each operation has diagnostic INFO-level logging to help identify failures.
_cleanup_agent_outputs_for_pr is defined at orchestrator/routes/pipelines.py:8378, but it has zero production call sites. The only non-definition references are inside the function body (log strings) and orchestrator/tests/test_cleanup_agent_outputs_for_pr.py. Verified via:
grep -rn "_cleanup_agent_outputs_for_pr" orchestrator/ shared/ gateway/ sandbox/
git blame shows the function was introduced in 18591b7 ("Persist BRC history for slice-1 (#2548)") which also added a single call site at PR-phase entry — see the source comment "Drop .egg-state/agent-outputs/ before any other PR-phase commits." That call site was deleted by b9081c7 (#2891, the PR-phase removal merge): the parent 8480ab6 has 10 references to the symbol, the other parent 34279b5 has 9, and the merge result has 9. The function survived as orphaned dead code; the docstring still says "at PR-phase entry," which is the giveaway. _open_context_pr_at_implement_start (which is what runs at the new plan→implement boundary) does not invoke it — see steps 1–4 in its docstring at pipelines.py:10112-10130.
So the bullet at line 968 and the paragraph at line 1617 both invent a behavior that does not happen. An operator who finds an unexpected .egg-state/agent-outputs/ file in a slice PR diff will read these docs, conclude that cleanup must have crashed, and search for _cleanup_agent_outputs_for_pr: … log lines that the orchestrator will never emit.
This is a doc bug introduced by this PR (neither claim exists on main — verified with git show origin/main:docs/guides/sdlc-pipeline.md | grep -E "Agent-outputs cleanup|agent-outputs cleanup, BRC" returns nothing). The whole point of the PR is to make the post-PR-phase-removal docs accurate; documenting cleanup that does not run defeats that goal.
Fix (doc-only, in scope):
- Delete the "Agent-outputs cleanup" bullet at
docs/guides/sdlc-pipeline.md:968. - Delete or rewrite the paragraph at
docs/guides/sdlc-pipeline.md:1617. The "BRC history writing, and a final push" half is genuinely the per-phase write path documented in §"Worktree State Synchronization" — keep that if you want, but drop the "agent-outputs cleanup" claim.
(Separately, the orphaned _cleanup_agent_outputs_for_pr function and its test file should likely be deleted or re-wired into _open_context_pr_at_implement_start. That is a code change and out of scope for a doc PR — but worth filing a follow-up issue so the orphaned function does not stay buried in pipelines.py advertising the wrong behavior via its docstring.)
What I did not re-check
Did not re-run the test suite per the review-conventions guidance. The third-party reviewer (agent-mode-design) at 2026-05-30T23:41:47Z approved on scope grounds (no agent prompts / workflows changed) — that approval is correct for their lens but does not validate accuracy of the content.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…allers Reviewer flagged that two passages introduced by this PR describe `_cleanup_agent_outputs_for_pr()` running at implement-phase start, but the function has zero production call sites since #2891 deleted the PR-phase entry that used to call it. Removing the bullet at line 968 and the 'agent-outputs cleanup' clause at line 1617 so the docs no longer assert behavior that does not happen. The orphaned function itself is tracked for code cleanup in #2899.
|
Addressed the new blocking finding from the re-review of
Verified the reviewer's claim against the code: The pre-existing mypy errors in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns in the re-review delta. The new commit (1758449) only removes one bullet and one phrase from docs/guides/sdlc-pipeline.md to drop a stale claim about an _cleanup_agent_outputs_for_pr() code path. No agent prompts, output formats, workflows, or sandbox/gateway boundaries are touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit 17584491845517c6b05e743581628efa7af8faa6
Previous blocking finding — addressed
The new commit (1758449) removes exactly the two passages flagged in my prior review:
| Location | Status |
|---|---|
docs/guides/sdlc-pipeline.md:968 — "Agent-outputs cleanup" bullet under "Context PR (opened at implement-phase start, #2777)" |
✅ Bullet deleted |
docs/guides/sdlc-pipeline.md:1617 — "agent-outputs cleanup, BRC history writing, and a final push" enumeration |
✅ Reads "BRC history writing and a final push" |
Verification
Re-confirmed that _cleanup_agent_outputs_for_pr still has zero production call sites:
$ grep -rn "_cleanup_agent_outputs_for_pr" orchestrator/ shared/ gateway/ sandbox/
The only non-definition references remain inside the function body's log strings (orchestrator/routes/pipelines.py:8378-8504) and orchestrator/tests/test_cleanup_agent_outputs_for_pr.py. The orphaned-function cleanup is properly tracked in #2899, which lays out the delete-vs-rewire decision for operators.
Surrounding context
Read the two affected sections post-edit. Both flow cleanly:
- Context PR section (965–969): Bullets now cover the open mechanism, title/description sourcing, draft preservation, and failure handling — internally consistent with no dangling references.
- Troubleshooting preamble (1612–1616): The sentence "Before opening the context PR at the plan→implement boundary, the orchestrator runs BRC history writing and a final push" is now accurate and matches the troubleshooting steps that follow (
_write_brc_history,_commit_statefiles_to_worktree, push reconcile).
Other agent-outputs references checked
grep -rn "agent-outputs" docs/ returns ~30 hits across agent-roles.md, agent-development.md, agent-tools.md, slice-dag.md, orchestrator.md, testing.md, concurrent-execution.md, sdlc-pipeline.md, etc. All remaining references describe the directory (.egg-state/agent-outputs/) as the agent-handoff artifact location — none assert a cleanup behavior. Nothing else needs changing.
Scope
Doc-only delta of -2 / +1 lines in a single file. No code touched.
Verdict
LGTM. The blocking finding from the prior review is resolved correctly and minimally, with the orphan-function follow-up properly tracked in #2899.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…in docs/guides/sdlc-pipeline.md Kept main's new plan pre-flight validation paragraph (#2777) and used this PR's updated PR-metadata wording describing the context-PR opened at the plan→implement boundary.
Conflict Resolution SummaryResolved merge conflict with
DetailSingle conflict near line 873 in the "Task Population" section. Main (#2896) added a new "Plan pre-flight validation at plan→implement" paragraph but still carried the old wording in the following PR-metadata paragraph ("auto-create the PR when the implement phase completes"). This PR rewrites that paragraph to describe the new context-PR topology ("open the context PR at the plan→implement boundary"), which is the whole point of the change. Resolution: keep both paragraphs — main's new pre-flight description (purely additive) followed by this PR's updated PR-metadata wording. No content from either side was dropped. Verification
Please review: the relative ordering of the two paragraphs in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns in the re-review delta. Since 17584491845517c6b05e743581628efa7af8faa6, the only new commit is bc64ea6 — a merge of origin/main resolving a conflict in docs/guides/sdlc-pipeline.md. The substantive delta is +2 / -0 lines documenting the new orchestrator-side plan pre-flight validator (from #2777, brought in via main). No agent prompts, output formats, workflows, or sandbox/gateway boundaries are affected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit bc64ea68f0944abf9e305fe7a232551b9c131197
Delta since my prior review at 17584491: a single merge commit picking up 44d40cd2b from main (which landed #2896 — the plan pre-flight validator + hard-required context PR opener docs). Net textual change in the PR branch is +2 / -0 lines on docs/guides/sdlc-pipeline.md.
Merge resolution — clean
| Check | Result |
|---|---|
| Conflict resolution preserved this PR's "open the context PR at the plan→implement boundary" wording (line 875) | ✅ |
Main's additive pre-flight paragraph (line 873) accurate vs. code (orchestrator/routes/phases.py:348-514, shared/egg_contracts/plan_parser.py:1736-1821) |
✅ — each claim (422 on missing fields with full missing_fields list, force=true bypass, 500 preflight_unavailable on infra errors, silent skip on missing draft path/file) maps to the code |
| Paragraph ordering (pre-flight before PR-metadata) matches runtime order (validator → populate → opener) | ✅ |
| Lint/test infra unchanged | ✅ — pre-existing mypy errors in shared/egg_agent/client.py:387-388 remain on main (not touched) |
Blocking — pre-existing inaccuracy in the paragraph this PR rewrites
docs/guides/sdlc-pipeline.md:875 — the second half of the PR-metadata paragraph documents a fall-through path that does not exist in the new opener:
If neither the contract nor the plan draft on disk contains a
pr.title, the PR falls through to a stub (issue title or pipeline ID) and is opened as a draft with a warning banner listing parse failures so reviewers can diagnose and repair before merging.
This is wrong. _open_context_pr_at_implement_start (orchestrator/routes/pipelines.py:10101-10375) — which is what the rewritten first sentence now (correctly) points at — handles a missing/empty contract.pr.title by raising ContextPrCreationError(reason="missing_pr_metadata") at pipelines.py:10304-10308:
if contract.pr is None or not (contract.pr.title or "").strip():
raise ContextPrCreationError(
"contract.pr.title is missing or empty; cannot open context PR",
reason="missing_pr_metadata",
)There is no stub-title fallback, no plan-draft second look (the helper that backed that — _pr_metadata_from_plan_draft at pipelines.py:9675-9732 — has zero call sites in the post-PR-phase-removal tree; verified with grep -n "_pr_metadata_from_plan_draft\|_build_pr_body" orchestrator/routes/pipelines.py returning only the definition and a stale docstring reference to a non-existent _build_pr_body). The single create_pr call site for the context PR (pipelines.py:10313) does not pass draft=True, and grep -rn "draft=True" orchestrator/ shared/ returns nothing. Per the upstream guarantee, advance_phase plan→implement with force=false 422s on missing pr.title via the very pre-flight validator the paragraph above this one documents — so on the normal path the case the second half describes is already structurally impossible, and on the force=true path the failure mode is the typed exception, not a stub draft PR with a warning banner.
The first sentence of this paragraph was rewritten by this PR in 183ee9746 ("auto-create the PR when the implement phase completes" → "open the context PR at the plan→implement boundary"), so this is exactly the paragraph the PR is purposefully cleaning up. The second half describes the deleted _finalize_pr_phase behavior (which did have a stub-draft fall-through with parse-warning banner, see the stale docstring at pipelines.py:9687-9694 mentioning "the stub tier" / "see #1975"). Carrying it over verbatim contradicts the new opener's actual behavior and defeats the PR's stated mission.
I missed this in my prior reviews — apologies. Per the repo's pre-existing-issues rule, since the PR is already in this paragraph and the rewrite is the whole point, it should be fixed here rather than deferred.
Fix (doc-only, in scope): delete or rewrite the final sentence. A faithful replacement matching the code:
If
contract.pr.titleis missing or empty when the opener runs, the pipeline fails withContextPrCreationError(reason="missing_pr_metadata")and is marked FAILED — the pre-flight validator atadvance_phaseplan→implement is the structural gate that prevents this on the normal path; the failure case is reachable only viaforce=truewith a malformed plan.
(or just drop the sentence — the pre-flight paragraph immediately above already covers the normal-path guarantee.)
Out of scope but worth noting
_pr_metadata_from_plan_draft (pipelines.py:9675-9732) is orphan dead code in the same shape as _cleanup_agent_outputs_for_pr (tracked in #2899). The PR-phase deletion in b9081c7e6 removed the caller and left the helper plus its stub-tier docstring behind. Probably worth folding into #2899 or filing a sibling follow-up — not a blocker for this PR.
What I did not re-check
Did not re-run the test suite per the review-conventions guidance. Verified the merge stat (+2 / -0 across one file) via git diff --stat 17584491..bc64ea68f.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The final sentence of the PR-metadata paragraph in docs/guides/sdlc-pipeline.md described a stub-title fall-through that opens the PR as a draft with a warning banner. That path belonged to the deleted _finalize_pr_phase and does not exist in _open_context_pr_at_implement_start (which raises ContextPrCreationError(reason="missing_pr_metadata") instead). Replaced with an accurate description of the actual failure path, referencing the pre-flight validator as the structural gate.
|
Addressed the new blocking finding from the re-review of
VerificationVerified the reviewer's claim against the code:
The pre-existing mypy errors in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
Delta since bc64ea6 is a single-line correction in docs/guides/sdlc-pipeline.md replacing the stale "falls through to a stub … opened as a draft" sentence with an accurate description of ContextPrCreationError(reason="missing_pr_metadata") and the pre-flight validator's role as the structural gate. Pure documentation accuracy fix, no impact on agent orchestration, prompts, or tool surfaces.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit 97645c9605d68c64f0d82ecd1d0fbf7dda6d22d9
Delta since my prior review at bc64ea68: a single one-line change to docs/guides/sdlc-pipeline.md:875 (the final sentence of the PR-metadata paragraph). All other surface area is unchanged.
Previous blocker — addressed correctly
The fictional stub-title fall-through ("falls through to a stub … opened as a draft with a warning banner") is gone. The new sentence correctly states there is no stub fallback and correctly cites the pre-flight validator as the structural gate on the normal path. ✅
New blocking finding — the replacement sentence has two factual errors
The new sentence at docs/guides/sdlc-pipeline.md:875:
The pre-flight validator above is the structural gate that ensures
contract.pr.titleis populated on the normal path; if the opener is reached with a missing or empty title (only possible viaforce=trueplan→implement with a malformed draft), it raisesContextPrCreationError(reason="missing_pr_metadata")and the pipeline is marked FAILED — there is no stub-title fall-through.
Both bolded clauses are wrong. This wording is a faithful transcription of the "faithful replacement matching the code" I suggested in my previous review — it isn't, and I should have verified the code before suggesting it. Apologies. But per the project's pre-existing-issues rule, the wrong wording still needs to be corrected here, since the PR is rewriting exactly this sentence.
Error 1 — the pipeline is NOT marked FAILED
On the canonical advance_phase REST path (orchestrator/routes/phases.py:610-637), when _open_context_pr_at_implement_start raises ContextPrCreationError(reason="missing_pr_metadata"):
return make_error_response(
f"Context PR could not be opened: {ctx_err}",
422,
details={"reason": ctx_err.reason},
reason="context_pr_open_failed",
)The handler returns HTTP 422 with reason: context_pr_open_failed and the pipeline stays in PLAN — the inline comment at phases.py:622-625 makes this explicit:
The state-lock-protected mutation below has NOT yet run at this point, so the pipeline remains in PLAN / its prior status — no orphan state.
No PipelineStatus.FAILED mutation occurs on this path. Grep confirms: the only PipelineStatus.FAILED writes in routes/phases.py are at lines 1610/1614, inside the signal_error endpoint — a separate operator/agent-driven endpoint, not a consequence of missing_pr_metadata. On the soft-fail backstop paths (pipelines.py:16005, 21846, 23210, 24227), the exception is logger.warning'd and execution continues — again, no FAILED mutation.
An operator reading the current doc will believe the pipeline is dead and needs hard-reset recovery when in fact a simple plan fix + re-advance_phase clears the 422.
Error 2 — the missing-title case is NOT "only possible via force=true"
The opener gate at phases.py:610 is if target_phase == PipelinePhase.IMPLEMENT and not force: — so on force=true the opener at advance_phase isn't called at all; the pipeline transitions to IMPLEMENT and the runner-side soft-fail backstop (pipelines.py:21845) calls the opener instead, where the raise is swallowed.
The opener-reached-with-empty-title case on the non-force canonical path is also reachable, by the staircase the doc itself implies two paragraphs above:
- Validator silently skips (draft path undeclared or draft file absent on disk — see
phases.py:455-465) - Populate runs but produces a non-
POPULATEDoutcome (PARSE_FAILED/DRAFT_MISSING/ etc.). The call site atphases.py:552-557onlylogger.warnings and continues, socontract.pr.titlestays empty. - Opener at
phases.py:617runs against the emptycontract.pr.title→ raisesmissing_pr_metadata→ 422.
So the "(only possible via force=true...)" parenthetical excludes a legitimate non-force reachability path.
Fix (doc-only, in scope)
A version that matches what the code actually does on each path:
The pre-flight validator above is the structural gate that ensures
contract.pr.titleis populated on the normal path. If the opener is reached with a missing or empty title — reachable when the validator silently skips (no plan draft on disk) and the plan-exit populate step fails to writecontract.pr, or viaforce=truewhich bypasses both the validator and the canonical opener call — it raisesContextPrCreationError(reason="missing_pr_metadata"). On the canonicaladvance_phasepath the orchestrator returns HTTP 422 withreason: context_pr_open_failedand the pipeline remains in its prior phase (PLAN); on the runner-sideforce=truebackstops the raise is logged and execution continues without a context PR, leaving slice PRs without a base to cascade onto. There is no stub-title fall-through on any path.
(If that's too long, the shorter accurate form is: drop the "only possible via force=true" parenthetical, replace "marked FAILED" with "the canonical advance_phase path returns 422 and the pipeline remains in PLAN; the soft-fail runner backstops log-and-continue".)
What I did not re-check
Did not re-run the test suite per review-conventions guidance. Verified delta size with git diff bc64ea6..97645c9 --stat (one file, +1/-1). The agent-mode-design reviewer's approval at 2026-05-31T00:43:14Z is correct for their scope (no agent prompts / workflows / sandbox surface touched).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The prior sentence claimed the pipeline is marked FAILED and that the missing-title case is only reachable via force=true. Both are wrong: - On the canonical advance_phase path the handler returns 422 with reason context_pr_open_failed and the pipeline stays in its prior phase (PLAN) — see orchestrator/routes/phases.py:632-637. - The case is also reachable on the non-force path when the validator silently skips (no plan draft on disk) and the populate step fails to write contract.pr — phases.py:444-465 + 552-557 log-and-continue. - The runner-side force=true backstops (pipelines.py:16004-16013, 21845-21861, plus two more) log-and-continue rather than marking the pipeline FAILED.
|
Addressed the new blocking finding from the re-review of
VerificationRe-confirmed the reviewer's claims against the code:
The new sentence uses the reviewer's "faithful replacement" wording verbatim, with both factual errors corrected. The pre-existing mypy errors in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. The delta since 97645c9 is a single doc-accuracy correction in docs/guides/sdlc-pipeline.md clarifying the context-PR opener's failure modes (HTTP 422 with context_pr_open_failed on the canonical advance_phase path; runner-side force=true backstops log-and-continue). No agent prompts, workflows, sandbox surface, or LLM API touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit 6a1433a9179d42dc83125e57f0ca0dcc28fb44c8
Delta since my prior review at 97645c9: a single one-line change to docs/guides/sdlc-pipeline.md:875 (the final sentence of the PR-metadata paragraph). All other surface area is unchanged.
Previous blockers — both addressed correctly
| Item | Status |
|---|---|
Error 1 — sentence claimed pipeline is marked FAILED on missing_pr_metadata raise |
✅ Rewritten to "the orchestrator returns HTTP 422 with reason: context_pr_open_failed and the pipeline remains in its prior phase (PLAN)". Verified against routes/phases.py:632-637 (returns 422 with reason="context_pr_open_failed") and the inline comment at phases.py:622-625 ("the state-lock-protected mutation below has NOT yet run … no orphan state"). |
Error 2 — sentence claimed the missing-title case is "only possible via force=true" |
✅ Rewritten to enumerate both reachability paths: non-force canonical (validator-skip + populate-fail) and force=true. Verified against phases.py:444-465 (validator skips silently on _draft_rel is None or absent file), phases.py:552-557 (populate non-POPULATED outcome logs-and-continues), and phases.py:610 (force=true bypasses both validator and canonical opener). |
I also spot-checked the four runner-side soft-fail backstops cited in the commit message:
pipelines.py:16004-16013— slice-loop entry:logger.warning+ continue ✅pipelines.py:21845-21861— implement-entry backstop:logger.warning+ continue ✅pipelines.py:23209-23218—_run_pipelineauto-advance:logger.warning+ continue ✅pipelines.py:24226-24241— HITL-resume site:logger.warning+ continue ✅
None of the four mutate PipelineStatus.FAILED.
Non-blocking — two wording imprecisions in the new sentence
These were also present in the "faithful replacement matching the code" wording I gave you in the prior review (the author copied it verbatim and that's fair) — flagging here so a follow-up doc PR can tighten them up if you care. They are not substantive enough to NACK on:
-
"on the runner-side
force=truebackstops" — the four backstops above are not exclusivelyforce=truebackstops. The code comments themselves consistently scope them more broadly: the implement-entry backstop covers "inline_run_pipelineauto-advance and the HITL-approval recovery instart_pipeline" (pipelines.py:21825-21834); the auto-advance backstop exists because "auto-advance does NOT route throughroutes/phases.py:advance_phase" (pipelines.py:23195-23206), which is a natural plan-exit path, not force=true; the HITL-resume site fires when "an operator-resumed pipeline would silently strand its slice stack" (pipelines.py:24214-24218). A tighter wording is "runner-side soft-fail backstops" or "the four runner-side backstops" — matches the shorter alternative form I gave in the prior review. -
"leaving slice PRs without a base to cascade onto" — slice PRs do have a base:
_resolve_slice_base_branch(pipelines.py:10398-10447) resolves topipeline_branch(egg/<id>/work) for root slices when there is noparent_branch_at_creation. The actual operator-facing failure mode is that the slice stack is stranded onegg/<id>/workwith no integrating context PR — i.e. nothing merges the work branch intomain. The code's own comments use that framing consistently (pipelines.py:21841,23199-23200,24217-24218all say "strand[ed] slice stack onegg/<id>/work"). The doc's "without a base" wording diverges from the code's vocabulary and could confuse an operator hunting for the symptom.
What I did not re-check
Did not re-run the test suite per the review-conventions guidance. Verified the delta with git diff 97645c9..6a1433a --stat (single file, +1 / -1). The agent-mode-design reviewer's approval at 2026-05-31T01:14:01Z is correct for their scope (no agent prompts, workflows, sandbox surface, or LLM API touched).
LGTM with the two non-blocking wording suggestions above.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Address two non-blocking wording imprecisions flagged on docs/guides/sdlc-pipeline.md:875: 1. 'on the runner-side `force=true` backstops' overstated the scope — the four backstops (slice-loop entry, implement-entry, auto-advance, HITL-resume) cover non-force soft-fail paths too. Replaced with 'runner-side soft-fail backstops' to match the code's own framing. 2. 'leaving slice PRs without a base to cascade onto' diverged from the code's vocabulary — slice PRs do have a base (`pipeline_branch` resolved by `_resolve_slice_base_branch`); the operator-facing failure is that the slice stack is stranded on `egg/<pipeline_id>/work` with no integrating context PR. Replaced with the stranded-stack framing consistent with the inline comments at pipelines.py:21841, 23199-23200, 24217-24218.
|
Addressed both non-blocking wording suggestions from the re-review of
VerificationRe-confirmed both reviewer claims against the code:
The pre-existing mypy errors in — Authored by egg |
|
egg feedback addressed. View run logs 24 previous review(s) hidden. |
There was a problem hiding this comment.
No agent-mode design concerns in the re-review delta. The new commit (62c0ef8) only tightens one sentence's wording in docs/guides/sdlc-pipeline.md — refining the description of the runner-side soft-fail backstop ("force=true backstops" → "soft-fail backstops"; "slice PRs without a base to cascade onto" → "slice stack stranded on egg/<pipeline_id>/work with no integrating context PR"). No agent prompts, output formats, workflows, or sandbox/gateway boundaries are touched.
— Authored by egg
There was a problem hiding this comment.
Re-review of commit 62c0ef8284a221cf6bfa5dff8374040df7bd3a37
Delta since my prior review at 6a1433a: a single one-line change to docs/guides/sdlc-pipeline.md:875 (the final sentence of the PR-metadata paragraph). All other surface area is unchanged. Confirmed via gh api repos/jwbron/egg/compare/6a1433a...62c0ef8 → one file, +1 / -1.
Previous non-blocking suggestions — both addressed correctly
| Item | Status |
|---|---|
"on the runner-side force=true backstops" overstated the scope (the four backstops cover non-force soft-fail paths too) |
✅ Now reads "on the runner-side soft-fail backstops". Matches the code's own framing — pipelines.py:21823-21834 (implement-entry backstop docstring covers "inline _run_pipeline auto-advance and the HITL-approval recovery in start_pipeline"), pipelines.py:23193-23206 ("auto-advance does NOT route through routes/phases.py:advance_phase" — a natural plan-exit path, not force=true), pipelines.py:24214-24218 (HITL-resume site fires on operator-resumed pipelines). None of these are force=true-only. |
| "leaving slice PRs without a base to cascade onto" diverged from the code's vocabulary | ✅ Now reads "leaving the slice stack stranded on egg/<pipeline_id>/work with no integrating context PR". Matches the inline-comment phrasing at pipelines.py:21841 ("stranded slice stacks on egg/<id>/work"), pipelines.py:23200 ("leaving the slice stack stranded on egg/<id>/work"), and pipelines.py:24217-24218 ("operator-resumed pipeline would silently strand its slice stack on egg/<id>/work"). |
Verified the rewritten sentence still parses cleanly in context
Read the surrounding paragraph at docs/guides/sdlc-pipeline.md:870-877. The new wording integrates without rough edges — the "On the canonical advance_phase path … ; on the runner-side soft-fail backstops …" parallelism reads cleanly, and the "stranded on egg/<pipeline_id>/work with no integrating context PR" branch correctly identifies the operator-facing failure symptom (rather than the previous misleading "without a base" framing that contradicts _resolve_slice_base_branch).
No new issues introduced
The single-line change does not touch:
- Diagrams (still 75-char-uniform after the prior round)
- The "Context PR" / "Context-PR / Slice-PR State File Troubleshooting" sections (the agent-outputs cleanup claim raised on the
6855d38re-review remains a separate concern but was not introduced by this PR — see commit183ee97history; outside the scope of this commit's delta) - README.md or
docs/architecture/sdlc-pipeline.md
What I did not re-check
Did not re-run the test suite per the review-conventions guidance. CI's wait-for-checks is the gate. The agent-mode-design reviewer's approval at 6a1433a on scope grounds (no agent prompts, workflows, sandbox surface, or LLM API touched) still holds for this one-line wording change.
LGTM.
— Authored by egg
|
egg agent-mode-design completed. View run logs 24 previous review(s) hidden. |
|
egg review completed. View run logs 24 previous review(s) hidden. |
1 participant
Update documentation to reflect changes from #2891 (slice-2/4 of issue #2777):
context_branch/context_title/context_descriptionfromPRMetadata; fix BRC history file note.The
PipelinePhase.PRenum row was hard-removed in #2777 (cq-4 / TASK-2-2).IMPLEMENTis now terminal. The context PR (egg/<pipeline_id>/work → main) opens automatically at the plan→implement boundary via_open_context_pr_at_implement_start. Contract schema bumped to 1.2 with a wrap-mode migration shim that strips the three removedPRMetadatafields from pre-1.2 contracts on load.Triggered by: #2891
Issue: none
Test Plan
tests/docs/test_context_pr_doc_terminology.py) was already updated in the source commitAuthored-by: egg