Skip to content

Deepen Worktree ownership of Issue branch synchronization #492

Closed

Description

@JacobStephens2

What to build

Deepen the Issue branch module in Worktree so each Delivery uses coherent commit facts when it synchronizes with origin. A Base branch merge must identify the commit used for CI comparison. A Foreign commit observation must identify the commits, the local head the review starts from, and the exact origin head a later merge consumes. Sibling Runs changing shared tracking refs, or user branches named like those refs, must not change those facts after they have been sampled.

The Repair loop keeps choosing Repairs and spending its budgets. Worktree owns origin-ref resolution, sampling, ancestry inspection and merging the sampled commits. This gives callers one place to learn the Git correctness rules and tests one interface to exercise.

Acceptance criteria

  • A clean Base branch merge returns its sampled origin commit together with its outcome. Delivery passes that commit to both the CI watch and the Check re-run; it does not rederive attribution from a shared tracking ref after merging.
  • A Foreign commit observation carries the exact sampled origin head, the local head before the merge, the human upstream label and the oldest-first commit list. The later merge consumes that observation once, rejects an observation from another worktree or whose local head changed before mutation, and the review Repair uses its local head as its fixed point. A later observation discovers newly arrived commits.
  • Origin sampling resolves fully qualified remote-tracking refs. Local branches or tags named origin/main or origin/issue-7 cannot select an unintended commit for synchronization or CI comparison.
  • The Base branch moved check and the Spec review's fast-forward catch-up also inspect or merge a sampled commit. Catch-up continues to refuse divergence.
  • A conflicted merge stays tied to its selected commit through the conflict Repair and completion check. An unfinished or aborted merge fails validation, even if the tracking ref subsequently changes.
  • Foreign commit observation remains read-only. The upstream-move budget is spent before its merge, and refusal of a sixth batch leaves that batch unmerged. The existing shared five-move and five-Repair budgets, conflict-then-review ordering, and unchanged-head Check re-run rule retain their behavior.
  • Progress lines, conflict prompts and readable merge messages retain the upstream branch label while Git consumes pinned commit IDs. Ordinary errors and interruption still reach the existing Failed run path.
  • Tests through Worktree's interface use real local Git and a bare origin to cover tracking-ref changes between observation and consumption, shadowing branch/tag names, stable Base branch attribution after a ref moves, and pinned conflict validation. Assertions check returned facts, commit ancestry and observable failures.
  • Repair-loop tests demonstrate that the same returned Base branch commit reaches the watch and the Check re-run, that a Foreign commit review receives the observation's fixed point, and that budget refusal precedes merging. Replace assertions tied to the retired split reads rather than retaining those reads for tests.
  • Existing merge, CI and Spec run behavior tests pass, as do formatting, Clippy and the full test suite.

Blocked by

None (can start immediately).

Decisions

The design tree is settled. Its root is Worktree's ownership of Issue branch synchronization. Round 1 settles scope and policy; round 2 settles sampling and its consumers; round 3 settles verification, documentation and delivery size. Every branch below has a decision and a reason; the frontier is empty.

Round 1: ownership and constraints

  1. Deepen the existing Worktree module. Keep synchronization facts and Git implementation knowledge there; keep Repairs, their budgets, CI decisions and session execution in the Repair loop. Reason: those are the responsibilities the current callers already separate, and deleting the deepened module would spread ref sampling and ancestry rules back across those callers.
  2. Keep the existing delivery contracts. Merge rather than rebase, review Foreign commits from the pre-merge local head, validate conflict completion, compare CI with the Base branch commit merged into that head, and Self-merge only the watched head. Reason: these are settled by the domain model and ADRs 0002, 0004, 0006, 0008 and 0010. This deepening needs no policy decision from the Day shift.
  3. Limit the work to ongoing synchronization. Acquisition, Failed run preservation, Launch directory preflight and Architecture review checkout are outside this Ticket. Reason: acquisition and preservation have their own recently deepened interfaces; changing their behavior is unnecessary for coherent synchronization facts.

