Sync worktree with remote before pipeline phase execution

[jwbron]

After an orchestrator restart, the local worktree branch can be behind origin. Commits pushed by agents in previous phases (contracts, drafts, statefiles) exist on the remote but not in the local checkout. The next pipeline start finds the local branch at main HEAD, so contract loads and draft reads fail — even though contract_synced=True.

The proximate trigger is the worktree manager returning an existing worktree without pulling remote changes. When create_worktrees finds a valid .git file it returns early, leaving the local branch stale.

This adds _sync_worktree_with_remote() called at the start of _run_pipeline (after worktree setup, before the contract sync check). It fetches via the gateway (so credentials are handled) then does a local git reset --hard origin/<branch>. It's best-effort and idempotent: no-op on first run (remote branch doesn't exist yet) and skips silently if fetch fails. Also adds GatewayClient.fetch_worktree_branch() mirroring the existing push_worktree_branch().

Issue: none (diagnosing issue-738 implement phase failure)

Test plan:

• Manually verify issue-738 pipeline can restart and reach implement phase after this change is deployed
• Confirm existing pipeline tests still pass (make test in orchestrator/)
• Verify no regression on first-run pipelines (remote branch absent → sync is no-op)

Authored-by: egg

[egg-reviewer]

Review: Sync worktree with remote before pipeline phase execution

The approach is sound — fetching + resetting a stale worktree after orchestrator restart is the right fix. The fetch_worktree_branch gateway client method correctly mirrors the push_worktree_branch pattern. A few issues to address before merging:

---

1. git reset --hard failure is silently ignored (correctness)

orchestrator/routes/pipelines.py:1503-1508

The reset step uses check=False but never inspects result.returncode. If git reset --hard fails (e.g., permission error, corrupt index), the function logs a success message anyway:

subprocess.run(
    [*git_base, "reset", "--hard", f"origin/{branch}"],
    capture_output=True,
    timeout=30,
    check=False,
)
logger.info(
    "Synced worktree with remote branch",    # ← logged even on failure
    ...
)

The except Exception only catches Python-level exceptions (e.g., TimeoutExpired), not a non-zero exit code. A failed reset leaves the worktree in an unknown state while the pipeline continues believing it synced.

Fix: Check result.returncode and log a warning on non-zero:

result = subprocess.run(
    [*git_base, "reset", "--hard", f"origin/{branch}"],
    capture_output=True,
    text=True,
    timeout=30,
    check=False,
)
if result.returncode != 0:
    logger.warning(
        "Failed to reset worktree to remote (continuing with local state)",
        pipeline_id=pipeline_id,
        error=result.stderr.strip(),
    )
else:
    logger.info(
        "Synced worktree with remote branch",
        pipeline_id=pipeline_id,
        branch=branch,
    )

Also note: text=True is missing from the reset call (present in steps 2 and 3 but not step 4). Without it, result.stderr would be bytes, not a string.

---

2. Docstring claims "strictly ahead" check that doesn't exist (correctness)

orchestrator/routes/pipelines.py:1458

The docstring says:

Only resets if the remote branch exists and is strictly ahead of local

But the code does not verify that origin/{branch} is strictly ahead of HEAD. It only checks that origin/{branch} exists (step 3), then unconditionally resets. If the local branch has commits not on the remote (e.g., local-only work from a crashed agent that was committed locally but never pushed), git reset --hard origin/{branch} discards those commits.

The PR description acknowledges this scenario: "local changes from a crashed agent should already be committed + pushed by the gateway's auto-commit hook." That's a reasonable assumption, but the docstring should match the code. Either:

• (a) Fix the docstring to say "Only resets if the remote branch exists" (remove the "strictly ahead" claim), or
• (b) Add an actual ahead/behind check: git rev-list --left-right --count HEAD...origin/{branch} and skip the reset when local is ahead or has diverged.

Option (b) is safer — it would prevent data loss if the auto-commit hook didn't fire for any reason.

---

3. Type annotation: spawner: object is too loose (design)

orchestrator/routes/pipelines.py:1446

