[issue-2777][slice-1/4] Context-PR opener + wiring (slice-1a) - #2888
Merged
Conversation
13 risks identified. Top three: - R1 HIGH/HIGH: TASK-1-9 (#2570 silent rebase) root cause lies in OOS _sync_worktree_with_remote per decision-11 — HITL inevitable unless re-scoped or deferred. - R3 HIGH/HIGH: TASK-1-5 schema bump misses 5 on-disk contract fixtures, the stacked_pr_reconciler.py cascade-base consumer, and 7 additional context_branch read sites outside TASK-1-2 / TASK-1-7 scope. - R2 HIGH/CERTAIN: TASK-1-6 ConsensusEvaluator deletion undercounts the call surface by two production modules (routes/phases.py:119-124 and routes/signals.py:847-871) plus ~17 tests — runtime ImportError post-deploy. Also documents R4-R13 (PR-phase test surface miss, plan line-number drift, post-consensus-stall predicate semantics, dangling is_context_push, per-slice tracker key cross-task agreement, persistence helper rename, ImportError shim collapse risk, BLE001 audit bar, slice-1 review-cycle budget, integration test fixture path). Includes runtime-primitive audit (#2594) calling out the namesake-namespace carve-out at gateway_client.py:1441 (NOT :2567 as the plan draft incorrectly cites) and trust-boundary review across the orchestrator/gateway/agent boundaries.
…elines.py never existed
The bootstrap-committed plan draft was authored against a prior repo
state where `integration_tests/local_pipeline/` was the kubectl-gated
integration-tests tier. That directory was deleted on 2026-05-11 in
commit `f7803637d1` ("test: delete deprecated local_pipeline + squid
tests"). The plan also referenced
`orchestrator/tests/test_pipelines.py`, which has never existed in
HEAD — orchestrator pipeline tests live under feature-split
`test_pipeline_*.py` / `test_pipelines_*.py` files instead.
Replan changes:
1. Pipeline header now reads `Pipeline: issue-2777-replan` (was
`issue-2777`).
2. Added a re-plan note documenting the #2809 architect-owns-slices
constraint and noting that the architect's scaffold file had not
landed at the time this draft was authored; the slice structure
mirrors the contract's already-populated slices field (the operator
approved this decomposition at decision-13).
3. Updated the Integration-test trust-boundary scope section
(Primitives §10) to reflect the current
`integration_tests/conftest.py` layout:
- `orchestrator_url` fixture at `integration_tests/conftest.py:357`
- `egg_stack.gateway_url` attribute (not a pytest fixture) at
`integration_tests/conftest.py:78`
- kubectl gating via `_kubectl_available` at line 158, skip at 347
- new tests must live under `integration_tests/regression/`
(recovery/regression tier) or `integration_tests/sdlc/`.
4. TASK-1-16 (context-PR up-front integration test) and TASK-2-6
(orchestrator-pod recycle integration test) now place files under
`integration_tests/regression/` and document the path rationale.
5. TASK-1-17 (orchestrator unit-test fan-out) replaces the
`orchestrator/tests/test_pipelines.py` reference with
`orchestrator/tests/test_rebase_pipeline_branch.py` (the existing
dedicated regression file for `_rebase_pipeline_branch_onto_base`)
and `test_pipeline_failure_path.py` / `test_pipelines_api.py` for
the dead-function cleanup grep.
6. TASK-1-15 explicitly creates
`orchestrator/tests/test_context_pr_opener.py` (matches the
established feature-split filename convention) and adds it to the
task's `files` list.
7. Test-strategy prose and PR description now reference
`integration_tests/regression/` and note the legacy directory's
deletion date + commit SHA so reviewers can spot-check.
Substantive content unchanged: the 2-slice decomposition (slice-1 A+D,
slice-2 C; #2792 OUT OF SCOPE per decision-11), all 31 task
descriptions, the Primitives §1 inventory, and the operator's HITL
resolutions (cq-1..cq-10, feedback-1) remain as the operator approved
them. Only the stale file paths were corrected.
Authored-by: task_planner
…alyst integration Replans issue #2777 against current HEAD. Architecture mirrors the operator-approved 2-slice DAG from the prior pipeline (issue-2777): slice-1 = A+D (context-PR collapse + PR-phase deletion + coupled cleanup), slice-2 = C (slice/phase restart hardening, bundles #2409). #2792 remains OUT OF SCOPE per decision-11 (now also independently resolved by #2797). Key replan changes vs the prior plan: - ALL file:line citations refreshed against HEAD. pipelines.py drifted ~+632 lines (upper half) / ~+1383 lines (slice-loop region). No architectural primitives have been removed by intervening commits. - ConsensusEvaluator deletion (cq-5) expanded from 5 to 8 reference clusters: pipelines.py adds 3516-3522 (a 6th cluster not in the prior plan); risk_analyst R2 added phases.py:119-124 and signals.py:847-871 (clusters 7+8). AC-18 enumerates the full 8-cluster surface. - PRMetadata schema (cq-2): augmented with _migrate_schema_version_to_1_2 helper following the existing _migrate_schema_version_to_1_1 pattern to handle ~5 on-disk contract fixtures (risk_analyst R3). AC-19 rewires stacked_pr_reconciler.py's cascade-base consumer to derive from context_pr_number + _resolve_slice_base_branch. - #2570 silent-rebase (TASK-1-9): risk_analyst R1 verified the actual root cause is the bare-rebase fallback inside _sync_worktree_with_remote at pipelines.py:7219-7239 (OOS per decision-11). AC-9a now mandates a PROACTIVE plan-phase HITL registration (not reactive at implement runtime). Recommended resolution: narrow scope override for the bare-rebase fallback only. - PR-phase deletion (cq-4): AC-20 adds gateway/tests/test_phase_transition.py to the gateway test rewrite list, and explicitly PRESERVES the namesake-namespace phase="pr" carve-outs at gateway_client.py:1409/1441, test_session_manager.py, test_gateway.py (NOT to be deleted). - _CONTEXT_BRANCH_RE deletion (cq-4 / R7): AC-21 expands cleanup to cover the dangling is_context_push variable cluster at gateway.py:1344-1392. - Persistence helper (R9): AC-22 mandates _open_context_pr_at_implement_start explicitly names the persistence path (save_contract / update_contract). - _check_post_consensus_stall (R6): AC-23 mandates either re-derived predicate or short-circuit deletion. - _tracker_key shape consistency (R8): AC-24 pins call signature across AC-15 (restart_phase) and AC-16 (startup_reconciliation). - #2409 narrowed: peer_consensus.reconstruct_tracker_from_messages already accepts slice_id at HEAD (peer_consensus.py:1919); message_store already filters on metadata['slice_id'] (:407-416). Gap is only startup_reconciliation.py:312 calling without slice_id. No schema change needed to message_store.Message. Outputs: - .egg-state/agent-outputs/issue-2777-replan-architect-output.json (32 acceptance criteria; 13 risk_analyst findings integrated) - .egg-state/agent-outputs/issue-2777-replan-architect-slices.yaml (slice scaffold: slice-1 root, slice-2 depends on slice-1) #2809 acknowledged: architect retained the 2-slice operator-chosen DAG without further sub-slicing because the natural seams within slice-1 all touch overlapping line ranges in pipelines.py — sub-slicing would convert intra-PR review into cross-PR merge-conflict resolution. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…slice scaffold verbatim Addresses reviewer_plan's 7 blocking items on v1 plus the R7 non-blocking dangling-variable cleanup. Also adopts the architect's binding slice scaffold (`.egg-state/agent-outputs/issue-2777-replan-architect-slices.yaml`, commit `0dc42f4b6`) verbatim for slice headers per #2809. Blocking items addressed: 1. TASK-1-6 (delete ConsensusEvaluator) extends from 5 → 7 reference clusters: adds `orchestrator/routes/phases.py:119-124` (complete_phase handler) and `orchestrator/routes/signals.py:847-871` (READY heartbeat handler). Both were missed by v1's grep claim and would have produced an ImportError at startup. Verified at HEAD via `grep -n "consensus" orchestrator/routes/{phases,signals}.py`. Files list extended; acceptance criteria updated to require all 7 clusters removed. 2. TASK-1-5 schema cleanup gains a migration-entry requirement (`_migrate_schema_version_to_1_2`) — Q5's "no in-flight pipelines" applies to live runs but the 5 on-disk fixtures (issue-2777-replan / 2769 / 2548 / 2474 / 1557-v2) carry the removed fields and would refuse to load without migration. NEW TASK-1-5b added: structural rewire of `orchestrator/stacked_pr_reconciler.py` (10 read sites at HEAD lines 94, 112, 120, 129, 150, 157-158, 247, 275, 283) onto `_resolve_slice_base_branch` from TASK-1-13 / TASK-2-3. Cascade- base safety net (cq-9 intent) preserved through the merge-base fallback. TASK-1-2 extended with 7 surviving read sites in `pipelines.py` (10801, 10804, 10844, 11096-11097, 11519-11542, 16755, 16781, 20193) plus a post-edit verification grep. 3. TASK-1-3 deletion surface extended from 10 → 11 site-categories: adds `shared/egg_contracts/phase_defaults.py:105` (`PipelinePhase.PR: PhaseConfig(...)` row) — verified at HEAD; removing the enum without dropping this row produces a KeyError on startup. Dropped `gateway_client.py:2567` from the namesake- namespace carve-out (verified at HEAD: it's a `gh pr list` CLI args list entry, not the session-namespace `phase='pr'` string). Added preserved-hit enumeration covering `gateway/tests/test_session_manager.py:1127, 1170` and `gateway/tests/test_gateway.py:4371`. Verification grep updated. 4. TASK-1-9 (#2570 silent rebase) restructured to make the OOS-collision EXPECTATION explicit upfront. Both v1 reviewers independently verified the diagnosed root cause lies inside `_sync_worktree_with_remote` (`pipelines.py:7219-7232`, the "#2222 contamination" vector) which is OUT OF SCOPE per decision-11; the AC-9a gate will fire by construction. Default- recommended HITL resolution is option 3 (xfail + follow-up issue). New audit-note artifact `.egg-state/agent-outputs/issue-2777-replan-task-1-9-audit.md` captures the diagnosis before the HITL fires. 5. TASK-1-1 names the persistence helper: `_persist_context_pr_number(pipeline_id, pr_number)` wraps the existing `update_contract` machinery under the per-pipeline state-lock pattern. Helper is called once after the gh-pr-list hit or successful gh-pr-create. Idempotent-path AC now asserts the persistence write is observed even on the idempotent path (covers the resume-from-orphaned-pipeline case). 6. TASK-1-3 (8) extends the overseer-monitor sub-bullet with `_check_post_consensus_stall` semantic rewire (`orchestrator/overseer/monitor.py:1122-1160`, #1911 stall signal). Blanket find/replace would silently weaken the predicate because `context_pr_number` is now set throughout implement (not after a PR-phase boundary). Required choice between (a) delete the short-circuit with proof of unreachability or (b) re-derive the equivalent predicate from `context_pr_number` + slice-closure + consensus-quiescence. Choice and rationale must be stated in the commit message. 7. Approach §"Anchor SHA & re-anchoring" section added documenting that all file:line citations are anchored against refine-commit `1cb235871` and HEAD has drifted by ~600 lines on `pipelines.py` between refine and plan-phase consensus. Implementer protocol mandates `grep -n` re-anchoring at HEAD before editing any cited line, prefer symbol+adjacent-text anchors over absolute line numbers, and commit before-grep output for reviewer verification. Non-blocking R7 addressed: - TASK-1-4 extended with `is_context_push` cleanup (5 references at `gateway/gateway.py:1344-1392`); preferred treatment is full removal of variable + downstream conditional branches; fallback is hard-pin to False with intentional-scaffolding comment. Architect scaffold adoption (#2809): - slice-1 name and goal copied verbatim from the architect's scaffold (commit `0dc42f4b6`). - slice-2 name updated to "Slice/phase restart hardening (bundles #2409)" and goal copied verbatim from the scaffold. - Both slices now use `parent_slice_id` (per the architect's format) instead of `dependencies`. slice-1 has `parent_slice_id: null` (root), slice-2 has `parent_slice_id: 1`. - Re-plan note updated to reflect that the architect's scaffold landed mid-cycle and is now being copied verbatim. Slice/task counts: 2 slices, 25 tasks in slice-1 (was 24 — new TASK-1-5b), 7 tasks in slice-2 (unchanged). Authored-by: task_planner
…16-3526) Addresses risk_analyst's v2 NACK: a single blocking item. risk_analyst independently verified at HEAD (via `sed -n '3510,3530p' orchestrator/routes/pipelines.py`) that pipelines.py has SIX ConsensusEvaluator clusters, not five — making the total EIGHT across the codebase (6 pipelines.py + 1 phases.py + 1 signals.py) rather than seven as v2 enumerated. The missed cluster at HEAD lines 3516-3526 is the "Failed to clear legacy consensus after hard-reset ack" block — a distinct call site from the restart_phase consensus-clear path (cluster 3 at HEAD :3289-3293). Deleting `orchestrator/consensus.py` without removing this cluster guarantees an ImportError post-restart on the hard-reset ack path. Changes: 1. TASK-1-6 description: re-anchored from "SEVEN reference clusters" to "EIGHT reference clusters total" with the new cluster inserted between (3) and (4) in the enumeration. Citations now give BOTH the refine-anchor SHA `1cb235871` line range AND the HEAD line range for each cluster, addressing the risk_analyst non-blocking suggestion to reduce re-anchoring friction for high-leverage tasks. 2. TASK-1-6 acceptance criteria: count updated to EIGHT; explicit acceptance bullet for the hard-reset-ack cluster removal at HEAD :3516-3526. 3. TASK-1-6 files list: adds `orchestrator/peer_consensus.py` to cover the non-blocking risk_analyst follow-on — the `peer_consensus.py:1604` alias method (compatibility shim) must be verified reachable post-deletion; if dead, removed lockstep; if reachable, noted with follow-up issue link in the commit message. 4. TASK-1-6 description: new "Notes / follow-on cleanup" paragraph documents the alias-method verification protocol. No other tasks changed — risk_analyst's remaining ACK-able items (TASK-1-1 persistence helper R9, TASK-1-3 namespace correction R4, TASK-1-9 OOS framing R1, TASK-1-3 (8) monitor rewire R6, TASK-1-5b stacked_pr_reconciler rewire R3) were all positively acknowledged in the v2 NACK. The anchor-SHA re-anchoring section in §Approach is also confirmed addressing R5 cleanly. risk_analyst's commitment: "Re-propose with the cluster count corrected and I will ACK on the next review." Authored-by: task_planner
… 2-slice scaffold
Surfaces the operator's iteration-1 HITL directive ("split slice-1 into
sequential sub-slices 1a/1b/1c") against the architect's iteration-1
response (preserved 2-slice DAG; rejected further sub-slicing because
slice-1 components have overlapping textual conflicts inside
pipelines.py).
Per the task_planner system-prompt directive — "slice composition is NOT
your call (#2809); copy the architect's scaffold verbatim; flag
slice-size concerns in prose so reviewers pick them up as NACK pressure" —
this plan follows the architect's binding 2-slice scaffold without
re-shaping, but adds a prominent "## Iteration 1" section that:
1. Quotes the operator's directive verbatim (slice-1 has 16 coder tasks
> the 14-task slice-1 that wedged the prior run; suggested 1a/1b/1c
sketch).
2. Quotes the architect's defense (cq-1 chose option 4 of 5;
sub-slicing would create cross-PR merge-conflict resolution).
3. Maps the operator's natural-seam sketch onto the existing task graph
for reviewers' reference (1a = primitives+wiring {1-1a, 1-1, 1-13};
1b = scaffold+PR-phase deletions {1-2, 1-3, 1-4, 1-5, 1-5b, 1-6,
1-7, 1-14}; 1c = cohesion-independent cleanup {1-8, 1-9, 1-10,
1-11, 1-12}).
4. Explicitly invites reviewer_plan / risk_analyst to NACK architect
on slice_size if they agree with the operator; commits to
re-proposing this plan against the architect's revised scaffold if
that happens.
Also re-anchors the Anchor-SHA section against current HEAD
(28f7ef9) — re-verifies _should_skip_pr_phase_auto_pr (:8854),
_open_context_pr_for_pipeline (:10634),
_maybe_open_base_pr_for_plan_to_implement (:11280 + call sites :16503,
:22132, :23671, :24666; phases.py:500), the eight ConsensusEvaluator
clusters across pipelines.py / phases.py / signals.py (architect's
AC-18), and gateway primitives at gateway.py:1113 / :1344-1392.
Task content unchanged from v3 — the architect's binding 2-slice
scaffold is unchanged, so the existing 25-task slice-1 + 7-task
slice-2 enumeration carries over verbatim.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rator directive
Operator iteration directive: the prior 16-coder-task slice-1 wedged because the
coder shipped unwired WIP partials it could not finish in one BRC cycle.
Subdivide along natural seams into three sequential sub-slices:
slice-1a (root) — new primitives + wiring (only ADDS code).
`_open_context_pr_at_implement_start`, the
PlanPreflightError validator, `_is_slice_dag_mode`,
`_resolve_slice_base_branch`, and rewires of the
five `_maybe_open_base_pr_for_plan_to_implement`
call sites (pipelines.py:16503, :22132, :23671,
:24666, phases.py:500). Primary use case is
reachable on its own at end-of-1a.
slice-1b (->1a) — scaffold + PR-phase deletions (only DELETES code,
plus the stacked_pr_reconciler cascade-base rewire
that depends on the schema field deletion). The
seven context-PR helpers; the PR phase across ~26
files (orchestrator + gateway + schema + docs +
tests); `_CONTEXT_BRANCH_RE` + `is_context_push`
cleanup; PRMetadata 1.1->1.2 schema bump (cq-2);
`orchestrator/consensus.py` + all 8 reference
clusters (cq-5).
slice-1c (->1b) — cohesion-independent cleanup tail. cq-8 idempotency
in `create_slice_pr`; #2570 silent-rebase fix with
AC-9a HITL gate; per-site BLE001 audit in the
slice-loop region (Q2); slice-loop except-ImportError
shim collapse (Q3); umbrella-terminology structural
deletion (cq-6 subsumes #2389); SliceScheduler #2199
noqa markers + docs (cq-3); stale archaeology
deletions; end-to-end integration test (Q4).
slice-2 (->1c) — restart hardening, unchanged. Eager-persist
parent_branch_at_creation + merge-base fallback
(cq-9); per-slice tracker iteration in
restart_phase; slice_id-threaded reconstruction in
startup_reconciliation (bundles #2409); bootstrap
recognition of non-COMPLETE slices.
Linear DAG: 1a -> 1b -> 1c -> 2 (forest invariant honoured).
ALL other Wave 2 decisions remain binding and unchanged: cq-1's [A+D]->[C]
dependency direction, cq-2..cq-10, feedback Q1-Q5. Only intra-A+D
granularity changes. #2792 / Goal 4 stays OUT OF SCOPE per decision-11
(independently resolved in merged #2797).
File:line citations re-verified at HEAD (28f7ef9) by sub-agent fact-check.
All ~50 cited primitives intact; one drift corrected
(_SLICE_INTEGRATION_BRANCH_RE def at gateway.py:1104, not :1351).
Sub-slice sizes: 1a ~5 coder tasks, 1b ~7, 1c ~7, 2 ~5 — each fits a
single BRC cycle. Intermediate dead-code state of
`_maybe_open_base_pr_for_plan_to_implement` (zero callers after 1a, deleted
by 1b) is explicit in INV-11 / R12 / AC-25.
…or sub-sliced 1a/1b/1c composition Carries forward iteration-0 R1-R13 against the new 4-slice composition (slice-1a new primitives + wiring, slice-1b scaffold + PR-phase deletions, slice-1c cohesion-independent cleanup, slice-2 restart hardening unchanged) per operator directive #2809. R12 (slice size) and R13 (integration test fixture) are RESOLVED by the sub-slicing structural change. NEW risks N1-N6: - N1: inter-slice dead-code window between 1a and 1b (LOW) - N2: gateway phase='pr' namespace coupling — VERIFIED via code-walk at gateway/gateway.py:3680-3710 that filter_operation ValueError is caught and request allowed via backward-compat fallthrough; surfaces as 'Invalid session phase value' WARNING log noise on every PR open post-1b, NOT a structural break. MEDIUM severity + CERTAIN likelihood. Architect must pick mitigation option (a/b/c). - N3: feedback-Q4 integration test in 1c vs 1a placement (MEDIUM) - N4: slices.yaml parent_slice_id encoding for 1a → 1b → 1c → 2 ordering invariant (MEDIUM) - N5: _resolve_slice_base_branch helper signature lockdown (LOW) - N6: sequential merge / rebase conflict risk (LOW) Overall risk: MEDIUM_HIGH (down from HIGH). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… egg/issue-2777-replan-risk_analyst/work
…ised scaffold Risk_analyst NACKed v4 because the architect HAD revised the binding scaffold to 4 slices (commit b6b58a1) after my v4 propose, so v4 no longer matched the binding scaffold. This v5 re-shapes the plan to match the architect's revised 4-slice linear-chain DAG. Slice scaffold (verbatim from .egg-state/agent-outputs/issue-2777-replan-architect-slices.yaml): id=1 "Context-PR opener + wiring (slice-1a)" — parent=null id=2 "Scaffold + PR-phase deletions (slice-1b)" — parent=1 id=3 "Cohesion-independent cleanup tail (slice-1c)" — parent=2 id=4 "Slice/phase restart hardening (slice-2)" — parent=3 Task re-allocation (per architect's AC mapping): - slice-1 (id=1): 3 coder tasks (TASK-1-1 validator, TASK-1-2 opener, TASK-1-3 helpers — all ADDS, no deletions; AC-1, AC-1a, AC-12a, AC-22, AC-25) - slice-2 (id=2): 6 coder + 4 tester = 10 tasks (TASK-2-1..2-10: scaffold deletion, PR-phase deletion across ~26 files, _CONTEXT_BRANCH_RE + is_context_push cleanup, PRMetadata schema + _migrate_schema_version_to_1_2, stacked_pr_reconciler rewire, ConsensusEvaluator 8-cluster deletion; AC-2, AC-3, AC-3a, AC-4, AC-4a, AC-4b, AC-4c, AC-5, AC-5a, AC-18, AC-19, AC-20, AC-21, AC-23) - slice-3 (id=3): 7 coder + 4 tester + 1 documenter = 12 tasks (TASK-3-1..3-12: umbrella deletion, create_slice_pr idempotency, #2570 bundle with AC-9a HITL gate, BLE001 audit, ImportError shim collapse, #2199 noqa markers, archaeology, integration test for context-PR-up-front, sdlc test rewrites, docs; AC-6, AC-6a, AC-7, AC-8, AC-9, AC-9a, AC-10, AC-11, AC-12) - slice-4 (id=4): 5 coder + 1 tester + 1 documenter = 7 tasks (TASK-4-1..4-7: restart_phase per-slice iteration, eager-persist parent_branch_at_creation, merge-base fallback, bootstrap non- COMPLETE handling, startup_reconciliation slice_id threading, tests, docs; AC-13, AC-14, AC-15, AC-16, AC-17, AC-24) Task content preserved verbatim from iteration-0 v3 with re-numbered cross-references (single simultaneous substitution pass over all task bodies and the prose primitive table). Task-ID mapping table is included in the Iteration 1 prose section so reviewers can verify each old task moved to its expected new slot. Re-anchored prose primitive table (TASK-1-13 → TASK-1-3 helpers; TASK-2-3 → TASK-4-3 merge-base fallback); fixed integration-test trust-boundary references (TASK-1-16 → TASK-3-9, TASK-2-6 → TASK-4-6, TASK-1-16a → TASK-3-10); added new primitives for PlanPreflightError, ContextPrCreationError, _persist_context_pr_number, _migrate_schema_version_to_1_2. Operator's "16-task wedge" threshold is no longer crossed at any sub-slice. Max is slice-3 at 12 tasks (7 coder), with cohesion- independence allowing per-task NACKs without blocking the slice. #2792 / Goal 4 remains OUT OF SCOPE per decision-11. All other Wave 2 decisions (cq-1..cq-10, feedback Q1–Q5) preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cts AC-16) Reviewer_plan NACKed v5 on TASK-4-5(1) and TASK-4-5(2), which directed the implementer to add a top-level slice_id field to message_store.Message and extend reconstruct_tracker_from_messages with a slice_id kwarg. Both contradict the architect's binding AC-16 + replan_change_log: these primitives already exist at HEAD per #2725: - peer_consensus.py:1919-1926: signature already has slice_id kwarg - message_store.py:407-416: filter already uses metadata['slice_id'] - routes/messages.py:770: senders already populate metadata['slice_id'] If the implementer followed v5 TASK-4-5(1) verbatim, they would add a redundant top-level field alongside the metadata key, creating a two-source-of-truth bug. If they followed (2), they would re-do an already-landed signature extension. Fix (per reviewer_plan's NACK guidance): - Drop TASK-4-5(1) and TASK-4-5(2). - Reword TASK-4-5 to state the existing primitives are unchanged, with only the startup_reconciliation.py call site and the signals.py handle_consensus_confirmed_signal skip being the gap. - Update acceptance criteria to explicitly state NO Message schema change and NO reconstruct_tracker_from_messages signature change (so a future reviewer cannot misread the task as schema work). - Drop orchestrator/message_store.py and orchestrator/peer_consensus.py from TASK-4-5.files (only startup_reconciliation.py + signals.py are touched). Also fixed reviewer_plan's non-blocking findings: - TASK-3-3 HITL option text "slice-1" → "slice-3" (3 occurrences in the option labels emitted via mcp__sdlc__register_open_question). - TASK-3-3 audit artifact path issue-2777-replan-task-1-9-audit.md → issue-2777-replan-task-3-3-audit.md (matches canonical re-numbering). - TASK-4-1 "wait for the slice-1 rebase" → "wait for the slice-2 (id=2) rebase" (ambiguity removal). All other task content preserved verbatim from v5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Plan->implement populator dropped the linear-chain edges (plan yaml used parent_slice_id, which the yaml-tasks parser ignores). All 4 slices got dependencies=[] -> scheduler spawned them as concurrent roots. Restore the 1->2->3->4 chain so restart_phase implement serializes correctly. See #2870.
…ropped slice keys (#2872) * Fix #2870: emit canonical 'dependencies' in slice scaffold; warn on unknown slice keys The plan→implement contract populator silently dropped the slice dependency chain whenever the architect scaffold expressed ordering via ``parent_slice_id``. ``plan_parser`` reads slice edges only from ``dependencies`` (or the ``depends_on`` alias, #2743) and never ``parent_slice_id``, so multi-slice linear chains parsed to all-roots and ran concurrently — guaranteeing integration-branch conflicts for overlapping slices. Root cause: #2779 (2026-05-22) settled ``dependencies`` as the canonical single-parent key and forbade ``parent_slice_id`` in ``yaml-tasks.schema.json`` (``additionalProperties: false``). #2821 (2026-05-27) then introduced the architect slice scaffold and told the architect + task_planner prompts to emit/preserve ``parent_slice_id`` — a key the parser, schema, and even the task_planner's own worked example do not use. This is that prompt-side drift. Fix (two layers): - Align the prompts back to the canonical vocabulary: the architect scaffold and task_planner copy instructions now emit ``dependencies: slice-<N>`` (omit for roots), matching the schema, the parser, and the task_planner Slice-DAG worked example. No schema or parser vocabulary change — ``parent_slice_id`` stays out, as #2779 intended. - Make the next drift loud, not silent: ``plan_parser`` now emits a ParseWarning when a slice carries a key outside the set it consumes. The schema already encodes this rule but is only enforced in tests, never at parse/populate time — so an unrecognized key (e.g. a future stray ``parent_slice_id``) would otherwise vanish with its data. Tests: parser warns on ``parent_slice_id`` while still documenting the drop; no false-positive on the full known-key set; prompt tests assert the scaffold emits ``dependencies: slice-1`` and no ``parent_slice_id``. * Address review: update stale parent_slice_id docs; generalize warn msg Replace the remaining `parent_slice_id` scaffold-vocabulary references in docs (slice-dag.md, agent-roles.md) with the canonical `dependencies` key this PR settles on, so they no longer contradict the fix or risk re-introducing #2870. The runtime `iter_ready()` tuple reference is left as-is (it is the DAG field, not the scaffold key). Generalize the unknown-slice-key ParseWarning message so it points at the actual stray key(s) instead of hardcoding `parent_slice_id` as the example, which was misleading when a different unknown key triggered it. * ci: re-trigger Test workflow after transient runner disk exhaustion The prior Integration Tests run failed during k3s image import with 'no space left on device' (3/3 import attempts) — a runner infra flake, not a code failure. The same branch passed the Test workflow on the prior commit, and this PR touches no integration-test infrastructure. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
is_slice_branch_merged_into_parent treated 'slice tip is an ancestor of parent' as merged → COMPLETE. An empty (un-started) slice branch's tip is exactly the parent SHA it was forked at, so when the parent advances it becomes a trivial ancestor — falsely marking the slice complete and skipping it, leaving dependents to run without their prerequisite. Git topology alone can't tell an empty branch from a merged one (both are ancestors of the advanced parent), so record the branch's creation base SHA and require the tip to have moved past it. - Slice.integration_base_sha: new optional contract field, the origin SHA the integration branch was forked at. - Persist it once, right after create_slice_integration_branch succeeds (branch fresh, tip == base, no agent spawned yet). - is_slice_branch_merged_into_parent gains an integration_base_sha kwarg: when the tip still equals the recorded base, return False (un-started). Purely additive — an unknown base (legacy slices) falls back to the prior ancestor-only check, so #2549 behaviour is preserved. - Both call sites (bootstrap reconciliation + mid-run race check) pass the recorded base.
…pawn (#2873) * Fix #2869: retry transient gateway connection failures during slice spawn A brief DNS/connection blip to the gateway during agent session registration (and slice integration-branch creation) hard-failed the whole pipeline with no retry — a "Temporary failure in name resolution" during the issue-2777-replan implement phase raised SpawnFailureError ~97s in and auto-failed the run. Classify connection-level failures (URLError: DNS failure, connection refused, host unreachable — the request never landed) as a new GatewayConnectionError subclass of GatewayError, and add a bounded retry-with-backoff helper (_retry_transient) that retries *only* that subclass. Timeouts and 4xx/5xx stay permanent (no retry) so a non-idempotent op can't be duplicated and real errors fail fast. Opt the spawn-critical paths in: spawn-time register_session (the spawner) and every network step of create_slice_integration_branch (session registration, parent/integration ls-remote, fetches, and the push). Retry is opt-in (default off) so other gateway callers — health checks, etc. — keep their existing fail-fast behavior. After the budget (~4 attempts, ~7s) is exhausted the original error propagates, so the #2806 auto-fail safety net still owns sustained outages. GatewayConnectionError subclasses GatewayError, so existing `except GatewayError` handlers are unchanged. * Address review: soften URLError retry-safety phrasing, wrap stray OSError Soften the "always safe to retry / request never reached the gateway" phrasing on the URLError branch and the GatewayConnectionError docstring to "not delivered/processed", and document that a response-phase RemoteDisconnected (a ConnectionResetError, not a URLError) deliberately falls through elsewhere. Add a trailing `except OSError` to _make_request so a response-phase disconnect is wrapped as a plain GatewayError (not GatewayConnectionError, so it is not retried) instead of propagating raw past callers' `except GatewayError` handlers. Covered by a new classification test. * Address review: wrap response-phase HTTPException (IncompleteRead) in _make_request A connection drop during response.read() surfaces as http.client.IncompleteRead — an HTTPException, not an OSError — so it slipped past the trailing except OSError branch and propagated raw, bypassing callers' except GatewayError handlers (e.g. the spawner's KubernetesSpawnError wrap). Add a trailing except HTTPException branch that wraps such response-phase protocol errors as a plain GatewayError, deliberately NOT a GatewayConnectionError (a partial read means the gateway may already have processed the request, so it must not be retried). Placed after except OSError so RemoteDisconnected (both an OSError and an HTTPException) still routes through the OSError branch. Covered by test_incomplete_read_wrapped_but_not_transient. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Document the new Slice.integration_base_sha field added in #2874. The slice-dag architecture doc maintains a table of notable Slice model fields; the new field should be listed there so readers understand its purpose in distinguishing empty (un-started) slice branches from genuinely merged ones on pipeline restart. Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
* ci: free runner disk to fix integration-test disk-pressure flake The integration-test job intermittently failed with all egg pods stuck Pending on an 'untolerated taint(s)' scheduling error. Root cause is disk exhaustion on the runner, not a manifest bug: every egg image is stored twice (Docker daemon overlay + k3s/containerd) across four images x two tags, with the multi-GB egg-litellm base on top of the build cache. The OS disk crosses kubelet's ephemeral-storage eviction threshold around deploy time, kubelet stamps a node.kubernetes.io/disk-pressure:NoSchedule taint, and the egg pods (which don't tolerate it) sit Pending until await-egg-deploy.sh times out. Add two steps: - Free disk space before building: strip preinstalled toolchains egg never uses (~25-30 GB). - Reclaim Docker image store after the k3s import: the daemon copies and build cache are dead weight once images are in containerd (pods pull IfNotPresent from there). Both print df -h / so future disk regressions show up in the logs. * ci: use maintained free-disk-space action instead of hand-rolled rm Swap the hardcoded 'rm -rf' of preinstalled toolchains for jlumbroso/free-disk-space (pinned to v1.3.1), which tracks the runner image layout so we don't chase paths as GitHub changes it. Opt out of: - docker-images: 'make build' runs next and needs its layer cache; the Docker store is reclaimed post-import by 'Reclaim Docker image store'. - tool-cache: this step runs after 'Set up Python', so /opt/hostedtoolcache holds the interpreter uv's .venv links against; wiping it would break the build and test steps. * ci: correct docker-images rationale and reclaim all unused images Address review feedback on the free-disk-space change: - Fix the docker-images:false comment: the action's docker-images:true runs `docker image prune -a` (evicts preinstalled base images make build reuses as cache layers), not the build cache as the old comment implied. - Bump the post-import reclaim from `docker image prune -f` to `-af` so it frees all unused images, not just dangling ones — safe because make build is done and make deploy is kubectl-only, giving more headroom against the disk-pressure eviction threshold. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* Fix #2805: cap egg-owned MCP tool output at the tool layer Defense-in-depth follow-up to #2804/#2810. The Agent SDK message reader crashes the agent (exit 255) when a tool result exceeds its 1 MB JSON buffer; #2810 made that observable and terminal, but the prevention -- never producing an oversized payload -- lands here for the tools we own. New shared helper shared/egg_tool_output.py (flat, stdlib-only so both the orchestrator and the sandbox can import it) exposes two strategies: - truncate + structured marker (head preview + a per-tool 'how to narrow' hint), for paginated/structured tools, and - write-to-file + preview descriptor, for large unpaginated content the agent can re-Read/grep (mirrors Claude Code's own Bash spill). Wired at both egg chokepoints: - Layer 1 (operator-facing): PipelineToolHandler.handle_tool_call caps every dict result before mcp_server.py serializes it, with per-tool narrow hints for the at-risk set (get_service_logs, get_container_logs, list_containers, list_tasks, list_checkpoints, search_checkpoints, list_agent_local_commits). - Layer 2 (sandbox agent @tool): invoke_handler/_success_payload truncate by default; checkpoint_show opts into file-spill since a checkpoint is a full, unpaginated transcript. Cap is 100 KB, override via EGG_TOOL_OUTPUT_CAP_BYTES. Built-in Claude Code tools (Read/Edit/Grep) -- the actual #2777 blocker -- are a different mechanism (PreToolUse predictive cap) tracked in #2876. Tests: shared helper unit tests, layer-1 handle_tool_call cap test, layer-2 truncation + spill tests. * Address review feedback on tool-output cap (#2805) Warn when EGG_TOOL_OUTPUT_CAP_BYTES is set but unparseable/non-positive instead of silently dropping operator config. Spill checkpoint output as indent=2 JSON so Read's line-based offset/limit works, and bound the inline preview to a fixed 4 KB. Measure the orchestrator cap against the indent=2 serialization mcp_server.py actually ships. Cap Layer-2 error payloads. Best-effort prune of stale spill files. Drop the preview under pathologically tiny caps so the marker stays minimal. Document the env var; tighten test assertions to the real cap. * Scale spill preview with cap; tighten error-cap test assertion (#2805) * Fix stale spill_to_file docstring: preview is cap-dependent --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
#2877) * Fix #2876: bound built-in CC tool output via PreToolUse predictive cap Built-in tools (Read/Grep/Edit/Bash) run inside the Claude Code CLI, so egg can't wrap their output the way it caps its own MCP @tool payloads (#2805). A tool result above the Agent SDK's 1 MB JSON buffer kills the agent with exit 255 (#2804); #2810 made that a clean fail-fast but does not prevent it. PostToolUse can't suppress an oversized built-in payload either — it fires after the payload has crossed the channel that crashes the reader (#2810 dropped that approach). This adds the remaining lever: a PreToolUse hook that fires *before* the tool runs and denies calls likely to overflow, with a reason telling the agent how to narrow the call. This is the layer that unblocked the #2777 slice-1 coder, which crashed reading the 1.1 MB, 24k-line orchestrator/routes/pipelines.py whole. - New shared/egg_agent/tool_output_cap.py with predictive heuristics: - Read: no `limit` and file > EGG_READ_CAP_BYTES (default 256 KiB) -> deny, point at offset/limit. - Grep: output_mode=content with no head_limit and no path/glob scope (whole-repo content dump) -> deny, point at head_limit / files_with_matches. Stays narrow to avoid denying common small greps. - Wired as always-on PreToolUse hooks in client.py (mirrors the _deny_web_tools pattern); the overflow hits every route including first-party Opus. Kill switch EGG_TOOL_OUTPUT_CAP=false. - Tests for the predictive-deny path (Read/Grep) and hook registration. - Document the layer in docs/reference/agent-recovery.md. * Address #2876 review: warn on invalid cap env, harden Read heuristics - _read_cap_bytes: log a warning when EGG_READ_CAP_BYTES is set but unparseable or non-positive, instead of silently swallowing the operator's intent and using the default (review blocking item). - check_read_output_risk: gate on an estimated payload (limit × ~bytes/line) rather than the mere presence of a limit, so an oversized limit (e.g. limit=10_000_000) no longer bypasses the cap; tailor the deny remedy for binary files (pages for PDFs, no line-paging advice for images/notebooks, which Read returns whole). - client.py hook: prefer the live PreToolUse cwd, falling back to the launch cwd. - tests: cover the 0/negative/unparseable/unset env branches, oversized and binary limit cases, and pin EGG_TOOL_OUTPUT_CAP so ambient env can't cause spurious failures. - docs: update the predictive-cap table for the new Read semantics. * Address #2876 re-review: honor PDF pages, warn-once on bad cap env Fix the blocking PDF remedy dead-end: an oversized PDF was denied on disk size alone while the deny reason told the agent to use `pages`, so a pages-scoped re-read was denied identically. check_read_output_risk now treats a non-empty `pages` range as bounding for PDFs (mirroring the text `limit` path; the Read tool caps a pages request at 20 pages), closing the loop. Also make the invalid-EGG_READ_CAP_BYTES warning fire once per distinct value instead of on every Read, and give notebooks a jq-oriented remedy instead of the generic file/stat advice. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Adds the new primitives and call-site wiring for slice-1a of the #2777 replan. Slice-1 ADDS primitives only; the legacy `_maybe_open_base_pr_for_plan_to_implement` wrapper is left in place (unreferenced) for TASK-2-1 in slice-2 to delete. TASK-1-1 (plan-phase pre-flight validator): * Adds `PlanPreflightError` (Exception subclass) and `validate_plan_preflight(content)` to `shared/egg_contracts/plan_parser.py`. * Validator rejects malformed planner output with a typed `missing_fields` payload covering the five AC-1a cases (yaml-tasks, pr.title, pr.description, pr.test_plan, pr.manual_steps). * Wires the validator into `advance_phase` (routes/phases.py) so plan→implement transitions surface a 422 with the missing field by name. `force=True` recovery path skips the validator. TASK-1-2 (context-PR opener + persistence helper): * Adds `ContextPrCreationError` and the hard-required, idempotent `_open_context_pr_at_implement_start(pipeline_id)` opener in orchestrator/routes/pipelines.py. * Adds `_persist_context_pr_number` as the single contract-write primitive consumed by the opener (under the per-pipeline state lock + save_contract). * Replaces the soft-fail call at `routes/phases.py:advance_phase` with the new opener; failures surface as 422 with reason codes. * Removes the four legacy `_maybe_open_base_pr_for_plan_to_implement` call sites in pipelines.py (slice-loop entry, implement-entry backstop, run_pipeline autoadvance, HITL resume) per the cq-4 hard-required single-call-site directive. The wrapper itself remains for TASK-2-1 (slice-2) to delete. TASK-1-3 (cq-10 surgical decomposition): * Adds `_is_slice_dag_mode(contract)` and `_resolve_slice_base_branch(contract, slice_id, *, pipeline_id, pipeline_branch)` module-level helpers. * `_is_slice_dag_mode` replaces the 2 surviving bare `len(slices) > 1` recompute sites at HEAD. (The third site cited at the plan-anchored line 15519 no longer exists at HEAD.) * `_resolve_slice_base_branch` provides the helper TASK-2-1 will consume to replace `_resolve_slice_1_context_branch_from_contract`; TASK-4-3 will extend it with a merge-base fallback. Notes for tester: * The deletion of the four legacy call sites breaks two existing test files that explicitly assert the wrapper is called from each site: `orchestrator/tests/test_context_pr_transition_paths.py` and `orchestrator/tests/test_slice_1_context_branch_base_resolution.py`. These need to be updated by tester in this BRC cycle (they are outside the coder file boundary). Verification: * `python3 -m ast` parses all three modified files. * `pytest tests/shared/egg_contracts/test_plan_parser.py` — 126/126 pass (no regressions in plan_parser). * `validate_plan_preflight` smoke-tested against empty content, yaml-tasks-only, and complete plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address reviewer_concurrency v1 NACK: the plan-exit validator and context-PR opener were running AFTER the state-lock-protected phase mutation. On a 422 (validator missing-field) or 422/500 (opener gateway failure) the early-return left the pipeline in IMPLEMENT/RUNNING with no runner thread driving it — the orphan-state hazard. Fix per the reviewer's preferred suggestion: move the entire plan-exit block (validator → populate → commit statefiles → context-PR opener) to BEFORE the `with get_pipeline_state_lock(pipeline_id):` block in advance_phase. Failure paths now return 422/500 BEFORE any phase mutation, so the pipeline remains in PLAN/its prior status — no orphan state. Ordering inside the new pre-lock block (matches reviewer's spec): 1. AC-1a validator (cheap, reads on-disk plan; skipped on force=True). 2. Populate (writes contract.pr.title/description that the opener reads; warn-on-fail per the #1941 recovery-hammer contract). 3. Commit statefiles (so the new runner thread pushes rather than resets the populated contract). 4. Context-PR opener (idempotent; raises ContextPrCreationError on failure → 422). The pre-lock previous_phase is TOCTOU-vulnerable in the sense that a concurrent advance_phase could land first; in that case the lock-acquired validate_phase_transition rejects the second caller with 400, so the only cost of a stale read is one wasted validator+opener cycle — no state corruption. The opener is idempotent on its inner `gh pr list` pre-flight so a second caller that races a successful first opener call re-persists the same PR number. The post-lock block that previously housed this work is reduced to a comment block explaining the move. Non-blocking notes from reviewer_concurrency (kept for slice-3 follow-up): - TOCTOU between `list_open_prs` and `create_pr` in the opener: belt-and-suspenders re-check could be added; deferred since per-pipeline state lock in advance_phase serializes the realistic concurrency surface. - `int(entry["number"])` and `int(match.group(1))` may raise on malformed gateway responses; defer per reviewer's deferral. - Regex `/pull/(\d+)\b` not start-anchored; tightened to `/pull/(\d+)(?:[/?#]|$)` would be hardening but not security. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address reviewer_concurrency v1 non-blocking findings on the new context-PR opener: 1. `int(entry["number"])` at the idempotent-path filter site could raise on a malformed gateway response, escaping the try/except that wraps only the `list_open_prs` call. Wrap the int coercion in its own try/except that raises `ContextPrCreationError(reason= "lookup_bad_response")`. The gateway's existing client-side filter in `list_open_prs` already drops entries with missing `number`, but the explicit guard means a regression in that filter cannot escape the opener's typed-exception contract. 2. Tighten the PR-URL regex from `/pull/(\d+)\b` to `/pull/(\d+)(?:[/?#]|$)` so a hypothetical digit-suffixed slug like `/pulled-files/12345` cannot smuggle a wrong number through, while `/pull/12345/files` and `/pull/12345?diff=split` still parse correctly. Wrap the `int(match.group(1))` in its own try/except for the same defence-in-depth reason. Both findings were flagged as non-blocking (code-quality concerns, not security) but easy hardening in flight. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sites; harden validator; fix local-mode asymmetry) Address reviewer_code_holistic NACK on v1: Blocker 1 — re-add the 4 deleted opener call sites in pipelines.py. The v1 deletion was based on the plan's "single canonical site at phases.py:500 advance_phase" AC, but the existing `test_context_pr_transition_paths.py::TestCallSiteWiring` is the canary: the 4 sites cover the 4 runner-driven paths that bypass advance_phase REST (auto-advance, implement-entry backstop, HITL resume in start_pipeline, and the slice-loop entry safety net). Without them, pipelines that auto-advance plan→implement through `_run_pipeline` or resume through HITL-approval reach the slice loop with `contract.pr.context_pr_number is None` and the slice stack strands on `egg/<id>/work` — the exact #2769/#2593 symptom this issue was filed to fix. Restored all 4 sites to call the new `_open_context_pr_at_implement_start` opener. The opener's `gh pr list` pre-flight makes redundant calls cheap (one round-trip → idempotent hit → re-persist). The 4 safety-net call sites log-and-continue on `ContextPrCreationError`; the canonical `advance_phase` site keeps the hard-required 422 contract (failures there reach the operator). Blocker 2 — tighten the pre-flight validator's silent-fallback paths. v1 had two `except Exception: logger.warning(...); continue` paths around (a) `resolve_worktree_path`/`_get_draft_path` and (b) the plan_parser import + `validate_plan_preflight` call. Either failing silently bypassed the entire validator. Replaced with narrowly-typed handlers that distinguish: - ImportError on validator dependencies → 500 `preflight_unavailable` - OSError on worktree probe / draft read → 500 `preflight_unavailable` - ImportError on plan_parser → 500 `preflight_unavailable` - PlanPreflightError → 422 `preflight_invalid_plan` with missing_fields - Draft path absent / declared-none → INFO log, skip (legitimate) No `except Exception` blocks gate the new feature. Blocker 3 — fix local-mode short-circuit asymmetry. v1's `if not pipeline.repo or not pipeline.base_branch: return None` also silently skipped misconfigured remote pipelines (e.g. `repo` set but `base_branch` empty). Changed to `if not repo and not base_branch` for the genuine local-mode skip, and raise `ContextPrCreationError(reason=MISSING_BASE_BRANCH/MISSING_REPO)` when only one of the two is set. The asymmetric-config case now surfaces as a typed 422 rather than a silent skip. Non-blocking — hoist `ContextPrCreationError.reason` strings to a `StrEnum`. Added `ContextPrCreationReason` with all 15 values that the opener can raise. The constructor validates the reason against the enum at construction so a typo cannot silently slip a new `reason=` into production. Tests in TASK-3-8 will bind on `ContextPrCreationReason.GATEWAY_ERROR.value` etc. — a single source of truth between producer and tests, addressing the synthetic-key concern. Existing `reason="..."` call sites are unchanged (strings are coerced) so no functional change. Non-blocking — the tombstone comment at `_run_implement_phase_slices` that mis-attributed slice-1's state to slice-2 is replaced by the re-added opener call site, which is self-documenting. Verification: AST parse OK on both files; plan_parser tests 126/126 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ependencies[0]) Two blocking findings from reviewer_code v2: 1. `validate_plan_preflight` AC-1a case (e) was silently passing. `ParseResult.pr_manual_steps` cannot distinguish "key missing" from "key present with empty value" because the parser's `extract_pr_metadata_from_yaml` normalises both to `""` via `_normalize_optional_string`. So `if result.pr_manual_steps is None` never fired and a plan missing the entire `manual_steps` key passed the validator silently. Fix: inspect `result.raw_yaml["pr"]["manual_steps"]` for key presence directly, which is structural rather than value-shape- dependent. Verified manually: - Plan without `manual_steps` key → raises with `missing_fields=['pr.manual_steps']`. - Plan with `manual_steps: ""` → passes (empty value is allowed per the AC). 2. `_resolve_slice_base_branch` non-root branch was dead code. The helper read `getattr(slice_record, "parent_slice_id", None)`, but `shared/egg_contracts/models.py:341` defines the field as `dependencies: list[str]` (the canonical post-#2137 forest- constraint key). `getattr` always returned `None` so the non-root branch at the bottom of the function was unreachable; every slice resolved to `pipeline_branch`. This would have silently mis-routed non-root slices when TASK-2-1 in slice-2 wires the helper in. Fix: derive `parent_slice_id = deps[0] if deps else None` from `slice_record.dependencies`. Mirrors the existing convention at `slice_scheduler.py:245` and `pipelines.py:2598`. Verified manually: - Root slice (no dependencies) → returns `egg/<id>/work`. - Non-root slice (depends on slice-1) → returns `egg/<id>/slice-1`. - Eager-persisted parent overrides both → returns recorded value. Docstring updated to name `slice.dependencies[0]` as the source field. Verification: AST parse OK; `pytest tests/shared/egg_contracts/test_plan_parser.py` — 126/126 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tester v3 NACK blocker: v2's reorder of the plan-exit work to pre-state-lock inadvertently narrowed the outer conditional from `if previous_phase == PipelinePhase.PLAN:` to `if previous_phase == PipelinePhase.PLAN and target_phase == PipelinePhase.IMPLEMENT:`, which moved the `_populate_contract_from_plan_safe` + `_commit_statefiles_to_worktree` block inside the narrower condition. This regressed #1941: a `force=true` advance from PLAN → PR no longer populates `contract.pr.title` / `contract.pr.description` from the plan's yaml-tasks appendix, so the PR-phase auto-PR path falls back to placeholder title/body — the exact symptom #1941 fixed. Restructured to: ``` if previous_phase == PipelinePhase.PLAN: if target_phase == PipelinePhase.IMPLEMENT and not force: # validator (already gated on plan→implement non-force) # populate (runs for ANY plan-exit — #1941 contract) if target_phase == PipelinePhase.IMPLEMENT: # opener (only runs on plan→implement) ``` The outer condition stays wide (`previous_phase == PLAN`); only the validator and opener arms narrow to `target_phase == IMPLEMENT`. Populate runs uniformly on every plan-exit per the recovery-hammer contract. Verification: - `pytest orchestrator/tests/test_advance_phase_populate_on_plan_exit.py` — 5/5 pass (previously 2 failed: `test_force_advance_out_of_plan_calls_populate` and `test_populate_is_followed_by_commit`). - `pytest tests/shared/egg_contracts/test_plan_parser.py` — 126/126 pass. - AST parse OK. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(tester v4 NACK) Tester v4 NACK blocker: `make lint` fails with `UP037` on `pipelines.py:96` because `reason: "str | ContextPrCreationReason"` is a stringified forward reference, but `ContextPrCreationReason` is defined earlier in the same module. The quotes are unnecessary. Fix: drop the quotes — `reason: str | ContextPrCreationReason = ContextPrCreationReason.UNKNOWN`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
Response to egg-reviewer re-review feedback (commit
|
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Re-review of commit eecf368 over prior review at e87257e
Delta over the previous re-review is 97 LOC: 4 new tests in orchestrator/tests/test_open_context_pr_at_implement_start.py covering the three ContextPrCreationReason branches I flagged as non-blocking last round (ROUTES_UNAVAILABLE, LOADER_UNAVAILABLE — both the opener and the persist helper have their own local import, so two pins — and CONTRACT_LOAD_FAILED).
Verified
Each test binds on a real production branch. I traced the four call sites:
test_routes_import_failure_raises_routes_unavailable→pipelines.py:11539(from routes import get_state_store_for_pipeline, resolve_worktree_pathinside the opener). Themonkeypatch.setitem(sys.modules, "routes", None)exploits the documented Python import-machinery contract thatsys.modules[name] is NoneraisesImportErroron the nextfrom <name> import ....test_loader_import_failure_raises_loader_unavailable(opener) →pipelines.py:11661(from egg_contracts.loader import load_contracton the post-list_open_prsmiss path). The patches before the masking ensure flow reaches that branch.test_load_contract_failure_raises_contract_load_failed→pipelines.py:11671(except Exceptionaroundload_contract(identifier, worktree_repo_path)). Standardside_effect=RuntimeError(...)patch on the loader.TestPersistContextPrNumber::test_loader_import_failure_raises_loader_unavailable→pipelines.py:11442(the persist helper's own local loader import).
Each test verifies both the typed reason and the cause chain (isinstance(exc_info.value.cause, ImportError) / RuntimeError), so a regression that drops cause=imp_err from the typed-exception construction would be caught — not just the reason string.
Test isolation is sound. monkeypatch.setitem is function-scoped and reverts after each test. I ran the 23-test file end-to-end — no ordering dependency, no leaked module masking.
Mocks only at boundaries. routes.get_state_store_for_pipeline, routes.resolve_worktree_path, routes.pipelines._get_spawner, and egg_contracts.loader.load_contract are the only patches. The production helpers (_pipeline_identifier, _compute_gateway_mode, the open-PRs loop, _persist_context_pr_number) all run real.
Minor
The monkeypatch.setitem(sys.modules, name, None) pattern leans on a Python implementation detail (documented but not part of typing.TYPE_CHECKING-style stable API). It's the right move here — there is no other clean way to make an in-test from <fully-imported-module> import ... raise ImportError — and the same pattern would be used in a chaos-injection harness. Not blocking; flagging only because a future Python could in principle change the contract.
Disposition
This was the only outstanding (non-blocking) item from my last round. All six prior blockers + non-blockers remain fully addressed in e87257e. With the new coverage in eecf368, every ContextPrCreationReason value the producer can raise has an explicit unit-test pin.
Commenting per the self-authored-bot-PR convention.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Response to egg-reviewer re-review feedback (commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…l-output-cap reframing (#2885)
Contributor
Author
Conflict Resolution SummaryResolved merge conflicts with
Why this is safe: I verified Compatibility check: The only non-conflicting PR consumer of these APIs is Verification:
Please review: No semantic conflicts existed — all six were stale-vs-newer on files the PR didn't actually modify. Reviewer should still spot-check that the post-#2885 SDK reader-buffer change doesn't interact badly with the new context-PR opener path, but the opener doesn't touch any of the SDK/tool-cap surfaces so I don't expect coupling. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
There was a problem hiding this comment.
Contract Verification — PR #2888 (slice-1 of #2777)
Verified the three slice-1 tasks (task-1-1, task-1-2, task-1-3) against the contract acceptance criteria. The implementation is complete and well-tested, with two documented, reviewer-driven deviations that I want to flag explicitly so a human reviewer can sign off on them.
Verification context
- Contract file (
.egg-state/contracts/issue-2777-replan.json) carriescurrent_phase: refine, all slices/tasksstatus: pending, andacceptance_criteria: []at the top level. There are no top-levelac-Ncriterion IDs — every acceptance criterion lives in the per-taskacceptance_criteriastring field. Consequentlyegg-contract verify-criterion --criterion ac-Nis not applicable (and the orchestrator is unreachable in this sandbox regardless). - Verified against the diff
origin/main..b5fac8c(the PR head merge commit, which took main's tool-output-cap reframing #2885).
TASK-1-1 — Plan-phase pre-flight validator — MET
PlanPreflightErrorclass added atshared/egg_contracts/plan_parser.py:76with structuredmissing_fieldspayload, empty-payload guard, and stable rendered message that names the first missing field. Exported via__all__.validate_plan_preflight()atshared/egg_contracts/plan_parser.py:1835implements all five rejection cases (a–e), including the structuralmanual_stepskey-presence check viaraw_yamlinspection (rather than the normalised value, which would conflate "key missing" with "empty string").- Surfaces ALL missing fields in a single raise (operator sees the full picture in one NACK), not just the first.
- Wired into
orchestrator/routes/phases.pyat the plan→implement branch BEFORE the state lock, with narrowly-typedOSError/ImportErrorexception handlers that distinguish "infra unavailable" (500preflight_unavailable) from "plan malformed" (422preflight_invalid_plan) from "draft absent" (silent skip). - Tests at
tests/shared/egg_contracts/test_plan_parser.py:2318: 11 dedicated tests including each of the five rejection cases individually, all-fields-missing aggregation, malformed YAML, empty content, and themanual_steps: ""happy-path round-trip.
Minor note (non-blocking): the task description called for PlanPreflightError(BaseException); the implementation derives from Exception. The docstring justifies this — BaseException would force every except Exception site to be audited, and the contract AC only requires "typed PlanPreflightError" without specifying the base class.
TASK-1-2 — Context-PR opener — MET with reviewer-driven deviations
_open_context_pr_at_implement_start()atorchestrator/routes/pipelines.py:11488is idempotent (list_open_prsfilter for our head→base), readscontract.pr.title/contract.pr.description(not the removedcontext_title/context_description), and raisesContextPrCreationErroron every gateway / contract / persistence failure.ContextPrCreationError+ closedContextPrCreationReasonStrEnum carry typed reasons; unknown reason coerces toUNKNOWNwith a loud warn-log rather than escaping asValueErrorpast the typed-exception handlers._persist_context_pr_number()atpipelines.py:11400wraps the contract write underget_pipeline_state_lock(pipeline_id)and is the sole writer ofcontext_pr_numberon the opener's paths. Called exactly once per opener invocation, on BOTH the idempotent hit AND the create-success branch (the resume-from-orphaned-pipeline recovery the AC explicitly requires).- Canonical wiring at
phases.py:advance_phaseplan→implement is hard-required: aContextPrCreationErrorreturns 422context_pr_open_failedBEFORE the state-lock-protected mutation runs, so a failure leaves the pipeline in PLAN rather than a stranded IMPLEMENT/RUNNING. - Tests at
orchestrator/tests/test_open_context_pr_at_implement_start.py(615 LOC, 25+ tests): idempotency test explicitly assertscreate_pr.assert_not_called()AND_persist_context_pr_numberIS called with the existing PR number; happy-path; every typed reason; import failures;ContextPrCreationErrortypo coercion.
Deviation 1 (acknowledged & justified): AC says "The four soft-fail call sites at pipelines.py:15120, 20572, 22051, 22994 are removed." The implementation kept all four (slice-loop entry, implement-entry backstop, _run_pipeline auto-advance, HITL-resume) as best-effort safety nets, swapping the legacy _maybe_open_base_pr_for_plan_to_implement for the new opener and downgrading the call style to log-and-continue (vs hard-required at advance_phase REST). The justification is reviewer_code_holistic blocker 1: runner-driven plan→implement paths do NOT route through advance_phase REST, so deleting them silently strands slice stacks on egg/<id>/work (the exact #2593/#2769 symptom this work is meant to remove). The new opener's gh pr list idempotency makes the redundant calls cheap. This contradicts the literal AC text but is the right call — flagging for human sign-off on accepting the AC-vs-implementation divergence.
Deviation 2 (minor): AC says "no return None swallow path". The opener DOES return None for the local-mode short-circuit (not repo_set and not base_set), with an info log and a docstring carve-out matching the legacy wrapper's local-pipeline behaviour. Partial-config pipelines (asymmetric repo/base_branch) DO raise (covered by typed tests). I read the AC as targeting the unconditional swallow that the legacy wrapper had; the documented local-mode skip is a deliberate, tested exception.
TASK-1-3 — Surgical helpers — MET
_is_slice_dag_mode(contract)atpipelines.py:11751with full docstring; called at the 2 surviving sites (_should_skip_pr_phase_auto_pr:9002and the_run_pipelineslice-loop selector at:23401). AC permits "2 or 3" depending on TASK-2-2's outcome, and TASK-2-2 (PR-phase deletion in slice-2) is still pending, so 2 sites is in-range._resolve_slice_base_branch(contract, slice_id, *, pipeline_id, pipeline_branch)atpipelines.py:11771with a comprehensive docstring covering all three resolution arms (eager-persistedparent_branch_at_creation, root-slicepipeline_branch, non-root derived fromslice.dependencies[0]). RaisesValueErrorif the requested slice id is absent.- Per the docstring's "Consumed by slice-2 TASK-2-1" tombstone, the helper is intentionally not yet wired into the slice-loop's base-branch derivation; slice-2 will land that. Indirect test coverage via
test_slice_1_context_branch_base_resolution.pyexercises the slice-1 root resolution paths.
Minor gap: The slice-1 test plan called for direct unit tests of both helpers, but only _resolve_slice_base_branch has indirect coverage; no test imports or calls _is_slice_dag_mode directly. Contract AC only requires existence + docstrings + correct wiring (all met), so this is not a blocker — flagging for slice-3's TASK-3-8 to pick up if it hasn't already.
No regressions found
test_context_pr.py:test_call_site_is_gated_on_plan_phaseandtest_context_pr_transition_paths.py:TestCallSiteWiringwere intentionally removed (commented tombstones in place) because they enforced the old soft-fail wrapper's exact call-site count — the new opener replaces that check via the typedadvance_phaseintegration tests.test_advance_phase_thread.pyandtest_phase_error_reason_codes.pycorrectly patch out_open_context_pr_at_implement_startso existing focused tests don't take the new opener path.
Verdict
All three slice-1 tasks meet their acceptance criteria with the two documented deviations called out above. The deviations are coherent responses to reviewer feedback already captured in the commit history (v3 reviewer_code_holistic blocker 1, v3 reviewer_code v2 NACK blocker 1) and the docstrings of the affected functions. I am approving the contract surface; the AC-text-vs-implementation deltas in TASK-1-2 deserve a human eyeball before merge.
— Authored by egg
Contributor
There was a problem hiding this comment.
Re-review of merge commit b5fac8c over prior review at eecf368
Delta since the last review is a single first-parent commit: the Merge origin/main (b5fac8c). No PR-authored code changes beyond conflict resolution. The slice-1 surface area (ContextPrCreationReason/Error, _open_context_pr_at_implement_start, _persist_context_pr_number, _resolve_slice_base_branch, the 23-test test_open_context_pr_at_implement_start.py file, the 10-test TestValidatePlanPreflight class) is preserved verbatim through the merge — confirmed by git diff b5fac8c d669b17 -- orchestrator/routes/pipelines.py showing only the PR-side ContextPrCreationError/opener content as the delta against main.
The conflict-resolution table in the PR comment is incomplete, and the omission produces a real documentation bug.
Blocking
1. docs/reference/agent-recovery.md ships with a duplicated, self-contradictory ### Predictive Output Cap (PreToolUse) section
The conflict-resolution summary lists six conflicting files (agent-tools.md, client.py, tool_output_cap.py, egg_tool_output.py, test_client.py, test_tool_output_cap.py) and claims all were resolved by taking main's version. docs/reference/agent-recovery.md is NOT in that table — it auto-merged without a textual conflict because the additions on each side landed in non-overlapping line ranges. But the auto-merge is semantically wrong:
- Merge base (
3a51f72): 30###headers, no Predictive Output Cap section. - PR side (
eecf368): 31 headers, added a Predictive Output Cap section at line 136 with the crash-prevention framing from #2876. - Main side (
d669b17): 32 headers, replaced the older section with a cost-discipline reframing from #2885 and added the new SDK-Reader-Buffer section above it. - Merge result (
b5fac8c): 33 headers — both versions of the Predictive Output Cap section now coexist, lines 146–161 (new #2885 framing) and lines 163–178 (old #2876 framing).
The two sections directly contradict each other in the same file:
- Line 149 (first copy, #2885 reframing): "These caps are model-context/cost discipline, not crash prevention (the reader buffer above is the crash fix)."
- Line 167 (second copy, #2876 original): "The fail-fast above is a backstop, not prevention." — i.e., framing the caps as crash prevention.
Same header twice in a row, same source-pointer paragraph twice in a row, contradictory framing, two identical tool tables. A reader scrolling through agent-recovery.md will hit the new framing, then immediately the obsoleted framing it was meant to replace, and have no way to tell which is current. This also breaks any tooling that relies on heading uniqueness (anchor generation, TOC, doc linters).
This is the entire stated purpose of the merge ("take main's tool-output-cap reframing (#2885)") — and the commit failed to take it cleanly. The intended #2885 reframing landed additively alongside the PR's older version rather than replacing it.
Fix: Delete lines 163–178 (the second ### Predictive Output Cap (PreToolUse) section, ending at the blank line before ### Transient Exit Codes). The first section is the post-#2885 version that matches main and is the intended end state.
I verified the rest of the merge result is byte-identical to origin/main (d669b17) on every non-PR file:
$ git diff b5fac8c d669b17 -- Makefile docs/development/STRUCTURE.md \
docs/guides/deployment.md docs/guides/local-quickstart.md \
docs/reference/agent-tools.md orchestrator/consensus_wrapper.py \
orchestrator/peer_consensus.py orchestrator/routes/signals.py \
orchestrator/tests/test_pipeline_prompts.py \
orchestrator/tests/test_producer_push_consensus.py \
orchestrator/tests/test_signals.py scripts/check-egg-images-present.sh \
shared/egg_agent/client.py shared/egg_agent/tool_output_cap.py \
shared/egg_tool_output.py tests/shared/egg_agent/test_client.py \
tests/shared/egg_agent/test_tool_output_cap.py
# empty
So the six explicitly-resolved files were resolved cleanly. agent-recovery.md is the only miss, and it is a doc-only fix.
Verified — prior items remain addressed
All three prior rounds of feedback are preserved through the merge: the force=True opener gate at routes/phases.py:612, the 29-test coverage suite in test_open_context_pr_at_implement_start.py + TestValidatePlanPreflight, the _persist_context_pr_number docstring rewrite, the ContextPrCreationError ValueError fallback, the _resolve_slice_base_branch slice-2 anchor comment, and the legacy-wrapper tombstones. None of the merged-in changes from main touched these symbols.
Non-blocking
2. Compatibility check claim in the conflict-resolution summary is partially overclaimed
The summary asserts the PR has been verified safe against main via "the only non-conflicting PR consumer of these APIs is orchestrator/mcp_tools.py (cap_result_dict)." That check is correct for cap_result_dict (still exported at shared/egg_tool_output.py:188) and for check_builtin_tool_output_risk / check_read_output_risk / check_grep_output_risk / is_output_cap_disabled (still defined at shared/egg_agent/tool_output_cap.py:266, :180, :237, :74). But the doc surface (agent-recovery.md) was not in the compatibility-check sweep, which is how blocker #1 above slipped through. For future stale-merge resolutions, the doc paths under docs/reference/ that cross-reference the same subsystems are worth grepping for duplicated header text as a low-cost check.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The merge in b5fac8c brought in main's #2885 cost-discipline reframing of "Predictive Output Cap (PreToolUse)" but failed to remove the older #2876 crash-prevention version that landed on the PR side earlier in the slice. The two sections were textually non-overlapping at merge time so git auto-merged them additively, leaving agent-recovery.md with two identical "### Predictive Output Cap (PreToolUse)" headers (L146 and L163), contradictory framing in adjacent paragraphs, and a duplicated tool-heuristics table. Delete the second (older) copy. The remaining section is the post-#2885 version that matches main verbatim and is the intended end state.
Contributor
Author
Response to egg-reviewer re-review feedback (merge commit
|
This comment has been minimized.
This comment has been minimized.
Contributor
Author
There was a problem hiding this comment.
Contract Verification (Re-review)
Delta since last review (b5fac8c2 → babdaaef): single docs-only commit.
babdaaef docs(agent-recovery): drop duplicate Predictive Output Cap section
What changed
docs/reference/agent-recovery.md — 17 lines deleted, 0 added.
The previous merge (b5fac8c) brought in main's #2885 cost-discipline reframing of the Predictive Output Cap (PreToolUse) section, but git auto-merged additively against the older #2876 crash-prevention version that the slice had landed earlier. Result: two identical ### Predictive Output Cap (PreToolUse) headers (formerly L146 and L163), contradictory framing in adjacent paragraphs, and a duplicated tool-heuristics table. This commit deletes the older copy.
Verification
grep -n "Predictive Output Cap" docs/reference/agent-recovery.md→ exactly one hit at L146 ✓- The remaining section is the post-#2885 version (
cost discipline, not crash prevention), which matches main verbatim ✓ - Logical ordering preserved: L138-144 Reader Buffer (crash fix) → L146-161 Predictive Output Cap (cost discipline) → L163 Transient Exit Codes ✓
- Deletion is pure subtraction; no code paths, runtime behavior, or tests are affected ✓
Contract state
Pipeline is in refine phase; all slice-1/2/3/4 tasks remain pending; acceptance_criteria array is empty. No contract-level verification surface to update (verify-criterion not applicable). No previously verified criteria to regress.
Verdict
Approve. Pure docs cleanup; resolves a real defect introduced by the prior merge; no contract impact.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review of commit babdaae over prior review at b5fac8c
Delta is a single docs-only commit: 17 lines deleted from docs/reference/agent-recovery.md, removing the duplicate ### Predictive Output Cap (PreToolUse) section the merge had auto-additively introduced. No code changes since the prior re-review.
Blocker — fully fixed
1. Duplicate ### Predictive Output Cap (PreToolUse) section — FIXED in babdaae.
Verified:
grep -n "^### Predictive Output Cap" docs/reference/agent-recovery.mdreturns a single hit at line 146.diff <(git show origin/main:docs/reference/agent-recovery.md) docs/reference/agent-recovery.mdproduces no output — the PR file is byte-identical toorigin/main's version. So the retained section is exactly the post-#2885 cost-discipline framing, not a partially-merged hybrid.- The deleted block is the older #2876 crash-prevention framing ("The fail-fast above is a backstop, not prevention"), correctly identified by the commit message as the section that needed to go.
- No other content was disturbed by the cleanup — the 17-line deletion sits cleanly between two unchanged sections.
Non-blocker — author disagreed, accepted
2. Pre-push heuristic for stale-merge duplicate-header sweep.
Author's framing is right: that's a conflict-resolution workflow change, not a slice-1 code change. The doc bug itself is what was in scope, and it's fixed.
Other section-header repeats in the file — verified intentional
The file legitimately has multiple ### Configuration (Retry Manager L11, Circuit Breaker L75), ### How It Works (L216, L268, L294 — Agent-Level Restart, Phase-Level Restart, Salvaging Unpushed Local Commits), and ### Restart with Backoff (L183) vs. ### Retry with Backoff (L423, Resilience Utilities). All sit under distinct ## sections — none are merge artifacts.
Slice-1 code surface
Unchanged since e87257e / eecf368, which the prior re-reviews approved (context-PR opener with the force=True gate at routes/phases.py:612, PlanPreflightError validator, surgical helpers, 23-test test_open_context_pr_at_implement_start.py, 10-test TestValidatePlanPreflight class). Nothing in this re-review changes that disposition.
The PR is in shape to merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
Author
| egg is addressing review feedback... |
Contributor
Author
Response to re-review on
|
Contributor
Author
|
egg feedback addressed. View run logs 17 previous review(s) hidden. |
jwbron
added a commit
that referenced
this pull request
May 31, 2026
… PR opener [doc-updater] (#2896) * docs: document #2777 plan preflight validator + hard-required context PR opener [doc-updater] Update documentation to reflect changes from #2888 (issue #2777 slice-1): - docs/guides/sdlc-pipeline.md: add note that plan→implement advance_phase now runs a structural pre-flight check before populate; returns 422 preflight_invalid_plan with missing_fields list; force=true bypasses it - docs/architecture/sdlc-pipeline.md: update context-PR parenthetical to note #2777 replaced the soft-fail advance_phase opener with a hard-required _open_context_pr_at_implement_start that runs before the state lock Triggered by: #2888 Authored-by: egg * docs: correct pr.manual_steps semantics; add infra/skip branches Address egg-reviewer feedback on PR #2896: - BLOCKING: The pre-flight paragraph said pr.manual_steps must be non-empty, but validate_plan_preflight (plan_parser.py:1802-1817) deliberately treats it as key-presence only — an empty string is allowed (contract default). The prior wording contradicted the existing 'use an empty string if none' guidance four lines above on line 854. Split the rule so pr.title / pr.description / pr.test_plan stay grouped as non-empty checks, and pr.manual_steps is documented as a present-key check. - Non-blocking completeness: Document the 500 / preflight_unavailable branches (import / OSError) and the silent-skip cases (no draft path declared, draft file absent) so the section is a complete reference for what advance_phase plan->implement can return. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
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.
Issue #2777 — clean up the sliced implementation phase of the SDLC pipeline.
The sliced implement path (
_run_implement_phase_slices/_run_one_slice_innerplus the context-PR machinery ingateway_client.pyandgateway.py) has accreted significantcomplexity across #2137, #2548, #2593, and #2744. A separate
egg/<id>/contextbranch was introduced as a parallel stack root,and every downstream piece of complexity exists only to service that
separate branch: temp-worktree materialisation, two-tier idempotency,
ContextBranchDivergedhandling, a soft-fail wrapper called fromfive sites, an observability-dedup set, and a gateway push-exemption
regex. Each prior recurrence of the "context PR not opened" bug
(#2593 → #2744 → #2769) added another call site to the scaffold
instead of removing the fragility. The PR phase is also a no-op in
slice-DAG mode (
_should_skip_pr_phase_auto_prreturnsTruewholesale), so there is no backstop when the context PR is silently
missed.
This stack realigns the topology and trims the accumulated mess in
four stacked PRs (linear chain 1 → 2 → 3 → 4, per the architect's
iteration-1 sub-slicing of A+D into 1a/1b/1c at the operator's
direction):
Slice 1 (id=1, slice-1a) — Context-PR opener + wiring.
ADDS new primitives only:
_open_context_pr_at_implement_start(hard-required idempotent up-front opener), a
PlanPreflightErrorvalidator at plan-phase completion, and the surgical helpers
_is_slice_dag_modeand_resolve_slice_base_branch(cq-10).Rewires the five
_maybe_open_base_pr_for_plan_to_implementcall sites at
pipelines.py:16503,:22132,:23671,:24666,plus
phases.py:500. The legacy wrapper is left in place butunreferenced.
Slice 2 (id=2, slice-1b) — Scaffold + PR-phase deletions.
DELETES the entire
egg/<id>/contextscaffold:_open_context_pr_for_pipelineand its 21 silent return-Nonepaths,
_lookup_existing_context_pr,_gather_context_pr_files,_persist_context_pr_linkage_on_contract,_maybe_open_base_pr_for_plan_to_implement(now unreferenced),_resolve_slice_1_context_branch_from_contract, the_context_pr_events_emitteddedup set, thecreate_context_branchgateway-client method,ContextBranchDiverged, the_CONTEXT_BRANCH_REgatewaypush-exemption (plus dangling
is_context_push). Deletes thePR phase entirely (
_should_skip_pr_phase_auto_pr+ caller +route registration + all
PipelinePhase.PRreads/writes across~26 files). Removes
context_branch/context_title/context_descriptionfromPRMetadata(schema v1.1 → v1.2with
_migrate_schema_version_to_1_2migrator). Rewiresstacked_pr_reconciler.pycascade-base to derive fromcontext_pr_number+_resolve_slice_base_branch. Deletesorchestrator/consensus.pyand its 8 reference clustersacross
pipelines.py(6),phases.py(1),signals.py(1).Picks one
_check_post_consensus_stallsemantic per AC-23.Slice 3 (id=3, slice-1c) — Cohesion-independent cleanup.
Adds
gh pr listidempotency pre-flight tocreate_slice_pr(cq-8). Diagnoses and stops the silent rebase of
egg/<id>/workonto
main(Pipeline work branch is being rebased onto main, breaking isolation and causing slice rebase conflicts #2570 bundle), with AC-9a HITL gate if diagnosispoints at an OOS primitive. Audits each
# noqa: BLE001swallow-all in the slice-loop region individually (Q2).
Collapses the 9 dual-path
except ImportErrorslice-loop shims(Q3). Structurally deletes the "umbrella" terminology
(cq-6 subsumes Drop "umbrella" terminology from slice-PR code/docs/PR bodies #2389). Adds
# noqa: ARG002/ dead-codemarkers and Add per-slice MCP controls (restart_slice, etc.) for #2137 slice scheduling #2199 docstring banners to the SliceScheduler hooks
(cq-3). Deletes stale archaeology comments. Adds the end-to-end
integration test for the up-front context-PR open path (Q4).
Slice 4 (id=4, slice-2) — Slice/phase restart hardening.
Makes
restart_phaseslice-aware. Eager-persistsparent_branch_at_creationat PENDING→IN_PROGRESS. Adds amerge-base fallback in
_resolve_slice_base_branch. Extendsbootstrap reconciliation for IN_PROGRESS / BLOCKED slices with
commits-on-origin > 0. Adds per-slice consensus tracker
reconstruction in
startup_reconciliation(closes Slice-scoped consensus trackers can't reconstruct from message store after orchestrator restart #2409 —threading
slice_idinto existingreconstruct_tracker_from_messages).Impact: idempotent-by-construction context PR removes the
recurring "context PR not opened" failure class (#2593, #2744,
#2769). Pipelines surviving an orchestrator-pod recycle resume
instead of re-spawning. The schema bump and PR-phase deletion are
breaking changes for the in-flight pipelines; per feedback Q5 none
exist, so the clean break is safe. Net deletions estimated at
~600 lines against ~200 added (new opener, new helpers,
BLE001 audit replacements, tests).
This slice
Context-PR opener + wiring (slice-1a)
Files affected:
shared/egg_contracts/plan_parser.pyorchestrator/routes/phases.pyorchestrator/routes/pipelines.pyTasks:
yaml-tasksblock missing or unparseable; (b)pr.titlemissing or empty; (c)pr.descriptionmissing or empty; (d)pr.test_planmissing or empty; (e)pr.manual_stepsmissing (empty string is allowed). The validator lives inshared/egg_contracts/plan_parser.py(ororchestrator/routes/phases.pyif the planner parser is invoked through the phase router; locate by searching for# yaml-tasksparsing). Raise a typedPlanPreflightError(BaseException)with a structured payload naming the missing field(s) so the BRC NACK surface emits a clear actionable message. Unit test in TASK-3-8: feed three malformed plan drafts (missing yaml-tasks; missingpr:; missingpr.test_plan) and assert the validator raises with the expected field name. Ordering: this validator MUST be in place BEFORE TASK-1-2's runtime opener — the opener depends on a well-formed contract — so prefer to land this first within the slice.PlanPreflightError. - The five rejection cases (a)–(e) are each exercised by a unit test in TASK-3-8. - The NACK message names the missing field by name (not a generic "plan invalid")._open_context_pr_at_implement_start(pipeline_id: str) -> intinorchestrator/routes/pipelines.py. The helper is the single up-front context-PR opener for the plan→implement boundary. Behaviour: (1) callgh pr list --head egg/<pipeline_id>/work --base main --state open --json numberviaGatewayClient.create_pr's existingghplumbing (extract a_gh_pr_list_for_head_basehelper if needed); (2) on hit, persistpr_numbertocontract.pr.context_pr_numberand return it; (3) on miss, callGatewayClient.create_prwith title/description fromcontract.pr.titleandcontract.pr.description(existing fields), persistcontext_pr_number, return; (4) on gateway failure, raiseContextPrCreationError(new typed exception, top-level inpipelines.py) — NO soft-failreturn None. Persistence call site (added per reviewer_plan v2 blocker 5). After TASK-2-1 deletes_persist_context_pr_linkage_on_contract(currently atpipelines.py:9791plan-anchor /:10423HEAD), the new opener becomes the SOLE writer ofcontext_pr_number. To avoid making the opener a non-transactional state mutator, extract a private helper_persist_context_pr_number(pipeline_id: str, pr_number: int) -> Nonethat wraps the contract write through the existing per-pipeline state-lock +update_contractmachinery inpipelines.py(locate the existing pattern viagrep -n "update_contract\|_update_contract\|with _pipeline_state_lock" orchestrator/routes/pipelines.py | head -20). The opener calls_persist_context_pr_number(...)once, immediately after either thegh pr listhit or the successfulgh pr create. The helper is single-purpose (no other consumers); ordering with TASK-2-1 deletion is critical — TASK-2-1 depends on TASK-1-2 having extracted the helper before tearing down the old persistence path. Wire the opener into the single plan→implement transition site: replace the existing_maybe_open_base_pr_for_plan_to_implementcall atphases.py:500(the only call site that survives) and delete the other four call sites (pipelines.py:15120, 20572, 22051, 22994) — those existed only because the soft-fail wrapper needed multiple retry points. Document the new helper with a docstring stating "hard-required; raises on failure; idempotent via gh pr list pre-flight"._open_context_pr_at_implement_startexists inpipelines.py, raisesContextPrCreationErroron gateway failure, noreturn Noneswallow path. -_persist_context_pr_numberexists as a private helper inpipelines.py, wrapsupdate_contract(or the equivalent under the per-pipeline state-lock pattern named bygrep -n "update_contract" pipelines.py), and is called exactly once by_open_context_pr_at_implement_start(after either thegh pr listhit or the successfulgh pr create). - The function is called exactly once per plan→implement transition (viaphases.py:500advance_phase). - The four soft-fail call sites atpipelines.py:15120, 20572, 22051, 22994are removed. - The function usescontract.pr.titleandcontract.pr.description(NOTcontext_title/context_description, which are removed in TASK-2-4). - Idempotency verified by unit test in TASK-3-8: whengh pr listreturns an existing PR, nogh pr createis invoked AND_persist_context_pr_numberIS still called with the existing PR number (the persistence write must be observed even on the idempotent path — covers the resume-from-orphaned-pipeline case where the contract on disk lostcontext_pr_numbermid-run). - A unit test (in TASK-3-8) verifies that a gateway failure surfaces asContextPrCreationErrorand is NOT silently swallowed by the implement-phase entry handler inphases.py(i.e. the error propagates to the BRC surface, not into areturn Nonepath)._is_slice_dag_mode(contract) -> boolas a module-level helper inorchestrator/routes/pipelines.pythat returnslen(contract.slices) > 1. Replace the 3 bare recompute sites atpipelines.py:8259(inside_should_skip_pr_phase_auto_pr— verify the site survives TASK-2-2's deletion; if not, drop this replacement),pipelines.py:15060, andpipelines.py:15519with calls to the helper. (2) Add_resolve_slice_base_branch(contract, slice_id) -> stras a module-level helper. Readscontract.slices[<slice_id>].parent_branch_at_creationand returns it; for root slices (no upstream slice dependencies), returnsf"egg/{pipeline_id}/work". This replaces the deleted_resolve_slice_1_context_branch_from_contractand is extended by TASK-4-3 to include a merge-base fallback. Wire the new helper into the slice-1 base resolution atpipelines.py:15394–15405(note: TASK-2-1 already did this wiring — this task supplies the helper that TASK-2-1 consumes). Ordering: TASK-1-3 must complete BEFORE TASK-2-1 so that TASK-2-1 has a non-empty helper to call._is_slice_dag_modeexists and is called at the 2 or 3 surviving sites (depending on TASK-2-2's outcome). -_resolve_slice_base_branchexists, returnsegg/<id>/workfor root slices andparent_branch_at_creationotherwise. - The new helpers have docstrings.Test Plan
_open_context_pr_at_implement_start(happy / idempotent / hard-required paths),
PlanPreflightErrorvalidator (5 rejection cases per AC-1a),_is_slice_dag_mode,_resolve_slice_base_branch(withoutmerge-base fallback — lands in slice-4).
longer import.
test_dag_visualizer.pyrewritten forIMPLEMENT-terminal.
test_pr_metadata.pyrewritten for theremoved fields + migration tests for
_migrate_schema_version_to_1_2. Gateway-side test filesrewritten for default-deny on
target='pr'.stacked_pr_reconcilercascade-base test exercises the new derivation.
_check_post_consensus_stalltest asserts the new semantic(per AC-23 choice).
create_slice_pridempotency pre-flight; noumbrella string anywhere; 3-5 BLE001 sample sites; Pipeline work branch is being rebased onto main, breaking isolation and causing slice rebase conflicts #2570
invariant test asserts merge-base unchanged after N≥3 phase
transitions with M≥2 main PRs merged in parallel.
integration_tests/regression/(kubectl-gated recovery/regression tier; the legacy
integration_tests/local_pipeline/directory was deletedon 2026-05-11 in commit
f7803637d1) that spawns a sliced-DAG pipeline, asserts a single context PR exists with
head=egg/<id>/work base=main, clearscontext_pr_numberand re-triggers implement-start hook, asserts no duplicate
PR is opened (AC-8). SDLC integration tests rewritten for
implement→complete transition.
restart_phase(per-slice trackerclear), eager-persist of
parent_branch_at_creation(fieldpresent at PENDING→IN_PROGRESS), the merge-base fallback in
_resolve_slice_base_branch, and the extended bootstrapreconciliation that resumes non-COMPLETE slices without
re-spawning.
integration_tests/regression/that kills the orchestrator pod mid-implement on a sliced
pipeline, restarts, and asserts per-slice consensus trackers
reconstruct (Slice-scoped consensus trackers can't reconstruct from message store after orchestrator restart #2409 closure proof / AC-16).
is callable; no behavior change observable yet.
confirm context PR opens automatically; confirm PR phase
removed.
create_slice_pridempotency; confirm no umbrella string;
make test-allgreen.
on a sliced pipeline; restart; confirm slice resumes without
respawning, per-slice consensus trackers report prior state.
Manual Steps
Pre-merge (slice-1, id=1): None. Slice-1a only ADDS code.
Pre-merge (slice-2, id=2):
state at deploy time (feedback Q5 confirmed none; re-confirm
at merge). The PRMetadata schema bump (v1.1 → v1.2) auto-
migrates via
_migrate_schema_version_to_1_2.accepts pushes to
egg/<id>/work; removing_CONTEXT_BRANCH_REmust not leave a hole.
Pre-merge (slice-3, id=3):
mcp__sdlc__register_open_questionif Phase-1 diagnosisidentifies the root cause inside an OOS primitive. The HITL
is mid-implement, not pre-merge.
Pre-merge (slice-4, id=4): None.
Post-merge (slice-1, id=1): None.
Post-merge (slice-2, id=2): None (PR-phase concept closed
structurally).
Post-merge (slice-3, id=3): Close #2389 with a reference to
slice-3's PR (cq-6 subsumes). Close #2570 with a reference to
slice-3's PR (AC-9 invariant test); if AC-9a HITL was option (c)
xfail-and-defer, close instead with the follow-up issue
reference.
Post-merge (slice-4, id=4): Close #2409 with a reference to
slice-4's PR (subsumed).
Stack
issue-2777-replanegg/issue-2777-replan/workSlice slice-1 of pipeline issue-2777-replan. Stacked on top of
egg/issue-2777-replan/work.