diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 27e427835fd..e646a7d7f2b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -500,12 +500,18 @@ jobs: - name: Setup Go uses: actions/setup-go@v5 with: - # Deliberately independent from the minimum patch in server/go.mod: - # CI follows the newest 1.26 patch available to setup-go. - go-version: "1.26.x" + # Keep the security floor even when the setup-go manifest lags; + # continue accepting newer patches within Go 1.26. + go-version: "~1.26.9" check-latest: true cache-dependency-path: server/go.sum + - name: Test Windows worktree delivery, conflict replay and checkpoint CAS + working-directory: server + # Native Windows Git/worktree coverage for #8541, including isolated Prepare and conflict replay. + # Verbose output exposes a missing test selection or a skip. + run: go test ./internal/daemon/execenv -v -run '^(TestFinalizeFastForwardsConversationBranchToOffBranchDelivery|TestFinalizeFastForwardKeepsRefsWhenCheckpointIsLocked|TestFastForwardStateRejectsStaleConversationTip|TestFinalizeFastForwardRejectsConcurrentCheckpointUpdate|TestFinalizeFastForwardRejectsMissingPreparedCheckpoint|TestFinalizeRefusesFastForwardWhenConversationBranchIsCheckedOutElsewhere|TestFinalizeRefusesDivergedOffBranchDelivery|TestIsolatedPrepareFastForwardsOffBranchDelivery|TestFinalizeSameTipRetryRejectsConcurrentCheckpoint|TestFinalizeSameTipRetryAfterFastForwardSucceeds|TestFinalizeSameTipRetryRejectsConcurrentBranchMove|TestConflictResolvedByTheAgentIsDeliveredAndNotReplayedAgain|TestConflictAfterAUserCommitOnTheBranchStillOffersTheEditAgain|TestFinalizeConflictReplayRejectsConcurrentCheckpointUpdate)$' -count=1 -timeout=10m + - name: Test Windows execution-environment isolation working-directory: server # Keep this job scoped to the runtime regression it exists to prove. diff --git a/server/internal/daemon/execenv/local_worktree.go b/server/internal/daemon/execenv/local_worktree.go index 440bee6c4ef..9577f2f1185 100644 --- a/server/internal/daemon/execenv/local_worktree.go +++ b/server/internal/daemon/execenv/local_worktree.go @@ -162,6 +162,10 @@ type LocalWorktree struct { // turn replayed and still be recorded as having delivered it, which is how // the user's edits went missing from the turn after (MUL-6881 review). BaseCommit string + // preparedStateRef is the exact checkpoint ref written before the agent ran. + // Finalize must not overwrite a concurrent checkpoint update, even when the + // conversation branch itself has not moved. + preparedStateRef string // DirtyBaseCaptured records that the user had uncommitted tracked edits // which were replayed into the worktree. DirtyBaseCaptured bool @@ -225,20 +229,22 @@ func (w *LocalWorktree) MarshalJSON() ([]byte, error) { type wire LocalWorktree return json.Marshal(struct { *wire - CreatedBranch bool `json:"created_branch"` - UserState string `json:"user_state"` - PriorState string `json:"prior_state"` - Owner branchOwner `json:"owner"` - TracksState bool `json:"tracks_state"` - SnapshotPending bool `json:"snapshot_pending"` + CreatedBranch bool `json:"created_branch"` + PreparedStateRef string `json:"prepared_state_ref"` + UserState string `json:"user_state"` + PriorState string `json:"prior_state"` + Owner branchOwner `json:"owner"` + TracksState bool `json:"tracks_state"` + SnapshotPending bool `json:"snapshot_pending"` }{ - wire: (*wire)(w), - CreatedBranch: w.createdBranch, - UserState: w.userState, - PriorState: w.priorState, - Owner: w.owner, - TracksState: w.tracksState, - SnapshotPending: w.snapshotPending, + wire: (*wire)(w), + CreatedBranch: w.createdBranch, + PreparedStateRef: w.preparedStateRef, + UserState: w.userState, + PriorState: w.priorState, + Owner: w.owner, + TracksState: w.tracksState, + SnapshotPending: w.snapshotPending, }) } @@ -246,17 +252,19 @@ func (w *LocalWorktree) UnmarshalJSON(data []byte) error { type wire LocalWorktree aux := struct { *wire - CreatedBranch bool `json:"created_branch"` - UserState string `json:"user_state"` - PriorState string `json:"prior_state"` - Owner branchOwner `json:"owner"` - TracksState bool `json:"tracks_state"` - SnapshotPending bool `json:"snapshot_pending"` + CreatedBranch bool `json:"created_branch"` + PreparedStateRef string `json:"prepared_state_ref"` + UserState string `json:"user_state"` + PriorState string `json:"prior_state"` + Owner branchOwner `json:"owner"` + TracksState bool `json:"tracks_state"` + SnapshotPending bool `json:"snapshot_pending"` }{wire: (*wire)(w)} if err := json.Unmarshal(data, &aux); err != nil { return err } w.createdBranch = aux.CreatedBranch + w.preparedStateRef = aux.PreparedStateRef w.userState = aux.UserState w.priorState = aux.PriorState w.owner = aux.Owner @@ -400,6 +408,13 @@ func PrepareLocalWorktree(params LocalWorktreeParams, logger *slog.Logger) (*Loc tracksState: plan.tracksState && actualBranch == plan.name, } + // A continued branch already has a verified checkpoint. A replay conflict + // skips recordState until the agent resolves it; retain that exact ref so + // Finalize can CAS against it instead of treating the turn as unprepared. + if wt.tracksState && plan.continues { + wt.preparedStateRef = plan.priorState + } + // Tear the worktree back down on every failure below. A half-replayed tree // is the worst outcome available: it looks like a working checkout, so // nothing downstream questions it, while the agent silently reads different @@ -597,6 +612,7 @@ func (w *LocalWorktree) Finalize(logger *slog.Logger) (LocalWorktreeOutcome, err if dirty { committed, err := w.commitAll(logger) if err != nil { + outcome.Branch = "" // No recorded delivery; the preserved worktree is authoritative. outcome.PreservedPath = w.Path if logger != nil { logger.Error("execenv: could not commit the agent's changes; keeping the worktree so the work is recoverable", @@ -654,7 +670,8 @@ func (w *LocalWorktree) Finalize(logger *slog.Logger) (LocalWorktreeOutcome, err // makes that recoverable — the worktree is still there to preserve, exactly // as for a commit that could not be made. if !dropped { - if verifyErr := w.verifyDeliveryPoint(tip); verifyErr != nil { + advanceFrom, verifyErr := w.verifyDeliveryPoint(tip) + if verifyErr != nil { outcome.Branch = "" outcome.PreservedPath = w.Path if logger != nil { @@ -666,15 +683,22 @@ func (w *LocalWorktree) Finalize(logger *slog.Logger) (LocalWorktreeOutcome, err "recover the work from there, and let the run keep the commit the worktree started from instead of resetting past it", w.Branch, verifyErr, w.Path, w.GitRoot) } - if recErr := w.recordState(tip, logger); recErr != nil { + var recErr error + if advanceFrom != "" { + recErr = w.recordFastForwardState(advanceFrom, tip, logger) + } else { + recErr = w.recordFinalizedState(tip) + } + if recErr != nil { + outcome.Branch = "" // Without a recorded checkpoint, no delivery can be claimed. outcome.PreservedPath = w.Path if logger != nil { logger.Error("execenv: could not record the delivered task branch; keeping the worktree", "path", w.Path, "branch", w.Branch, "git_root", w.GitRoot, "error", recErr) } return outcome, fmt.Errorf( - "could not record branch %s as this conversation's: %w; the work is committed to that branch and the "+ - "task worktree is preserved at %s (listed by `git worktree list` in %s) — a follow-up run will start "+ + "could not record branch %s as this conversation's: %w; the task worktree is preserved "+ + "at %s (listed by `git worktree list` in %s) — a follow-up run will start "+ "a new branch instead of continuing this one", w.Branch, recErr, w.Path, w.GitRoot) } @@ -1058,6 +1082,19 @@ type branchRecord struct { // means a branch that moved in that window simply fails the ancestor test next // time, which is the safe direction. func writeBranchRecord(gitRoot, branch, userState, checkpoint string, owner branchOwner) (string, error) { + record, err := createBranchRecord(gitRoot, branch, userState, checkpoint, owner) + if err != nil { + return "", err + } + if out, err := runGit(gitRoot, "update-ref", userStateRef(branch), record); err != nil { + return "", fmt.Errorf("git update-ref: %s: %w", strings.TrimSpace(out), err) + } + return record, nil +} + +// createBranchRecord writes the checkpoint object without publishing its ref. +// Finalize can then publish it with the conversation branch in one transaction. +func createBranchRecord(gitRoot, branch, userState, checkpoint string, owner branchOwner) (string, error) { if checkpoint == "" { return "", fmt.Errorf("no checkpoint to record for branch %s", branch) } @@ -1067,9 +1104,6 @@ func writeBranchRecord(gitRoot, branch, userState, checkpoint string, owner bran if err != nil { return "", fmt.Errorf("git commit-tree: %w", err) } - if out, err := runGit(gitRoot, "update-ref", userStateRef(branch), record); err != nil { - return "", fmt.Errorf("git update-ref: %s: %w", strings.TrimSpace(out), err) - } return record, nil } @@ -1419,43 +1453,140 @@ func quotedPaths(paths []string) string { // verifyDeliveryPoint checks that tip is a commit this conversation can put its // name on before it becomes the branch's recorded checkpoint. // -// Two things are asserted, and they are the two ways a delivery can be -// something other than what this task built. The tip has to BE the task's -// branch — a run that checked out something else, or a branch someone moved -// underneath it, delivers a commit this record has no business describing. And -// it has to still contain the commit this turn started from — this turn's own -// baseline when it made one, otherwise the branch tip it continued. A run that -// resets its worktree back to the user's own HEAD passes neither test but the -// second is the one that matters, twice over: recording a plain user commit as -// the checkpoint is what makes a branch they later recreate there look like -// ours, and a tip without this turn's starting point no longer carries the -// snapshot about to be recorded as delivered (MUL-6881 review). -func (w *LocalWorktree) verifyDeliveryPoint(tip string) error { +// The ordinary case ends on the conversation branch itself, so its ref already +// equals tip. A repository may instead require the agent to check out its own +// task branch inside the worktree. That is still a valid delivery when tip is a +// strict descendant of the conversation branch: advancing the conversation ref +// is then only a fast-forward, and keeps the next turn on the work just +// delivered. +// +// Refuse any divergence. Return the expected old tip rather than moving the +// branch here: Finalize atomically updates the branch AND its checkpoint, so a +// failed checkpoint write cannot strand an unrecorded branch advance. +func (w *LocalWorktree) verifyDeliveryPoint(tip string) (string, error) { if !w.tracksState { // Nothing will be recorded for this branch, so there is nothing to prove. - return nil + return "", nil } if tip == "" { - return errors.New("the task worktree has no resolvable HEAD") + return "", errors.New("the task worktree has no resolvable HEAD") } - branchTip, err := runGitTrimmed(w.GitRoot, "rev-parse", "--verify", "refs/heads/"+w.Branch) + branchRef := "refs/heads/" + w.Branch + branchTip, err := runGitTrimmed(w.GitRoot, "rev-parse", "--verify", branchRef) if err != nil { - return fmt.Errorf("resolve branch %s: %w", w.Branch, err) + return "", fmt.Errorf("resolve branch %s: %w", w.Branch, err) } - if branchTip != tip { - return fmt.Errorf("the worktree delivered %s while branch %s points at %s, so the run did not deliver onto its own branch", - shortID(tip), w.Branch, shortID(branchTip)) + advanceBranch := branchTip != tip + if advanceBranch { + if _, err := runGit(w.GitRoot, "merge-base", "--is-ancestor", branchTip, tip); err != nil { + return "", fmt.Errorf("the worktree delivered %s while branch %s points at %s, so the run did not deliver a fast-forward of its conversation branch", + shortID(tip), w.Branch, shortID(branchTip)) + } } if w.BaseCommit == "" { - return fmt.Errorf("branch %s has no commit of this task's own to prove it by", w.Branch) + return "", fmt.Errorf("branch %s has no commit of this task's own to prove it by", w.Branch) } if _, err := runGit(w.GitRoot, "merge-base", "--is-ancestor", w.BaseCommit, tip); err != nil { - return fmt.Errorf("the delivered commit %s no longer contains %s, the commit this turn started from", + return "", fmt.Errorf("the delivered commit %s no longer contains %s, the commit this turn started from", shortID(tip), shortID(w.BaseCommit)) } + if advanceBranch { + // update-ref is atomic but deliberately lower-level than the porcelain + // branch commands: it will move a branch even when another worktree has + // it checked out. Do not change HEAD underneath a sibling/user checkout. + if checkedOutAt, checkedOut, err := branchWorktreePath(w.GitRoot, w.Branch); err != nil { + return "", fmt.Errorf("check whether branch %s is in use before fast-forwarding it: %w", w.Branch, err) + } else if checkedOut { + return "", fmt.Errorf("branch %s is checked out at %s, so it cannot be advanced to delivered commit %s without moving another worktree underneath it", + w.Branch, checkedOutAt, shortID(tip)) + } + return branchTip, nil + } + return "", nil +} + +// recordFastForwardState advances the conversation ref and its ownership +// checkpoint in one Git ref transaction. A failed update of either ref leaves +// both untouched; the worktree and its off-branch commit remain recoverable. +func (w *LocalWorktree) recordFastForwardState(from, tip string, logger *slog.Logger) error { + if w.userState == "" { + return fmt.Errorf("branch %s has no user snapshot to record", w.Branch) + } + if w.preparedStateRef == "" { + return fmt.Errorf("branch %s has no checkpoint recorded before this turn; refusing to overwrite an unknown state", w.Branch) + } + record, err := createBranchRecord(w.GitRoot, w.Branch, w.userState, tip, w.owner) + if err != nil { + return err + } + branchRef := "refs/heads/" + w.Branch + // Compare-and-swap BOTH refs. The prepared checkpoint is the only state + // this turn is entitled to replace; a concurrent writer can update it + // without moving the branch, and must not be silently overwritten. + input := fmt.Sprintf("start\nupdate %s %s %s\nupdate %s %s %s\nprepare\ncommit\n", + branchRef, tip, from, userStateRef(w.Branch), record, w.preparedStateRef) + out, err := runGitInput(w.GitRoot, input, "update-ref", "--stdin") + if err != nil { + return fmt.Errorf("atomically fast-forward branch %s from %s to %s and record its checkpoint: %s: %w", + w.Branch, shortID(from), shortID(tip), strings.TrimSpace(out), err) + } + w.preparedStateRef = record + if logger != nil { + logger.Warn("execenv: worktree delivered on another branch; fast-forwarded the conversation branch", + "path", w.Path, "branch", w.Branch, "from", from, "to", tip) + } return nil } +// recordFinalizedState handles a delivery whose HEAD already equals the +// conversation tip. This includes a retry after a successful fast-forward CAS +// when removing the worktree failed. Do not fall back to unconditional +// update-ref: that would erase a checkpoint written by another process. +func (w *LocalWorktree) recordFinalizedState(tip string) error { + if w == nil || !w.tracksState || w.Branch == "" || w.userState == "" { + return nil + } + if w.preparedStateRef == "" { + return fmt.Errorf("branch %s has no prepared checkpoint to finalize safely", w.Branch) + } + record, err := createBranchRecord(w.GitRoot, w.Branch, w.userState, tip, w.owner) + if err != nil { + return err + } + // Verify the branch tip and compare-and-swap the checkpoint atomically. + // This rejects both a concurrent branch move and a checkpoint rewrite. + input := fmt.Sprintf("start\nverify %s %s\nupdate %s %s %s\nprepare\ncommit\n", + "refs/heads/"+w.Branch, tip, userStateRef(w.Branch), record, w.preparedStateRef) + out, err := runGitInput(w.GitRoot, input, "update-ref", "--stdin") + if err != nil { + return fmt.Errorf("finalize branch %s checkpoint with CAS: %s: %w", + w.Branch, strings.TrimSpace(out), err) + } + w.preparedStateRef = record + return nil +} + +// branchWorktreePath reports where branch is currently checked out, if +// anywhere. A low-level update-ref does not enforce git's normal checked-out +// branch protection, so callers that move a branch ref must check this first. +func branchWorktreePath(gitRoot, branch string) (string, bool, error) { + out, err := runGitStdout(gitRoot, "worktree", "list", "--porcelain") + if err != nil { + return "", false, err + } + target := "branch refs/heads/" + branch + var worktreePath string + for _, line := range strings.Split(out, "\n") { + switch { + case strings.HasPrefix(line, "worktree "): + worktreePath = strings.TrimPrefix(line, "worktree ") + case line == target: + return worktreePath, true, nil + } + } + return "", false, nil +} + // unmergedPaths lists the files git considers unresolved in a worktree. func unmergedPaths(worktreePath string) ([]string, error) { out, err := runGitStdout(worktreePath, "diff", "--name-only", "--diff-filter=U", "-z") @@ -1512,9 +1643,11 @@ func (w *LocalWorktree) recordState(checkpoint string, logger *slog.Logger) erro if w == nil || !w.tracksState || w.Branch == "" || w.userState == "" { return nil } - if _, err := writeBranchRecord(w.GitRoot, w.Branch, w.userState, checkpoint, w.owner); err != nil { + record, err := writeBranchRecord(w.GitRoot, w.Branch, w.userState, checkpoint, w.owner) + if err != nil { return err } + w.preparedStateRef = record if logger != nil { logger.Debug("execenv: recorded the local-directory snapshot for the task branch", "branch", w.Branch, "checkpoint", checkpoint) @@ -1682,6 +1815,20 @@ func runGitEnv(dir string, extraEnv []string, args ...string) (string, error) { return string(out), err } +// runGitInput runs a bounded local Git command with its protocol on stdin. +// Used for update-ref transactions so both delivery refs commit or neither does. +func runGitInput(dir, input string, args ...string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), gitTimeout) + defer cancel() + + full := append([]string{"-C", dir}, args...) + cmd := exec.CommandContext(ctx, "git", full...) + cmd.Stdin = strings.NewReader(input) + cmd.WaitDelay = 5 * time.Second + out, err := cmd.CombinedOutput() + return string(out), err +} + // runGitTrimmed runs git for its stdout value, discarding stderr so a // diagnostic line can't be mistaken for the value (`rev-parse` output, a // config value, a stash sha). diff --git a/server/internal/daemon/execenv/local_worktree_test.go b/server/internal/daemon/execenv/local_worktree_test.go index 0fae1a72543..56b791894a7 100644 --- a/server/internal/daemon/execenv/local_worktree_test.go +++ b/server/internal/daemon/execenv/local_worktree_test.go @@ -521,6 +521,9 @@ func TestFinalizeKeepsWorktreeWhenCommitFails(t *testing.T) { if err == nil { t.Fatal("Finalize returned nil error after the commit failed") } + if outcome.Branch != "" { + t.Errorf("Branch = %q, want empty: commit failure did not deliver the branch", outcome.Branch) + } if outcome.PreservedPath != wt.Path { t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) } @@ -1001,6 +1004,8 @@ func TestPrepareLocalWorktreeHandsConflictingUserEditsToTheAgent(t *testing.T) { func TestConflictResolvedByTheAgentIsDeliveredAndNotReplayedAgain(t *testing.T) { t.Parallel() repo := newTestRepo(t) + // This test asserts exact LF bytes after repeated Git worktree checkouts. + gitRun(t, repo, "config", "core.autocrlf", "false") writeFile(t, filepath.Join(repo, "tracked.txt"), "A\n") first := prepareTurn(t, repo, "MUL-6881", turnOneTask) @@ -1009,6 +1014,11 @@ func TestConflictResolvedByTheAgentIsDeliveredAndNotReplayedAgain(t *testing.T) writeFile(t, filepath.Join(repo, "tracked.txt"), "C\n") second := prepareTurn(t, repo, "MUL-6881", turnTwoTask) + // A conflicted replay does not publish a new prepare checkpoint. It must + // retain the previously verified ref for Finalize's compare-and-swap. + if got, err := readUserStateRef(repo, second.Branch); err != nil || second.preparedStateRef != got { + t.Fatalf("conflict prepare checkpoint = %q, recorded = %q, err = %v", second.preparedStateRef, got, err) + } if len(second.ReplayConflicts) == 0 { t.Fatal("turn two saw no conflict between the agent's B and the user's C") } @@ -1569,6 +1579,329 @@ func TestFinalizeRefusesToRecordADeliveryThatResetPastItsBaseline(t *testing.T) } } +// A repository may require the agent to work on its own task-named branch +// inside Multica's worktree. If that branch simply extends the conversation +// branch, Finalize can safely book the delivery by fast-forwarding the +// conversation ref instead of turning a successful run into a failure. +func TestFinalizeFastForwardsConversationBranchToOffBranchDelivery(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + // Git for Windows may default to CRLF checkout; keep the LF fixture exact. + gitRun(t, repo, "config", "core.autocrlf", "false") + + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + conversationTip := gitRun(t, repo, "rev-parse", wt.Branch) + if conversationTip != wt.BaseCommit { + t.Fatalf("conversation tip = %s, want this turn's base %s", conversationTip, wt.BaseCommit) + } + + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "delivered on the repo branch\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + outcome := finalizeOK(t, wt) + if outcome.Branch != wt.Branch { + t.Fatalf("Branch = %q, want conversation branch %q", outcome.Branch, wt.Branch) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != delivered { + t.Fatalf("conversation branch = %s, want delivered tip %s", got, delivered) + } + ref, err := readUserStateRef(repo, wt.Branch) + if err != nil { + t.Fatalf("readUserStateRef: %v", err) + } + record, err := readBranchRecord(repo, ref) + if err != nil { + t.Fatalf("readBranchRecord: %v", err) + } + if record.checkpoint != delivered { + t.Errorf("checkpoint = %s, want delivered tip %s", record.checkpoint, delivered) + } + + // The next turn must continue from the recovered delivery rather than redo + // work from the old conversation tip. + next := prepareTurn(t, repo, "MUL-8541", turnTwoTask) + if !next.Continued { + t.Fatal("next turn did not continue the fast-forwarded conversation branch") + } + if got := readFile(t, filepath.Join(next.WorkDir, "agent.txt")); got != "delivered on the repo branch\n" { + t.Errorf("next turn lost the delivered file: %q", got) + } + finalizeOK(t, next) +} + +// A failed checkpoint update must not leave the conversation branch advanced +// without its matching state record. Git's multi-ref transaction is all-or-none +// even when the checkpoint ref is locked by another process. +func TestFinalizeFastForwardKeepsRefsWhenCheckpointIsLocked(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + originalTip := gitRun(t, repo, "rev-parse", wt.Branch) + originalState, err := readUserStateRef(repo, wt.Branch) + if err != nil { + t.Fatalf("read initial state: %v", err) + } + + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "delivered off branch\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + commonDir, err := gitCommonDirFor(repo) + if err != nil { + t.Fatalf("git common dir: %v", err) + } + lockPath := filepath.Join(commonDir, filepath.FromSlash(userStateRef(wt.Branch))+".lock") + if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { + t.Fatalf("create ref directory: %v", err) + } + if err := os.WriteFile(lockPath, []byte("busy"), 0o600); err != nil { + t.Fatalf("lock checkpoint ref: %v", err) + } + t.Cleanup(func() { _ = os.Remove(lockPath) }) + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize succeeded despite a locked checkpoint ref") + } + if outcome.Branch != "" { + t.Errorf("Branch = %q, want no claimed delivery", outcome.Branch) + } + if outcome.PreservedPath != wt.Path { + t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != originalTip { + t.Errorf("conversation branch advanced to %s, want %s", got, originalTip) + } + state, err := readUserStateRef(repo, wt.Branch) + if err != nil || state != originalState { + t.Errorf("state ref = %q, err = %v; want %s", state, err, originalState) + } + if got := gitRun(t, repo, "rev-parse", "repo/task-branch"); got != delivered { + t.Errorf("delivery ref = %s, want %s", got, delivered) + } + if _, statErr := os.Stat(wt.Path); statErr != nil { + t.Errorf("delivery worktree was not preserved: %v", statErr) + } +} + +// A concurrent branch move after verification must fail the expected-old-SHA +// check without publishing the new checkpoint either. +func TestFastForwardStateRejectsStaleConversationTip(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + originalTip := gitRun(t, repo, "rev-parse", wt.Branch) + originalState, err := readUserStateRef(repo, wt.Branch) + if err != nil { + t.Fatalf("read initial state: %v", err) + } + + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + other := filepath.Join(t.TempDir(), "conversation") + gitRun(t, repo, "worktree", "add", "--quiet", other, wt.Branch) + writeFile(t, filepath.Join(other, "concurrent.txt"), "concurrent work\n") + gitRun(t, other, "add", "-A") + gitRun(t, other, "commit", "-m", "concurrent work") + concurrentTip := gitRun(t, other, "rev-parse", "HEAD") + gitRun(t, repo, "worktree", "remove", "--force", other) + + if err := wt.recordFastForwardState(originalTip, delivered, worktreeTestLogger()); err == nil { + t.Fatal("stale compare-and-swap advanced the conversation branch") + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != concurrentTip { + t.Errorf("conversation branch = %s, want concurrent tip %s", got, concurrentTip) + } + state, err := readUserStateRef(repo, wt.Branch) + if err != nil || state != originalState { + t.Errorf("state ref = %q, err = %v; want %s", state, err, originalState) + } +} + +// A concurrent writer may replace the checkpoint without moving the branch. +// Finalize must not erase that record: both refs have to be compare-and-swapped +// against the exact values this task owned before the agent ran. +func TestFinalizeFastForwardRejectsConcurrentCheckpointUpdate(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + originalTip := gitRun(t, repo, "rev-parse", wt.Branch) + preparedState := wt.preparedStateRef + if preparedState == "" { + t.Fatal("Prepare did not record its checkpoint") + } + + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + // A different snapshot makes a valid but distinct checkpoint record, + // even though the conversation branch still points at originalTip. + otherSnapshot := gitRun(t, repo, "commit-tree", wt.userState+"^{tree}", "-m", "concurrent snapshot") + concurrentState, err := writeBranchRecord(repo, wt.Branch, otherSnapshot, originalTip, wt.owner) + if err != nil { + t.Fatalf("write concurrent checkpoint: %v", err) + } + if concurrentState == preparedState { + t.Fatal("concurrent checkpoint did not change the state ref") + } + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize overwrote a concurrently updated checkpoint") + } + if outcome.Branch != "" { + t.Errorf("Branch = %q, want no claimed delivery", outcome.Branch) + } + if outcome.PreservedPath != wt.Path { + t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != originalTip { + t.Errorf("conversation branch = %s, want %s", got, originalTip) + } + state, err := readUserStateRef(repo, wt.Branch) + if err != nil || state != concurrentState { + t.Errorf("checkpoint ref = %q, err = %v; want %s", state, err, concurrentState) + } + if got := gitRun(t, repo, "rev-parse", "repo/task-branch"); got != delivered { + t.Errorf("delivery ref = %s, want %s", got, delivered) + } + if _, statErr := os.Stat(wt.Path); statErr != nil { + t.Errorf("delivery worktree was not preserved: %v", statErr) + } +} + +// If Prepare could not publish a checkpoint, Finalize must preserve the +// off-branch delivery rather than overwrite an unknown record. +func TestFinalizeFastForwardRejectsMissingPreparedCheckpoint(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + originalTip := gitRun(t, repo, "rev-parse", wt.Branch) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + + // Simulate another actor removing the state ref after Prepare. + gitRun(t, repo, "update-ref", "-d", userStateRef(wt.Branch)) + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize succeeded after its prepared checkpoint was deleted") + } + if outcome.Branch != "" || outcome.PreservedPath != wt.Path { + t.Errorf("outcome = %+v, want preserved worktree and no claimed branch", outcome) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != originalTip { + t.Errorf("conversation branch = %s, want %s", got, originalTip) + } + if _, err := readUserStateRef(repo, wt.Branch); err == nil { + t.Fatal("Finalize recreated a concurrently deleted checkpoint") + } + if _, statErr := os.Stat(wt.Path); statErr != nil { + t.Errorf("delivery worktree was not preserved: %v", statErr) + } +} + +// Moving the ref is only safe when nobody else has the conversation branch +// checked out. git update-ref itself does not protect linked worktrees, so keep +// the successful delivery preserved rather than changing another checkout's +// HEAD underneath it. +func TestFinalizeRefusesFastForwardWhenConversationBranchIsCheckedOutElsewhere(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "off-branch delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + other := filepath.Join(t.TempDir(), "conversation") + gitRun(t, repo, "worktree", "add", "--quiet", other, wt.Branch) + conversationTip := gitRun(t, repo, "rev-parse", wt.Branch) + if _, err := gitTry(t, repo, "merge-base", "--is-ancestor", conversationTip, delivered); err != nil { + t.Fatal("test setup is not a fast-forward delivery") + } + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize moved a conversation branch that another worktree had checked out") + } + if !strings.Contains(err.Error(), "checked out at") { + t.Errorf("error does not explain that the branch is in use: %v", err) + } + if outcome.PreservedPath != wt.Path { + t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != conversationTip { + t.Errorf("conversation branch moved to %s, want %s", got, conversationTip) + } + if _, statErr := os.Stat(wt.Path); statErr != nil { + t.Errorf("delivery worktree was removed after the ref move was refused: %v", statErr) + } +} + +// A fast-forward recovery must never become a force-update. Once another +// worktree has advanced the conversation branch on a different line, the +// off-branch delivery and the conversation branch have diverged; Finalize must +// preserve the task worktree and leave the conversation branch untouched. +func TestFinalizeRefusesDivergedOffBranchDelivery(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "off-branch delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + other := filepath.Join(t.TempDir(), "conversation") + gitRun(t, repo, "worktree", "add", "--quiet", other, wt.Branch) + writeFile(t, filepath.Join(other, "other.txt"), "concurrent conversation work\n") + gitRun(t, other, "add", "-A") + gitRun(t, other, "commit", "-m", "concurrent conversation work") + conversationTip := gitRun(t, other, "rev-parse", "HEAD") + gitRun(t, repo, "worktree", "remove", "--force", other) + + if _, err := gitTry(t, repo, "merge-base", "--is-ancestor", conversationTip, delivered); err == nil { + t.Fatal("test setup did not create divergent histories") + } + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize accepted a delivery that diverged from the conversation branch") + } + if !strings.Contains(err.Error(), "did not deliver a fast-forward") { + t.Errorf("error does not explain the divergence: %v", err) + } + if outcome.Branch != "" { + t.Errorf("outcome named branch %q for a refused delivery", outcome.Branch) + } + if outcome.PreservedPath != wt.Path { + t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != conversationTip { + t.Errorf("conversation branch moved to %s, want concurrent tip %s", got, conversationTip) + } + if _, statErr := os.Stat(wt.Path); statErr != nil { + t.Errorf("worktree removed despite refusing the divergent delivery: %v", statErr) + } +} + // The same guard from the other side: whatever the worktree delivered has to BE // the task's branch. A run that ended somewhere else — a detached checkout, a // different branch — delivered a commit this record has no business describing, @@ -1590,7 +1923,7 @@ func TestFinalizeRefusesToRecordADeliveryFromOffTheBranch(t *testing.T) { if err == nil { t.Fatal("Finalize recorded a delivery that is not the branch's tip") } - if !strings.Contains(err.Error(), "did not deliver onto its own branch") { + if !strings.Contains(err.Error(), "did not deliver a fast-forward of its conversation branch") { t.Errorf("error does not explain the mismatch: %v", err) } if outcome.PreservedPath != wt.Path { @@ -1726,6 +2059,11 @@ func TestConflictAfterAUserCommitOnTheBranchStillOffersTheEditAgain(t *testing.T writeFile(t, filepath.Join(repo, "tracked.txt"), "rewritten by the user instead\n") second := prepareTurn(t, repo, "MUL-6881", turnTwoTask) + // A conflicted replay does not publish a new prepare checkpoint. It must + // retain the previously verified ref for Finalize's compare-and-swap. + if got, err := readUserStateRef(repo, second.Branch); err != nil || second.preparedStateRef != got { + t.Fatalf("conflict prepare checkpoint = %q, recorded = %q, err = %v", second.preparedStateRef, got, err) + } if !second.Continued { t.Fatal("second turn did not continue the branch the user had committed on") } @@ -1757,6 +2095,116 @@ func TestConflictAfterAUserCommitOnTheBranchStillOffersTheEditAgain(t *testing.T } } + +func TestFinalizeConflictReplayRejectsConcurrentCheckpointUpdate(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + writeFile(t, filepath.Join(repo, "tracked.txt"), "original\n") + + first := prepareTurn(t, repo, "MUL-6881", turnOneTask) + writeFile(t, filepath.Join(first.WorkDir, "tracked.txt"), "agent\n") + finalizeOK(t, first) + + writeFile(t, filepath.Join(repo, "tracked.txt"), "user\n") + second := prepareTurn(t, repo, "MUL-6881", turnTwoTask) + if len(second.ReplayConflicts) == 0 { + t.Fatal("expected a conflicting user edit") + } + prior := gitRun(t, repo, "rev-parse", userStateRef(second.Branch)) + if second.preparedStateRef != prior { + t.Fatalf("prepared checkpoint = %q, want %q", second.preparedStateRef, prior) + } + tip := gitRun(t, repo, "rev-parse", second.Branch) + otherSnapshot := gitRun(t, repo, "commit-tree", second.userState+"^{tree}", "-m", "concurrent snapshot") + concurrentRef, err := writeBranchRecord(repo, second.Branch, otherSnapshot, tip, second.owner) + if err != nil { + t.Fatalf("concurrent checkpoint: %v", err) + } + if concurrentRef == prior { + t.Fatal("concurrent checkpoint did not change") + } + + // Resolve in favour of the existing branch content: there is no new + // commit, but Finalize must still refuse to overwrite the other writer. + writeFile(t, filepath.Join(second.WorkDir, "tracked.txt"), "agent\n") + gitRun(t, second.Path, "add", "tracked.txt") + outcome, err := second.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("Finalize overwrote a concurrently changed conflict checkpoint") + } + if outcome.Branch != "" || outcome.PreservedPath != second.Path { + t.Errorf("failed delivery = %+v, want no branch and preserved worktree %q", outcome, second.Path) + } + if got := gitRun(t, repo, "rev-parse", userStateRef(second.Branch)); got != concurrentRef { + t.Errorf("checkpoint = %s, want concurrent ref %s", got, concurrentRef) + } + if got := gitRun(t, repo, "rev-parse", second.Branch); got != tip { + t.Errorf("branch = %s, want unchanged %s", got, tip) + } + if _, err := os.Stat(second.Path); err != nil { + t.Errorf("worktree not preserved: %v", err) + } +} + +// The daemon finalizes the JSON-decoded worktree returned by the preparation +// helper, not the in-process worktree. The expected checkpoint ref must survive +// that boundary or every off-branch fast-forward is rejected as unprepared. +func TestIsolatedPrepareFastForwardsOffBranchDelivery(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + env, err := PrepareIsolated(ctx, preparationHelperTestCommand(), PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: testBranchOwner.WorkspaceID, + TaskID: turnOneTask, + IssueIdentifier: "MUL-8541", + Provider: "claude", + AgentName: "J", + Task: TaskContextForEnv{ + IssueID: testBranchOwner.ConversationID, + AgentID: testBranchOwner.AgentID, + }, + LocalWorktree: &LocalWorktreeParams{LocalPath: repo}, + }, worktreeTestLogger()) + if err != nil { + t.Fatalf("PrepareIsolated: %v", err) + } + wt := env.LocalWorktree + if wt.preparedStateRef == "" { + t.Fatal("PrepareIsolated lost the prepared checkpoint ref across JSON") + } + original := gitRun(t, repo, "rev-parse", wt.Branch) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "isolated off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + if delivered == original { + t.Fatal("test did not create a new delivery commit") + } + + outcome := finalizeOK(t, wt) + if outcome.Branch != wt.Branch { + t.Fatalf("delivered branch = %q, want %q", outcome.Branch, wt.Branch) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != delivered { + t.Fatalf("conversation branch = %s, want %s", got, delivered) + } + ref, err := readUserStateRef(repo, wt.Branch) + if err != nil { + t.Fatalf("readUserStateRef: %v", err) + } + record, err := readBranchRecord(repo, ref) + if err != nil { + t.Fatalf("readBranchRecord: %v", err) + } + if record.checkpoint != delivered { + t.Errorf("checkpoint = %s, want delivered tip %s", record.checkpoint, delivered) + } +} + // Production never prepares in the daemon's own process: PrepareIsolated runs // Prepare in a helper and the Environment comes back as JSON. Every guarantee // Finalize makes depends on state this struct keeps unexported, and ordinary @@ -1882,3 +2330,122 @@ func TestIsolatedPrepareKeepsTheReadOnlyBranchDrop(t *testing.T) { t.Error("a turn that changed nothing left its branch behind") } } + +// A cleanup failure can leave the task worktree in place after the two-ref +// fast-forward CAS succeeded. Retrying Finalize must not replace a checkpoint +// that another actor wrote in the meantime. +func TestFinalizeSameTipRetryRejectsConcurrentCheckpoint(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + before := gitRun(t, repo, "rev-parse", wt.Branch) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + + if err := wt.recordFastForwardState(before, delivered, worktreeTestLogger()); err != nil { + t.Fatalf("first CAS: %v", err) + } + checkpointAfterCAS, err := readUserStateRef(repo, wt.Branch) + if err != nil || wt.preparedStateRef != checkpointAfterCAS { + t.Fatalf("prepared checkpoint = %s, read = %s, err = %v", wt.preparedStateRef, checkpointAfterCAS, err) + } + + otherSnapshot := gitRun(t, repo, "commit-tree", wt.userState+"^{tree}", "-m", "concurrent snapshot") + concurrentState, err := writeBranchRecord(repo, wt.Branch, otherSnapshot, delivered, wt.owner) + if err != nil { + t.Fatalf("concurrent checkpoint: %v", err) + } + if concurrentState == checkpointAfterCAS { + t.Fatal("concurrent checkpoint did not change") + } + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("same-tip retry overwrote a concurrently changed checkpoint") + } + if outcome.Branch != "" { + t.Errorf("Branch = %q, want no claimed delivery", outcome.Branch) + } + if outcome.PreservedPath != wt.Path { + t.Errorf("PreservedPath = %q, want %q", outcome.PreservedPath, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != delivered { + t.Errorf("branch = %s, want delivered %s", got, delivered) + } + if got, err := readUserStateRef(repo, wt.Branch); err != nil || got != concurrentState { + t.Errorf("checkpoint = %s, err = %v; want %s", got, err, concurrentState) + } + if _, err := os.Stat(wt.Path); err != nil { + t.Errorf("worktree not preserved: %v", err) + } +} + +// Repeating Finalize with unchanged refs should complete the previously +// interrupted cleanup instead of rejecting the transaction as stale. +func TestFinalizeSameTipRetryAfterFastForwardSucceeds(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + before := gitRun(t, repo, "rev-parse", wt.Branch) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + if err := wt.recordFastForwardState(before, delivered, worktreeTestLogger()); err != nil { + t.Fatalf("first CAS: %v", err) + } + outcome, err := wt.Finalize(worktreeTestLogger()) + if err != nil { + t.Fatalf("same-tip retry: %v", err) + } + if outcome.PreservedPath != "" { + t.Errorf("unexpected preserved worktree: %s", outcome.PreservedPath) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != delivered { + t.Errorf("branch = %s, want %s", got, delivered) + } +} + +// A same-tip retry must also reject a branch that moved after the first CAS, +// even if its checkpoint was not modified by the other writer. +func TestFinalizeSameTipRetryRejectsConcurrentBranchMove(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + wt := prepareTurn(t, repo, "MUL-8541", turnOneTask) + before := gitRun(t, repo, "rev-parse", wt.Branch) + gitRun(t, wt.Path, "checkout", "-b", "repo/task-branch") + writeFile(t, filepath.Join(wt.WorkDir, "agent.txt"), "off-branch delivery\n") + gitRun(t, wt.Path, "add", "-A") + gitRun(t, wt.Path, "commit", "-m", "agent delivery") + delivered := gitRun(t, wt.Path, "rev-parse", "HEAD") + if err := wt.recordFastForwardState(before, delivered, worktreeTestLogger()); err != nil { + t.Fatalf("first CAS: %v", err) + } + checkpointAfterCAS, err := readUserStateRef(repo, wt.Branch) + if err != nil { + t.Fatalf("readUserStateRef: %v", err) + } + moved := gitRun(t, repo, "commit-tree", delivered+"^{tree}", "-p", delivered, "-m", "concurrent branch move") + gitRun(t, repo, "update-ref", "refs/heads/"+wt.Branch, moved, delivered) + + outcome, err := wt.Finalize(worktreeTestLogger()) + if err == nil { + t.Fatal("same-tip retry accepted a concurrently moved conversation branch") + } + if outcome.Branch != "" || outcome.PreservedPath != wt.Path { + t.Errorf("failed delivery = %+v, want no branch and preserved worktree %q", outcome, wt.Path) + } + if got := gitRun(t, repo, "rev-parse", wt.Branch); got != moved { + t.Errorf("branch = %s, want concurrent tip %s", got, moved) + } + if got, err := readUserStateRef(repo, wt.Branch); err != nil || got != checkpointAfterCAS { + t.Errorf("checkpoint = %s, err = %v; want unchanged %s", got, err, checkpointAfterCAS) + } + if _, err := os.Stat(wt.Path); err != nil { + t.Errorf("worktree not preserved: %v", err) + } +}