docs: update docs for slice-DAG implement phase [doc-updater] - #2231
Conversation
Update documentation to reflect changes from d1a2139 (PR #2220): - README.md: describe slice-DAG model in the Implement phase step - STRUCTURE.md: add slice_scheduler.py and stacked_pr_reconciler.py entries; update dependency_graph.py to mention its generification - orchestrator-cli.md: add the five new EGG_ORCH_*SLICE* and EGG_ORCH_STACKED_PR_RECONCILER_* env vars to the reference table - sdlc-pipeline.md: introduce slice-DAG paragraph in Multi-Agent Orchestration section with link to architecture/slice-dag.md - concurrent-execution.md: correct "sharing the pipeline branch" claim (implement now uses per-slice integration branches) Triggered by: #2220 Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Summary
Doc-only PR. I verified each new claim against the code: orchestrator/slice_scheduler.py, orchestrator/stacked_pr_reconciler.py, the five env-var helpers in orchestrator/env_config.py, the PEP-695 generic in shared/egg_contracts/dependency_graph.py, and the egg/issue-N/slice-M branch convention all match what the docs assert. The new prose in the README, sdlc-pipeline.md top section, and STRUCTURE.md is accurate.
The PR is not mergeable as-is because the two doc files it touches contradict themselves after the edits, and one new env-var entry oversells current behavior.
Blocking
1. docs/guides/concurrent-execution.md — same file now says both things
The PR adds at L5:
Implement-phase note: the implement phase no longer runs as a single team on a shared branch. Instead, the plan's tasks are split into a DAG of independent slices — each slice runs its own concurrent agent team on its own integration branch.
But it leaves all of the following untouched in the same file:
- L49 —
**Shared branch**: All agents operate on the pipeline's shared branch (e.g., \egg/issue-123`). Agents coordinate commits via the message bus to sequence their work and avoid conflicts.(under "Agent Startup Protocol", which listsimplement` in the role table at L47) - L862 —
- All agents push to the same shared pipeline branch (e.g., \egg/issue-{N}`)` (under "Per-Agent Worktree Isolation → Architecture") - L867 —
2. Agent pushes to the shared branch via the gateway - L877 —
Per-agent worktrees are created at phase start from the pipeline branch.
A reader who scrolls past the new note hits four restatements of the model the note is correcting, with the role table at L47 explicitly tying that wrong model to the implement phase. The PR description claims "corrected the 'all sharing the pipeline branch' claim" — the correction is the lead paragraph only; the rest of the file still teaches the old model.
Either:
- (a) Rewrite L49, L862–L877 to read "for refine and plan, all agents operate on the pipeline's shared branch; for implement, each slice has its own integration branch (
egg/issue-N/slice-M) — see Slice-DAG"; or - (b) Drop the new top-of-file note (which would leave the file accurate for refine/plan only and silent on implement, also acceptable but worse).
(a) is the right move since the PR's stated goal is to surface the slice-DAG model in this guide.
2. docs/guides/sdlc-pipeline.md — same problem in the same PR
The PR adds at L297:
Each slice has its own integration branch (
egg/issue-N/slice-M), agent team, BRC consensus, and stacked PR…
But L1362–L1369 still has an entire "Shared Pipeline Branch" subsection:
All concurrent agents operate on the pipeline's shared branch (e.g.,
egg/issue-999). Rather than each agent having an isolated worktree branch, all agents commit directly to a single shared history.
And L1441 still claims:
Commit conflicts: Since all concurrent agents share a single branch, agents coordinate commits via the message bus to avoid conflicts.
These flatly contradict L297. Both the "Shared Pipeline Branch" header and the "Commit conflicts" troubleshooting bullet need to be qualified to "refine and plan phases" or removed — leaving them implies the implement phase still works the way the deleted line did.
Non-blocking
3. docs/reference/orchestrator-cli.md env-var descriptions for EGG_ORCH_SLICE_LOCAL_MAX_CYCLES and EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES overstate current behavior
The PR adds:
EGG_ORCH_SLICE_LOCAL_MAX_CYCLES— Per-slice BRC re-proposal ceiling before HITL escalation. Default3.
EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES— Pipeline-wide cap on the summed total of slice re-proposal cycles. Default10. Either the local or global cap tripping escalates to HITL.
But docs/architecture/slice-dag.md (this same PR's link target) is explicit:
The
record_cycletwo-tiermax_cyclesaccounting and thehitl_escalatorhook onSliceSchedulerare public API and unit-tested, but the slice run loop does not yet callrecord_cycleon each BRC re-proposal. The env knobsEGG_ORCH_SLICE_LOCAL_MAX_CYCLES/EGG_ORCH_SLICE_GLOBAL_MAX_CYCLESare read but not exercised today; #2199 wires the trip flag through the BRC re-proposal loop.
I confirmed: record_cycle is not called anywhere in orchestrator/routes/pipelines.py (where _run_implement_phase_slices lives). So an operator who reads the orchestrator-cli table and sets EGG_ORCH_SLICE_LOCAL_MAX_CYCLES=2 to tighten the cap will see no behavior change today.
Add a "(API live, not yet wired in the run loop — see Slice-DAG Implement Phase; #2199)" caveat to both rows so the reference table doesn't promise more than the implementation delivers.
4. Out-of-scope but worth filing as a follow-up
Two architecture docs the PR doesn't touch still teach the shared-branch model for the implement phase:
docs/architecture/orchestrator.md:181—- All agents in a pipeline push to the same shared branch (e.g., \egg/issue-{N}`)`docs/architecture/git-isolation.md:259—In concurrent pipeline execution, all agents push to the same shared branch…
Not blocking on this PR (its declared scope is the five files in the diff), but a follow-up doc PR should sweep these for the same correction. Easy to forget — leaving a TODO(slice-dag) comment near them or filing a stub issue would help.
Nits
- L297 of
sdlc-pipeline.mdsays "stacked PR targeting the parent slice's branch (or the pipeline branch for root slices)". Verified againstorchestrator/stacked_pr_reconciler.py:3and:149— accurate. No change requested. - STRUCTURE.md entries for
slice_scheduler.pyandstacked_pr_reconciler.py(L101–L102) are placed sensibly (right afterconcurrent_executor.py) and describe the modules accurately. No change requested.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback from egg-reviewer: - concurrent-execution.md: qualify "shared branch" claims at L49 (top-level branch model paragraph), L862 (per-agent worktree architecture bullet), L867 (push coordination step), and L877 (reviewer worktree sync) so the whole guide consistently teaches the slice model for the implement phase rather than contradicting the new top-of-file note. - sdlc-pipeline.md: rewrite the "Shared Pipeline Branch" subsection (now "Branch Model") and the "Commit conflicts" troubleshooting bullet to match the slice-DAG model introduced earlier in the same file. - orchestrator-cli.md: add "API live, not yet wired in the run loop — see [Slice-DAG Implement Phase]; #2199" caveat to the EGG_ORCH_SLICE_LOCAL_MAX_CYCLES and EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES rows so the reference table doesn't oversell current behavior. Authored-by: egg
|
Thanks for the careful review. Pushed Blocking #1 —
Blocking #2 —
Non-blocking #3 — env-var caveat ✅
Non-blocking #4 — out-of-scope follow-up sweep
— 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 3e65387 only refines doc prose to disambiguate the branch model per phase (refine/plan share the pipeline branch, implement uses per-slice integration branches) and adds a "API live, not yet wired" caveat to two env-var rows. No prompts, agent workflows, output schemas, or LLM call sites change.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review verdict: approve
All three findings from my prior review at 3e65387 are addressed by the new commit fe6018a. Verified each fix in the working tree:
Blocking #1 — concurrent-execution.md contradictions ✅
- L49 ("Branch model"): now reads
For **refine** and **plan**, all agents operate on the pipeline's shared branch … For **implement**, each slice runs on its own integration branch (egg/issue-N/slice-M); the shared-branch coordination described below applies *within* a slice's agent team. Cross-references../architecture/slice-dag.md. - L862: bullet split into refine/plan vs implement, naming
egg/issue-{N}/slice-{M}for the latter. - L867: push step now reads
Agent pushes to the team's branch (pipeline branch for refine/plan, slice integration branch for implement). - L877: reviewer worktree sync prose now says
from the team's branch — the pipeline branch for refine/plan, the slice integration branch for each implement slice.
The whole guide now teaches the slice model consistently for the implement phase. The lead note at L5 and the four call-sites below it agree.
Blocking #2 — sdlc-pipeline.md contradictions ✅
- L1362–1373: subsection retitled "Branch Model" and rewritten — refine/plan share the pipeline branch, implement is a DAG of slices on per-slice integration branches.
- L1445–1449: "Commit conflicts" troubleshooting bullet now scoped to
Within a single team's branch (the pipeline branch for refine/plan, or a slice's integration branch for implement).
L297 ("stacked PR targeting the parent slice's branch") and the new branch-model subsection no longer contradict each other.
Non-blocking #3 — env-var caveat ✅
EGG_ORCH_SLICE_LOCAL_MAX_CYCLES and EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES rows in docs/reference/orchestrator-cli.md:83-84 now end with *API live, not yet wired in the run loop — see [Slice-DAG Implement Phase](../architecture/slice-dag.md); #2199.* Re-confirmed against orchestrator/: record_cycle is defined in slice_scheduler.py:299 but is not called in orchestrator/routes/pipelines.py, so the caveat is accurate. The producer's call to use a doc-level link (rather than #status) is correct — slice-dag.md carries its status as a top-of-doc blockquote, not a ## Status heading.
Non-blocking #4 — out-of-scope follow-up
docs/architecture/orchestrator.md:181 and docs/architecture/git-isolation.md:259 still teach the old shared-branch model for the implement phase. Producer indicated they will be addressed in a follow-up doc PR rather than expand scope here. Acceptable — note that until that follow-up lands, a reader of the architecture docs will get the pre-slice-DAG mental model.
Spot checks
- All three new
../architecture/slice-dag.mdlink targets resolve to the existing doc. - No new shared-branch claims introduced anywhere in the three modified files; remaining occurrences are correctly phase-qualified.
- The delta (~22 insertions / 18 deletions across three files) matches the producer's stated summary; no unrelated changes snuck in.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
Update documentation to reflect the slice-DAG implement phase shipped in #2220.
Summary
EGG_ORCH_MAX_PARALLEL_SLICES)orchestrator/slice_scheduler.pyandorchestrator/stacked_pr_reconciler.py; updateddependency_graph.pydescription to reflect its PEP-695 generification for both agent-role and slice DAGsEGG_ORCH_*SLICE*andEGG_ORCH_STACKED_PR_RECONCILER_INTERVAL_SECONDSenv vars to the environment variable reference tabledocs/architecture/slice-dag.mdTest plan
Triggered by: #2220
Authored-by: egg