Fix #1889 + #1890: bridge contract decisions at phase_gate; harden plan HITL gate

[jwbron]

Summary

Two related HITL-gate bugs surfaced on the issue-1762-membump /
issue-1765-membump pipelines. Both cases were that substantive
human-facing work got silently dropped between BRC consensus and the
pipeline advancing past a phase_gate approval.

Closes #1889 and #1890.

#1889 — Refiner-registered decisions/feedback silently discarded at phase_gate

When an agent called egg-contract add-decision / egg-contract add-feedback,
those entries only mutated the contract JSON at
.egg-state/contracts/{identifier}.json. The orchestrator's decision queue
never saw them, so approving the phase_gate via HTTP/MCP (the /sdlc skill's
Phase 4 path) advanced the pipeline with the questions unanswered. The
terminal HITL handler bridged these via the [q] Answer open questions menu,
but the API path didn't — a real asymmetry.

Fix: added _queue_and_await_contract_decisions in
orchestrator/routes/pipelines.py. After phase_gate approval (and before
_persist_phase_gate_resolution), the helper:

1. Reads the contract from the worktree.
2. Promotes any unresolved type=hitl decision (scoped to the current phase)
   into an orchestrator choice decision.
3. Promotes any unsubmitted feedback (scoped to the current phase) into an
   orchestrator feedback decision.
4. Waits for each via the existing decision queue, then writes the
   resolution / answers back to the contract so next-phase agents see them.

This matches the issue's "Expected Behaviour A — surface them as additional
pending_decisions after the phase_gate resolves." Wrapped in a try/except
at the call site so a bridge bug can never strand the pipeline.

#1890 — Plan phase_gate not auto-created; bus pollution

Two independent issues on the same run:

a) Uncaught-exception escape skipped the HITL gate. _populate_contract_from_plan
and _sync_pipeline_decisions_to_contract have internal try/except blocks,
but an escape from either (e.g. an OSError inside save_contract on a large
38-task plan) would propagate and skip the HITL-gate block below, leaving
the pipeline stalled until the overseer intervened. Fix: wrapped both call
sites in try/except, matching the pattern the adjacent helpers
(_commit_statefiles_to_worktree, push_worktree_branch) already use.

b) egg-orch consensus confirmed was not idempotent. The CLI's retry
loop (for i in 1..10; do egg-orch consensus confirmed; done) emitted a
fresh CONSENSUS_CONFIRMED message on every call, spamming the bus and
making BRC replay transcripts noisy. Fix: handle_consensus_confirmed_signal
now consults the message store before writing. A role that has already
emitted a final CONFIRMED in the current phase returns idempotent: true
without duplicating the write. The pending_acks path dedupes similarly,
and the pending → final transition still writes once. The tracker's
handle_confirmed is still called every time so its own state stays in
sync.

(Not addressed in this PR: the overseer respawn cascade at max_respawns=3
and the plan_phase_gate_not_created label being LLM-generated. Both are
separate concerns; fixing the underlying bug here reduces how often the
overseer needs to intervene in the first place.)

Test plan

• pytest tests/test_brc_phase_propagation.py tests/test_decision_queue.py tests/test_hitl_revision.py tests/test_consensus.py — all pass (138/138)
• pytest tests/test_contract_decision_bridge.py — 4 new tests, all pass
• pytest tests/test_consensus_confirmed_idempotent.py — 4 new tests, all pass
• ruff check + ruff format pre-commit hooks clean
• Re-run a full /sdlc pipeline where the refiner registers decisions and verify they surface as individual pending_decisions after phase_gate approval
• Confirm plan-phase HITL gate auto-fires on next large-plan pipeline run

🤖 Generated with Claude Code

[egg-reviewer]

Review: Fix #1889 + #1890

Thorough review of both fixes and new tests. Overall the changes are well-designed — the contract decision bridge closes a real asymmetry between the terminal and API paths, and the idempotency logic for CONSENSUS_CONFIRMED is the right approach. I have one issue to fix and several non-blocking suggestions.

---

[Blocking] Missing type == "hitl" filter on contract decisions

orchestrator/routes/pipelines.py:8792-8796

