Fix #2465: coerce Contract field assignments via validate_assignment by jwbron · Pull Request #2484 · jwbron/egg

jwbron · 2026-05-06T22:08:33Z

Summary

Add model_config = ConfigDict(validate_assignment=True) to Contract so setattr (the path apply_mutation / _set_value ultimately takes) coerces values back to their declared type.
Closes apply_mutation leaves current_phase as plain str (no validate_assignment on Contract) #2465.

Before this change, apply_mutation(field_path="current_phase", new_value="plan") left current_phase as a plain str until the next save/load round-trip — breaking .value reads and emitting PydanticSerializationUnexpectedValue warnings on the next save_contract.

This is Option A from the issue (the principled fix). Option B would have only normalised current_phase; validate_assignment=True covers every enum/typed field on the contract uniformly. Blast radius is small: the wrap-mode _migrate_phases_to_slices validator already short-circuits on non-dict input, and the only mode="after" validator (_require_issue_or_pipeline_id) is a cheap assertion.

Test plan

New regression test TestApplyMutation::test_current_phase_string_assignment_coerces_to_enum covers both acceptance criteria — type is PipelinePhase after the mutation, and model_dump(mode="json") emits no PydanticSerializationUnexpectedValue warning.
pytest tests/shared/egg_contracts/test_validator.py — 25/25 pass.
pytest tests/shared/egg_contracts/test_models.py tests/shared/egg_contracts/test_models_gaps.py orchestrator/tests/test_contracts_routes.py — 208/208 pass.
make test — only pre-existing failures (3× TestPathTraversalProtection in gateway/tests/test_phase_api.py, reproduce on main).

— Authored by James Wiesebron with help from Claude Code

`apply_mutation(field_path="current_phase", new_value="plan")` was leaving `current_phase` as a plain `str` until the next save/load round-trip, because `_set_value` calls `setattr` and `Contract` lacked `validate_assignment=True`. That broke `.value` / `.name` reads and emitted `PydanticSerializationUnexpectedValue` warnings on `model_dump`. Enable `validate_assignment` on `Contract` so all enum and typed-field assignments are coerced — the principled fix per the issue, covering every enum field rather than just `current_phase`.

egg-reviewer

Approve — Option A is the right fix. validate_assignment=True correctly closes the "string-stored, enum-expected" gap on current_phase without disturbing the existing wrap-mode and after-mode validators (the wrap-mode _migrate_phases_to_slices short-circuits on non-dict input as advertised, and _require_issue_or_pipeline_id is a cheap assertion). I traced the wired-in side effects — _legacy_phases is preserved across re-validation on assignment, and the only call sites that mutate Contract fields directly (orchestrator/routes/pipelines.py:5131, :14445, :14452, :14493, orchestrator/routes/decisions.py:291, shared/egg_contracts/orchestration.py:414) all pass type-correct values. Test exercises both acceptance criteria from #2465 (enum identity post-assign + no PydanticSerializationUnexpectedValue on the next dump). LGTM.

Non-blocking — `apply_mutation` no longer reports invalid values as a failed mutation

shared/egg_contracts/validator.py:107-120 catches only (KeyError, IndexError, AttributeError) from _set_value. With this change, an out-of-domain value now raises pydantic_core.ValidationError from inside setattr, which is not in that tuple. Reproduced locally:

apply_mutation(contract=c, role=Role.HUMAN, actor='t',
               field_path='current_phase', new_value='invalid_phase_value')
# UNCAUGHT: ValidationError: Input should be 'refine', 'plan', 'implement' or 'pr'

So the contract /mutate route in orchestrator/routes/contracts.py:348 (where new_value comes from arbitrary client JSON) goes from "silent corruption — a 500 will surface eventually on serialization" to "uncaught ValidationError → unhandled-exception 500". Strictly an improvement — failing fast is better than persisting junk — but the route no longer gets a structured MutationResult(success=False, message=...) for the invalid-value case, which means the client sees an opaque 500 instead of the clean 4xx the surrounding code is shaped for. The gateway phase_api.py:291 caller is unaffected because next_phase.value is always a valid enum string there.

Suggested follow-up (separate PR is fine): add pydantic_core.ValidationError (or pydantic.ValidationError) to the except tuple in apply_mutation so the new strictness flows back through the existing MutationResult(success=False, ...) channel. A regression test covering "invalid string for an enum field returns success=False" would lock in the contract.

Non-blocking — scope of `validate_assignment`

This change applies the new strictness only to Contract. Sibling models (Task, Slice, Decision, AgentExecutionModel, etc.) still happily store task.status = "garbage" etc. That matches #2465's "Option A is the principled fix" framing, but the asymmetry is worth surfacing — once the pattern is established, extending it to the typed-field-bearing nested models (or, equivalently, adding validate_assignment=True to a shared base config) would make the validation surface uniform. Not for this PR.

— Authored by egg

