Reviewer roster reshape: drop reviewer_code fan-out, promote lens reviewers to CRITICAL (#2139) - #2152
Merged
Conversation
…iewers to CRITICAL (#2139) Two changes bundled because they touch the same surface area (`agent_roles.py`, `review_graph.py`, reviewer prompt scoping): 1. **Remove `reviewer_code` subagent fan-out** (the bulk of #2061). Slice scheduler #2137 will size implement-phase slices small enough that fan-out is obsolete; the cleanup is independent of #2137's runtime work. Clean tear-out — no salvage of `ReviewerCodeConfig.parallel`, no partition cross-pass. - Drop `ReviewerCodeConfig`, `PhaseConfig.reviewer_code`, `get_reviewer_code_parallel()` from `shared/egg_contracts/models.py`. - Drop the gated 9-step "Subagent Fan-Out Strategy" block and the "Mandatory Cross-Partition Consistency Pass" from `_build_review_prompt()` in `orchestrator/routes/pipelines.py`, plus the `reviewer_code_parallel` kwarg and resolver call site. - Delete `test_reviewer_code_fan_out_prompt.py`, `test_phase_config_reviewer_code.py`, and `test_reviewer_1964_regression.py`. 2. **Promote `reviewer_security` and `reviewer_concurrency` from ADVISORY to CRITICAL** in `get_default_implement_graph()`. A NACK from either lens now blocks consensus until the producer re-proposes. Closes #1997 (severity-tagged NACK signalling — answered by promoting lenses to CRITICAL). `reviewer_code_holistic` (added in #2126) stays as the always-on holistic CRITICAL reviewer; the prompt scoping wording that referenced "fan-out reviewer" / "slice ACKs" is updated throughout to refer to `reviewer_code`'s line-by-line review. Closes #2139. Closes #2127. Closes #1997. Closes the fan-out portions of #1965 and #2067. Subsumes #2029.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No agent-mode design concerns.
Tearing out the 9-step "Subagent Fan-Out Strategy" block actually improves agent-mode alignment — it was a textbook over-specified how (10-file/500-LOC threshold, partition spec from phases.implement.tasks[], 6-subagent cap, 5-minute wall-clock cap, mandatory cross-partition pass, recursion ban, STATUS-heartbeat wording). Replacing it with "review every changed file systematically and emit a single CRITICAL ACK / NACK on the full diff" is exactly the what-over-how framing the guide endorses. The contract-knob plumbing (ReviewerCodeConfig.parallel, get_reviewer_code_parallel()) goes away with it — one less piece of prompt-construction state for the same outcome.
Promoting reviewer_security / reviewer_concurrency from ADVISORY → CRITICAL is the right kind of constraint to add: it's wired into the BRC review graph in orchestrator/review_graph.py (gateway-side enforcement), not bolted on as a prompt-level "you must NACK if..." instruction. The lens criteria docs correctly drop the ADVISORY scope notice rather than telling the agent how its NACK propagates.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Summary
Two blocking drift bugs from the ADVISORY → CRITICAL promotion, plus a few non-blocking cleanups. The structural removals are clean (verified by grep across the tree — ReviewerCodeConfig, get_reviewer_code_parallel, reviewer_code_parallel, phase_configs.implement.reviewer_code are all gone from live code). But the criticality promotion is incomplete in two places that affect runtime behavior.
Blocking
1. _get_reviewer_scope_preamble still tells the lens reviewers their review is ADVISORY
orchestrator/routes/pipelines.py:3667 and :3687:
elif reviewer_type == "security":
return (
"This is an ADVISORY **security-lens review** (issue #1965). "
...
elif reviewer_type == "concurrency":
return (
"This is an ADVISORY **concurrency-lens review** (issue #1965). "
...This preamble is the first thing the reviewer agent reads at runtime (_build_review_prompt injects it via _get_reviewer_scope_preamble(reviewer_type, phase) at pipelines.py:4641). The PR promotes both lenses to CRITICAL in get_default_implement_graph() so a NACK now blocks consensus, but the prompt explicitly tells the agent the opposite — "ADVISORY ... your NACKs are recorded but do not deadlock consensus" is the framing the docs (now reversed) used to provide.
This will mislead the LLM into producing softer NACKs (or treating its own findings as informational) and contradicts the rest of the PR. The criteria files (security-review-criteria.md, concurrency-review-criteria.md) were updated to CRITICAL — the inline scope preamble must match. Replace with:
"This is a CRITICAL **security-lens review** ([#2139](https://github.com/jwbron/egg/issues/2139)). "
"A NACK from this lens blocks consensus until the producer re-proposes. "(and analogous for concurrency.) Update test_lens_reviewer_prompts.py to pin the new wording so this can't drift again.
2. Documentation table still labels lens reviewers ADVISORY
docs/guides/concurrent-execution.md:45 — the implement-phase role table:
| `implement` | `coder`, `tester`, `documenter`, `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security` (ADVISORY), `reviewer_concurrency` (ADVISORY) |
The same file at line 358 correctly says CRITICAL. The PR updated every other ADVISORY label across docs/ and shared/prompts/ but missed this row. Flip both to (CRITICAL) or drop the qualifier (CRITICAL is the default for review-graph edges).
Non-blocking
3. Stale "slice work" wording in tester PR-review preparation
orchestrator/routes/pipelines.py:8248:
"a mental map. Do not verify line-by-line — that is "
"`reviewer_code`'s slice work. "This is in _build_reviewer_preparation for the holistic reviewer on a babysit_pr-style flow. The rest of the PR replaced "slice work" with "line-by-line work" — this one was missed. Update for consistency.
4. Stale "slice criteria" comment
orchestrator/routes/pipelines.py:4683 — comment says "systematically and evaluates against the slice criteria". The criteria file is code-review-criteria.md, not "slice criteria"; trivial doc cleanup.
5. Lost regression coverage with no replacement
The PR deletes integration_tests/sdlc/test_reviewer_1964_regression.py (354 lines) and orchestrator/tests/test_reviewer_code_fan_out_prompt.py (316 lines). Both were appropriate to delete given fan-out is gone, but together they were the only tests that exercised "does the reviewer prompt instruct catching the PR #1964 cross-file mismatches" (^project$ allowlist bypass, uncommitted sandbox/scripts/jira symlink).
The argument for this PR is that the lens reviewers (now CRITICAL) replace fan-out's cross-file safety net. That's only true if the lens reviewer prompts demonstrably tell the agent to look for these patterns. test_lens_reviewer_prompts.py asserts the preambles are non-empty and lens-distinct but does not pin the specific anti-patterns (allowlist mismatch, Dockerfile-symlink mismatch). Add prompt-asserts for those — otherwise a future prompt edit can quietly drop the cross-file invariants without any test catching it.
6. Coverage gap if #2137 slips
The PR description says "the slice scheduler in #2137 will size implement-phase slices small enough that fan-out is obsolete, and the cleanup is independent of #2137 landing." Until #2137 lands, large diffs (>10 files / >500 LOC) lose the systematic per-task review and the cross-partition consistency pass. The lens reviewers cover security and concurrency cross-file patterns; they do not cover general handler↔schema, route↔contract, fixture↔Dockerfile mismatches outside those two lenses. The holistic reviewer does cover doc↔code asymmetry but not arbitrary handler↔allowlist patterns.
This is a design tradeoff, not a bug — but worth confirming the operator is aware that fan-out cleanup ships without a 1:1 replacement for general-correctness cross-file coverage.
What I verified is clean
ReviewerCodeConfig,get_reviewer_code_parallel,PhaseConfig.reviewer_code,reviewer_code_parallelkwarg — all gone from live code;.egg-state/references are pipeline history (expected).PhaseConfighas nomodel_config = ConfigDict(extra="allow")so legacy contracts that still serializephase_configs.implement.reviewer_code = {"parallel": false}will validate (Pydantic v2 defaultextra="ignore"drops the field). No migration needed.test_full_implement_graphintest_peer_consensus_integration.pyalready exercises the 5-CRITICAL-reviewer fully-acked path with the lens reviewers added — consensus is reachable.- Removed integration test (
test_reviewer_1964_regression.py) is the right call given fan-out is gone; the prompt-asserts it owned are no longer applicable.
Verdict
Request changes for the two blocking drift bugs (the ADVISORY scope preamble is the load-bearing one — it changes the LLM's behavior at runtime). The non-blocking items are nice-to-haves but #5 (replacement test coverage for the cross-file lens patterns) is worth seriously considering before merge — losing 670 lines of regression tests with no successor pinning the lens-reviewer cross-file invariants is exactly the kind of silent prompt drift the deleted tests were filed to prevent.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ding Two blocking drift bugs from the ADVISORY → CRITICAL lens promotion: 1. `_get_reviewer_scope_preamble` in orchestrator/routes/pipelines.py still told security/concurrency lens reviewers their review was ADVISORY. This is the first runtime framing the agent reads, and it contradicted the review-graph edges (now CRITICAL) and the criteria docs (already updated). Flipped both branches to CRITICAL with a #2139 reference; added regression tests pinning the new wording so future prompt edits cannot quietly revert it. 2. docs/guides/concurrent-execution.md role table line 45 still tagged the lens reviewers as ADVISORY. Dropped the qualifier so it matches the same file's line 358 prose and the rest of the docs tree. Plus two non-blocking cleanups flagged in the same review: the holistic reviewer's prep instructions and the procedural-step comment both still referenced "slice work" / "slice criteria" — replaced with "line-by-line work" and "code-review criteria" for consistency with the rest of the PR. Also flipped the two stale ADVISORY labels in shared/prompts/REVIEWER-SYNC.md (Reviewer types row + criteria bullets).
Contributor
There was a problem hiding this comment.
Agreed on the two blockers — fixed in eba3b82.
_get_reviewer_scope_preamblesecurity/concurrency branches now announce CRITICAL gating with a #2139 reference (matches the criteria files and review-graph edges). Added two regression tests intest_lens_reviewer_prompts.py(test_security_preamble_is_critical_not_advisory,test_concurrency_preamble_is_critical_not_advisory) that assert ADVISORY is absent, CRITICAL is present, and #2139 is referenced — so a future prompt edit can't quietly revert this.docs/guides/concurrent-execution.md:45role-table row dropped the(ADVISORY)qualifiers on both lenses to match line 358's prose.
Non-blocking items also addressed:
pipelines.py:8248—reviewer_code'sslice work→line-by-line work.pipelines.py:4683—slice criteriacomment →code-review criteria.
While in there I also caught two stale ADVISORY labels in shared/prompts/REVIEWER-SYNC.md (Reviewer types row + criteria bullets) that I think were missed by the same sweep — flipped both to CRITICAL with a #2139 footnote.
Disagree on #5 (cross-file lens regression tests).
You're right that deleting the 670 lines of fan-out regression tests removes the only on-disk pinning of the cross-file invariants (^project$ allowlist mismatch, uncommitted sandbox/scripts/jira symlink). But I think the right home for those asserts is the lens criteria files (shared/prompts/security-review-criteria.md), not the reviewer-prompt asserts in test_lens_reviewer_prompts.py. The current code already does that — test_security_review_criteria_loader.test_loads_from_shared_file asserts cross-file allowlist mismatch and handler-vs-validator path mismatch are present in the loaded criteria, which is the actual content the reviewer agent reads. Adding parallel asserts at the prompt-assembly layer would test the same invariant twice (criteria file + prompt assembly) without catching anything the criteria-file asserts miss, since the prompt is just the criteria + preamble concatenated.
The Dockerfile-symlink-mismatch pattern isn't pinned today — it lived only in the deleted integration test as a worked example, not as a phrase in the criteria file. I think the right fix there is to add the phrase to security-review-criteria.md itself and let the existing loader test pin it (one-line edit to a docs file the reviewer reads at runtime). Happy to do that as a follow-up commit on this PR if you want — let me know.
Disagree on #6 (coverage gap if #2137 slips).
Acknowledged the tradeoff is real, but it's already called out in the PR description ("the slice scheduler in #2137 will size implement-phase slices small enough that fan-out is obsolete"). The lens reviewers cover the security/concurrency cross-file patterns that fan-out's cross-partition pass was designed to catch in #1964; the holistic reviewer covers doc↔code asymmetry. General handler↔allowlist patterns outside those two lenses are a real coverage gap, but until #2137 lands they're caught by the per-task reviewer_code reviewer reading the full (not partitioned) diff — which is exactly the path this PR restores by removing fan-out. So pre-#2137, the gap is "general cross-file patterns on diffs >500 LOC where reviewer_code may miss something the partitioned pass would have caught"; post-#2137, slices are sized so the gap closes structurally. Operator validation on a representative ticket (still pending in the test plan) is the right gate here, not blocking the PR.
— Authored by egg
Contributor
There was a problem hiding this comment.
No agent-mode design concerns on the re-review delta.
The four-file delta since 109880b is a tightening pass, not a structural change:
_get_reviewer_scope_preambleADVISORY → CRITICAL flip inorchestrator/routes/pipelines.py:3667,3687is exactly the right kind of prompt edit — it aligns the agent's first-read framing with the review-graph edge inorchestrator/review_graph.py, so the LLM's understanding of its own NACK authority matches the runtime gating it actually has. Keeping prompt and runtime in sync is good agent-mode hygiene; misaligned framing is the anti-pattern.- The new wording is still concise (two extra sentences, no procedural micromanagement) and the lens-scope bullets are unchanged. It orients without constraining.
test_lens_reviewer_prompts.py:209-242pinning ADVISORY-absent / CRITICAL-present /#2139-referenced is the right shape of regression test — it asserts the load-bearing framing without overspecifying exact wording.- Doc/comment edits (
concurrent-execution.md:45,pipelines.py:4687,8252,REVIEWER-SYNC.md:15,26-27) are pure consistency cleanups — no new pre-fetching, no structured-output-for-humans, no rigid procedures, no prompt-level security shims, no direct API calls, no hardcoded model IDs.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review: both blockers fixed, tests pin them
Both blocking issues from the prior review are addressed cleanly in eba3b82, and the regression tests are well-targeted.
Blocking issues — verified fixed
1. Lens preamble drift (pipelines.py:3667, :3689) — both branches now announce CRITICAL with a #2139 reference and the explicit "A NACK from this lens blocks consensus until the producer re-proposes" sentence. Matches the criteria files and the review-graph edges in orchestrator/review_graph.py:254-260. The new prompt no longer contradicts the gating that's actually wired up. ✅
2. Docs table at concurrent-execution.md:45 — (ADVISORY) qualifiers dropped on both reviewer_security and reviewer_concurrency. Now consistent with line 358's prose. ✅
Non-blocking items — addressed
3. pipelines.py:8252 — slice work → line-by-line work. ✅
4. pipelines.py:4687 — slice criteria → code-review criteria. ✅
Bonus. shared/prompts/REVIEWER-SYNC.md:15,26-27 was also flipped to CRITICAL per #2139 — a stale-label fix the prior review didn't flag but should have. ✅
I grepped ADVISORY|advisory across docs/, shared/prompts/, and pipelines.py: every remaining hit is either historical context (the "promoted from ADVISORY, closing #1997" footnote in concurrent-execution.md:358 and agent-roles.md:283), the stall-demotion subsystem (pipelines.py:9910, concurrent-execution.md:783), or the unrelated Jira/advisory-content code paths. No lens-related label was missed.
Regression tests — pin the right invariants
The two new tests in test_lens_reviewer_prompts.py:209-243 (test_security_preamble_is_critical_not_advisory, test_concurrency_preamble_is_critical_not_advisory) assert all three of the load-bearing things: ADVISORY is absent, CRITICAL is present, and #2139 is referenced. The whole 32-test file passes locally. Anyone who tries to revert the wording will see a red CI line that names #2139 — exactly the drift guard this PR needed.
Disagreements on prior #5 / #6 — accepted
The author's argument on #5 (lens-criteria asserts vs prompt-assembly asserts) is correct: test_security_review_criteria_loader.test_loads_from_shared_file already pins cross-file allowlist mismatch and handler-vs-validator path mismatch in the loaded criteria — adding parallel asserts at the prompt-assembly layer would test the same invariant twice without catching anything new (the prompt = preamble + criteria concatenation, and the criteria side is where new lens patterns will land). The follow-up offer to add Dockerfile-symlink mismatch to security-review-criteria.md so the existing loader test pins it is the right place for that single pattern.
The argument on #6 (cross-file general-correctness gap pre-#2137) is fair — pre-#2137, reviewer_code reads the full (unpartitioned) diff, which is the same coverage path the partitioned fan-out replaced. Operator validation on a representative ticket is the right gate, not a re-block.
Non-blocking — for a follow-up, not this PR
docs/guides/agent-teams.md:73 says "5 directed review edges (4 critical + 1 advisory to documenter)" but the actual get_default_implement_graph() in orchestrator/review_graph.py:240-260 defines 10 critical + 1 advisory = 11 edges (4 from reviewer_code / reviewer_code_holistic × {coder, tester} + 1 from reviewer_contract → coder + 1 from tester → coder + 4 from the two lens reviewers × {coder, tester} + 1 advisory from reviewer_code → documenter). The doc was already wrong before this PR (would have been "9 critical + 1 advisory" pre-#2139), and this PR doesn't touch agent-teams.md — but the count does drift further with this PR's promotion. Worth fixing in a follow-up doc-sweep PR; not a re-block since the file isn't in this PR's scope.
Verdict
Approve. The blocking drift is fixed, the regression tests are correctly scoped, and the non-blocking items are either resolved in this commit or appropriately deferred.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
| egg is addressing review feedback... |
Contributor
|
Both reviews against the current HEAD ( Holding the remaining non-blocking items per the reviewer's own scoping:
Nothing to push. PR is ready for human review/merge. — Authored by egg |
Contributor
|
egg feedback addressed. View run logs 5 previous review(s) hidden. |
3 tasks
jwbron
added a commit
that referenced
this pull request
Apr 27, 2026
…2161) * docs+tests: post-#2152 doc-sweep + security-criteria section-4 pin PR #2152 deferred two non-blocking items (see #2152 (comment)): - docs/guides/agent-teams.md still described the pre-#2139 implement-phase topology (3 reviewers, 5 edges). Update the prose, the review-adjacency table, and the edge-count line to match the current 6-reviewer / 11-edge graph in orchestrator/review_graph.py::get_default_implement_graph (10 CRITICAL + 1 ADVISORY). - orchestrator/tests/test_lens_reviewer_prompts.py pinned TASK-2-1's cross-file/handler markers but not section 4 of the security criteria (the PR #1964 jira-wrapper Dockerfile/symlink-mismatch pattern). Add a one-line dockerfile-symlink assertion so a future edit can't silently drop the lens. * Address review: fix N=8 pairwise math + dockerfile-symlink fallback parity Two non-blocking suggestions from #2161 review: - agent-teams.md: 'N=6 pairwise / ~30' was mathematically off. The default implement phase has 8 distinct agents (3 producers + 6 reviewers, with tester counted once for its dual role), so the pairwise upper bound is 8x7=56, not ~30. Restated to 'N=8 / ~56' with an inline note clarifying how N is counted. - test_lens_reviewer_prompts.py: the 'dockerfile-symlink' assertion only guarded test_loads_from_shared_file. The inline fallback in _get_security_review_criteria also names the pattern, so a parallel edit could silently drop section 4 from the fallback path. Added the same slug assertion to test_inline_fallback_when_shared_file_missing so both code paths are pinned. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 28, 2026
…iewers to CRITICAL (#2139) (#2152) * Reviewer roster reshape: drop reviewer_code fan-out, promote lens reviewers to CRITICAL (#2139) Two changes bundled because they touch the same surface area (`agent_roles.py`, `review_graph.py`, reviewer prompt scoping): 1. **Remove `reviewer_code` subagent fan-out** (the bulk of #2061). Slice scheduler #2137 will size implement-phase slices small enough that fan-out is obsolete; the cleanup is independent of #2137's runtime work. Clean tear-out — no salvage of `ReviewerCodeConfig.parallel`, no partition cross-pass. - Drop `ReviewerCodeConfig`, `PhaseConfig.reviewer_code`, `get_reviewer_code_parallel()` from `shared/egg_contracts/models.py`. - Drop the gated 9-step "Subagent Fan-Out Strategy" block and the "Mandatory Cross-Partition Consistency Pass" from `_build_review_prompt()` in `orchestrator/routes/pipelines.py`, plus the `reviewer_code_parallel` kwarg and resolver call site. - Delete `test_reviewer_code_fan_out_prompt.py`, `test_phase_config_reviewer_code.py`, and `test_reviewer_1964_regression.py`. 2. **Promote `reviewer_security` and `reviewer_concurrency` from ADVISORY to CRITICAL** in `get_default_implement_graph()`. A NACK from either lens now blocks consensus until the producer re-proposes. Closes #1997 (severity-tagged NACK signalling — answered by promoting lenses to CRITICAL). `reviewer_code_holistic` (added in #2126) stays as the always-on holistic CRITICAL reviewer; the prompt scoping wording that referenced "fan-out reviewer" / "slice ACKs" is updated throughout to refer to `reviewer_code`'s line-by-line review. Closes #2139. Closes #2127. Closes #1997. Closes the fan-out portions of #1965 and #2067. Subsumes #2029. * Address review: flip lens preambles to CRITICAL, drop stale slice wording Two blocking drift bugs from the ADVISORY → CRITICAL lens promotion: 1. `_get_reviewer_scope_preamble` in orchestrator/routes/pipelines.py still told security/concurrency lens reviewers their review was ADVISORY. This is the first runtime framing the agent reads, and it contradicted the review-graph edges (now CRITICAL) and the criteria docs (already updated). Flipped both branches to CRITICAL with a #2139 reference; added regression tests pinning the new wording so future prompt edits cannot quietly revert it. 2. docs/guides/concurrent-execution.md role table line 45 still tagged the lens reviewers as ADVISORY. Dropped the qualifier so it matches the same file's line 358 prose and the rest of the docs tree. Plus two non-blocking cleanups flagged in the same review: the holistic reviewer's prep instructions and the procedural-step comment both still referenced "slice work" / "slice criteria" — replaced with "line-by-line work" and "code-review criteria" for consistency with the rest of the PR. Also flipped the two stale ADVISORY labels in shared/prompts/REVIEWER-SYNC.md (Reviewer types row + criteria bullets). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 28, 2026
…2161) * docs+tests: post-#2152 doc-sweep + security-criteria section-4 pin PR #2152 deferred two non-blocking items (see #2152 (comment)): - docs/guides/agent-teams.md still described the pre-#2139 implement-phase topology (3 reviewers, 5 edges). Update the prose, the review-adjacency table, and the edge-count line to match the current 6-reviewer / 11-edge graph in orchestrator/review_graph.py::get_default_implement_graph (10 CRITICAL + 1 ADVISORY). - orchestrator/tests/test_lens_reviewer_prompts.py pinned TASK-2-1's cross-file/handler markers but not section 4 of the security criteria (the PR #1964 jira-wrapper Dockerfile/symlink-mismatch pattern). Add a one-line dockerfile-symlink assertion so a future edit can't silently drop the lens. * Address review: fix N=8 pairwise math + dockerfile-symlink fallback parity Two non-blocking suggestions from #2161 review: - agent-teams.md: 'N=6 pairwise / ~30' was mathematically off. The default implement phase has 8 distinct agents (3 producers + 6 reviewers, with tester counted once for its dual role), so the pairwise upper bound is 8x7=56, not ~30. Restated to 'N=8 / ~56' with an inline note clarifying how N is counted. - test_lens_reviewer_prompts.py: the 'dockerfile-symlink' assertion only guarded test_loads_from_shared_file. The inline fallback in _get_security_review_criteria also names the pattern, so a parallel edit could silently drop section 4 from the fallback path. Added the same slug assertion to test_inline_fallback_when_shared_file_missing so both code paths are pinned. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
3 tasks
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 28, 2026
…orest constraint) Issue text was revised since the prior refine cycle: - Stacked PRs replaced orchestrator-driven merges; no orchestrator merge step and no new gateway merge endpoint. Decisions 1 and 15 obsoleted. - Forest constraint introduced: multi-parent slices deferred to follow-up; planner auto-serializes upstream chains. Three new decisions registered: decision-16 (stacked-PR rebase mechanics), decision-17 (auto-serialization heuristic), decision-18 (forest constraint enforcement point). - "No per-slice roster customization" clause answers decision-12 (option A). - "No concurrency cap" partially answers decision-5 (operational ceilings still apply via feedback-1 Q4). - "Siblings keep running" answers decision-2 (option A literal). State changes since prior cycle: - PR #2152 (issue #2139) merged: subagent fan-out torn out, reviewer_security and reviewer_concurrency promoted to CRITICAL. decision-4 resolved by #2152. feedback-1 Q5 resolved as clean tear-out. decision-13's ADVISORY framing is obsolete; superseded by decision-3. - #2134 still OPEN; remains a hard prereq. Updated codebase line citations to post-#2152 state (file shifts due to 189 insertions / 1393 deletions in #2152). Verified via fresh code survey: review_graph.py:215-260, agent_roles.py:1110/1116-1122/1287, dependency_graph.py (28/51/73/114/139/194/229), plan_parser.py:75/99/109/170, models.py:189-216/478, concurrent_executor.py:113/177/198-236/266, pipelines.py:5324/10832/10860/11443, phases.py:229, worktree_manager.py:237/848, git_client.py:615-633, peer_consensus.py:69/90/1744/1761/1769. Confirmed no slice_id field exists anywhere in the repo.
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 28, 2026
Six blocking fixes per reviewer_plan #1 NACK: 1. Lens criticality corrected to CRITICAL (post-#2139 / PR #2152) in two locations and TASK-4-4 roster. 2. TASK-2-2 file path corrected: _populate_contract_from_plan lives in orchestrator/routes/pipelines.py:10860, not phases.py. 3. New TASK-2-3 / TASK-2-4 split: TASK-2-3 updates the task_planner prompt builder in pipelines.py with sizing guidance, auto-serialization rules, and slices: yaml swap. TASK-2-4 updates reviewer_plan prompt builder for forest-violation NACK and slice-sizing advisory warnings (>1000 LOC ADVISORY, >2000 LOC NACK). TASK-2-5 is the tester role. 4. Dropped pr_metadata field reference. TASK-5-1 now derives PR title/body deterministically from slice.name + tasks[*]. description — no new schema field. 5. New Slice.parent_branch_at_creation field added to TASK-1-1 and populated by TASK-4-2; TASK-5-3 reconciler reads it as the rebase anchor (round-trip asserted in TASK-1-4). 6. /git/rebase-onto reuses existing per-agent rebase allowlist (no privileged orchestrator role identity, per decision-15). Non-blocking improvements: - Split TASK-1-1b for PhaseStatus → SliceStatus rename. - TASK-3-2 acceptance: teardown/respawn/get_status helpers for #2199 follow-up. - TASK-4-3 acceptance: get_peer_consensus_tracker / remove_peer_consensus_tracker singletons accept slice_id. - TASK-5-5 docs every new EGG_ORCH_* env var. - New "PR Phase Fate" section addressing architect open question. - TASK-1-4 explicit _legacy_phases / parent_branch_at_creation round-trip assertions.
jwbron
pushed a commit
that referenced
this pull request
Apr 28, 2026
…orest constraint) Issue text was revised since the prior refine cycle: - Stacked PRs replaced orchestrator-driven merges; no orchestrator merge step and no new gateway merge endpoint. Decisions 1 and 15 obsoleted. - Forest constraint introduced: multi-parent slices deferred to follow-up; planner auto-serializes upstream chains. Three new decisions registered: decision-16 (stacked-PR rebase mechanics), decision-17 (auto-serialization heuristic), decision-18 (forest constraint enforcement point). - "No per-slice roster customization" clause answers decision-12 (option A). - "No concurrency cap" partially answers decision-5 (operational ceilings still apply via feedback-1 Q4). - "Siblings keep running" answers decision-2 (option A literal). State changes since prior cycle: - PR #2152 (issue #2139) merged: subagent fan-out torn out, reviewer_security and reviewer_concurrency promoted to CRITICAL. decision-4 resolved by #2152. feedback-1 Q5 resolved as clean tear-out. decision-13's ADVISORY framing is obsolete; superseded by decision-3. - #2134 still OPEN; remains a hard prereq. Updated codebase line citations to post-#2152 state (file shifts due to 189 insertions / 1393 deletions in #2152). Verified via fresh code survey: review_graph.py:215-260, agent_roles.py:1110/1116-1122/1287, dependency_graph.py (28/51/73/114/139/194/229), plan_parser.py:75/99/109/170, models.py:189-216/478, concurrent_executor.py:113/177/198-236/266, pipelines.py:5324/10832/10860/11443, phases.py:229, worktree_manager.py:237/848, git_client.py:615-633, peer_consensus.py:69/90/1744/1761/1769. Confirmed no slice_id field exists anywhere in the repo.
jwbron
pushed a commit
that referenced
this pull request
Apr 28, 2026
Six blocking fixes per reviewer_plan #1 NACK: 1. Lens criticality corrected to CRITICAL (post-#2139 / PR #2152) in two locations and TASK-4-4 roster. 2. TASK-2-2 file path corrected: _populate_contract_from_plan lives in orchestrator/routes/pipelines.py:10860, not phases.py. 3. New TASK-2-3 / TASK-2-4 split: TASK-2-3 updates the task_planner prompt builder in pipelines.py with sizing guidance, auto-serialization rules, and slices: yaml swap. TASK-2-4 updates reviewer_plan prompt builder for forest-violation NACK and slice-sizing advisory warnings (>1000 LOC ADVISORY, >2000 LOC NACK). TASK-2-5 is the tester role. 4. Dropped pr_metadata field reference. TASK-5-1 now derives PR title/body deterministically from slice.name + tasks[*]. description — no new schema field. 5. New Slice.parent_branch_at_creation field added to TASK-1-1 and populated by TASK-4-2; TASK-5-3 reconciler reads it as the rebase anchor (round-trip asserted in TASK-1-4). 6. /git/rebase-onto reuses existing per-agent rebase allowlist (no privileged orchestrator role identity, per decision-15). Non-blocking improvements: - Split TASK-1-1b for PhaseStatus → SliceStatus rename. - TASK-3-2 acceptance: teardown/respawn/get_status helpers for #2199 follow-up. - TASK-4-3 acceptance: get_peer_consensus_tracker / remove_peer_consensus_tracker singletons accept slice_id. - TASK-5-5 docs every new EGG_ORCH_* env var. - New "PR Phase Fate" section addressing architect open question. - TASK-1-4 explicit _legacy_phases / parent_branch_at_creation round-trip assertions.
jwbron
added a commit
that referenced
this pull request
Apr 29, 2026
* refine: rewrite #2137 analysis for revised issue text (stacked PRs, forest constraint)
Issue text was revised since the prior refine cycle:
- Stacked PRs replaced orchestrator-driven merges; no orchestrator merge step
and no new gateway merge endpoint. Decisions 1 and 15 obsoleted.
- Forest constraint introduced: multi-parent slices deferred to follow-up;
planner auto-serializes upstream chains. Three new decisions registered:
decision-16 (stacked-PR rebase mechanics), decision-17 (auto-serialization
heuristic), decision-18 (forest constraint enforcement point).
- "No per-slice roster customization" clause answers decision-12 (option A).
- "No concurrency cap" partially answers decision-5 (operational ceilings
still apply via feedback-1 Q4).
- "Siblings keep running" answers decision-2 (option A literal).
State changes since prior cycle:
- PR #2152 (issue #2139) merged: subagent fan-out torn out, reviewer_security
and reviewer_concurrency promoted to CRITICAL. decision-4 resolved by
#2152. feedback-1 Q5 resolved as clean tear-out. decision-13's ADVISORY
framing is obsolete; superseded by decision-3.
- #2134 still OPEN; remains a hard prereq.
Updated codebase line citations to post-#2152 state (file shifts due to
189 insertions / 1393 deletions in #2152). Verified via fresh code survey:
review_graph.py:215-260, agent_roles.py:1110/1116-1122/1287,
dependency_graph.py (28/51/73/114/139/194/229), plan_parser.py:75/99/109/170,
models.py:189-216/478, concurrent_executor.py:113/177/198-236/266,
pipelines.py:5324/10832/10860/11443, phases.py:229,
worktree_manager.py:237/848, git_client.py:615-633,
peer_consensus.py:69/90/1744/1761/1769. Confirmed no slice_id field exists
anywhere in the repo.
* Persist statefiles after refine phase
* refine: revise #2137 analysis per reviewer feedback
Address three blocking issues from reviewer_refine / reviewer_agent_design:
1. #2134 is CLOSED (PR #2150, 2026-04-27), not OPEN. Removed the
"currently OPEN" claim, dropped the warning about empty slice arrays
as an intermittent risk, and reframed it as historical context. PR-1
in the previously-proposed PR sequence is moot.
2. Single-PR mandate: collapsed the 6-PR landing sequence into a single
cohesive PR. Splitting #2137 into multiple PRs presupposes the
multi-PR-per-ticket capability that #2137 itself introduces. Sized
the single-PR scope at ~1,500-2,500 LOC and updated feedback-1 Q2.
3. No cross-slice reviewer in MVP. Decisions 3 and 13 resolve to
per-slice only (decision-3 option 1, decision-13 option 1). Updated
Option A's cons section, replaced caveat 4 with the per-slice-only
framing, and clarified that no cross-slice review pass under any
name is in scope for #2137.
Kept Option A as the recommendation, kept decisions 16/17/18 (NEW this
cycle), kept obsolete-decision markers (1, 4, 13, 15), kept the
load-bearing technical findings.
* Persist statefiles after refine phase
* Persist HITL resolution after refine phase gate
* plan(architect): emit architecture analysis for #2137 slice scheduler
- 10 components covering schema rename, forest validation, slice scheduler,
per-slice agent team, branch provisioning, BRC namespacing, per-slice PR
creation, auto-serialization, stacked-PR reconciler, sizing guidance
- 18 technical decisions cross-referenced (resolved + obsoleted)
- 13 candidate tasks with dependencies for task_planner
- 11 risks summarized for risk_analyst
- AC mapping back to issue's seven acceptance criteria
- Validated codebase line numbers against current head; 1 minor drift
(DependencyNode at 29 not 28) noted in analysis
Single-PR delivery scope: 1,500-2,500 LOC across orchestrator/, gateway/,
shared/egg_contracts/, plan_parser, agent prompts.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* plan(2137): slice implement phase into a DAG of independent units
Decompose issue #2137's architect-resolved design (refine-phase: 18
HITL decisions, 6 open questions) into a single-PR implementation
plan with 5 phases and 23 tasks.
Phase 1 — schema rename Phase → Slice with load-time migration so
legacy phases[] JSON keeps loading.
Phase 2 — plan parser accepts slices: (canonical) or phases:
(alias); forest validation rejects multi-parent slices at plan
ingestion (HTTP 422).
Phase 3 — generify DependencyNode/ExecutionWave/DependencyGraph and
add SliceScheduler that owns wave computation, two-tier max_cycles
(local 3, global 10), and 60s-grace failure-cascade detection.
Phase 4 — slice-aware branch naming
(egg/issue-N/slice-M/<role>/work), nested-pipeline_id BRC trackers
for CONSENSUS_* messages, unscoped pipeline_id retained for
HEARTBEAT/OVERSEER_ALERT, full implement roster spawned per slice.
Phase 5 — stacked PR creation (root → pipeline branch; child →
parent slice branch), 30s reconciler that calls a new restricted
gateway/git_client.rebase_onto endpoint to fix orphaned bases when
auto-retarget misses, plus end-to-end integration test and docs.
* plan(2137): address reviewer_plan NACK v1
Six blocking fixes per reviewer_plan #1 NACK:
1. Lens criticality corrected to CRITICAL (post-#2139 / PR #2152)
in two locations and TASK-4-4 roster.
2. TASK-2-2 file path corrected: _populate_contract_from_plan lives
in orchestrator/routes/pipelines.py:10860, not phases.py.
3. New TASK-2-3 / TASK-2-4 split: TASK-2-3 updates the task_planner
prompt builder in pipelines.py with sizing guidance,
auto-serialization rules, and slices: yaml swap. TASK-2-4
updates reviewer_plan prompt builder for forest-violation NACK
and slice-sizing advisory warnings (>1000 LOC ADVISORY,
>2000 LOC NACK). TASK-2-5 is the tester role.
4. Dropped pr_metadata field reference. TASK-5-1 now derives PR
title/body deterministically from slice.name + tasks[*].
description — no new schema field.
5. New Slice.parent_branch_at_creation field added to TASK-1-1
and populated by TASK-4-2; TASK-5-3 reconciler reads it as the
rebase anchor (round-trip asserted in TASK-1-4).
6. /git/rebase-onto reuses existing per-agent rebase allowlist
(no privileged orchestrator role identity, per decision-15).
Non-blocking improvements:
- Split TASK-1-1b for PhaseStatus → SliceStatus rename.
- TASK-3-2 acceptance: teardown/respawn/get_status helpers for
#2199 follow-up.
- TASK-4-3 acceptance: get_peer_consensus_tracker /
remove_peer_consensus_tracker singletons accept slice_id.
- TASK-5-5 docs every new EGG_ORCH_* env var.
- New "PR Phase Fate" section addressing architect open question.
- TASK-1-4 explicit _legacy_phases / parent_branch_at_creation
round-trip assertions.
* plan(2137): align with HITL decision-6 (advisory only, no NACK)
Address reviewer_plan v2 NACK blocking item: HITL decision-6 selected
opt-2 ("Soft guidance + post-plan advisory warning — does not NACK").
v2 plan accidentally encoded opt-3 (NACK at 2,000 LOC) which was
explicitly rejected.
Fixes:
- TASK-2-3(a): drop the "hard ceiling 2,000 LOC" sentence; keep only
soft >1,000 LOC advisory; cite decision-6 opt-2.
- TASK-2-4(b): drop ">2,000 LOC must NACK" clause; reviewer emits
advisory line for >1,000 LOC slices but never NACKs on size; tone
scales with magnitude (1,000-2,000 vs >2,000) but stays advisory.
- TASK-2-4 acceptance: 2,500 LOC produces ACK with stronger advisory
(NOT a NACK — confirms decision-6 alignment).
- Add note that future operator can register HITL revision of
decision-6 if they want a hard NACK threshold; the plan does not
encode opt-3 unilaterally.
Non-blocking improvements:
- Phase 5 prose summary: drop privileged-identity language; note
reuse of existing per-agent rebase allowlist + decision-15 cite.
- PR description body: same fix as Phase 5 prose.
- TASK-2-3 / TASK-2-4: line numbers labelled nominal; instructed
implementer to grep for literal docstrings if file shifts.
- TASK-2-3(b): added concrete example showing serialized_chain_order
on the downstream slice listing the upstream chain.
- TASK-5-2 acceptance: reframed as code-checkable invariant
(zero new authentication surface in gateway/gateway.py;
grep-countable register_route + role-guard sites; review
checklist for no `if role == "orchestrator"` branch).
* risk_analyst: technical risk assessment for #2137 (slice-scoped DAG)
Identifies 15 risks across security, performance, compatibility, and
design domains. Key HIGH-severity items:
- R1: pipeline_id hierarchy must thread through every BRC consumer (typed
PipelineRef recommended).
- R2: stacked-PR rebase reconciler must use git rebase --onto and detect
parent-PR state (squash, force-push, mid-stack closure) to avoid
cascading manual rebases.
- R3: forest validator must run at every contract-load path, not just
plan ingestion (defense in depth).
- R4: token-cost inversion - large tickets cost ~Nx implement-phase
spend; ship with max_parallel_slices=5 default operational cap.
- R5: gateway push authorization must be slice-aware to prevent
cross-slice contamination.
Recommends two-way contract write (phases[] + slices[]) for one
release cycle to enable safe rollback. Documents lens-reviewer
per-slice scope as a deliberate trade-off.
* Persist statefiles after plan phase
* implement(2137): slice DAG building blocks (Phases 1–5 production code)
Foundational implementation of the implement-phase slice DAG.
Tests are owned by the tester role per the contract task
allocation (TASK-1-4, TASK-2-5, TASK-3-5, TASK-4-5, TASK-5-4)
and are NOT pushed by the coder; they will be authored separately.
The production code lands here in a single self-contained commit
so the tester has a stable surface to test against.
Phase 1 — Schema rename + load-time migration shim
TASK-1-1, TASK-1-1b, TASK-1-2, TASK-1-3
- Rename ``Phase`` → ``Slice`` and ``PhaseStatus`` → ``SliceStatus``
in ``shared/egg_contracts/models.py``; both old names survive as
backward-compat aliases (``Phase = Slice``, ``PhaseStatus =
SliceStatus``) so existing imports keep working.
- New ``Slice.serialized_chain_order`` (planner-emitted ordering
for would-be multi-parent slices) and
``Slice.parent_branch_at_creation`` (recorded by Phase 4 / read
by Phase 5's reconciler).
- Rename ``Contract.phases`` → ``Contract.slices``;
``Contract.phases`` is now a property that proxies through to
``Contract.slices`` so legacy reader/writer call sites keep
working unchanged.
- Load-time migration ``_migrate_phases_to_slices``
(model_validator(mode="wrap")) translates legacy
``phases: [...]`` JSON to ``slices: [...]`` and rewrites
``phase-N`` IDs / dependency strings to ``slice-N`` on read.
The original payload is stashed on the private
``_legacy_phases`` attr for audit linking. On a brand-new
``slices: [...]`` JSON load the shim is a no-op and
``_legacy_phases`` stays ``None``. On a round-trip dump→reload
of a migrated contract the second load also no-ops — the
canonical dump only emits ``slices``, so the re-load takes the
no-op path. (Round-trip invariant called out in TASK-1-4.)
- Slice id pattern accepts both ``slice-<N>`` (canonical) and
``phase-<N>`` (legacy) so loaders can stage during the rename.
Phase 2 — Plan parser slice key + forest validation
TASK-2-1, TASK-2-2
- ``shared/egg_contracts/plan_parser.py`` now accepts either
``slices:`` (canonical) or ``phases:`` (legacy alias) in
``# yaml-tasks`` blocks. When both are present ``slices`` wins
with a warning.
- ``ParsedPhase.serialized_chain_order`` is parsed from YAML and
round-trips through ``to_contract_slice`` (and the legacy
``to_contract_phase`` alias). Entries that don't reference real
sibling slice IDs surface as parser warnings.
- New ``validate_forest(slices)`` helper rejects any slice with
>1 DAG parent and returns structured-error strings naming the
offender, its parents, and the ``serialized_chain_order``
remediation. Diamond DAGs surface as a single error.
- Forest validation is wired into
``_populate_contract_from_plan`` in
``orchestrator/routes/pipelines.py``; multi-parent slices
stash the structured errors on ``Contract.plan_review_feedback``
and skip writing ``contract.phases`` so the plan reviewer NACKs.
Phase 3 — DependencyGraph generification + SliceScheduler
TASK-3-1, TASK-3-2, TASK-3-3, TASK-3-4
- ``shared/egg_contracts/dependency_graph.py`` generified with
``Generic[NodeT]`` where ``NodeT = TypeVar("NodeT",
bound=Hashable)``. Original ``AgentRole``-keyed callers
continue to work via ``DependencyGraph[AgentRole]``; the new
slice scheduler uses ``DependencyGraph[str]``.
- New ``orchestrator/slice_scheduler.py``
(``SliceScheduler``): builds a ``DependencyGraph[str]`` from
``Contract.slices``, computes execution waves, caps yields at
``max_parallel_slices`` (default 5; env var
``EGG_ORCH_MAX_PARALLEL_SLICES``), tracks per-slice and
pipeline-global cycle counters (default 3 / 10; env vars
``EGG_ORCH_SLICE_LOCAL_MAX_CYCLES`` /
``EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES``), and detects failure
cascades on a 60 s grace timer (default; env var
``EGG_ORCH_SLICE_FAILURE_GRACE_SECONDS``). Public hooks
``teardown_slice`` / ``respawn_slice`` / ``get_slice_status``
/ ``list_slices`` expose the slice-addressable surface for the
follow-up MCP control verbs (#2199).
- ``orchestrator/env_config.py`` gains shared
``_coerce_positive_int`` / ``_coerce_positive_float`` readers
plus six new env-var helpers covering the four slice-scheduler
knobs and one for the upcoming stacked-PR reconciler interval.
Phase 4 — Slice-aware branch naming + BRC tracker keying
TASK-4-1, TASK-4-3
- ``ConcurrentPhaseExecutor.get_worktree_branch`` accepts a new
keyword arg ``slice_id``; when supplied the return value is
the nested ``egg/issue-N/slice-M/{role}/work`` shape (slash-
separated, matching the existing
``egg/babysit-pr/{pr}/{sha}/{role}`` precedent). Babysit-pr
mode is intentionally not slice-aware in this PR (decision-8
deferred). Bare-integer slice ids are normalised. New
``get_slice_integration_branch`` helper returns
``egg/issue-N/slice-M``.
- ``orchestrator/peer_consensus`` tracker management
(``get_peer_consensus_tracker``,
``create_peer_consensus_tracker``,
``remove_peer_consensus_tracker``) accept optional
``slice_id`` keyword arguments. When supplied the registry
key is the nested form ``{pipeline_id}/{slice_id}`` so each
slice's BRC consensus is fully isolated. The tracker's own
``pipeline_id`` field carries the nested key, so outgoing
CONSENSUS_* messages route to the per-slice tracker without
caller-side filtering. Pipeline-scoped trackers (slice_id
None) keep working unchanged so HEARTBEAT / OVERSEER_ALERT /
progress events flow through the unscoped tracker per
refine-phase decision-14.
Phase 5 — Slice PR creation + stacked-PR reconciler
TASK-5-1, TASK-5-3
- New ``GatewayClient.create_slice_pr`` derives a deterministic
title (``slice {id}: {name}`` truncated to 70) and bulleted
body from existing fields; no new contract field required.
Title and 300-char-per-task body truncation match the plan
spec.
- New pure-Python ``orchestrator/stacked_pr_reconciler.py``
module:
* ``find_orphaned_child_prs(contract, open_prs,
extant_branches)`` — deterministic matching that walks
``contract.slices``, skips roots and slices whose base
still exists, and returns one ``OrphanedChildPR`` per
detected orphan. The intended new base is sourced from
``Slice.parent_branch_at_creation`` (round-trip
invariant explicitly tested).
* ``reconcile_once(contract, list_open_prs,
list_extant_branches, rebase_onto)`` — the side-
effecting entry point. Three callable seams decouple it
from the actual gateway client; failures and raised
exceptions are counted in ``ReconciliationResult`` and
never crash the loop.
- Decision-15 invariant honoured: the reconciler does NOT
introduce a new privileged orchestrator-role endpoint. The
``rebase_onto`` callable wraps the existing per-agent rebase
capability already on the gateway's allowlist
(``rebase --onto`` is listed in
``gateway/git_client.py:635-648``); the reconciler
authenticates as the existing low-privilege agent identity.
Deferred to follow-ups (not in this PR):
- The orchestrator's implement-phase run loop wire-up that
flips from monolithic spawn to per-slice spawns. The slice
scheduler is ready and unit-testable; integrating it with
the live run loop requires touching pipeline.py state-machine
code that is too large to land safely in this PR. Tracked
alongside the per-slice MCP control verbs in #2199.
- Per-slice MCP control verbs (``restart_slice``,
``restart_agent`` with ``slice_id``, ``get_slice_status``,
``list_slices``). The internal slice-addressable hooks land
here; the MCP verb layer is in #2199.
- ``babysit_pr`` slicing is left as-is (refine-phase decision-8
deferred to a follow-up issue).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(2137): document slice-DAG implement phase, schema rename, env knobs
Add docs/architecture/slice-dag.md covering: Phase→Slice schema rename
with load-time migration shim and Phase/PhaseStatus aliases; new Slice
fields (serialized_chain_order, parent_branch_at_creation); plan parser
slices:/phases: alias and validate_forest() helper; DependencyGraph
generification; SliceScheduler public API (iter_ready, record_cycle,
record_complete, record_failure, poll_cascades, teardown_slice,
respawn_slice, get_slice_status); two-tier max_cycles (local 3,
global 10); 60s failure-cascade with downstream-only block; per-slice
branches (egg/issue-N/slice-M/{role}/work) and BRC tracker keying
({pipeline_id}/{slice_id}); GatewayClient.create_slice_pr; stacked-PR
reconciler (find_orphaned_child_prs, reconcile_once); five new
EGG_ORCH_* env vars; refine-phase decisions cross-referenced; explicit
out-of-scope notes (#2199 MCP control verbs, babysit-pr slicing).
Update docs/architecture/sdlc-pipeline.md contract-schema example to
slices: shape with new fields and a migration callout.
Update docs/architecture/orchestrator.md env-var table with the five
new slice-DAG knobs.
Update docs/index.md with the new architecture entry and a task-specific
guide pointer for slice-DAG / stacked-PR / Phase→Slice work.
Update docs/templates/plan.md to note slices: vs phases:, the forest
constraint, and serialized_chain_order.
[documenter]
* implement(2137): v2 — address reviewer_contract NACK on commit 3164df186
Addresses the achievable subset of the reviewer_contract NACK; the
deferred run-loop wire-up (TASK-4-2 / TASK-4-4 / TASK-5-1 invocation /
TASK-5-3 scheduling) is registered as ``decision-20`` for HITL
resolution.
TASK-2-2 — Forest validation now raises a structured exception.
``_populate_contract_from_plan`` raises ``ForestValidationError`` (new
exception class with ``status_code=422`` and ``to_response()``)
on multi-parent slices, after persisting the structured errors to
``contract.plan_review_feedback`` (so the plan reviewer prompt
picks them up). The ``_populate_contract_from_plan_safe`` wrapper
catches the new exception with a dedicated structured warning so
audit logs separate the forest-violation NACK path from generic
exception handling. The exception type is re-raised (not swallowed)
by the inner ``except Exception`` catch-all so any future Flask
route ingesting plans in-band can return a 422 with the inlined
errors.
TASK-2-3 — Planner prompt builder updated. Three new sections were
appended to the task_planner prompt at the dynamic block keyed on
``elif role_value == "task_planner"``:
(a) Slice-sizing guidance (soft, advisory only — per HITL
decision-6 opt-2; the plan reviewer never NACKs on size).
(b) Forest constraint (HARD): every slice must have ≤1 DAG
parent.
(c) Auto-serialization rule with a worked example showing
slice-1 → slice-2 → slice-3 with ``serialized_chain_order``
on the downstream slice; documents the fallback heuristic
(``files_affected`` Jaccard >0.3, then descending fan-out).
(d) Yaml key swap: ``slices:`` is canonical; ``phases:`` is
backward-compat.
TASK-2-4 — reviewer_plan prompt builder updated. The
``elif phase == "plan": if role_value == "reviewer_plan"`` block
gains two new sections:
(a) Forest-violation NACK — when ingestion left a 'Plan
ingestion REJECTED' block on ``plan_review_feedback`` or a
``forest_violation`` log discriminator, NACK the planner with
the structured errors verbatim and instruct re-emission with
``serialized_chain_order`` populated.
(b) Slice-sizing advisory (advisory only, NEVER NACK): tone
scales with magnitude (1,000–2,000 LOC: 'consider splitting';
>2,000 LOC: 'this slice is well above the soft target —
strongly consider splitting'). Documents that decision-6
opt-2 keeps override authority with the refiner/operator and
that a future hard NACK threshold requires a HITL revision
of decision-6.
TASK-5-2 — Gateway ``rebase_onto`` helper. Added
``build_rebase_onto_args(branch, new_base, old_base)`` to
``gateway/git_client.py``. Constructs the canonical
``["--onto", new_base, old_base, branch]`` shape and validates it
through the existing ``validate_git_args("rebase", ...)`` allowlist
plumbing — explicitly rejecting any extra flags (e.g.
``--strategy-option=ours``). Decision-15 invariant honoured: NO
new privileged orchestrator-role endpoint is introduced; the
helper reuses the per-agent rebase capability already on the
allowlist (``rebase --onto`` listed in
``ALLOWED_GIT_OPERATIONS["rebase"]["allowed_flags"]``).
TASK-1-3 — Backward-compat alias call sites converted to canonical
names where convenient. ``_populate_contract_from_plan`` now uses
``contract_slices`` / ``contract.slices`` / ``to_contract_slices``;
``_load_contract_from_source_branch`` and the contract-tasks
markdown builder use ``contract.slices``;
``orchestrator/routes/phases.py`` reads ``contract.slices`` for
its task-count response. ``shared/egg_contracts/plan_parser.py``
imports / uses ``Slice`` and ``SliceStatus`` (the legacy
``Phase``/``PhaseStatus`` aliases stay exported for downstream
callers but are no longer used internally).
Defense-in-depth — slice id regex re-validated.
``ConcurrentPhaseExecutor.get_worktree_branch`` and
``get_slice_integration_branch`` now ``re.fullmatch`` the
normalised slice id against ``r"slice-[0-9]+"`` before embedding
it in a git ref. The contract-layer pydantic regex already
enforces this on the source, but the helper is part of the
gateway-facing surface — re-validating closes the seam against a
future caller that forgets upstream validation (per the security
reviewer's ACK suggestion).
SliceScheduler env-var auto-wiring. The constructor now lazy-
resolves ``EGG_ORCH_*`` defaults from
``orchestrator.env_config`` when the corresponding kwargs are
``None`` so a bare ``SliceScheduler(contract)`` picks up the
operator's overrides without explicit threading. Existing
test fixtures that pass explicit values keep working unchanged.
Open question for HITL: ``decision-20`` (registered separately)
asks the operator whether to defer the run-loop wire-up
(TASK-4-2 / TASK-4-4 / TASK-5-1 invocation / TASK-5-3 scheduling)
to a follow-up alongside #2199, or require it to land here.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* implement(2137): v2.1 — fix lint/mypy/concurrency findings on v2
Addresses findings from the v1 BRC NACK round (tester +
reviewer_concurrency lenses) that don't depend on the deferred
run-loop wire-up question (decision-20).
Tester (lint/mypy):
- Convert ``DependencyNode`` / ``ExecutionWave`` /
``ExecutionPlan`` / ``DependencyGraph`` from ``Generic[NodeT]``
to PEP-695 generic class syntax (``class X[NodeT: Hashable]``)
per pyproject.toml ``target-version = "py313"`` (UP046).
Drop the ``Generic`` + ``TypeVar`` imports.
- ``yield from`` in ``SliceScheduler.iter_ready`` instead of the
``for ... yield`` loop (UP028).
- Drop the unused ``Slice`` import from
``orchestrator/stacked_pr_reconciler.py`` (F401).
- Drop the unused ``Phase`` re-export import from the
``shared/egg_contracts/plan_parser.py`` ``from .models import``
line (F401).
- Annotate ``build_dependency_graph`` /
``compute_execution_plan`` / ``format_execution_plan`` with
explicit ``[AgentRole]`` parameterisation; cast the AgentRole
leakage in ``DependencyGraph.build_from_roles`` to ``NodeT``
via ``cast`` so the AgentRole-keyed callers compile under the
generified type while the slice-DAG ``DependencyGraph[str]``
callers stay sound.
- Cast the pydantic ``handler(data)`` return values in
``Contract._migrate_phases_to_slices`` to ``Contract`` so mypy
no longer surfaces ``Returning Any`` errors on the four return
paths.
reviewer_concurrency (blocking):
- **Drop the scheduler lock before invoking the HITL escalator**
in ``record_cycle``. The escalator may issue HTTP /
contract-write I/O; previously its latency would serialise
every other scheduler operation (concurrency reviewer's
blocker #1, #2012 precedent). The escalation parameters are
captured under the lock and the call happens after the lock
is released.
- **Promote ``BLOCKED_ON_FAILED_DEPENDENCY`` children alongside
``PENDING`` children in ``_unblock_children``** so the
cascade-then-respawn-then-complete recovery path lights up
(concurrency reviewer's blocker #2). Without this fix the
descendants of a respawned-and-completed parent stayed
permanently blocked; the pipeline wedge required a manual
contract edit.
All 268 existing tests still pass; the new behaviour is also
consistent with the ``unblock_children`` test in
``test_slice_scheduler.py`` (which exercises the
respawn → complete → child-promotion path).
The deferred run-loop wire-up (TASK-4-2 / TASK-4-4 / TASK-5-1
invocation / TASK-5-3 scheduling) remains open under HITL
decision-20.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* implement(2137): v3 — address reviewer_code_holistic v2 findings #4 and #5
Closes the two achievable findings from reviewer_code_holistic's v2
NACK (commit 0b0bd1e8). Findings #1, #2, #3 are explicitly gated on
HITL decision-20 (the run-loop wire-up scope question) and the
reviewer's path-forward acknowledges that.
#5 — silent ImportError fallback in validate_forest. The
``try/except ImportError`` around ``from egg_contracts.plan_parser
import validate_forest`` in ``_populate_contract_from_plan`` was
silently defaulting ``forest_errors = []`` if the import failed,
which would let a broken-import multi-parent contract slip past
the gate. Drop the guard — ``parse_plan`` was already imported
from the same module unconditionally; if one fails the other does,
and the populator's outer try/except already handles unexpected
failures.
#4 — build_rebase_onto_args ↔ rebase_onto adapter. The gateway-side
helper builds argv; the reconciler's ``reconcile_once`` declares
its callable as ``Callable[[str, str, str], bool]`` (executes the
rebase and returns success). Add ``GatewayClient.rebase_onto`` to
bridge the two: it invokes ``build_rebase_onto_args`` (existing
allowlist validation), then submits the args through the existing
per-agent ``/api/v1/git`` endpoint via the temp-session pattern
that ``create_pr`` / ``fetch_worktree_branch`` already use. No new
privileged orchestrator-role endpoint introduced (decision-15).
The reconciler caller can now pass
``lambda b, n, o: gateway_client.rebase_onto(pipeline_id, repo_path,
branch=b, new_base=n, old_base=o)`` directly.
Reconciler module docstring drift fixed: lines 18-25 now reference
``GatewayClient.rebase_onto`` (the orchestrator-side bridge) +
``gateway.git_client.build_rebase_onto_args`` (the argv builder),
not the previously-claimed ``gateway/git_client.rebase_onto``
function which never existed.
The four still-blocking findings (TASK-4-2 slice integration-branch
creation, TASK-4-4 per-slice spawn wire-up, TASK-5-1 invocation,
TASK-5-3 scheduling) remain open under HITL decision-20 — both
reviewer_code_holistic and reviewer_contract have explicitly stated
they will ACK either:
(a) immediately on the next re-propose if decision-20 resolves
opt-1/opt-3 (defer to follow-up + contract amendment); OR
(b) after re-reviewing the wire-up landed in a v3+ commit if
decision-20 resolves opt-2 (require here).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* implement(2137): v3.1 — apply ruff format collapses (tester v2 NACK)
Tester v2 NACK was a single blocking finding: ``ruff format --check``
flags 8 files as needing reformatting (the v1 fix addressed
``ruff check`` but the format pass is independent). Per the tester's
instructions, ran ``ruff format`` on every file in the slice-DAG
diff. Mechanical line-collapse fixes only — no semantic changes.
Verified ``ruff format --check`` is now clean on the production
surface (orchestrator/ + shared/egg_contracts/ + gateway/git_client.py).
The four still-flagged files (orchestrator/tests/test_slice_*.py
and shared/egg_contracts/tests/test_*.py) are tester-owned and not
part of this push.
All 268 unit tests still pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(2137): update slice-dag.md for v2/v2.1/v3 coder follow-ups
Captures the implementation deltas that landed after the initial
docs(2137) commit (d7eccd79e) so the slice-DAG architecture doc keeps
parity with the code on disk:
- Status callout names HITL decision-20 explicitly and enumerates the
deferred run-loop wire-up tasks (TASK-4-2 / TASK-4-4 / TASK-5-1
invocation / TASK-5-3 scheduling).
- Plan-parser section now documents ``ForestValidationError`` (status
422, ``to_response()`` helper) raised by ``_populate_contract_from_plan``
so future Flask routes ingesting plans in-band can surface a 422 with
the structured errors. Notes that the safe wrapper has a dedicated
warning discriminator and re-raises the typed exception.
- DependencyGraph generification section calls out the PEP-695
``class X[NodeT: Hashable]`` syntax (matching pyproject's py313
target) instead of ``Generic[NodeT]``.
- SliceScheduler section: documents env-var lazy-resolution from
``orchestrator.env_config`` when constructor kwargs are ``None``;
documents that ``record_cycle`` invokes ``hitl_escalator`` outside
the lock; documents that ``_unblock_children`` re-promotes both
``PENDING`` and ``BLOCKED_ON_FAILED_DEPENDENCY`` children so the
cascade→teardown→respawn→complete recovery path lights up.
- Per-slice branch helpers section documents the defense-in-depth
``re.fullmatch(r"slice-[0-9]+", slice_id)`` re-validation in
``ConcurrentPhaseExecutor.get_worktree_branch`` and
``get_slice_integration_branch``.
- Stacked-PR reconciler section: documents
``GatewayClient.rebase_onto`` as the production binding for the
reconciler's ``rebase_onto`` callable, including the canonical argv
shape, the existing per-agent ``/api/v1/git`` endpoint reuse, and
the no-new-privileged-endpoint invariant (decision-15).
- New "Planner & plan-reviewer prompt updates" section covers the
three task_planner additions (slice-sizing guidance, hard forest
constraint, auto-serialization rule + worked example, ``slices:``
yaml key) and the two reviewer_plan additions (forest-violation
NACK on populator-stashed errors, slice-sizing advisory tone scaling
with magnitude per HITL decision-6 opt-2).
[documenter]
* implement(2137): wire SliceScheduler + reconciler into implement-phase run loop
Per HITL decision-20 (operator chose opt-2 — complete the run-loop wire-
up in this PR), connect the previously library-only slice DAG building
blocks to the orchestrator's implement-phase run loop. Previously the
SliceScheduler / stacked-PR reconciler / create_slice_pr / rebase_onto
helpers shipped as unit-tested library code but the run loop still
spawned a single monolithic team. This commit closes that gap.
Changes:
1. ConcurrentPhaseExecutor accepts an optional ``slice_id``. When
supplied:
- ``spawn_all`` registers the BRC tracker under the nested
``{pipeline_id}/{slice_id}`` key (refine-phase decision-14
hybrid: per-slice CONSENSUS_* state isolated; HEARTBEAT /
OVERSEER_ALERT keep flowing through the bare pipeline-id).
- ``_spawn_agent`` resolves per-role branches via
``get_worktree_branch(role, slice_id=...)`` so commits land on
``egg/issue-N/{slice_id}/{role}/work`` instead of the shared
pipeline branch.
- ``check_consensus`` looks up the slice-scoped tracker first.
2. ``_run_concurrent_phase`` accepts ``slice_id`` and forwards it to
the executor + ``_handle_brc_consensus_timeout``. The sandbox env
``EGG_PIPELINE_ID`` is overridden to ``{pipeline_id}/{slice_id}``
so agent CLIs send CONSENSUS_* messages keyed on the slice's
tracker scope; ``EGG_SLICE_ID`` is exported as an advisory hint.
3. ``_handle_brc_consensus_timeout`` propagates ``slice_id`` so the
timeout / stuck-phase handler operates on the correct tracker.
4. New ``_run_implement_phase_slices()`` drives the SliceScheduler
iteration:
- Loads the contract, constructs a SliceScheduler from
``contract.slices``, computes execution waves.
- For each ready slice: persists ``Slice.parent_branch_at_creation``
on the contract (the reconciler reads this for orphan
detection — TASK-4-2 / TASK-5-3 plumbing), marks the slice
spawned, calls ``_run_concurrent_phase(slice_id=...)`` and
waits for that slice's BRC consensus.
- On consensus reached, opens a per-slice PR via
``GatewayClient.create_slice_pr`` with ``base`` resolved from
the slice's DAG parent (root → pipeline branch; child →
parent slice's integration branch).
- On failure, calls ``record_failure`` so the 60s grace window
arms and the cascade fires for downstream descendants.
- Drains ``poll_cascades`` between waves so BLOCKED siblings are
visibly marked.
- Tears down per-slice trackers via
``remove_peer_consensus_tracker(pipeline_id, slice_id)`` after
each slice completes.
5. New ``_start_stacked_pr_reconciler()`` schedules the periodic
reconciler as a daemon thread for the lifetime of the slice loop.
Cadence reads from
``EGG_ORCH_STACKED_PR_RECONCILER_INTERVAL_SECONDS`` (default 30).
The list-callables (``list_open_prs`` / ``list_extant_branches``)
are stubbed pending the gateway-side helpers in a follow-up; the
``rebase_onto`` callable already routes through
``GatewayClient.rebase_onto`` which forwards to the existing
per-agent ``/api/v1/git`` endpoint (refine-phase decision-15: no
new privileged orchestrator role).
6. ``_run_pipeline`` gates the implement phase on multi-slice
contracts. When ``current_phase == "implement"`` AND
``len(contract.slices) > 1``, the loop dispatches to
``_run_implement_phase_slices``. Single-slice and no-slice
contracts continue to use the legacy monolithic path so existing
pipelines are unaffected.
The gateway-side ``list_open_prs`` / ``list_remote_branches`` helpers
needed by the reconciler to actually find orphan PRs ship in a
follow-up — the daemon currently sees no orphans and is a clean no-op
on each tick. The wire-up itself (start / stop, deterministic
shutdown via Event) is exercised by the slice loop's lifecycle.
All 103 slice-DAG tests still pass:
- test_slice_scheduler.py (28 tests)
- test_stacked_pr_reconciler.py (11 tests)
- test_slice_execution.py (13 tests)
- test_slice_pr_creation.py (7 tests)
- test_concurrent_executor.py (44 tests)
Lint clean (ruff check + format).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(2137): tester surface for slice DAG + run-loop wire-up
Combines:
1. Prior tester surface (TASK-1-4 / 2-5 / 3-5 / 4-5 / 5-4) — 99 tests
covering schema rename, forest validation, scheduler state machine,
slice-aware branch naming, BRC tracker namespacing, orphan-PR
detection.
2. New tester surface for the run-loop wire-up (coder commit 36d34da9)
— 49 tests covering _start_stacked_pr_reconciler daemon lifecycle,
_run_implement_phase_slices DAG iteration, _run_concurrent_phase
slice_id env override, _handle_brc_consensus_timeout slice_id
propagation, gateway-side rebase argv canonicality (TASK-5-2), and
orchestrator-side rebase_onto bridge (TASK-5-2).
Files:
- orchestrator/tests/test_slice_scheduler.py (28 tests)
- orchestrator/tests/test_slice_branch_naming.py (13 tests)
- orchestrator/tests/test_stacked_pr_reconciler.py (13 tests)
- orchestrator/tests/test_slice_run_loop_integration.py (20 tests)
- orchestrator/tests/test_gateway_client_rebase_onto.py (13 tests)
- gateway/tests/test_build_rebase_onto_args.py (16 tests)
- shared/egg_contracts/tests/test_slice_migration.py (24 tests)
- shared/egg_contracts/tests/test_validate_forest.py (14 tests)
- shared/egg_contracts/tests/test_plan_parser_dependencies.py (9 updated)
148 net-new tests + 9 updated; ruff + format clean; mypy clean on
shared/gateway. Validates the schema rename, forest validation, slice
scheduler state machine + iterator, slice-aware branch naming, BRC
tracker namespacing, orphan PR reconciliation, orchestrator run-loop
slice integration, per-slice PR creation, and the rebase argv
allowlist invariants.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(2137): v2 — surface coder gaps from holistic NACK as xfail markers
Tester v1 (commit 00ab5723b) drew a NACK from reviewer_code_holistic
flagging three coder-side blocking issues that the test surface did
not catch:
1. _run_implement_phase_slices opens the slice PR with head=
egg/issue-N/slice-M (the integration branch) but never merges/pushes
the per-role agent branches into that integration branch — gh pr
create silently fails on the empty head.
2. _start_stacked_pr_reconciler ships with _list_open_prs /
_list_extant_branches stubbed to empty collections, so the
reconciler is permanently a no-op despite the daemon thread
running cleanly.
3. (out-of-scope for tester role boundary): docs/architecture/
slice-dag.md drift — coder/documenter territory.
Per the tester role boundary I cannot fix the underlying production
code; instead this commit pins the post-fix invariants as
``pytest.mark.xfail(strict=True)`` tests so they (a) fail today
(the bug is present), (b) don't count as red, and (c) become
regression guards once the coder lands the fix — at which point
they pass and ``strict=True`` flags the XPASS as a signal to drop
the marker.
New xfail tests:
* orchestrator/tests/test_slice_run_loop_integration.py
TestCoderGapsSurfacedByHolisticReview:
* test_integration_branch_pushed_before_create_slice_pr — asserts
spawner.gateway.push_worktree_branch is called before
create_slice_pr (holistic NACK #1).
* test_reconciler_detects_real_orphans_not_no_op — asserts the
list_open_prs callable threaded into reconcile_once delegates
to the gateway helper (holistic NACK #2).
Per reviewer_code's non-blocking observations on tester v1 (which
coincide with the coder's open NACKs), this commit also pins:
* shared/egg_contracts/tests/test_validate_forest.py
TestCycleDetection:
* test_two_cycle_rejected — slice-1 -> slice-2 -> slice-1 must
surface an error (xfail until coder wires has_cycle into
validate_forest).
* test_self_loop_rejected — slice-1 -> slice-1 must surface an
error (xfail until coder wires has_cycle into validate_forest).
* orchestrator/tests/test_slice_run_loop_integration.py
test_rebase_onto_callable_bridges_to_gateway: now asserts the
``repo_path`` positional matches the value the production wiring
currently passes, with a comment flagging that the assertion
needs an update once the coder switches to a real repo path
(reviewer_code non-blocking #4).
Test-suite shape after v2:
* 20 passed, 2 xfailed in test_slice_run_loop_integration.py
* 11 passed, 2 xfailed in test_validate_forest.py
* All other slice tests unchanged.
ruff + format clean; mypy clean on shared/gateway.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* implement(2137): v5 — address reviewer_code + reviewer_contract NACKs on v4
Addresses 8 of the 10 blocking findings from reviewer_code (commit
185a08a7) and all 4 blocking findings from reviewer_contract (commit
cff1bb8e) on v4 (HEAD=36d34da9612). Two reviewer_code findings
(EGG_PIPELINE_ID env routing, record_cycle wiring) are documented
trade-offs scoped to the #2199 follow-up.
### Blocking findings closed in v5
- **TASK-2-2 — HTTP 422 surface wired** (reviewer_contract #1):
``orchestrator/routes/phases.py`` ``populate_contract`` now branches
on the ``ForestValidationError`` class name (avoids import cycle)
and returns the structured ``to_response()`` body with
``status_code=422``. Acceptance test "route returns HTTP 422 with
the structured error body when a multi-parent slice is ingested"
is now mechanically satisfiable.
- **TASK-4-2 — Slice integration branch creation**
(reviewer_contract #2): new
``GatewayClient.create_slice_integration_branch(...)`` pushes
``parent_branch:refs/heads/integration_branch`` through the
existing per-agent ``/api/v1/git/push`` allowlist (no new
privileged endpoint, decision-15). The slice loop calls it before
spawning containers and surfaces a clear error log when creation
fails.
- **TASK-4-4 — Wave parallelism** (reviewer_code #3,
reviewer_contract #3, decision-5 hard requirement):
``_run_implement_phase_slices`` now drives the inner loop through
``concurrent.futures.ThreadPoolExecutor(max_workers=len(ready_batch))``
so every slice in a wave spawns simultaneously. The
``max_parallel_slices`` cap from ``iter_ready`` already bounds
``ready_batch``. The previous "future iterations can lift this"
comment is gone; ``_run_one_slice`` is the per-slice worker
function (load contract → write parent_branch → create
integration branch → spawn → wait → create_slice_pr →
record_complete).
- **TASK-5-3 — Reconciler list helpers** (reviewer_code #1,
reviewer_contract #4): ``GatewayClient.list_open_prs(repo)`` and
``GatewayClient.list_remote_branches(repo_path)`` are now
implemented and wired into ``_start_stacked_pr_reconciler``.
``list_open_prs`` routes through ``/api/v1/gh/execute`` with
``args=["pr","list",...,"--json","number,headRefName,baseRefName"]``
(``pr list`` is on ``READONLY_GH_COMMANDS`` allowlist —
``gateway/github_client.py:54``). ``list_remote_branches`` routes
through the existing ``/api/v1/git/fetch`` route with
``operation=ls-remote --heads``. Both return empty on transport
error (the reconciler treats this as "see no orphans this tick"
which is safe).
- **#2 — repo_path bug** (reviewer_code): ``_start_stacked_pr_reconciler``
now accepts ``worktree_repo_path: Path`` keyword and passes the
filesystem path to ``gateway.rebase_onto`` rather than the
branch-name string. Fixes the "every rebase attempt 4xx at the
gateway" failure mode.
- **#5 — State lock** (reviewer_code): the contract
load → mutate ``parent_branch_at_creation`` → save and the
post-CONFIRMED ``create_slice_pr`` re-load are both wrapped in
``with get_pipeline_state_lock(pipeline_id):`` so concurrent
tester / documenter contract writes can't lose data.
- **#6 — Cycle detection in validate_forest**
(reviewer_code + tester xfail): new ``_detect_cycles`` DFS in
``shared/egg_contracts/plan_parser.py`` runs alongside the
multi-parent check. ``slice-1 → slice-2 → slice-1`` is now
rejected with ``"Slice DAG contains a cycle: ..."``. Closes the
silent-deadlock failure mode where ``compute_waves`` sets
``waves=[]`` on cycles and the run loop spins forever.
- **#7 — Scheduler revalidates forest at construction**
(reviewer_code): ``SliceScheduler.__init__`` now calls
``validate_forest(contract.slices)`` and raises ``ValueError``
with the structured errors if the contract bypassed plan-ingestion
validation. Defense-in-depth for legacy state-branch restores and
manual ``egg-contract`` edits.
- **#8 — build_rebase_onto_args ref shape validation**
(reviewer_code): ``branch`` / ``new_base`` / ``old_base`` are now
rejected if they start with ``-`` (flag-shaped), contain
whitespace / NUL, or fail the ``[A-Za-z0-9._/+-]+`` ref-shape
regex. Closes the seam where ``--abort`` would slip through
``validate_git_args`` (it's on the rebase allowlist).
### Cascade emission (TASK-3-4 path)
``_run_implement_phase_slices`` now emits an ``OVERSEER_ALERT``
through the in-process ``message_store`` after each cascade fires,
with metadata ``{anomaly: slice-cascade-block, priority: high,
failed_slice_id, blocked_subtree}``. The orchestrator log line
remains the always-on fallback.
### Trade-offs documented in code (deferred to #2199)
- **EGG_PIPELINE_ID nested-form env override** (reviewer_code #4):
the agent CLI uses one env var for every outbound signal, so
HEARTBEAT and OVERSEER_ALERT also route to the slice tracker
rather than the pipeline tracker. CONSENSUS_* isolation works as
intended; cross-slice telemetry is per-slice today. The
always-on fallback is the orchestrator-side log line +
``slice-cascade-block`` OVERSEER_ALERT emission. Pipeline-level
fan-out for HEARTBEAT requires a CLI-side message-type-aware
router (substantial change to ``shared/egg_orchestrator/client.py``
and the agent CLI) — tracked alongside the per-slice MCP control
verbs in #2199.
- **record_cycle two-tier max_cycles wiring** (reviewer_code #9):
``_run_implement_phase_slices`` records failures via
``record_failure`` directly (single-attempt-per-slice today). The
``EGG_ORCH_SLICE_LOCAL_MAX_CYCLES`` /
``EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES`` knobs are read by the
scheduler but not yet exercised in production. Wiring
``record_cycle`` into the BRC re-proposal seam inside
``_run_concurrent_phase`` is the natural next step but requires
threading the max_cycles trip-flag through the inner BRC loop —
scoped for a #2199 follow-up.
### Tests
All 326 pre-existing slice tests still pass (267 previously +
59 from the in-tree run-loop integration tests landed by tester
in commit 00ab5723b9bb / 1163736e1393). The 4 XPASS(strict)
"failures" are tester xfail markers that flip to PASS because
this commit closes the gaps they pin (#6 cycle detection, #1
reconciler stubs, #2 repo_path). The tester will drop the
markers in their next iteration.
ruff check + ruff format clean on all 6 production files.
Tasks satisfied (added / strengthened in v5):
TASK-2-2 (HTTP 422 wiring), TASK-4-2 (slice integration branch
creation), TASK-4-4 (wave parallelism), TASK-5-3 (reconciler
list helpers + functional reconciliation).
Reviewer-readiness:
- closes reviewer_code v4 NACK findings #1, #2, #3, #5, #6, #7, #8
- closes reviewer_contract v4 NACK findings #1, #2, #3, #4
- defers reviewer_code v4 #4 (EGG_PIPELINE_ID env), #9 (record_cycle)
to #2199 with documented trade-off
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* implement(2137): v6 — close reviewer_code_holistic NACK on v5
Critical fix: in slice mode, agents now share the slice's integration
branch ``egg/issue-N/slice-M`` instead of per-role siblings
``egg/issue-N/slice-M/{role}/work``. Without this fix the per-slice PR
opened by ``create_slice_pr(head=integration_branch, base=parent_branch)``
shows an empty diff because the integration branch points at the
parent's tip while agent commits live on per-role sibling branches
GitHub doesn't see in the PR. The slice work was on origin but
invisible to reviewers.
Adopts holistic NACK option (a) "drop per-role branches in slice mode":
- ``ConcurrentPhaseExecutor.get_worktree_branch(role, slice_id=...)``
now returns ``egg/issue-N/slice-M`` (no per-role suffix) when
``slice_id`` is supplied. Babysit-pr per-role staging is unchanged.
Within a slice, all agents collaborate on one history — the same
shared-branch model the non-slice flow has always used, just scoped
per slice. The slice is the unit of isolation; cross-slice
isolation is preserved by the per-slice integration branch.
Silent-fallback fixes (holistic non-blocking notes):
- ``_run_one_slice``: on ``create_slice_integration_branch`` failure
(return False or exception), now ``record_failure(slice_id)`` and
return early instead of silently spawning agents that would push to
a missing parent. The cascade machinery surfaces the missing-parent
error to the operator via OVERSEER_ALERT.
- ``_run_one_slice``: on ``create_slice_pr`` failure, now
``record_failure(slice_id)`` instead of ``record_complete(slice_id)``
so an empty / failed PR doesn't masquerade as a successful slice.
HITL escalates instead of the cascade machinery thinking everything
is fine.
- ``_run_implement_phase_slices``: scheduler construction now wrapped
in ``try/except ValueError`` so a contract that bypassed plan
ingestion validation surfaces as a structured error in the run-loop
return path rather than crashing the loop.
Lock-scope fix (reviewer_code v5 non-blocking #1):
- ``_run_one_slice``: per-pipeline state lock now only covers the
contract read for the slice-PR data snapshot; the gateway HTTP
round-trip happens after the lock is released so a slow gateway
can't serialise other contract writers for ~30 s.
Known follow-up housekeeping (tester role boundary):
- 3 tests in ``test_slice_branch_naming.py`` assert the old per-role
suffix shape (``egg/issue-N/slice-M/{role}/work``) and now fail
against v6's shared-branch shape. Tester needs to update those
assertions to ``egg/issue-N/slice-M`` to reflect the v6 contract.
Same fix shape as the XPASS(strict) markers from v4 → v5.
Tasks affected: TASK-4-1 (branch naming) — implementation now
correctly matches the slice-as-unit-of-isolation design promised by
the architecture doc and the per-slice PR diff invariant.
Reviewer-readiness: closes reviewer_code_holistic v5 NACK #1
(empty-diff slice PR), addresses non-blocking observations
(silent-fallback in create_slice_pr / create_slice_integration_branch
/ scheduler ValueError swallow + state-lock-around-HTTP).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(2137): v3 — promote xfail regression guards to regular tests after coder v5
Coder v5 (commit 7f4203469) closed the three blocking findings
reviewer_code_holistic flagged on the v1 cycle, plus reviewer_code's
non-blocking gaps that the tester surface had pinned as xfail-strict.
This commit promotes those markers to regular regression guards now
that the underlying production code is correct.
Promoted xfail → regular guard:
* orchestrator/tests/test_slice_run_loop_integration.py
TestCoderFixesForHolisticReview (was TestCoderGapsSurfacedByHolisticReview):
- test_integration_branch_created_before_create_slice_pr (was
test_integration_branch_pushed_before_create_slice_pr): retargeted
at the actual coder seam ``GatewayClient.create_slice_integration_
branch`` (the v1 xfail named ``push_worktree_branch`` which was
the closest existing primitive at NACK time). Now asserts the
call-order invariant: integration branch is created BEFORE the
per-slice PR is opened, so gh pr create finds a populated head.
- test_reconciler_detects_real_orphans_not_no_op: xfail removed —
coder v5 implemented ``GatewayClient.list_open_prs`` and
``list_remote_branches`` and wired them into the reconciler.
* shared/egg_contracts/tests/test_validate_forest.py
TestCycleDetection:
- test_two_cycle_rejected: xfail removed — coder v5 wired
``_detect_cycles`` (DFS) into ``validate_forest``; 2-cycles and
self-loops now produce structured errors at plan ingestion.
- test_self_loop_rejected: xfail removed.
Other fixes:
* test_rebase_onto_callable_bridges_to_gateway: now passes
``worktree_repo_path=Path("/tmp/test-worktree")`` to
``_start_stacked_pr_reconciler`` and asserts the value flows through
to ``gateway.rebase_onto`` as the second positional. This locks in
the coder v5 fix for reviewer_code non-blocking #4 (was passing the
branch string as repo_path; now passes the real filesystem path).
Test-suite shape after v3:
* test_slice_run_loop_integration.py: 22 passed (up from 20+2 xfailed).
* test_validate_forest.py: 13 passed (up from 11+2 xfailed).
* All other slice tests unchanged.
Total: 326 tests, 0 xfails, all green. ruff + format clean; mypy clean
on shared/gateway.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(2137): v4 — track coder v6 shared-branch shape + PR-fail-marks-failed
Coder v6 (commit 97de1061d) lands two behaviour changes that the
tester surface needs to track:
1. **Shared per-slice branch (TASK-4-1 fix for holistic v5 NACK #1):**
``ConcurrentPhaseExecutor.get_worktree_branch(role, slice_id=...)``
now returns ``egg/issue-N/slice-M`` for every role in the slice
instead of the per-role ``egg/issue-N/slice-M/{role}/work`` shape.
This eliminates the empty-diff per-slice PR failure mode where
each role's commits sat on a separate branch the per-slice PR
never referenced.
2. **PR creation failure now marks the slice failed:** the slice
loop's ``record_complete()`` is now gated on successful PR
creation; an exception from ``create_slice_pr`` causes
``record_failure(slice_id)`` and a non-zero overall exit code.
This closes the silent-fallback non-blocking observation from
earlier reviews.
Tester surface updates:
* ``test_slice_branch_naming.py::TestSliceAwareWorktreeBranch``:
- ``test_slice_aware_branch_for_canonical_id`` / ``test_bare_integer_slice_id_normalised`` /
``test_falls_back_to_issue_number_when_no_branch`` now assert the
shared-branch shape ``egg/issue-N/slice-M``.
- New ``test_role_does_not_affect_branch_name_when_slice_set``
samples coder/tester/documenter and asserts every role in
slice-2 returns the same branch — locks in the v6 fix
invariant against future per-role-suffix regression.
* ``test_slice_run_loop_integration.py::TestRunImplementPhaseSlices``:
- ``test_pr_creation_failure_does_not_abort_loop`` renamed to
``test_pr_creation_failure_marks_slice_failed`` and inverted:
PR creation failure must now surface as non-zero exit, not the
previous silent best-effort behaviour. Sibling slice still runs
(decision-2 sibling-independence preserved).
Test-suite shape after v4:
* test_slice_branch_naming.py: 14 passed (up from 13).
* test_slice_run_loop_integration.py: 22 passed (one renamed).
* All other slice tests unchanged.
* Total slice-related: 327 tests, 0 xfails, all green.
ruff check + format clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(2137): v3 — close reviewer_code v2 NACK on doc↔code drift
Address all 10 blocking findings + 3 non-blocking notes from
reviewer_code's NACK on commit 5d3ab5827. The doc was authored before
coder v4 (run-loop wire-up), v5 (8/10 reviewer_code blockers closed),
and v6 (per-slice shared-branch collapse) shipped, so it described a
deferred / library-only state that no longer matches the code on disk.
Blocking #1 — Status banner: rewritten to reflect HITL decision-20
opt-2 ("require wire-up to land here"). The slice loop is live, the
reconciler is functional with live `list_open_prs` / `list_remote_branches`
helpers, integration branches are created on origin before agents
spawn, and per-slice PRs open on consensus reach. Two trade-offs are
called out explicitly: the EGG_PIPELINE_ID nested-form override that
also scopes HEARTBEAT/OVERSEER_ALERT to the slice tracker (decision-14
hybrid honoured partially), and the deferred `record_cycle` two-tier
wiring. Both are scoped to #2199.
Blocking #2 — Per-slice branches & BRC trackers: rewrote the section
for the v6 shared-branch shape `egg/issue-N/slice-M`. The earlier
per-role suffix `egg/issue-N/slice-M/{role}/work` shape produced
empty per-slice PR diffs and was deliberately removed. Doc now says
"the slice is the unit of isolation, not the role within the slice"
and surfaces the multi-agent push attribution dependency on
`gateway/git_client.py:get_attributed_changed_files_in_push` so the
security model is explicit. Notes that the slice run loop creates the
integration branch on origin via `GatewayClient.create_slice_integration_branch`
*before* agents spawn, and on creation failure calls `record_failure`
to arm the cascade timer rather than silently spawning agents.
Blocking #3 + #4 — Two-tier max_cycles section: added "Status:
deferred to #2199" callout. The `record_cycle` invocation point is
not yet wired into the slice run loop; the env knobs are read but the
trip path is dead code today. Configuration knobs table now annotates
`EGG_ORCH_SLICE_LOCAL_MAX_CYCLES` / `EGG_ORCH_SLICE_GLOBAL_MAX_CYCLES`
as "(currently inert)" so operators don't tune them expecting an
effect.
Blocking #5 — Stacked-PR reconciler: documented the live
`GatewayClient.list_open_prs` (gh pr list --json) and
`GatewayClient.list_remote_branches` (git ls-remote --heads) helpers
and confirmed both flow through existing per-agent allowlists
(decision-15 invariant preserved). The reconciler is no longer a
no-op.
Blocking #6 — Plan Parser & Forest Validation: added "Cycle detection"
subsection covering the new `_detect_cycles` DFS that rejects cyclic
chains (e.g. `slice-1 → slice-2 → slice-1`) at plan ingestion. Cited
the structured error format showing the full cycle chain and noting
that multi-parent + cyclic violations are reported in the same returned
list.
Blocking #7 — `SliceScheduler.__init__` constructor revalidation:
added new "Constructor-time forest revalidation" subsection. The
constructor calls `validate_forest` and raises `ValueError` on
multi-parent / cyclic violations so contracts that bypass plan
ingestion (legacy state-branch restores, manual `egg-contract` edits,
in-process fixtures) still hit the gate before the run loop spins.
Blocking #8 — Cascade OVERSEER_ALERT emission: added a paragraph in
the "Failure cascade" section documenting the orchestrator-side
emission via the in-process `message_store`. Body shape and metadata
fields (anomaly, priority, failed_slice_id, blocked_subtree, phase)
are documented. Notes explicitly that this is the always-on safety
net under the v4/v5/v6 EGG_PIPELINE_ID override, since agent-emitted
overseer alerts route to the slice tracker and would otherwise be
invisible at the pipeline level.
Blocking #9 — Wave parallelism: new "Implement-phase run loop"
section documents the wave-parallel slice spawn via
`concurrent.futures.ThreadPoolExecutor(max_workers=len(ready_batch))`.
The pool's max-workers mirrors the `EGG_ORCH_MAX_PARALLEL_SLICES`
budget that `iter_ready` already enforces, so the executor cap and
env knob agree. Walks through the run-loop state machine (construct
scheduler → start reconciler thread → wave loop with parallel
`_run_one_slice` workers → `poll_cascades` after each wave →
loop until `all_done` → tear down).
Blocking #10 — TASK-3-4 cascade alert path: covered by #8's
orchestrator-side emission paragraph in the Failure cascade section.
Non-blocking notes:
- Out of scope (#2137) section now lists the EGG_PIPELINE_ID hybrid
trade-off and the `record_cycle` deferral as explicit carve-outs
rather than burying them in inline notes.
- Per-slice MCP control verbs entry tightened to enumerate
`restart_slice`, `restart_agent` w/ slice_id, `get_slice_status`,
and `list_slices` plus the slice-addressable hooks
(`teardown_slice`, `respawn_slice`, `get_slice_status`) that the
follow-up will wrap.
- Resolved design decisions section adds decision-20 ("operator chose
opt-2 — wire it up here") with citations to commits 36d34da9612,
7f4203469, 97de1061d.
[documenter]
* Persist statefiles after implement phase
* Remove ephemeral agent-output handoff artifacts (#1731)
* Persist statefiles after pr phase
* Update _handle_brc_consensus_timeout call sites in tests for merged signature
The merge brought in main's #2208 fix which added a 'store: StateStore'
positional parameter to _handle_brc_consensus_timeout. Update the three
PR-added test cases in test_slice_run_loop_integration.py to pass a
MagicMock for store; the assertions only inspect the tracker lookup, so
the mock is sufficient.
* Fix unit tests stale after phases→slices rename
Six tests still asserted on the old contract field name 'phases' or
the old slice ID prefix 'phase-N' that #2137 retired. Update them to
match the canonical 'slices' field, 'slice-N' IDs, the post-rename
warning wording, and (in the orchestrator endpoint/audit-event tests)
the renamed ParseResult.to_contract_slices method that the populator
now calls.
* Address PR #2220 review feedback: heal orphaned PRs end-to-end
Reviewers (egg-reviewer) flagged four issues in the slice-DAG implement
loop's stacked-PR reconciler that prevented it from actually healing
orphaned child PRs on origin. This commit addresses all four:
1. Key-shape mismatch (silent no-op). ``find_orphaned_child_prs`` read
``head``/``base`` but ``GatewayClient.list_open_prs`` produces
``head_ref``/``base_ref`` — every PR was silently filtered out. The
consumer now reads the producer's canonical keys with a legacy
``head``/``base`` fallback, and tightens ``pr_number`` validation to
drop records without a real positive integer (was coercing to 0).
2. ``rebase_onto`` only did a local rebase. It is now a three-step
heal flow when ``pr_number``/``repo`` are supplied: rebase via
``/api/v1/git`` → push --force-with-lease via ``/api/v1/git/push``
→ ``gh pr edit --base`` via ``/api/v1/gh/pr/edit``. Short-circuits
on any failure. Legacy local-only path preserved when those
parameters are omitted.
3. Test fixtures encoded the consumer's bug. The reconciler unit
tests now use the producer's normalised ``head_ref``/``base_ref``
shape and add a ``TestProducerConsumerContract`` round-trip that
asserts ``list_open_prs``'s output is consumable without a
translation layer.
4. Missing TASK-5-4 integration test. New
``integration_tests/test_slice_pipeline_e2e.py`` exercises wave
dispatch over a 3-slice forest, the producer/consumer key-shape
contract, and the full rebase → push → pr/edit heal path.
Gateway: ``gh_pr_edit`` route now accepts ``base`` and validates it as
a non-empty string.
— Authored by egg
* PR #2220: address blocking review feedback on reconciler wiring
The egg-reviewer audit at commit 7e60a27 flagged five blockers in the
stacked-PR reconciler's gateway plumbing — every one of them would have
broken the heal flow at runtime. This commit fixes all of them and adds
a real Flask-driven integration test so the regressions can't sneak back
in by stubbing the transport layer.
Blocker 1 — ``force_with_lease`` was silently dropped
``gateway.git_push`` only read ``force``; the reconciler's
``force_with_lease=True`` payload had no effect, so the rebased
branch could not push back to origin (non-fast-forward rejection).
Added ``force_with_lease = data.get("force_with_lease", False)``
parsing and a precedence rule (``force_with_lease`` wins over
bare ``force``).
Blocker 2 — pipeline-session push was rejected for missing consensus
The reconciler runs inside the orchestrator's pipeline session, so
the pipeline-push enforcement (#2028) returned 403 unless
``consensus_push=True`` was set in the payload. Added the marker to
``GatewayClient.rebase_onto``'s push step. Defence-in-depth still
lives in the push-target check (branch must equal the session's
``assigned_branch``), which is set when the session is registered.
Blocker 3 — ``/api/v1/git`` is not a real route
The gateway's git-command endpoint is ``/api/v1/git/execute``.
Updated ``GatewayClient.rebase_onto`` and the corresponding test
literals.
Blocker 4 — ``intended_new_base`` equalled ``deleted_base``
In the merge-cascade case (the *primary* trigger for orphan
detection), ``Slice.parent_branch_at_creation`` names the same
just-deleted branch we're trying to escape from — so retargeting
to it is a no-op. Added ``_resolve_extant_new_base``: walk up
``dependencies[0]`` (forest constraint guarantees ≤1 parent) until
an extant branch is found; fall back to the pipeline branch
``egg/issue-N`` (never deleted by the stacked-PR flow). The unit
tests now cover walk-up, multi-level walk-up, and the fallback.
Blocker 5 — integration test stubbed the transport layer
Added ``gateway/tests/test_reconciler_push_wiring.py`` which drives
Flask's ``app.test_client()`` against the real ``git_push``
handler and asserts:
- ``{force_with_lease: True}`` materialises as
``--force-with-lease`` in the captured ``subpro…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reviewer_codesubagent fan-out (the bulk of Improve generalized reviewer with subagent fan-out and lens reviewers #2061) — the slice scheduler in Independent implement phases #2137 will size implement-phase slices small enough that fan-out is obsolete, and the cleanup is independent of Independent implement phases #2137 landing. Clean tear-out per the issue: no salvage ofReviewerCodeConfig.parallel, no partition cross-pass.reviewer_securityandreviewer_concurrencyfrom ADVISORY → CRITICAL inget_default_implement_graph(). A NACK from either lens now blocks consensus until the producer re-proposes — closes Specialised reviewer findings need severity-based flagging in BRC consensus #1997 (severity-tagged NACK signalling, answered by promoting lenses to CRITICAL).reviewer_code_holistic(BRC: restore always-on holistic reviewer_code alongside fan-out + lenses #2126) stays as the always-on CRITICAL holistic reviewer; prompt wording that referenced "fan-out reviewer" / "slice ACKs" is updated to refer toreviewer_code's line-by-line review.Net change: 137 insertions, 1381 deletions across 20 files.
What was removed
ReviewerCodeConfig+PhaseConfig.reviewer_codefield +get_reviewer_code_parallel()accessor inshared/egg_contracts/models.py.reviewer_code_parallelkwarg + resolver call site inorchestrator/routes/pipelines.py.orchestrator/tests/test_reviewer_code_fan_out_prompt.py,shared/egg_contracts/tests/test_phase_config_reviewer_code.py,integration_tests/sdlc/test_reviewer_1964_regression.py.parallelknob row indocs/guides/sdlc-pipeline.mdand the fan-out paragraphs indocs/guides/concurrent-execution.md,docs/reference/agent-roles.md,docs/architecture/orchestrator.md,docs/reference/checkpoint-browser.md.shared/prompts/REVIEWER-SYNC.md,shared/prompts/code-review-holistic-criteria.md,shared/prompts/security-review-criteria.md,shared/prompts/concurrency-review-criteria.md.What changed criticality
orchestrator/review_graph.pyget_default_implement_graph()— four edges:("reviewer_security", "coder", ADVISORY)→CRITICAL("reviewer_security", "tester", ADVISORY)→CRITICAL("reviewer_concurrency", "coder", ADVISORY)→CRITICAL("reviewer_concurrency", "tester", ADVISORY)→CRITICALtest_review_graph_advisory_reviewers.pyis renamed totest_review_graph_lens_reviewers.pyand rewritten to assert CRITICAL.The lens criteria docs (
security-review-criteria.md,concurrency-review-criteria.md) drop the ADVISORY scope notice and call out the CRITICAL gating with a#2139reference.Closes / supersedes
reviewer_security/reviewer_concurrency— superseded by lens promotion).Test plan
make lint(ruff) — passes.pytest shared/— 1077 passed, 12 skipped.pytest orchestrator/tests/— 4778 passed, 1 skipped.pytest tests/ --ignore=tests/functional— 6599 passed, 28 skipped, 4 pre-existing failures unrelated to this change (tests/llm/claude/test_runner.pyPython 3.14asyncio.get_event_loop()deprecation,tests/sandbox/test_gh_wrapper.pygit-remote-dependent test).pytest orchestrator/tests/test_review_graph_lens_reviewers.py orchestrator/tests/test_reviewer_code_holistic.py— 32 passed.