The PR description says: "Promotes any unresolved type=hitl decision (scoped to the current phase)" — but the code doesn't filter by decision type:

pending_decisions = [
    d
    for d in contract.decisions
    if not d.resolved and (d.phase is None or getattr(d.phase, "value", d.phase) == phase_value)
]

The contract Decision model has a type: DecisionType field with both HITL = "hitl" and AUTO = "auto" values. While currently egg-contract add-decision only creates HITL decisions, the model allows AUTO decisions, and future code paths could introduce them. An AUTO decision promoted as a human choice decision would be wrong — the human would be asked to answer something the system should resolve automatically.

Fix:

pending_decisions = [
    d
    for d in contract.decisions
    if not d.resolved
    and getattr(d.type, "value", d.type) == "hitl"
    and (d.phase is None or getattr(d.phase, "value", d.phase) == phase_value)
]

---

[Non-blocking] Feedback not marked as submitted when answer format doesn't parse

orchestrator/routes/pipelines.py:8883-8910

If the feedback orchestrator decision is resolved but the resolution JSON doesn't match the expected {"answers": {...}} structure (e.g., the human types a freeform string, or the format changes), answers stays empty and the contract feedback is never marked as submitted. The orchestrator considers the decision resolved, but the contract still shows the feedback as pending.

Consequence: on a subsequent phase gate (or manual bridge invocation), the same feedback would be surfaced again as a new orchestrator decision. The human answered already — they shouldn't be asked twice.

Suggestion: mark submitted = True as the default path after resolution, and only fill individual question answers if the format parses:

if resolved.status == DecisionStatus.RESOLVED:
    answers: dict[str, str] = {}
    try:
        payload = json.loads(resolved.resolution or "")
        if isinstance(payload, dict):
            raw_answers = payload.get("answers")
            if isinstance(raw_answers, dict):
                answers = {str(k): str(v) for k, v in raw_answers.items()}
    except (json.JSONDecodeError, TypeError):
        pass

    fb_id = pending_feedback.id

    def _apply_fb(
        latest: Any, _fb_id: str = fb_id, _answers: dict[str, str] = answers
    ) -> bool:
        if latest.feedback is None or latest.feedback.id != _fb_id:
            return False
        for q in latest.feedback.questions:
            if q.id in _answers:
                q.answer = _answers[q.id]
        latest.feedback.submitted = True
        latest.feedback.submitted_by = "human"
        latest.feedback.submitted_at = datetime.now(UTC)
        return True

    _save_contract_update(_apply_fb)

This way the feedback is always marked submitted after the human responds, even if we can't extract individual question answers.

---

[Non-blocking] _existing_confirmed_for_role loads up to 10,000 messages

orchestrator/routes/signals.py:1265

messages = store.get_messages(pipeline_id, limit=10000)

get_messages with a limit returns the newest N messages. If a pipeline has >10,000 messages and the CONFIRMED was emitted early, it could be missed. This causes a duplicate write (false negative in idempotency), which is the safe failure direction — but worth a brief comment explaining the tradeoff so a future reader doesn't "fix" this by lowering the limit.

---

[Non-blocking] Phase-null messages match any phase in idempotency check

orchestrator/routes/signals.py:1277-1279

msg_phase = getattr(m, "phase", None)
if phase is not None and msg_phase is not None and msg_phase != phase:
    continue

If msg_phase is None but phase is not, the message is counted as a match. In practice this shouldn't occur because all CONSENSUS_CONFIRMED writes set a phase, but the logic could cause a false positive across phase transitions if a message somehow has a null phase — the idempotency check would prevent the first legitimate write for a new phase.

This is unlikely to bite in practice since the code paths always set a phase, but an explicit comment would help.

---

[Non-blocking] Redundant _resolve_pipeline_phase call

orchestrator/routes/signals.py:1417

In the tracker-present path, _resolve_pipeline_phase is called to get current_phase. This loads the pipeline from the state store. But the pipeline was already loaded earlier at line ~1304 (store.load_pipeline(pipeline_id) via the tracker reconstruction path). If the tracker is found on the first try (common case), there's no prior load, so this is fine. But if the tracker was reconstructed, the pipeline is loaded twice.

