docs: fix worktree sync in-sync case description [doc-updater] - #2358
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Doc-only PR. Verified each claim against the post-#2349 code in orchestrator/routes/pipelines.py:5232-5530 (squash-merge 8d6d7a96d).
Accurate:
- The split of the old "Local behind or in-sync" bullet into "Local behind remote" + "Already in-sync" matches the code. The
already_in_syncearly return atpipelines.py:5377-5388is gated onlocal_ahead == 0 and remote_ahead == 0 and rev_list_okand explicitly skips the Step 4 reset, so describing it as an early return that skips the reset is correct. - The "Local behind remote" bullet correctly describes the fall-through path: with
local_ahead == 0andremote_ahead > 0, neither the local-ahead branch (line 5390) nor the diverged branch (line 5447) fires, and execution falls through to the Step 4 reset at line 5494. - The tightened prose — "performs a gateway-authenticated fetch ... then resolves divergence and resets the local branch to
origin/<branch>viagit reset --hardwhen needed" — matches the actual control flow (fetch, then conditional reset).
Non-blocking suggestion
The same section contains another bullet that is now slightly inaccurate against post-#2349 code, in exactly the same way the in-sync bullet was:
Prior phase succeeded, local ahead: Local commits are pushed to remote first, preserving completed work, then the worktree is reset.
When the push succeeds, the function emits local_ahead_pushed and returns at pipelines.py:5419 — no Step 4 reset runs in this path. The reset only runs on the local_ahead_push_failed fall-through (pipelines.py:5421-5430 → fall through to Step 4). The end-state is equivalent (after a successful push + re-fetch, local already matches origin/{branch}, so a reset would be a no-op), but the wording "then the worktree is reset" implies the reset always runs — exactly the implication this PR is otherwise fixing in the in-sync bullet. Worth tightening for consistency, e.g.:
Prior phase succeeded, local ahead: Local commits are pushed to remote first, preserving completed work. After a successful push the local branch already matches
origin/<branch>, so no further reset is needed; on push failure the worktree falls through to a reset againstorigin/<branch>, discarding the unpushed commits.
This is a follow-up suggestion, not a blocker for this PR.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The 'Prior phase succeeded, local ahead' bullet implied the reset always runs after a successful push. In _sync_worktree_with_remote() the local_ahead_pushed branch returns at pipelines.py:5419 — the Step 4 reset only runs when the push fails and falls through. Reword to match, in line with the in-sync clarification this PR already makes. Authored-by: egg
Review feedback dispositionsItem 1 — Tighten "Prior phase succeeded, local ahead" bullet for consistency with the in-sync clarification (egg-reviewer non-blocking suggestion): Reviewer noted the same wording problem in the neighboring bullet — "then the worktree is reset" implies the reset always runs, but — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. The delta since the last review is a single docs reword in docs/architecture/orchestrator.md clarifying that the "Prior phase succeeded, local ahead" path returns early on a successful push and only falls through to a git reset --hard on push failure — orchestrator-side worktree sync semantics, no agent-facing surface affected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of 046b086306b87c16658fafc1178ff3dfe8b8e975. Delta vs. previously-reviewed 3b27819 is a single 4/3 line edit to docs/architecture/orchestrator.md that addresses my prior non-blocking suggestion.
Verified against _sync_worktree_with_remote() in orchestrator/routes/pipelines.py:5377-5520:
- "Local commits are pushed to remote first, preserving completed work" — matches the
prior_phase_succeededbranch atpipelines.py:5392-5401, which callspush_worktree_branchbefore any reset. ✓ - "After a successful push the local branch already matches
origin/<branch>, so no further reset is needed" — matchespipelines.py:5402-5419: onpush_resulttruthy,fetch_worktree_branchre-syncsorigin/{branch}to the just-pushed tip and the functionreturns before Step 4. ✓ - "on push failure the worktree falls through to a reset against
origin/<branch>, discarding the unpushed commits" — matchespipelines.py:5420-5430(warning log, no return) and the fall-through comment atpipelines.py:5444-5445. The Step 4 reset atpipelines.py:5494-5500runsgit reset --hard origin/{branch}; because the push failed,origin/{branch}still does not contain the local commits, so they are discarded. ✓
The wording change keeps the section internally consistent with the in-sync clarification and uses the reviewer-suggested phrasing verbatim. No other bullets in the section drifted. No new issues.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
Update documentation to reflect changes from 8d6d7a9:
_sync_worktree_with_remote()already-in-sync path previously ran a no-opgit reset --hard; Fix #2346: emit worktree_sync_outcome at every _sync_worktree_with_remote return #2349 adds an early return that skips the reset entirely when local and remote are at the same commit.Triggered by: #2349
Authored-by: egg