fix(orchestrator): commit+push contract statefiles at slice boundaries (#3117)

[jwbron]

Summary

Fixes #3117 — the work branch's .egg-state/contracts/<pipeline_id>.json was committed exactly once ("Initialize SDLC contract") and never updated through the implement phase. All contract mutations — agent task-record updates via mutate_contract and the orchestrator's slice.status = COMPLETE flips — landed only on the shared pipeline worktree's disk copy. Observed on pipeline-2d9cc50d: the branch copy showed every task row pending forever, contributing to the #3114 misdiagnosis, and a worktree prune would have made that wrong record permanent.

The phase-boundary commit machinery (Persist statefiles after <phase> phase + push) already exists but is too coarse: it only fires when the whole implement phase completes, so a multi-slice phase accumulates everything uncommitted for its entire duration.

Change

• New closure _commit_and_push_slice_statefiles in _run_implement_phase_slices, reusing the existing work-branch pair: _commit_statefiles_to_worktree (idempotent, pipeline-scoped glob that includes the contract since Plan-phase contract update not reaching PR branch tip (blocks #1825) #1829) + GatewayClient.push_worktree_branch (launcher-auth, built-in non-FF reconcile from #3088).
• Hooked into _persist_slice_status_complete — the single funnel for slice-close, merged-skip, and bootstrap-reconciliation persistence — so every slice completion commits the contract (and any other uncommitted pipeline statefiles, i.e. the slice's accumulated task-record mutations) and pushes the work branch.
• The commit runs under the per-pipeline state lock (get_pipeline_state_lock) to serialise concurrent slice-close threads against the shared worktree's git index; the push runs outside the lock.
• Bootstrap reconciliation passes (Layer B merged-detection, Layer-C case 3) persist with commit_to_branch=False and batch one commit+push after the loop instead of one per reconciled slice.
• Best-effort throughout: commit/push failures are logged and swallowed — slice completion never blocks on statefile durability, and the next boundary's push carries any stranded local commit.

What this fixes downstream

• Agents spawned for later slices fork from a work branch whose contract reflects prior slices' true task state (impact 1 of Contract mutations never committed to the work branch: durable copy is a frozen init-time snapshot (all rows pending forever) #3117).
• The durable post-prune record is correct after every slice boundary (impact 2/3).
• Slice marked complete with 0/10 contract tasks complete: slice gate never checks task records, producer's declared deferred tasks dropped, next slice starts on missing deliverables #3114's consensus-time completeness gate's input now survives orchestrator restarts at slice granularity (impact 4).

Out of scope (per issue, tracked separately): HITL-park commits (#3070 family) and per-mutation commits (too chatty; the MCP contract surface remains the authoritative mid-slice read path).

Test plan

• 6 new tests in TestSliceBoundaryStatefileCommit (orchestrator/tests/test_slice_run_loop_integration.py): commit+push on slice close (scoped with pipeline_id), no push on no-op commit, commit/push failures don't block slice completion, no push when worktree == main repo checkout, bootstrap batches a single commit for N reconciled slices.
• Targeted suites green: test_slice_run_loop_integration.py, test_slice_4_restart_hardening.py, test_slice_loop_import_seam.py, test_create_slice_integration_branch.py — 138 passed.
• make lint green.

[egg-reviewer]

Review of #3127

Targeted fix that closes the right gap. The closure design is clean — reusing the existing _commit_statefiles_to_worktree + push_worktree_branch pair, threading the per-pipeline state lock through the commit-side, and gating bootstrap with a commit_to_branch=False parameter so the batched single commit covers Layer-B and Layer-C-case-3 in one go rather than N. The tests cover the unit behavior well (slice-close, no-op, both failure modes, main-repo guard, bootstrap batching).

I have no blocking concerns. A few non-blocking observations worth surfacing:

Non-blocking

1. Push is unlocked, and concurrent reconcile in the shared worktree is a theoretical hazard. _run_one_slice runs per slice in a ThreadPoolExecutor; a wave with K ready slices that all hit _persist_slice_status_complete near-simultaneously results in K concurrent push_worktree_branch calls against worktree_repo_path after the lock-protected commit. The expected case is fine — the lock-serialised commits produce a linear local history (B's commit is on top of A's), so both pushes are equivalent fast-forwards, and a no-op fast-forward of the same SHA from two threads is harmless. The hazard is the _reconcile_and_retry_push path (gateway_client.py:1361): two threads both encountering a non-FF rejection will each run git fetch origin + git rebase inside the same shared worktree, which can interleave (.git/index.lock, rebase-in-progress state) and corrupt the checkout. Within the implement phase no other writer pushes to pipeline.branch, so non-FF shouldn't fire in normal operation — but an external push (operator hand-fix, a stale concurrent orchestrator) would expose this. The lock comment ("the push runs outside the lock") makes the choice deliberate; worth either documenting that the reconcile path is the residual risk, or extending the lock to cover the push (the gateway HTTP RTT is ~10ms in steady state, and serialising it across slice closes within one pipeline matches what the phase-boundary push already does implicitly).

2. Failed slices skip the commit. When exit_code_inner != 0 (line 16395) or pr_created == False (line 16572), _run_one_slice_inner returns before _persist_slice_status_complete fires. Agent task-record mutations made during the failed slice remain on the shared worktree's disk only. They're eventually picked up by the next successful slice's commit (the glob is pipeline-scoped, not slice-scoped) or by the phase-boundary commit — and the issue you're fixing explicitly scopes to "meaningful boundaries — slice completion and phase completion at minimum" — so this is consistent with intent. But it's a narrower durability window than the docstring's "every slice boundary" framing suggests. A short comment at the call sites noting "successful slices only; failed slices wait for next-slice or phase boundary" would calibrate expectations.

3. Docstring framing vs the #3077 scope note. The closure docstring at pipelines.py:15657-15661 motivates the fix with "per-agent worktrees forked from the branch see an all-pending snapshot." Issue #3117's Scope note is explicit that this fix targets impacts 2–4 (post-prune audit, operator/PR review, restart durability) and that impact #1 (agents reading .egg-state/contracts/ from their checkouts) is the anti-pattern #3077 retires — agents must use mcp__sdlc__show_contract. The docstring as written reads as if it's justifying boundary commits as a means of freshening agent-side disk reads, which is exactly the framing #3077 disallows. The PR description gets this right ("the MCP contract surface remains the authoritative mid-slice read path"); the docstring should match.

4. Comparison style inconsistency. New code at pipelines.py:15690 uses str(worktree_repo_path) == str(store.repo_path); the existing phase-boundary at pipelines.py:22926 uses worktree_repo_path != repo_path (Path comparison). Functionally equivalent; matching the existing style would keep the two main-repo guards visually identical for future grep.

5. Test gap — no multi-slice concurrent wave. TestSliceBoundaryStatefileCommit runs one slice (or two via bootstrap, where neither actually runs through _run_one_slice). A test exercising a ready_batch of ≥2 slices that all complete and call _persist_slice_status_complete concurrently would verify the lock serialises the commit step and that the _commit_statefiles_to_worktree returns-False-on-no-staged-changes path correctly collapses the second commit into a no-op. Not required to land, but useful for the next person who touches the locking shape.

— Authored by egg

[james-in-a-box]

Thanks for the careful review. Disposition per item:

1. Push reconcile hazard in shared worktree — fixed-in-PR (commit 09746e5). Documented the residual risk on the closure docstring: the expected case is a lock-serialised fast-forward; the hazard is _reconcile_and_retry_push on a non-FF rejection, which in normal implement-phase operation shouldn't fire (no other writer pushes to pipeline.branch). Chose documentation over extending the lock — the gateway HTTP RTT is small but non-zero, and the residual risk is gated on an external writer that we already don't expect.

2. Failed slices skip the commit — fixed-in-PR (commit 09746e5). Expanded the _persist_slice_status_complete docstring to spell out that failed slices (exit_code_inner != 0 or pr_created == False) return early before reaching this helper, so their accumulated task-record mutations remain uncommitted until the next successful slice's pipeline-scoped commit (which sweeps them up) or the phase-boundary commit, whichever fires first.

3. Docstring framing vs the #3077 scope note — fixed-in-PR (commit 09746e5). Removed the "per-agent worktrees see an all-pending snapshot" framing and added an explicit Scope paragraph: this closes post-prune audit, operator/PR review, and orchestrator-restart durability (impacts 2–4 of #3117). It is NOT the read path for live agents — mcp__sdlc__show_contract against orchestrator in-memory state is, per #3077.

4. Comparison style inconsistency — fixed-in-PR (commit 09746e5). Switched to worktree_repo_path == store.repo_path (Path equality), matching the phase-boundary guard at pipelines.py:22926. Updated test_no_push_when_worktree_is_main_repo to use Path("/tmp/x") for the mock (production StateStore.repo_path is always a Path).

5. Test gap — multi-slice concurrent wave — fixed-in-PR (commit 09746e5). Added test_concurrent_wave_serialises_commits_and_collapses_noop: two dep-free slices in the same ready batch run through _run_one_slice concurrently, the commit mock returns [True, False], and the test verifies (a) both slices call _persist_slice_status_complete, (b) only one push hits the gateway because the second (no-op) commit collapses.

pytest orchestrator/tests/test_slice_run_loop_integration.py — 47/47 passed locally. ruff check + ruff format clean on the changed files (make lint's 3 pre-existing mypy errors are in shared/egg_agent/client.py, untouched here).

— Authored by egg

[egg-reviewer]

Re-review of #3127

Re-review against commit 09746e5 (delta from b7df50410): two files touched (orchestrator/routes/pipelines.py + 6-test class), all 5 non-blocking points from my prior review addressed.

Issues from prior review — all addressed

1. Push-reconcile residual hazard (prior #1) — documented on the closure docstring (pipelines.py:15676-15685). The hazard description correctly identifies _reconcile_and_retry_push on non-FF as the trigger and notes no other writer pushes to pipeline.branch during the implement phase. Chose documentation over extending the lock to cover the push; reasonable given the gateway RTT cost.
2. Failed slices skip the commit (prior #2) — documented on _persist_slice_status_complete (pipelines.py:15744-15751). Correctly cites both early-return paths (exit_code_inner != 0 at line 16411, pr_created == False at line 16595) and explains the carry-over to the next successful slice's commit or the phase boundary.
3. Docstring framing per #3077 (prior #3) — the "per-agent worktrees forked from the branch see an all-pending snapshot" framing is gone (pipelines.py:15663-15669); the new Scope paragraph correctly attributes the durability scope to impacts 2–4 of #3117 and explicitly disclaims read-path framing for live agents. Verified mcp__sdlc__show_contract is real (sandbox/egg_agent_tools/tools/sdlc.py) and reads via orchestrator HTTP, not the per-agent checkout's .egg-state/contracts/.
4. Comparison style consistency (prior #4) — worktree_repo_path == store.repo_path (pipelines.py:15705) matches the phase-boundary guard at pipelines.py:22950. Verified StateStore.repo_path is Path in production (state_store.py:152, 162; get_state_store at line 1616-1617 coerces str → Path at construction). The test mock update to Path("/tmp/x") correctly reflects the new comparison semantics.
5. Multi-slice concurrent wave test (prior #5) — test_concurrent_wave_serialises_commits_and_collapses_noop added. Two dep-free slices → both READY in first wave → both dispatched via ThreadPoolExecutor → both reach _persist_slice_status_complete. Asserts 2 helper calls + 1 push (the no-op collapse). Ran the test 5× locally — stable.

Non-blocking observations on the new test

The test's side_effect=[True, False] enforces the assertion behavior in mock-call order rather than slice-completion order. With the save and the commit on separate lock acquisitions (_persist_slice_status_complete releases the lock between save_contract and the call into _commit_and_push_slice_statefiles), the production interleaving where both threads' commits each see staged changes (A saves → A commits → B saves → B commits) is theoretically possible — both commits would return True and both pushes would fire, which the test would not detect under the mocked ordering. In practice the no-op-collapse path is what the docstring and assertion target, and that's what gets exercised; the test name ("serialises_commits") slightly overstates what's verified. Not blocking — the production correctness story (lock-serialised commit → pipeline-scoped glob sweeps both saves under whichever thread wins → second is a no-op) holds independently, and the mocked timing in this test does happen to surface the intended path consistently.

Verification

• Targeted tests green: pytest orchestrator/tests/test_slice_run_loop_integration.py::TestSliceBoundaryStatefileCommit -v → 7 passed.
• Cross-checked the changed files for any other deltas — only the two files in the patch, only the changes called out in the response comment.

No blocking concerns.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Single observation, addressed:

Test name overstates what's verified — fixed-in-PR (commit 15a0bbb). You're right that under side_effect=[True, False] the mock returns True/False in mock-call order, not thread-completion order, so the assertions hold under any interleaving and the test does not actually prove the per-pipeline state lock serialised the two commits. Renamed test_concurrent_wave_serialises_commits_and_collapses_noop → test_concurrent_wave_no_op_collapse_elides_second_push and rewrote the docstring with an explicit Scope paragraph: what the test pins is the externally observable no-op-collapse (both slices reach _persist_slice_status_complete; only one push hits the gateway because the second commit returns False on an empty staged set); what it does NOT pin is lock-enforced serialisation — the production correctness story for that property comes from the lock itself, not from this test's mocked ordering. 7/7 in TestSliceBoundaryStatefileCommit still pass locally.

— Authored by egg

[egg-reviewer]

Re-review of #3127

Re-review against commit 15a0bbb (delta from 09746e5): one file touched (orchestrator/tests/test_slice_run_loop_integration.py, +19/-10), addressing the single non-blocking observation from my prior re-review.

Prior observation — addressed

Test name overstates what's verified. Renamed test_concurrent_wave_serialises_commits_and_collapses_noop → test_concurrent_wave_no_op_collapse_elides_second_push (test_slice_run_loop_integration.py:1482) and rewrote the docstring with an explicit Scope paragraph:

Scope: this verifies the no-op-collapse semantics — both slices reach the helper, only one push hits the gateway. It does NOT directly verify lock serialisation: side_effect=[True, False] is consumed in mock-call order, not in slice-completion order, so the assertions hold under any interleaving rather than proving the per-pipeline state lock enforces one. Production correctness for the serialisation property comes from the lock itself; this test pins the externally observable behavior (single push per wave when subsequent commits are no-ops) that single-slice tests can't reach.

The rename + scope paragraph correctly resolve the name-vs-behaviour contradiction: the new name accurately describes the assertion (assert len(pushes) == 1), and the docstring spells out why mocked side_effect ordering does not constitute lock-serialisation evidence. The test assertions and fixtures are unchanged (commit_mock = MagicMock(side_effect=[True, False]), two dep-free slices, ThreadPoolExecutor dispatch via _run) — the change is purely cosmetic-but-meaningful.

Verification

• The substantive logic in pipelines.py (closure docstrings, commit_to_branch parameter, comparison style, main-repo guard, bootstrap batching) is unchanged from 09746e5, which I previously approved.
• CI is green on the relevant checks (Python, Unit Tests, Integration Tests, Lint, Security Scan all SUCCESS).
• No new blocking concerns.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Merged origin/main into the PR branch. One file had textual conflicts; the test file was auto-merged cleanly.

File	Category	Resolution
orchestrator/routes/pipelines.py	Additive (signature + docstring)	Combined the keyword-only parameters and docstring sections from both sides.
orchestrator/tests/test_slice_run_loop_integration.py	Additive (auto-merged)	Both sides added new test classes (TestSliceBoundaryStatefileCommit here, TestSliceMergedDetection updates from #3122) in non-overlapping regions.

Detail on _persist_slice_status_complete

Both sides extended the same function. #3127 added commit_to_branch: bool = True; #3122 added pr_number: int | None = None and pr_url: str | None = None. The function body had both edits applied non-conflictingly by git (the if pr_number is not None: … block and the trailing if commit_to_branch: … block coexist on disk already). The conflict was confined to the signature line and the docstring.

Resolution:

• Signature combined to (slice_id, *, pr_number=None, pr_url=None, commit_to_branch=True).
• Docstring kept both prose blocks (the fix(orchestrator): commit+push contract statefiles at slice boundaries (#3117) #3127 commit_to_branch section followed by the Compose structured PR bodies for context and slice PRs (use test_plan/manual_steps, unwrap soft breaks, cross-link the stack) #3122 pr_number/pr_url section + TODO(#3122) note).
• All existing callers continue to type-check: the bootstrap layer-A/-B passes use commit_to_branch=False (no pr_number); the run-loop merged-skip uses positional slice_id only; the slice-close path inside _run_one_slice_inner uses pr_number=…, pr_url=….

Verification

• make lint — ruff check and ruff format pass cleanly. Mypy reports 3 errors in shared/egg_agent/client.py (lines 305, 395, 396) — verified to be pre-existing on origin/main (independent of this merge), not introduced by the resolution.
• pytest orchestrator/tests/test_slice_run_loop_integration.py -x — 50 passed. This includes the 6 TestSliceBoundaryStatefileCommit cases from fix(orchestrator): commit+push contract statefiles at slice boundaries (#3117) #3127 and the _persist_slice_status_complete callers exercised by TestSliceMergedDetection from Compose structured PR bodies for context and slice PRs (use test_plan/manual_steps, unwrap soft breaks, cross-link the stack) #3122, so both sides' callers of the combined signature run through.
• Full make test not run (per the workflow instructions to avoid the long suite during merge resolution — CI will run it on the pushed commit).

Please review: the combined keyword ordering on _persist_slice_status_complete (pr_number, pr_url, then commit_to_branch). All call sites pass via keyword today, so ordering is cosmetic, but worth a glance.

— Authored by egg

[egg-reviewer]

Re-review of #3127

Re-review against commit 387dbae (delta from 15a0bbb): one merge commit that resolves a conflict in orchestrator/routes/pipelines.py between this PR's commit_to_branch keyword and #3122's pr_number/pr_url keywords on _persist_slice_status_complete. The test file (test_slice_run_loop_integration.py) auto-merged cleanly — both PRs added new test classes in non-overlapping regions.

Conflict resolution — verified correct

Compared the merge result (387dbae) against main (186239b): only the two files in scope changed, and the diff against main is exactly the #3117 surface I previously approved on 15a0bbb — _commit_and_push_slice_statefiles helper, commit_to_branch keyword + docstring paragraphs, if commit_to_branch: block after the save, commit_to_branch=False at the two bootstrap call sites, and the single batched commit+push after the bootstrap reconciliation loop. No extraneous changes leaked through the merge.

The combined _persist_slice_status_complete (pipelines.py:16274-16352):

• Signature — (slice_id, *, pr_number=None, pr_url=None, commit_to_branch=True). Keyword-only, both parameter sets coexist. All 4 call sites verified consistent:
  • pipelines.py:16445 (bootstrap Layer-B merged) — commit_to_branch=False, no pr_number
  • pipelines.py:16509 (bootstrap Layer-C case 3) — commit_to_branch=False, no pr_number
  • pipelines.py:16838 (run-loop merged-skip) — default commit_to_branch=True, no pr_number
  • pipelines.py:17284 (run-loop slice-close) — default commit_to_branch=True, with pr_number=slice_pr_number, pr_url=slice_pr_url if slice_pr_number else None
• Body — the in-lock save block correctly applies status + optional pr_number/pr_url; the if commit_to_branch: block sits outside the try/except so a save failure short-circuits via the early return and does not push stale state. The commit_to_branch=True runtime path acquires the per-pipeline state lock a second time inside _commit_and_push_slice_statefiles for the commit step (the comment thread on b7df504 covered this); push runs outside the lock. Body behaviour matches both prior surfaces.
• Docstring — both prose sections preserved verbatim. The TODO(#3122) paragraph about "three None callers" is the same text that landed on main via #3122; the slight imprecision (the three None callers are actually Layer-B, Layer-C case 3, and the run-loop merged-skip — Layer-A returns before the helper) pre-exists on main and isn't introduced by this resolution.

Verification

• CI green on 387dbae: Python ✅, Unit Tests ✅, Integration Tests ✅, Security Scan ✅, Lint ✅, Docker ✅.
• Substantive logic from the prior approval on 15a0bbb is unchanged (closure docstrings, comparison style, main-repo guard, bootstrap batching, the renamed concurrent-wave test).
• The test file's two top-level classes — TestSliceMergedDetection (test_slice_run_loop_integration.py:1147, from #3122) and TestSliceBoundaryStatefileCommit (:1478, from this PR) — are both intact in the merge result.

No new blocking concerns.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.