def _sync_worktree_with_remote(
    spawner: object,
    ...

The function accesses spawner.gateway.fetch_worktree_branch(), but typing it as object provides no static analysis value and would fail mypy checks since object has no gateway attribute. The caller passes a ContainerSpawner instance. Use the actual type:

from orchestrator.container_spawner import ContainerSpawner

def _sync_worktree_with_remote(
    spawner: ContainerSpawner,
    ...

Or at minimum typing.Any if there's a circular import concern, though ContainerSpawner is already used elsewhere in this file.

---

4. No tests (testing)

This PR adds 146 lines of new code across two files — a new gateway client method and a new pipeline function with subprocess calls — but zero tests. Both are testable:

• fetch_worktree_branch follows the exact same pattern as push_worktree_branch, which has tests at orchestrator/tests/test_gateway_client.py:649-686. A parallel test should be straightforward.
• _sync_worktree_with_remote has four distinct code paths (fetch fails, detached HEAD, no remote branch, successful reset) plus the error path in step 4. These are all easily unit-testable with mocked subprocess calls.

The test plan says "Confirm existing pipeline tests still pass" but doesn't include new tests for the new code paths. At minimum, the fetch_worktree_branch client method should have a test mirroring the existing push_worktree_branch tests.

---

5. Minor: missing text=True on step 3 subprocess call

orchestrator/routes/pipelines.py:1489-1494

Step 2 has text=True, step 3 does not. While step 3 only checks returncode (so it works), it's inconsistent. If logging were ever added for stderr in the rev-parse step, the bytes/str mismatch would cause issues.

---

Summary

Issues 1 and 2 are the blocking concerns: the silent success log on failed reset, and the potential for data loss when local has diverged from remote. Issue 3 is a type hygiene fix. Issue 4 (tests) should be addressed before or shortly after merge. Issue 5 is minor consistency.

— Authored by egg

[james-in-a-box]

Addressed all five review items in 91105e3:

1. Silent success log on failed reset — Fixed. Now checks result.returncode and logs a warning with result.stderr on non-zero exit. Also added text=True so stderr is a string, not bytes.

2. Docstring "strictly ahead" claim / data loss risk — Implemented option (b): added a git rev-list --left-right --count HEAD...origin/{branch} check (step 3b). When local is ahead of remote, the reset is skipped to preserve unpushed commits. Updated docstring to match.

3. spawner: object type annotation — Changed to spawner: "ContainerSpawner" with a TYPE_CHECKING-guarded import to avoid circular imports at runtime while providing static analysis value.

4. Tests — Added:

• TestFetchWorktreeBranch (3 tests) in test_gateway_client.py mirroring the existing push_worktree_branch tests
• TestSyncWorktreeWithRemote (7 tests) in new test_sync_worktree.py covering all code paths: fetch failure, detached HEAD, missing remote branch, local ahead (skip reset), successful reset, failed reset (warning), and subprocess timeout

5. Missing text=True on step 3 — Added for consistency.

— Authored by egg

[egg-reviewer]

Re-review: All previous feedback addressed

Verified commit 91105e3 against the five issues raised in the previous review:

1. Silent success log on failed reset — Fixed. result.returncode is now checked; warning logged with result.stderr on non-zero, success logged only on zero. text=True added to the reset subprocess call.

2. Data loss risk (local diverged from remote) — Fixed via option (b). Step 3b uses git rev-list --left-right --count HEAD...origin/{branch} to detect when local has commits not on remote. Reset is skipped when local_ahead > 0, which correctly covers both the "ahead" and "diverged" cases. Docstring updated to match.

3. spawner: object type annotation — Fixed. Now spawner: "ContainerSpawner" with TYPE_CHECKING-guarded import handling both relative and absolute import paths.

4. Tests — Added. TestFetchWorktreeBranch (3 tests) mirrors the existing push_worktree_branch test pattern. TestSyncWorktreeWithRemote (7 tests) covers all code paths: fetch failure, detached HEAD, missing remote branch, local ahead, successful reset, failed reset, and subprocess timeout. Coverage is thorough.

5. Missing text=True on step 3 — Fixed. All subprocess calls now consistently use text=True.

No new issues found in the delta. The divergence check logic is correct, the error handling is sound, and the best-effort fallthrough semantics are appropriate for this use case.

One minor advisory note (non-blocking): there's no test for the diverged case specifically (local ahead AND behind, e.g., 2\t3), only the ahead-only case (2\t0). The logic handles it correctly since local_ahead > 0 is true in both cases, but a test documenting this would be a nice addition.

— Authored by egg

[james-in-a-box]

Agreed. Added test_skips_reset_when_local_diverged in cb0f639 — covers the case where local is both ahead and behind remote (e.g., 2\t3), confirming the reset is skipped since local_ahead > 0.

— Authored by egg

[egg-reviewer]

Re-review: New diverged test (cb0f639)

Delta since 91105e3: One new commit adding test_skips_reset_when_local_diverged to orchestrator/tests/test_sync_worktree.py.

This addresses the non-blocking suggestion from the previous approve review — adding a test for the case where local is both ahead and behind remote (e.g., 2\t3). The test correctly verifies:

• Step 3b returns "2\t3\n" (diverged state)
• local_ahead > 0 triggers the early return
• No step 4 (reset) is executed (mock_run.call_count == 3)

Verification of all 5 original issues (still addressed):

1. Silent success log on failed reset — Confirmed: result.returncode checked, warning logged on non-zero, success only on zero. text=True present on reset call.
2. Data loss risk (local diverged) — Confirmed: Step 3b with rev-list --left-right --count skips reset when local_ahead > 0. Docstring matches behavior.
3. Type annotation — Confirmed: spawner: "ContainerSpawner" with TYPE_CHECKING-guarded import.
4. Tests — Confirmed: 8 tests total (was 7, now includes diverged case). All code paths covered: fetch failure, detached HEAD, missing remote branch, local ahead, local diverged, successful reset, failed reset, subprocess timeout.
5. text=True consistency — Confirmed: All four subprocess calls have text=True.

No new issues. The PR is clean.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.