Fix #2549: skip already-merged slices on pipeline restart by jwbron · Pull Request #2552 · jwbron/egg

jwbron · 2026-05-07T18:09:40Z

Summary

Closes #2549.

When a slice's PR is merged into the work branch, the orchestrator restart loop has no signal that the slice is done. iter_ready() yields it on the first tick; create_slice_integration_branch tries to push the (now post-merge) parent SHA onto the slice's existing ref; origin rejects the push as non-fast-forward; the slice is marked failed and cascades its descendants in ~5 seconds. The pipeline cannot resume without an operator manually deleting the stale slice ref.

The fix is three pieces:

GatewayClient.is_slice_branch_merged_into_parent — new detection helper. ls-remotes both refs, fetches them, and runs merge-base --is-ancestor existing parent. The inverse direction of the restart_phase fails to recover when slice integration branch already exists with non-ancestor commits from prior run #2512 restart-recovery check.
Bootstrap reconciliation in _run_implement_phase_slices runs once before the run loop. Layer (A): trust slices already marked SliceStatus.COMPLETE on the contract — no GitHub round-trip. Layer (B): for slices still PENDING, call the merged-detection helper; on a hit, mark complete and persist status=COMPLETE so subsequent restarts hit (A).
Race protection in _run_one_slice_inner re-runs the merged-detection right before create_slice_integration_branch, catching the narrow window where a slice's PR is merged after bootstrap but before its wave executes.

The fix also closes a latent gap: Slice.status had COMPLETE since the original schema, and the #2470 restart_agent parent-slice-complete fallback already reads it, but nothing wrote it. The successful-completion path in _run_one_slice_inner now persists SliceStatus.COMPLETE under the per-pipeline state lock — finally giving that reader a real signal.

Test plan

make test — 4039 tests pass
make lint — passes (only pre-existing soft-cap warnings on unrelated files)
New unit tests in tests/test_create_slice_integration_branch.py covering the is_slice_branch_merged_into_parent helper: ancestor / diverged / branch-absent / parent-absent / equal-tips / session-cleanup paths.
New integration tests in tests/test_slice_run_loop_integration.py::TestSliceMergedDetection covering bootstrap layer (A), bootstrap layer (B), pipeline-without-repo guard, run-loop race protection (bootstrap miss + race hit), success-path contract write, and detection-failure fall-through.
Live verification: rerun start_pipeline issue-2474-v2 after this lands. Expected: slice-1 detected as merged (Layer B), persisted status=COMPLETE, slice-2 onward proceeds normally.

When a slice's PR is merged into the work branch, the orchestrator restart loop has no signal that the slice is done — `iter_ready()` yields it on the first tick, `create_slice_integration_branch` tries to push the (now post-merge) parent SHA onto the slice's existing ref, and origin rejects it as non-fast-forward. The slice cascade- fails its descendants in ~5 seconds, blocking the entire stacked-PR workflow until an operator manually deletes the stale slice ref. The fix wires three things: * `GatewayClient.is_slice_branch_merged_into_parent` — a new detection helper. ls-remote both refs, fetch them, and check `merge-base --is-ancestor existing parent`. The inverse direction of the #2512 restart-recovery check. * Bootstrap reconciliation in `_run_implement_phase_slices`. Before the run loop starts, fold in (A) slices already marked `SliceStatus.COMPLETE` on the contract (cheap path; trust the contract) and (B) slices the gateway reports as already-merged (the live #2549 repro path). Both transitions persist `status=COMPLETE` so subsequent restarts hit (A). * Race protection in `_run_one_slice_inner`. Re-runs the merged- detection right before `create_slice_integration_branch` so a slice merged between bootstrap and its wave is also handled. Also closes a latent gap: `Slice.status` had `COMPLETE` as a value since the original schema and the #2470 `restart_agent` parent-slice- complete fallback already read it, but nothing wrote it. The successful-completion path in `_run_one_slice_inner` now persists `SliceStatus.COMPLETE` to the contract under the per-pipeline state lock, finally giving the #2470 reader a real signal.

egg-reviewer

Reviewed end-to-end: gateway helper, bootstrap pass, race-protection in _run_one_slice_inner, contract persistence, scheduler interaction, and tests. The fix correctly addresses the #2549 repro and closes the latent Slice.status == COMPLETE writer gap without breaking existing paths. Approving with non-blocking notes.

Correctness verified

Ancestry direction is correctly inverted from #2512. _sha_is_ancestor(existing_sha, parent_sha) checks whether the slice's tip is reachable from parent — i.e., the slice's commits are already in the parent. The #2512 path checks the inverse (parent reachable from existing, signalling preserved-but-not-merged work). The two cases are mutually exclusive modulo equal-tip, which both handle. (gateway_client.py:1791-1797 vs. gateway_client.py:1954-1971.)
s.dependencies[0] matches scheduler semantics. SliceScheduler._compute_initial_states uses parent = deps[0] if deps else None (slice_scheduler.py:245); the bootstrap pass re-derives parent via the same rule (routes/pipelines.py:12429-12432) and so does _run_one_slice_inner (routes/pipelines.py:12515-12518). Forest validation rejects multi-parent contracts, so single-element addressing is sound here.
Cascade unblocking works. record_complete calls _unblock_children (slice_scheduler.py:354); slices whose only dependency just got marked COMPLETE move from PENDING → READY without manual nudging.
Persistence locking is consistent with the existing pattern. _persist_slice_status_complete uses the same get_pipeline_state_lock + load-modify-save shape as parent_branch_at_creation writes at routes/pipelines.py:12526-12532. Bootstrap is single-threaded; the per-thread race-protection persist path inside _run_one_slice_inner only writes its own slice's status, so the wave-pool threads don't clobber each other.
Contract status writer closes a real gap. Slice.status == COMPLETE had no writer until this PR; the restart_agent parent-complete fallback at routes/pipelines.py:2653 was reading a field nothing populated. Now the success path and both detection paths write it, giving that reader a real signal.
Helper edge cases are handled correctly. Empty branch names, missing refs, equal tips, and merge-base failures all fall through to False (= "not merged" = let the regular create path run). The session is cleaned up via try/finally symmetric with create_slice_integration_branch.

Tests

Unit tests pin the merge-base direction explicitly: assert mb["args"] == ["--is-ancestor", existing_sha, parent_sha] with a comment naming the inversion vs. #2512. That is a real regression guard, not a self-seeding golden.
Integration tests call _run_implement_phase_slices as the entry point — they exercise the production code path through the bootstrap loop and the wave pool, then assert at observable surfaces (calls into _run_concurrent_phase, create_slice_integration_branch, create_slice_pr, and slice.status on the contract). They mock at the gateway boundary, which is the appropriate seam.
The race test (test_run_loop_race_skip_when_slice_merges_after_bootstrap) uses a counter-based side effect to flip the helper's return value between bootstrap and the run-loop check — a clean way to exercise the narrow window without a real concurrent merge.

Non-blocking suggestions

1. Bootstrap pass adds sequential GitHub I/O to startup (low priority)

routes/pipelines.py:12422-12454 iterates each PENDING slice and synchronously runs ls-remote ×2 + fetch ×2 + merge-base. For a contract with N PENDING slices, that's roughly N gateway round-trips before the run loop can start, even in the common case where nothing is merged. The per-slice cost is small but it scales linearly.

Two cheap mitigations if pipelines start growing:

Parallelise the per-slice helper calls (a ThreadPoolExecutor over the slice list, since each call uses its own synthetic session).
Short-circuit on s.status == SliceStatus.COMPLETE before reaching the helper (already done — continue at routes/pipelines.py:12426), and also short-circuit when the integration branch ref doesn't exist on origin. The helper already handles the latter, but a single list_remote_branches call up front could let the bootstrap skip the per-slice ls-remote entirely.

Not blocking; the run-loop race protection covers correctness, so the bootstrap is a pure latency optimisation.

2. Reconciler thread can leak if bootstrap raises (defensive)

_start_stacked_pr_reconciler at line 12322 starts the daemon thread, then the bootstrap loop runs at lines 12420-12461, and only at line 12463 does the try: block (with the matching finally: reconciler_stop.set() at 12936-12941) take ownership of the teardown.

The bootstrap loop body has try/except around the helper and the persist call, so an in-loop error won't escape — but the from egg_contracts.models import SliceStatus at line 12356, the _persist_slice_status_complete definition itself, or any other bootstrap-time failure outside the inner try/except would raise straight out of _run_implement_phase_slices without joining the reconciler.

Cheap fix: move the _start_stacked_pr_reconciler call inside the same try: block as the run loop, or wrap the bootstrap pass in its own try/finally that also stops the reconciler. Currently a theoretical risk, but it's the kind of latent leak that surfaces under unrelated regressions.

3. Bootstrap parent uses `s.dependencies[0]`, not `parent_branch_at_creation` (consistency note)

The contract has slice.parent_branch_at_creation (written by _run_one_slice_inner at line 12530) for exactly the "what was the parent when we made this branch?" question. The bootstrap recomputes it from dependencies[0] instead. Today the two should agree — both are derived via the same rule — but if a future re-plan path mutates dependencies post-creation, the bootstrap check would compare the integration branch against a different parent than the one its branch is actually stacked on, masking a genuine merge.

Not currently exploitable, but a one-line preference for s.parent_branch_at_creation or {issue_branch}/{s.dependencies[0]} (with the existing fallback for slices that never got a _run_one_slice_inner write) would future-proof this.

4. `test_bootstrap_does_nothing_when_pipeline_repo_unset` docstring is slightly misleading

The docstring says "Step (A) still applies because it doesn't need GitHub access," but the test fixture has a single PENDING slice (no COMPLETE slice to exercise step (A)). The assertion — that is_slice_branch_merged_into_parent is never called when pipeline.repo is None — is the right thing to check, but the docstring frames it as a step-(A) test when it's really a step-(B) skip-guard test. Tweaking the docstring (or adding a separate slice with status=COMPLETE to actually cover step (A) under repo=None) would clarify intent.

