Fix #2792: auto-recover from sync divergence with HITL ack - #2797
Conversation
When ``_sync_worktree_with_remote`` detects real divergence and the rebase autoresolve fails, the helper now pins local-only commits under ``refs/egg-backup/sync-recovery/<pipeline_id>/<unix_ts>`` and hard-resets HEAD to origin so downstream populator/decision-sync reads see a consistent worktree. Phase-boundary callers and the ``populate-contract`` HTTP route inspect the new ``WorktreeSyncOutcome`` return type; when ``hard_reset_performed`` is True they emit a dedicated two-option HITL (``Continue with post-reset state`` / ``Abort pipeline``) keyed on ``context="hard_reset_recovery:<phase>"``. The resolve_decision dispatch in routes/decisions.py routes Continue to a slim phase-restart helper and Abort to a status=CANCELLED writer. The ``_empty_contract_hitl_question`` wording no longer blames a phantom "populate-from-plan step silently failed earlier" — when the auto- reconcile path doesn't fire, the prose now accurately describes the divergence without inventing an earlier failure.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR closes a real gap — #2337 / #2627 left silent worktree divergence in the system, and the hard-reset auto-recovery with a HITL ack is the right shape. The new WorktreeSyncOutcome is well-modelled, the backup-ref naming convention (refs/egg-backup/sync-recovery/<pid>/<ts>) is recoverable via git for-each-ref, and the context discriminator (hard_reset_recovery:<phase>) keeps the dispatch hook stable. Tests cover the outcome shape, HITL text, and dispatch routing.
However, the three emission sites and the resume helper have substantial asymmetries that mean the destructive-recovery path doesn't actually behave consistently. Several of these are blocking; they're called out below with file:line references.
Blocking
B1. resume_pipeline_after_hard_reset_ack does not clear the BRC consensus tracker, evaluator, restart counts, or health-monitor anchors — phase re-run after a post-phase hard reset will not work correctly
orchestrator/routes/pipelines.py:3341-3435
The resume helper sets phase_exec.status = PENDING for the phase that was JUST marked FAILED, bumps run_epoch, and spawns a new _run_pipeline thread. Compare with restart_phase at lines 3250-3312, which does five additional things this helper skips: peer_consensus_tracker.clear(), consensus_evaluator.clear(pipeline_id), spawner.reset_restart_counts(pipeline_id), and health_monitor.reset_agent(role) per agent role.
The docstring justifies skipping container teardown and per-agent worktree deletion ("the hard-reset path is reached after BRC consensus already completed (or never spawned containers for this run)"), which is reasonable for those — but says nothing about the BRC tracker. At the post-phase emission site (line 21978+), the resume target IS the phase whose BRC tracker holds a CONFIRMED entry, because that phase completed normally before the sync ran. With the tracker not cleared, the re-spawned phase will either:
- Find the tracker already in a CONFIRMED terminal state and short-circuit (no actual re-work), or
- Have agents reconnect against stale per-role entries from the previous round.
This is exactly the class of bug that #2084 was opened against in the original restart_phase path, and #2084's fix is the reset_agent loop that this helper skips. The peer_consensus_tracker lookup pattern from restart_phase (3253-3262) needs to be replicated here, along with the evaluator clear, restart-count reset, and health-monitor anchor reset.
The phase-start emission site (line 20371) is less affected because the phase had not started — but the same code path runs there, so the fix is one helper, not three.
B2. Phase-start HITL emission site does not emit pipeline.failed events; observers will miss the destructive recovery
orchestrator/routes/pipelines.py:20371-20400 vs :21978-22027
The phase-start emission flips pipeline.status = FAILED, persists, emits the HITL decision, and returns. It never calls report_pipeline_status(pipeline, event_type="pipeline.failed", ...) or _emit_pipeline_event(pipeline, "pipeline.failed").
The post-phase emission site does both (lines 22021-22026), and every other FAILED-setter in this file does too — see _empty_contract_hitl cleanup at line 22100+, and the standard phase-failure block at line 21880+. Subscribers to pipeline.failed (collaborator dashboards, the gateway's pipeline-state cache, any external observer wired to the event stream) will see phase.completed followed by silence; they will not learn the pipeline became FAILED until they poll status directly.
Make the two sites emit symmetrically.
B3. Phase-start emission site leaves phase_exec.status untouched; state is asymmetric across the two recovery paths
orchestrator/routes/pipelines.py:20387-20391 vs :21994-22002
Phase-start emission writes only pipeline.status = FAILED + pipeline.error. It does not touch phase_exec.status or phase_exec.completed_at for the current phase. Post-phase emission writes phase_execution.status = FAILED + phase_execution.error + phase_execution.completed_at = datetime.now(UTC) in addition to the pipeline-level fields.
This means a GET /pipelines/<id> after a phase-start hard reset shows pipeline.status=FAILED but phase_exec.status=PENDING (the value going in) — the per-phase view does not reflect the failure. Either both fields should be set, or neither (with the resume helper compensating); choose one and apply it to both sites.
For phase-start specifically, leaving phase_exec.status=PENDING and phase_exec.error=None is defensible (the phase hadn't started), but then the post-phase site is overwriting a COMPLETE status with FAILED, which has its own oddness — the phase did complete, the sync just broke afterward. See B1 for the resume-side implication.
B4. populate_contract route runs the destructive sync but never emits an HITL or transitions pipeline state — automation drivers checking status_code alone will miss the hard reset
orchestrator/routes/phases.py:1103-1144 and :1210-1225
The route runs _sync_worktree_with_remote(...) pre-populate (good), and surfaces hard_reset_performed, backup_ref, discarded_commit_shas in the 200 success response and in details for 404/422/500 errors. The docstring explicitly intends this to be "the operator's signal" without an HITL gate.
The asymmetry is dangerous: the other two emission sites treat a hard reset as a terminal failure that requires operator ack before any downstream work runs. This route treats it as a no-stop event — _populate_contract_from_plan runs against the post-reset state immediately, and on success returns HTTP 200 with hard_reset_performed: true somewhere deep in data. An automation caller checking only response.ok will not notice that local commits were discarded.
If the design intent is "this route IS the recovery primitive, by the time an operator calls it they accept the destructive recovery" — that's defensible, but it needs to be expressed by:
- Refusing to run the populator at all if
hard_reset_performedis True, returning an explicit 409/422 reason; OR - Emitting the same HITL the other sites emit, gating the populator on the ack; OR
- At minimum, making the response a non-2xx status when
hard_reset_performedis True so automation cannot mistake it for success.
Surfacing the flag in the response body without changing the status code is the worst of the three options. (Note also: _populate_contract_from_plan consumes the post-reset worktree before any operator has seen the hard-reset flag — there is no gate where the operator can say "wait, don't populate.")
There's no test for the route's hard-reset surfacing behavior in test_hard_reset_recovery.py either; the test suite covers the helper, the question, and the dispatch, but does not exercise the route end-to-end.
B5. divergence_rebase_and_reset_failed is silently fall-through at every caller — the doubly-failed path lands on a divergent worktree with no recovery
orchestrator/routes/pipelines.py:7130-7149, callers at 20371, 21978, and phases.py:1115
When both rebase AND hard-reset fail, the helper returns WorktreeSyncOutcome(case="divergence_rebase_and_reset_failed", hard_reset_performed=False, ...). Every caller gates downstream behavior on outcome.hard_reset_performed — which is False here — so the failure-of-failures path is identical to "happy no-op" from the caller's perspective. The worktree is still divergent, but the pipeline continues with no signal.
I acknowledge pre-PR behavior was the same (rebase failure silently fell through), so this is not strictly a regression. But the entire PR is about closing the silent-divergence loop, and this corner case re-opens it — the helper now has a doubly-failed terminal case that the callers do not handle. At minimum, raise a typed exception on this path (analogous to PlanDraftMissingOnLocalError) so the callers can wire it into the same FAILED-cleanup flow, or surface it as a different HITL/alert. The current behavior makes the "the rebase fallback handles divergence" guarantee conditional on git reset --hard never failing, which is plausible-but-not-guaranteed on a busy worktree.
Non-blocking
N1. Backup ref name uses second-precision unix timestamp; two recoveries within the same second on the same pipeline will collide
orchestrator/routes/pipelines.py:7099 (unix_ts = int(time.time()))
git update-ref refs/egg-backup/sync-recovery/<pid>/<ts> against an existing ref will refuse to write without the old SHA (so the second write would fail and backup_ok = False, the ref points at the FIRST recovery's HEAD, and the second recovery's discarded SHAs are inlined into a WARN log only). Two recoveries in the same second is unlikely but not impossible (orchestrator restart loop, a HITL ack that races with a phase-start emission). time.time_ns() or appending a process-counter suffix avoids the collision and costs nothing.
N2. _collect_local_only_commits uses --no-commit-header, which requires Git 2.33+ (August 2021)
orchestrator/routes/pipelines.py:6656
Likely fine for the egg sandbox/orchestrator image, but worth a sanity check against the actual Git version pinned in the Dockerfile / CI base image, since a stale base would produce error: unknown option: --no-commit-header and the function would silently return () (the try/except swallows). If older Git support is needed, rev-list --pretty=format:%h %s followed by stripping commit <hash> lines achieves the same output.
N3. Test file does not cover the phase-start vs post-phase asymmetry, the resume helper's tracker behavior, or the populate-contract route surfacing
orchestrator/tests/test_hard_reset_recovery.py
The tests are well-scoped to the helper / HITL / dispatch units, but the integration concerns flagged in B1-B4 above (event emission, phase_exec state, BRC tracker clearing, populate-route gating) have no test coverage. Even granted that adding integration coverage in this file is out of scope for the slice — the resume helper specifically (B1) could be tested in isolation by mocking the spawner/tracker and asserting tracker.clear() was called.
N4. Resume helper's docstring justifies skipping container teardown by claiming "BRC consensus already completed (or never spawned containers for this run)" — that justifies skipping container teardown but NOT skipping the consensus state reset
orchestrator/routes/pipelines.py:3353-3363
The docstring frames the slim implementation as deliberate, listing three things it skips and giving a reason for each. The BRC tracker is missing from the list of things it skips — see B1. After fixing B1, update the docstring to reflect the actual skipped/included surface.
N5. _handle_hard_reset_recovery_resolution's unknown-resolution case is logged at INFO and silently swallowed
orchestrator/routes/decisions.py:153+
The dispatch returns silently if the resolution string is neither "Continue with post-reset state" nor "Abort pipeline". This is defensive (the SDLC skill should only emit one of the two options), but if it ever happens, the pipeline stays in FAILED with no recovery action. WARN is the right log level for "we got something we cannot route." More importantly, the decision is already marked RESOLVED by the outer dispatcher — so an unknown resolution silently consumes the only operator-facing ack the pipeline will ever have. Consider either: (a) refusing to mark the decision RESOLVED for unknown values, or (b) escalating via OVERSEER_ALERT.
Test plan verification
Tests reviewed: test_hard_reset_recovery.py (310 lines, new), test_sync_worktree.py (+153 lines), test_advance_phase_thread.py (+4), test_pipeline_failure_path.py (+8).
- Sync-helper subprocess scenarios: well-covered. The renamed
test_case_diverged_rebase_failed_then_recovers_via_resetasserts the 6-call subprocess sequence and the outcome fields. Good shape. - HITL question / dispatch wiring: well-covered.
- Wording regression for
_empty_contract_hitl_question: a defensible coupling — if the wording changes again, the test will need to be updated, but the assertion ("populate-from-plan step silently failed earlier" not in question) is narrowly targeted. - Integration coverage gaps: see N3.
No bypass of production paths detected; tests exercise _sync_worktree_with_remote directly via patch("routes.pipelines.subprocess.run") rather than reconstructing the outcome by hand.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
B1: resume_pipeline_after_hard_reset_ack now clears the BRC tracker, legacy consensus evaluator, restart counts, and Tier-1 health-monitor anchors (mirrors restart_phase). The original slim implementation re-spawned _run_pipeline against stale CONFIRMED state and could fire stale-elapsed health alerts (#2084 bug class). B2/B3: the phase-start emission site now sets phase_exec.status=FAILED, clears completed_at/error/start markers symmetrically with the post- phase site, and emits the pipeline.failed event so subscribers learn the pipeline is failed_pending_hitl. B4: populate_contract returns 409 when _sync_worktree_with_remote raises the new SyncRebaseAndResetFailedError or returns an outcome with hard_reset_performed=True. Operators must resolve the inline HITL before populator can run; the route no longer silently proceeds. B5: the doubly-failed recovery path now raises SyncRebaseAndResetFailed- Error instead of returning a WorktreeSyncOutcome the caller couldn't distinguish from a happy no-op. Both _run_pipeline emission sites and populate_contract route the typed exception through the same FAILED- cleanup flow as other terminal sync failures. N1: backup-ref timestamp upgraded from time.time() (1s resolution) to time.time_ns() so two recovery events in the same second never collide. N5: unknown HITL resolution in _handle_hard_reset_recovery_resolution now logs at WARN and broadcasts an OVERSEER_ALERT (priority=high) so the operator notices the dispatch was skipped — the prior INFO log was easy to miss while the pipeline stayed stuck. Tests cover the resume helper's tracker/evaluator/restart-count/health cleanup, the phase-agents-empty fallback path, the phase-mismatch guard, the OVERSEER_ALERT emission, and the new typed-exception raise path from the sync helper.
Review feedback dispositionsThanks for the careful pass. All ten items addressed in-PR in 7b505f6. Blocking
Non-blocking
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary
The B1/B2/B3/B5/N1/N4/N5 fixes are correctly implemented and mirror the patterns called out in the prior review:
- B1 —
resume_pipeline_after_hard_reset_acknow mirrorsrestart_phaselines 3250-3312 exactly: BRC trackerclear(), evaluatorclear(pipeline_id),spawner.reset_restart_counts(pipeline_id), and per-rolehealth_monitor.reset_agent. The roster-collection fallback toegg_contracts.agent_roles.get_roles_for_phasecovers the phase-start case wherephase_exec.agentsis empty. Verified atorchestrator/routes/pipelines.py:3417-3543. - B2/B3 — phase-start emission site now sets
phase_execution.status = FAILED,error,completed_at, callsreport_pipeline_status(... pipeline.failed ...)and_emit_pipeline_event(... pipeline.failed)symmetrically with the post-phase site. Verified atorchestrator/routes/pipelines.py:20594-20617. - B5 —
SyncRebaseAndResetFailedErroris the right shape (typedRuntimeErrorsubclass withbackup_refanddiscarded_commit_shas), the sync helper raises at the doubly-failed point, and all three call sites (phase-start:20523, post-phase:22185,phases.py:1126) catch it. - N1 —
time.time_ns()resolves the second-precision collision. - N4 — resume-helper docstring (
:3347-3375) now documents the consensus / restart-count / health-monitor cleanup. - N5 — unknown-resolution now WARN-logs and broadcasts
OVERSEER_ALERTwithpriority=highand a clear body explaining the pipeline will stay infailed_pending_hitluntil re-resolved.
However, B4 is only partially addressed and N3 is only partially addressed. Both are blocking.
Blocking
B4 (re-open). populate_contract returns 409 telling the operator to "ack the phase-boundary HITL" — but no HITL is ever emitted on this code path
orchestrator/routes/phases.py:1126-1148 (the SyncRebaseAndResetFailedError branch) and :1164-1187 (the hard_reset_performed=True branch) both:
- Return a 409 with a body that explicitly tells the operator to "ack the phase-boundary HITL".
- Do not call
_emit_hard_reset_recovery_hitlthemselves. - Do not persist any pipeline state transition.
The disposition asserts:
Automation drivers see a non-2xx; the inline HITL is emitted by the sync helper and must be resolved before populate is retried.
That is not what the code does. _sync_worktree_with_remote does not emit a HITL — its own docstring at orchestrator/routes/pipelines.py:6899-6902 is explicit:
Callers at phase boundaries inspect
hard_reset_performedon the returnedWorktreeSyncOutcomeand surface an HITL ack — recovery is automatic, acknowledgement is the human gate.
grep for _emit_hard_reset_recovery_hitl confirms the only emission sites are the four _run_pipeline callers (:20551, :20604, :22222, :22281). The populate_contract route is not among them.
The practical consequence: in the documented use case (operator-initiated recovery primitive — populate_contract called outside an active _run_pipeline), the operator gets a 409 instructing them to ack a HITL that does not exist. They will check /sdlc or the decisions API, find nothing, and be stuck without a clear next step. The actual recovery path (re-call populate_contract once the worktree is hard-reset and sees no divergence) is not documented in the error message.
The original B4 outlined three valid options. The fix takes (1) refuse + (3) non-2xx but the response wording implies (2) emit HITL, which was not implemented. Pick a lane:
- Either call
_emit_hard_reset_recovery_hitlfrom the route after the sync returnshard_reset_performed=True(and from theSyncRebaseAndResetFailedErrorbranch), persist pipeline.status=FAILED, and then return 409 — at which point the wording matches reality; - Or rewrite the 409 body to drop the "ack the phase-boundary HITL" language and tell the operator what they actually need to do (re-call the endpoint; the worktree is now in sync; the backup ref preserves the discarded commits).
The first option matches the post-phase / phase-start sites and gives the operator a uniform recovery surface across all three triggers of the hard reset. The second is the simpler one-liner but leaves the recovery surface split between code paths.
N3 (re-open). No test coverage for the populate_contract route's hard-reset paths
The disposition claims N3 was fixed, but the new tests in TestResumeHelperResetsConsensusAndHealth only cover the resume helper. The original N3 explicitly named three coverage gaps:
the integration concerns flagged in B1-B4 above (event emission, phase_exec state, BRC tracker clearing, populate-route gating) have no test coverage.
The two new 409 reasons (hard_reset_recovery_unacked at phases.py:1181 and sync_rebase_and_reset_failed at phases.py:1142) have zero assertion coverage — grep "hard_reset_recovery_unacked" orchestrator/tests/ and grep "sync_rebase_and_reset_failed" orchestrator/tests/ both return nothing.
This is the same class of issue that B4 was opened against: a fall-through path that "should" be terminal but is silently fall-through at every caller. Without a test that drives _sync_worktree_with_remote to hard_reset_performed=True (or raise SyncRebaseAndResetFailedError) through the route and asserts the 409 response shape, a future refactor of the route can drop these branches without anything failing.
Add tests for both 409 paths in test_hard_reset_recovery.py:
- mock
_sync_worktree_with_remoteto returnWorktreeSyncOutcome(case="divergence_recovered_via_reset", hard_reset_performed=True, backup_ref=..., discarded_commit_shas=...)→ assert 409 +reason="hard_reset_recovery_unacked"+details.backup_ref+ populator is not called; - mock
_sync_worktree_with_remoteto raiseSyncRebaseAndResetFailedError→ assert 409 +reason="sync_rebase_and_reset_failed"+details.backup_ref+ populator is not called.
Non-blocking
Asymmetric phase_execution is not None guard between phase-start and post-phase emission sites
orchestrator/routes/pipelines.py:20543 (phase-start, doubly-failed) and :20596 (phase-start, hard-reset-success) both guard if phase_execution is not None: before mutating; :22215 (post-phase, doubly-failed) and :22274 (post-phase, hard-reset-success) do not. At post-phase boundaries get_phase_execution is expected to return non-None, so this is defensible — but pick one and apply it to all four sites for consistency. Future refactors that move the call earlier will find the post-phase sites without guards and trip on the NoneType.status assignment.
Phase-start completed_at = datetime.now(UTC) on a phase that didn't actually start
orchestrator/routes/pipelines.py:20547 and :20600: the phase-start emission writes completed_at even though the phase never began work. Operationally fine since FAILED is terminal, but semantically the field name implies the phase completed work. Consider leaving completed_at = None on phase-start emissions and only setting it on post-phase emissions.
OVERSEER_ALERT broadcast in unknown-resolution dispatch silently swallows broadcast failures
orchestrator/routes/decisions.py:245-246: except Exception: pass after the alert build/send. If the message_store backing call fails (Redis down, etc.) the alert is dropped silently. The preceding logger.warning at :209-213 still fires, so there is a log signal — but the operator-facing alert is exactly the surface this re-introduces silent loss on. Consider logging the broadcast failure at WARN inside the except clause so it appears alongside the unknown-resolution warning.
except ValueError, AttributeError: (PEP 758 unparenthesized form) at :3430
Valid in Python 3.14 (the project's pinned minimum) and used elsewhere in the orchestrator (overseer/monitor.py, env_config.py, etc.), so this is consistent with house style. Flagging only for readers who haven't seen PEP 758 — no change requested.
Test plan verification
Tests reviewed in the delta:
test_hard_reset_recovery.py— three new tests inTestResumeHelperResetsConsensusAndHealthcorrectly exercise the helper's tracker/evaluator/restart-count/health-monitor cleanup, the empty-phase_exec.agentsroster-table fallback, and the phase-mismatch refusal. Thetest_unknown_resolution_is_logged_and_skippedupgrade correctly asserts theOVERSEER_ALERTenvelope shape (message_type, anomaly metadata, priority).test_sync_worktree.py—test_case_diverged_rebase_and_reset_failedcorrectly asserts the new typed-exception raise withbackup_refanddiscarded_commit_shasattached.
Gap: no test for the populate route's two 409 paths (see N3 re-open above).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
B4: populate_contract now calls shared helper _fail_pipeline_and_emit_hard_reset_recovery before returning 409 on both branches (hard_reset_performed=True and SyncRebaseAndResetFailedError). Helper persists pipeline+phase FAILED state, emits the hard-reset HITL, and broadcasts pipeline.failed events, so all three triggers (phase-start, post-phase, populate_contract) expose a uniform operator-facing recovery surface. N3: Added TestPopulateContractHardResetSurfacing class with two tests covering both 409 paths and asserting the helper is called and the populator is not. Non-blocking: added 'if phase_execution is not None:' guards to the two post-phase sites for consistency with the phase-start sites; and replaced silent 'except Exception: pass' with logger.warning() on OVERSEER_ALERT broadcast failure in decisions.py.
Follow-up review responses (commit 7e59496)B4 (re-open):
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary (delta 7b505f6..7e59496)
B4 and N3 re-opens are properly addressed. The fix extracts the lock + persist + HITL + events sequence into a new _fail_pipeline_and_emit_hard_reset_recovery helper (orchestrator/routes/pipelines.py:14832) and populate_contract now calls it on both 409 branches (SyncRebaseAndResetFailedError at orchestrator/routes/phases.py:1144 and hard_reset_performed=True at :1197). The 409 wording matches reality: an actionable HITL is emitted, the pipeline+phase are pinned to FAILED, and pipeline.failed is broadcast — automation drivers see non-2xx, operators see the same hard_reset_recovery:<phase> decision they'd see at a phase boundary.
The TestPopulateContractHardResetSurfacing class (orchestrator/tests/test_hard_reset_recovery.py:529) covers both 409 paths, asserts the helper is called with the right phase/backup_ref/discarded shas, and pins the contract that the populator is not invoked on a worktree the helper consumed. Tests pass locally (18/18 in the file).
Two additional follow-ups from the prior non-blocking notes are also in: the if phase_execution is not None: guards are now symmetric across the four _run_pipeline emission sites (:22266, :22328), and the silent except Exception: pass on the OVERSEER_ALERT broadcast in decisions.py:245 is now a WARN log.
No new blocking issues in the delta. A few non-blocking observations worth a follow-up.
Non-blocking
Helper has no direct unit test — both new tests mock it out
orchestrator/tests/test_hard_reset_recovery.py:593,659 patch _fail_pipeline_and_emit_hard_reset_recovery itself, so the new tests verify the route's wiring to the helper (correct kwargs, populator not called) but assert nothing about the helper's actual lock/save/HITL/events sequence. The inner _emit_hard_reset_recovery_hitl step is covered separately at :196; the lock+save+events glue is not unit-tested in isolation. Acceptable since the same logical sequence is exercised by the inline emission sites in _run_pipeline, but a direct test of the helper (mock store, assert save_pipeline was called with status=FAILED, assert _emit_hard_reset_recovery_hitl was invoked, assert both event paths fired) would future-proof the abstraction.
Four inline copies of the same sequence + one helper — drift risk
The helper now lives at :14832, but the four _run_pipeline emission sites (:20589, :20642, :22261, :22323) still open-code the same lock → load → set FAILED → save → emit HITL → report_pipeline_status → _emit_pipeline_event sequence. The two phase-start sites (:20589, :20642) are pure subsets of the helper's body and could be replaced directly; the two post-phase sites interleave _teardown_phase_overseer between the HITL emit and the event broadcast, which the current helper has no hook for. Worth a follow-up to either:
- migrate the two phase-start sites to the helper and accept the inline-vs-helper split for the post-phase sites; or
- extend the helper with an optional
pre_event_hook(or split into compose-able pieces) and migrate all four.
Five places to keep in sync if the FAILED-recovery shape ever changes again is the kind of footgun this PR's helper is supposed to remove.
Stale wording in the 409 response body
orchestrator/routes/phases.py:1207 still says "operator must ack the recovery via the phase-boundary HITL before re-running populate_contract." After this fix the route emits the recovery HITL itself — there is no separate "phase-boundary HITL" the operator needs to find. The HITL surfaces in /sdlc regardless of which trigger fired, so this isn't actively misleading, but the prose is a leftover from the original implementation and worth a one-line cleanup: drop "phase-boundary" or replace with "the hard-reset recovery HITL".
HITL question text and the SyncRebaseAndResetFailedError branch don't match reality
_hard_reset_recovery_hitl_question (orchestrator/routes/pipelines.py:14779-14790) says:
"the sync helper hard-reset HEAD to origin to keep downstream populator/decision-sync reads against a consistent state"
and offers:
"Continue with post-reset state — restart_phase {phase_label} so the populator and BRC agents re-run against the reconciled worktree"
When the helper is invoked from the SyncRebaseAndResetFailedError branch (now in three places: _run_pipeline phase-start :20599, post-phase :22273, and populate_contract :1144), the hard-reset itself failed — the worktree is still divergent, there is no reconciled state, and "Continue" would restart the phase only for the next phase-start sync to hit the same divergence and loop. Pre-existing in this PR (the prior commits already emitted the same HITL for the doubly-failed case), but the delta extends the surface to populate_contract as well.
Two options for a follow-up:
- Branch the question text on whether the reset actually completed and offer different options for the doubly-failed case (e.g. "Manually intervene" / "Abort"); or
- Suppress the "Continue" option when the doubly-failed branch fires.
The current behavior is observable but produces an option the operator cannot meaningfully use.
Lock release between FAILED-write and HITL-persist
_fail_pipeline_and_emit_hard_reset_recovery (:14853-14871) acquires the pipeline state lock, writes FAILED, releases, then calls _emit_hard_reset_recovery_hitl which re-acquires the lock to persist the decision. A reader observing the pipeline between the two acquisitions sees pipeline.status=FAILED with no pending decision — the operator's actionable HITL appears one lock-cycle later. Pre-existing in the inline sites (same shape), and the get_pipeline_state_lock is likely reentrant so the helper could hold across both writes. Worth a follow-up if you want to migrate the inline sites to the helper too.
Test plan verification
tests/test_hard_reset_recovery.py(18 passes locally) — the two new tests inTestPopulateContractHardResetSurfacingexercise both 409 paths throughclient.post("/api/v1/pipelines/issue-2792/phase/populate-contract"), mock_sync_worktree_with_remoteto drive each branch, and assert the response shape + populator-not-called + helper-called-with-correct-kwargs. No self-seeded golden / bypassed code path / name-vs-behaviour contradiction detected.- The helper itself has no isolated test (see above).
- The phase-start
phase_execution is not Noneguard symmetry is structural — no test asserts the new guards specifically, but the surrounding flow is exercised by the existing failure-path tests.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Helper unit tests: add TestFailPipelineAndEmitHelper (3 tests) covering call ordering (FAILED-write under lock, then HITL, then events), pre_event_hook timing, and reset_succeeded threading. - Drift risk: migrate all four _run_pipeline FAILED-recovery sites to call _fail_pipeline_and_emit_hard_reset_recovery; post-phase sites pass a pre_event_hook closure to tear down the per-phase overseer. - Stale wording: phases.py 409 response now says "ack the hard-reset recovery HITL (visible in /sdlc)" instead of "phase-boundary HITL". - HITL question for doubly-failed: add reset_succeeded param threaded through helper -> emit -> question/options; suppress the "Continue" option and switch the question text when reset itself failed. - Lock release between FAILED write and HITL persist: move the _emit_hard_reset_recovery_hitl call inside the get_pipeline_state_lock block (RLock is reentrant, so the inner _persist_hitl_decision re-acquire is safe).
|
All five non-blocking items from the egg-reviewer pass have been addressed in this PR. Disposition table (each item links to commit
Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary (delta 7e59496..0070af2)
All five non-blocking items from my prior review are properly addressed. The delta touches three files (+449/-147) and is well-contained:
- N1 (helper unit test) — new
TestFailPipelineAndEmitHelper(orchestrator/tests/test_hard_reset_recovery.py:776) with three direct tests: call-order under-lock (save_pipeline → emit_hitl → report_pipeline_status → emit_pipeline_event),pre_event_hookordering between HITL persist and public event, andreset_succeeded=Falsethreading through to the emit call. - N2 (drift risk from inline copies) — all four
_run_pipelineFAILED-recovery sites (pipelines.py:20666,:20701,:22327,:22383) now route through_fail_pipeline_and_emit_hard_reset_recovery. The two post-phase sites pass closure-basedpre_event_hookarguments that tear down the per-phase overseer between HITL persist and public broadcast, preserving the prior ordering. Loop-iteration captures (overseer_container_id,current_phase) are bound via default args to satisfy B023; the closures are invoked synchronously by the helper so late binding wouldn't bite even without the explicit binding. - N3 (stale "phase-boundary HITL" wording in 409 response) —
phases.py:1207-1209now says"operator must ack the hard-reset recovery HITL (visible in /sdlc) before re-running populate_contract". The HITL is emitted by the route itself via the helper, so the new wording matches reality. - N4 (HITL options + question text on doubly-failed branch) — new
_hard_reset_recovery_hitl_options(reset_succeeded)helper (pipelines.py:14821),_hard_reset_recovery_hitl_questionbranches onreset_succeeded(:14793for the success wording,:14807for the doubly-failed wording), and_emit_hard_reset_recovery_hitlthreads the flag through. WhenSyncRebaseAndResetFailedErrorfires (now from three sites:_run_pipelinephase-start:20673, post-phase:22334,populate_contractphases.py:1151), the options collapse to["Abort pipeline"]and the question text correctly reports the still-divergent state ("Continuing would only loop back into the same failure on the next sync"). - N5 (lock release between FAILED-write and HITL-persist) —
_emit_hard_reset_recovery_hitlis now invoked inside thewith get_pipeline_state_lock(pipeline_id):block of the helper (pipelines.py:14939).get_pipeline_state_lockreturns athreading.RLock(verified atstate_store.py:1280), so the inner_persist_hitl_decisionre-acquisition is safe. A reader between the two writes can no longer observestatus=FAILEDwithout the pending decision.
Tests pass locally — 24/24 in test_hard_reset_recovery.py (6 new), 87 passes across the three related test files, no regressions.
No blocking issues in the delta.
Non-blocking
Stale "phase-boundary HITL" wording remaining in populate_contract docstring
orchestrator/routes/phases.py:1068: the public docstring still says "The operator must ack the phase-boundary HITL before re-running this endpoint". The 409 response message at :1208 was updated to "hard-reset recovery HITL (visible in /sdlc)", but the docstring above it still describes the pre-fix wiring. After this PR the route emits the recovery HITL itself via _fail_pipeline_and_emit_hard_reset_recovery; there is no separate phase-boundary HITL for the operator to ack. Drop "phase-boundary" or replace it with "hard-reset recovery" to match the response body and the helper's actual behavior. One-line cleanup.
Dispatch handler doesn't validate option against the underlying HITL's options list
orchestrator/routes/decisions.py:190-207: _handle_hard_reset_recovery_resolution accepts "Continue with post-reset state" regardless of how the HITL was constructed. After this PR the doubly-failed branch (reset_succeeded=False) emits a HITL whose options list is ["Abort pipeline"] only — but if an operator hits the resolution API directly (bypassing the UI's options dropdown) with "Continue with post-reset state", the dispatch happily routes to resume_pipeline_after_hard_reset_ack, which spawns a fresh _run_pipeline thread that re-hits the same divergence at the next phase-start sync. This is a misuse scenario rather than a regression — the UI enforces the constraint — but a server-side check (load the decision, verify the resolution is in its options list, reject with unknown_resolution -> OVERSEER_ALERT path that already exists at :208-253) would close the gap. Defense-in-depth; safe to defer.
Helper test mocks _emit_hard_reset_recovery_hitl and store.load_pipeline — the actual lock is not exercised
TestFailPipelineAndEmitHelper (tests/test_hard_reset_recovery.py:776) verifies the call sequence the helper produces (save → emit_hitl → hook → report → event) but does so against MagicMock() for both the store and the emission function. The "FAILED-write under the same lock as HITL persist" invariant — the explicit reason for this round's change — has no test that drives both writes through a real get_pipeline_state_lock(pipeline_id) RLock and asserts that a concurrent reader can't observe FAILED without the pending decision. The current tests would still pass if a future refactor accidentally moved _emit_hard_reset_recovery_hitl back outside the lock. A real-lock test would future-proof the invariant; mock-only coverage is acceptable here since the invariant is small and well-documented inline, but worth a follow-up.
Four _run_pipeline sites still open-code the closure pattern
The two phase-start sites at pipelines.py:20666 and :20701 don't need a pre_event_hook (no overseer to tear down), so they call the helper plainly. The two post-phase sites at :22327 and :22383 define near-identical closures (_teardown_overseer_doubly_failed / _teardown_overseer_recovered) that differ only in the reason string and the reset_succeeded flag passed alongside. Folding both into a single _make_overseer_teardown_hook(reason) factory at module scope would remove ~30 lines of duplication. Minor; the duplication isn't a footgun since both closures invoke the helper synchronously.
Test plan verification
test_hard_reset_recovery.py— 24/24 pass. NewTestHardResetHitlDoublyFailedBranch(3 tests at:696) covers the options-list branch, the question-text branch, and the_emit_hard_reset_recovery_hitlplumbing forreset_succeeded=False. NewTestFailPipelineAndEmitHelper(3 tests at:776) covers the helper's call sequence, thepre_event_hookordering between HITL persist and public event, and thereset_succeeded=Falsethreading through to the emit call.test_sync_worktree.py,test_advance_phase_thread.py,test_pipeline_failure_path.py— 87/87 pass; the_sync_worktree_with_remotecapture-form regex intest_advance_phase_thread.py:443still matches.- No self-seeded golden / bypassed code path / name-vs-behaviour contradiction detected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Update populate_contract docstring to reference the hard-reset recovery HITL (visible in /sdlc) and note the route emits it. - Validate hard-reset HITL resolution against decision.options before dispatching; OVERSEER_ALERT on mismatch instead of silent fall-back. - Add real-lock test using state_store.get_pipeline_state_lock to prove FAILED write and HITL persist execute under the same RLock. - Extract _make_overseer_teardown_hook factory to deduplicate the two post-phase teardown closures.
Review feedback addressed in commit 4374471
Verification
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary (delta 0070af2..43744714)
The four non-blocking items from the prior review are properly addressed in commit 43744714:
- N1 (stale docstring in
populate_contract) —orchestrator/routes/phases.py:1062-1075now reads"emits the hard-reset recovery HITL itself"and"ack the hard-reset recovery HITL (visible in /sdlc)". The docstring matches the response body and the helper's actual behavior. - N2 (dispatch validates resolution against decision options) —
orchestrator/routes/decisions.py:153-292addsvalid_options: list[str] | None = None. The cross-check setsoption_mismatch_reasonwhen the resolution isn't in the list and gates both the Continue and Abort arms on it, routing mismatches to the unknown-resolution branch (WARN log + OVERSEER_ALERTwith a body that names the mismatch). The dispatch call site at:973passeslist(decision.options or []). Defense-in-depth against direct-API operators picking "Continue" on a doubly-failed HITL whose options collapsed to["Abort pipeline"]— closes the loop the prior review flagged. - N3 (helper has direct lock test) —
orchestrator/tests/test_hard_reset_recovery.py:1080addstest_failed_write_and_hitl_persist_held_under_same_lockthat drives both writes through the realthreading.RLockfromstate_store.get_pipeline_state_lock. See the non-blocking note below — the test name overstates what it actually verifies, but the intent and the lock-acquisition pattern (real RLock, real_persist_hitl_decision) are right. - N4 (extract shared
_make_overseer_teardown_hookfactory) —orchestrator/routes/pipelines.py:20449-20485extracts a factory inside_run_pipelinethat snapshotscontainer_idandphaseas function parameters (binding at factory-call time). Both post-phase emission sites (:22353,:22393) call it with the same shape, replacing the two ~20-line duplicated closures. The factory correctly stays inside_run_pipelinebecause it closes over the mutablephase_overseer_activenonlocal.
29/29 tests in test_hard_reset_recovery.py pass locally (5 new in this delta).
No blocking issues in the delta.
Non-blocking
The "real-lock" test doesn't actually validate the "lock spans both writes" invariant
orchestrator/tests/test_hard_reset_recovery.py:1080-1185 claims to enforce that the helper holds the lock across both save_pipeline calls, but the probe runs only during each individual save (inside fake_save's side effect) — never between them.
In a buggy refactor that releases the outer lock between save #1 and save #2 and lets _persist_hitl_decision re-acquire its own lock for save #2:
# Hypothetical buggy refactor
with get_pipeline_state_lock(pipeline_id):
...
store.save_pipeline(pipeline) # save #1 — probe sees False (outer lock held)
# <-- outer lock released; observer COULD see status=FAILED with no pending decision here
_emit_hard_reset_recovery_hitl(...) # inner _persist_hitl_decision acquires its own lock
# inside _persist_hitl_decision:
# with get_pipeline_state_lock(pipeline_id):
# ...
# store.save_pipeline(disk_pipeline) # save #2 — probe sees False (inner lock held)Both probes still return False, so acquisitions_during_save == [False, False]. The test passes against the buggy refactor it claims to defend against.
The actual invariant requires probing in the gap between the two saves, not during each save. One way to do that:
# Spawn the probe thread when _persist_hitl_decision starts (before it re-acquires the lock).
# Patch add_decision (or load_pipeline inside _persist_hitl_decision) to probe.
def fake_load(pid):
if pid == pipeline_id and mock_store.save_pipeline.call_count >= 1:
probe_other_thread() # this fires after save #1 returns
return pipeline
mock_store.load_pipeline.side_effect = fake_loadThe production code is correct — _emit_hard_reset_recovery_hitl is invoked inside the outer with get_pipeline_state_lock(pipeline_id): block at pipelines.py:14939, and the inner RLock re-acquire in _persist_hitl_decision:14712 is safe. But the test should match its docstring claim or the docstring should be softened — as written, the next reader will trust the test to catch a regression that it cannot catch.
Factory docstring overstates the late-binding concern
orchestrator/routes/pipelines.py:20455-20470: the docstring says "late binding would race a subsequent loop iteration". In practice the closure is invoked synchronously by _fail_pipeline_and_emit_hard_reset_recovery (via pre_event_hook() at :14948-14949), and every call site immediately breaks out of the loop afterwards (:22359, :22399). Late binding wouldn't actually race in the current shape — the parameter snapshot is defensive but not strictly necessary for correctness. Not a bug; the snapshot is the right default for a factory that returns closures, and the explicit binding is a fine guard against future refactors that move the call out of the synchronous path. Worth softening the docstring to "parameter snapshot binds the values at factory-call time so a future refactor that defers invocation past the current loop iteration is safe".
list(decision.options or []) is redundant
orchestrator/routes/decisions.py:973: decision.options is typed list[str] with default_factory=list (orchestrator/models.py:285-287), so it's never None. The or [] fallback never fires. list(decision.options) would be enough; the current form is harmless but adds a phantom case future readers may try to reason about.
Test plan verification
pytest orchestrator/tests/test_hard_reset_recovery.py— 29/29 pass locally. The 4 newTestDispatchResolutiontests correctly cover the four corners ofvalid_optionshandling (Continue rejected when not in list, Abort accepted in list, Continue accepted in list,valid_options=Nonekeeps legacy). The newTestFailPipelineAndEmitHelper::test_failed_write_and_hitl_persist_held_under_same_lockexercises a real RLock and asserts pipeline state after the helper returns — see the non-blocking note above for the gap in its coverage of the spanning-lock invariant.- No self-seeded golden / bypassed code path / name-vs-behaviour contradiction other than the test-vs-docstring mismatch flagged above.
— Authored by egg
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
|
egg review completed. View run logs 14 previous review(s) hidden. |
…alyst integration Replans issue #2777 against current HEAD. Architecture mirrors the operator-approved 2-slice DAG from the prior pipeline (issue-2777): slice-1 = A+D (context-PR collapse + PR-phase deletion + coupled cleanup), slice-2 = C (slice/phase restart hardening, bundles #2409). #2792 remains OUT OF SCOPE per decision-11 (now also independently resolved by #2797). Key replan changes vs the prior plan: - ALL file:line citations refreshed against HEAD. pipelines.py drifted ~+632 lines (upper half) / ~+1383 lines (slice-loop region). No architectural primitives have been removed by intervening commits. - ConsensusEvaluator deletion (cq-5) expanded from 5 to 8 reference clusters: pipelines.py adds 3516-3522 (a 6th cluster not in the prior plan); risk_analyst R2 added phases.py:119-124 and signals.py:847-871 (clusters 7+8). AC-18 enumerates the full 8-cluster surface. - PRMetadata schema (cq-2): augmented with _migrate_schema_version_to_1_2 helper following the existing _migrate_schema_version_to_1_1 pattern to handle ~5 on-disk contract fixtures (risk_analyst R3). AC-19 rewires stacked_pr_reconciler.py's cascade-base consumer to derive from context_pr_number + _resolve_slice_base_branch. - #2570 silent-rebase (TASK-1-9): risk_analyst R1 verified the actual root cause is the bare-rebase fallback inside _sync_worktree_with_remote at pipelines.py:7219-7239 (OOS per decision-11). AC-9a now mandates a PROACTIVE plan-phase HITL registration (not reactive at implement runtime). Recommended resolution: narrow scope override for the bare-rebase fallback only. - PR-phase deletion (cq-4): AC-20 adds gateway/tests/test_phase_transition.py to the gateway test rewrite list, and explicitly PRESERVES the namesake-namespace phase="pr" carve-outs at gateway_client.py:1409/1441, test_session_manager.py, test_gateway.py (NOT to be deleted). - _CONTEXT_BRANCH_RE deletion (cq-4 / R7): AC-21 expands cleanup to cover the dangling is_context_push variable cluster at gateway.py:1344-1392. - Persistence helper (R9): AC-22 mandates _open_context_pr_at_implement_start explicitly names the persistence path (save_contract / update_contract). - _check_post_consensus_stall (R6): AC-23 mandates either re-derived predicate or short-circuit deletion. - _tracker_key shape consistency (R8): AC-24 pins call signature across AC-15 (restart_phase) and AC-16 (startup_reconciliation). - #2409 narrowed: peer_consensus.reconstruct_tracker_from_messages already accepts slice_id at HEAD (peer_consensus.py:1919); message_store already filters on metadata['slice_id'] (:407-416). Gap is only startup_reconciliation.py:312 calling without slice_id. No schema change needed to message_store.Message. Outputs: - .egg-state/agent-outputs/issue-2777-replan-architect-output.json (32 acceptance criteria; 13 risk_analyst findings integrated) - .egg-state/agent-outputs/issue-2777-replan-architect-slices.yaml (slice scaffold: slice-1 root, slice-2 depends on slice-1) #2809 acknowledged: architect retained the 2-slice operator-chosen DAG without further sub-slicing because the natural seams within slice-1 all touch overlapping line ranges in pipelines.py — sub-slicing would convert intra-PR review into cross-PR merge-conflict resolution. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rator directive
Operator iteration directive: the prior 16-coder-task slice-1 wedged because the
coder shipped unwired WIP partials it could not finish in one BRC cycle.
Subdivide along natural seams into three sequential sub-slices:
slice-1a (root) — new primitives + wiring (only ADDS code).
`_open_context_pr_at_implement_start`, the
PlanPreflightError validator, `_is_slice_dag_mode`,
`_resolve_slice_base_branch`, and rewires of the
five `_maybe_open_base_pr_for_plan_to_implement`
call sites (pipelines.py:16503, :22132, :23671,
:24666, phases.py:500). Primary use case is
reachable on its own at end-of-1a.
slice-1b (->1a) — scaffold + PR-phase deletions (only DELETES code,
plus the stacked_pr_reconciler cascade-base rewire
that depends on the schema field deletion). The
seven context-PR helpers; the PR phase across ~26
files (orchestrator + gateway + schema + docs +
tests); `_CONTEXT_BRANCH_RE` + `is_context_push`
cleanup; PRMetadata 1.1->1.2 schema bump (cq-2);
`orchestrator/consensus.py` + all 8 reference
clusters (cq-5).
slice-1c (->1b) — cohesion-independent cleanup tail. cq-8 idempotency
in `create_slice_pr`; #2570 silent-rebase fix with
AC-9a HITL gate; per-site BLE001 audit in the
slice-loop region (Q2); slice-loop except-ImportError
shim collapse (Q3); umbrella-terminology structural
deletion (cq-6 subsumes #2389); SliceScheduler #2199
noqa markers + docs (cq-3); stale archaeology
deletions; end-to-end integration test (Q4).
slice-2 (->1c) — restart hardening, unchanged. Eager-persist
parent_branch_at_creation + merge-base fallback
(cq-9); per-slice tracker iteration in
restart_phase; slice_id-threaded reconstruction in
startup_reconciliation (bundles #2409); bootstrap
recognition of non-COMPLETE slices.
Linear DAG: 1a -> 1b -> 1c -> 2 (forest invariant honoured).
ALL other Wave 2 decisions remain binding and unchanged: cq-1's [A+D]->[C]
dependency direction, cq-2..cq-10, feedback Q1-Q5. Only intra-A+D
granularity changes. #2792 / Goal 4 stays OUT OF SCOPE per decision-11
(independently resolved in merged #2797).
File:line citations re-verified at HEAD (28f7ef9) by sub-agent fact-check.
All ~50 cited primitives intact; one drift corrected
(_SLICE_INTEGRATION_BRANCH_RE def at gateway.py:1104, not :1351).
Sub-slice sizes: 1a ~5 coder tasks, 1b ~7, 1c ~7, 2 ~5 — each fits a
single BRC cycle. Intermediate dead-code state of
`_maybe_open_base_pr_for_plan_to_implement` (zero callers after 1a, deleted
by 1b) is explicit in INV-11 / R12 / AC-25.
… dispatch (#2978) (#2981) * fix(orchestrator): unwrap choice envelope in hard-reset recovery HITL dispatch (#2978) The SDLC HITL CLI resolves a choice decision by sending {"action": "select", "selected": "<option>"} (sandbox/egg_lib/sdlc_hitl.py), which resolve_decision serializes into decision.resolution. But _handle_hard_reset_recovery_resolution did a bare-string compare against "Continue with post-reset state" / "Abort pipeline" and cross-checked the raw resolution against valid_options. The JSON envelope matched neither, so every operator selection routed to the unrecognized-option path (OVERSEER_ALERT), no dispatch ran, and the pipeline stayed wedged in failed_pending_hitl — the operator had to fall back to cancel_task. Add _normalize_choice_resolution to unwrap the select envelope to its bare option label before both the valid_options cross-check and the Continue/Abort dispatch, mirroring the phase-gate path that already parses this envelope. Bare-string and non-select resolutions pass through unchanged. Also reconcile orchestrator/README.md's stale pre-#2797 worktree-sync contract with the current rebase-then-hard-reset-recovery behavior, and note the non-destructive redesign tracked in #2979. Tests drive the real JSON envelope through the dispatch helper (Continue and Abort, including the Abort-only doubly-failed options list) plus direct unit coverage of the normalizer. The prior tests only fed already-extracted bare strings, which is why the bug shipped green. * fix(orchestrator): normalize choice envelope for all resolve_decision dispatch hooks (#2978) The #2978 review pointed out three parallel bugs in resolve_decision with the same shape as the hard-reset recovery one already fixed: Restart agent (#1428), Continue without (concurrent_executor failed reviewer), and the conditional-ACK 3-way gate (#2004). Each compared decision.resolution against bare option labels, so any operator selection routed through the SDLC HITL CLI envelope {"action": "select", "selected": "<option>"} fell into the no-op unrecognized-option branch while the decision was still marked RESOLVED — wedging the pipeline. Fix: compute dispatch_resolution = _normalize_choice_resolution(...) once at the dispatch boundary in resolve_decision() and route the four hooks through it. decision.resolution on disk, the DECISION_RESOLVED event payload, and the API response keep the raw envelope so the audit trail is unchanged; only the in-process value handed to the dispatch helpers (and the log lines they emit) is the bare label. Also from the review's non-blocking observations: - routes/decisions.py now imports _HARD_RESET_RECOVERY_CONTINUE / _HARD_RESET_RECOVERY_ABORT from routes.pipelines instead of hardcoding the same strings inline, so a rename on the producer side can't silently desync the dispatch. - The unknown-resolution OVERSEER_ALERT body interpolates the same constants in the "expected one of …" sentence. - _normalize_choice_resolution's docstring spells out that the persisted resolution / event / response keep the envelope; only dispatch-side values are normalized. Tests: - test_decisions_routes.py — envelope-form Restart agent triggers _handle_restart_agent and the persisted resolution stays the raw envelope; envelope-form Continue without on failed_role:<role> drives excuse_reviewer. - test_conditional_ack_hitl_gate.py — envelope-form Approve / Reject / Address each route to _persist_deferred_actions / _force_nack_conditional_edges / _invalidate_conditional_acks. - test_hard_reset_recovery.py — envelope-form Continue against the doubly-failed Abort-only options list is still rejected by the valid_options cross-check (mirror of test_continue_rejected_when_not_in_valid_options). Refs #2978. * Address non-blocking review feedback (#2978) - Add defense-in-depth _normalize_choice_resolution call to _handle_conditional_ack_gate for symmetry with _handle_hard_reset_recovery_resolution. Both helpers are now safe against future direct callers bypassing the resolve_decision dispatch-boundary normalization; idempotent on already-unwrapped strings. - Drop unreachable TypeError from _normalize_choice_resolution's except clause. The function's resolution: str signature plus the empty-string early return mean json.loads only sees a non-empty str, which never raises TypeError. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
…) (#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 recurrence loop on #2337 / #2627: when
_sync_worktree_with_remotedetects real divergence and the rebase autoresolve fails, the helper now reconciles inline via a hard reset and surfaces an HITL ack instead of leaving the downstream populator to fail loudly against stale state._sync_worktree_with_remotereturns a newWorktreeSyncOutcome(case,hard_reset_performed,backup_ref,discarded_commit_shas). On rebase failure the helper pins HEAD underrefs/egg-backup/sync-recovery/<pipeline_id>/<unix_ts>, thengit reset --hard origin/<branch>. Discriminator log lines stay in place; the new terminal case isdivergence_recovered_via_reset(ordivergence_rebase_and_reset_failedwhen the reset itself fails).POST /pipelines/<id>/phase/populate-contractHTTP route inspecthard_reset_performed. The first two mark the pipeline FAILED and emit a dedicated HITL keyed oncontext="hard_reset_recovery:<phase>"; the route surfaces the same fields in its response body.routes/decisions.pyroutesContinue with post-reset state→resume_pipeline_after_hard_reset_ack(slim phase-restart: reset phase exec, bumprun_epoch, spawn a fresh_run_pipelinethread) andAbort pipeline→abort_pipeline_after_hard_reset_ack(setsstatus=CANCELLED, emits the event; the existing PATCH cleanup machinery handles container/worktree teardown when the operator drives it)._empty_contract_hitl_question— the new wording references the auto-reconcile path without inventing a phantom earlier failure.Test plan
orchestrator/tests/test_hard_reset_recovery.py(new) — backup-ref name layout,WorktreeSyncOutcomeshape on non-recovery branches, HITL question/options/context, dispatch wiring (Continue/Abort/unknown), and the dropped-wording regression check.orchestrator/tests/test_sync_worktree.py— added taxonomy cases fordivergence_recovered_via_reset,divergence_rebase_and_reset_failed, and the backup-ref-write-failure inlined-SHAs path. Updated the two prior tests that assumed rebase failure short-circuited without a reset.orchestrator/tests/test_advance_phase_thread.py— relaxed the structural regex so it acceptsoutcome = _sync_worktree_with_remote(...)in addition to the bare call.orchestrator/tests/test_pipeline_failure_path.py— pinned theStalePipelineBranchErrortest's sync mock to a non-recoveryWorktreeSyncOutcomeso the new hard-reset HITL path doesn't fire against the mocked-out helper.make lintclean (ruff + ruff-format via pre-commit)..venv/bin/pytest).Out of scope (per the issue plan)
orchestrator/routes/pipelines.py(Add a max-file-size lint to prevent oversize source files (orchestrator/routes/pipelines.py is 670KB / 15,356 lines) #2248-allowlisted; Decompose 15 oversize Python source files to clear the file-size allowlist #2261 slice-15)..egg-state/brc-history/or.egg-state/contracts/conflicts..egg-state/contracts/writes through the gateway.