Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import by jwbron · Pull Request #2542 · jwbron/egg

jwbron · 2026-05-07T05:03:26Z

Summary

Closes #2535. Two bugs that together caused issue-2474-v2 slice-2 to spawn-and-fail in four seconds:

Bug A — gateway/git_client module unavailable — orchestrator/Dockerfile does not COPY gateway/, so from gateway.git_client import build_rebase_onto_args (added by restart_phase fails to recover when slice integration branch already exists with non-ancestor commits from prior run #2512 at orchestrator/gateway_client.py:1424) always raises ImportError in the deployed image. Slice integration branch reconciliation silently fails. Inlined the canonical argv builder as _build_rebase_onto_args in the orchestrator package; the gateway server's /git endpoint remains the authoritative allowlist boundary.
Bug B — slice-2 false consensus at elapsed_seconds=0.0 — ConcurrentPhaseExecutor.check_consensus() had two slice-unaware fallback paths (tracker reconstruction and the message-bus subset check). The per-slice tracker registry is already keyed by {pipeline_id}/{slice_id} and spawn_all registers a fresh empty tracker per slice, but when that tracker is empty both fallbacks scanned messages under the bare pipeline_id — picking up slice-1's CONSENSUS_CONFIRMED messages whose from_role set matches slice-2's roster (coder/tester/documenter + 5 reviewers re-spawn per slice) and declaring consensus instantly. Gated both fallbacks (plus the matching path in handle_consensus_confirmed_signal) on slice_id is None. The in-memory per-slice tracker is now the authoritative source for slice work; an empty fresh tracker correctly returns is_complete=False.

The same root cause — message-bus probes scoped only to the bare pipeline_id — also showed up in _existing_confirmed_for_role (the idempotency probe in the same handler). That made slice-N's first per-role CONFIRMED look "already confirmed" because slice-(N-1)'s same-role CONFIRMED was still in the bus, suppressing the new bus write and the #1473 consensus-confirmed marker. Fixed in the review-feedback follow-up commit by tagging CONSENSUS_CONFIRMED messages with metadata["slice_id"] and filtering the probe by it. Pipeline-scoped (no slice_id) callers see only pipeline-scoped messages, preserving the legacy non-slice path exactly.

Out of scope (tracked separately)

The same slice-unaware get_peer_consensus_tracker(pipeline_id) / reconstruct_tracker_from_messages(pipeline_id, ...) pattern still exists in orchestrator/kubernetes_monitor.py:915,934, startup_reconciliation.py:308,312, routes/pipelines.py:4166,4183 (status display), and the tier-1 incomplete_consensus_stall.py / consensus_stall.py health checks. None of those are consensus-determining (they observe rather than decide), so the worst case is bogus observability. Tracked under #2409, which already scopes the slice-aware reconstruction work and the slice_id field on Message.

Test plan

make lint clean
pytest orchestrator/tests/test_check_consensus_slice_isolation.py orchestrator/tests/test_gateway_client_rebase_onto.py orchestrator/tests/test_concurrent_executor.py orchestrator/tests/test_consensus.py orchestrator/tests/test_consensus_polling.py orchestrator/tests/test_consensus_confirmed_idempotent.py orchestrator/tests/test_slice_signal_routing.py — 117 passed (95 from the original PR + 3 new slice-idempotency regressions + 19 from the slice signal routing suite that exercises the new slice_id kwarg)
New regression test_check_consensus_slice_isolation.py covers both directions: a fresh slice-2 tracker with stale slice-1 CONFIRMs in the bus must NOT report is_complete; a pipeline-scoped (slice_id=None) executor still uses the legacy message-bus fallback for Documenter stuck in heartbeat loop after consensus re-review; overseer doesn't detect or nudge #1471/Orchestrator fails to detect consensus completion -- agents loop indefinitely after all confirm #1615 tracker-loss recovery.
New regression TestNoGatewayPackageDependency::test_rebase_onto_succeeds_when_gateway_package_unavailable patches __import__ to refuse gateway* imports and confirms rebase_onto still produces the canonical argv.
New idempotency regressions in test_consensus_confirmed_idempotent.py:
- test_slice_two_first_confirmed_not_idempotent_when_slice_one_already_confirmed — slice-2's first CONFIRMED is NOT short-circuited by slice-1's CONFIRMED in the bus; bus message + BRC protocol should require commit+push before consensus confirmation #1473 marker are written.
- test_slice_idempotency_within_same_slice — within slice-2, the second CONFIRMED IS deduped.
- test_pipeline_scoped_call_ignores_slice_scoped_prior_confirms — legacy non-slice callers ignore slice-scoped CONFIRMs and don't tag their own writes with a slice_id.

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

egg-reviewer

Both bug fixes are correct and well-scoped. The slice-isolation gating in concurrent_executor.check_consensus() and the mirroring 404 path in handle_consensus_confirmed_signal are sound, and the new regression tests target the exact contamination surface that produced the 4-second false-consensus on slice-2. Bug A's inlining of _build_rebase_onto_args is the right call given the deployed orchestrator image lacks gateway/, and the gateway server's /git endpoint at gateway/gateway.py:2228 does in fact re-validate via validate_git_args, so dropping the client-side validate_git_args call doesn't reduce the security floor.

That said there are several issues — none individually blocking, but the first two warrant changes before merge.

Issues

1. Dead code + Python-2-looking syntax in the new test (`test_check_consensus_slice_isolation.py:48-52`)

config = PipelineConfig()
try:
    config.concurrent_execution = True  # type: ignore[attr-defined]
except AttributeError, ValueError:
    config.__dict__["concurrent_execution"] = True

Two problems:

Dead code. PipelineConfig.concurrent_execution is a normal Pydantic bool field with default=False (orchestrator/models.py:424). Assigning True cannot raise AttributeError or ValueError, so the __dict__ fallback never runs. The # type: ignore[attr-defined] comment also has no effect — the attribute is defined.
Misleading syntax. except AttributeError, ValueError: is Python 2 catch-and-bind syntax. Python 3.14 happens to accept it as a tuple expression (verified: ast.parse produces Tuple([AttributeError, ValueError])), but it is a SyntaxError on every Python from 3.0 through 3.13. Even though pyproject.toml pins requires-python = ">=3.14", this looks like a typo for except (AttributeError, ValueError): — and a future reader will assume the same. If anything went wrong with the assignment, this clause would silently swallow it via the __dict__ write, which bypasses Pydantic validation.

Replace the whole block with config = PipelineConfig(concurrent_execution=True) (or just config.concurrent_execution = True with no try/except). Either is one line and removes the confusion.

2. Slice-unaware `_existing_confirmed_for_role` in the same handler the PR modifies (`routes/signals.py:1464-1517`, called at `:1672`)

After your fix, slice-2's handle_consensus_confirmed_signal correctly hits the slice-2 tracker via tracker.handle_confirmed(agent_role). But further down at line 1672 the handler calls _existing_confirmed_for_role(pipeline_id, agent_role, current_phase), which scans store.get_messages(pipeline_id, limit=10000) with no slice scope. Slice-1's coder CONSENSUS_CONFIRMED message is in the bus under the bare pipeline_id and the same role/phase, so it returns has_final=True for the slice-2 coder.

That has two visible effects:

Line 1701: if not has_final: → slice-2's CONFIRMED message is never written to the bus, and _write_consensus_confirmed_marker (line 1720) is never called for slice-2's worktree. The marker is the signal auto-commit uses to skip pushing unreviewed WIP (#1473).
Line 1723: payload["idempotent"] = True — the agent gets back "your confirmation was a duplicate" even though, from slice-2's tracker perspective, it was the first time.

The PR is already in this function and already adds slice_id-aware logic. The natural fix is to have _existing_confirmed_for_role filter messages by phase + slice_id (e.g. via metadata.get("slice_id")), or to pass slice_id through and noop the idempotency check when slice_id is not None (since per-slice CONFIRMED writes are independent). Per the review rules, pre-existing inconsistent behavior in code the PR modifies is a blocking concern when the PR is the natural place to fix it. I'd downgrade to non-blocking only if you explicitly file a follow-up issue and reference it in a new comment near _existing_confirmed_for_role, the way #2409 is referenced at line 1554-1559 for reconstruction.

3. Other slice-unaware reconstruction / message-scan paths remain (non-blocking, but please file a tracker)

The PR fixes concurrent_executor.check_consensus() and handle_consensus_confirmed_signal. The same kind of "scan messages by bare pipeline_id" / "reconstruct without slice_id" exists in:

orchestrator/kubernetes_monitor.py:915,934 — get_peer_consensus_tracker(pipeline_id) with no slice, then reconstruct_tracker_from_messages(pipeline_id, graph). For multi-slice pipelines the bare tracker doesn't exist; reconstruction will replay slice-1's messages and the monitor's track-1/track-2 decision is made on a contaminated tracker.
orchestrator/startup_reconciliation.py:295,312 — same pattern, on orchestrator startup.
orchestrator/routes/pipelines.py:4166,4183 — pipeline status display; not consensus-determining, but renders bogus state for slice-mode pipelines.
orchestrator/health_checks/tier1/incomplete_consensus_stall.py:235 and consensus_stall.py:159 — health check scanning.

#2409 is referenced for a slice-aware reconstruction path; please mention #2409 (or file a sibling issue) in the PR body so the gap isn't lost. As-is the PR description only acknowledges the two paths it touches.

4. `_build_rebase_onto_args` doesn't quite mirror the gateway helper (`orchestrator/gateway_client.py:55-93` vs `gateway/git_client.py:2004-2068`)

The docstring says "Mirrors :func:gateway.git_client.build_rebase_onto_args," but two real differences exist:

The new helper strips inputs and emits the stripped form: ["--onto", new_base.strip(), old_base.strip(), branch.strip()]. The original keeps the unstripped form. Practically harmless for valid refs (the regex already rejects internal whitespace, and leading/trailing whitespace is rejected by the regex too — actually wait, the regex is checked on the stripped value, so " foo " would pass the regex but be sent as "foo" by the new helper vs " foo " by the gateway helper, which would then fail validate_git_args).
The new helper does not call validate_git_args — only the regex shape check. The gateway server re-validates, so this is safe; but the helper's claim to "mirror" the gateway version is too strong. Adjust the docstring (e.g. "performs the same ref-shape sanity checks") or just call validate_git_args here too if you want exact parity (you'd need to import from gateway.git_client, which defeats the point — so prefer the docstring tweak).

5. Stale docstring (`orchestrator/stacked_pr_reconciler.py:27`)

:func:`gateway.git_client.build_rebase_onto_args` and submitted

Production now goes through _build_rebase_onto_args in orchestrator/gateway_client.py. Update or note both helpers.

6. 404 from the slice-tracker-missing branch (non-blocking)

routes/signals.py:1591-1592 returns 404 when the slice-scoped tracker is gone (e.g. orchestrator restart mid-slice). This is correct — false-confirming via the bus is worse — but the agent-side retry behavior matters: a tight retry loop on 404s while the slice scheduler races to recreate the tracker could thrash. Worth checking that the agent's egg-orch consensus confirmed retry uses a backoff. Not a regression introduced by this PR, but a side-effect of the new strictness.

Test coverage gaps (advisory)

test_rebase_onto_succeeds_when_gateway_package_unavailable patches __import__ only during the call. That correctly verifies "no gateway* import is needed at call time," but module-load imports (which all happen before the patch) aren't exercised. Consider adding @patch.dict(sys.modules, {"gateway": None}) outside the with block (or running a pytest --import-mode importlib config that proves nothing pulls gateway at import time) for stronger coverage.
Neither new test exercises the case where slice-2's tracker has actually progressed (e.g. half the roles have ACKed). The fix shouldn't break it, but a positive test that slice-2's tracker reaches is_complete=True on its own merits would lock the contract.

— Authored by egg

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

jwbron · 2026-05-07T05:42:46Z

Thanks for the careful review. Pushed 4726e82 to address it.

Issue 1 (test syntax, blocking) — replaced the unreachable try/except block with PipelineConfig(concurrent_execution=True). You're right that concurrent_execution is a normal Pydantic bool field; the original block was both dead and used Python-2 catch-and-bind syntax that 3.14 happens to parse as a tuple expression. Gone.

Issue 2 (slice-unaware _existing_confirmed_for_role, blocking) — went with option A (filter on metadata["slice_id"]) because option B (noop the idempotency check when slice_id is not None) would let a retry-looping slice agent spray duplicate CONFIRMs into the bus within its own slice, which is the failure mode #1890 was tightening. The change:

_existing_confirmed_for_role(pipeline_id, agent_role, phase, slice_id=None) now filters by metadata.get("slice_id") == slice_id (None matches None, preserving the legacy non-slice path exactly).
Both per-slice CONFIRMED writers (final at signals.py:1702 and pending_acks at signals.py:1681) now tag metadata["slice_id"] when slice_id is not None.
Caller at signals.py:1672 passes slice_id=slice_id.
Three new regressions in test_consensus_confirmed_idempotent.py cover (a) slice-2 first CONFIRMED not deduped by slice-1, (b) within-slice idempotency still works, (c) pipeline-scoped callers ignore slice-scoped CONFIRMs.
Inline docstring on _existing_confirmed_for_role references Slice-scoped consensus trackers can't reconstruct from message store after orchestrator restart #2409 the way the existing reconstruction comment does.

Issue 3 (other slice-unaware paths, non-blocking) — flagged in the PR body under "Out of scope (tracked separately)" with the specific call sites (kubernetes_monitor.py:915,934, startup_reconciliation.py:308,312, routes/pipelines.py:4166,4183, the two tier-1 health checks). Pointed at #2409 since its fix sketch already covers slice_id on Message + a slice-aware reconstruct_tracker_from_messages — the same lever that closes those sites in one sweep. Did not file a sibling issue (skipped to keep this round narrowly review-driven; happy to file one if you'd prefer separate tracking).

Issue 4 (_build_rebase_onto_args docstring drift) — softened "Mirrors" to "Performs the same ref-shape sanity checks as", and called out the two real differences (stripped argv, no validate_git_args call). The audit boundary is the gateway server, not the helper.

Issue 5 (stale docstring in stacked_pr_reconciler.py) — updated to point at orchestrator.gateway_client._build_rebase_onto_args with a one-sentence note explaining why it's inlined and why the security floor is unchanged.

Issue 6 (404 retry storm risk, non-blocking) — not addressed in this PR. The slice-tracker-missing 404 branch is correct (false-confirming via the bus is worse), and verifying the agent-side retry backoff is its own piece of work.

Test coverage gap #1 (module-load gateway* import paranoia) — not addressed; would need a pytest config tweak rather than a per-test patch. Worth a follow-up if we want stronger coverage but didn't feel review-blocking.

Test coverage gap #2 (positive slice-N tracker test) — covered by the three new tests in test_consensus_confirmed_idempotent.py (the first asserts slice-2's CONFIRMED is actually written, with slice_id metadata, and that the marker is created).

egg-reviewer

The follow-up commit 4726e829 cleanly addresses both blocking concerns from the prior review and the two doc nits.

Verified

Issue 1 (test syntax). _make_pipeline now uses PipelineConfig(concurrent_execution=True) and the dead try/except is gone. Pydantic validates the field on construction; the # type: ignore[attr-defined] is also gone since the attribute is properly defined (orchestrator/models.py:424). test_check_consensus_returns_incomplete_for_empty_slice_tracker and test_pipeline_scoped_executor_still_uses_message_bus_fallback both pass.

Issue 2 (slice-scope idempotency). The change correctly threads slice_id end-to-end:

_existing_confirmed_for_role (orchestrator/routes/signals.py:1464-1535) takes a slice_id parameter and filters with metadata.get("slice_id") != slice_id. The semantics — None matches None (untagged messages preserve legacy behaviour), "slice-X" matches only itself — are exactly what the dedup probe needs.
The per-slice tracker path (signals.py:1692-1693) passes slice_id=slice_id. The fallback path (signals.py:1642) defaults to slice_id=None, which is correct because the slice-scoped 404 short-circuit at signals.py:1602-1610 ensures that path is only reached for pipeline-scoped (slice_id is None) callers.
Both bus writes tag metadata["slice_id"] via _slice_meta (signals.py:1718, 1741); the ternary {"slice_id": slice_id} if slice_id is not None else {} keeps the pipeline-scoped wire format byte-identical to before, so older orchestrator versions reading these messages don't choke on an unexpected key.
The three new regression tests in test_consensus_confirmed_idempotent.py exercise the right surfaces:
- slice-2 first CONFIRMED is NOT marked idempotent by a slice-1 CONFIRMED (and the marker IS written, defending #1473);
- within slice-2 the second CONFIRMED still dedupes;
- pipeline-scoped callers don't get false-deduped by slice-scoped CONFIRMs and don't tag their own writes.

I traced data flow on each test path (which from_role matches, which phase matches, what metadata.get("slice_id") evaluates to in the filter, what payload comes out of the handler) — everything is consistent.

Issue 4 / Issue 5 (docstrings). _build_rebase_onto_args now reads "Performs the same ref-shape sanity checks as ..." with the two intentional differences spelled out (stripped argv, no validate_git_args call, server is the audit boundary). The stacked_pr_reconciler.py module docstring now correctly points at the inlined helper with a note explaining the inlining motive and the security floor invariant.

Tests. The full block of related tests passes locally: test_consensus_confirmed_idempotent.py (7), test_check_consensus_slice_isolation.py (2), test_slice_signal_routing.py (18), TestConsensusConfirmedDedupRegression (the #1896 dedup regression — confirms pipeline-scoped legacy behaviour is intact). 37/37 green.

Non-blocking observations

Pipeline-scoped fallbacks now have an obvious filter to apply. signals.py:1626 ({m.from_role for m in messages if m.message_type == "CONSENSUS_CONFIRMED"}) and concurrent_executor.py:730-736 both still count slice-tagged CONFIRMs into a pipeline-scoped roster. With metadata["slice_id"] now populated, those scans could be tightened to (m.metadata or {}).get("slice_id") is None — but this is genuinely #2409 territory (the broader sweep across kubernetes_monitor.py, startup_reconciliation.py, the tier-1 health checks, and routes/pipelines.py status display), and you've documented that explicitly. Worth a one-line cross-reference in #2409 noting that this PR added the metadata field the future filter needs.
Order of tracker.handle_confirmed vs the idempotency probe. Pre-existing behaviour — tracker.handle_confirmed(agent_role) runs unconditionally at signals.py:1680 before the dedup check at 1692. The tracker should itself be idempotent on repeated calls, but a stray retry will still mutate tracker accounting state even when the bus write is suppressed. Not introduced or affected by this PR; flagging only because the new test surface (deliberate retries from a slice-2 agent) makes it slightly more interesting.
Coverage of orchestrator restart with both slices live. The new tests cover slice-2-first vs slice-1-already-CONFIRMED, but don't exercise the "tracker recreated mid-flight" race where slice-2's tracker disappears, the 404 path returns, and the agent retries. This is a deliberate design choice (#2409 will close the reconstruction gap when slice_id is on Message), so I'd leave it alone — but a future test that asserts the 404 is what the agent gets, not a false-confirm, would lock the contract.

LGTM.

— Authored by egg

james-in-a-box · 2026-05-07T05:57:34Z

egg review completed. View run logs

1 previous review(s) hidden.

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

* 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 suggested changes May 7, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 7, 2026

View reviewed changes

jwbron merged commit 685253d into main May 7, 2026
17 checks passed

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

docs: update slice-dag rebase_onto helper reference [doc-updater] #2547

Merged

Navigation Menu

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#2542
jwbron merged 2 commits into
mainfrom
egg/issue-2535

jwbron commented May 7, 2026 •

edited

Loading

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

jwbron commented May 7, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jwbron commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Out of scope (tracked separately)

Test plan

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Issues

1. Dead code + Python-2-looking syntax in the new test (test_check_consensus_slice_isolation.py:48-52)

2. Slice-unaware _existing_confirmed_for_role in the same handler the PR modifies (routes/signals.py:1464-1517, called at :1672)

3. Other slice-unaware reconstruction / message-scan paths remain (non-blocking, but please file a tracker)

4. _build_rebase_onto_args doesn't quite mirror the gateway helper (orchestrator/gateway_client.py:55-93 vs gateway/git_client.py:2004-2068)

5. Stale docstring (orchestrator/stacked_pr_reconciler.py:27)

6. 404 from the slice-tracker-missing branch (non-blocking)

Test coverage gaps (advisory)

Uh oh!

This comment has been minimized.

jwbron commented May 7, 2026

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Verified

Non-blocking observations

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jwbron commented May 7, 2026 •

edited

Loading

1. Dead code + Python-2-looking syntax in the new test (`test_check_consensus_slice_isolation.py:48-52`)

2. Slice-unaware `_existing_confirmed_for_role` in the same handler the PR modifies (`routes/signals.py:1464-1517`, called at `:1672`)

4. `_build_rebase_onto_args` doesn't quite mirror the gateway helper (`orchestrator/gateway_client.py:55-93` vs `gateway/git_client.py:2004-2068`)

5. Stale docstring (`orchestrator/stacked_pr_reconciler.py:27`)