— Authored by egg

…trap, prefer parent_branch_at_creation, expand test - Move _start_stacked_pr_reconciler call to after the bootstrap pass so an exception during bootstrap (hard imports, programming errors) cannot leak the daemon thread. - Parallelize layer-(B) is_slice_branch_merged_into_parent calls with a ThreadPoolExecutor (cap 8). Each call uses its own synthetic gateway session, so concurrent calls are safe; this keeps startup latency bounded as forests grow. - Prefer slice.parent_branch_at_creation over deriving from dependencies[0] in the bootstrap parent-branch resolution. Today both should agree, but a future re-plan that mutates dependencies post-creation would otherwise compare against the wrong parent. - Expand test_bootstrap_does_nothing_when_pipeline_repo_unset to actually exercise step (A) under repo=None: add a slice with status=COMPLETE alongside a PENDING slice, then assert that step (A) skips the COMPLETE slice and step (B) is wholesale skipped.

james-in-a-box · 2026-05-07T18:32:52Z

Thanks for the thorough end-to-end review. Per-item disposition for the four non-blocking suggestions:

1. Bootstrap pass adds sequential GitHub I/O to startup

fixed-in-PR (commit 2673a56) — parallelised the layer-(B) is_slice_branch_merged_into_parent calls with a concurrent.futures.ThreadPoolExecutor capped at 8 workers and named slice-bootstrap-<pipeline_id>. Each helper call uses its own synthetic gateway session, so concurrent calls don't share state. Aggregation (scheduler.record_complete + _persist_slice_status_complete) stays serial after the executor returns. Layer (A) — the s.status == COMPLETE short-circuit — is still applied first as a no-I/O pass before submitting work to the pool, so contracts whose slices are already recorded as complete pay zero gateway round-trips.

2. Reconciler thread can leak if bootstrap raises

fixed-in-PR (commit 2673a56) — moved the _start_stacked_pr_reconciler(...) call from before the bootstrap pass to immediately before the run-loop try: block. The reconciler doesn't depend on bootstrap state, so the move is safe and the existing finally: cleanly owns its teardown. Now any exception during bootstrap (hard from egg_contracts.models import SliceStatus failure, programming error in the pass) exits without a daemon thread to leak.

3. Bootstrap parent uses `s.dependencies[0]`, not `parent_branch_at_creation`

fixed-in-PR (commit 2673a56) — bootstrap now resolves the parent branch as s.parent_branch_at_creation or f"{issue_branch}/{s.dependencies[0]}" or pipeline_branch. The dependency-derived path remains as a fallback for slices that never made it through _run_one_slice_inner (e.g. fresh contract on first run). Note: the run-loop race-protection check in _run_one_slice_inner already authoritatively derives parent_branch from the parent_slice_id argument it's about to persist as parent_branch_at_creation, so no change was needed there.

4. `test_bootstrap_does_nothing_when_pipeline_repo_unset` docstring is misleading

fixed-in-PR (commit 2673a56) — went with the test-coverage option rather than the docstring tweak. The test now has a slice-1 with status=SliceStatus.COMPLETE alongside a PENDING slice-2 under pipeline.repo = None, and asserts both that step (A) skips slice-1 (only slice-2 invokes _run_concurrent_phase) and that step (B) is wholesale skipped (is_slice_branch_merged_into_parent is never called). The docstring is also updated to match.

— Authored by egg

egg-reviewer

Re-review of commit 2673a566 against my prior e658dbee review. All four non-blocking suggestions from the previous review are properly addressed at the source level.

Verifications

1. Bootstrap parallelisation (`routes/pipelines.py:12434-12482`)

Layer (A) is split out as a no-I/O pass that runs first, populating layer_b_candidates with only the PENDING slices. Layer (B) wraps the per-slice helper call in _bootstrap_check_one, hands it to concurrent.futures.ThreadPoolExecutor(max_workers=min(len(layer_b_candidates), 8), thread_name_prefix=f"slice-bootstrap-{pipeline_id}"), and aggregates results serially after the pool returns.

The if pipeline.repo and layer_b_candidates: guard avoids constructing a zero-worker pool when the candidate list is empty.
bootstrap_pool.map(_bootstrap_check_one, layer_b_candidates) is materialised inside the with block via list(...), so all helper calls complete before the pool shuts down.
Each helper call goes through is_slice_branch_merged_into_parent (gateway_client.py:1736-1747), which registers a unique synthetic session keyed on f"{pipeline_id}-slice-merged-check-{integration_branch.replace('/', '-')}" — slice IDs are unique within a pipeline, so no two threads share a session.
Aggregation (scheduler.record_complete + _persist_slice_status_complete) is serial after the executor returns. Lock acquisition order is consistent.
_bootstrap_check_one captures pipeline_id, worktree_repo_path, issue_branch, pipeline_branch, spawner, gateway_mode, and logger by reference — all read-only within the bootstrap pass. Per-call slice_obj is a parameter, not a captured variable.
concurrent.futures is already imported at the top of the file (pipelines.py:5).

2. Reconciler thread leak fix (`routes/pipelines.py:12493-12500`)

_start_stacked_pr_reconciler(...) is now called after the bootstrap pass (between the bootstrap-summary logger.info and the run-loop try: block). Confirmed at pipelines.py:12493 immediately followed by try: at line 12502 with the matching finally: reconciler_stop.set() at the end of the run loop. No code can raise between the start and the try: block, so the daemon thread is fully owned by the existing teardown.

If a hard ImportError (e.g. the from egg_contracts.models import SliceStatus at line 12356) or a programming error in the bootstrap pass raises, the function exits without ever starting the reconciler — no leak.

3. Bootstrap parent prefers `parent_branch_at_creation` (`routes/pipelines.py:12442-12450`)

Resolution order is now slice_obj.parent_branch_at_creation or f"{issue_branch}/{slice_obj.dependencies[0]}" or pipeline_branch. This matches what the slice was actually forked off of, surviving any future re-plan that mutates dependencies post-creation. The fallback to the dependency-derived parent is preserved for slices that never made it through _run_one_slice_inner (fresh contract on first run). The run-loop race-protection check in _run_one_slice_inner already authoritatively derives parent_branch from the parent_slice_id argument, so no change was needed there — confirmed at pipelines.py:12565-12625.

4. Test now exercises step (A) under `pipeline.repo = None` (`tests/test_slice_run_loop_integration.py:1107-1156`)

test_bootstrap_does_nothing_when_pipeline_repo_unset now adds a slice-1 with status=SliceStatus.COMPLETE alongside a PENDING slice-2 (deps=["slice-1"]). The test asserts both:

spawner.gateway.is_slice_branch_merged_into_parent.assert_not_called() — step (B) is wholesale skipped when pipeline.repo is None.
mock_run_phase is invoked only for slice-2 — step (A) trusted the contract for slice-1 without any gateway round-trip.

The docstring matches the assertions. Both step (A) (covered by the new COMPLETE slice) and the step (B) skip-guard (covered by the assert_not_called() check) are now exercised.

No new issues found

Closure thread safety: captured variables are read-only across the bootstrap pass.
Failure modes: helper failures (gateway down, transient fetch errors, missing refs) all degrade to False ("not merged") and let the run-loop race protection at pipelines.py:12591-12626 catch the case. Same safety floor as before.
Edge cases: len(layer_b_candidates) == 1 correctly produces a single-worker pool; len == 0 is short-circuited by the truthiness guard.
Tests still match the production code path (_run_implement_phase_slices is the entry point), assert at observable surfaces (_run_concurrent_phase / create_slice_pr / slice.status), and mock at the gateway boundary.

LGTM.

— Authored by egg

james-in-a-box · 2026-05-07T18:46:39Z

egg review completed. View run logs

3 previous review(s) hidden.

