fix(orchestrator): non-destructive worktree-divergence reconcile (#2979)

[jwbron]

Closes #2979.

Stops reconciling sync divergence destructively and stops failing the whole pipeline post-consensus. Prevents the self-inflicted divergence at the source, and pauses (instead of discarding committed work + FAILing) on the rare residual case — matching the issue's desired outcome.

This is the verified direction from the audit comment on the issue.

What changed

1. Source prevention — agents can't git-push contracts (the structural fix)

gateway/phase_filter.py + .egg/phase-permissions.json: removed .egg-state/contracts/* from the refine and plan allowlists. The orchestrator is the sole writer of contracts — agents mutate them through the /api/v1/contract/mutate API, never git. Permitting the push let a broad git add land a stale contract snapshot on origin that then conflicted with the orchestrator's authoritative contract commit at the post-phase sync. That was the only non-agent-outputs path both sides touched on the work branch, so it was the driver of the divergence.

With the push blocked, the post-phase rebase only ever replays disjoint paths (orchestrator contracts/brc-history vs agent drafts/outputs/reviews) and reconciles cleanly — making the destructive path structurally unreachable in normal operation. Safe because agents push specific paths (git add <files>, not git add -A) and no test relies on git-pushing contracts; an explicit contract push now gets a 403-with-hint instead of triggering a destructive reset.

2. Non-destructive reconcile (_sync_worktree_with_remote)

When the rebase autoresolve can't reconcile a divergence, it has already run git rebase --abort (reapplying the autostash), so the worktree is back at the clean local HEAD with the orchestrator's committed work intact. The helper now leaves it there, pins a refs/egg-backup/sync-recovery/<id>/<ts> ref as a stable operator handle, and returns a divergence_unreconciled outcome — instead of git reset --hard origin (which #2792/#2797 did, discarding work to a backup ref and FAILing the pipeline).

The rev_list_failed fall-through that reset with an unknown ahead-count and no backup ref now bails non-destructively too. The Step-4 reset is now only reached for local-behind / prior-phase-failed-discard, neither of which can lose un-pushed committed work. Invariant: the orchestrator never reset --hard over a commit not provably already on origin.

3. Pause, don't FAIL (+ inline resume)

The two in-loop phase-boundary callers (post-phase, phase-start) now set AWAITING_HUMAN and block on a reconcile HITL (the same proven pause primitive the plan-approval gate uses), 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. Bounded pause budget guards against a never-reconciled loop. The non-blocking populate_contract route returns HTTP 409 divergence_reconcile_unacked + AWAITING_HUMAN for the operator to re-run after reconciling.

4. Retired the destructive #2792/#2797 machinery this replaces

• the hard-reset recovery HITL (question/options/emission) + _fail_pipeline_and_emit_hard_reset_recovery
• the hard_reset_recovery:<phase> decision-resolution dispatch hook
• the restart_phase-based resume_pipeline_after_hard_reset_ack / abort_pipeline_after_hard_reset_ack helpers
• SyncRebaseAndResetFailedError (the reset is gone, so the doubly-failed case can't occur)

Net: pipelines.py shrinks (~600 lines of destructive machinery removed, ~370 of pause/resume added).

5. README

orchestrator/README.md worktree-sync contract reconciled to the landed non-destructive behavior (its line was stale — it still described the pre-#2792 "leave unchanged" behavior).

Testing

• make lint ✅
• Targeted suites ✅ (full make test-all left for CI): test_sync_worktree.py, test_hard_reset_recovery.py (rewritten for the new behavior), test_pipeline_failure_path.py, test_contract_preserved_across_post_phase_sync.py, test_decisions_routes.py, gateway test_phase_filter*.py, test_gateway.py — 572 passed.

New/updated coverage: refine/plan contract pushes are blocked (+ atomic mixed-push rejection); divergence-rebase-fail leaves the worktree intact (no reset) and reports divergence_unreconciled; rev_list_failed bails non-destructively; the reconcile HITL question/options/abort-detector; _emit_divergence_reconcile_hitl (AWAITING_HUMAN, not FAILED); _fail_pipeline_after_divergence_abort; and the _sync_worktree_reconciling_divergence pause→resume / abort / budget-exhausted loop.

Relationships

• Composes with fix(orchestrator): preserve committed contract decisions when post-phase push fails (#2972) #2980 ([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), which makes the adjacent push-failed branch of the same helper non-destructive — different hunks; a trivial merge if both land.
• [orchestrator/sync] Hard-reset recovery HITL ack is undispatchable — choice-envelope mismatch leaves pipeline wedged in failed_pending_hitl #2978 Slice A owns the HITL-ack envelope fix; this PR reconciles the README contract per the issue's "re-check it here once the reconcile design changes again."

Out of scope (follow-ups)

• APPLY-phase contract pushes left as-is (the Jira applier is a separate flow not analyzed here).
• Tier-2 "discard on assumption" sites flagged in the audit (_rebase_pipeline_branch_onto_base confused-HEAD, PR-open refresh) — verify-on-origin hardening is separate.

[james-in-a-box]

Merge conflict resolution — origin/main into egg/2979-non-destructive-sync-reconcile

Merged main (HEAD da30951d) into the PR branch via git merge (not rebase, per workflow). Two explicit conflicts plus four auto-merge semantic-drift fixes.

Explicit conflicts resolved

File	Resolution
orchestrator/README.md	Kept HEAD's new "Worktree Sync" wording — main's text described the old destructive behavior with a note that "a redesign is coming"; this PR is that redesign, so HEAD's description is the correct destination.
orchestrator/routes/decisions.py	Composed both PRs: kept #2978's _normalize_choice_resolution helper (still consumed by _handle_conditional_ack_gate and the resolve_decision boundary), and dropped main's _handle_hard_reset_recovery_resolution dispatch handler which #2979 removes. Refreshed the _handle_conditional_ack_gate docstring to drop the now-dead reference to the removed handler.

Auto-merge semantic drift fixed

Git's three-way merge happily textually merged main's additions into files this PR had rewritten — leaving tests and patterns that reference symbols the PR deleted. Cleaned up:

File	Drift
orchestrator/tests/test_hard_reset_recovery.py	Removed TestDispatchResolutionChoiceEnvelope class (from main) — tests the _handle_hard_reset_recovery_resolution function this PR deletes. Kept TestNormalizeChoiceResolution (#2978 helper survives).
orchestrator/tests/test_sync_worktree.py	Removed two assert outcome.hard_reset_performed is False lines — the field was removed from WorktreeSyncOutcome in HEAD.
orchestrator/tests/test_contract_preserved_across_post_phase_sync.py	Same hard_reset_performed cleanup.
shared/egg_restrictions/phase_patterns.py	Dropped .egg-state/contracts/* from refine & plan allowed_patterns so the sandbox-side mirror tracks the gateway. HEAD's .egg/phase-permissions.json already removed contracts from refine/plan (contracts mutate through /api/v1/contract/mutate API, not git, per #2979); main shipped this new mirror file with the old permissive entry.
shared/tests/test_phase_patterns.py	Flipped test_refine_allows_contracts → test_refine_blocks_contracts and added test_plan_blocks_contracts to match the new phase-layer policy.

Local validation

• ruff check + ruff format --check on all six resolved files — clean.
• pytest gateway/tests/test_phase_filter_restrictions.py shared/tests/test_phase_patterns.py — all 115 pass (including the TestPhaseLayerSharedMirrorParity drift guard).
• pytest orchestrator/tests/test_decisions_routes.py test_hard_reset_recovery.py test_conditional_ack_hitl_gate.py test_restart_agent.py — 190 pass.
• pytest orchestrator/tests/test_sync_worktree.py test_contract_preserved_across_post_phase_sync.py — 45 pass; 4 fail solely because the sandbox gateway blocks git init --bare. Sandbox-environmental, not semantic — CI will run unrestricted.

Per the workflow constraint, did not run the full make test suite (10-15 min, risks timing out the wrapper). All semantic checks for the conflict zone are green.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 2}

[egg-reviewer]

Verdict: Approve with notes

The non-destructive contract is well realized. The orchestrator can no longer reset --hard over committed work; it pauses on AWAITING_HUMAN (not FAILED), pins a backup ref, and blocks the _run_pipeline loop on the same proven wait_for_decision primitive the phase-approval gate uses. The 3-pause budget bounds operator loops. The gateway-side phase filter blocks .egg-state/contracts/* pushes in refine/plan so the divergence is prevented at the source — defense in depth.

Verified:

• The lock-spanning invariant is preserved everywhere — pipeline.status = AWAITING_HUMAN and the HITL persist happen under the same get_pipeline_state_lock(...) (reentrant) in both the in-loop wrapper at orchestrator/routes/pipelines.py:14388-14408 and the non-blocking _emit_divergence_reconcile_hitl path that populate_contract uses.
• Backup ref still pinned in the non-destructive branch — operators retain the offline-recovery handle they had under the destructive path.
• Resume path correctly flips back to RUNNING (pipeline + phase execution) before re-running the sync; a still-divergent worktree re-pauses rather than silently failing.
• The two _run_pipeline callers (phase-start at pipelines.py:20869 and post-phase at pipelines.py:22227) handle (outcome, aborted) correctly; post_phase_sync_outcome is initialized to None before the try at :22209 so the abort check at :22253 can't AttributeError on the exception path.
• _divergence_reconcile_is_abort defaults to "resume" on ambiguous resolutions — the correct conservative default now that resume is non-destructive. JSON-envelope unwrap ({"action": "select", "selected": ...}) handled.
• Test coverage is comprehensive: divergence pause / resume / abort, budget exhaustion, JSON envelope, populate_contract surfacing. The test_sync_worktree.py assertions explicitly check no reset --hard is issued after a rebase fail.
• All CI green (Python, Unit Tests, Integration Tests, Security Scan, Docker, full lint suite).

Notes (non-blocking)

1. Stale rationale comment in the post-phase sync block. orchestrator/routes/pipelines.py:22205 still reads:

"This must run BEFORE _populate_contract_from_plan and _sync_pipeline_decisions_to_contract — otherwise git reset --hard in _sync_worktree_with_remote would revert their on-disk modifications."

After this PR _sync_worktree_with_remote no longer issues git reset --hard on divergence — the rationale ("the hard reset would revert their modifications") is stale. The ordering is still correct (the sync still applies remote state via the autoresolve rebase), but the comment now misrepresents why. Worth a one-line cleanup in a follow-up: replace the git reset --hard clause with the actual current reason (the autoresolve rebase needs to land remote state before the populate reads .egg-state/).

2. populate_contract can accumulate duplicate divergence HITLs. _emit_divergence_reconcile_hitl → _persist_hitl_decision → pipeline.add_decision() at orchestrator/models.py:1239 unconditionally appends; there is no dedupe on context. If the operator re-POSTs /phase/populate-contract against an already-paused pipeline (e.g. via the /sdlc UI without resolving the prior HITL, or an automated retry), each call emits a fresh pending decision. The route itself doesn't guard against this — it re-runs the sync and re-emits unconditionally. UX nit (not a correctness bug; the abort path still works on the most recent decision), but consider either dedup-by-context in add_decision, or having populate_contract early-return 409 when a divergence_reconcile_unacked decision is already pending. Low-priority follow-up.

3. Mid-pause exception leaves pipeline AWAITING_HUMAN with no waiter. Theoretical edge case in _sync_worktree_reconciling_divergence: if an exception surfaces between the AWAITING_HUMAN write and wait_for_decision (e.g. inside dq.wait_for_decision itself, or in report_pipeline_status), the post-phase caller's outer try/except at :22239 catches and continues, but the on-disk pipeline status remains AWAITING_HUMAN while the orchestrator thread moves on to the next phase. The exception path is narrow (the lock-spanning save already succeeded), but if you want to harden, a try/finally around the post-flip-to-AWAITING_HUMAN section that resets to RUNNING on unexpected error would close it. Not blocking.

— Authored by egg

[james-in-a-box]

Feedback addressed

Pushed ab65188 addressing all three non-blocking notes from the review in-PR rather than deferring to follow-ups — each was small enough to bundle cleanly.

• Note 1 — Stale git reset --hard rationale comment at orchestrator/routes/pipelines.py:22205 — fixed-in-PR (commit ab65188). Replaced the now-stale "the hard reset would revert their modifications" clause with the modern rationale: the autoresolve rebase inside _sync_worktree_with_remote needs to land remote state before the populate step reads .egg-state/, otherwise populate reads a stale local view and either produces an empty contract or overwrites agent-pushed drafts that only exist on origin. The historical reset-clause is kept as a parenthetical so the why-it-once-read-this-way is still discoverable.

• Note 2 — populate_contract can accumulate duplicate divergence HITLs — fixed-in-PR (commit ab65188). Went with the route-side early-return rather than dedup-in-add_decision (the model-side dedupe would either silently swallow the second add — surprising — or branch on context — invasive). Specifically:

  • Added _DIVERGENCE_RECONCILE_HITL_CONTEXT = "divergence_reconcile_unacked" and threaded it through both _emit_divergence_reconcile_hitl (the non-blocking route path) and the in-loop _sync_worktree_reconciling_divergence (consistency).
  • Added _find_pending_divergence_reconcile_decision(pipeline) — scans pipeline.get_pending_decisions() for the canonical context.
  • populate_contract now early-returns 409 with reason="divergence_reconcile_unacked", details.already_paused=True, and details.decision_id=<existing> when a prior call already paused the pipeline on an unacked reconcile HITL. No re-sync, no re-emit, no populate.
  • Test: test_already_paused_returns_409_without_resyncing_or_re_emitting.
  • The existing TestEmitDivergenceReconcileHitl assertion that context is None was updated to assert the new canonical context string.

• Note 3 — Mid-pause exception leaves pipeline AWAITING_HUMAN with no waiter — fixed-in-PR (commit ab65188). Wrapped the post-AWAITING_HUMAN section (broadcast → wait_for_decision → get_decision → resume-write) in a try/except. On unexpected exception, the helper reverts the on-disk pipeline + phase status from AWAITING_HUMAN back to RUNNING (guarded — only if still pinned) before re-raising, so the outer _run_pipeline try/except no longer strands the pipeline on a never-acked HITL. Secondary errors from the revert itself are swallowed (log + continue) so the original failure isn't masked. The abort path (operator chose Abort pipeline) returns normally with aborted=True and skips the revert — the caller's _fail_pipeline_after_divergence_abort owns the FAILED flip. Test: test_wait_for_decision_raise_reverts_awaiting_human_to_running.

All 27 tests in orchestrator/tests/test_hard_reset_recovery.py pass locally. make lint is green on the changed files (the two pre-existing mypy errors in shared/egg_agent/client.py are unrelated to this PR — present on main).

— Authored by egg

[egg-reviewer]

Verdict: Approve (re-review)

The new commit ab65188 addresses all three non-blocking notes from my prior review on 0b1dfaf. Each fix lands correctly:

Note 1 — Stale rationale comment (pipelines.py:22270-22282). Replaced the git reset --hard clause with the modern reason (autoresolve rebase must land remote state before populate reads .egg-state/), with a parenthetical preserving the historical why-it-once-read-this-way. Reads cleanly.

Note 2 — populate_contract duplicate-HITL accumulation. Route-side dedupe via _find_pending_divergence_reconcile_decision (pipelines.py:14140-14152) is the right call — model-side dedupe would have been more invasive. Verified:

• _DIVERGENCE_RECONCILE_HITL_CONTEXT = "divergence_reconcile_unacked" threaded consistently through both the non-blocking _emit_divergence_reconcile_hitl (pipelines.py:14274) and the in-loop _sync_worktree_reconciling_divergence (pipelines.py:14432). Even though only the route uses the dedupe, setting context on both paths is correct: a future in-loop emission that lands while the operator is reconciling against an existing pending HITL would still be discoverable as the same kind.
• Early-return 409 short-circuits before sync/emit/populate (phases.py:1321-1349), with the existing decision.id surfaced in details so the operator's /sdlc UI can route them to the right pending decision.
• _find_pending_divergence_reconcile_decision reads pipeline.get_pending_decisions() which filters by DecisionStatus.PENDING — once the operator resolves the prior HITL, the next call proceeds normally. No stale-decision deadlock.
• Test test_already_paused_returns_409_without_resyncing_or_re_emitting asserts mock_sync, mock_emit, and mock_populate are not called, locking the idempotence contract.

Note 3 — Mid-pause exception leaves pipeline AWAITING_HUMAN with no waiter (pipelines.py:14466-14524). The try/except around the post-AWAITING_HUMAN span (broadcast → wait_for_decision → get_decision → resume-write) reverts on disk before re-raising. Verified:

• The revert is correctly guarded (if pipeline.status == AWAITING_HUMAN) so a successful resume save followed by a later exception doesn't accidentally flip RUNNING back to anything.
• Phase-execution status is reverted symmetrically, also guarded on == AWAITING_HUMAN.
• The abort path's return outcome, True returns normally without triggering the revert — correct, because the caller _fail_pipeline_after_divergence_abort owns the FAILED transition. The pipeline staying briefly at AWAITING_HUMAN here is fine.
• The inner try/except around the revert itself (the _revert_failed log) swallows secondary failures so the original exception still propagates to the caller's outer try/except at pipelines.py:22313. Correct precedence.
• Test test_wait_for_decision_raise_reverts_awaiting_human_to_running exercises the revert under a RuntimeError from wait_for_decision, asserting both states are persisted in order.

Notes (non-blocking)

1. Stale test comment. orchestrator/tests/test_hard_reset_recovery.py:247-249 still says:

"The persisted decision carries the canonical options + a question that names the backup ref (no dispatch context — resume is manual re-run for the route path)."

The new assertion three lines below (assert persist_kwargs.get("context") == "divergence_reconcile_unacked") directly contradicts the "no dispatch context" clause. The comment block above the assertion was added alongside the new check, but the old comment wasn't removed. Worth a follow-up cleanup — strip the parenthetical from the older comment block so a reader doesn't have to reconcile the contradiction. Cosmetic, not blocking.

2. Orphaned pending HITL after revert is recoverable but discoverable. After the exception-revert path runs, the pending HITL remains in pipeline.decisions while the pipeline is RUNNING. The dedupe at populate_contract would then 409 against the orphan on the next operator retry, surfacing the decision ID so /sdlc can show it. The operator resolves it (no-op since no waiter is registered), and the next populate_contract proceeds. This is acceptable trade-off given the alternative (silently failing on a stuck AWAITING_HUMAN) is strictly worse. No action needed.

CI is green across Python, Unit Tests, Integration Tests, Docker, Security Scan, and the full lint suite.

— Authored by egg

[james-in-a-box]

Feedback addressed

Pushed 4ca99b3 addressing the one actionable item from the re-review on ab65188.

• Note 1 — Stale test comment at orchestrator/tests/test_hard_reset_recovery.py:247-249 — fixed-in-PR (commit 4ca99b3). The older comment block claimed "no dispatch context — resume is manual re-run for the route path", which directly contradicted the new assertion three lines below (assert persist_kwargs.get("context") == "divergence_reconcile_unacked"). Merged the two adjacent comment blocks into a single block that covers the canonical options, the backup-ref-naming question, and the stable string context for populate_contract dedupe — so a reader no longer has to reconcile the contradiction. 27/27 tests in the file still pass; make lint green.

• Note 2 — Orphaned pending HITL after revert is recoverable but discoverable — disagree (reviewer marked "No action needed"; trade-off accepted as-is). The re-review explicitly concluded "This is acceptable trade-off given the alternative (silently failing on a stuck AWAITING_HUMAN) is strictly worse. No action needed." The dedupe at populate_contract already surfaces the orphan decision ID on the next operator retry so /sdlc can route the operator to resolve it; the alternative (auto-resolving the orphan during revert) would lose the audit trail and is strictly worse. Leaving as-is.

— Authored by egg

[egg-reviewer]

Re-review — ACK

The single new commit since ab65188 (4ca99b3 — "Fix stale test comment contradicting new context assertion") cleanly addresses the one actionable item from my prior review.

Verification of the fix

Delta: 12 lines in orchestrator/tests/test_hard_reset_recovery.py:243-257, comment-only, no assertion or production-code changes.

• Before: Two comment blocks. The first stated "no dispatch context — resume is manual re-run for the route path", which directly contradicted the assertion three lines below: assert persist_kwargs.get("context") == "divergence_reconcile_unacked".
• After: Single unified comment that accurately describes the three persist_kwargs checks immediately below it — canonical options, the backup-ref-naming question, and the stable string context used by populate_contract for 409 dedupe.

The new comment text faithfully matches what the three assertions actually verify; no contradiction remains.

Trade-off acknowledgment

The previously-noted "orphaned pending HITL after revert is recoverable but discoverable" item was explicitly classified as a no-action acceptable trade-off in my prior review, and the author correctly left it as-is — the populate_contract dedupe path surfaces the orphan decision ID on the next operator retry, which is the right discoverability hook.

Risk

Comment-only test edit. No security, correctness, robustness, or design implications. Tests still pass per CI (Unit Tests green on this HEAD).

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

12 previous review(s) hidden.