Fix HITL gate ignoring human feedback

[james-in-a-box]

Fix HITL gate ignoring human feedback on plan/refine phases

The HITL gate at phase boundaries (refine, plan) unconditionally advanced
to the next phase after the human resolved the decision, regardless of
whether they approved or provided corrective feedback. Human corrections
were silently discarded — the resolution text was never read.

Now the orchestrator checks the decision resolution text after the HITL
gate is cleared. If it's not an approval keyword, the phase is re-run
with the human's corrections injected into the worker prompt via the
existing review_feedback mechanism. This uses the same code path as
agentic reviewer feedback, so the worker sees the human's corrections
in its "Prior Review Feedback" section and revises its draft in-place.

Changes:

• Add hitl_revision_feedback variable to bridge feedback across outer
  loop iterations (since review_feedback resets each iteration)
• After HITL decision resolves, check resolution against approval keywords
• On non-approval: store feedback, increment review_cycles, reset phase
  status to RUNNING, and continue to re-run the phase
• Update HITL gate options from ["approve"] to ["approve", "request changes"]
• Update question text to inform the human they can provide feedback

Issue: #655

Test plan:

• Existing orchestrator tests pass (319/319, 1 pre-existing failure in test_state_store excluded)
• Start a local pipeline, let it reach the plan HITL gate
• Provide corrective feedback instead of approving
• Verify the plan phase re-runs with the feedback in the worker prompt
• Approve on the second pass and verify the pipeline advances normally

Authored-by: egg

[egg-reviewer]

Review: Fix HITL gate ignoring human feedback

The approach is sound — using the existing review_feedback mechanism to inject HITL corrections is a clean design choice. However, there are several issues that need to be addressed before this is production-ready.

---

1. No circuit breaker on HITL revision loop (Bug)

File: orchestrator/routes/pipelines.py, line 2432

The inner review cycle enforces max_review_cycles (line 2309-2318) as a circuit breaker to prevent infinite loops. The new HITL revision path increments review_cycles (line 2432) and continues the outer loop, but never checks the limit. A human who repeatedly clicks "request changes" will cause infinite container spawns with no upper bound.

This is especially dangerous because each iteration spawns a full agent container (_spawn_and_wait), consuming compute resources indefinitely.

Fix: Check max_review_cycles before continuing:

phase_execution.review_cycles += 1
if phase_execution.review_cycles >= pipeline.config.max_review_cycles:
    logger.warning(
        "HITL revision circuit breaker — advancing despite feedback",
        pipeline_id=pipeline_id,
        phase=current_phase.value,
        review_cycles=phase_execution.review_cycles,
    )
    # Fall through to the approval path below
else:
    store.save_pipeline(pipeline)
    report_pipeline_status(...)
    continue

---

2. "request changes" option without feedback is a no-op revision (Bug)

File: orchestrator/routes/pipelines.py, line 2418-2427

When the human selects the "request changes" checkbox on a GitHub issue comment without writing additional text, the resolution is literally the string "request changes" (extracted by the sdlc-hitl.yml workflow from the checked option label). This string is not in _APPROVE_KEYWORDS, so it triggers a full phase re-run.

