docs: update worktree divergence reconcile description [doc-updater] - #2365
Conversation
Update the _sync_worktree_with_remote() section in orchestrator.md to reflect that diverged worktrees are now reconciled via rebase (using the same _rebase_with_agent_output_autoresolve helper as the gateway-side push-reject path) rather than the old --ff-only merge attempt. Also notes the base_branch=None fallback warning. Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review summary
Documentation-only PR. Two lines updated in docs/architecture/orchestrator.md to align with the post-#2337 behavior of _sync_worktree_with_remote(). I verified each claim against orchestrator/routes/pipelines.py:5232-5519 and orchestrator/gateway_client.py:1998-2277.
Verified accurate
- Trigger context ("orchestrator restarts and phase boundaries"): matches the function's own docstring at
orchestrator/routes/pipelines.py:5242-5243("After an orchestrator restart or a phase boundary, the local worktree branch may be behind the remote"). - Helper name
_rebase_with_agent_output_autoresolve: matches the call site atorchestrator/routes/pipelines.py:5493. Lives inorchestrator/gateway_client.py:1998and is the same helper invoked by the gateway-side push-reject reconcile path, as the doc claims. - Failure handling ("the error is logged and the function returns"): matches
orchestrator/routes/pipelines.py:5509-5519—divergence_rebase_failedis logged at error level and the function returns without falling through to the step-4 reset. base_branch=Nonefallback: the warning emit atorchestrator/routes/pipelines.py:5479-5485and the actual bare-form fallback in_build_rebase_cmdatorchestrator/gateway_client.py:2261-2262confirm both the warning and the contamination risk. The #2222 reference matches the in-code comment atorchestrator/routes/pipelines.py:5466-5478andorchestrator/gateway_client.py:2253-2254.
Both real callers (orchestrator/routes/pipelines.py:14169 and :15396) thread pipeline.base_branch, so the base_branch=None path is a defensive corner case. Documenting it is appropriate — the warning is real and an operator hitting that log line will want to know what it means.
Non-blocking suggestion
The summary sentence directly above the bullet list (unchanged by this PR) still reads:
"then resolves divergence and resets the local branch to
origin/<branch>viagit reset --hardwhen needed."
After #2337, the divergence case no longer resets — it rebases. The "when needed" hedge covers it loosely, and the bullet list directly below is accurate, so this isn't wrong, just slightly imprecise. If you wanted to tighten in a follow-up:
"then reconciles divergence (rebasing local commits) or resets the local branch to
origin/<branch>depending on the case described below."
Not blocking — the bullets carry the load.
Verdict
No blocking issues. The PR is a faithful, scoped doc-sync for #2337. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Per non-blocking review suggestion on #2365: the summary sentence above the bullet list still described 'reset the local branch to origin/<branch> via git reset --hard when needed', which is imprecise after #2337 — the diverged case now rebases. Replace with a phrasing that covers both reconcile paths (rebase or reset) and points at the bullets for specifics.
Review feedback dispositionegg-reviewer (review bot) — APPROVED with one non-blocking suggestion:
egg-reviewer (agent-mode-design bot) — APPROVED, no concerns. No other actionable feedback from authorized reviewers. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review
Delta since the prior review (fc8943f1 → e79d24d1) is the single-commit follow-up that applies my own non-blocking suggestion verbatim. Re-verified against the current code:
- Summary sentence at
docs/architecture/orchestrator.md:234: now reads "then reconciles divergence (rebasing local commits) or resets the local branch toorigin/<branch>depending on the case described below." This faithfully captures both control-flow branches in_sync_worktree_with_remote():- The diverged-case rebase at
orchestrator/routes/pipelines.py:5493(rebases local commits). - The reset-to-origin step covering the succeeded-ahead/push-fail-fallback, failed-ahead, and behind-remote cases.
- The "depending on the case described below" hedge correctly defers to the bullets, including the in-sync early-return.
- The diverged-case rebase at
- The previous summary's
git reset --hardimplementation detail dropped out of the top-line, but the bullets carry the same "reset" language uniformly, so no information is lost in context. - Code references still match:
_sync_worktree_with_remoteat:5232,_rebase_with_agent_output_autoresolveat:5493,divergence_rebase_failedlog path at:5513.base_branch=Nonewarning emit and bare-form fallback unchanged from the prior review.
No new concerns. The original feedback was addressed exactly as suggested. Pure docs-only change with no functional risk.
Verdict
Approving.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
…2365) * docs: update worktree divergence reconcile description Update the _sync_worktree_with_remote() section in orchestrator.md to reflect that diverged worktrees are now reconciled via rebase (using the same _rebase_with_agent_output_autoresolve helper as the gateway-side push-reject path) rather than the old --ff-only merge attempt. Also notes the base_branch=None fallback warning. Authored-by: egg * docs: tighten summary sentence above divergence bullets Per non-blocking review suggestion on #2365: the summary sentence above the bullet list still described 'reset the local branch to origin/<branch> via git reset --hard when needed', which is imprecise after #2337 — the diverged case now rebases. Replace with a phrasing that covers both reconcile paths (rebase or reset) and points at the bullets for specifics. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Update documentation to reflect changes from 69b2a18 (Fix #2337):
_sync_worktree_with_remote()divergence case now uses_rebase_with_agent_output_autoresolveinstead of--ff-onlymerge. The old doc described the stale fast-forward-merge behavior; the new description names the rebase helper, explains thebase_branch=Nonecontamination-risk fallback, and corrects the trigger context to include phase boundaries (not just orchestrator restarts).Triggered by: #2352
Authored-by: egg