fix(orchestrator): preserve committed contract decisions when post-phase push fails (#2972) - #2980
Conversation
…ase push fails (#2972) When a phase's post-completion `_sync_worktree_with_remote` found the worktree local-ahead with `remote_ahead == 0` and the push to origin failed, it fell through to `git reset --hard origin/<branch>` and returned `reset_succeeded` (`hard_reset_performed=False`) — silently discarding the just-committed `.egg-state/contracts/<id>.json` decision commit before the phase_gate decision bridge could surface it. This dropped a refiner's `register_open_question` / `request_feedback` items in pipeline-8cf1f000: the refine boundary hit `local_ahead=1, remote_ahead=0` + a push failure (git credential error) → silent reset, so Wave 2 never surfaced and the pipeline advanced to plan with `decisions: []`. The same divergence at the plan boundary (`remote_ahead=5`) correctly went loud via `divergence_recovered_via_reset` — the contrast pinpoints the gap. Fix: when the push fails and `remote_ahead == 0`, origin holds nothing to incorporate, so return `local_ahead_push_failed` without resetting, preserving the committed work in the worktree for the bridge and the next push attempt. The #2488 "commit before sync" guard only protected the rebase path (local-behind → diverged → rebase); this closes the push-failed reset gap it left. Tests: flip the sync taxonomy + push-failed unit tests to assert no-reset, and add a real-git regression in test_contract_preserved_across_post_phase_sync.py that exercises a failed push with a committed decision and asserts HEAD/decision survive.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #2980 — preserve committed contract decisions when post-phase push fails
Thorough review of orchestrator/routes/pipelines.py and the two test files. End-to-end trace of the fix against the post-phase flow in _run_pipeline (around line 22298–23031).
Verdict: Approve with one non-blocking doc fix
The fix is correct, minimal, and well-scoped to the silent-data-loss path described in #2972. The root-cause narrative against pipeline-8cf1f000 is accurate, the asymmetry it identifies (loud divergence_recovered_via_reset HITL vs. silent reset_succeeded after push-fail) is real, and the chosen remedy (return early without resetting when remote_ahead == 0 and prior_phase_succeeded) is the right disposition: resetting a committed local tip to an origin tip it is already strictly ahead of can only lose data — there is zero reconcile benefit.
End-to-end trace I verified
- Pre-sync commit (
_commit_statefiles_to_worktree, line 22310) captures the agent's contract mutation as a real commit (the #2488 step). _sync_worktree_with_remotenow returnslocal_ahead_push_failedinstead of falling through togit reset --hard origin/<branch>.- Downstream consumers in the post-phase flow all read from the worktree on disk (
_sync_pipeline_decisions_to_contract→_write_brc_history→_commit_statefiles_to_worktree→ HITL gate →_queue_and_await_contract_decisions); HEAD is preserved, so each of these now sees the committed decisions. _queue_and_await_contract_decisions(line 20037) callsload_contract(pipeline_identifier, worktree_repo_path)— reads from disk, so the bridge picks up the decisions and surfaces them as Wave 2 HITL questions. The fix closes the data-loss loop.
Caller compatibility
WorktreeSyncOutcome.case is never inspected by production callers — only hard_reset_performed. The new local_ahead_push_failed case returns hard_reset_performed=False, matching what the old fall-through-then-reset path returned (reset_succeeded with hard_reset_performed=False), so the phase-boundary HITL emission at line 22409 and the phase-start emission at line 20991 stay quiescent for this case — consistent with the prior behavior. No breakage at any call site.
Test quality
test_push_fails_preserves_local_without_reset(test_sync_worktree.py:167) — call-count and reset-argv assertion correctly proves the step-4 reset doesn't run. Mock-based; tight.test_case_local_ahead_push_failed_preserves_without_reset(test_sync_worktree.py:601) — outcome-taxonomy update; verifies the log sequence drops the trailingreset_succeeded.test_push_failure_preserves_committed_decisions_without_reset(test_contract_preserved_across_post_phase_sync.py:393) — real-git regression. Exercises the production helper, asserts HEAD is unchanged, contract body byte-equals expected,decision-1is present, outcome shape correct. Independently authored expectations, no self-seeding. Solid.
Non-blocking — stale docstring
orchestrator/routes/pipelines.py:7067–7071 (NOT touched by this PR but rendered incorrect by it):
Paths that fall through to the step-4 reset
(``local_ahead_push_failed``, ``local_ahead_discarded``,
``rev_list_failed``) emit a sequence — first a discriminator naming
WHY we fell through, then the terminal ``reset_succeeded`` /
``reset_failed`` event.
After this PR local_ahead_push_failed no longer falls through to the step-4 reset — it emits exactly one log line and returns. The new outcome-taxonomy test (line 623: _outcome_cases(...) == ["local_ahead_push_failed"]) is the authoritative shape; the docstring should be updated to match, e.g. move local_ahead_push_failed out of the fall-through list and note it's a single-log-line terminal outcome. This is the kind of "I read the docstring and traced the code; they disagree" trap I'd want to spare the next debugger.
While there, consider also revising line 7037: "On a successful push, reset to origin (a no-op fast-forward that keeps the worktree clean)" — the success path at line 7244–7262 doesn't actually run a reset, it just re-fetches and returns. That phrasing predates this PR but is now in fresh context, so it's the natural moment to correct it.
Operational note (non-blocking)
The push-failure path is now non-destructive, but the only operator signal is a single WARN log line — quieter than the OVERSEER_ALERT + HITL ack on the symmetric divergence path. That asymmetry is defensible (the next phase-boundary's sync will retry the push, so the failure is recoverable in a way that rebase-failure is not), and the alternative is strictly worse than the bug it closes, so I'd land this as-is. But for a follow-up: if persistent push failures accumulate local commits across many phases without any HITL surfacing, an operator skimming only ERROR-level logs would miss it. A bounded counter ("nth consecutive push-failed sync on this pipeline → escalate to OVERSEER_ALERT") would close the asymmetry without re-introducing data loss.
Out-of-scope follow-ups acknowledged in the PR
The PR correctly notes the trigger (gateway credential failure) is a separate concern, and that the rev_list_failed Step-4 fall-through can also reset away local-ahead work but isn't fixable in the same way (the branch can't know local_ahead is > 0 — that's what failed). Both are tracked appropriately as follow-ups.
— Authored by egg
|
egg review completed. View run logs |
…) (#2986) * fix(orchestrator): non-destructive worktree-divergence reconcile (#2979) Stop reconciling sync divergence destructively and stop failing the whole pipeline post-consensus. Prevents the self-inflicted divergence at the source and pauses (instead of discarding work + FAILing) on the rare residual case. Source prevention (the structural fix): agents can no longer git-push `.egg-state/contracts/*` in refine/plan (gateway/phase_filter.py + .egg/phase-permissions.json). The orchestrator is the sole writer of contracts — agents mutate them through the contract API, never git — so removing the push permission makes the agent-pushed stale contract that drove the divergence structurally impossible. The post-phase rebase then only ever replays disjoint paths (orchestrator contracts/brc vs agent drafts/outputs) and reconciles cleanly. An explicit contract push now gets a 403-with-hint instead of triggering a destructive reset. Non-destructive reconcile: when the rebase autoresolve can't reconcile a divergence it has already aborted back to the clean local HEAD (the autostash is reapplied), so the orchestrator's committed work is intact. `_sync_worktree_with_remote` now leaves the worktree there, pins a backup ref as a stable operator handle, and reports `diverged_unreconciled` instead of `git reset --hard origin`. The `rev_list_failed` fall-through that reset with an unknown ahead-count (no backup) also bails non-destructively. The Step-4 reset now only runs for local-behind / prior-phase-failed-discard, neither of which can lose un-pushed work. Pause, don't FAIL: the two in-loop phase-boundary callers set AWAITING_HUMAN and block on a reconcile HITL (the proven phase-gate pause), then re-run the sync and resume the phase's post-processing inline once the operator acks — no full re-run, no re-divergence loop. The non-blocking populate_contract route returns HTTP 409 (`divergence_reconcile_unacked`) + AWAITING_HUMAN for the operator to re-run after reconciling. Retires the #2792/#2797 destructive machinery this replaces: the hard-reset recovery HITL + FAILED helper, the `hard_reset_recovery:` decision dispatch hook, and the restart_phase-based resume/abort helpers. README worktree-sync contract reconciled to the landed behavior. Relationship: composes with #2980 (#2972), which makes the adjacent push-failed branch non-destructive — different hunks of the same helper. * Fix checks: update post-BRC sync wrap test for new helper name #2979 routed the post-BRC worktree-sync through the new _sync_worktree_reconciling_divergence wrapper but test_sync_worktree_with_remote_is_wrapped still searched for the old direct _sync_worktree_with_remote call name. The try/except invariant from #2219 is still satisfied — the regex just needed updating for the new helper and the tuple-unpack assignment form. * Address review feedback: stale comment, HITL dedupe, exception revert Addresses the three non-blocking notes from the egg-reviewer review on #2986: 1. Stale rationale comment at pipelines.py:22205 — replaced the git reset --hard would revert justification with the actual modern reason: the autoresolve rebase needs to land remote state before populate reads .egg-state/. The destructive reset is gone after #2979, so the comment now matches the code. 2. populate_contract dedupes the reconcile HITL — set a stable context='divergence_reconcile_unacked' on emitted reconcile HITLs, and have the non-blocking route early-return 409 referencing the existing decision when a prior populate already paused the pipeline on a still-unacked reconcile HITL. Keeps the route idempotent under operator retries (e.g. /sdlc refresh before resolving, automated retry loop) so pipeline.decisions does not accumulate duplicates. 3. _sync_worktree_reconciling_divergence reverts on mid-pause error — wrapped the post-AWAITING_HUMAN wait-and-resolve span in a try/except. An unexpected exception (broadcast IO error, decision-queue runtime error, transient get_decision failure) now reverts the on-disk pipeline status from AWAITING_HUMAN back to RUNNING before re-raising, so the outer _run_pipeline try/except doesn't strand the pipeline on a never-acked HITL with no waiter. The abort path (operator chose Abort pipeline) still returns normally with aborted=True and is unaffected. Tests: - TestEmitDivergenceReconcileHitl now asserts context is set. - New test: populate_contract early-returns 409 when a pending reconcile HITL already exists, without re-running sync or re-emit. - New test: a wait_for_decision exception inside _sync_worktree_reconciling_divergence reverts AWAITING_HUMAN to RUNNING before propagating. All 27 tests in test_hard_reset_recovery.py pass. * Fix stale test comment contradicting new context assertion The comment block at TestEmitDivergenceReconcileHitl said 'no dispatch context — resume is manual re-run for the route path', but the new assertion three lines below checks that context == 'divergence_reconcile_unacked'. Merged the two comment blocks into one that covers both the question/options and the context, so a reader no longer has to reconcile the contradiction. --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
Summary
Closes the silent data-loss path behind #2972: refiner-registered HITL decisions (
register_open_question/request_feedback) were dropped before the phase_gate decision bridge could surface them, so the operator never got Wave 2 and the pipeline advanced straight toplan.Two-wave surfacing was never broken —
_queue_and_await_contract_decisions()is implemented and is called inline right after the phase_gateapprove, before the phase advances. It just read an empty contract, because the decisions had already been wiped out of the shared worktree by the post-phase worktree sync.Root cause (confirmed against
pipeline-8cf1f000)The decisions were persisted to the shared worktree (the reviewer confirmed them mid-refine) and were committed before the sync by the #2488 "commit before sync" guard. They were then discarded by
_sync_worktree_with_remote:local_ahead=1, remote_ahead=0, and the push to origin failed (reconcile_fetch_failed—fatal: could not read Username for 'https://github.com'). The helper fell through to the Step-4git reset --hard origin/<branch>and returnedreset_succeededwithhard_reset_performed=False→ no operator signal, decisions gone, advance toplan.remote_ahead=5) routed throughdivergence_recovered_via_reset→hard_reset_performed=True→ a loud HITL ack (decision-2,hard_reset_recovery:plan).That contrast pinpoints the gap: the divergence-rebase-failed reset is loud, but the push-failed / local-ahead reset is silent, even though both discard committed work.
The #2488 guard only protected the rebase path (it turns local-behind into diverged → rebase). When
remote_ahead == 0and the push fails, origin has nothing the worktree lacks, so the reset is pure data loss for zero reconcile benefit.Fix
In
_sync_worktree_with_remote, when the prior phase succeeded,local_ahead > 0,remote_ahead == 0, and the push fails: returnlocal_ahead_push_failedwithout resetting. The committed local commits stay in the worktree for downstream reads (the decision bridge, populator) and the next push attempt; the existingWARNINGlog remains the breadcrumb that the tip is unpushed. The intended-discard case (prior phase failed) still falls through to the Step-4 reset.This is non-destructive and matches the codebase's "fail loud, don't silently destroy" posture (#2337 / #2627 / #2792).
Tests
test_sync_worktree.py: fliptest_push_fails_*and the outcome-taxonomy test to assert no reset on push failure (mock_run.call_count == 3, outcomelocal_ahead_push_failed,hard_reset_performed is False).test_contract_preserved_across_post_phase_sync.py: add a real-git regression (test_push_failure_preserves_committed_decisions_without_reset) that commits a decision, fails the push withremote_ahead == 0, and asserts the decision and HEAD survive. Module docstring updated to record that the push-failed path is now fixed ([orchestrator/HITL] Refiner-registered decisions (register_open_question/request_feedback) never surface after phase_gate approve — pipeline skips Wave 2 and advances, silently bypassing operator input #2972) alongside the SDLC: agent-registered HITL decisions (register_open_question / request_feedback) don't surface to operator; phase advances silently past them #2488 pre-commit fix.make test(changeset-aware) green: 17508 passed, 0 failed.make lintclean.Out of scope / follow-ups
pipeline-8cf1f000was an environmental git-credential failure on the gateway push reconcile (could not read Username for https://github.com) plus an unresolvable PR base branch (jwbron-claude-md). That's a separate gateway/credential concern; this PR only stops the silent decision loss it caused.rev_list_failedStep-4 fall-through can also reset away local-ahead work, but it can't determinelocal_ahead(that's what failed), so it's left as-is — a rarer, separately-tracked gap.