Round 2: sampled facts and consumption

  1. Use one private origin-sampling implementation, then commit IDs. After fetching, resolve the fully qualified remote-tracking ref to a commit ID once. Keep its human branch label separately. All subsequent computation for that observation uses the ID. Reason: fully qualified refs prevent shadowing; IDs prevent sibling fetches from changing an observation. The observation represents the tracking-ref sample at resolution time, without requiring origin to remain stationary.
  2. Return the selected Base branch commit with a clean merge outcome. Carry it directly through the Repair loop into the watch, Check re-run, Inherited failure output and Base fix. Retire the separate post-merge ancestry read. For a conflict, keep the pending record tied to the selected commit and take the existing conflict/validation/next-round route. Reason: a later tracking-ref read cannot establish which Base branch commit this operation selected, while the merge already knows it. A no-op merge still has a selected Base branch commit to compare against.
  3. Make Foreign commit observation an opaque value produced by Worktree. It contains the sampled origin head, pre-merge local head, upstream label and oldest-first commits; callers inspect the facts they need and pass the owned value back to the same Worktree for one merge. Worktree rejects a wrong-owner observation or a changed local head before mutation. Production callers cannot manufacture observations from independent strings; the existing scripted test adapter may construct coherent values through a test-only fixture constructor. Retire the no-argument merge of whichever tracking ref was last fetched and the separate local-head read used for the review. Reason: observation and consumption have to remain separate so the Repair loop can refuse a batch before mutation, but their facts must refer to the same commits, and a stale or misapplied observation must not silently mutate a different head.
  4. Preserve the budget and Repair ordering. A nonempty Foreign commit observation spends an upstream move before its pinned merge; a conflict then spends a conflict Repair, validates completion, and spends the review Repair as today. Keep rereading origin until nothing new remains. Reason: merging during observation would change the side effects of an exhausted upstream budget; moving the review-budget check earlier would also change current behavior.
  5. Pin ancillary synchronization operations internally. The Base branch moved check compares its sampled commit with the local head; the Spec review's catch-up fast-forwards to its sampled origin head. Retain the existing pending-merge validation against a commit ID and reject incomplete or aborted merges. Reason: these operations cross the same shared-ref seam and need the same resolution rules, while their callers need no new sampling knowledge.
  6. Keep readable upstream labels. Use the branch label in progress, conflict prompts and merge messages, with the selected SHA as the Git merge target. Reason: diagnostics and existing merge behavior tests describe the source branch; changing Git's argument should not replace that context with a raw commit label.
  7. Use the existing Git seam, with no new adapter abstraction or long-lived lock. Worktree's implementation uses Git in production and real local Git in tests. Do not hold a repository lock across CI or a Repair session, and do not support concurrent external mutation of this same worktree's local head as a new contract. Reason: sibling Runs share tracking refs but own separate Issue branches; immutable facts solve the supported interference without serializing sessions or adding a hypothetical seam.

Round 3: tests, documentation and size

  1. The interface is the test surface. Exercise Worktree with real repositories and deterministic ref updates between calls, using the acquisition/preservation test fixtures as prior art. Cover advance, rewind or replacement where they distinguish the sampled target from a later ref; include shadowing names, clean/no-op outcomes, stale-observation refusal, catch-up divergence, and conflict completion. Reason: the missing behavior is the relationship among actual commits and refs, which scripted return values alone cannot verify. No timing sleeps or Git-command-order assertions are needed.
  2. Replace obsolete split-read tests and retain policy tests. Migrate the Repair loop's scripted adapter to coherent merge/observation facts, assert the commit sent to CI and the fixed point sent to review, and retain its budget and failure tests. Keep the existing end-to-end merge, CI and Spec tests. Reason: callers and tests should cross the same seam, and two independent scripts must not keep an incoherent interface alive after its implementation is removed.
  3. No glossary or ADR change is required. This implements the existing Issue branch, Foreign commit and Inherited failure concepts and contradicts no ADR. Reason: commit sampling is a reversible implementation choice, with no new domain term or surprising policy trade-off to record.
  4. Publish one standalone Ticket. Reason: one owner, its Delivery adapter, its Repair-loop callers and their tests fit a single fresh context window; no independently useful intermediate Ticket is needed. There are no blocking links or parent issue to publish.

Architecture review

Strength: Strong · Dependencies: local-substitutable

Files: src/worktree.rs:183, src/delivery.rs:524, src/delivery/repair_loop.rs:250, src/worktree/acquisition_tests.rs:304, tests/mergeable.rs:42, tests/merge_run.rs:338, tests/ci.rs, tests/spec_run.rs

Before

flowchart LR
  R[Repair loop] --> W[Worktree]
  W --> G[Git]
  G --> O[Origin]
  W -. shared ref lifetime .-> R
  R -. merge then rederive Base commit .-> W
  classDef leak stroke:#dc2626,stroke-width:2px;
  class R,W leak
Loading

After

flowchart LR
  R[Repair loop] -->|sampled commit facts| W[Worktree]
  W -->|pinned commits| G[Git]
  G --> O[Origin]
  classDef deep stroke-width:3px;
  class W deep
Loading

Problem: Worktree's shallow synchronization interface leaks shared-ref lifetime and read-before-merge ordering across the seam into the Repair loop, so CI attribution and a Foreign commit batch can drift between calls.

Solution: Deepen Worktree's implementation with coherent sampled commit facts, giving its interface depth without moving Repair policy behind the seam.

Wins

  • Locality: ref rules concentrate in Worktree
  • Leverage: coherent facts across Delivery paths
  • Interface becomes the test surface
  • Existing Git adapter seam suffices

Deletion test: deleting this deep module would spread ref resolution, commit sampling and ancestry verification across its callers.

Coverage: no open issue proposes this deepening; the 51 open issue titles and bodies were checked on the reviewed Base branch head 21bf200a8025fca8cfa5753840aab07666101ca8.

Evidence: all 23 existing Worktree tests and all 25 Repair-loop tests passed (cargo test --bin thirdshift worktree:: and cargo test --bin thirdshift delivery::repair_loop::). Three temporary tests through the current Worktree interface check that Base branch attribution survives a tracking-ref rewind, ignores a shadowing local origin/main branch, and that a later Foreign commit merge consumes the batch previously observed. All three failed at their commit-equality assertions, confirming the three drifts; the Foreign commit probe merged the second origin commit despite having observed only the first. The temporary tests were then removed. The probes are throwaway review work, not implementation of this Ticket. Both Mermaid blocks were parsed successfully.

Activity

  1. added
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    architect-planAn Architect plan: the Spec or Ticket an Architecture review published
    in-progressA Claim: thirdshift has taken this issue
    and removed
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    on Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    architect-planAn Architect plan: the Spec or Ticket an Architecture review published

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions