Fix #2515: restart_phase falls back to deterministic roster when agents cache empty by jwbron · Pull Request #2518 · jwbron/egg

jwbron · 2026-05-07T03:02:34Z

Summary

restart_phase reads its respawn roster from phase_exec.agents, a runtime cache the route itself resets to [] between clear and spawn. If the spawn step fails, the cache is left empty and every subsequent restart_phase 400s on No agents found in phase {phase} to restart. Combined with start_pipeline 409ing on the resulting CANCELLED status, the pipeline becomes unrecoverable except by cancel_task(cleanup=true), which discards all prior work (refine drafts, plan drafts, completed slices, …).
Add a fallback that mirrors what _run_concurrent_phase already does at spawn time: prefer pipeline.active_roles (CUSTOM-mode / BABYSIT overrides per Replace interactive mode with a generic custom-phase MCP primitive #1762), then fall back to get_roles_for_phase(repo, has_contract). Only return the 400 when all three sources produce nothing.

The deterministic derivation matches the roster the next spawn would have produced anyway, so the fix doesn't change the happy path — it only kicks in when the cache is empty.

The issue also flagged that the same self-corrupting clear-then-spawn pattern likely exists in start_pipeline's recovery branch. That's a separate audit and not addressed here.

Closes #2515.

Test plan

.venv/bin/pytest orchestrator/tests/test_restart_phase.py — 28/28 pass, including two new regression tests:
- test_empty_agents_falls_back_to_phase_default_roster — CANCELLED pipeline with phase_exec.agents=[] and no active_roles recovers via the phase-default roster.
- test_empty_agents_uses_active_roles_override_when_set — same shape but with a CUSTOM-mode active_roles=["coder","tester"]; verifies the fallback honours the override rather than expanding to the full phase-default roster.
Ruff check clean on the two changed files.

…ts 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.

egg-reviewer

Approve. The fix is tightly scoped, correctly diagnoses the wedge, and faithfully mirrors the deterministic-roster derivation that _run_concurrent_phase already runs at spawn time, so the response payload's agents_to_restart will match the roster the next spawn produces.

Verification

Analogy holds. The new fallback in orchestrator/routes/pipelines.py:3155-3186 is byte-for-byte the same logic as _run_concurrent_phase at orchestrator/routes/pipelines.py:12801-12825: pipeline.active_roles → get_roles_for_phase(phase, include_reviewers=True, repo=pipeline.repo, has_contract=...), with the same per-value try/except ValueError skip for roles unknown to this orchestrator's AgentRole. Same args, same filters — so the "agents_to_restart" list returned by the route equals what _run_concurrent_phase will actually spawn, including the has_contract / repo-based reviewer filtering (reviewer_contract drop, EGG_ONLY_REVIEWERS filter).
Lock discipline. The fallback runs inside the pipeline state lock at pipelines.py:3117, after the pipeline is re-loaded fresh, before the phase_exec.containers/agents = [] clear at pipelines.py:3209-3210. No TOCTOU window introduced.
Worktree-deletion / health-monitor reset / response-body parity. All three downstream consumers of agent_roles (spawner.gateway.delete_worktrees at line 3254, _hm.reset_agent at line 3323, and the agents_to_restart response payload) get the same deterministic roster the spawn will produce, so per-agent worktree cleanup and per-role anchor reset don't silently no-op on the empty-cache path.
Validators backstop the bare-except. Pipeline._validate_active_roles (orchestrator/models.py:893-935) already rejects unknown role values and reviewer-only / cross-phase-only rosters at write time, so the only realistic path that hits the except ValueError: continue skip is a future orchestrator removing an AgentRole value that an in-flight pipeline still references — the safer-than-nothing default-roster fallback is correct there.
Tests pass locally. pytest orchestrator/tests/test_restart_phase.py → 28/28 pass. The two new tests cover both fallback branches: phase-default roster (asserts producer presence rather than pinning the reviewer set, which correctly accommodates repo/has_contract filtering) and active_roles override (asserts exact equality, which is the right shape for that branch).
Scope. The PR explicitly defers the analogous self-corrupting clear-then-spawn pattern in start_pipeline's recovery branch to a separate audit, which is appropriate scoping for a wedge fix.

Non-blocking observations

Subtle behavioural divergence from _run_concurrent_phase on all-unknown active_roles. When pipeline.active_roles is set but every entry is unknown to this orchestrator's AgentRole, the inner loop produces agent_roles == [], and the new code's if not agent_roles: check at pipelines.py:3164 falls through to get_roles_for_phase and expands to the full phase-default roster. _run_concurrent_phase (pipelines.py:12801-12825) keeps its own roles list empty on the same input — its else: branch only fires when _roster_override is falsy, not when iteration produces nothing. This is a defensive case (the model validator rejects unknown values at write time, so it would only trigger after a role removal in a newer orchestrator loading an older pipeline), but the divergence means the route's response and the eventual spawn would disagree on what gets spawned. If you want strict parity, gate the second if not agent_roles: on _roster_override is None or not _roster_override instead — or, better, accept the divergence as a deliberate "expand rather than empty-spawn" choice and document it in the existing comment block. Either way, surfacing the choice explicitly avoids a future reader assuming the two callsites are identical.
Worktree deletion is still pipeline-scoped, not slice-scoped. Pre-existing and not introduced by this PR, but worth flagging since #2515's reproduction is on a slice-based pipeline (issue-2474): the agent_worktree_id = f"{pipeline_id}-{role.value}" pattern at pipelines.py:3252 doesn't include slice_id, while slice-aware worktrees do (issue-2261-v9-slice-2-coder per the docstring at pipelines.py:3442). On a slice-based pipeline, restart_phase will leave per-slice agent worktrees behind. Out of scope for this fix, but consider following up — a phase restart on a slice pipeline should probably enumerate enumerate_agent_worktrees and delete all matches for the phase, rather than guess names.
Deferred import is consistent with file convention (the same from egg_contracts.agent_roles import get_roles_for_phase as _get_roles_for_phase deferred import appears at pipelines.py:12798 inside _run_concurrent_phase), so the second copy at pipelines.py:3166-3168 is fine. Mentioning only because a reader might be tempted to hoist it; don't — it would change the import surface for unrelated callers of the module.

— Authored by egg

… 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.

james-in-a-box · 2026-05-07T03:20:21Z

Thanks for the careful review. Per-item disposition:

Subtle behavioural divergence on all-unknown active_roles — fixed-in-PR (commit 26a5e6c). Went with strict parity: converted the second if not agent_roles: into an else: attached to the override branch in orchestrator/routes/pipelines.py:3155-3192. Now when active_roles is set we use it verbatim and never fall through to get_roles_for_phase, mirroring _run_concurrent_phase at pipelines.py:12800-12825 exactly. The all-unknown-override case now hits the final No agents found 400 honestly rather than promising a phase-default roster the spawn would never produce. Added test_active_roles_set_but_all_unknown_returns_400_not_phase_default (orchestrator/tests/test_restart_phase.py:1148-1192) which mutates active_roles post-construct to bypass _validate_active_roles and simulate load-time drift after a role removal in a newer orchestrator. 29/29 tests pass; ruff clean.
Worktree deletion is pipeline-scoped, not slice-scoped — deferred-to #2522. You explicitly flagged it as out of scope for this fix and suggested following up; filed #2522 with the pointer to pipelines.py:3252, the slice-aware example at pipelines.py:3442, and the suggestion to enumerate via enumerate_agent_worktrees rather than guess names. The reproduction in restart_phase 400s when phase_exec.agents is empty (e.g. after a failed prior restart) — should always work for cancel_task(cleanup=false) pipelines #2515 (issue-2474) is exactly the slice-based pipeline shape your concern targets, so the follow-up is queued behind real impact.
Deferred import is consistent with file convention — disagree (no change requested). You explicitly said "don't hoist it" — leaving as-is. Mentioning here only so it appears in the per-item disposition list.

— Authored by egg

egg-reviewer

Approve. The follow-up commit (26a5e6c) cleanly resolves the strict-parity divergence I flagged in the prior review. The route's roster derivation now matches _run_concurrent_phase byte-for-byte for the in-band code paths.

Verification

Strict parity achieved. pipelines.py:3163-3194 is now if _roster_override: <verbatim-with-skip> else: <get_roles_for_phase>, identical in shape to pipelines.py:12808-12833. The route no longer falls through to the phase-default roster on the all-unknown-override edge case, so the response payload's agents_to_restart, the worktree-delete loop, and the health-monitor reset can no longer disagree with what _run_concurrent_phase would actually spawn. The follow-up makes the PR's "no behavioural change to the happy path" claim from the original description fully accurate.
Final 400 still fires honestly. The outer-most if not agent_roles: return 400 at pipelines.py:3196-3199 correctly catches both legitimate-empty-roster cases (_roster_override truthy but every entry unknown; phase-default returns nothing). The route surfaces the dead-roster case as an HTTP 400 before mutating any pipeline state, which is strictly more informative than _run_concurrent_phase's silent empty-roster spawn — a deliberate improvement in the route's synchronous-HTTP context, not a divergence in the operator-visible contract.
No state mutation on the 400 path. The mutations at pipelines.py:3217-3229 (phase_exec.containers/agents = [], pipeline.status = RUNNING, run_epoch bump) all live below the empty-roster guard, so the route now provably does NOT corrupt CANCELLED state on the all-unknown branch. The new test asserts this explicitly (assert pipeline.status == PipelineStatus.CANCELLED).
Validator backstop is correctly characterised. The new test's docstring accurately calls out that _validate_active_roles (models.py:893-935) rejects unknown values at write time but cannot guard load-time drift after a role removal in a newer orchestrator. Mutating pipeline.active_roles post-construct to bypass the field validator is the right way to simulate that scenario in a unit test.
Tests pass. pytest orchestrator/tests/test_restart_phase.py → 29/29 pass on this branch. The new regression test_active_roles_set_but_all_unknown_returns_400_not_phase_default covers exactly the divergence the prior review flagged.
Ruff clean on both touched files.

Non-blocking observations

Defensive try/except Exception around the else: branch is route-specific and correct, but worth a one-line comment. The except Exception: # noqa: BLE001 at pipelines.py:3188 wrapping _get_roles_for_phase does not exist in _run_concurrent_phase — there, an import or call failure would propagate up and fail the worker thread. In the route it's appropriate (we'd rather emit a 400 than a 500 to the HTTP caller), but the existing comment block at pipelines.py:3155-3162 only documents the parity choice, not this deliberate divergence. A future reader auditing for "exact parity with _run_concurrent_phase" might assume the bare-except is a bug — a one-line # Catch derivation failures so the route returns 400 rather than 500 near pipelines.py:3188 would close that gap. Not blocking; the safety floor (no crash, fall through to honest 400) is intact.
Docstring on test_active_roles_set_but_all_unknown_returns_400_not_phase_default is clear and the assertion shape is right. Asserting both status_code == 400 AND pipeline.status == PipelineStatus.CANCELLED correctly pins both halves of the contract: the operator gets the honest 400 signal and state is not silently mutated past the failed restart. Worth keeping in mind as the canonical shape for "fail-loud-on-misconfiguration" tests in this file.

— Authored by egg

…ry/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).

james-in-a-box · 2026-05-07T03:35:14Z

Thanks for the re-review and the approval. Per-item disposition for the new round:

Route-specific try/except Exception deserves a one-line comment — fixed-in-PR (commit e7ec32d). Added a 7-line comment block at orchestrator/routes/pipelines.py:3188-3195 documenting why the bare-except is deliberate divergence from _run_concurrent_phase (which lets the same failure propagate up the worker thread), and why returning a 400 is more useful to the operator in the synchronous-HTTP route context than letting a 500 escape. Future readers auditing the two callsites for "exact parity" will see explicitly that the bare-except is a route-specific safety floor, not a bug. Ruff clean; 29/29 tests still pass.
Test docstring + assertion shape are clear — disagree (no change requested). This was positive feedback flagging the test as the canonical shape for "fail-loud-on-misconfiguration" tests in this file — listing here only so it appears in the per-item disposition.

— Authored by egg

egg-reviewer

Approve. The follow-up commit (e7ec32d) cleanly addresses the comment-block suggestion from the prior review. Future readers auditing the two callsites for "exact parity with _run_concurrent_phase" will now see explicitly that the bare-except wrapping the else: branch is a deliberate route-specific safety floor (400 vs 500 on derivation failure), not a missed parity item.

Verification

Comment is semantically accurate. The new 7-line block at orchestrator/routes/pipelines.py:3189-3195 correctly characterises the divergence: _run_concurrent_phase (called from inside the _run_pipeline worker thread spawned at pipelines.py:16675) lets a _get_roles_for_phase failure propagate up to the worker thread, while the route catches it and falls through to the existing if not agent_roles: return 400 guard at pipelines.py:3203-3206. The "honest 400 in synchronous HTTP context vs unhandled 500" framing matches the actual control flow on both callsites.
Comment placement is right. Sitting on the except Exception as exc: # noqa: BLE001 line at pipelines.py:3188 is the precise location where a future reader doing a side-by-side parity audit would notice the asymmetry, so the comment surfaces at the moment of confusion rather than buried in the existing block at pipelines.py:3155-3162 (which already documents the strict-parity choice for the if/else shape).
No behavioural change. Comment-only delta — git diff 26a5e6c..e7ec32d shows a single 7-line addition inside the existing except body, no logic touched. The 29 tests in test_restart_phase.py still cover the same code paths.
Ruff clean. # noqa: BLE001 on the existing line is preserved.