* Fix #2549: skip already-merged slices on pipeline restart When a slice's PR is merged into the work branch, the orchestrator restart loop has no signal that the slice is done — `iter_ready()` yields it on the first tick, `create_slice_integration_branch` tries to push the (now post-merge) parent SHA onto the slice's existing ref, and origin rejects it as non-fast-forward. The slice cascade- fails its descendants in ~5 seconds, blocking the entire stacked-PR workflow until an operator manually deletes the stale slice ref. The fix wires three things: * `GatewayClient.is_slice_branch_merged_into_parent` — a new detection helper. ls-remote both refs, fetch them, and check `merge-base --is-ancestor existing parent`. The inverse direction of the #2512 restart-recovery check. * Bootstrap reconciliation in `_run_implement_phase_slices`. Before the run loop starts, fold in (A) slices already marked `SliceStatus.COMPLETE` on the contract (cheap path; trust the contract) and (B) slices the gateway reports as already-merged (the live #2549 repro path). Both transitions persist `status=COMPLETE` so subsequent restarts hit (A). * Race protection in `_run_one_slice_inner`. Re-runs the merged- detection right before `create_slice_integration_branch` so a slice merged between bootstrap and its wave is also handled. Also closes a latent gap: `Slice.status` had `COMPLETE` as a value since the original schema and the #2470 `restart_agent` parent-slice- complete fallback already read it, but nothing wrote it. The successful-completion path in `_run_one_slice_inner` now persists `SliceStatus.COMPLETE` to the contract under the per-pipeline state lock, finally giving the #2470 reader a real signal. * Address #2552 review notes: defer reconciler start, parallelize bootstrap, prefer parent_branch_at_creation, expand test - Move _start_stacked_pr_reconciler call to after the bootstrap pass so an exception during bootstrap (hard imports, programming errors) cannot leak the daemon thread. - Parallelize layer-(B) is_slice_branch_merged_into_parent calls with a ThreadPoolExecutor (cap 8). Each call uses its own synthetic gateway session, so concurrent calls are safe; this keeps startup latency bounded as forests grow. - Prefer slice.parent_branch_at_creation over deriving from dependencies[0] in the bootstrap parent-branch resolution. Today both should agree, but a future re-plan that mutates dependencies post-creation would otherwise compare against the wrong parent. - Expand test_bootstrap_does_nothing_when_pipeline_repo_unset to actually exercise step (A) under repo=None: add a slice with status=COMPLETE alongside a PENDING slice, then assert that step (A) skips the COMPLETE slice and step (B) is wholesale skipped. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* Initialize SDLC contract for issue #2548 * Refine analysis for issue #2548 Analyzes the missing analysis/plan/BRC visibility on slice PRs. Compares four options (context PR / embed in slice-1 / embed in terminal slice / render in PR body), recommends Option A (dedicated context PR + per-slice implement BRC files), and registers five HITL decisions plus five open feedback questions on contract. * Persist agent statefile writes before refine sync * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * Risk assessment for issue #2548 plan phase Identifies 14 risks (R1–R14) across compatibility, gateway-policy, schema, security, performance, and operator-experience categories. Captures HITL decisions 1–5 and feedback Q1–Q5 as decision_inputs. Key risks: - R1: Gateway slice-integration regex blocks egg/<id>/context push - R2/R9: Hard-switchover (decision-4) needs operator drain runbook - R5: Public-repo exposure of agent transcripts (Q3 chose include) - R8: New PRMetadata fields must be Optional with safe defaults - R10: Decision-3 covers merge gate but not creation failure semantics Recommends: go-with-conditions, gateway change ships first, schema additions ship with safe defaults, surface 2 new HITL questions (creation-failure semantics, transcript size/scrub). * Plan #2548: context PR + per-slice BRC history Five-slice forest chain (slice-1 → slice-2 → slice-3 → slice-4 → slice-5) following the operator's HITL resolutions: - D1: dedicated context PR - D2: hard-split implement BRC into per-slice files; no aggregate - D3: doc-only auto-open (no merge gate before slicing) - D4: hard switchover, no backfill - D5: context PR base = pipeline.base_branch (not hardcoded main) Slices: contract schema delta -> per-slice BRC writer -> context branch + doc-only PR opener -> slice-1 base wiring + per-slice BRC commit + reconciler fallback -> docs. * Architecture analysis for issue #2548 plan phase Architect output describes the design for landing refine/plan analysis docs, agent transcripts, and refine/plan BRC histories on a dedicated context PR (egg/<id>/context, base=<pipeline.base_branch>) that slice-1 stacks on top of, plus splitting the implement-phase BRC history at write time into per-slice files committed to each slice's integration branch before its PR opens. Reflects HITL decisions 1-5 and feedback Q1-Q5 from the refine phase. Hard switchover for new pipelines only; no backfill. * Persist statefiles after plan phase * Fix #2532: align .github/ block in agent_roles.py for plan and reviewer roles (#2550) * Fix #2532: align .github/ block in agent_roles.py for plan and reviewer roles Adds `.github/` to the `blocked_write` list of every plan-side and reviewer role in `shared/egg_contracts/agent_roles.py` whose `patterns.py` counterpart already blocks it: - ARCHITECT_ROLE, TASK_PLANNER_ROLE, RISK_ANALYST_ROLE - _REVIEWER_BLOCKED_WRITE (covers reviewer_code, reviewer_code_holistic, reviewer_agent_design, reviewer_refine, reviewer_plan, reviewer_security, reviewer_concurrency) - _REVIEWER_CONTRACT_BLOCKED_WRITE The planner prompt reads `agent_roles.py` via `get_file_patterns()`; the gateway reads `patterns.py` via `AgentFilePattern.can_write()`. PR #2525 closed the same drift for the tester (issue #2521); this closes the remaining cases. The disagreement is benign today because every affected role's `allowed_write` is confined to `.egg-state/...` paths that never collide with `.github/`, but bringing the two views into lockstep means the next allowlist widening cannot silently bypass the branch-protection invariant from #2508. Adds `shared/tests/test_github_block_alignment.py` with three parametrized regression tests across the affected roles: agent_roles view blocks `.github/`, patterns view blocks `.github/`, and the two views agree. A "load-bearing" test in #2525's style is not constructible here — the allowlists never intersect `.github/` — so the test instead asserts cross-view consistency. Notes vs. the issue inventory: - REFINER is NOT in scope. The issue lists it, but `REFINER_PATTERNS` in `patterns.py` uses its own custom blocked list (not `_PLAN_AGENT_BLOCKED`), and that list also omits `.github/`. The two views already agree for refiner, so there's no drift to fix — whether refiner *should* block `.github/` is a separate change. - The reviewer count expanded from the issue's 3 (reviewer_code, reviewer_code_holistic, reviewer_contract) to 8: every reviewer role sharing `_REVIEWER_BLOCKED_WRITE` is fixed by editing the shared list once. * Extract _PLAN_AGENT_BLOCKED_WRITE shared constant Mirrors patterns.py's _PLAN_AGENT_BLOCKED structure: ARCHITECT_ROLE, TASK_PLANNER_ROLE, and RISK_ANALYST_ROLE now share a single blocked_write list instead of inlining identical 9-element lists. Eliminates one future drift surface within agent_roles.py itself, as suggested in PR #2550 review. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2549: skip already-merged slices on pipeline restart (#2552) * Fix #2549: skip already-merged slices on pipeline restart When a slice's PR is merged into the work branch, the orchestrator restart loop has no signal that the slice is done — `iter_ready()` yields it on the first tick, `create_slice_integration_branch` tries to push the (now post-merge) parent SHA onto the slice's existing ref, and origin rejects it as non-fast-forward. The slice cascade- fails its descendants in ~5 seconds, blocking the entire stacked-PR workflow until an operator manually deletes the stale slice ref. The fix wires three things: * `GatewayClient.is_slice_branch_merged_into_parent` — a new detection helper. ls-remote both refs, fetch them, and check `merge-base --is-ancestor existing parent`. The inverse direction of the #2512 restart-recovery check. * Bootstrap reconciliation in `_run_implement_phase_slices`. Before the run loop starts, fold in (A) slices already marked `SliceStatus.COMPLETE` on the contract (cheap path; trust the contract) and (B) slices the gateway reports as already-merged (the live #2549 repro path). Both transitions persist `status=COMPLETE` so subsequent restarts hit (A). * Race protection in `_run_one_slice_inner`. Re-runs the merged- detection right before `create_slice_integration_branch` so a slice merged between bootstrap and its wave is also handled. Also closes a latent gap: `Slice.status` had `COMPLETE` as a value since the original schema and the #2470 `restart_agent` parent-slice- complete fallback already read it, but nothing wrote it. The successful-completion path in `_run_one_slice_inner` now persists `SliceStatus.COMPLETE` to the contract under the per-pipeline state lock, finally giving the #2470 reader a real signal. * Address #2552 review notes: defer reconciler start, parallelize bootstrap, prefer parent_branch_at_creation, expand test - Move _start_stacked_pr_reconciler call to after the bootstrap pass so an exception during bootstrap (hard imports, programming errors) cannot leak the daemon thread. - Parallelize layer-(B) is_slice_branch_merged_into_parent calls with a ThreadPoolExecutor (cap 8). Each call uses its own synthetic gateway session, so concurrent calls are safe; this keeps startup latency bounded as forests grow. - Prefer slice.parent_branch_at_creation over deriving from dependencies[0] in the bootstrap parent-branch resolution. Today both should agree, but a future re-plan that mutates dependencies post-creation would otherwise compare against the wrong parent. - Expand test_bootstrap_does_nothing_when_pipeline_repo_unset to actually exercise step (A) under repo=None: add a slice with status=COMPLETE alongside a PENDING slice, then assert that step (A) skips the COMPLETE slice and step (B) is wholesale skipped. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Egg/issue 2474 v2/work (#2556) * Initialize SDLC contract for issue #2474 * slice slice-1: Cleanup — k3s only, drop dead test tiers (#2533) * Slice 1 (coder portion): retire e2e tier scaffolding (#2474) Drops the real-LLM end-to-end test scaffolding the coder role can reach under its file boundaries (`pyproject.toml`, `Makefile`, `integration_tests/agent_findings.py`): - Remove `e2e` and `agent_flaky` pytest markers from `pyproject.toml`. The matching `tests/config/test_ci_config.py` required-markers assertion is in the tester's scope; tester picks it up alongside task-1-2 (delete `tests/functional/`) so the marker set lands consistently. - Drop the `test-e2e` Make target, its `.PHONY` entry, and its `make help` line; retag `test-integration` to k3s in the help block and module banner. `test-security` is retained. - Delete `integration_tests/agent_findings.py` — the JSONL findings recorder for the agent_flaky fuzz tier; orphan once `test_agent_security_fuzz.py` is removed by tester (task-1-3 e2e tests). - Stage `.github-staging/workflows/test-e2e.yml` as a deletion-marker: agent file-boundaries block writes under `.github/`, so the staged file's header explicitly directs the human reviewer to `git rm .github/workflows/test-e2e.yml` (and the marker itself) rather than `git mv` it into place. The PR builder's auto "Move staged `.github/` changes" step (issue #2508) surfaces the marker. Tasks split: - task-1-3 coder portion: pyproject markers, Makefile target, agent_findings.py, .github-staging marker. - task-1-3 tester portion (handed off): delete `integration_tests/test_e2e_workflow.py` and `integration_tests/test_agent_security_fuzz.py`; update `tests/config/test_ci_config.py` required-markers set. - task-1-1 / task-1-4 (handed off): conftest edits live in tester's scope (`**/conftest.py`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Drop .github-staging/ deletion-marker; rely on pre-merge condition (#2474) Address reviewer_code NACK on slice-1 v1: the staging-promote pattern in `_build_github_staging_manual_step()` (orchestrator/routes/pipelines.py:8490) unconditionally renders `git mv .github-staging/<path> .github/<path>` boilerplate for every staged file — there's no opt-out for "this marker expresses a deletion intent." A reviewer who skims past the YAML-comment header inside the staged file and follows the auto-generated `git mv` either fails loudly ("destination exists") or, with `git mv -f`, silently overwrites the live workflow with the retired stub — neither resolves into the intended `git rm`. The documented BRC pattern for "human action that agents cannot push through the gateway" is `--pre-merge-condition` on a reviewer ACK (issue #1998 / `_collect_pre_merge_obligations`), which renders as a "Pre-merge Obligations" section in the PR body with a do-not-merge banner. reviewer_contract attached such an obligation on their v1 ACK, so the merger sees the `git rm` instruction unambiguously without the contradictory staging-promote step. Behaviour change: none against the runtime pipeline. The live `.github/workflows/test-e2e.yml` deletion remains a pre-merge human obligation; only the in-tree marker file is removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Slice-1 tester scope: delete tests/functional/, k3s-only conftests, retire e2e tests Picks up everything in the tester's gateway file scope for slice-1 (issue #2474): task-1-2 (delete tests/functional/): - Remove all 5 files under tests/functional/. Acceptance criterion: `tests/functional/` no longer exists. NOTE: the matching `functional:` marker registration in pyproject.toml AND the `tests/functional/conftest.py` allowlist entry in scripts/check-hardcoded-ports.py are gateway-blocked from the tester role (only coder can push pyproject.toml / scripts/). Both have been HANDOFFed back to the coder for inclusion in their next propose; see the HANDOFF message issued alongside this commit. Leaving the marker registered is harmless (no tests carry the marker any more); leaving the allowlist entry registered is harmless (the file is gone so the lint script never visits it). task-1-1 (k3s-only egg_stack): - integration_tests/conftest.py: drop `_docker_egg_stack()`, the EGG_RUNTIME=docker branch in `egg_stack`, the `docker_available` import, and stale docker-compose comments. `egg_stack` now skips with a clear pointer to docs/guides/testing.md when kubectl is unavailable. - integration_tests/local_pipeline/conftest.py: same treatment for `local_pipeline_stack`. Drops the COMPOSE_FILE / MOCK_SANDBOX_DIR constants, `_cleanup_orphaned_containers`, the docker-compose-up block, and the `docker_available` import. task-1-4 (retire orphan agent-led helpers in integration_tests/conftest.py): - Delete `run_claude_structured()`, `assert_agent_verdict()`, the `AgentVerdict` dataclass (including `infrastructure_failure`), `VERDICT_SCHEMA`, `TEST_AGENT_SYSTEM_PROMPT`, and the orphaned `_allocate_test_container_ip()`, `_capture_container_logs()`, `_preflight_gateway_check()` helpers. Drop the now-unused imports (`json`, `requests`, `ContainerNetworkConfig`, `build_sandbox_docker_cmd`). task-1-3 (delete e2e test files; tester scope): - rm integration_tests/test_e2e_workflow.py - rm integration_tests/test_agent_security_fuzz.py - tests/config/test_ci_config.py: required-markers assertion narrowed from {integration, functional, e2e, security, agent_flaky} to {integration, security}, with a docstring reference to issue #2474. Test infrastructure preserved: - GATEWAY_PORT remains imported and re-exported from integration_tests/conftest.py because test_network_security.py imports it directly via `from integration_tests.conftest import GATEWAY_PORT, exec_in_container`. - isolated_container / external_container / test_container fixtures are retained for the test_credential_security and test_network_isolation tiers (both still in tree). They will skip in k3s mode (the docker network name does not resolve), but slice-3 of this PR train adds k3s-native equivalents that supersede them. Acceptance criteria verified for the tester portion: - `tests/functional/` no longer exists. - `grep -rn "run_claude_structured|assert_agent_verdict"` returns no hits. - `grep -nE "EGG_RUNTIME=docker|_docker_egg_stack|docker_available"` on the two conftest files returns no hits. - `make lint` passes. * Slice-1 cleanup: drop functional marker + stale port allowlist (#2474) Address tester HANDOFF 2cc2c216-4c53-45 (non-blocking, raised on coder v2 ACK). Now that `tests/functional/` and `integration_tests/docker-compose.yml` are gone (slice-1 tester commit 3827cb571), this commit cleans up the dead-weight references to those paths that the tester role is gateway-blocked from reaching: - `pyproject.toml`: drop the `functional:` marker registration. The marker was the last live reference to the deleted `tests/functional/` tier; `tests/config/test_ci_config.py` was narrowed by tester to `required = {integration, security}` so the required-markers test still passes (subset check) — but the marker registration itself was orphan after the tier deletion. - `scripts/check-hardcoded-ports.py`: remove two stale `ALLOWLIST_PATHS` entries pointing at files that no longer exist: `integration_tests/docker-compose.yml` and `tests/functional/conftest.py`. The latter matters for task-1-2's acceptance criterion `grep -rn "tests.functional|@pytest.mark.functional"` returns no hits — the regex `tests.functional` matches the literal string `tests/functional/conftest.py` (`.` matches `/`), so the allowlist entry was a real gap, not just polish. `make lint` is clean; `tests/config/` test suite still passes with the trimmed marker set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * rm e2e workflow * Address review: drop more dead code from k3s-only cleanup Follow-ups on PR #2533 review (https://github.com/jwbron/egg/pull/2533#pullrequestreview-4241318207): - Delete now-orphan integration_tests/local_pipeline/mock-sandbox/ (Dockerfile + phase-runner.sh) — only consumer was the deleted docker fallback in local_pipeline/conftest.py. - Remove tests.utils.gateway_client.docker_available() and its re-export — zero remaining callers after this PR removed the conftest call sites. - Strip stale -m "not functional" from Makefile (test, test-all) and update docs/guides/testing.md §2 step 8. - Rewrite integration_tests/conftest.py docstring to describe what the legacy fixtures actually do under k3s. Add explicit pytest.skip in isolated_container/external_container/test_container when the stack is k8s-backed (was silently skipping with a generic-sounding "could not start container" message). - Drop unused certs_volume field from EggStack; document why compose_project / external_network are retained. - Expand __all__ in integration_tests/conftest.py to cover the re-exported public surface (EggStack, ContainerInfo, exec_in_container, GATEWAY_PORT) — previously listed GATEWAY_PORT only. - Drop STRUCTURE.md mock-sandbox entry. Skipping the "except FileNotFoundError, subprocess.TimeoutExpired:" nit — ruff format 0.15.12 actively strips parens from except tuples, so the parenthesized form would not survive `make lint-fix`. Part of pipeline issue-2474-v2; the terminal slice carries the program-level narrative. * Address review: remove stale entries from STRUCTURE.md Drop the four file entries from the integration_tests/ tree listing that this PR (slice-1 cleanup) deletes: docker-compose.yml, agent_findings.py, test_agent_security_fuzz.py, test_e2e_workflow.py. Reviewer noted the local_pipeline/ subsection was already updated in a423d311 but the parent listing was missed. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2495: discriminate authorization vs. value errors at /mutate boundary (#2517) * Fix #2495: discriminate authorization vs. value errors at /mutate boundary The `/mutate` route was returning 403 for every `MutationResult.success=False`, which is correct for role-authorization rejections but misleading for value/path errors (bad `field_path`, out-of-range index, out-of-domain enum value). A client receiving 403 for `Invalid value for current_phase: …` would reasonably retry with a different role, which can't help. Adds `error_kind: Literal["authorization", "value"] | None` to `MutationResult` so the route can map cleanly without parsing message strings: 403 for authorization, 400 for value errors. Adds regression tests for all three branches. * Address review: assert error_kind in validator tests + export MutationErrorKind Closes the validator-level test gap flagged in the PR review: - test_apply_invalid_mutation_rejected now asserts error_kind == "authorization" so a regression that drops the discriminator on the role-rejection path fails at the unit-test boundary, not just the route boundary. - test_invalid_enum_value_returns_failed_mutation now asserts error_kind == "value" for the pydantic ValidationError path. - New test_invalid_path_returns_failed_mutation covers the (KeyError, IndexError, AttributeError) path through _set_value and asserts error_kind == "value". Re-exports MutationErrorKind from shared/egg_contracts/__init__.py so callers can type-hint against the discriminator without reaching into the validator submodule. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document .github-staging/ convention in agent-roles reference [doc-updater] (#2516) * docs: document .github-staging/ convention in agent-roles reference * docs: correct tester guidance — HANDOFF instead of .github-staging/ The tester's allowed_patterns in shared/egg_restrictions/patterns.py covers only test files, conftest, pin files, and .egg-state/agent-outputs/ — it does not include .yml/.yaml/.json. AgentFilePattern.can_write requires both a non-blocked path AND a positive allowlist hit, so .github-staging/workflows/ci.yml returns False for the tester even though .github-staging/ is not on the tester's blocked list. A tester following the previous text would attempt to stage CI fixes under .github-staging/ and be rejected. Replace that advice with the correct path: hand off to the coder via HANDOFF, mirroring the existing coder→tester handoff pattern. Surfaced by egg-reviewer on PR #2516. The patterns.py / agent_roles.py divergence the same review noted is tracked separately in #2521. * docs: expand tester Directed Coordination to cover outbound HANDOFF --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py (#2525) * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py #2514 added `.github/` to `TESTER_ROLE.blocked_write` in `agent_roles.py` but skipped the mirror entry in `TESTER_PATTERNS.blocked_patterns` in `patterns.py` — the planner prompt and the gateway saw different views of the tester's write scope. The omission was benign because the tester's allowlist already excludes `.github/`, but it stops being benign the moment someone widens that allowlist. Add `.github/` to `TESTER_PATTERNS.blocked_patterns` so both files agree, matching the lockstep pattern already used for documenter, autofixer, and conflict_resolver. Add regression tests in the gateway pattern suite and the shared restrictions unit suite. * Address review on #2525: load-bearing tests, slim comment - Use `.github/test_actions.py` as the load-bearing assertion in both test files. It matches the tester's `**/test_*.py` allowlist, so only the new `.github/` blocked entry stops it. The two pre-existing paths (`.github/CODEOWNERS`, `.github/PULL_REQUEST_TEMPLATE.md`) stay as breadth assertions; both are blocked even without the new entry, so they would have passed against the unfixed patterns. - Slim the rationale block in `patterns.py:224-234` to a one-liner pointing at `CODER_PATTERNS` for the full `.github/` rationale. The original 11-line comment over-claimed lockstep across roles the diff didn't actually touch (architect, task_planner, refiner, reviewer roles); the one-liner doesn't. Drift in the non-tester roles (architect/task_planner/risk_analyst/ refiner/reviewer/reviewer_contract) is tracked in #2532. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2490: extend validate_assignment to sibling Contract models (#2520) * Fix #2490: extend validate_assignment to sibling Contract models #2484 added `model_config = ConfigDict(validate_assignment=True)` to `Contract` so `setattr` on Contract fields coerces values back to their declared type. The reviewer flagged a remaining asymmetry: sibling models (`Task`, `Slice`, `Decision`, `AgentExecutionModel`, …) still silently accepted untyped assignments like `task.status = "garbage"`, so the validation surface was uneven across the contract object graph. Lift the config to a shared `EggContractBaseModel` (Option B from the issue) and have every model in `shared/egg_contracts/models.py` inherit from it, so the strictness applies uniformly without per-model duplication. Drop the per-model config from `Contract` itself — the shared base now provides it. The audit of sibling-model mutation sites (`shared/egg_contracts/orchestration.py` `set_execution`, `orchestrator/routes/decisions.py` `contract.pr =`, etc.) confirms they assign well-typed values (enum members or constructed model instances), so the new strictness does not break existing call sites. * Address PR #2520 feedback: fix nested-model test, refresh validator comment - Rewrite test_pr_metadata_invalid_deferred_actions_raises to assign a raw dict, which actually exercises pydantic's list-element coercion path on the outer setattr; the previous form raised from the inner DeferredAction(...) constructor regardless of validate_assignment (item 1). - Update the validator.py except ValidationError comment to reference EggContractBaseModel (where the config now lives) and add #2490 to the issue list, since the same catch now covers sibling-model setattrs (Task.status, Slice.status, Decision.type, ...) too (item 2). — Authored by egg --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2501: don't flip stale during an in-flight state-store probe (#2519) * Fix #2501: extend probe freshness while a probe is in flight `StateStoreProbe.snapshot()` flipped the cached `healthy` to `False` purely because the cache age crossed `interval * stale_multiplier`, even when a probe was actively running and about to refresh it. Under slice-spawn load `git worktree add` occasionally ran 30-40s, longer than the 30s default staleness window, so the request-path dual-write in `routes/health.py` recorded `unhealthy` and the BG callback recorded `healthy` 0-3s later when the same probe completed — producing the spurious `recent_transitions` flap pairs reported in the issue. Track probe start time and, while a probe is in flight, treat the cache as fresh until the in-flight probe itself has been running longer than the staleness window. A genuinely wedged probe still surfaces as stale once that bound is exceeded. * Address review feedback on #2501 in-flight grace fix - Document worst-case ~2*stale_window wedge-detection bound and the intentional 'fresh-but-old' semantics during the grace in snapshot()'s docstring (reviewer minor: source-recoverable rationale). - Expand the inline comment in the grace branch to cite the 'fix #1' framing from #2501 so the bound's rationale is recoverable from the source alone (reviewer minor). - Add an integration-level test that drives snapshot() while a real probe_now() is parked mid-probe on a worker thread, populating the in-flight flag and _probe_started_at_monotonic via the production code path. Closes the loop end-to-end so a future refactor that stops setting _probe_started_at_monotonic from probe_now() fails this test where the field-poking variants would silently keep passing (reviewer non-blocking suggestion). * Tighten worst-case wedge detection bound in snapshot() docstring Reviewer noted that the '~2 * stale_window' / '60s with 30s default' framing is loose. The BG loop fires every `interval` seconds, so the in-flight probe starts within `interval` of the last good completion, and the grace extends only until that probe's own age exceeds `stale_window`. The tight bound is therefore `stale_window + interval`, which under the defaults (interval=15s, stale_multiplier=2.0) is ~45s of blindness, not ~60s. The 2 * stale_window framing only saturates when stale_multiplier=1.0. Address-only docstring change; no behavior change. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty (#2518) * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty restart_phase reads its respawn roster from phase_exec.agents — a runtime cache that the route's own clear-then-spawn flow resets to []. If the spawn step fails before re-populating the cache, every subsequent restart_phase 400s on "No agents found in phase {phase} to restart" and start_pipeline 409s on the (now CANCELLED) status, leaving the only escape cancel_task(cleanup=true) — which discards all prior work. Fall back to the same deterministic source the executor itself uses: pipeline.active_roles (CUSTOM-mode / BABYSIT overrides, #1762) first, then get_roles_for_phase(repo, has_contract). Same precedence as _run_concurrent_phase, so the recovered roster matches what the next spawn would have produced anyway. * Match _run_concurrent_phase exactly: skip phase-default fallback when active_roles set When pipeline.active_roles is set but every entry is unknown to this orchestrator's AgentRole (defensive case after a role removal in a newer schema), the prior implementation fell through to get_roles_for_phase and expanded to the full phase-default roster. _run_concurrent_phase keeps its roles list empty in the same case, so the route's response (and the downstream worktree-delete / health-monitor reset) would diverge from what the spawn would actually produce. Convert the second 'if not agent_roles:' into an 'else:' attached to the override branch so the strict-parity behaviour matches: when an override is set we use it verbatim and never fall through. The final 'No agents found' 400 still fires honestly when the override is all-unknown. Adds a regression test that mutates active_roles post-construct (bypassing the field validator) to simulate the load-time-drift edge case. * Document deliberate route-vs-worker divergence in roster-derivation try/except The except Exception wrap around _get_roles_for_phase doesn't exist in _run_concurrent_phase, so a future reader auditing the two callsites for parity might mistake the bare-except for a bug rather than a deliberate route-specific safety floor (return 400 not 500). * Tighten line-range citation in roster-derivation comment to 12813-12840 --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2522: enumerate per-agent worktrees on phase restart (#2526) * Fix #2522: enumerate per-agent worktrees on phase restart restart_phase guessed worktree names as ``{pipeline_id}-{role}``, which misses slice-scoped worktrees (``{pipeline_id}-slice-{N}-{role}``) and leaves them on disk after a restart on a slice-based pipeline. Drive deletion off ``agent_salvage.enumerate_agent_worktrees`` (already the source of truth in ``cleanup_pipeline`` and salvage) and filter to the roles being restarted. The pipeline-level worktree (``agent_role=None``) and worktrees for non-restarted roles are intentionally preserved. * Address review: salvage before restart-phase delete; cleanup-style enumeration Blocking review feedback (#2522 / PR #2526): 1. Silent loss of unpushed agent commits during phase restart restart_phase now calls agent_salvage.auto_salvage_pipeline before the deletion loop (mirroring cleanup_pipeline's #2429 invariant). Restart is precisely the scenario where unpushed commits accumulate - operators hit it because agents got stuck or wedged - so the previous code was the one orchestrator-side worktree-delete path that bypassed salvage. Salvage failures are best-effort; deletion still happens. 2. Broken/corrupted worktrees regressed the original #1723 cleanup enumerate_agent_worktrees gates on a usable .git marker, so wedged-btrfs-mount worktrees were being silently skipped after this PR's switch to enumeration. Added validate_git=False flag (default stays True for salvage callers) so cleanup callers receive broken entries with repo_path falling back to the worktree dir itself. restart_phase now opts in to the cleanup-style listing. Non-blocking feedback addressed in the same commit: - Test fixture _make_pipeline_with_slice_agents builds AgentExecution with slice_id populated, matching what concurrent_executor writes. - New test exercises continue-on-error across three worktrees with the middle one's delete raising; locks down loop semantics. - Narrower exception class (OSError | ImportError | RuntimeError) around enumerate_agent_worktrees. - log_extras suppresses slice_id=None on non-slice pipelines. New tests: - test_restart_phase_continues_after_partial_worktree_deletion_failure - test_restart_phase_salvages_before_deleting_worktrees - test_restart_phase_salvage_failure_is_nonfatal - test_restart_phase_deletes_broken_worktree_without_git_marker - test_validate_git_false_returns_broken_worktrees - test_validate_git_false_preserves_validated_repo_path --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2539: drop duplicate `slice ` prefix in non-terminal slice PR titles (#2540) `create_slice_pr` was rendering non-terminal slice PR titles as `slice slice-1: …` because `slice_id` already starts with `slice-`. Drop the literal prefix so the title is just `{slice_id}: {slice_name}` (e.g. `slice-1: Cleanup — k3s only, drop dead test tiers`), and update the two test assertions and docs reference that pinned the buggy form. * Fix #2531: add `--for STATUS` to producer pre-confirm wait-loop (#2536) When every reviewer ACKed the current version, no further `CONSENSUS_ACK` / `CONSENSUS_NACK` events arrive on the bus. The orchestrator's directed `STATUS` nudge ("Ready to confirm — all confirm preconditions satisfied", `metadata.ready_to_confirm == True`) is the only signal that the global preconditions cleared, but the producer prompt's pre-confirm wait-loop filter omitted `STATUS` — so the producer slept through the nudge and only woke via the health-monitor `OVERSEER_ALERT` backstop minutes later, observed in pipeline `issue-2474-v2` slice-1 (6/8 stall, ~57 min phase elapsed). The reference doc at `agent-wait-patterns.md` already prescribed waiting on `STATUS` for the pending-acks recovery path; the prompt template just hadn't caught up. This change closes that gap and adds a regression test that pins `--for STATUS` plus the on-wake guidance ("go to step 5 CONFIRM if `metadata.ready_to_confirm`, otherwise re-enter the wait") across every producer role × phase. The `_send_brc_confirmation_nudge` docstring is updated to reflect the new pre-confirm filter. * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import (#2542) * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import Two bugs surfaced when issue-2474-v2 spawned slice-2: every container exited four seconds in with no work attempted, leaving slice-2's integration branch empty and the PR-create call to fail with "No commits between ...". Bug A (`gateway/git_client module unavailable`): the deployed orchestrator image ships only `orchestrator/`, `routes/`, `health_checks/`, and the shared `egg_*` packages — `gateway/` is not copied. The `from gateway.git_client import build_rebase_onto_args` call added by #2512 always raises ImportError in production, so every slice integration branch reconciliation silently fails. Inline the canonical argv builder as `_build_rebase_onto_args` in `orchestrator/gateway_client.py`; the gateway server's `/git` endpoint remains the authoritative allowlist boundary, and CI test paths that keep `gateway/` on `sys.path` continue to work unchanged. Bug B (slice-2 consensus reached at elapsed_seconds=0.0): the per-slice tracker registry already keys by `{pipeline_id}/{slice_id}`, but `ConcurrentPhaseExecutor.check_consensus()` had two slice-unaware fallback paths. When slice-2's tracker is fresh and empty (the steady state right after spawn, before any agent has proposed), (1) `reconstruct_tracker_from_messages` was called with the bare pipeline_id and (2) the message-bus fallback scanned `store.get_messages(pipeline_id)` pipeline-wide. Slice-1's eight CONSENSUS_CONFIRMED messages are persisted under the bare pipeline_id and have the same role names as slice-2's roster, so both paths falsely declared consensus on slice-2's first poll iteration. Gate both fallbacks (and the matching path in `handle_consensus_confirmed_signal`) on `slice_id is None`. The in-memory per-slice tracker is the authoritative source; an empty fresh tracker correctly returns is_complete=False and the polling loop keeps going. * Address #2542 review: slice-scope idempotency, fix test syntax, doc tweaks Five issues from egg-reviewer on the #2535 PR: 1. test_check_consensus_slice_isolation.py: replace dead try/except that used Python-2 catch-and-bind syntax (`except A, B:`) with a direct `PipelineConfig(concurrent_execution=True)` constructor call. The original block was unreachable — `concurrent_execution` is a normal Pydantic bool field that cannot raise on assignment — and the misleading syntax would surprise any future reader. 2. routes/signals.py: scope `_existing_confirmed_for_role` to a slice so the idempotency probe doesn't see sibling-slice CONFIRMs as "already confirmed for this role". A new `slice_id` parameter filters by `metadata["slice_id"]`; the per-slice tracker path tags CONSENSUS_CONFIRMED writes with that same metadata key. Without this, slice-2's first coder CONFIRMED would be silently suppressed (no bus message, no #1473 marker) because slice-1's coder CONFIRMED was still in the bus under the bare pipeline_id. Pipeline-scoped (slice_id is None) callers continue to see only pipeline-scoped messages, preserving legacy behaviour exactly. 3. orchestrator/gateway_client.py: soften the "Mirrors" claim in the `_build_rebase_onto_args` docstring. The helper does NOT call validate_git_args (which would defeat the inlining) and emits stripped argv, so document those two intentional differences. 4. orchestrator/stacked_pr_reconciler.py: update the module docstring to point at the inlined `_build_rebase_onto_args` in orchestrator.gateway_client (with a note explaining why the inlining is needed and why the security floor is unchanged). 5. tests/test_consensus_confirmed_idempotent.py: extend the helper `_fake_message` with a `slice_id` parameter and add three regression tests: - slice-2's first CONFIRMED is NOT marked idempotent by a slice-1 CONFIRMED in the bus - within slice-2, the second CONFIRMED IS deduped - pipeline-scoped callers ignore slice-scoped CONFIRMs The wider sweep of slice-unaware peer-consensus lookups in kubernetes_monitor.py, startup_reconciliation.py, routes/pipelines.py status display, and the tier-1 health checks is left for #2409 (the existing tracker covers the same root-cause: slice_id needs to flow through more places). PR body updated to flag this. * Fix #2538: slice PRs carry contract.pr narrative on every slice (#2543) Every slice PR — terminal and non-terminal — now renders the planner-authored program title, description, test plan, and manual steps from contract.pr, so reviewers see program rationale on whichever slice they open first. Previously only the terminal slice carried the narrative; reviewers approaching slice-1 (the bottom of the stack and the canonical merge entry point) saw only task bullets plus a pointer to the terminal slice's PR. Title disambiguation: terminal slice gets the bare program_title; non-terminals get a [<slice-id>] prefix so the GitHub PR list stays scannable when several stacked PRs are open at once. Per-merge obligations remain terminal-only (the merge gate is the last-to-merge PR in the stack) — the existing #2354 invariants and fail-fast assertion are preserved. The terminal slice keeps a "merge gate / umbrella" banner so reviewers can spot the merge gate; non-terminals skip it. The old "see terminal slice's PR for the program-level narrative" pointer is gone — the narrative is right there now. * Fix #2537: attribute slice PRs to orchestrator, not coder (#2541) * Fix #2537: attribute slice PRs to orchestrator, not coder The slice-PR creation path is orchestrator-only — `gh pr create*` is blocked for the implement phase, and the pr phase has no agent spawn. But `_run_implement_phase_slices` was hard-coding `agent_role="coder"` on the synthetic session that opens the slice PR, which caused the gateway to label the PR `agent:coder` and inject `agent_role=coder` into the `` comment. Pass `agent_role="orchestrator"` so slice PRs match the attribution the non-sliced `_auto_create_pr` path already uses. * Fix /status/wait test flake: handshake before publish The three event-bus tests in TestStatusWaitRoute used a 0.1s sleep in the fire thread before publishing — racy on slow CI. The route's preamble (cursor parse, terminal short-circuit, staleness probe, current_sequence() snap) can exceed the grace window, so the publish lands before event_bus.subscribe(None, _on_event) and the event is never delivered. Replace the sleep with a deterministic handshake that polls event_bus._wildcard_handlers and returns the moment the route has subscribed. The message-bus path (test_overseer_alert_wakes_route) uses a different wake mechanism and is left untouched. * docs: add --for STATUS to producer pre-confirm wait-loop example (#2546) Syncs docs/guides/concurrent-execution.md with the fix from #2531: the producer RESPOND TO REVIEWS (step 4) wait-loop now includes --for STATUS so the orchestrator's "Ready to confirm" directed nudge wakes the producer when every reviewer has already ACKed and no further CONSENSUS_ACK/CONSENSUS_NACK will arrive. docs/reference/agent-wait-patterns.md was already updated in the same PR; this doc had a stale copy of the canonical snippet. Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2527: validate task role↔file alignment at plan time (#2551) * Fix #2527: validate task role↔file alignment at plan time Adds `validate_task_role_alignment` in `shared/egg_contracts/plan_parser.py` that mirrors the gateway's push-time blocked-pattern check for each task's `role` against its `files_affected`. The plan reviewer's prompt now runs the validator on the parsed plan draft and injects a "Structural Role-Alignment Check" section listing every offending task with the eligible-role hint (or the `.github-staging/` remediation when no producer role can push the file set). The plan-review criteria gain a deterministic blocking item that points at this section, so a mis-assignment surfaces as a NACK before any producer cycle is wasted. Per-task logic lives in `_check_role_files` so the #2530 follow-up (`includes_tests: true` opt-in for coders coupling tests with their own production code) has a clear hook point. Section is omitted when the validator reports no violations; the prompt is unchanged for clean plans. Push-time enforcement remains in place as defense in depth. * Move #2527 validator to orchestrator-side propose-time enforcement PR-1 review flagged a cross-module silent no-op: in concurrent BRC mode (the default for plan phase) the original prompt-time helper ``_build_role_alignment_check_section`` always returned ``""`` because ``_run_concurrent_phase`` builds every reviewer prompt up-front before the planner has produced the plan. The criteria text then told the reviewer that "absence of that section means the automated check found no violations" — the opposite of the truth. Replace it with deterministic enforcement at the right seam: ``_validate_planner_role_alignment`` runs in ``handle_consensus_propose_signal`` for ``agent_role=="task_planner"``, mirroring the existing ``_validate_tester_check_coverage`` pattern. It reads the plan content as committed at the proposed SHA via ``git show <commit>:<plan_path>`` (so a stale local checkout can't mask a real misassignment) and raises ``ValueError`` on violations, which the caller turns into HTTP 400 — the proposal is rejected BEFORE the tracker is mutated and BEFORE any reviewer sees it. Also addresses the non-blocking comments: * Lazy ``posixpath`` / ``match_pattern`` / ``AGENT_PATTERNS`` imports in ``_is_file_blocked_for_role`` are moved to module scope (no circular-import risk; per-call overhead removed). * Tests now exercise the production sequence end-to-end: ``test_rejected_proposal_does_not_mutate_tracker`` builds the exact propose signal a planner emits in concurrent BRC mode, mocks ``git show`` to return a misassigned plan, and asserts the tracker is left untouched. The PR-1 prompt-emission tests are removed (the helper they pinned is gone) and replaced with criteria-text regression guards that lock out the "absence = no violations" wording. * Address PR review feedback (round 2) Blocking: - Revert egg_restrictions.patterns import in plan_parser.py to lazy function-local. The module-scope hoist in PR-1 round 1 re-introduced the egg_restrictions ↔ egg_contracts import cycle that shared/egg_restrictions/matchers.py was deliberately split out to avoid (see its docstring), breaking the gateway production boot path (python3 gateway/gateway.py). Conftest pre-load order hid the cycle in pytest. egg_restrictions.matchers.match_pattern stays at module scope — only AGENT_PATTERNS needs to be lazy. - Add TestImportOrderingRegression that subprocess-runs 'import egg_restrictions.patterns' under PYTHONPATH=shared so the cycle surfaces in a clean interpreter (mirrors gateway boot). Non-blocking: - Reword the role-alignment criterion in _get_plan_review_criteria to say 'before the proposal reaches you' instead of 'before this prompt is ever rendered' — concurrent BRC mode builds reviewer prompts up-front, so prompt-render time isn't the right reference point. - Update stale comment in test_pipeline_prompts.py that pointed at test_signals.py::test_propose_validates_planner_role_alignment (no such file/test) — the validator-runs-here tests live in this same file under TestPlannerRoleAlignmentValidation. - Thread already-loaded pipeline_state and worktree_path from handle_consensus_propose_signal into _validate_planner_role_alignment via keyword args (with backward-compat fallback to in-function loads) so the validator's dependency on the prior _verify_commit_on_branch block is explicit and the state-store + worktree lookups aren't duplicated. * Fix stale gateway boot path comment in import-ordering regression test The PYTHONPATH=shared mirror comment cited scripts/start-gateway.sh which doesn't exist. Replace with the actual production references: gateway/Dockerfile:99 (PYTHONPATH=/app), gateway/entrypoint.sh:286 (exec python3 gateway.py), gateway/Dockerfile:70-75 (shared/ COPY). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document plan-time role↔file alignment validation (#2558) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg <egg@localhost> * Fix #2554: render per-role agent-status table on BRC dashboard emits (#2557) * Render per-role agent-status table on BRC dashboard emits The Phase 3 monitor previously rendered a 3-line `Pipeline Status` block plus a stacked 4-line `Enhanced dashboard` consensus paragraph on each `wait-status` emit. With 8+ agents active during a busy implement-phase BRC, the prose form blurs — a stalled reviewer is not visually distinct from a working one, and a NACK row drops to the end of the paragraph. Replace the two stacked blocks with one per-role markdown table when `concurrent.consensus` is present. Columns derive directly from the `peer_consensus.evaluate()` envelope (`agents[role].producer_phase` / `reviewer_phase` / `confirmed`, plus structured `unresolved_nacks`), so schema drift surfaces as an empty cell rather than a wrong cell. Dual-role agents (`tester`) render `<producer_phase> / <reviewer_phase>`. Always render full state on BRC emits — the table is an at-a-glance scan, so deltas-only would defeat its purpose. Non-BRC lines keep the existing compact 3-line form with deltas-only behavior. Mirror the same two-path render in Phase S5 (lightweight pipeline) and emit the table one final time in Phase 5 success summary so the operator has a closing snapshot of which roles confirmed. Fix #2554 * Address review: drop nonexistent Slice column; fix S5/S6 mirrors; generalize producer ordering - Drop Slice column entirely. last_status.pipeline.current_slice_id does not exist on the Pipeline model — slice_id lives on AgentExecution (per-agent), not on the pipeline root, and the minimal envelope does not carry it. Rendering it would have produced 'Slice: —' on every emit and silently misled operators about sliced vs non-sliced state. - Phase S5 (lightweight): replace stale 'concise/deltas-only' line with the same dual-path phrasing as Phase 3 line 411, and drop Slice from the Path B header reference. - Phase S6 (lightweight Complete): add the closing-snapshot bullet so BRC consensus is rendered one final time on success — lightweight pipelines start at implement, so this is the common case. - Generalize producer ordering: pull producers/reviewers from concurrent.consensus.review_graph (sorted alphabetically by ReviewGraph.to_dict) instead of the implement-only hardcoded list, so refine/plan producers (refiner, architect, task_planner, risk_analyst) order correctly without further prose drift. - Consensus fallback: specify the Phase column renders '—' when concurrent.consensus is missing, and explicitly forbid inventing a message-type-to-phase mapping (a CONSENSUS_PROPOSE tells you the producer is in PROPOSED but says nothing about reviewer phases). * Address re-review: dedup dual-role agents, fix example ordering The producers/reviewers split sourced from review_graph emits tester in both lists for the implement graph, so an LLM following the rule literally would render tester twice. Add an explicit dedup directive to the Role column rule. Reorder the example table to match the alphabetical ordering rule (review_graph.producers is sorted). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@localhost> * Fix #2529: runtime escape hatch for impossible tasks (#2553) * Fix #2529: runtime escape hatch for impossible tasks Adds two MCP tools and a typed `Impasse` primitive so a producer that discovers mid-execution that its task is structurally impossible can emit a structured signal instead of inventing workarounds. The orchestrator detects the impasse post-phase and either auto-delegates to a suggested role (first attempt, `wrong_role` only) or escalates to HITL (second attempt or non-`wrong_role`). - `mcp__sdlc__check_file_restriction` — pure-local read against `shared/egg_restrictions/patterns.py`. Returns `can_write` plus `alternative_role` when exactly one producer covers the path. Lets the agent self-check before burning tokens on exploration. - `mcp__sdlc__report_impasse` — persists a typed `egg_contracts.Impasse` (category, reason, suggested_role, blocked_files, evidence) under `AgentOutput.impasse`. Once called, the agent must exit cleanly without committing. - `orchestrator/impasse_routing.py` — `collect_impasses` + `route_impasses`. Auto-delegate fires only for fresh tasks (`delegation_attempts == 0`) with a single eligible alternative producer role; everything else creates a HITL decision via `apply_mutation` with a delegate / cancel / manual-resolve / other option set. - `_run_concurrent_phase_with_impasse_retry` wraps the existing slice-loop spawn so an `all_delegated` outcome triggers one BRC retry against the mutated contract; any escalation surfaces to the operator without auto-retry. - Producer prompt picks up an "If a task is impossible, use these tools instead of inventing workarounds" section. Test surface: schema round-trip, both handlers (allowed/blocked/ batch/error paths), routing helper (delegate, second-impasse HITL, plan_bug / external_blocker / unresolved-task escalations, self-delegation defense), and the existing tool-registry + CLI-drift gates updated for the two new no-CLI verbs (rationale in handler docstrings per decision-13). * Address PR #2553 review: producer escape hatch + routing hardening Blocking fix: - Move runtime escape-hatch instructions out of the task_planner-only _build_role_restrictions_section into a new _build_impasse_escape_hatch_section, then inject it into the coder prompt (early-return branch) and the tester / documenter prompts (post-phase-restrictions branch). Producers — the only roles that emit impasses — now actually see check_file_restriction / report_impasse guidance instead of inventing workarounds. The planner keeps a brief post-failure-delegation summary so it knows the auto-delegation path exists; planners do not emit impasses. - Add end-to-end TestProducerEscapeHatchInPrompts coverage that parametrises over coder/tester/documenter and asserts both tool names plus the "DO NOT invent workarounds" header appear, and that architect / planner stay free of the actionable producer-only directive. Non-blocking fixes: - Routing: route_impasses gains a force_escalate kw. The slice-loop wrapper sets it on its terminal iteration so a delegation that cannot re-run a BRC cycle gets escalated to HITL rather than silently mutating the contract and exiting on a stale role assignment. - Routing: drop the 120-char truncation of impasse.reason in _record_delegate's audit-log entry. The schema caps reason at 2000 chars and the audit log can hold the full payload — preserve it for post-mortem debugging. - Slice loop: clear the impasse field from each producer's per- pipeline agent-output file between iterations. save_agent_output's mode="w" already overwrites when a producer respawns and reaches its handoff write, but a producer that crashes pre-handoff in iter-N+1 would otherwise let iter-N's impasse persist and re-trigger routing as a spurious "second impasse on same task" HITL. - Handler: reject category="wrong_role" without suggested_role at the mcp__sdlc__report_impasse boundary. Without it the orchestrator router can only escalate, which silently degrades the producer's deliberately set wrong_role signal — point the agent back at check_file_restriction so the fix lands in the same iteration. - Handler: also require task_id for category="wrong_role". The router's role-match fallback is fragile when a slice has multiple tasks per role or role-less tasks; explicit task_id eliminates guesswork on the auto-delegation path. Other categories keep task_id optional. - Pipelines: comment the monolithic-implement fallback (the second _run_concurrent_phase call in the implement handler) explaining that auto-delegation is intentionally scoped to the slice loop since it rewires a task within a slice. Closes review feedback items 1-7 on PR #2553. * Address PR #2553 re-review: docs drift + cleanup test Address two of the three non-blocking suggestions from the approve re-review on commit 696d392. * docs/reference/agent-tools.md: mcp__sdlc__report_impasse row now documents that task_id and suggested_role are mandatory for category=wrong_role (handler raises HandlerError when either is missing). Other categories keep both fields optional since they always escalate to HITL. * orchestrator/routes/pipelines.py: extract the per-pipeline agent-output cleanup closure to a module-level helper named _clear_stale_impasses_for_producers so it can be unit tested directly. Behaviour is identical — the helper drops the impasse field after every successful all-DELEGATE iteration. * orchestrator/tests/test_pipeline_impasse_cleanup.py: new file with five focused tests covering the cleanup happy path, the no-impasse no-op path, the missing-output-file path, multi-producer cleanup in one pass, and per-pipeline scoping. The third suggestion (mixed-decision iter-0 still wastes a DELEGATE role flip) was explicitly flagged "Worth a follow-up issue, not blocking here" by the reviewer — filed as #2563. * Address PR #2553 minor observations: type annotation + hoist imports Two non-blocking observations from the third review (commit 7da68cf): - Annotate `producer_roles` on `_clear_stale_impasses_for_producers` as `list[ContractAgentRole]` so a future caller sees the expected element type without grepping the call site. Quoted under TYPE_CHECKING + `# noqa: UP037` to match the file's existing pattern for `ContainerSpawner` (lines 413, 684, 6316, 6707, 7021). - Hoist `load_agent_output` / `save_agent_output` imports from inside the helper to module level, mirroring `impasse_routing.py:50` which already imports `load_agent_output` directly. The seam fallback is unused at runtime (egg_contracts is the actual installed package, not shared.egg_contracts) and impasse_routing.py confirms a plain module-level import works. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * [slice-1] Add context PR + per-slice BRC history (closes #2548) (#2555) * Add PRMetadata.context_* fields + planner prompt updates (#2548) slice-1 / task-1-1 + task-1-3 — the foundation slice for the context-PR mechanism. Subsequent slices build the gateway primitive, the orchestrator hook, and the slice-1 base rewiring on top of these fields. Schema 1.1 — extends ``PRMetadata`` with four optional fields: - ``context_title`` / ``context_description`` — planner-emitted framing for the dedicated context PR (e.g. "Strategic plan for #N" vs the slice's "Implement …"). Both fall back to ``title`` / ``description`` when omitted. - ``context_branch`` / ``context_pr_number`` — orchestrator-populated runtime values (the ``egg/<id>/context`` branch name and the GitHub PR number once the context PR has been opened). Planners must NOT emit these. Bumps ``Contract.schemaVersion`` default from ``"1.0"`` to ``"1.1"`` and adds an ``after``-mode migration shim that promotes pre-1.1 contracts to 1.1 on load. The bump is purely additive — pre-1.1 JSON loads cleanly with the new fields defaulting to ``None``. Plan-parser plumbing — ``ParseResult`` grows ``pr_context_title`` / ``pr_context_description`` and a new ``extract_pr_context_metadata_from_yaml`` helper extracts the optional keys without breaking the existing ``extract_pr_metadata_from_yaml`` 5-tuple signature (and the ~10 callers + tests that unpack it). Planner prompt — both planner-prompt sites in ``pipelines.py`` (the plan-phase prompt under ``_build_phase_prompt`` and the task_planner-role prompt under ``_build_agent_prompt``) gain the ``_PR_CONTEXT_GUIDANCE`` paragraph and the ``_PR_CONTEXT_YAML_EXAMPLE_LINES`` commented-out hints inside the ``pr:`` YAML block. Both helpers are defined once next to ``_PR_DESCRIPTION_GUIDANCE`` so the two prompt sites stay in sync when the guidance evolves. Contract populator — ``_populate_contract_from_plan`` now copies ``result.pr_context_title`` / ``pr_context_description`` onto the new PRMetadata it builds, and preserves any orchestrator-populated ``context_branch`` / ``context_pr_number`` across re-populates so a later plan re-parse does not blow away runtime state set by slice-3's hook. Test impact: bumping the default ``schemaVersion`` to ``"1.1"`` causes ``tests/shared/egg_contracts/test_models.py::test_minimal_contract`` to fail on the literal ``"1.0"`` assertion. The fix-up belongs to the tester role (task-1-2) along with the new ``PRMetadata.context_*`` round-trip coverage; coder boundaries forbid pushing test edits. Lint (ruff format + check) and mypy delta are clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Add PRMetadata.context_* test coverage + 1.0→1.1 migration tests (#2548) slice-1 / task-1-2 — adversarial + regression coverage for the four new optional ``PRMetadata.context_*`` fields and the ``schemaVersion`` 1.0→1.1 promotion shim added by the coder in commit 75d8ca09c. Coverage: * ``TestPRMetadataContextFields`` — defaults to None, full round-trip with all four fields populated, omitted-keys round-trip preserves None. * ``TestPRMetadataContextPRNumberValidator`` — pins the ``ge=1`` validator: 0/-1 are rejected at construct AND at setattr (under the shared ``EggContractBaseModel.validate_assignment=True`` from #2490); None and large positive ints accepted. * ``TestPRMetadataSchemaVersionMigration`` — 1.0 payload loads with context defaults, dump→reload chain stays at 1.1, default is 1.1, legacy ``deferred_actions`` survive migration, and an unrecognized version (1.2 / 2.0) is NOT silently downgraded. * ``TestPRMetadataContextEmptyStringSemantics`` — empty strings are accepted at the model layer so the orchestrator hook's ``context_title or title`` fallback works for both None and "". * ``TestPlanParserContextFieldExtraction`` — covers task-1-3's ingestion path: ``extract_pr_context_metadata_from_yaml`` returns None pair for missing/None/absent inputs; collapses whitespace to None; warns on non-string ``context_title``; ``parse_plan`` threads the values onto ``ParseResult.pr_context_*``. Also updates ``test_models.py::test_minimal_contract`` from the literal ``"1.0"`` schemaVersion assertion to ``"1.1"`` — the coder flagged this as a known follow-up in commit 75d8ca09c (coder cannot push test edits under the role boundary). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address review feedback on PR #2555 (#2548) Blocking fix: - _populate_contract_from_plan now also preserves PRMetadata.deferred_actions alongside context_branch / context_pr_number. The conditional-ACK gate at decisions.py:complete_phase writes deferred actions; the populator's start_phase=implement re-entry path was silently wiping them, erasing the merge-blocking Pre-merge Obligations handoff. Add a regression test in orchestrator/tests/test_short_flow_contract_population.py. Non-blocking improvements: - extract_pr_context_metadata_from_yaml now warns symmetrically on non-string context_description (mirrors the context_title branch), preventing silent str() coercion of structured planner values. - Updated schemaVersion / _migrate_schema_version_to_1_1 docstrings to reflect that the bump fires at every load (mode="after"), not lazily on next save, and to acknowledge the migration is silent (no audit entry). - Aligned TestPRMetadataContextEmptyStringSemantics docstring with reality (planner path collapses empty strings to None; only hand-edited or migrated payloads can produce a "" PRMetadata). - New tests for the symmetric context_description warning. * docs: document schema 1.1 and pr.context_* fields (#2548) Slice-1 lands the schema delta + planner-prompt update half of the context-PR mechanism (#2548): `PRMetadata` grows four optional `context_*` fields and `Contract.schemaVersion` defaults to `"1.1"` with an additive `1.0 → 1.1` migration. The actual context-PR mechanism (branch creation, PR opening, slice-1 base wiring) is implemented in slices 3-4 and gets its own end-to-end documentation pass in slice-5. This commit updates the docs that reference contract examples and the yaml-tasks `pr:` block so they reflect the slice-1-landed schema state: - `docs/templates/plan.md`: add optional `context_title` / `context_description` keys to the yaml-tasks `pr:` example as commented-out hints, plus a new prose blockquote explaining when planners may emit them and which sibling fields (`context_branch`, `context_pr_number`) are orchestrator-populated. - `docs/architecture/sdlc-pipeline.md`: bump the example `schemaVersion` from `1.0` to `1.1` and add a "Schema 1.1 (#2548)" blockquote summarising the additive migration. - `docs/guides/sdlc-pipeline.md`: same `schemaVersion` bump in the example JSON plus a short blockquote pointing readers at the migration semantics. The PR-stack diagrams, BRC-history file naming, and slice-1-base discussion in `docs/guides/concurrent-execution.md`, `docs/architecture/orchestrator.md`, `docs/reference/orchestrator-cli.md`, and `docs/guides/babysit-pr.md` remain untouched — those describe behavior that does not yet exist on this branch and are slice-5's responsibility once the mechanism is wired end-to-end. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Allowlist plan_parser.py for file-size hard cap on egg/issue-2548/work (#2548) Slice-1 (foundation) tester NACK: on the egg/issue-2548/work merge target, slice-1's extract_pr_context_metadata_from_yaml + ParseResult.pr_context_* plumbing stacks on top of #2527's validate_task_role_alignment additions, pushing shared/egg_contracts/plan_parser.py to ~1,530 lines and breaching the 1,500-line hard cap that scripts/check-file-sizes.py enforces. The slice-1 branch alone is at 1,388 lines (clean), but the work-branch state that the lint actually runs against is over. Fix per reviewer_contract's forward-looking concern and tester's blocking finding: add the file to scripts/file-size-allowlist.yaml under #2548 so make lint passes during the slice-1 BRC. Decomposition is tracked under the same issue and is the cheaper of the two unblock option…

