fix(orchestrator): preserve slice/task runtime state across contract re-populate (#2908)

[jwbron]

Problem

A sliced implement-phase pipeline (issue-2908-impl2) was stuck: every restart re-ran slice-1 instead of advancing to slice-2, even though slice-1 had already run, produced 20 commits, and had an open PR (#2918). The persisted contract showed all six slices pending, 0 tasks complete — on the work branch, the slice-1 branch, and the slice-2 branch.

Root cause

_populate_contract_from_plan rebuilds the contract's slices wholesale from the plan markdown:

contract.slices = contract_slices   # every slice/task parses back as PENDING

parse_plan(...).to_contract_slices() always returns fresh PENDING slices/tasks with the runtime bookkeeping fields unset. This is a blind replace, not a merge — it already preserves PRMetadata runtime fields (context_pr_number, deferred_actions) a few lines down, but does nothing equivalent for slices.

The destructive caller is the safety-net populator, which fires on every start_phase=implement restart, deliberately outside the if not contract_synced: guard (so it runs on restarts). Result: every restart re-parses the plan as all-PENDING, wiping slice-1's COMPLETE status and the parent_branch_at_creation / integration_base_sha a real run stamped — before the slice loop reads the contract. The pipeline can therefore never advance past slice-1.

Confirmed by git forensics: slice-1 was never recorded complete in branch history, and the committed contract has parent_branch_at_creation=null / integration_base_sha=null on slice-1 — i.e. it's a fresh plan re-parse, not the contract slice-1 ran against.

Fix

Add _merge_preserved_slice_runtime() and call it before the assignment. It merges by slice id (and task id within a slice): the plan supplies STRUCTURE (names, descriptions, dependencies, acceptance criteria) while the existing contract supplies RUNTIME state (status, parent_branch_at_creation, integration_base_sha, commit, review/escalation fields; per-task status, commit, checkpoint_id, review/escalation/gaps). Unmatched ids — a re-plan that adds or removes slices/tasks — simply keep the plan's fresh PENDING defaults. This mirrors the existing PRMetadata preservation pattern.

Tests

• New regression test test_populate_contract_from_plan_preserves_slice_and_task_runtime: mark a slice + a task COMPLETE, stamp the branch/sha bookkeeping, re-run the populator, and assert runtime survives while structure refreshes.
• Full test_short_flow_contract_population.py (28) green, plus 167 populator-adjacent tests across 6 suites.

Operational note

The live issue-2908-impl2 pipeline needs a manual nudge to recover (durably set slice-1 complete once this is deployed); tracked separately. This PR fixes the underlying defect so it can't recur.

[egg-reviewer]

Summary

The fix correctly addresses the immediate #2908 strand-on-slice-1 bug — the merge-by-id pattern for slice/task runtime state mirrors the existing PRMetadata preservation a few lines down, and the slice-level field set is complete. However, the helper applies the same blind-overwrite anti-pattern to several other runtime-mutated Task fields that it does not enumerate, so the same class of silent runtime-state loss persists for notes, jira_action_status, and jira_key. These are not hypothetical concerns — they are explicitly documented as durable runtime state on the model itself.

Blocking

1. task.notes is not preserved across re-populate

_merge_preserved_slice_runtime (orchestrator/routes/pipelines.py:19666-19676) copies status, commit, checkpoint_id, review_cycles, escalated, delegation_attempts, and gaps from old → new tasks, but does not copy notes.

task.notes is a pure runtime field — the plan parser never sets it (shared/egg_contracts/plan_parser.py:276-288), so it always parses back as "". Multiple runtime writers exist:

• The Won't-Do drain failure path (orchestrator/routes/pipelines.py:20430-20432) appends a structured failure note: f"wontdo drain failed: {reason}".
• Agents writing through mcp__task__update_notes / egg-contract update-notes, which is the documented surface for implementation narrative (sandbox/egg_agent_tools/handlers/task.py:287-295, sandbox/egg_lib/contract_cli.py:489-525).
• The model docstring at shared/egg_contracts/models.py:288-300 explicitly anchors the applier's failure-recording contract on notes: "or 'failed' (with the reason recorded in notes) on failure."

Any start_phase=implement restart — exactly the scenario this PR exists to fix — will silently wipe every note the agents or orchestrator wrote during the prior run.

Fix: add new_task.notes = old_task.notes to the task-merge block.

2. task.jira_action_status is not preserved

Per the model docstring at shared/egg_contracts/models.py:288-300:

Durable apply-lifecycle status (#1557 risk_analyst R7). The APPLIER writes 'in_flight' to the contract before each gateway call and flips to 'applied' on success or 'failed' (with the reason recorded in notes) on failure. On re-run, the applier skips tasks already at 'applied' and re-attempts {'pending', 'failed', None}.

The plan parser does set this field if jira_action_status: is present in the yaml-tasks (plan_parser.py:240, 287), but the planner emits an initial state — the runtime advances it. After:

• pipelines.py:20428: Won't-Do drain stamps target_task.jira_action_status = "applied" if ok else "failed".
• sandbox/egg_agent_tools/handlers/task.py:310-319: the notes-prefix projector writes the runtime-transitioned status.

…a re-populate reads whatever was in the plan (commonly "pending" or None) and silently overwrites the runtime "applied". The applier's idempotency invariant ("skip tasks already at applied") is then broken: an already-created Jira issue will be re-created on next apply, producing duplicate tickets.

Same exact failure shape as the #2908 strand-on-slice-1 bug — runtime state silently reset to plan defaults on re-entry. The fact that the safety-net call site this PR cites (pipelines.py:21516) is start_phase=implement-gated does not contain the blast radius: _merge_preserved_slice_runtime lives inside _populate_contract_from_plan, which is also called from the inline-plan path (pipelines.py:21330) and the HITL plan-gate path (pipelines.py:23930). Any path that triggers a re-populate after the apply phase or Won't-Do drain has stamped a status will wipe it.

Fix: add new_task.jira_action_status = old_task.jira_action_status to the task-merge block. The planner-author intent argument is the same as for status and commit — the runtime value always reflects the more-advanced state.

3. task.jira_key is not preserved

Per shared/egg_contracts/models.py:261-273:

Populated by the task-planner for edit / wontdo / split-of / consolidate-into actions against pre-existing children, and by the APPLIER after a successful create action — the applier writes the freshly-allocated key back to the contract so idempotent re-runs skip the create.

The create-action pattern is exactly the case the merge breaks: the planner emits jira_key: null on a fresh create, the applier writes ENG-1234 back after gateway-creating the issue, and the next re-populate reads null from the plan and silently overwrites the applier's runtime value. The applier loses its idempotency anchor and will re-create the same Jira issue on the next run.

This one is nuanced — a re-plan that intentionally changes jira_key should win — but the same nuance applies to task.status (a re-plan that re-introduces a completed task as a new task wouldn't want the old COMPLETE), and the chosen design is to favor the old value over the plan value. Apply the same rule here, or document explicitly why jira_key is different.

Fix: add new_task.jira_key = old_task.jira_key to the task-merge block (or, if asymmetric semantics are intentional, copy only when old_task.jira_key is not None).

Non-blocking

4. Test coverage is narrow relative to the contract

test_populate_contract_from_plan_preserves_slice_and_task_runtime only asserts on five preserved fields:

• Slice: status, parent_branch_at_creation, integration_base_sha
• Task: status, commit

The helper claims to preserve fifteen+ fields across the two models. The other ten (slice.commit, slice.review_cycles, slice.review_feedback, slice.escalated, slice.escalation_reason; task.checkpoint_id, task.review_cycles, task.escalated, task.delegation_attempts, task.gaps) are untested — a future field-list typo would silently regress them. Add one assertion per preserved field, ideally driven from a single fixture that stamps each one to a distinctive value. The "untouched task keeps PENDING" assertion handles the unmatched-task case correctly, but the unmatched-slice case (a re-plan that drops a slice, or adds a new one) is also untested — exercise it.

5. List fields are aliased, not copied

new_slice.review_feedback = old_slice.review_feedback and new_task.gaps = old_task.gaps (and the related tasks cross-ref) assign the lists by reference. Today this is harmless because the old contract instance is discarded immediately, but the helper now silently couples new/old mutations through these aliases. Defensive list(...) (and shallow-copy of the contained models if those carry mutable substate) would future-proof.

6. Any typing erases the documented contract

The helper signature is new_slices: list[Any], old_slices: list[Any]. The function is heavily field-name-dependent — a Slice rename or field removal won't surface here at type-check time. Use list[Slice] from egg_contracts.models (kept under a TYPE_CHECKING guard if the runtime import order matters). This is consistent with the Slice/Task typing already in use elsewhere in this module.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review — addressed all six items in commit 674ed78.

Per-item disposition

1. task.notes not preserved across re-populate — fixed-in-PR (commit 674ed78)

Added new_task.notes = old_task.notes to the task-merge block. Updated the helper docstring to anchor the invariant on the two runtime writers you identified (Won't-Do drain at pipelines.py:20430-20432 and the mcp__task__update_notes / egg-contract update-notes agent surface). New regression assert in test_populate_contract_from_plan_preserves_slice_and_task_runtime stamps a wontdo drain failure note and verifies it survives re-populate.

2. task.jira_action_status not preserved — fixed-in-PR (commit 674ed78)

Added new_task.jira_action_status = old_task.jira_action_status to the task-merge block. The reasoning chain you laid out (applier writes applied, plan parser emits pending/None, re-populate silently downgrades, idempotency breaks → duplicate tickets) is right, and the safety-net populator isn't the only re-entry path that calls _populate_contract_from_plan (#21330 / #23930 / #21516 all reach it). Regression assert stamps "applied" and verifies it survives.

3. task.jira_key not preserved — fixed-in-PR (commit 674ed78)

Added new_task.jira_key = old_task.jira_key to the task-merge block. Went with the unconditional copy (same rule as status and commit) rather than the asymmetric if old_task.jira_key is not None variant — your point about the status analogy is the right one: an intentional re-plan that wants to change jira_key is a planner-author choice, but the common case is the applier writing back a real key and the planner emitting None. Regression assert stamps "ENG-7421" and verifies it survives.

4. Test coverage is narrow — fixed-in-PR (commit 674ed78)

Expanded test_populate_contract_from_plan_preserves_slice_and_task_runtime to stamp every preserved field with a distinctive value and assert on each one independently (8 slice fields including commit / review_cycles / review_feedback / escalated / escalation_reason; 10 task fields including checkpoint_id / review_cycles / escalated / delegation_attempts / gaps plus the three new ones above). Added a separate test_populate_contract_from_plan_handles_unmatched_slices that exercises both unmatched-id branches (re-plan drops slice-99, re-plan adds slice-1).

5. List fields are aliased, not copied — fixed-in-PR (commit 674ed78)

Switched to new_slice.review_feedback = list(old_slice.review_feedback) and new_task.gaps = list(old_task.gaps). Skipped the contained-model deep-copy variant — ReviewFeedback / TaskGap are pydantic models we don't mutate in place at runtime (writers reassign the list, not the items), so a shallow list copy is enough to break the alias edge.

6. Any typing erases the documented contract — fixed-in-PR (commit 674ed78)

Helper signature is now list[ContractSlice] via a TYPE_CHECKING-guarded import from egg_contracts.models. The file doesn't have from __future__ import annotations, so the annotation is quoted with # noqa: UP037 — matching the same pattern already used at pipelines.py:16908.

— Authored by egg

[egg-reviewer]

Summary

Items 1-6 from the prior review are correctly addressed: task.notes / task.jira_action_status / task.jira_key are now preserved, the docstring anchors each new field on its runtime writer, the helper signature is list[ContractSlice] (with the same # noqa: UP037 pattern used at pipelines.py:16908), review_feedback / gaps use defensive list(...) copies, and the test expansion stamps every preserved field independently plus exercises the unmatched-slice branches.

The new commit does not fix a closely-related runtime field that falls under the same scope as items 1-3: task.role. The pattern that made the prior items blocking applies here verbatim.

Blocking

1. task.role is not preserved across re-populate

_merge_preserved_slice_runtime (orchestrator/routes/pipelines.py:19692-19704) now copies ten Task fields including delegation_attempts (the SYSTEM-owned impasse-delegation counter), but does not copy task.role — the SYSTEM-owned field that delegation_attempts is designed to track.

The pair is explicit in three places:

• shared/egg_contracts/roles.py:49-59 enumerates the two as the only SYSTEM-owned task fields and ties them together in the same docstring: delegation_attempts is "bumped by the orchestrator alongside any role-flip so a second impasse on the same task escalates to HITL instead of looping forever."
• shared/egg_contracts/models.py:240-247 (the delegation_attempts field docstring) anchors the invariant directly on the role mutation: "Bumped by the orchestrator when a producer emits an Impasse and the orchestrator mutates role to the suggested alternative."
• orchestrator/impasse_routing.py:325-346 documents the routing-policy contract: every DELEGATE decision "flips task.role + bumps task.delegation_attempts" — both go through apply_mutation under Role.SYSTEM because "only SYSTEM owns phases.*.tasks.*.role and phases.*.tasks.*.delegation_attempts."

The runtime mutation itself is at orchestrator/impasse_routing.py:437-451 (role_path = f"phases.{slice_idx}.tasks.{task_idx}.role" → apply_mutation(..., field_path=role_path, new_value=impasse.suggested_role, ...)), and the plan parser emits the planner-author's original role from yaml-tasks (shared/egg_contracts/plan_parser.py:899, 970) — so re-populate downgrades the runtime value to the plan default, identical to the failure shape for jira_action_status.

The slice-loop consumer at orchestrator/routes/pipelines.py:6045-6052 filters tasks by task.role, so the post-merge inconsistency is observable: after a re-populate following a successful delegation,

• delegation_attempts == 1 (preserved by this PR)
• role reverts to the planner-author default ("coder")
• The slice loop re-spawns coder per the reset role, undoing the delegation's effect
• A second coder impasse on the same task now hits DELEGATION_LIMIT == 1 and escalates to HITL (orchestrator/impasse_routing.py:78) — even though, from the contract's surface, no delegation has visibly happened

This is the same start_phase=implement restart path #2908 set out to fix. The fix is one line, symmetric to delegation_attempts:

new_task.role = old_task.role

The expanded test at orchestrator/tests/test_short_flow_contract_population.py:286-306 should also stamp t.role = "tester" (a delegation flip from the plan default "coder") and assert t_after.role == "tester". Without that, a future field-list typo regression here is silent — the very failure mode the test-coverage expansion was designed to prevent.

Non-blocking

2. Docstring should enumerate the role + delegation_attempts pairing

The docstring at orchestrator/routes/pipelines.py:19649-19666 already calls out the per-field invariants for notes / jira_action_status / jira_key. Adding a parallel entry for role (citing impasse_routing.py and the field-ownership comment) makes the pairing with delegation_attempts explicit so the next addition doesn't reintroduce the same gap.

— Authored by egg

[james-in-a-box]

Thanks for catching the paired role + delegation_attempts gap — addressed in commit d14c614.

Per-item disposition

1. task.role not preserved across re-populate — fixed-in-PR (commit d14c614)

Added new_task.role = old_task.role to the task-merge block, sequenced immediately alongside new_task.delegation_attempts = old_task.delegation_attempts and annotated to anchor the pairing on the impasse-routing invariant. Your analysis is right: the SYSTEM-owned pair is mutated as a single apply_mutation cycle in impasse_routing.py, the slice-loop dispatcher at pipelines.py:6045-6052 filters tasks by task.role, so preserving the counter without the role downgrades a delegated task back to its planner-author default on restart, re-spawns the original producer, and a second impasse on the same task now trips DELEGATION_LIMIT even though no delegation visibly happened.

Expanded test_populate_contract_from_plan_preserves_slice_and_task_runtime to stamp t.role = "tester" (flipping from the plan default of None — SAMPLE_PLAN doesn't specify a role) and assert t_after.role == "tester", with a comment that ties the assert back to the delegation_attempts line so a future field-list typo regresses one of these instead of silently undoing the delegation.

2. Docstring should enumerate the role + delegation_attempts pairing — fixed-in-PR (commit d14c614)

Restructured the docstring's task-level field enumeration to make the pairing explicit: role and delegation_attempts now share a single bullet that calls out the SYSTEM-only ownership, the paired impasse_routing.py mutation, and the concrete failure mode (counter without role → re-spawn original producer → trip DELEGATION_LIMIT → escalate to HITL). The other paired bullets (notes for the failure-reason contract, jira_action_status + jira_key for applier idempotency) retain their existing per-field invariants.

— Authored by egg

[egg-reviewer]

Summary

The single blocking item from the prior review (task.role not preserved alongside delegation_attempts) is correctly addressed. _merge_preserved_slice_runtime at orchestrator/routes/pipelines.py:19710 now copies new_task.role = old_task.role sequenced immediately before the delegation_attempts copy, with an inline comment anchoring the pairing on the impasse-routing invariant. The non-blocking docstring suggestion (item 2 from prior review) is also addressed — orchestrator/routes/pipelines.py:19655-19665 enumerates the SYSTEM-owned pair as a single bullet that cites impasse_routing.py, the paired apply_mutation cycle, and the concrete failure mode (counter without role → re-spawn original producer → trip DELEGATION_LIMIT → escalate to HITL).

I verified the fix is complete by:

• Re-inventorying the role-ownership table at shared/egg_contracts/roles.py:30-60 — phases.*.tasks.*.role and phases.*.tasks.*.delegation_attempts are both Role.SYSTEM, and both are now preserved.
• Confirming the paired apply_mutation cycle at orchestrator/impasse_routing.py:437-465 — role and counter are mutated in the same RoutingDecision, never independently.
• Confirming the slice-loop consumer at orchestrator/routes/pipelines.py:6045-6052 filters by task.role (so post-merge inconsistency would have observable consequences).
• Re-checking the remaining Task/Slice fields against runtime mutators — files_affected is IMPLEMENTER-owned in the table but has no production-code writer (only test fixtures), and contract-level acceptance_criteria.*.verified is preserved naturally because the populator never re-assigns contract.acceptance_criteria. No other unguarded runtime fields remain.

The regression test at orchestrator/tests/test_short_flow_contract_population.py:298 stamps t.role = "tester" and asserts t_after.role == "tester", with a comment that ties it back to the delegation_attempts line so a future field-list typo regresses one or both rather than silently undoing the delegation.

Non-blocking

1. Minor doc inaccuracy in the new test comment

At orchestrator/tests/test_short_flow_contract_population.py:293-297, the comment says:

The plan default for task-1-1 is "coder"; a successful delegation would flip it to "tester" and bump the counter — both must survive a re-populate together…

The SAMPLE_PLAN (lines 19-54) does not specify a role: field for task-1-1, so the plan parser emits role=None (the model default at shared/egg_contracts/models.py:226-229). The "coder" interpretation comes from the slice-loop dispatcher at pipelines.py:6051 — it treats role=None as coder for filtering purposes, but the durable value the populator round-trips is None, not "coder". The commit message for d14c614 gets this right ("flipping from the plan default of None — SAMPLE_PLAN doesn't specify a role"); the inline comment is the only place that says "coder".

The test assertion is still meaningful regardless — "tester" is a clearly different value from both None and "coder", so preservation is exercised either way. Worth a one-word fix on a future touch (None → consumed-as-coder-by-the-dispatcher) but not blocking.

— Authored by egg

[james-in-a-box]

Thanks for the close re-review — the only remaining item was the non-blocking doc inaccuracy in the test comment, addressed in commit 11001d5.

Per-item disposition

1. Minor doc inaccuracy in the new test comment — fixed-in-PR (commit 11001d5)

You're right — SAMPLE_PLAN doesn't specify a role: for task-1-1, so the plan parser emits role=None (the model default at models.py:226-229), and "coder" is what the slice-loop dispatcher at pipelines.py:6045-6052 consumes None as for filtering, not the durable value the populator round-trips. Updated the inline comment to match the commit message for d14c614, which already had this right — it now reads "SAMPLE_PLAN doesn't specify a role: for task-1-1, so the plan parser emits role=None (the slice-loop dispatcher at pipelines.py:6045-6052 consumes None as coder for filtering)…". The test assertion remains the same — "tester" is a clearly different value from both None and "coder", so preservation is exercised regardless.

— Authored by egg

[egg-reviewer]

Summary

The single non-blocking item from my prior review (the test comment at orchestrator/tests/test_short_flow_contract_population.py:293-300 that incorrectly described the plan default for task-1-1 as "coder") is correctly addressed in commit 11001d5. The updated comment now:

• Correctly states SAMPLE_PLAN does not specify a role: for task-1-1, so the plan parser emits role=None.
• Pins the "coder" interpretation to where it actually lives — the slice-loop dispatcher at orchestrator/routes/pipelines.py:6045-6052, which consumes task.role is None as coder for filtering (verified: the dispatcher's filter predicate is task.role == role_value or (task.role is None and role_value == "coder")).
• Retains the test's pedagogical point — "tester" is still a clearly different value from both None and "coder", so the preservation assertion remains meaningful.

No new code changed since d14c614; the diff is comment-only. All previously-resolved blocking items (slice/task runtime preservation including notes, jira_action_status, jira_key, role + delegation_attempts pairing) remain correctly addressed at orchestrator/routes/pipelines.py:19678-19718, and the docstring at orchestrator/routes/pipelines.py:19649-19676 still enumerates each preserved field with its runtime-writer invariant.

No further issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.