fix(orchestrator): unwrap choice envelope in hard-reset recovery HITL dispatch (#2978)

[jwbron]

Summary

Slice A of #2978: the hard-reset recovery HITL ack was undispatchable through
the standard SDLC flow, leaving pipelines wedged in failed_pending_hitl.

The SDLC HITL CLI resolves a choice decision by sending
{"action": "select", "selected": "<option>"} (sandbox/egg_lib/sdlc_hitl.py:716),
which resolve_decision serializes into decision.resolution. But
_handle_hard_reset_recovery_resolution (orchestrator/routes/decisions.py) 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 → OVERSEER_ALERT: hard-reset-recovery-unknown-resolution, decision
marked RESOLVED, no dispatch ran, pipeline stuck (live repro on pipeline-8cf1f000:
the operator had to fall back to cancel_task).

The phase-gate path already unwraps this envelope (routes/pipelines.py:18573,
22783); the hard-reset dispatch was the one place that didn't.

Changes

• routes/decisions.py — add _normalize_choice_resolution, which unwraps the
  {"action": "select", "selected": ...} envelope to its bare option label. Call it
  at the top of _handle_hard_reset_recovery_resolution, before the
  valid_options cross-check and the Continue/Abort compares. Bare-string (legacy /
  direct-API) and any non-select shape pass through unchanged.
• README.md — reconcile the stale pre-Fix #2792: auto-recover from sync divergence with HITL ack #2797 worktree-sync contract ("fast-forward
  merge … leaves the worktree unchanged") with the current
  rebase-then-hard-reset-recovery behavior, and point at the [orchestrator/sync] Non-destructive divergence reconcile + prevent self-inflicted plan-sync divergence (split from #2978) #2979 redesign.
• tests/test_hard_reset_recovery.py — drive the real JSON envelope through the
  dispatch helper (Continue + Abort, including the Abort-only doubly-failed
  valid_options), plus direct unit coverage of the normalizer. The prior dispatch
  tests only fed already-extracted bare strings, which is why the bug shipped green.

Out of scope (tracked in #2979)

The destructive hard-reset reconcile itself — discarding committed work to a backup
ref and marking the pipeline FAILED post-consensus — and the self-inflicted plan-sync
divergence at the source. This PR only makes the existing recovery ack reachable.

Test plan

• .venv/bin/pytest orchestrator/tests/test_hard_reset_recovery.py — 36 passed
  (5 new envelope/normalizer tests).
• ruff check + ruff format --check clean; pre-commit hooks pass.

Closes #2978.

[egg-reviewer]

Review

The targeted fix for _handle_hard_reset_recovery_resolution is correct and well-tested. The new _normalize_choice_resolution helper correctly unwraps the {"action": "select", "selected": ...} envelope, the integration in the dispatch hook puts the normalization before the valid_options cross-check (right ordering — the doubly-failed branch test confirms this still routes a Continue envelope to the unknown-resolution path when only Abort pipeline is allowed), and the edge-case handling (bare-string passthrough, malformed JSON, non-select action shapes, empty input) matches what callers need. The README reconciliation accurately describes the post-#2797 rebase-then-hard-reset behavior and correctly forward-references #2979 for the non-destructive redesign. CI is green and the integration tests cover the production code path through the dispatch helper rather than direct calls on already-unwrapped strings — exactly the gap the PR description correctly identifies in the prior tests.

Blocking — parallel bugs in the same file left unfixed

The PR introduces _normalize_choice_resolution as a module-level helper but applies it to only one of the four dispatch hooks reached from resolve_decision() (orchestrator/routes/decisions.py:874) that have the identical bug pattern. Every other branch still does a bare-string compare against decision.resolution, which — as the PR itself documents — is the JSON envelope, not the bare option label, whenever the operator resolves through the standard SDLC HITL CLI (every choice decision routes through _handle_choice in sandbox/egg_lib/sdlc_hitl.py:709-718, which wraps the selection in {"action": "select", "selected": "..."}).

Concretely still broken after this PR:

1. orchestrator/routes/decisions.py:955 — if decision.resolution == "Restart agent": against the overseer-created agent-failure HITL (orchestrator/overseer/monitor.py:1924, options ["Restart agent", "Continue monitoring", "Cancel pipeline"]). Operator selects "Restart agent" via provide_input → envelope → bare-string compare fails → _handle_restart_agent never runs → the failed container is not respawned, but the decision is marked RESOLVED. Same wedged-pipeline shape as #2978.
2. orchestrator/routes/decisions.py:973 — if decision.resolution == "Continue without" against the concurrent-executor failed-reviewer HITL (orchestrator/concurrent_executor.py:596, options ["Retry (respawn agent)", "Abort phase", "Continue without"]). Same envelope, same dropped dispatch → tracker.excuse_reviewer() is never called → BRC consensus stays blocked on the failed reviewer indefinitely. This is exactly the recovery path operators reach for after an agent dies, so the silent-no-op leaves the pipeline stuck at the worst time.
3. orchestrator/routes/decisions.py:390-401 (_handle_conditional_ack_gate) — the 3-way conditional-ACK HITL (orchestrator/routes/phases.py:860-864, decision_type="choice") compares resolution directly against CONDITIONAL_ACK_APPROVE / CONDITIONAL_ACK_REJECT / CONDITIONAL_ACK_ADDRESS. Same envelope → all three branches fall through to the "unrecognized option" logger.info and all three downstream effects are skipped (_persist_deferred_actions doesn't write the obligations to contract.pr.deferred_actions, the producer is never NACK'd back, the ACK isn't invalidated). The PR body becomes incoherent and consensus state silently desyncs from operator intent.

These three are not "tracked separately under #2979" — #2979 is scoped to the destructive-recovery redesign, and #2978's body explicitly identifies the envelope/dispatch fragility as the kind of bug that needs fixing at every site (the "two-wave HITL gap" reference is the symptom across sites, not a separate non-overlapping issue). The fix is trivial — either call _normalize_choice_resolution from the three other sites, or (cleaner) move the normalization to resolve_decision() itself, e.g.:

decision = queue.resolve_decision(decision_id, resolution)
# Normalize choice-envelope resolutions once at the dispatch boundary so
# every helper below sees the bare option label instead of the JSON envelope.
dispatch_resolution = _normalize_choice_resolution(decision.resolution or "")
...
if dispatch_resolution == "Restart agent":
    _handle_restart_agent(pipeline_id, decision.question)
...

That preserves decision.resolution as the raw envelope on disk (audit trail intact) but fixes all four dispatch hooks at once. The PR is in the right file, has the right helper, and has the test scaffolding to extend. Per the review rules — "Pre-existing issues are still blocking: If a PR modifies code that already has broken or inconsistent behavior, request changes to fix it" — please fold these three parallel bugs into this PR. They are the same bug, in the same function, fixable with the same helper this PR introduces.

Non-blocking observations

• Duplicated option strings. _HARD_RESET_RECOVERY_CONTINUE / _HARD_RESET_RECOVERY_ABORT are defined as constants at orchestrator/routes/pipelines.py:14327-14328, but orchestrator/routes/decisions.py:254,265 hardcode the same strings inline. Pre-existing inconsistency that drifts the moment one side renames an option; importing the constants from the producer side would prevent that. Worth fixing while you're here but won't block.
• Logged resolution is now the normalized form. Behavioral change — previously the logger.warning / OVERSEER_ALERT body at orchestrator/routes/decisions.py:276,316 would have included the raw envelope JSON; now they include the unwrapped bare label (or the original on non-select shapes). Diagnostically cleaner, but worth a sentence in the docstring noting that the audit trail (decision.resolution on disk) still carries the envelope; logs do not.
• Coverage micro-gap. No envelope-form test for the doubly-failed Continue-against-["Abort pipeline"] cross-check rejection — the existing bare-string test (test_continue_blocked_when_only_abort_allowed in the same file) covers the logic post-normalization, so the call paths are exercised, but a single envelope-form assertion mirroring it would prevent future drift if normalization is ever moved/removed. Not blocking on its own.

— Authored by egg

[james-in-a-box]

Thanks for the review — folded all three parallel-bug fixes into this PR using the cleaner dispatch-boundary normalization you suggested, plus the two non-blocking observations.

Disposition

• Blocking Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 — Restart agent bare-string compare (decisions.py:955) — fixed-in-PR (commit 1c6633e). Normalizes once at the dispatch boundary in resolve_decision(); dispatch_resolution = _normalize_choice_resolution(decision.resolution or "") is then used for the "Restart agent" compare. Covered by test_restart_agent_envelope_triggers_restart in test_decisions_routes.py.
• Blocking Phase 1: Repository setup and CI infrastructure #2 — Continue without bare-string compare (decisions.py:973) — fixed-in-PR (commit 1c6633e). Same dispatch_resolution covers the failed-reviewer path. Covered by test_continue_without_envelope_calls_excuse_reviewer.
• Blocking Phase 1.5: Documentation extraction #3 — conditional-ACK gate bare-string compares (decisions.py:390-401) — fixed-in-PR (commit 1c6633e). _handle_conditional_ack_gate now receives dispatch_resolution so the three CONDITIONAL_ACK_* branches dispatch. Covered by test_approve_envelope_persists_deferred_actions, test_reject_envelope_force_nacks_edges, test_address_envelope_invalidates_acks in test_conditional_ack_hitl_gate.py.
• Non-blocking — duplicated option strings in decisions.py:254,265 — fixed-in-PR (commit 1c6633e). _handle_hard_reset_recovery_resolution now imports _HARD_RESET_RECOVERY_CONTINUE / _HARD_RESET_RECOVERY_ABORT from routes.pipelines instead of hardcoding the literals; the unknown-resolution OVERSEER_ALERT body uses the same constants in its "expected one of …" sentence.
• Non-blocking — log/audit divergence docstring note — fixed-in-PR (commit 1c6633e). _normalize_choice_resolution's docstring now spells out that the persisted decision.resolution, the DECISION_RESOLVED event payload, and the API response all keep the raw envelope; only the in-process dispatch value (and log lines that echo it) is the unwrapped form. Audit-trail invariant pinned by test_resolution_persisted_as_envelope_for_audit_trail.
• Non-blocking — envelope-form coverage for doubly-failed Continue-against-["Abort pipeline"] — fixed-in-PR (commit 1c6633e). Added test_select_continue_envelope_blocked_when_only_abort_allowed in test_hard_reset_recovery.py, mirroring the existing bare-string test_continue_rejected_when_not_in_valid_options — guards against regression if normalization is ever moved or removed.

Implementation note

Took the cleaner option from the review: normalize once in resolve_decision() and route all four hooks through dispatch_resolution. decision.resolution on disk, the DECISION_RESOLVED event, and the API response intentionally still carry the raw envelope so the audit trail records exactly what the operator sent.

_handle_hard_reset_recovery_resolution also keeps its internal _normalize_choice_resolution call as defense-in-depth — the existing direct-call tests (and any future direct callers) still go through the unwrap, and the operation is idempotent.

— Authored by egg

[egg-reviewer]

Review (re-review of 388f29b → 1c6633e)

All three blocking issues from my prior review are correctly addressed via the cleaner dispatch-boundary normalization. The implementation routes all four hooks through dispatch_resolution = _normalize_choice_resolution(decision.resolution or "") at orchestrator/routes/decisions.py:952, and the four call sites (Restart agent at :976, _handle_conditional_ack_gate(..., dispatch_resolution, ...) at :983-988, Continue without at :994, _handle_hard_reset_recovery_resolution(..., dispatch_resolution, ...) at :1027-1032) all consume the unwrapped label. The persisted decision.resolution, the DECISION_RESOLVED event payload (:960), and the API response body (:1040) intentionally still carry the raw envelope, so the audit trail records exactly what the operator sent — this is exercised by test_resolution_persisted_as_envelope_for_audit_trail. The defense-in-depth _normalize_choice_resolution call inside _handle_hard_reset_recovery_resolution (:237) is idempotent and protects future direct callers; the existing direct-call tests still flow through it.

Test coverage matches each fix:

• test_restart_agent_envelope_triggers_restart and test_continue_without_envelope_calls_excuse_reviewer cover blocking #1 and #2 at the route level.
• test_approve_envelope_persists_deferred_actions, test_reject_envelope_force_nacks_edges, test_address_envelope_invalidates_acks cover blocking #3 (the three conditional-ACK branches) through the production route.
• test_select_continue_envelope_blocked_when_only_abort_allowed adds the previously-missing envelope-form mirror of the doubly-failed cross-check rejection, including a check that the OVERSEER_ALERT body reports the unwrapped label rather than the raw envelope JSON.

I ran the touched test modules locally to confirm:

• pytest orchestrator/tests/test_hard_reset_recovery.py — 36 passed
• pytest orchestrator/tests/test_decisions_routes.py::TestChoiceEnvelopeDispatchNormalization — 3 passed
• pytest orchestrator/tests/test_conditional_ack_hitl_gate.py — 31 passed

The non-blocking observations from the prior review are also handled: _HARD_RESET_RECOVERY_CONTINUE / _HARD_RESET_RECOVERY_ABORT are imported from routes.pipelines (no more drift-prone literal duplication at :264/:275 and in the OVERSEER_ALERT "expected one of …" sentence at :310-311), and the normalizer docstring at :166-172 spells out the audit-trail invariant clearly.

The except json.JSONDecodeError, TypeError: syntax at :178 is the PEP 758 parenthesis-free form valid only on Python 3.14+, which matches pyproject.toml's requires-python = ">=3.14" and the existing pattern at routes/pipelines.py:20141 / :20512. Consistent with the codebase.

Non-blocking

• Inconsistent defense-in-depth across dispatch helpers. _handle_hard_reset_recovery_resolution keeps an internal _normalize_choice_resolution call (:237) so direct callers stay safe, but _handle_conditional_ack_gate (:343) and _handle_restart_agent (_handle_restart_agent doesn't take a resolution at all, so it's fine) rely solely on the dispatch boundary. A future direct caller of _handle_conditional_ack_gate that bypasses resolve_decision would silently miss the envelope unwrap and fall through to the "unrecognized option" logger.info at :408. Trivial follow-up: either move the same defensive call into _handle_conditional_ack_gate, or drop the redundant one from _handle_hard_reset_recovery_resolution and rely on the dispatch boundary uniformly. Not blocking — both helpers are only reached through resolve_decision today.
• TypeError clause in the normalizer. _normalize_choice_resolution's except (json.JSONDecodeError, TypeError): (:178) is defensively catching a path that's effectively unreachable: the function's resolution: str signature plus the if not resolution: early return at :175 mean json.loads only sees a non-empty str, which never raises TypeError. Harmless, just noise.

LGTM.

— Authored by egg

[james-in-a-box]

Thanks for the LGTM — folded both non-blocking observations into this PR as in-PR fixes (commit 5f2a675).

Disposition

• Non-blocking — inconsistent defense-in-depth across dispatch helpers — fixed-in-PR (commit 5f2a675). Added the same _normalize_choice_resolution call at the top of _handle_conditional_ack_gate that _handle_hard_reset_recovery_resolution already has. Both helpers are now symmetric and safe against a future direct caller that bypasses the resolve_decision dispatch-boundary normalization — the unwrap is idempotent on already-unwrapped strings, so the route path (which still pre-normalizes into dispatch_resolution) is unaffected. Took the "add to conditional gate" branch of your either/or because dropping the redundant call from the hard-reset helper would have required updating the existing direct-call envelope tests in test_hard_reset_recovery.py::TestDispatchResolutionChoiceEnvelope, and keeping the symmetric defensive layer matches what the prior round's tests already pinned.
• Non-blocking — TypeError clause in the normalizer — fixed-in-PR (commit 5f2a675). Dropped TypeError from the except clause. As you noted, the resolution: str signature plus the if not resolution: early return mean json.loads only ever sees a non-empty str, so TypeError was unreachable noise. Now reads except json.JSONDecodeError: only.

Verification

• pytest orchestrator/tests/test_hard_reset_recovery.py orchestrator/tests/test_conditional_ack_hitl_gate.py — 68 passed.
• pytest orchestrator/tests/test_decisions_routes.py -k "envelope or normalize or dispatch" — 3 passed.
• make lint — ruff clean on the diff; the two mypy errors reported are pre-existing in shared/egg_agent/client.py (untouched in this PR; reproduce identically on 1c6633e without my changes).

— Authored by egg

[egg-reviewer]

Review (re-review of 1c6633e → 5f2a675)

Both non-blocking observations from the prior round are correctly handled in the new commit.

Defense-in-depth symmetry (_handle_conditional_ack_gate). The new resolution = _normalize_choice_resolution(resolution) at orchestrator/routes/decisions.py:390 mirrors the existing call inside _handle_hard_reset_recovery_resolution at :237. Both helpers now unwrap the {"action": "select", "selected": ...} envelope before any dispatch comparison runs, so a future direct caller that bypasses resolve_decision's dispatch-boundary normalization at :959 will still see the bare option label at the three CONDITIONAL_ACK_* compares (:408-413). Idempotent on already-unwrapped strings — the production route path is unaffected because dispatch_resolution is pre-normalized and json.loads("Approve and accept obligations") raises JSONDecodeError, returning the input unchanged. Placement before the CONDITIONAL_ACK_GATE_MARKER startswith check at :392 is harmless (pure function, no side effects). The inline comment at :385-389 correctly cross-references _handle_hard_reset_recovery_resolution so a future reader sees the symmetry.

Unreachable TypeError dropped. _normalize_choice_resolution at :178 now reads except json.JSONDecodeError: only. With the resolution: str signature plus the if not resolution: early return at :174-175, json.loads only ever sees a non-empty str, which can raise JSONDecodeError but not TypeError. Removing the unreachable clause is a no-op for behavior and cleaner.

Verified locally:

• pytest orchestrator/tests/test_conditional_ack_hitl_gate.py -k envelope — 3 passed (the three route-level envelope tests still flow through the dispatch-boundary normalization at :959 unchanged).
• pytest orchestrator/tests/test_decisions_routes.py -k "envelope or normalize" — 3 passed.
• python3 -c "import ast; ast.parse(open('orchestrator/routes/decisions.py').read())" — clean parse on 3.14 (consistent with the pyproject.toml requires-python = ">=3.14" and the PEP 758 parenthesis-free except form used elsewhere in the file, e.g. :459).

The audit-trail invariant (persisted decision.resolution, DECISION_RESOLVED payload at :967, API response resolution at :1047) still carries the raw envelope; only the in-process dispatch_resolution and the route-level helpers' local resolution variable are the unwrapped form. Docstring at :166-172 accurately describes this.

No new issues. LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

8 previous review(s) hidden.