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) + } +}