egg-reviewer Bot approved these changes May 7, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 7, 2026

View reviewed changes

jwbron merged commit ec60216 into main May 7, 2026
21 checks passed

jwbron mentioned this pull request May 8, 2026

Pipeline work branch is being rebased onto main, breaking isolation and causing slice rebase conflicts #2570

Closed

james-in-a-box Bot mentioned this pull request May 8, 2026

Egg/issue 2548/work #2572

Merged

Navigation Menu

Fix #2549: skip already-merged slices on pipeline restart - #2552

Fix #2549: skip already-merged slices on pipeline restart#2552
jwbron merged 2 commits into
mainfrom
egg/issue-2549

jwbron commented May 7, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jwbron commented May 7, 2026

Summary

Test plan

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Correctness verified

Tests

Non-blocking suggestions

1. Bootstrap pass adds sequential GitHub I/O to startup (low priority)

2. Reconciler thread can leak if bootstrap raises (defensive)

3. Bootstrap parent uses s.dependencies[0], not parent_branch_at_creation (consistency note)

4. test_bootstrap_does_nothing_when_pipeline_repo_unset docstring is slightly misleading

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

1. Bootstrap pass adds sequential GitHub I/O to startup

2. Reconciler thread can leak if bootstrap raises

3. Bootstrap parent uses s.dependencies[0], not parent_branch_at_creation