Minor efficiency concern, not worth changing.

---

Tests

The new test files are well-structured:

• test_contract_decision_bridge.py: Good coverage of the happy path (decisions promoted), feedback promoted, phase scoping, and missing-contract no-op. The _FakeQueue helper is a clean stand-in.
• test_consensus_confirmed_idempotent.py: Tests all four key scenarios — first write, duplicate final, duplicate pending_acks, and pending-to-final transition.
• The existing test_confirmed_fallback_uses_local_phase change is correct — removing coder from the existing messages prevents the new idempotency check from short-circuiting the fallback write that the test exercises.

---

Summary

The fixes address real production bugs. The contract decision bridge (#1889) closes an important asymmetry, and the confirmed-signal idempotency (#1890) prevents bus pollution from agent retry loops. The try/except hardening around _populate_contract_from_plan / _sync_pipeline_decisions_to_contract is a sound defensive measure.

The type filter on contract decisions is the one thing to fix before merge. The feedback persistence suggestion is worth considering but not blocking.

— Authored by egg

[james-in-a-box]

All feedback addressed in eafe8b8:

[Blocking] Missing type == "hitl" filter — Fixed. pending_decisions now filters by getattr(d.type, "value", d.type) == "hitl". Added test_bridge_skips_auto_decisions to cover AUTO exclusion.

[Non-blocking] Feedback not marked as submitted on unparseable resolution — Agreed. Removed the if answers: guard so feedback is always marked submitted after the human responds, regardless of whether individual answers parse. Added test_bridge_marks_feedback_submitted_on_unparseable_resolution.

[Non-blocking] 10k message limit tradeoff — Added clarifying comment explaining the safe failure direction.

[Non-blocking] Phase-null matching behavior — Added comment explaining why null msg_phase is treated as a match (conservative choice).

[Non-blocking] Redundant _resolve_pipeline_phase call — Acknowledged, not changing per review recommendation.

All 148 tests pass (138 existing + 10 new, including the 2 added in this commit). Ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: Fix #1889 + #1890 (eafe8b8)

All feedback from the first review has been properly addressed. No new issues found in the delta.

---

Verification of Previously Raised Issues

[Blocking] Missing type == "hitl" filter — Fixed. The pending_decisions comprehension now includes getattr(d.type, "value", d.type) == "hitl". test_bridge_skips_auto_decisions covers the exclusion. Correct.

[Non-blocking] Feedback not marked submitted on unparseable resolution — Fixed. The if answers: guard is removed; _save_contract_update(_apply_fb) now runs unconditionally after resolution. test_bridge_marks_feedback_submitted_on_unparseable_resolution covers the edge case (freeform string resolution → submitted=True, individual answers remain None). Correct.

[Non-blocking] 10k message limit — Comment added at the get_messages call explaining the safe-failure-direction tradeoff. Clear and accurate.

[Non-blocking] Phase-null matching — Comment added at the phase comparison explaining the conservative choice. Clear.

[Non-blocking] Redundant _resolve_pipeline_phase — Acknowledged. The refactored tracker-present path now calls _resolve_pipeline_phase once and reuses current_phase for both pending_acks and final confirmed paths — an improvement over the original code's two calls.

---

Delta Review (af4d130 → eafe8b8)

The delta is tight: type filter addition, feedback persistence fix, two clarifying comments, and two new tests. No unrelated changes, no regressions.

Confirmed that save_contract uses atomic writes (temp file + os.rename), so the reload-mutate-save pattern in _save_contract_update is safe against corruption. The TOCTOU window between reload and save is a pre-existing architectural property of the contract system, not introduced here, and the risk is low given single-pipeline-thread execution context.

Test coverage: 10 new tests across two files covering the happy path, edge cases (auto decisions, unparseable feedback, phase scoping, missing contract), and all four idempotency scenarios. The existing test_confirmed_fallback_uses_local_phase adjustment (removing coder from prior messages) is correct — prevents the new idempotency check from short-circuiting the fallback path under test.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

3 previous review(s) hidden.