Fix #2337: rebase fallback in _sync_worktree_with_remote + loud-fail populator/slice-gate - #2352
Conversation
…populator/slice-gate The orchestrator-side worktree sync used `git merge --ff-only` on divergence, which by definition cannot reconcile a `local-ahead AND remote-ahead` worktree. The function returned silently, leaving agents' plan-phase commits on origin while the populator ran against a stale local checkout, hit `if not plan_path.exists(): return`, and produced an empty `contract.slices`. The implement phase's slice-loop gate then silently routed to monolithic, demoting issue-2261's 15-slice plan to a slice-1-only PR (#2335) carrying program-level title/description. Three behavioural changes, one structural change: 1. `_sync_worktree_with_remote` reuses gateway_client's `_rebase_with_agent_output_autoresolve` on divergence — the same helper the gateway-side push-reject path already uses. The merge --ff-only branch is gone. 2. Every return path emits a single `worktree_sync_outcome` log line with a `case` discriminator (fetch_failed, detached_head, no_remote_tracking, local_ahead_pushed, divergence_rebased, divergence_rebase_failed, reset_succeeded, reset_failed, ...). The pre-fix function had four no-log early returns, which made `[no further sync log lines]` the only signal of which path fired. 3. `_populate_contract_from_plan_safe` takes a `source` parameter. `source="plan_complete"` (natural call site) now probes `origin/{branch}:{draft_rel}` via git cat-file when the local draft is missing — if origin has it, raises `PlanDraftMissingOnLocalError` with an OVERSEER_ALERT log so the pipeline can fail loudly. `source="advance_phase_force"` keeps the swallow-everything behaviour from #1941. 4. The implement-phase slice-loop gate calls `_slice_gate_block_monolithic_demotion`: when contract.slices is empty but the on-disk plan draft parses to N>1 slices, the phase is marked FAILED with an OVERSEER_ALERT instead of routing to _run_concurrent_phase. Defensive belt-and-braces — given the loud-fail populator above, this gate should rarely fire, but it catches any future code path that bypasses the populator. Tests cover all four paths: rebase happy/error, every outcome log case, source="plan_complete" vs "advance_phase_force" branches with mocked origin probe, and the slice-gate helper across multi/single/ missing/parse-fail plan drafts.
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ACK / APPROVE — non-blocking suggestions
The core fix is sound and correctly addresses #2337. Replacing git merge --ff-only with _rebase_with_agent_output_autoresolve lets the worktree reconcile the bidirectional divergence (local ahead and origin ahead) that previously stranded plan-phase commits on origin. The defense-in-depth is well-layered:
- Sync rebases divergence instead of bailing out (
orchestrator/routes/pipelines.py:5232-5511) - Populator natural-source loud-fail (
source="plan_complete") raisesPlanDraftMissingOnLocalErrorwhen local is missing the draft but origin has it (orchestrator/routes/pipelines.py:13008+) - Slice gate recheck in
_run_pipelinere-parses the on-disk plan and refuses to demote N>1 plans to monolithic implement (orchestrator/routes/pipelines.py:12906-12957, 14949-14998)
The worktree_sync_outcome case-discriminator logging gives ops a clean signal-per-path for future post-mortems. The _populate_contract_from_plan_safe Literal["plan_complete", "advance_phase_force"] split correctly preserves #1941 swallow-everything semantics for force-advance while strengthening the natural plan→implement transition. Tests cover the new helpers in isolation reasonably well.
I traced the full data flow end-to-end (sync → populator → slice gate → _run_pipeline outer loop) and verified the multi-slice test fixture parses to 2 slices via the production parse_plan. Nothing I found rises to blocking; below are non-blocking suggestions.
Non-blocking suggestions
S1 — PR description over-claims single-emit invariant
The PR body says "Every return path emits a single worktree_sync_outcome." The push-fail-fallthrough branch (pipelines.py:~5380-5400) emits local_ahead_push_failed_falling_through_to_reset and then a second outcome from the subsequent reset path. Either reword the description or note explicitly that fall-through paths emit a sequence (which is actually nicer for ops than a single conflated event).
S2 — Missing worktree_sync_outcome on prior-phase-failed discard
The "Prior phase failed — discarding local-ahead commits" branch around pipelines.py:5414-5420 doesn't emit a worktree_sync_outcome, breaking the case-discriminator parity. Add case="local_ahead_discarded" (or similar) so log-based dashboards see every exit path.
S3 — PlanDraftMissingOnLocalError handler bypasses graceful cleanup
The new try/except at pipelines.py:15224-15250 marks the pipeline FAILED and breaks out of the outer loop directly, skipping the if phase_failed: cleanup block (which would have called _teardown_phase_overseer and report_pipeline_status gracefully). The finally: at pipelines.py:15989+ does eventually tear down the overseer container so it's not a leak — but it's inconsistent with the slice-gate failure handler at 14949-14998, which does set phase_failed = True and reuses the existing cleanup path. Consider restructuring the plan-draft-missing path to do the same; it makes the cleanup story uniform and keeps the next reader from wondering whether the divergence was intentional.
S4 — _origin_has_plan_draft has no direct test
All current callers mock it. A subprocess-arg regression (e.g. swapping git cat-file -e for git rev-parse, dropping origin/, mis-quoting the path) would slip through. A minimal test against a tmp_path git repo with a real git init && git fetch setup would cost little and protect a load-bearing branch.
S5 — Reset paths double-log
Both reset_failed and reset_exception emit the legacy log line and the new worktree_sync_outcome. Not harmful — slightly redundant. Could collapse to a single emission once the dashboards are migrated to the new case field.
S6 — Integration test gap on the new failure paths
The new tests cover the helper functions in isolation. There's no integration coverage of (a) PlanDraftMissingOnLocalError raising into _run_pipeline and exiting cleanly, or (b) the slice-gate state transition with a real contract+plan mismatch. Both paths are now load-bearing for plan→implement correctness; an integration test against a tmp pipeline would meaningfully harden the change.
S7 — pipeline.base_branch is None inherits #2222 contamination
When pipeline.base_branch is None, _rebase_with_agent_output_autoresolve falls through _build_rebase_cmd (gateway_client.py:2222) to plain git rebase origin/{branch} — the same form that triggered #2222 main-contamination. This is not a regression (it matches the gateway-side push-reject rebase behavior), but worktree-sync now inherits the same risk surface. Worth a code comment at the call site or a defensive log noting that a None base_branch falls back to the bare-rebase form, so the next person debugging a contamination bug knows to look here.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
S2: Emit worktree_sync_outcome with case=local_ahead_discarded_falling_through_to_reset
on the prior-phase-failed branch so log-based dashboards see a uniform
exit-path event for every branch.
S3: Mirror the slice-gate failure handler in the PlanDraftMissingOnLocalError
catch — set phase_failed=True and run the full cleanup sequence (overseer
teardown, status report, best-effort push) inline so both load-bearing
plan-phase failure paths have a uniform cleanup story.
S4: Direct subprocess-argv tests for _origin_has_plan_draft. The sandbox
blocks git init so we can't build a real-clone fixture; instead the tests
mock subprocess.run and assert the exact argv (cat-file -e
origin/{branch}:{path} with -C {repo}) so any subprocess-arg regression
the reviewer named — swapping cat-file for rev-parse, dropping the -e flag,
dropping the origin/ prefix, mis-quoting the path — trips a test failure.
S7: Comment + warning log at the divergence-rebase call site flagging
that base_branch=None falls back to the bare-rebase form (the #2222
contamination vector) so the next person debugging contamination has a
breadcrumb. Add tests verifying the warning fires with base_branch=None
and stays silent with base_branch set.
S5 (deferred to #2359): collapse double-log on reset paths once dashboards
migrate to the new case discriminator.
S6 (deferred to #2360): integration tests for the new failure paths.
|
Thanks for the thorough review. Per-item disposition:
Post-fix: 90 unit tests pass across — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Resolves conflicts between this branch's rebase-fallback divergence-handling work and main's worktree-sync outcome-taxonomy refactor (#2349). Both PRs touched the same `_sync_worktree_with_remote` exit-path logging surface. Resolution: kept this branch's rebase implementation (replaces the old `--ff-only` reconcile, which couldn't fix true divergence), and adopted main's case-discriminator taxonomy and assertion style (`logger.info(..., case=...)`, strict `_outcome_cases(mock) == [...]` assertions). The two ff-only-only tests from main (`test_case_diverged_ff_*`) were dropped — the code path they covered no longer exists on this branch. Renamed this branch's `local_ahead_discarded_falling_through_to_reset` to main's shorter `local_ahead_discarded` for taxonomy consistency.
Merge conflict resolution — origin/main → PR #2352Merged Resolution summary
Verification
Pushed as — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Verdict: ACK / APPROVE — non-blocking suggestions
Re-review at 1a58797ef. All four substantive items from the prior review (S2, S3, S4, S7) are correctly addressed in commit 4948912, and the subsequent origin/main merge cleanly composes this PR's rebase implementation with #2349's case-discriminator taxonomy. 61 tests across test_sync_worktree, test_populate_contract_audit_events, and test_slice_gate_recheck pass.
Verification of prior review items
-
S2 —
local_ahead_discardeddiscriminator emitted ✓ Code atorchestrator/routes/pipelines.py:5444-5451emitscase="local_ahead_discarded"before falling through to step-4 reset. The merge correctly renamed away the_falling_through_to_resetsuffix to match main's shorter taxonomy. Testtest_case_local_ahead_discarded(test_sync_worktree.py:581-604) asserts the strict sequence["local_ahead_discarded", "reset_succeeded"]. -
S3 —
PlanDraftMissingOnLocalErrorcleanup parity ✓ Handler atorchestrator/routes/pipelines.py:15437-15496mirrors theif phase_failed:cleanup block at15314-15357: same overseer-teardown-under-lock, samereport_pipeline_status+_emit_pipeline_event, same best-effortpush_worktree_branchwith try/except, samebreak. Both load-bearing failure paths now have a uniform cleanup story. -
S4 — Direct argv tests for
_origin_has_plan_draft✓ Five tests inTestOriginHasPlanDraft(test_populate_contract_audit_events.py:432-555) assert the exact argv structure (cat-file -e origin/{branch}:{path}with correct-Cordering), zero/non-zero exit handling, and exception swallow. Each named regression in the prior review (cat-file→rev-parse swap, dropping-e, droppingorigin/, mis-quoted path, mis-routed-C) has a dedicated assertion. -
S7 —
base_branch=Nonecontamination breadcrumb ✓ Block comment at5462-5474and warning log at5475-5481flag the #2222 contamination vector. Teststest_divergence_with_base_branch_none_logs_contamination_warningandtest_divergence_with_base_branch_set_does_not_log_contamination_warning(test_sync_worktree.py:644-695) verify both states.
Merge resolution review
The merge of origin/main into the PR branch (1a58797ef) had to reconcile this PR's rebase-fallback work with main's worktree_sync_outcome outcome-taxonomy refactor (#2349). Both touched the same _sync_worktree_with_remote exit-path logging surface. The resolution is correct:
- Kept this PR's
_rebase_with_agent_output_autoresolvecall (line5489-5494); main's--ff-onlyretry path is gone, as it should be — by definition it cannot reconcile true divergence. - Adopted main's
casetaxonomy (kwarg orderingpipeline_id, branch, case=, strict_outcome_cases(mock) == [...]assertions), correctly renaming this PR'slocal_ahead_discarded_falling_through_to_resettolocal_ahead_discarded. - Adopted main's
already_in_syncearly return (5359-5371) — this PR didn't need it, but composes cleanly. - Adopted main's collapsed
reset_failed(subsumes priorreset_exception) andrev_list_failed(subsumes the rc!=0 / parts!=2 / exception triad). - Dropped main's
test_case_diverged_ff_*tests (the--ff-onlycode path no longer exists) and dropped this PR'stest_local_ahead_discarded_emits_outcomein favor of main's pre-existingtest_case_local_ahead_discardedcovering the same path under the shorter case name. - Kept this PR's S7 contamination-warning tests verbatim.
End-to-end data flow re-verified: sync rebases on divergence → populator natural call site (source="plan_complete") raises PlanDraftMissingOnLocalError if local missing draft + origin has it → _run_pipeline catches and runs the inline cleanup → slice-gate recheck at 15171 is the belt-and-braces backstop. Three independent layers, no silent demotion path.
Non-blocking suggestions
N1 — Dead phase_failed = True at pipelines.py:15459
The PlanDraftMissingOnLocalError handler does inline cleanup and then breaks out of the outer while True: at _run_pipeline:14674. The break at 15496 exits that outer loop directly — it does NOT route through the if phase_failed: block at 15314 (which is at column 12, inside the same outer while). So phase_failed = True set at 15459 is never re-read and the assignment is dead. It's harmless (and could be confusing for the next reader). Either drop it, or add a one-line comment that it's set for parity with the slice-gate handler even though the inline cleanup makes it unused on this branch.
N2 — Function docstring overclaims single-emit invariant (same root cause as S1)
pipelines.py:5262-5264 reads:
"Every return path emits a single
worktree_sync_outcomelog line with acasediscriminator so production logs name which path fired."
This claim was inherited from main (PR #2349's docstring) and survived the merge unchanged, but the same prose-vs-behaviour gap S1 flagged on the PR body applies here: the three fall-through paths (local_ahead_push_failed, local_ahead_discarded, rev_list_failed) intentionally emit a sequence — first the discriminator, then reset_* from step 4. The tests in TestSyncWorktreeOutcomeTaxonomy correctly assert sequences (e.g. ["local_ahead_discarded", "reset_succeeded"]), so the contract-as-tested is sequence-on-fall-through, single-on-direct-return. Reword to match. Suggested:
"Every return path emits at least one
worktree_sync_outcomelog line with acasediscriminator. Paths that fall through to the step-4 reset emit a sequence — first a discriminator naming WHY we fell through, then the terminalreset_succeeded/reset_failedevent."
The prose-only PR-body edit S1 flagged is the same fix and is still pending operator action (gateway blocks egg from editing PRs owned by jwbron).
N3 — _origin_has_plan_draft docstring references rev-parse
pipelines.py:13165-13174 docstring says:
"A False return means either origin really doesn't have the draft, or the rev-parse query itself failed — caller should treat both as ..."
The helper uses git cat-file -e, not git rev-parse. Stale wording from a previous version. Replace rev-parse with cat-file so the docstring matches the implementation it's documenting (and matches the test class that explicitly asserts the cat-file -e argv).
N4 — S5 / S6 deferral tracking issues
The author correctly deferred S5 (collapse double-log on reset paths) to #2359 and S6 (integration tests on the new failure paths) to #2360 with conditioning rationale. No action here — flagging for the merger so the issues stay on the radar.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
N1: Drop dead 'phase_failed = True' in the PlanDraftMissingOnLocalError handler at _run_pipeline. The handler runs inline cleanup and then breaks out of the outer 'while True:' loop directly, so the assignment is never re-read by the 'if phase_failed:' block above. Cleanup parity with the slice-gate handler is preserved by the existing block comment naming the mirrored cleanup sequence. N2: Reword the _sync_worktree_with_remote docstring to match the sequence-on-fall-through behaviour the tests in TestSyncWorktreeOutcomeTaxonomy already assert. The three fall-through paths (local_ahead_push_failed, local_ahead_discarded, rev_list_failed) emit a discriminator naming WHY we fell through, then the terminal reset_succeeded / reset_failed event from step 4. Keeps prose and behaviour in sync. N3: Replace 'rev-parse' with 'cat-file' in the _origin_has_plan_draft docstring. Stale wording from a prior version — the helper uses git cat-file -e (the test class TestOriginHasPlanDraft asserts the exact argv).
|
Thanks for the re-review. Per-item disposition:
Post-fix verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Verdict: ACK / APPROVE
Re-review at 85c2b62f1. The delta since 1a58797ef is a single-file, 8-line change to orchestrator/routes/pipelines.py that addresses N1, N2, and N3 from the prior review. All three are correctly fixed; nothing else moves in this iteration.
Verification of prior review items
-
N1 — Dead
phase_failed = Trueremoved. ✓ The assignment in thePlanDraftMissingOnLocalErrorhandler is gone (pipelines.py:15441-15499). I traced control flow to confirm the removal is safe: the inline cleanup mirrors theif phase_failed:block at15318-15361(overseer teardown under lock,report_pipeline_status+_emit_pipeline_event, best-effortpush_worktree_branch, thenbreak), and thebreakat column 20 exits the outerwhile True:at_run_pipeline:14678(column 8). No other reader ofphase_failedexists between the handler exit and the outerexcept/finally(grepconfirms zero references in lines 16000–16500), so the assignment was genuinely dead. Three otherphase_failed = Truesites at14900,15068,15204,15253,15294are untouched and still feed the shared cleanup block correctly. -
N2 —
_sync_worktree_with_remotedocstring matches behaviour. ✓pipelines.py:5262-5268now says "at least one" outcome and explicitly enumerates the three fall-through cases (local_ahead_push_failed,local_ahead_discarded,rev_list_failed). I cross-checked the enumeration against the implementation: those are exactly the three case-discriminators that emit beforereturnis skipped and control reaches the step-4 reset:rev_list_failed(5369/5379): emits, leavesrev_list_ok=False, all three Step 3c branches (already_in_sync,local_ahead,divergence) evaluate False → falls through to Step 4.local_ahead_push_failed(5437): emits,if push_result:block doesn't return → falls through.local_ahead_discarded(5452): emits, the surroundingelse:doesn't return → falls through.
All other case-discriminators (fetch_failed,detached_head,branch_detect_failed,no_remote_tracking,rev_parse_failed,already_in_sync,local_ahead_pushed,divergence_rebased,divergence_rebase_failed) return immediately. The behind-only case (local_ahead==0 and remote_ahead>0) reaches Step 4 with no preceding emission, so it produces a single outcome (reset_succeeded/reset_failed) — correctly excluded from the sequence list. The docstring is now an accurate contract.
-
N3 —
_origin_has_plan_draftdocstring referencescat-file. ✓pipelines.py:13176now readscat-fileinstead ofrev-parse, matching the implementation at13190-13192(["cat-file", "-e", f"origin/{branch}:{draft_rel}"]) and the argv assertions inTestOriginHasPlanDraft.
Other observations
- The S1 PR-body wording from the original review is still pending operator action (gateway blocks
eggfrom editing PRs owned byjwbron). Not blocking; flagged for the merger. - Deferred items #2359 (S5 — collapse double-log on reset paths once dashboards migrate) and #2360 (S6 — integration coverage of the new failure paths) remain open with conditioning rationale. Still on the radar.
No new issues in the delta.
— Authored by egg
|
egg review completed. View run logs 9 previous review(s) hidden. |
* Fix #2367: pass pipeline_branch into _sync_worktree_with_remote The orchestrator-side worktree runs on egg/<pid>/work but the agent-facing remote branch is egg/<pid>. Reading the local branch via `git branch --show-current` and looking up `origin/<that>` always missed, exiting at case=no_remote_tracking before the rebase fallback (#2352) or the step-4 reset could run — stranding plan-phase commits on origin while the loud-fail populator marked the pipeline FAILED. Add a keyword-only pipeline_branch arg and use it for every remote-side ref (rev-parse, rev-list, gateway push target, divergence-rebase target, reset target). The local branch name is still used for detached-HEAD detection and is logged alongside remote_branch in worktree_sync_outcome so operators can grep both names. Both pipelines.py call sites now pass pipeline.branch. Also fixes the latent companion bug at the local-ahead push: the gateway builds HEAD:refs/heads/{branch} from its `branch` argument, so passing the /work-suffixed local name would have pushed to origin/egg/<pid>/work. Masked today by the step-3 early-out, but the regression guard test pins the correct target. * Tighten #2367 regression guard with argv-aware rev-parse mock The reviewer flagged that test_no_remote_tracking_does_not_fire_when_ pipeline_branch_resolves accepted any rev-parse target as success — so under the buggy code (which would query origin/<local_branch>) the mock would still return 0 and the test would still pass. The named regression guard provided zero regression value. Replace the linear side_effect with an argv-aware callable: rev-parse returns 0 only when origin/egg/issue-42 is in the argv, and 128 otherwise. Under the pre-fix code path (querying origin/egg/issue-42/work) the mock now returns 128 → no_remote_tracking fires → the test fails. The fix path (querying origin/egg/issue-42) still passes. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* Fix #2367: pass pipeline_branch into _sync_worktree_with_remote The orchestrator-side worktree runs on egg/<pid>/work but the agent-facing remote branch is egg/<pid>. Reading the local branch via `git branch --show-current` and looking up `origin/<that>` always missed, exiting at case=no_remote_tracking before the rebase fallback (#2352) or the step-4 reset could run — stranding plan-phase commits on origin while the loud-fail populator marked the pipeline FAILED. Add a keyword-only pipeline_branch arg and use it for every remote-side ref (rev-parse, rev-list, gateway push target, divergence-rebase target, reset target). The local branch name is still used for detached-HEAD detection and is logged alongside remote_branch in worktree_sync_outcome so operators can grep both names. Both pipelines.py call sites now pass pipeline.branch. Also fixes the latent companion bug at the local-ahead push: the gateway builds HEAD:refs/heads/{branch} from its `branch` argument, so passing the /work-suffixed local name would have pushed to origin/egg/<pid>/work. Masked today by the step-3 early-out, but the regression guard test pins the correct target. * Tighten #2367 regression guard with argv-aware rev-parse mock The reviewer flagged that test_no_remote_tracking_does_not_fire_when_ pipeline_branch_resolves accepted any rev-parse target as success — so under the buggy code (which would query origin/<local_branch>) the mock would still return 0 and the test would still pass. The named regression guard provided zero regression value. Replace the linear side_effect with an argv-aware callable: rev-parse returns 0 only when origin/egg/issue-42 is in the argv, and 128 otherwise. Under the pre-fix code path (querying origin/egg/issue-42/work) the mock now returns 128 → no_remote_tracking fires → the test fails. The fix path (querying origin/egg/issue-42) still passes. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
…orktree (#2714) (#2719) * fix: rebase with --autostash so divergence reconcile survives dirty worktree (#2714) The orchestrator writes statefile / agent-output deltas continuously without committing them eagerly, so the worktree is routinely dirty at sync time. PR #2352 added a divergence-rebase fallback inside `_sync_worktree_with_remote`, but the underlying `git rebase` refuses on any unstaged changes — making the rebase abort 100% of the time at plan-complete sync, leaving the populator's draft missing on local and halting the pipeline at plan_complete. Set `--autostash` on both forms returned by `_build_rebase_cmd` (plain `git rebase origin/{branch}` and `--onto origin/{branch} origin/{base_branch}`). Git stashes unstaged changes before the rebase and pops them on success; on abort the stash entry is kept in the stash list for recovery. Adds an end-to-end regression test that reproduces the production shape (divergent remote + dirty worktree, both rebase forms) and asserts the unstaged statefile delta is restored after the rebase. * fix: detect autostash-pop conflicts + reproduce dirty-worktree bug in test (#2714 review) Two review-driven fixes on top of the original --autostash one-liner: 1. Reproduce the bug in the regression test. The old setup created an untracked file under .egg-state/, but git rebase only refuses for *modified tracked* files; an untracked-only worktree does not block rebase, so the test passed with or without --autostash. Seed a tracked .egg-state/contract.json in the initial commit and modify it in the work tree so the precondition that produces the bug actually fires. 2. Detect the autostash-pop-conflict failure mode. git rebase --autostash exits 0 even when the final stash pop conflicts: rebase succeeded, but UU entries land in the worktree and the autostash entry stays in git stash list. Without a check the orchestrator would treat a half-merged worktree as a successful sync. New helper _autostash_pop_conflict_result probes _list_unmerged_paths after every rebase-success path and surfaces reconcile_autostash_pop_conflict so the caller can react. New regression test exercises the failure path end-to-end. Also corrects the abort-behavior wording in both docstrings: git rebase --abort *reapplies* the autostash to the working tree automatically; it only preserves the stash list entry as a fallback when the reapply itself conflicts. * test: tighten dirty-worktree precondition to '' M'' (unstaged only) The reviewer flagged that the precondition assertion in test_rebase_succeeds_against_dirty_worktree accepted ''M '' (staged-only) and ''MM'' (both staged and unstaged) in addition to '' M'' (unstaged). The autostash bug fires only on *unstaged* modifications -- staged-only would trip ''Your index contains uncommitted changes'' instead. The current test setup always lands at '' M'', so the broader pattern doesn't currently false-positive, but a future drift to a staged-only setup could silently regress the test back to not exercising the bug. Pin the assertion to startswith('' M'') and update the comment to spell out why the other shapes don''t exercise the autostash path. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
Summary
Fixes #2337. The orchestrator-side worktree sync used
git merge --ff-onlyon divergence, which can't reconcile a worktree where local has unpushed commits AND origin has commits the local doesn't. It returned silently, leaving agents' plan-phase commits on origin while the populator ran against the stale local checkout, hit itsplan_draft_missingearly-return, and producedcontract.slices=[]. The implement-phase slice-loop gate then silently routed to monolithic, demoting issue-2261's 15-slice plan to a slice-1-only PR (#2335) carrying program-level copy.Three behavioural changes plus one structural fail-loud guard:
_sync_worktree_with_remoterebases on divergence. Reuses gateway_client's_rebase_with_agent_output_autoresolve— the same helper the gateway-side push-reject path uses. The--ff-onlybranch is gone.worktree_sync_outcome. Single structured log line with acasediscriminator (fetch_failed,detached_head,no_remote_tracking,local_ahead_pushed,divergence_rebased,divergence_rebase_failed,reset_succeeded,reset_failed, …). The pre-fix function had four no-log early returns —[no further sync log lines]was literally the only signal of which path fired._populate_contract_from_plan_safetakes asourceparameter.source="plan_complete"(natural call site) probesorigin/{branch}:{draft_rel}viagit cat-filewhen local is missing the draft; if origin has it, raisesPlanDraftMissingOnLocalErrorwith anOVERSEER_ALERTlog so_run_pipelinemarks the pipeline FAILED.source="advance_phase_force"keeps the swallow-everything contract from Force-advance out of plan phase skips _populate_contract_from_plan, leaving contract.pr empty and PR metadata as fallback placeholders #1941._slice_gate_block_monolithic_demotionhelper: whencontract.slicesis empty but the on-disk plan draft parses to N>1 slices, the implement phase is marked FAILED withOVERSEER_ALERT slice_gate_blocked_monolithic_demotioninstead of routing through_run_concurrent_phase. Defensive belt-and-braces — given the loud-fail populator above, this should rarely fire, but it catches any future code path that bypasses the populator.The decision-sync
state_storemismatch noted in #2337 as "worth filing separately" is left for a follow-up issue.Test plan
make teston changed files: 49 tests pass (sync_worktree, populate_contract_audit_events, slice_gate_recheck, advance_phase_populate_on_plan_exit).test_advance_phase_thread,test_pipeline_failure_path,test_slice_run_loop_integration— 69 tests, all green.ruff checkandruff formatclean across all changed files.worktree_sync_outcome case=divergence_rebasedappears in logs at plan→implement boundary; confirm contract.slices populates when origin had unpushed plan commits at sync time.