Non-blocking observations

Line-range citation in the new comment is slightly off. The comment at pipelines.py:3191 cites pipelines.py:12808-12833 as the analogous block in _run_concurrent_phase, but line 12808 is part of an unrelated EGG_SLICE_ID / extra_env comment block (the # the key is in _PROTECTED_ENV_KEYS`` paragraph), and line 12833 lands mid-_get_roles_for_phase call at `repo=pipeline.repo,`. The actual analogous block — the import, the `roles: list[AgentRole] = []` initialiser, the `if _roster_override / else / for r in _get_roles_for_phase(...)` derivation, and the inner `try/except ValueError` skip — runs from `pipelines.py:12813` through `pipelines.py:12840`. Tightening to `12813-12840` (or even `12815-12840` if you prefer to anchor on `roles: list[AgentRole] = []` rather than the deferred import) would have a future reader land directly on the parity-relevant lines instead of starting in an unrelated comment paragraph. Not blocking — the qualitative reference (`_run_concurrent_phase`) is unambiguous and grep-able, and line numbers in code comments inevitably drift, but since the comment exists specifically to help with a parity audit, accurate boundaries amplify the value.

— Authored by egg

james-in-a-box · 2026-05-07T03:48:47Z

Thanks for the re-review and the approval. Per-item disposition for the new round:

Line-range citation in the roster-derivation comment is slightly off — fixed-in-PR (commit a3a4f9e). Tightened the citation at orchestrator/routes/pipelines.py:3191 from 12808-12833 to 12813-12840. The new range now starts on the deferred import (from egg_contracts.agent_roles import get_roles_for_phase as _get_roles_for_phase) and ends on the inner try/except ValueError: continue skip, landing a future reader doing a parity audit directly on the four lines that matter (import + roles: list[AgentRole] = [] initialiser + if _roster_override / else / for r in _get_roles_for_phase(...) derivation + inner skip), rather than the unrelated EGG_SLICE_ID / extra_env paragraph at 12805-12810. Comment-only delta; ruff clean; 29/29 tests still pass.

— Authored by egg

egg-reviewer

Approve. The follow-up commit (a3a4f9e) tightens the line-range citation in the roster-derivation comment exactly as suggested. The new boundaries land a future reader doing a parity audit directly on the four parity-relevant lines, rather than the unrelated EGG_SLICE_ID / extra_env paragraph.

Verification

New range is byte-accurate. orchestrator/routes/pipelines.py:12813 is the deferred import (from egg_contracts.agent_roles import get_roles_for_phase as _get_roles_for_phase), pipelines.py:12815 is the roles: list[AgentRole] = [] initialiser, pipelines.py:12816-12828 is the if _roster_override: branch, and pipelines.py:12829-12840 is the else: branch with the for r in _get_roles_for_phase(...) derivation and the inner try/except ValueError: continue skip ending on line 12840. The 12813-12840 boundaries cover exactly the import + initialiser + derivation + inner skip — the four pieces a parity audit cares about.
Old range was off by both ends. pipelines.py:12808 lands inside the unrelated EGG_SLICE_ID / extra_env paragraph at 12805-12810, and pipelines.py:12833 lands mid-_get_roles_for_phase call at repo=pipeline.repo,. The tightened range fixes both ends — start moves from a paragraph that has nothing to do with roster derivation onto the deferred import, and end moves from the middle of the for header onto the inner continue.
Comment-only delta. git diff e7ec32d..a3a4f9e shows a single character-range change inside one comment line (12808-12833 → 12813-12840), no logic touched. The 29 tests in test_restart_phase.py cover the same code paths.
Comment placement is unchanged. Still on pipelines.py:3191, sitting in the existing 7-line block that documents the deliberate route-vs-worker try/except Exception divergence — the precise location where a future reader doing a side-by-side parity audit would notice the asymmetry.

No new concerns. The PR has progressively addressed all three rounds of non-blocking suggestions (strict-parity else:, deliberate-divergence comment, accurate line-range citation) without touching the core fix or its test coverage. Ready to merge.

— Authored by egg

james-in-a-box · 2026-05-07T03:58:50Z

egg review completed. View run logs

9 previous review(s) hidden.

…ts 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>

* 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 3827cb5), 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 (#2533 (review)): - 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 a423d31 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>

* 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