With Contract.model_config = ConfigDict(validate_assignment=True) (#2484), setattr on a Contract field with an out-of-domain value now raises pydantic.ValidationError instead of silently storing the bad value. Catch it alongside the existing (KeyError, IndexError, AttributeError) tuple in apply_mutation so the contract /mutate route returns a structured 4xx via MutationResult(success=False, ...) instead of an opaque 500. Addresses non-blocking review feedback on #2484.

james-in-a-box · 2026-05-06T22:25:43Z

Thanks for the review! Both items addressed:

Item 1 — apply_mutation no longer reports invalid values as a failed mutation: fixed-in-PR (commit 8b24572)

Added pydantic.ValidationError to the except block in apply_mutation so out-of-domain values flow through the existing MutationResult(success=False, message=...) channel instead of bubbling up as an unhandled 500. Confirmed your reproduction first (apply_mutation(field_path="current_phase", new_value="invalid_phase_value") raises uncaught ValidationError on 8bbbe31), then locked the new behaviour in with TestApplyMutation::test_invalid_enum_value_returns_failed_mutation — asserts success=False, the error message names the field, and the original value is unchanged. The contract /mutate route in orchestrator/routes/contracts.py:348 now returns a structured 4xx for the invalid-value case, matching the surrounding code's shape. Decided to land this here rather than a follow-up because it's directly caused by the validate_assignment=True change in this PR — felt like the right scope to keep them together.

Item 2 — scope of validate_assignment (sibling models): deferred-to #2490

Filed as a follow-up because (i) the reviewer explicitly scoped it out of this PR ("Not for this PR") and (ii) extending it to Task / Slice / Decision / AgentExecutionModel plus auditing existing call sites is meaningfully larger than #2465's minimal diff. Issue #2490 captures both options the reviewer suggested (per-model vs. shared base config) and lists the call sites to audit.

— Authored by egg

egg-reviewer

Approve — re-review of the delta from 8bbbe31 to 8b24572.

The new commit addresses the prior non-blocking suggestion exactly as proposed: apply_mutation now catches pydantic.ValidationError and routes it through the existing MutationResult(success=False, message=...) channel. Verified behaviour end-to-end:

Import is correct (shared/egg_contracts/validator.py:11): from pydantic import ValidationError is the canonical pydantic v2 re-export of pydantic_core.ValidationError, so the same object the validator-on-assignment path raises.
Catch placement is correct (shared/egg_contracts/validator.py:123-133): separate except after the (KeyError, IndexError, AttributeError) tuple. ValidationError is a ValueError subclass — it does not overlap with the prior tuple, so the order doesn't matter, and the prior tuple still doesn't accidentally swallow it.
Message threads field_path so the route's _error(result.message, ...) returns something the client can act on (e.g. Invalid value for current_phase: 1 validation error for Contract …).
No partial-state risk: setattr raises before pydantic stores the bad value, so contract.current_phase is unchanged, and the early return precedes audit-entry creation, so audit_log is not appended for the failed mutation. The new test asserts both invariants (assert sample_contract.current_phase is PipelinePhase.REFINE after the failed call).
Test is on the production code path (tests/shared/egg_contracts/test_validator.py:286-311): drives through apply_mutation rather than constructing a MutationResult directly, so a regression in the catch tuple breaks the test.
Local run: PYTHONPATH=. pytest tests/shared/egg_contracts/test_validator.py → 26/26 pass.

Sibling-model strictness (item 2 from the prior review) is tracked as #2490 with explicit per-model vs shared-base options and the call-site audit list — appropriate scoping.

Non-blocking — `/mutate` route returns 403 for invalid-value errors

orchestrator/routes/contracts.py:357-371 returns status_code=403 for every success=False from apply_mutation. That status code is semantically correct for the role-authorization failure path (validate_mutation rejection), but it is misleading for the two value-error paths:

The pre-existing (KeyError, IndexError, AttributeError) clause (Failed to apply mutation: … for bad field_path or out-of-range index).
The new ValidationError clause this PR adds (Invalid value for {field_path}: …).

A client that gets a 403 reasonably retries with a different role. For Invalid value for current_phase: … the right status is 400 (malformed input) or 422 (well-formed but semantically invalid). This is pre-existing — the new code merely follows the established (flawed) pattern, and the prior review's suggestion was specifically that the new strictness flow through the existing MutationResult channel, which is what landed. Worth a follow-up to discriminate authorization vs value errors at the route boundary, but not for this PR.

— Authored by egg

james-in-a-box · 2026-05-06T22:38:54Z

egg is addressing review feedback...

james-in-a-box · 2026-05-06T22:40:35Z

Thanks for the re-review and the approval!

Item — /mutate route returns 403 for invalid-value errors: deferred-to #2495

Filed as a follow-up because the reviewer explicitly scoped it out ("Worth a follow-up to discriminate authorization vs value errors at the route boundary, but not for this PR") — reason (iii) in the deferral framework. The 403 behaviour is pre-existing in orchestrator/routes/contracts.py:357-371; this PR's ValidationError clause merely follows the established pattern, exactly as the prior review suggested ("flow through the existing MutationResult channel"). Issue #2495 captures the proposal (discriminate via a MutationResult.error_kind field or distinct exception types), names the three branches the route needs to map, and lists the regression-test acceptance criteria.

— Authored by egg

james-in-a-box · 2026-05-06T22:41:20Z

egg feedback addressed. View run logs

4 previous review(s) hidden.

…2484) * Fix #2465: coerce Contract field assignments via validate_assignment `apply_mutation(field_path="current_phase", new_value="plan")` was leaving `current_phase` as a plain `str` until the next save/load round-trip, because `_set_value` calls `setattr` and `Contract` lacked `validate_assignment=True`. That broke `.value` / `.name` reads and emitted `PydanticSerializationUnexpectedValue` warnings on `model_dump`. Enable `validate_assignment` on `Contract` so all enum and typed-field assignments are coerced — the principled fix per the issue, covering every enum field rather than just `current_phase`. * Surface pydantic ValidationError from apply_mutation as MutationResult With Contract.model_config = ConfigDict(validate_assignment=True) (#2484), setattr on a Contract field with an out-of-domain value now raises pydantic.ValidationError instead of silently storing the bad value. Catch it alongside the existing (KeyError, IndexError, AttributeError) tuple in apply_mutation so the contract /mutate route returns a structured 4xx via MutationResult(success=False, ...) instead of an opaque 500. Addresses non-blocking review feedback on #2484. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* 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>