4. test_bootstrap_does_nothing_when_pipeline_repo_unset docstring is misleading

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Verifications

1. Bootstrap parallelisation (routes/pipelines.py:12434-12482)

2. Reconciler thread leak fix (routes/pipelines.py:12493-12500)

3. Bootstrap parent prefers parent_branch_at_creation (routes/pipelines.py:12442-12450)

4. Test now exercises step (A) under pipeline.repo = None (tests/test_slice_run_loop_integration.py:1107-1156)

No new issues found

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

3. Bootstrap parent uses `s.dependencies[0]`, not `parent_branch_at_creation` (consistency note)

4. `test_bootstrap_does_nothing_when_pipeline_repo_unset` docstring is slightly misleading

3. Bootstrap parent uses `s.dependencies[0]`, not `parent_branch_at_creation`

4. `test_bootstrap_does_nothing_when_pipeline_repo_unset` docstring is misleading

1. Bootstrap parallelisation (`routes/pipelines.py:12434-12482`)

2. Reconciler thread leak fix (`routes/pipelines.py:12493-12500`)

3. Bootstrap parent prefers `parent_branch_at_creation` (`routes/pipelines.py:12442-12450`)

4. Test now exercises step (A) under `pipeline.repo = None` (`tests/test_slice_run_loop_integration.py:1107-1156`)