docs: update divergence HITL docs for non-destructive reconcile [doc-updater] - #2988
Conversation
…2979) Replace the stale "Hard-Reset Recovery" section in docs/hitl-decisions.md with accurate documentation of the non-destructive reconcile approach introduced by #2979: - Rename section and remove references to the destructive hard-reset path - Update pipeline status from FAILED → AWAITING_HUMAN - Update HITL context key: hard_reset_recovery → divergence_reconcile_unacked - Update options table: "Continue with post-reset state" → "Reconciled — resume" - Rewrite recovery steps: operator now manually reconciles the worktree first - Remove the "doubly-failed case" (no longer possible; the sync never hard-resets) - Update populate_contract HTTP 409 reason codes and note idempotency Triggered by: d40243d (fix(orchestrator): non-destructive worktree-divergence reconcile (#2979) (#2986)) Authored-by: egg
There was a problem hiding this comment.
Review summary
Doc-only update to docs/hitl-decisions.md reflecting the non-destructive reconcile introduced by #2979 / #2986. I traced every claim in the diff against the code in orchestrator/routes/pipelines.py and orchestrator/routes/phases.py and the rewrite is accurate. No blocking issues. A few non-blocking suggestions to tighten the doc are below.
Verification
| Doc claim | Code site | Verdict |
|---|---|---|
Pipeline pinned to AWAITING_HUMAN, not FAILED |
pipelines.py:14260, 14414 |
✓ |
HITL context = "divergence_reconcile_unacked" |
pipelines.py:14137 |
✓ |
Options: Reconciled — resume, Abort pipeline |
pipelines.py:14124–14129 (em-dash matches) |
✓ |
| Worktree left at local HEAD, committed work intact (autoresolve aborts) | pipelines.py:7206–7229 ("the autoresolve has already run git rebase --abort…") |
✓ |
Backup ref created via git update-ref (not pushed) |
pipelines.py:7244–7250 |
✓ |
Abort → pipeline FAILED; backup ref preserves commits |
pipelines.py:14288–14342 (_fail_pipeline_after_divergence_abort) |
✓ |
3 fire sites: phase start / post-phase / populate_contract |
pipelines.py:20939, pipelines.py:22301, phases.py:1402 |
✓ |
populate_contract returns HTTP 409 with reason="divergence_reconcile_unacked" |
phases.py:1343, 1415 |
✓ |
populate_contract idempotency (dedupe via existing pending decision) |
phases.py:1329–1349 via _find_pending_divergence_reconcile_decision |
✓ |
Removed "doubly-failed" branch and sync_rebase_and_reset_failed reason |
No references remain in production code (grep confirms) |
✓ |
Non-blocking suggestions
-
Reconciled — resumeis not auto-resume for thepopulate_contractsite. The options table says "Orchestrator re-runs the worktree sync and resumes the phase's post-processing from where it paused." That's accurate for the two_run_pipelinefire sites (which block onwait_for_decisioninside_sync_worktree_reconciling_divergenceand continue once the operator resolves), but_emit_divergence_reconcile_hitl(used bypopulate_contract) returns immediately after persisting the HITL — there is no listener that transitions the pipeline back toRUNNINGon resolution. For thepopulate_contractpath the operator must re-POSTpopulate_contractafter reconciling; the HITL resolution alone is inert. Worth a sentence in thepopulate_contractbullet, e.g. "Unlike the in-loop fire sites, resolvingReconciled — resumedoes not auto-resume — the operator must re-callpopulate_contractagainst the reconciled worktree." -
Pause budget is undocumented.
pipelines.py:14122defines_MAX_DIVERGENCE_RECONCILE_PAUSES = 3. If the operator picksReconciled — resumethree times without actually reconciling the worktree,_sync_worktree_reconciling_divergencegives up and the caller calls_fail_pipeline_after_divergence_abortwithbudget_exhausted=True, marking the pipelineFAILED. The "Abort pipeline" row of the options table is the only placeFAILEDshows up — operators reading this doc would not know that repeated unresolvable resumes also terminate asFAILED. Consider adding a short note under the options table, e.g. "After 3 unresolved resume attempts the pipeline is markedFAILEDwithreason="…reconcile pause budget was exhausted"; the backup ref is preserved either way." -
Helper attribution is slightly imprecise. The opening sentence says "The process … happens inside
_sync_worktree_with_remoteand its callers in_run_pipelineorpopulate_contract". In practice steps 1–3 (enumerate, backup ref, leave at HEAD) are in_sync_worktree_with_remote, step 4 (AWAITING_HUMAN+ HITL persistence) lives in_sync_worktree_reconciling_divergence(blocking,_run_pipelinesites) or_emit_divergence_reconcile_hitl(non-blocking,populate_contract). The prior version of the section enumerated this split per step; the new version collapsed it. Naming the two callers explicitly would help an operator who needs to follow the code.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address three non-blocking suggestions from the review on #2988: - Clarify helper attribution: steps 1-3 happen in _sync_worktree_with_remote; step 4 lives in _sync_worktree_reconciling_divergence (blocking, _run_pipeline) or _emit_divergence_reconcile_hitl (non-blocking, populate_contract) - Note that auto-resume on 'Reconciled — resume' only applies to the two _run_pipeline fire sites; the populate_contract site requires the operator to re-POST against the reconciled worktree - Document the pause budget: 3 unresolved resume attempts mark the pipeline FAILED with reason '…the reconcile pause budget was exhausted' Authored-by: egg
Feedback addressedThanks for the careful read. All three non-blocking suggestions were factual doc gaps, so I applied them in PR rather than deferring. Pushed as 1a19f1e.
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review summary
The delta from 9db34f3 → 1a19f1e is a single doc-only commit that addresses all three non-blocking suggestions from my prior review. I traced each new claim back to the code in orchestrator/routes/pipelines.py and orchestrator/routes/phases.py; the rewrite is accurate. Approving.
Delta verification
| Suggestion | New text | Code anchor | Verdict |
|---|---|---|---|
#1 — Reconciled — resume is not auto-resume for populate_contract |
"Auto-resume only applies to the two _run_pipeline fire sites … The populate_contract site uses the non-blocking _emit_divergence_reconcile_hitl and returns immediately — resolving Reconciled — resume there is inert; the operator must re-POST populate_contract against the reconciled worktree." |
_emit_divergence_reconcile_hitl docstring at orchestrator/routes/pipelines.py:14240–14252 ("Used by the non-blocking populate_contract route … The operator reconciles the worktree, resolves this decision, and re-runs populate_contract against the now-reconciled worktree.") and _sync_worktree_reconciling_divergence docstring at pipelines.py:14379–14381 ("Only call this from inside the _run_pipeline loop thread, which is allowed to block; route handlers that cannot block use _emit_divergence_reconcile_hitl instead.") |
✓ |
| #2 — Pause budget + FAILED outcome | "After 3 unresolved Reconciled — resume attempts the pipeline is marked FAILED with reason=\"…the reconcile pause budget was exhausted\" (_MAX_DIVERGENCE_RECONCILE_PAUSES = 3 in pipelines.py). The backup ref is preserved either way." |
Constant defined at pipelines.py:14122 (_MAX_DIVERGENCE_RECONCILE_PAUSES = 3); budget loop at pipelines.py:14396–14406 returns aborted=True when pauses >= max_reconcile_pauses; reason string literal at pipelines.py:14313–14317 ("the reconcile pause budget was exhausted"); backup ref preservation via _fail_pipeline_after_divergence_abort (no ref deletion in the FAILED path) |
✓ |
| #3 — Helper attribution split | "Steps 1–3 happen inside _sync_worktree_with_remote; step 4 happens in one of two callers — _sync_worktree_reconciling_divergence (blocking, used by the _run_pipeline loop) or _emit_divergence_reconcile_hitl (non-blocking, used by the populate_contract route)" |
_sync_worktree_reconciling_divergence calls _sync_worktree_with_remote at pipelines.py:14386–14394 and then pins to AWAITING_HUMAN + emits the HITL at pipelines.py:14412–14433; _emit_divergence_reconcile_hitl pins + emits at pipelines.py:14254–14275; call sites confirmed at pipelines.py:20939, pipelines.py:22301 (in-loop) and phases.py:1402 (populate_contract) |
✓ |
Other checks
- Diff is scoped to
docs/hitl-decisions.md. No code changes, no test changes required. - The ellipsis in
"…the reconcile pause budget was exhausted"is the right convention — the actual error message wraps the constant in"Worktree diverged from origin at <phase> and could not be auto-reconciled; <reason> (#2979). …"(pipelines.py:14318–14323), so the leading…signals "substring within a larger error string" rather than mis-quoting the literal. - No new claims to scrutinise beyond the three above; the rest of the section is unchanged from 9db34f3.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
Update
docs/hitl-decisions.mdto reflect the non-destructive worktree-divergence reconcile introduced by #2979 (#2986).The "Sync Divergence: Hard-Reset Recovery" section described the old behavior where a failed rebase autoresolve triggered a destructive
git reset --hardand pinned the pipeline toFAILED. That code was removed in #2979. The updated section describes the new behavior:AWAITING_HUMANinstead ofFAILEDhard_reset_recovery:<phase>→divergence_reconcile_unackedsync_rebase_and_reset_failedreason code removed (no longer possible)populate_contractHTTP 409 reason updated fromhard_reset_recovery_unackedtodivergence_reconcile_unacked, with idempotency noteTriggered by: d40243d (fix(orchestrator): non-destructive worktree-divergence reconcile (#2979) (#2986))
Issue: none
Test Plan
orchestrator/routes/pipelines.pyconstants (_DIVERGENCE_RECONCILE_RESUME,_DIVERGENCE_RECONCILE_ABORT,_DIVERGENCE_RECONCILE_HITL_CONTEXT) and the new HITL question builderAuthored-by: egg