* Initialize SDLC contract for issue #2474 * slice slice-1: Cleanup — k3s only, drop dead test tiers (#2533) * Slice 1 (coder portion): retire e2e tier scaffolding (#2474) Drops the real-LLM end-to-end test scaffolding the coder role can reach under its file boundaries (`pyproject.toml`, `Makefile`, `integration_tests/agent_findings.py`): - Remove `e2e` and `agent_flaky` pytest markers from `pyproject.toml`. The matching `tests/config/test_ci_config.py` required-markers assertion is in the tester's scope; tester picks it up alongside task-1-2 (delete `tests/functional/`) so the marker set lands consistently. - Drop the `test-e2e` Make target, its `.PHONY` entry, and its `make help` line; retag `test-integration` to k3s in the help block and module banner. `test-security` is retained. - Delete `integration_tests/agent_findings.py` — the JSONL findings recorder for the agent_flaky fuzz tier; orphan once `test_agent_security_fuzz.py` is removed by tester (task-1-3 e2e tests). - Stage `.github-staging/workflows/test-e2e.yml` as a deletion-marker: agent file-boundaries block writes under `.github/`, so the staged file's header explicitly directs the human reviewer to `git rm .github/workflows/test-e2e.yml` (and the marker itself) rather than `git mv` it into place. The PR builder's auto "Move staged `.github/` changes" step (issue #2508) surfaces the marker. Tasks split: - task-1-3 coder portion: pyproject markers, Makefile target, agent_findings.py, .github-staging marker. - task-1-3 tester portion (handed off): delete `integration_tests/test_e2e_workflow.py` and `integration_tests/test_agent_security_fuzz.py`; update `tests/config/test_ci_config.py` required-markers set. - task-1-1 / task-1-4 (handed off): conftest edits live in tester's scope (`**/conftest.py`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Drop .github-staging/ deletion-marker; rely on pre-merge condition (#2474) Address reviewer_code NACK on slice-1 v1: the staging-promote pattern in `_build_github_staging_manual_step()` (orchestrator/routes/pipelines.py:8490) unconditionally renders `git mv .github-staging/<path> .github/<path>` boilerplate for every staged file — there's no opt-out for "this marker expresses a deletion intent." A reviewer who skims past the YAML-comment header inside the staged file and follows the auto-generated `git mv` either fails loudly ("destination exists") or, with `git mv -f`, silently overwrites the live workflow with the retired stub — neither resolves into the intended `git rm`. The documented BRC pattern for "human action that agents cannot push through the gateway" is `--pre-merge-condition` on a reviewer ACK (issue #1998 / `_collect_pre_merge_obligations`), which renders as a "Pre-merge Obligations" section in the PR body with a do-not-merge banner. reviewer_contract attached such an obligation on their v1 ACK, so the merger sees the `git rm` instruction unambiguously without the contradictory staging-promote step. Behaviour change: none against the runtime pipeline. The live `.github/workflows/test-e2e.yml` deletion remains a pre-merge human obligation; only the in-tree marker file is removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Slice-1 tester scope: delete tests/functional/, k3s-only conftests, retire e2e tests Picks up everything in the tester's gateway file scope for slice-1 (issue #2474): task-1-2 (delete tests/functional/): - Remove all 5 files under tests/functional/. Acceptance criterion: `tests/functional/` no longer exists. NOTE: the matching `functional:` marker registration in pyproject.toml AND the `tests/functional/conftest.py` allowlist entry in scripts/check-hardcoded-ports.py are gateway-blocked from the tester role (only coder can push pyproject.toml / scripts/). Both have been HANDOFFed back to the coder for inclusion in their next propose; see the HANDOFF message issued alongside this commit. Leaving the marker registered is harmless (no tests carry the marker any more); leaving the allowlist entry registered is harmless (the file is gone so the lint script never visits it). task-1-1 (k3s-only egg_stack): - integration_tests/conftest.py: drop `_docker_egg_stack()`, the EGG_RUNTIME=docker branch in `egg_stack`, the `docker_available` import, and stale docker-compose comments. `egg_stack` now skips with a clear pointer to docs/guides/testing.md when kubectl is unavailable. - integration_tests/local_pipeline/conftest.py: same treatment for `local_pipeline_stack`. Drops the COMPOSE_FILE / MOCK_SANDBOX_DIR constants, `_cleanup_orphaned_containers`, the docker-compose-up block, and the `docker_available` import. task-1-4 (retire orphan agent-led helpers in integration_tests/conftest.py): - Delete `run_claude_structured()`, `assert_agent_verdict()`, the `AgentVerdict` dataclass (including `infrastructure_failure`), `VERDICT_SCHEMA`, `TEST_AGENT_SYSTEM_PROMPT`, and the orphaned `_allocate_test_container_ip()`, `_capture_container_logs()`, `_preflight_gateway_check()` helpers. Drop the now-unused imports (`json`, `requests`, `ContainerNetworkConfig`, `build_sandbox_docker_cmd`). task-1-3 (delete e2e test files; tester scope): - rm integration_tests/test_e2e_workflow.py - rm integration_tests/test_agent_security_fuzz.py - tests/config/test_ci_config.py: required-markers assertion narrowed from {integration, functional, e2e, security, agent_flaky} to {integration, security}, with a docstring reference to issue #2474. Test infrastructure preserved: - GATEWAY_PORT remains imported and re-exported from integration_tests/conftest.py because test_network_security.py imports it directly via `from integration_tests.conftest import GATEWAY_PORT, exec_in_container`. - isolated_container / external_container / test_container fixtures are retained for the test_credential_security and test_network_isolation tiers (both still in tree). They will skip in k3s mode (the docker network name does not resolve), but slice-3 of this PR train adds k3s-native equivalents that supersede them. Acceptance criteria verified for the tester portion: - `tests/functional/` no longer exists. - `grep -rn "run_claude_structured|assert_agent_verdict"` returns no hits. - `grep -nE "EGG_RUNTIME=docker|_docker_egg_stack|docker_available"` on the two conftest files returns no hits. - `make lint` passes. * Slice-1 cleanup: drop functional marker + stale port allowlist (#2474) Address tester HANDOFF 2cc2c216-4c53-45 (non-blocking, raised on coder v2 ACK). Now that `tests/functional/` and `integration_tests/docker-compose.yml` are gone (slice-1 tester commit 3827cb5), this commit cleans up the dead-weight references to those paths that the tester role is gateway-blocked from reaching: - `pyproject.toml`: drop the `functional:` marker registration. The marker was the last live reference to the deleted `tests/functional/` tier; `tests/config/test_ci_config.py` was narrowed by tester to `required = {integration, security}` so the required-markers test still passes (subset check) — but the marker registration itself was orphan after the tier deletion. - `scripts/check-hardcoded-ports.py`: remove two stale `ALLOWLIST_PATHS` entries pointing at files that no longer exist: `integration_tests/docker-compose.yml` and `tests/functional/conftest.py`. The latter matters for task-1-2's acceptance criterion `grep -rn "tests.functional|@pytest.mark.functional"` returns no hits — the regex `tests.functional` matches the literal string `tests/functional/conftest.py` (`.` matches `/`), so the allowlist entry was a real gap, not just polish. `make lint` is clean; `tests/config/` test suite still passes with the trimmed marker set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * rm e2e workflow * Address review: drop more dead code from k3s-only cleanup Follow-ups on PR #2533 review (#2533 (review)): - Delete now-orphan integration_tests/local_pipeline/mock-sandbox/ (Dockerfile + phase-runner.sh) — only consumer was the deleted docker fallback in local_pipeline/conftest.py. - Remove tests.utils.gateway_client.docker_available() and its re-export — zero remaining callers after this PR removed the conftest call sites. - Strip stale -m "not functional" from Makefile (test, test-all) and update docs/guides/testing.md §2 step 8. - Rewrite integration_tests/conftest.py docstring to describe what the legacy fixtures actually do under k3s. Add explicit pytest.skip in isolated_container/external_container/test_container when the stack is k8s-backed (was silently skipping with a generic-sounding "could not start container" message). - Drop unused certs_volume field from EggStack; document why compose_project / external_network are retained. - Expand __all__ in integration_tests/conftest.py to cover the re-exported public surface (EggStack, ContainerInfo, exec_in_container, GATEWAY_PORT) — previously listed GATEWAY_PORT only. - Drop STRUCTURE.md mock-sandbox entry. Skipping the "except FileNotFoundError, subprocess.TimeoutExpired:" nit — ruff format 0.15.12 actively strips parens from except tuples, so the parenthesized form would not survive `make lint-fix`. Part of pipeline issue-2474-v2; the terminal slice carries the program-level narrative. * Address review: remove stale entries from STRUCTURE.md Drop the four file entries from the integration_tests/ tree listing that this PR (slice-1 cleanup) deletes: docker-compose.yml, agent_findings.py, test_agent_security_fuzz.py, test_e2e_workflow.py. Reviewer noted the local_pipeline/ subsection was already updated in a423d31 but the parent listing was missed. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2495: discriminate authorization vs. value errors at /mutate boundary (#2517) * Fix #2495: discriminate authorization vs. value errors at /mutate boundary The `/mutate` route was returning 403 for every `MutationResult.success=False`, which is correct for role-authorization rejections but misleading for value/path errors (bad `field_path`, out-of-range index, out-of-domain enum value). A client receiving 403 for `Invalid value for current_phase: …` would reasonably retry with a different role, which can't help. Adds `error_kind: Literal["authorization", "value"] | None` to `MutationResult` so the route can map cleanly without parsing message strings: 403 for authorization, 400 for value errors. Adds regression tests for all three branches. * Address review: assert error_kind in validator tests + export MutationErrorKind Closes the validator-level test gap flagged in the PR review: - test_apply_invalid_mutation_rejected now asserts error_kind == "authorization" so a regression that drops the discriminator on the role-rejection path fails at the unit-test boundary, not just the route boundary. - test_invalid_enum_value_returns_failed_mutation now asserts error_kind == "value" for the pydantic ValidationError path. - New test_invalid_path_returns_failed_mutation covers the (KeyError, IndexError, AttributeError) path through _set_value and asserts error_kind == "value". Re-exports MutationErrorKind from shared/egg_contracts/__init__.py so callers can type-hint against the discriminator without reaching into the validator submodule. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document .github-staging/ convention in agent-roles reference [doc-updater] (#2516) * docs: document .github-staging/ convention in agent-roles reference * docs: correct tester guidance — HANDOFF instead of .github-staging/ The tester's allowed_patterns in shared/egg_restrictions/patterns.py covers only test files, conftest, pin files, and .egg-state/agent-outputs/ — it does not include .yml/.yaml/.json. AgentFilePattern.can_write requires both a non-blocked path AND a positive allowlist hit, so .github-staging/workflows/ci.yml returns False for the tester even though .github-staging/ is not on the tester's blocked list. A tester following the previous text would attempt to stage CI fixes under .github-staging/ and be rejected. Replace that advice with the correct path: hand off to the coder via HANDOFF, mirroring the existing coder→tester handoff pattern. Surfaced by egg-reviewer on PR #2516. The patterns.py / agent_roles.py divergence the same review noted is tracked separately in #2521. * docs: expand tester Directed Coordination to cover outbound HANDOFF --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py (#2525) * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py #2514 added `.github/` to `TESTER_ROLE.blocked_write` in `agent_roles.py` but skipped the mirror entry in `TESTER_PATTERNS.blocked_patterns` in `patterns.py` — the planner prompt and the gateway saw different views of the tester's write scope. The omission was benign because the tester's allowlist already excludes `.github/`, but it stops being benign the moment someone widens that allowlist. Add `.github/` to `TESTER_PATTERNS.blocked_patterns` so both files agree, matching the lockstep pattern already used for documenter, autofixer, and conflict_resolver. Add regression tests in the gateway pattern suite and the shared restrictions unit suite. * Address review on #2525: load-bearing tests, slim comment - Use `.github/test_actions.py` as the load-bearing assertion in both test files. It matches the tester's `**/test_*.py` allowlist, so only the new `.github/` blocked entry stops it. The two pre-existing paths (`.github/CODEOWNERS`, `.github/PULL_REQUEST_TEMPLATE.md`) stay as breadth assertions; both are blocked even without the new entry, so they would have passed against the unfixed patterns. - Slim the rationale block in `patterns.py:224-234` to a one-liner pointing at `CODER_PATTERNS` for the full `.github/` rationale. The original 11-line comment over-claimed lockstep across roles the diff didn't actually touch (architect, task_planner, refiner, reviewer roles); the one-liner doesn't. Drift in the non-tester roles (architect/task_planner/risk_analyst/ refiner/reviewer/reviewer_contract) is tracked in #2532. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2490: extend validate_assignment to sibling Contract models (#2520) * Fix #2490: extend validate_assignment to sibling Contract models #2484 added `model_config = ConfigDict(validate_assignment=True)` to `Contract` so `setattr` on Contract fields coerces values back to their declared type. The reviewer flagged a remaining asymmetry: sibling models (`Task`, `Slice`, `Decision`, `AgentExecutionModel`, …) still silently accepted untyped assignments like `task.status = "garbage"`, so the validation surface was uneven across the contract object graph. Lift the config to a shared `EggContractBaseModel` (Option B from the issue) and have every model in `shared/egg_contracts/models.py` inherit from it, so the strictness applies uniformly without per-model duplication. Drop the per-model config from `Contract` itself — the shared base now provides it. The audit of sibling-model mutation sites (`shared/egg_contracts/orchestration.py` `set_execution`, `orchestrator/routes/decisions.py` `contract.pr =`, etc.) confirms they assign well-typed values (enum members or constructed model instances), so the new strictness does not break existing call sites. * Address PR #2520 feedback: fix nested-model test, refresh validator comment - Rewrite test_pr_metadata_invalid_deferred_actions_raises to assign a raw dict, which actually exercises pydantic's list-element coercion path on the outer setattr; the previous form raised from the inner DeferredAction(...) constructor regardless of validate_assignment (item 1). - Update the validator.py except ValidationError comment to reference EggContractBaseModel (where the config now lives) and add #2490 to the issue list, since the same catch now covers sibling-model setattrs (Task.status, Slice.status, Decision.type, ...) too (item 2). — Authored by egg --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2501: don't flip stale during an in-flight state-store probe (#2519) * Fix #2501: extend probe freshness while a probe is in flight `StateStoreProbe.snapshot()` flipped the cached `healthy` to `False` purely because the cache age crossed `interval * stale_multiplier`, even when a probe was actively running and about to refresh it. Under slice-spawn load `git worktree add` occasionally ran 30-40s, longer than the 30s default staleness window, so the request-path dual-write in `routes/health.py` recorded `unhealthy` and the BG callback recorded `healthy` 0-3s later when the same probe completed — producing the spurious `recent_transitions` flap pairs reported in the issue. Track probe start time and, while a probe is in flight, treat the cache as fresh until the in-flight probe itself has been running longer than the staleness window. A genuinely wedged probe still surfaces as stale once that bound is exceeded. * Address review feedback on #2501 in-flight grace fix - Document worst-case ~2*stale_window wedge-detection bound and the intentional 'fresh-but-old' semantics during the grace in snapshot()'s docstring (reviewer minor: source-recoverable rationale). - Expand the inline comment in the grace branch to cite the 'fix #1' framing from #2501 so the bound's rationale is recoverable from the source alone (reviewer minor). - Add an integration-level test that drives snapshot() while a real probe_now() is parked mid-probe on a worker thread, populating the in-flight flag and _probe_started_at_monotonic via the production code path. Closes the loop end-to-end so a future refactor that stops setting _probe_started_at_monotonic from probe_now() fails this test where the field-poking variants would silently keep passing (reviewer non-blocking suggestion). * Tighten worst-case wedge detection bound in snapshot() docstring Reviewer noted that the '~2 * stale_window' / '60s with 30s default' framing is loose. The BG loop fires every `interval` seconds, so the in-flight probe starts within `interval` of the last good completion, and the grace extends only until that probe's own age exceeds `stale_window`. The tight bound is therefore `stale_window + interval`, which under the defaults (interval=15s, stale_multiplier=2.0) is ~45s of blindness, not ~60s. The 2 * stale_window framing only saturates when stale_multiplier=1.0. Address-only docstring change; no behavior change. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty (#2518) * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty restart_phase reads its respawn roster from phase_exec.agents — a runtime cache that the route's own clear-then-spawn flow resets to []. If the spawn step fails before re-populating the cache, every subsequent restart_phase 400s on "No agents found in phase {phase} to restart" and start_pipeline 409s on the (now CANCELLED) status, leaving the only escape cancel_task(cleanup=true) — which discards all prior work. Fall back to the same deterministic source the executor itself uses: pipeline.active_roles (CUSTOM-mode / BABYSIT overrides, #1762) first, then get_roles_for_phase(repo, has_contract). Same precedence as _run_concurrent_phase, so the recovered roster matches what the next spawn would have produced anyway. * Match _run_concurrent_phase exactly: skip phase-default fallback when active_roles set When pipeline.active_roles is set but every entry is unknown to this orchestrator's AgentRole (defensive case after a role removal in a newer schema), the prior implementation fell through to get_roles_for_phase and expanded to the full phase-default roster. _run_concurrent_phase keeps its roles list empty in the same case, so the route's response (and the downstream worktree-delete / health-monitor reset) would diverge from what the spawn would actually produce. Convert the second 'if not agent_roles:' into an 'else:' attached to the override branch so the strict-parity behaviour matches: when an override is set we use it verbatim and never fall through. The final 'No agents found' 400 still fires honestly when the override is all-unknown. Adds a regression test that mutates active_roles post-construct (bypassing the field validator) to simulate the load-time-drift edge case. * Document deliberate route-vs-worker divergence in roster-derivation try/except The except Exception wrap around _get_roles_for_phase doesn't exist in _run_concurrent_phase, so a future reader auditing the two callsites for parity might mistake the bare-except for a bug rather than a deliberate route-specific safety floor (return 400 not 500). * Tighten line-range citation in roster-derivation comment to 12813-12840 --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2522: enumerate per-agent worktrees on phase restart (#2526) * Fix #2522: enumerate per-agent worktrees on phase restart restart_phase guessed worktree names as ``{pipeline_id}-{role}``, which misses slice-scoped worktrees (``{pipeline_id}-slice-{N}-{role}``) and leaves them on disk after a restart on a slice-based pipeline. Drive deletion off ``agent_salvage.enumerate_agent_worktrees`` (already the source of truth in ``cleanup_pipeline`` and salvage) and filter to the roles being restarted. The pipeline-level worktree (``agent_role=None``) and worktrees for non-restarted roles are intentionally preserved. * Address review: salvage before restart-phase delete; cleanup-style enumeration Blocking review feedback (#2522 / PR #2526): 1. Silent loss of unpushed agent commits during phase restart restart_phase now calls agent_salvage.auto_salvage_pipeline before the deletion loop (mirroring cleanup_pipeline's #2429 invariant). Restart is precisely the scenario where unpushed commits accumulate - operators hit it because agents got stuck or wedged - so the previous code was the one orchestrator-side worktree-delete path that bypassed salvage. Salvage failures are best-effort; deletion still happens. 2. Broken/corrupted worktrees regressed the original #1723 cleanup enumerate_agent_worktrees gates on a usable .git marker, so wedged-btrfs-mount worktrees were being silently skipped after this PR's switch to enumeration. Added validate_git=False flag (default stays True for salvage callers) so cleanup callers receive broken entries with repo_path falling back to the worktree dir itself. restart_phase now opts in to the cleanup-style listing. Non-blocking feedback addressed in the same commit: - Test fixture _make_pipeline_with_slice_agents builds AgentExecution with slice_id populated, matching what concurrent_executor writes. - New test exercises continue-on-error across three worktrees with the middle one's delete raising; locks down loop semantics. - Narrower exception class (OSError | ImportError | RuntimeError) around enumerate_agent_worktrees. - log_extras suppresses slice_id=None on non-slice pipelines. New tests: - test_restart_phase_continues_after_partial_worktree_deletion_failure - test_restart_phase_salvages_before_deleting_worktrees - test_restart_phase_salvage_failure_is_nonfatal - test_restart_phase_deletes_broken_worktree_without_git_marker - test_validate_git_false_returns_broken_worktrees - test_validate_git_false_preserves_validated_repo_path --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2539: drop duplicate `slice ` prefix in non-terminal slice PR titles (#2540) `create_slice_pr` was rendering non-terminal slice PR titles as `slice slice-1: …` because `slice_id` already starts with `slice-`. Drop the literal prefix so the title is just `{slice_id}: {slice_name}` (e.g. `slice-1: Cleanup — k3s only, drop dead test tiers`), and update the two test assertions and docs reference that pinned the buggy form. * Fix #2531: add `--for STATUS` to producer pre-confirm wait-loop (#2536) When every reviewer ACKed the current version, no further `CONSENSUS_ACK` / `CONSENSUS_NACK` events arrive on the bus. The orchestrator's directed `STATUS` nudge ("Ready to confirm — all confirm preconditions satisfied", `metadata.ready_to_confirm == True`) is the only signal that the global preconditions cleared, but the producer prompt's pre-confirm wait-loop filter omitted `STATUS` — so the producer slept through the nudge and only woke via the health-monitor `OVERSEER_ALERT` backstop minutes later, observed in pipeline `issue-2474-v2` slice-1 (6/8 stall, ~57 min phase elapsed). The reference doc at `agent-wait-patterns.md` already prescribed waiting on `STATUS` for the pending-acks recovery path; the prompt template just hadn't caught up. This change closes that gap and adds a regression test that pins `--for STATUS` plus the on-wake guidance ("go to step 5 CONFIRM if `metadata.ready_to_confirm`, otherwise re-enter the wait") across every producer role × phase. The `_send_brc_confirmation_nudge` docstring is updated to reflect the new pre-confirm filter. * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import (#2542) * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import Two bugs surfaced when issue-2474-v2 spawned slice-2: every container exited four seconds in with no work attempted, leaving slice-2's integration branch empty and the PR-create call to fail with "No commits between ...". Bug A (`gateway/git_client module unavailable`): the deployed orchestrator image ships only `orchestrator/`, `routes/`, `health_checks/`, and the shared `egg_*` packages — `gateway/` is not copied. The `from gateway.git_client import build_rebase_onto_args` call added by #2512 always raises ImportError in production, so every slice integration branch reconciliation silently fails. Inline the canonical argv builder as `_build_rebase_onto_args` in `orchestrator/gateway_client.py`; the gateway server's `/git` endpoint remains the authoritative allowlist boundary, and CI test paths that keep `gateway/` on `sys.path` continue to work unchanged. Bug B (slice-2 consensus reached at elapsed_seconds=0.0): the per-slice tracker registry already keys by `{pipeline_id}/{slice_id}`, but `ConcurrentPhaseExecutor.check_consensus()` had two slice-unaware fallback paths. When slice-2's tracker is fresh and empty (the steady state right after spawn, before any agent has proposed), (1) `reconstruct_tracker_from_messages` was called with the bare pipeline_id and (2) the message-bus fallback scanned `store.get_messages(pipeline_id)` pipeline-wide. Slice-1's eight CONSENSUS_CONFIRMED messages are persisted under the bare pipeline_id and have the same role names as slice-2's roster, so both paths falsely declared consensus on slice-2's first poll iteration. Gate both fallbacks (and the matching path in `handle_consensus_confirmed_signal`) on `slice_id is None`. The in-memory per-slice tracker is the authoritative source; an empty fresh tracker correctly returns is_complete=False and the polling loop keeps going. * Address #2542 review: slice-scope idempotency, fix test syntax, doc tweaks Five issues from egg-reviewer on the #2535 PR: 1. test_check_consensus_slice_isolation.py: replace dead try/except that used Python-2 catch-and-bind syntax (`except A, B:`) with a direct `PipelineConfig(concurrent_execution=True)` constructor call. The original block was unreachable — `concurrent_execution` is a normal Pydantic bool field that cannot raise on assignment — and the misleading syntax would surprise any future reader. 2. routes/signals.py: scope `_existing_confirmed_for_role` to a slice so the idempotency probe doesn't see sibling-slice CONFIRMs as "already confirmed for this role". A new `slice_id` parameter filters by `metadata["slice_id"]`; the per-slice tracker path tags CONSENSUS_CONFIRMED writes with that same metadata key. Without this, slice-2's first coder CONFIRMED would be silently suppressed (no bus message, no #1473 marker) because slice-1's coder CONFIRMED was still in the bus under the bare pipeline_id. Pipeline-scoped (slice_id is None) callers continue to see only pipeline-scoped messages, preserving legacy behaviour exactly. 3. orchestrator/gateway_client.py: soften the "Mirrors" claim in the `_build_rebase_onto_args` docstring. The helper does NOT call validate_git_args (which would defeat the inlining) and emits stripped argv, so document those two intentional differences. 4. orchestrator/stacked_pr_reconciler.py: update the module docstring to point at the inlined `_build_rebase_onto_args` in orchestrator.gateway_client (with a note explaining why the inlining is needed and why the security floor is unchanged). 5. tests/test_consensus_confirmed_idempotent.py: extend the helper `_fake_message` with a `slice_id` parameter and add three regression tests: - slice-2's first CONFIRMED is NOT marked idempotent by a slice-1 CONFIRMED in the bus - within slice-2, the second CONFIRMED IS deduped - pipeline-scoped callers ignore slice-scoped CONFIRMs The wider sweep of slice-unaware peer-consensus lookups in kubernetes_monitor.py, startup_reconciliation.py, routes/pipelines.py status display, and the tier-1 health checks is left for #2409 (the existing tracker covers the same root-cause: slice_id needs to flow through more places). PR body updated to flag this. * Fix #2538: slice PRs carry contract.pr narrative on every slice (#2543) Every slice PR — terminal and non-terminal — now renders the planner-authored program title, description, test plan, and manual steps from contract.pr, so reviewers see program rationale on whichever slice they open first. Previously only the terminal slice carried the narrative; reviewers approaching slice-1 (the bottom of the stack and the canonical merge entry point) saw only task bullets plus a pointer to the terminal slice's PR. Title disambiguation: terminal slice gets the bare program_title; non-terminals get a [<slice-id>] prefix so the GitHub PR list stays scannable when several stacked PRs are open at once. Per-merge obligations remain terminal-only (the merge gate is the last-to-merge PR in the stack) — the existing #2354 invariants and fail-fast assertion are preserved. The terminal slice keeps a "merge gate / umbrella" banner so reviewers can spot the merge gate; non-terminals skip it. The old "see terminal slice's PR for the program-level narrative" pointer is gone — the narrative is right there now. * Fix #2537: attribute slice PRs to orchestrator, not coder (#2541) * Fix #2537: attribute slice PRs to orchestrator, not coder The slice-PR creation path is orchestrator-only — `gh pr create*` is blocked for the implement phase, and the pr phase has no agent spawn. But `_run_implement_phase_slices` was hard-coding `agent_role="coder"` on the synthetic session that opens the slice PR, which caused the gateway to label the PR `agent:coder` and inject `agent_role=coder` into the `` comment. Pass `agent_role="orchestrator"` so slice PRs match the attribution the non-sliced `_auto_create_pr` path already uses. * Fix /status/wait test flake: handshake before publish The three event-bus tests in TestStatusWaitRoute used a 0.1s sleep in the fire thread before publishing — racy on slow CI. The route's preamble (cursor parse, terminal short-circuit, staleness probe, current_sequence() snap) can exceed the grace window, so the publish lands before event_bus.subscribe(None, _on_event) and the event is never delivered. Replace the sleep with a deterministic handshake that polls event_bus._wildcard_handlers and returns the moment the route has subscribed. The message-bus path (test_overseer_alert_wakes_route) uses a different wake mechanism and is left untouched. * docs: add --for STATUS to producer pre-confirm wait-loop example (#2546) Syncs docs/guides/concurrent-execution.md with the fix from #2531: the producer RESPOND TO REVIEWS (step 4) wait-loop now includes --for STATUS so the orchestrator's "Ready to confirm" directed nudge wakes the producer when every reviewer has already ACKed and no further CONSENSUS_ACK/CONSENSUS_NACK will arrive. docs/reference/agent-wait-patterns.md was already updated in the same PR; this doc had a stale copy of the canonical snippet. Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

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

