From 3d3d2e0615d0c2ba8043c19ac68bbcb9700ce016 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Thu, 9 Jul 2026 21:55:19 -0600 Subject: [PATCH] fix(foreman): anchor recovery soft-reset at merge-base branch point repo.BaseBranchSHA (#995) returns the CURRENT upstream tip at recovery time, but the task branch was cut from the upstream tip at branch-cut time (#813). If upstream advanced between those points, SoftResetToBase's `git reset --soft ` re-staged the intervening upstream delta into the recovered commit (squashed alongside the model's edits). Anchor the reset at the true branch point instead: merge-base(base, HEAD), the lowest common ancestor of the current upstream tip and the task branch = exactly where the branch was cut. The commits-ahead count is anchored there too, keeping the two consistent. Only the model's own commits are re-staged; the upstream delta between branch point and tip is preserved on the branch, never re-staged. Closes #1002 Signed-off-by: Jory Irving --- pkg/foreman/agent/repo/commit.go | 29 +++++++-- pkg/foreman/agent/repo/commit_test.go | 93 +++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 pkg/foreman/agent/repo/commit_test.go diff --git a/pkg/foreman/agent/repo/commit.go b/pkg/foreman/agent/repo/commit.go index 66420586..85d553e9 100644 --- a/pkg/foreman/agent/repo/commit.go +++ b/pkg/foreman/agent/repo/commit.go @@ -127,11 +127,18 @@ func BaseBranchSHA(ctx context.Context, workspace, upstreamURL, baseBranch strin } // SoftResetToBase moves commits from HEAD back into the working tree -// without touching the index, relative to base. Used when a model -// self-committed its work and the executor wants to re-apply it with -// DCO sign-off and executor-owned author identity. base should be a -// commit SHA (resolved via BaseBranchSHA), not a ref name, so a stale -// local branch cannot drag upstream commits into the recovered commit. +// without touching the index, relative to the branch point. Used when a +// model self-committed its work and the executor wants to re-apply it with +// DCO sign-off and executor-owned author identity. base should be a commit +// SHA (resolved via BaseBranchSHA), not a ref name, so a stale local branch +// cannot drag upstream commits into the recovered commit. +// +// base is the CURRENT upstream tip, which may be AHEAD of the point the task +// branch was actually cut from when upstream advanced mid-run. The reset +// anchors at the TRUE branch point — merge-base(base, HEAD) — not at base, so +// only the model's own commits are re-staged; the intervening upstream delta +// between the branch point and the current tip is never squashed into the +// recovered commit (#1002). func SoftResetToBase(ctx context.Context, workspace string, base string) error { if workspace == "" { return fmt.Errorf("SoftResetToBase: workspace is required") @@ -139,14 +146,22 @@ func SoftResetToBase(ctx context.Context, workspace string, base string) error { if base == "" { return fmt.Errorf("SoftResetToBase: base is required") } - count, err := CommitsAheadOfBase(ctx, workspace, base) + out, err := runGit(ctx, workspace, baseEnv(), "merge-base", base, "HEAD") + if err != nil { + return fmt.Errorf("SoftResetToBase: merge-base %s HEAD: %w", base, err) + } + branchPoint := strings.TrimSpace(out) + if branchPoint == "" { + return fmt.Errorf("SoftResetToBase: empty merge-base for %s..HEAD", base) + } + count, err := CommitsAheadOfBase(ctx, workspace, branchPoint) if err != nil { return fmt.Errorf("SoftResetToBase: %w", err) } if count == 0 { return ErrNothingToCommit } - if _, err := runGit(ctx, workspace, baseEnv(), "reset", "--soft", base); err != nil { + if _, err := runGit(ctx, workspace, baseEnv(), "reset", "--soft", branchPoint); err != nil { return fmt.Errorf("SoftResetToBase: reset: %w", err) } return nil diff --git a/pkg/foreman/agent/repo/commit_test.go b/pkg/foreman/agent/repo/commit_test.go new file mode 100644 index 00000000..910e01e0 --- /dev/null +++ b/pkg/foreman/agent/repo/commit_test.go @@ -0,0 +1,93 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package repo + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestSoftResetToBase_AnchorsAtBranchPointNotUpstreamTip reproduces #1002: when +// upstream advances mid-run, BaseBranchSHA returns a tip AHEAD of where the task +// branch was actually cut. The soft reset must anchor at the true branch point +// (merge-base(base, HEAD)) so only the model's own edits are re-staged — the +// intervening upstream delta must never be dragged into the recovered commit. +func TestSoftResetToBase_AnchorsAtBranchPointNotUpstreamTip(t *testing.T) { + gitOrSkip(t) + dir := t.TempDir() + bare := initBareOrigin(t, filepath.Join(dir, "up")) + seedOrigin(t, bare) // main @ A (README.md) + + // Workspace cut from A; the model self-commits fix.txt on a task branch. + ws := mustClone(t, bare, filepath.Join(dir, "ws")) + mustGit(t, ws, "checkout", "-b", "foreman/wl/issue-1002") + if err := os.WriteFile(filepath.Join(ws, "fix.txt"), []byte("model edit\n"), 0o644); err != nil { + t.Fatalf("write fix.txt: %v", err) + } + mustGit(t, ws, "-c", "user.email=u@x", "-c", "user.name=u", "add", "fix.txt") + mustGit(t, ws, "-c", "user.email=u@x", "-c", "user.name=u", "commit", "-m", "model self-commit") + + // Upstream advances to B (adds upstream.txt) AFTER the branch was cut. + adv := mustClone(t, bare, filepath.Join(dir, "adv")) + commitFile(t, adv, "upstream.txt", "intervening upstream delta\n") + + // Recovery: BaseBranchSHA fetches the current upstream tip (B) into ws and + // returns it — the value the executor passes to SoftResetToBase. + baseSHA, err := BaseBranchSHA(context.Background(), ws, bare, "main") + if err != nil { + t.Fatalf("BaseBranchSHA: %v", err) + } + if err := SoftResetToBase(context.Background(), ws, baseSHA); err != nil { + t.Fatalf("SoftResetToBase: %v", err) + } + + staged := gitOut(t, ws, "diff", "--cached", "--name-only") + if !strings.Contains(staged, "fix.txt") { + t.Errorf("model edit fix.txt must be staged for the recovered commit; staged=%q", staged) + } + if strings.Contains(staged, "upstream.txt") { + t.Errorf("intervening upstream delta must NOT be re-staged (would revert merged work); staged=%q", staged) + } +} + +// TestSoftResetToBase_NoSelfCommitIsNothingToCommit verifies that when the model +// added no commits of its own, recovery reports ErrNothingToCommit rather than +// fabricating a commit — even if upstream advanced past the branch point. +func TestSoftResetToBase_NoSelfCommitIsNothingToCommit(t *testing.T) { + gitOrSkip(t) + dir := t.TempDir() + bare := initBareOrigin(t, filepath.Join(dir, "up")) + seedOrigin(t, bare) + + ws := mustClone(t, bare, filepath.Join(dir, "ws")) + mustGit(t, ws, "checkout", "-b", "foreman/wl/issue-1002") + + // Upstream advances; the workspace itself made no commits. + adv := mustClone(t, bare, filepath.Join(dir, "adv")) + commitFile(t, adv, "upstream.txt", "intervening upstream delta\n") + + baseSHA, err := BaseBranchSHA(context.Background(), ws, bare, "main") + if err != nil { + t.Fatalf("BaseBranchSHA: %v", err) + } + if err := SoftResetToBase(context.Background(), ws, baseSHA); err != ErrNothingToCommit { + t.Errorf("want ErrNothingToCommit, got %v", err) + } +}