But hitl_revision_feedback will be "request changes" — the worker sees "request changes" in its "Prior Review Feedback" section with zero actionable guidance. It will re-run the entire phase and likely produce identical output, then hit the HITL gate again in an endless loop (compounded by issue #1 above).

Fix: Either:

• (a) Add "request changes" to the detection and prompt the human for actual feedback text, or
• (b) Treat bare option labels ("request changes", "Request Changes", etc.) as needing clarification and re-queue a follow-up decision asking for specifics, or
• (c) At minimum, log a warning when the feedback is very short and likely just an option label.

---

3. _populate_contract_from_plan called on every re-run (Bug)

File: orchestrator/routes/pipelines.py, lines 2359-2362 and 2440

When the plan phase completes, _populate_contract_from_plan (line 2360) runs before the HITL gate (line 2365). If the human requests changes, the continue at line 2440 re-enters the outer loop, the plan phase re-runs, completes again, and _populate_contract_from_plan is called a second time.

That function overwrites contract.phases entirely (contract.phases = [phase]), discarding any task status changes or metadata that may have been written between runs. For the plan phase this is likely benign since nothing else writes to the contract yet, but it's a latent bug if the contract lifecycle changes, and it does redundant work on every revision.

Fix: Guard the call:

if current_phase.value == "plan" and phase_execution.review_cycles == 0:
    _populate_contract_from_plan(...)

Or make _populate_contract_from_plan idempotent by merging rather than overwriting.

---

4. completed_at not reset on re-run (Minor)

File: orchestrator/routes/pipelines.py, lines 2343-2345 and 2430-2431

Before the HITL gate, the phase is marked COMPLETE with completed_at = datetime.utcnow() (line 2344). When human requests changes, the code sets phase_execution.status = PipelineStatus.RUNNING (line 2431) but does not reset completed_at. The phase execution will have status=RUNNING with a stale completed_at timestamp, which could confuse monitoring/dashboards and produce incorrect duration calculations.

Fix: Reset completed_at when re-running:

phase_execution.status = PipelineStatus.RUNNING
phase_execution.completed_at = None  # Reset — phase is re-running
phase_execution.review_cycles += 1

---

5. _APPROVE_KEYWORDS set recreated on every outer loop iteration (Nit)

File: orchestrator/routes/pipelines.py, line 2418

_APPROVE_KEYWORDS is defined inside the HITL gate block, which runs on every outer loop iteration. The leading underscore convention suggests module-level intent. Move it to module scope alongside _HITL_GATE_PHASES for clarity and to avoid re-creation.

---

Summary

Issues #1 and #2 together create a scenario where a human clicks "request changes" without typing feedback, triggering an unbounded loop of container spawns producing identical output. This needs to be fixed before merge. Issues #3 and #4 are less severe but should also be addressed.

— Authored by egg

[james-in-a-box]

All five items addressed in 8731c04. Summary:

#1 — Circuit breaker: Added max_review_cycles check before the continue. When the limit is hit, we log a warning and fall through to the approval path instead of looping forever.

#2 — Bare option label: When the resolution is just "request changes" (a bare option label with no actionable text), we now re-queue a follow-up decision asking the human for specifics. If the follow-up is also bare or an approval, we treat it as approval rather than entering a no-op loop.

#3 — _populate_contract_from_plan guard: Now only called when phase_execution.review_cycles == 0, so it runs exactly once on the first successful plan completion. Subsequent re-runs after HITL feedback skip it.

#4 — completed_at reset: Added phase_execution.completed_at = None when setting the phase back to RUNNING for a revision re-run.

#5 — Module-scope constants: Moved _APPROVE_KEYWORDS to module scope alongside _HITL_GATE_PHASES. Also added _BARE_OPTION_LABELS at module scope for the bare option detection logic from #2.

Tests: 342/343 pass (1 pre-existing failure in test_state_store). Ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: Address review feedback on HITL revision loop

Checked all five original items against the delta (d8c109e → 8731c04). Items #1, #3, #4, and #5 are properly addressed. Item #2 is handled well with the follow-up decision mechanism. One new bug introduced by the fix, and one minor issue.

---

1. Circuit breaker fall-through leaks hitl_revision_feedback into the next phase (Bug)

File: orchestrator/routes/pipelines.py, lines 2474 and 2484–2493

When the circuit breaker fires (line 2484), execution falls through to the "Approved — resume and advance" path (line 2503). But hitl_revision_feedback was already set at line 2474:

hitl_revision_feedback = resolution           # line 2474 — always set
# ...
if phase_execution.review_cycles >= max_cycles:
    # ...
    store.save_pipeline(pipeline)
    # Fall through to the approval path below   # line 2493

At the top of the next outer loop iteration (line 1958):

review_feedback: str | None = hitl_revision_feedback
hitl_revision_feedback = None

The stale HITL feedback from the previous phase's circuit-breaker exit is injected as review_feedback for the next phase's worker. The next phase's worker will receive revision feedback that was intended for the phase that was force-advanced.

Fix: Clear hitl_revision_feedback in the circuit-breaker branch, before falling through:

if phase_execution.review_cycles >= max_cycles:
    logger.warning(...)
    hitl_revision_feedback = None  # Don't leak feedback to next phase
    store.save_pipeline(pipeline)
    # Fall through to the approval path below

Or move the hitl_revision_feedback = resolution assignment into the else branch (line 2494) next to the continue, since that's the only path where the feedback is actually consumed.

---

2. Follow-up timeout path reads unresolved decision (Minor)

File: orchestrator/routes/pipelines.py, lines 2443–2452

When wait_for_decision raises DecisionTimeoutError, the code catches it and then immediately calls dq.get_decision(followup.id) at line 2451. A timed-out decision has resolution = None, so (resolved_followup.resolution or "").strip() evaluates to "", which is in _APPROVE_KEYWORDS, so it silently approves. This works correctly as a default behavior.

However, the logic relies on "" being in _APPROVE_KEYWORDS as a side effect — the empty string was originally included to handle the case where a human selects "approve" without additional text. The timeout-as-approval behavior is the same implicit path but for a different reason. A comment at line 2445–2450 noting that timeout intentionally falls through to approval would prevent future confusion if someone removes "" from _APPROVE_KEYWORDS.

This is non-blocking — just a clarity improvement.

---

Previous feedback status

#	Issue	Status
1	Circuit breaker on HITL revision loop	Fixed — max_review_cycles checked before continue
2	Bare option label → no-op revision	Fixed — follow-up decision queued for specifics
3	_populate_contract_from_plan called on re-run	Fixed — guarded with review_cycles == 0
4	completed_at not reset on re-run	Fixed — completed_at = None on revision
5	_APPROVE_KEYWORDS in loop body	Fixed — moved to module scope

Issue #1 above (feedback leak) is the only blocking item.

— Authored by egg

[james-in-a-box]

Both items addressed in d889a7a.

#1 — Feedback leak on circuit breaker fall-through: Moved the hitl_revision_feedback = resolution assignment from before the circuit breaker check into the else branch (the continue path). The circuit-breaker fall-through path no longer sets hitl_revision_feedback, so stale feedback cannot leak into the next phase's worker.

#2 — Timeout-as-approval comment: Added a comment in the DecisionTimeoutError handler explaining that timeout intentionally resolves to "" (via None → "") which is in _APPROVE_KEYWORDS, documenting the implicit approval-on-timeout behavior.

Tests: 319/320 pass (1 pre-existing failure in test_state_store). Ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: Fix HITL circuit breaker leaking feedback into next phase (d889a7a)

Checked both items from the previous review against the delta (8731c04 → d889a7a). Both are properly addressed. One new bug introduced by the fix.

---

Previous feedback status

#	Issue	Status
1	Circuit breaker leaking hitl_revision_feedback into next phase	Fixed — assignment moved into the else branch (line 2498), circuit-breaker path no longer sets it
2	Timeout-as-approval comment	Fixed — comment at lines 2446–2449 documents the implicit behavior

---

1. Circuit breaker fall-through leaves completed_at = None on the final phase record (Bug)

File: orchestrator/routes/pipelines.py, lines 2482, 2495, 2507–2513

When the circuit breaker fires, the code at lines 2481–2482 sets:

phase_execution.status = PipelineStatus.RUNNING
phase_execution.completed_at = None  # Reset — phase is re-running

This is saved at line 2495. The code then falls through to the "Approved" path (lines 2507–2513), which reloads the pipeline and restores status = COMPLETE but does not restore completed_at:

pipeline = store.load_pipeline(pipeline_id)          # line 2508 — loads completed_at=None
pipeline.status = PipelineStatus.RUNNING
phase_execution = pipeline.get_phase_execution(current_phase)
phase_execution.status = PipelineStatus.COMPLETE      # line 2512 — status fixed
# completed_at is still None                          # ← missing
store.save_pipeline(pipeline)

The phase ends up persisted as status=COMPLETE, completed_at=None. This will break any duration calculations (completed_at - started_at) and confuse monitoring dashboards.

This only affects the circuit-breaker path (not the normal approval path), since the normal approval flow never clears completed_at.

Fix: Restore completed_at in the approval path when coming from the circuit breaker:

# Approved — resume and advance
pipeline = store.load_pipeline(pipeline_id)
pipeline.status = PipelineStatus.RUNNING
phase_execution = pipeline.get_phase_execution(current_phase)
phase_execution.status = PipelineStatus.COMPLETE
if phase_execution.completed_at is None:
    phase_execution.completed_at = datetime.utcnow()
store.save_pipeline(pipeline)

---

Summary

The two items from the last review are correctly fixed. The feedback leak is resolved by scoping the assignment to the continue path, and the timeout behavior is documented.

The new issue (#1) is a direct consequence of the fix for the original review's item #4 (completed_at reset). The reset was correct for the continue path (where the phase genuinely re-runs), but the circuit-breaker fall-through path does not re-complete the phase — it force-advances. That path needs to restore completed_at since the phase won't go through the normal completion flow again.

— Authored by egg

[james-in-a-box]

Addressed in b481ea8.

#1 — Circuit breaker leaves completed_at = None: Added a guard in the "Approved — resume and advance" path that restores completed_at if it's None. When coming from the circuit-breaker fall-through (where completed_at was cleared for the re-run that never happened), completed_at is now set to datetime.utcnow(). On the normal approval path, completed_at is already set from when the phase originally completed, so the guard is a no-op.

Tests: 3226/3226 pass (81 skipped). Ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: Restore completed_at on circuit breaker fall-through (b481ea8)

Checked the delta since d889a7a. The single change adds a guard at lines 2513–2514:

if phase_execution.completed_at is None:
    phase_execution.completed_at = datetime.utcnow()

This correctly handles the circuit-breaker fall-through path where completed_at was cleared at line 2482 for a re-run that never happens. On the normal approval path, completed_at is already set from line 2350, so the guard is a no-op. Correct fix.

---

Previous feedback status

#	Issue	Status
1	Circuit breaker on HITL revision loop	Fixed
2	Bare option label → no-op revision	Fixed — follow-up decision mechanism
3	_populate_contract_from_plan called on re-run	Fixed — guarded with review_cycles == 0
4	completed_at not reset on re-run	Fixed
5	_APPROVE_KEYWORDS in loop body	Fixed — module scope
6	Feedback leak on circuit breaker fall-through	Fixed — assignment scoped to else branch
7	Timeout-as-approval comment	Fixed
8	Circuit breaker leaves completed_at = None	Fixed — guard at line 2513

All eight issues from previous reviews are properly addressed.

---

Advisory: shared review_cycles budget (non-blocking)

The inner agentic review loop (line 2330) and the HITL revision path (line 2483) both increment the same phase_execution.review_cycles counter, checked against the same max_review_cycles limit (default 3). If the inner loop consumes 2 cycles on agentic review, the human gets at most 1 HITL revision before the circuit breaker fires.

This is an existing design choice and not introduced by this PR, but worth noting since the HITL feedback feature makes it user-visible — a human who provides valid feedback may hit the circuit breaker not because they've been excessive, but because the agentic reviewer consumed most of the budget. Consider separate counters or a dedicated HITL revision budget in a follow-up.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the thorough review. All eight issues confirmed fixed.

Re: the advisory on shared review_cycles budget — agreed this is worth addressing. The inner agentic loop consuming cycles that reduce the human's HITL revision budget is a poor UX, especially now that HITL revisions are a real feature. I'll track a follow-up to add a separate hitl_review_cycles counter with its own limit so the two budgets don't interfere.

Tests: 3226/3226 pass (81 skipped). Ruff clean.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

10 previous review(s) hidden.