egg-reviewer Bot approved these changes May 6, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

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

Extend validate_assignment=True to sibling Contract models for uniform validation #2490

Closed

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 6, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

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

Discriminate authorization vs. value errors at /mutate route boundary #2495

Closed

4 tasks

jwbron merged commit 8080791 into main May 6, 2026
21 checks passed

jwbron mentioned this pull request May 7, 2026

Fix #2490: extend validate_assignment to sibling Contract models #2520

Merged

5 tasks

Navigation Menu

Fix #2465: coerce Contract field assignments via validate_assignment - #2484

Fix #2465: coerce Contract field assignments via validate_assignment#2484
jwbron merged 2 commits into
mainfrom
egg/fix-2465-validate-assignment

jwbron commented May 6, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

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

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

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

Uh oh!

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

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jwbron commented May 6, 2026

Summary

Test plan

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Non-blocking — apply_mutation no longer reports invalid values as a failed mutation

Non-blocking — scope of validate_assignment

Uh oh!

This comment has been minimized.

This comment has been minimized.

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

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Non-blocking — /mutate route returns 403 for invalid-value errors

Uh oh!

This comment has been minimized.

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

Uh oh!

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

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Non-blocking — `apply_mutation` no longer reports invalid values as a failed mutation

Non-blocking — scope of `validate_assignment`

Non-blocking — `/mutate` route returns 403 for invalid-value errors