docs: update sdlc-pipeline arch for #2685 context PR + PR phase [doc-updater] - #2697
Conversation
…nges Document that the context PR now includes the contract JSON (resolved via the contract loader so integer issue identifiers use the canonical issue-<N>.json path), and that the PR phase suppresses the legacy <pipeline_branch> → main auto-PR in slice-DAG mode where per-slice PRs already exist stacked on the context PR. Authored-by: egg
There was a problem hiding this comment.
No agent-mode design concerns. Docs-only update to architecture reference reflecting already-merged behavior changes from #2694.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Docs-only update to docs/architecture/sdlc-pipeline.md. Reviewed against the code that #2694 actually landed.
Verification of the doc claims
The new text is factually correct:
-
Contract JSON included via the loader. Matches
_gather_context_pr_filesinorchestrator/routes/pipelines.py:9976-10079. The static globs at_STATIC_CONTEXT_PR_FILE_GLOBS(lines 9710-9717) deliberately omit the contract; the loader-based resolution at lines 10039-10077 (get_contract_pathcanonical +_legacy_contract_pathfallback) is what picks upissue-<N>.json. The doc's "rather than a bare{identifier}.jsonglob" framing matches the comment at lines 9718-9725 verbatim. -
Slice-DAG auto-PR suppression. Matches
_should_skip_pr_phase_auto_pratorchestrator/routes/pipelines.py:8337-8403. Threshold isslice_count > 1(line 8401), babysit short-circuits unconditionally (lines 8375-8376), and contract-load failures fail safe to running the legacy path (lines 8388, 8398) — single- and zero-slice contracts still get the legacy auto-PR. The doc's "multiple slices" wording is accurate for the> 1threshold. -
Stacking direction. Slice-1 stacks on the context branch (
_run_one_slice_innerlines 15334-15346); child slices stack on parent slices, not the context branch directly. The doc's "per-slice PRs already exist stacked on the context PR" is a fine high-level summary even though the stack roots through slice-1.
Non-blocking — sibling docs are now out of sync with #2685
The PR only updates sdlc-pipeline.md, but the same artifact list is enumerated in two other docs that #2685 made stale. Both still claim the context PR ships only the refine/plan drafts, BRC history, and agent transcripts — no contract JSON:
docs/architecture/orchestrator.md:181-186— "Context PR (slice-aware mode)" section, enumerated bullet list of committed artifacts. Missing the contract JSON entry.docs/guides/concurrent-execution.md:960-972— same enumerated list under "Context PR" mechanics. Missing the contract JSON entry.
Additionally, orchestrator.md:150-161 ("Special case: PR phase") describes the auto-PR behavior unconditionally — "the orchestrator auto-creates the PR via GatewayClient.create_pr()" — without mentioning the slice-DAG skip. A one-line note that the auto-PR is suppressed in slice-DAG mode (with a link to this section in sdlc-pipeline.md) would close that gap.
These are non-blocking — what the PR does write is correct, and the rule that pre-existing inconsistencies in code the PR touches must be fixed in-PR doesn't strictly apply to sibling files. But the PR's purpose is to bring docs in sync with #2685, and operators landing on the more detailed enumerations in orchestrator.md or concurrent-execution.md will still get a misleading picture of what's on the context PR diff. Worth either expanding this PR's scope or filing a follow-up.
Minor
- The compound parenthetical at lines 145-151 chains two #2593 sub-clauses and a #2685 sub-clause inside one set of parens. Readable, but splitting #2685 onto its own short paragraph after the paren-close would make the slice-DAG behavior easier to find when grep-reading the file.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reviewer flagged that the original PR only updated sdlc-pipeline.md but the same context-PR artifact list is enumerated in two other docs that #2685 made stale, and the PR-phase auto-PR is described unconditionally in orchestrator.md without mentioning the slice-DAG skip: - docs/architecture/orchestrator.md: add a 'contract JSON resolved via get_contract_path' bullet to the enumerated context-PR artifact list, and a paragraph under 'Special case: PR phase' noting the slice-DAG auto-PR suppression (_should_skip_pr_phase_auto_pr) and the babysit / contract-load fail-safe behaviors. - docs/guides/concurrent-execution.md: same contract JSON bullet on the matching enumerated list under 'Context PR is opened first'. - docs/architecture/sdlc-pipeline.md: split the trailing parenthetical so the #2685 sub-clause is its own paragraph (reviewer nit on the compound parenthetical chaining two #2593 and one #2685 sub-clause inside one set of parens). Simplify the earlier mention of the contract JSON in the artifact line to point at the new paragraph, avoiding duplication of the loader rationale. Authored-by: egg
|
Thanks for the careful read. Addressed in commit Per-item dispositionSibling docs out of sync (
Compound parenthetical at — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of commit 8a913aa against the prior eb35904 review.
Prior feedback dispositions verified
The previous (non-blocking) review identified three items. All three are now addressed in-PR:
docs/architecture/orchestrator.md:181-186missing contract JSON bullet → line 189 now has the bullet. Wording matches the parallel entry insdlc-pipeline.mdand the_gather_context_pr_filescomment atorchestrator/routes/pipelines.py:9718-9725. ✓docs/architecture/orchestrator.md:150-161unconditional auto-PR description → line 159 adds a paragraph noting slice-DAG suppression (len(contract.slices) > 1), babysit short-circuit, and the contract-load fail-safe. The[Context PR](#context-pr-slice-aware-mode-2548)anchor resolves correctly to the## Context PR (slice-aware mode, [#2548](...))heading at line 178. ✓docs/guides/concurrent-execution.md:960-972missing contract JSON bullet → lines 973-980 add the entry with the same loader / canonical-path framing. ✓docs/architecture/sdlc-pipeline.md:145-151compound parenthetical → the #2685 sub-clause is now its own**#2685 added two follow-on behaviors:**paragraph at lines 149-156, and the earlier in-paragraph mention at line 136 was trimmed to point at the new paragraph rather than duplicate the loader rationale. ✓
Verification of new claims against code
I re-walked each new code reference against the live tree:
_should_skip_pr_phase_auto_pratorchestrator/routes/pipelines.py:8337-8403— babysit short-circuit (8375-8376),slice_count > 1threshold (8401), contract-load fail-safe returning(False, None)(8388, 8398). The doc's "babysit-pr mode short-circuits the same gate because the PR already exists" and "Contract-load failures fail safe to running the legacy path" match the helper docstring and behavior._STATIC_CONTEXT_PR_FILE_GLOBSatorchestrator/routes/pipelines.py:9710-9717and the inline comment at 9718-9725 explaining the intentional contract omission. The new docs paraphrase this comment accurately._gather_context_pr_filescontract resolution atorchestrator/routes/pipelines.py:10039-10077— canonicalget_contract_pathfirst, then_legacy_contract_pathfallback. Matches the docs' "canonicalissue-<N>.json... legacy bare<N>.jsonas a fallback" framing.- Call-site at
orchestrator/routes/pipelines.py:20271-20287confirms the helper drives theSkipping PR-phase auto-PR (slice-DAG mode: per-slice PRs exist)log line and the_finalize_pr_phase_failedgate.
Verdict
No new issues. Docs accurately reflect the #2685 code paths and the prior non-blocking feedback has been addressed in-PR rather than punted to a follow-up.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
docs: Update sdlc-pipeline architecture doc for #2685 changes
Update
docs/architecture/sdlc-pipeline.mdto reflect two behavioralchanges introduced in #2694 (Fix #2685):
Context PR artifacts: The context PR now includes the contract
JSON alongside analysis.md, plan.md, BRC history, and agent
transcripts. The path is resolved dynamically via the contract loader
so integer issue identifiers map to the canonical
issue-<N>.jsonform rather than a bare
{identifier}.jsonglob.PR phase auto-PR skip in slice-DAG mode: When a pipeline has
multiple slices (slice-DAG mode), the PR phase no longer opens the
legacy
<pipeline_branch> → mainauto-PR. Per-slice PRs alreadyexist stacked on the context PR, making the program-level auto-PR
redundant and confusing to reviewers.
Triggered by: a5d6856 (#2694)
Authored-by: egg