diff --git a/api/foreman/v1alpha1/agentictask_types.go b/api/foreman/v1alpha1/agentictask_types.go index 9431936f..38cd5d48 100644 --- a/api/foreman/v1alpha1/agentictask_types.go +++ b/api/foreman/v1alpha1/agentictask_types.go @@ -49,6 +49,25 @@ const ( AgenticTaskKindFreeform AgenticTaskKind = "freeform" ) +// BranchStrategy controls how the executor cuts an issue-fix task's working +// branch relative to the current base, so a re-dispatch of the same issue can +// never revert already-merged work. +// +kubebuilder:validation:Enum=reset;rebase +type BranchStrategy string + +const ( + // BranchStrategyReset cuts the working branch fresh from the CURRENT base + // tip, ignoring any prior attempt (payload.reviseFromBranch). This is the + // default: a fresh issue-fix, retry, or repair re-dispatch redoes the work + // against latest base, so a stale prior branch can never drift from base and + // revert merged commits. + BranchStrategyReset BranchStrategy = "reset" + // BranchStrategyRebase restores the prior attempt (payload.reviseFromBranch) + // and rebases it onto the CURRENT base, so an in-review PR revision carries + // its earlier commits forward on top of merged work instead of reverting it. + BranchStrategyRebase BranchStrategy = "rebase" +) + // AgenticTaskFailureReason categorizes WHY a task did not reach a // "succeeded-on-target" outcome. Distinct from AgenticTaskVerdict // (which carries the externally-meaningful WHAT: GO / NO-GO / @@ -296,6 +315,16 @@ type AgenticTaskPayload struct { // +optional ReviseFromBranch string `json:"reviseFromBranch,omitempty"` + // BranchStrategy selects how the working branch is cut relative to the + // current base (see BranchStrategy). Defaults to "reset": cut fresh from the + // current base and ignore any prior attempt (reviseFromBranch), so a retry + // or repair re-dispatch cannot revert merged work. Set "rebase" for the + // in-review PR revision path, where the prior attempt is restored and + // rebased onto the current base. Issue-fix only. + // +kubebuilder:default=reset + // +optional + BranchStrategy BranchStrategy `json:"branchStrategy,omitempty"` + // Prompt is the agent input. Required for freeform. // +optional Prompt string `json:"prompt,omitempty"` diff --git a/charts/foreman/templates/crds/agentictasks.yaml b/charts/foreman/templates/crds/agentictasks.yaml index b5f8e372..1b15875b 100644 --- a/charts/foreman/templates/crds/agentictasks.yaml +++ b/charts/foreman/templates/crds/agentictasks.yaml @@ -203,6 +203,19 @@ spec: (default derived from the issue's labels via conventional commit prefixes: fix/, feat/, chore/, etc.). type: string + branchStrategy: + default: reset + description: |- + BranchStrategy selects how the working branch is cut relative to the + current base (see BranchStrategy). Defaults to "reset": cut fresh from the + current base and ignore any prior attempt (reviseFromBranch), so a retry + or repair re-dispatch cannot revert merged work. Set "rebase" for the + in-review PR revision path, where the prior attempt is restored and + rebased onto the current base. Issue-fix only. + enum: + - reset + - rebase + type: string gateAdvisories: description: |- GateAdvisories holds non-blocking findings copied from the upstream diff --git a/charts/foreman/templates/crds/workloads.yaml b/charts/foreman/templates/crds/workloads.yaml index 7218ee84..ebbda790 100644 --- a/charts/foreman/templates/crds/workloads.yaml +++ b/charts/foreman/templates/crds/workloads.yaml @@ -469,6 +469,19 @@ spec: (default derived from the issue's labels via conventional commit prefixes: fix/, feat/, chore/, etc.). type: string + branchStrategy: + default: reset + description: |- + BranchStrategy selects how the working branch is cut relative to the + current base (see BranchStrategy). Defaults to "reset": cut fresh from the + current base and ignore any prior attempt (reviseFromBranch), so a retry + or repair re-dispatch cannot revert merged work. Set "rebase" for the + in-review PR revision path, where the prior attempt is restored and + rebased onto the current base. Issue-fix only. + enum: + - reset + - rebase + type: string gateAdvisories: description: |- GateAdvisories holds non-blocking findings copied from the upstream diff --git a/config/crd/bases/foreman.llmkube.dev_agentictasks.yaml b/config/crd/bases/foreman.llmkube.dev_agentictasks.yaml index 3f5dcc02..50a48753 100644 --- a/config/crd/bases/foreman.llmkube.dev_agentictasks.yaml +++ b/config/crd/bases/foreman.llmkube.dev_agentictasks.yaml @@ -202,6 +202,19 @@ spec: (default derived from the issue's labels via conventional commit prefixes: fix/, feat/, chore/, etc.). type: string + branchStrategy: + default: reset + description: |- + BranchStrategy selects how the working branch is cut relative to the + current base (see BranchStrategy). Defaults to "reset": cut fresh from the + current base and ignore any prior attempt (reviseFromBranch), so a retry + or repair re-dispatch cannot revert merged work. Set "rebase" for the + in-review PR revision path, where the prior attempt is restored and + rebased onto the current base. Issue-fix only. + enum: + - reset + - rebase + type: string gateAdvisories: description: |- GateAdvisories holds non-blocking findings copied from the upstream diff --git a/config/crd/bases/foreman.llmkube.dev_workloads.yaml b/config/crd/bases/foreman.llmkube.dev_workloads.yaml index c4697158..9951caa6 100644 --- a/config/crd/bases/foreman.llmkube.dev_workloads.yaml +++ b/config/crd/bases/foreman.llmkube.dev_workloads.yaml @@ -468,6 +468,19 @@ spec: (default derived from the issue's labels via conventional commit prefixes: fix/, feat/, chore/, etc.). type: string + branchStrategy: + default: reset + description: |- + BranchStrategy selects how the working branch is cut relative to the + current base (see BranchStrategy). Defaults to "reset": cut fresh from the + current base and ignore any prior attempt (reviseFromBranch), so a retry + or repair re-dispatch cannot revert merged work. Set "rebase" for the + in-review PR revision path, where the prior attempt is restored and + rebased onto the current base. Issue-fix only. + enum: + - reset + - rebase + type: string gateAdvisories: description: |- GateAdvisories holds non-blocking findings copied from the upstream diff --git a/internal/foreman/controller/workload_iteration.go b/internal/foreman/controller/workload_iteration.go index 389dbb76..0b08d2df 100644 --- a/internal/foreman/controller/workload_iteration.go +++ b/internal/foreman/controller/workload_iteration.go @@ -244,10 +244,13 @@ func reviewIterationSteps( Repo: w.Spec.Repo, Issue: n, Branch: branch, - // The prior attempt lives at the task's own - // branch name on the push remote; the executor - // restores it before the model runs (#951). + // The prior attempt lives at the task's own branch name on + // the push remote; the executor restores it (#951) and, under + // the rebase strategy, replays it onto the current base so + // this in-review revision does not revert work merged since + // the prior attempt (#1029). ReviseFromBranch: branch, + BranchStrategy: foremanv1alpha1.BranchStrategyRebase, AllowOverwrite: true, Prompt: reviewFeedbackPrompt(noGo), }, diff --git a/pkg/foreman/agent/executor_native.go b/pkg/foreman/agent/executor_native.go index b289ec8b..a3d9a323 100644 --- a/pkg/foreman/agent/executor_native.go +++ b/pkg/foreman/agent/executor_native.go @@ -351,20 +351,22 @@ func (e *NativeAgentLoopExecutor) Execute(ctx context.Context, task *foremanv1al } // setupTaskBranch cuts the task's working branch in the freshly cloned -// workspace. Precedence: +// workspace, honoring payload.branchStrategy (default "reset"). Precedence: // -// 1. payload.reviseFromBranch on an issue-fix task (#951): restore the -// prior attempt by fetching that ref from the push remote ("origin", -// the clone the branch pushes back to) and branching from it, so a -// revision task starts with its prior attempt's files present. The -// executor owns this restore — prompt-driven git proved fragile -// under stuck-loop forcing windows. When the ref is gone from the -// remote (pruned, or the prior attempt never pushed) it logs and -// falls through to (2) rather than failing. -// 2. Upstream base fetch (#813): when the task carries a repo slug, -// fetch the base ref from the upstream project and branch from -// that, so a stale fork default branch cannot produce a stale-base -// branch. Origin stays the fork; the branch still pushes there. +// 1. rebase strategy + payload.reviseFromBranch on an issue-fix task (#951, +// #1029): restore the prior attempt from the push remote ("origin") and +// rebase it onto the CURRENT base, so an in-review revision carries its +// earlier commits forward on top of work merged since instead of reverting +// it. The executor owns this restore+rebase — prompt-driven git proved +// fragile under stuck-loop forcing windows. When the ref is gone from the +// remote (pruned, or the prior attempt never pushed) it logs and falls +// through to (2). Under the default "reset" strategy the restore is skipped +// entirely: a retry or repair re-dispatch redoes the work fresh from base, +// so a stale prior branch can never drift from base and revert merged work. +// 2. Upstream base fetch (#813): when the task carries a repo slug, fetch the +// base ref from the upstream project and branch from that (the reset path), +// so a stale fork default branch cannot produce a stale-base branch. Origin +// stays the fork; the branch still pushes there. // 3. Clone-HEAD checkout for freeform tasks without a repo slug. func setupTaskBranch( ctx context.Context, @@ -374,8 +376,20 @@ func setupTaskBranch( auth *repo.Auth, log logr.Logger, ) error { - if ref := task.Spec.Payload.ReviseFromBranch; ref != "" && + strategy := task.Spec.Payload.BranchStrategy + if strategy == "" { + strategy = foremanv1alpha1.BranchStrategyReset + } + + // rebase (in-review revision): restore the prior attempt AND rebase it onto + // the current base, so its commits replay on top of work merged since rather + // than reverting it. reset (the default) skips the restore entirely and cuts + // fresh from the current base below, so a retry or repair re-dispatch can + // never carry a stale branch that drifts from base. + if strategy == foremanv1alpha1.BranchStrategyRebase && + task.Spec.Payload.ReviseFromBranch != "" && task.Spec.Kind == foremanv1alpha1.AgenticTaskKindIssueFix { + ref := task.Spec.Payload.ReviseFromBranch found, err := repo.CreateBranchFromRemoteRef(ctx, repo.RemoteRefBranchOptions{ Workspace: workspace, Branch: branch, @@ -389,8 +403,18 @@ func setupTaskBranch( return err } if found { - log.Info("restored prior attempt for revision", - "reviseFromBranch", ref, "branch", branch) + // Replay the restored prior attempt onto the current base. A + // conflict fails the task loud rather than reverting merged work. + if err := repo.RebaseOntoBase(ctx, repo.RebaseOntoBaseOptions{ + Workspace: workspace, + BaseBranch: baseBranch, + UpstreamURL: resolveUpstream(task.Spec.Payload.Repo), + Auth: auth, + }); err != nil { + return err + } + log.Info("restored prior attempt and rebased onto base for revision", + "reviseFromBranch", ref, "branch", branch, "baseBranch", baseBranch) return nil } log.Info("reviseFromBranch ref not found on push remote; falling back to base branch", diff --git a/pkg/foreman/agent/executor_native_internal_test.go b/pkg/foreman/agent/executor_native_internal_test.go index c0e17382..f841d7e8 100644 --- a/pkg/foreman/agent/executor_native_internal_test.go +++ b/pkg/foreman/agent/executor_native_internal_test.go @@ -2231,6 +2231,10 @@ func revisionTask(branch string) *foremanv1alpha1.AgenticTask { Issue: 641, Branch: branch, ReviseFromBranch: branch, + // The in-review revision path restores the prior attempt and + // rebases it onto the current base (#1029). Under the default + // reset strategy the restore is skipped entirely. + BranchStrategy: foremanv1alpha1.BranchStrategyRebase, }, }, } @@ -2308,6 +2312,52 @@ func TestSetupTaskBranch_MissingRefFallsBackToBase(t *testing.T) { } } +// TestSetupTaskBranch_ResetSkipsPriorAttempt pins the #1029 contract: under the +// default "reset" strategy a task carrying reviseFromBranch is NOT restored — +// the branch is cut fresh from the current base — so a retry or repair +// re-dispatch cannot carry a stale prior branch that reverts merged work. +func TestSetupTaskBranch_ResetSkipsPriorAttempt(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + dir := t.TempDir() + bare, mainSHA := seededRemote(t, dir) + const branch = "foreman/wl/issue-641" + + // A prior attempt exists on the remote — it WOULD be restored under rebase. + prior := filepath.Join(dir, "prior") + gitIn(t, "", "clone", bare, prior) + gitIn(t, prior, "checkout", "-b", branch) + if err := os.WriteFile(filepath.Join(prior, "fix.txt"), []byte("attempt 1\n"), 0o644); err != nil { + t.Fatalf("write fix.txt: %v", err) + } + gitIn(t, prior, "add", "fix.txt") + gitIn(t, prior, "commit", "-m", "attempt 1") + gitIn(t, prior, "push", "origin", branch) + + ws := filepath.Join(dir, "ws") + gitIn(t, "", "clone", bare, ws) + + // reset strategy despite reviseFromBranch being set. + task := revisionTask(branch) + task.Spec.Payload.BranchStrategy = foremanv1alpha1.BranchStrategyReset + + err := setupTaskBranch(context.Background(), task, ws, branch, "main", + func(string) string { return bare }, nil, logr.Discard()) + if err != nil { + t.Fatalf("setupTaskBranch: %v", err) + } + if got := gitIn(t, ws, "rev-parse", "HEAD"); got != mainSHA { + t.Errorf("HEAD = %s, want base tip %s (reset cuts fresh, ignores prior attempt)", got, mainSHA) + } + if _, err := os.Stat(filepath.Join(ws, "fix.txt")); err == nil { + t.Error("prior attempt's fix.txt must NOT be present under reset strategy") + } + if got := gitIn(t, ws, "branch", "--show-current"); got != branch { + t.Errorf("current branch = %q, want %q", got, branch) + } +} + // TestRecoverSelfCommitsOrNoChange_StaleForkBaseIsNotCountedAsCommitsAhead // reproduces the #982/#813 scenario where the fork clone's local "main" // lags the upstream tip the task branch was actually cut from. If the diff --git a/pkg/foreman/agent/repo/branch.go b/pkg/foreman/agent/repo/branch.go index d713b2e8..7fee0194 100644 --- a/pkg/foreman/agent/repo/branch.go +++ b/pkg/foreman/agent/repo/branch.go @@ -231,6 +231,70 @@ func CreateBranchFromRemoteRef(ctx context.Context, opts RemoteRefBranchOptions) return true, nil } +// RebaseOntoBaseOptions configures RebaseOntoBase. +type RebaseOntoBaseOptions struct { + // Workspace is the working tree whose current branch is rebased. + Workspace string + // BaseBranch is the ref to rebase onto. Empty defaults to "main". + BaseBranch string + // UpstreamURL is the git URL the base ref is fetched from (the task's + // upstream/own repo). When empty there is no base source (e.g. a freeform + // task) and the rebase is a no-op. + UpstreamURL string + // Auth, when non-nil, provides the GIT_ASKPASS scaffolding for the fetch. + Auth *Auth +} + +// RebaseOntoBase fetches BaseBranch from UpstreamURL and rebases the current +// branch onto that fetched tip, so a restored prior attempt (see +// CreateBranchFromRemoteRef) replays its commits ON TOP of the CURRENT base. +// Any work merged into base since the prior attempt is preserved rather than +// reverted — the bug that made a stale revision branch delete already-merged +// files. A rebase conflict aborts the half-applied rebase and returns an error +// so the task fails loud instead of pushing a branch that reverts merged work. +// When UpstreamURL is empty there is no base to rebase onto and it is a no-op. +func RebaseOntoBase(ctx context.Context, opts RebaseOntoBaseOptions) error { + if opts.Workspace == "" { + return fmt.Errorf("RebaseOntoBase: Workspace is required") + } + if opts.UpstreamURL == "" { + return nil + } + base := opts.BaseBranch + if base == "" { + base = "main" + } + if !gitRefSafe(base) { + return fmt.Errorf("RebaseOntoBase: invalid base branch %q", base) + } + if strings.HasPrefix(opts.UpstreamURL, "-") { + return fmt.Errorf("RebaseOntoBase: invalid upstream url %q", opts.UpstreamURL) + } + + env := baseEnv() + if opts.Auth != nil { + env = append(env, opts.Auth.Env()...) + } + if _, err := runGit(ctx, opts.Workspace, env, "fetch", opts.UpstreamURL, base); err != nil { + return fmt.Errorf("RebaseOntoBase: fetch %s %s: %w", opts.UpstreamURL, base, err) + } + // git rebase re-commits the replayed commits, so it needs a committer + // identity even though each commit's original author is preserved. Supply a + // stable foreman identity so the rebase never fails on a freshly-cloned + // workspace with no user.name/email configured. + rebaseEnv := append(baseEnv(), + "GIT_COMMITTER_NAME=foreman", + "GIT_COMMITTER_EMAIL=foreman@llmkube.dev", + ) + if _, err := runGit(ctx, opts.Workspace, rebaseEnv, "rebase", "FETCH_HEAD"); err != nil { + // Leave the workspace clean: a conflict means the revision genuinely + // clashes with merged work and must fail loud, not silently revert it. + _, _ = runGit(ctx, opts.Workspace, baseEnv(), "rebase", "--abort") + return fmt.Errorf("RebaseOntoBase: rebase onto %s: %w", base, err) + } + return nil +} + // baseEnv is the minimal env for read/local-only git ops that do not // need GIT_ASKPASS (branch, status, log). HOME is carried through so // git can read ~/.gitconfig if present. diff --git a/pkg/foreman/agent/repo/branch_test.go b/pkg/foreman/agent/repo/branch_test.go index 90b41f37..3b584ea4 100644 --- a/pkg/foreman/agent/repo/branch_test.go +++ b/pkg/foreman/agent/repo/branch_test.go @@ -222,6 +222,72 @@ func TestCreateBranchFromRemoteRef(t *testing.T) { } } +// TestRebaseOntoBase verifies a restored prior attempt is replayed ON TOP of +// the CURRENT base, so work merged into base since the prior attempt is +// preserved rather than reverted (the stale-revision-branch bug, LLMKube#1029). +func TestRebaseOntoBase(t *testing.T) { + gitOrSkip(t) + dir := t.TempDir() + + bare := initBareOrigin(t, filepath.Join(dir, "origin")) + seedOrigin(t, bare) + + // Prior attempt: a branch cut from the seed base, adding fix.txt. + const branch = "foreman/wl/issue-1029" + pushPriorAttempt(t, bare, filepath.Join(dir, "prior"), branch, "fix.txt") + + // Base advances AFTER the prior attempt: main gains merged.txt. + mainWork := mustClone(t, bare, filepath.Join(dir, "main-work")) + commitFile(t, mainWork, "merged.txt", "merged since the prior attempt\n") + + // Revision workspace: restore the prior attempt. It is based on the OLD + // base, so merged.txt is absent — a PR of this branch as-is would revert it. + workspace := mustClone(t, bare, filepath.Join(dir, "workspace")) + found, err := CreateBranchFromRemoteRef(context.Background(), RemoteRefBranchOptions{ + Workspace: workspace, Branch: branch, Remote: "origin", Ref: branch, + }) + if err != nil || !found { + t.Fatalf("restore prior attempt: found=%v err=%v", found, err) + } + if _, err := os.Stat(filepath.Join(workspace, "merged.txt")); err == nil { + t.Fatal("test setup: merged.txt should be absent on the restored prior attempt") + } + + // Rebase the prior attempt onto the current base. + if err := RebaseOntoBase(context.Background(), RebaseOntoBaseOptions{ + Workspace: workspace, BaseBranch: "main", UpstreamURL: bare, + }); err != nil { + t.Fatalf("RebaseOntoBase: %v", err) + } + + // Both files present: merged work preserved, prior attempt replayed on top. + if _, err := os.Stat(filepath.Join(workspace, "merged.txt")); err != nil { + t.Errorf("merged.txt must be preserved after rebase (not reverted): %v", err) + } + if _, err := os.Stat(filepath.Join(workspace, "fix.txt")); err != nil { + t.Errorf("prior attempt's fix.txt must be carried forward: %v", err) + } +} + +// TestRebaseOntoBase_NoUpstreamIsNoop verifies an empty UpstreamURL (no base +// source, e.g. a freeform task) is a no-op rather than an error. +func TestRebaseOntoBase_NoUpstreamIsNoop(t *testing.T) { + gitOrSkip(t) + dir := t.TempDir() + bare := initBareOrigin(t, filepath.Join(dir, "origin")) + seedOrigin(t, bare) + workspace := mustClone(t, bare, filepath.Join(dir, "workspace")) + before := gitOut(t, workspace, "rev-parse", "HEAD") + if err := RebaseOntoBase(context.Background(), RebaseOntoBaseOptions{ + Workspace: workspace, BaseBranch: "main", UpstreamURL: "", + }); err != nil { + t.Fatalf("RebaseOntoBase no-op: %v", err) + } + if after := gitOut(t, workspace, "rev-parse", "HEAD"); after != before { + t.Errorf("HEAD moved on no-op rebase: %s -> %s", before, after) + } +} + // TestCreateBranchFromRemoteRef_MissingRef verifies the fallback // contract: a ref absent from the remote (pruned, or the prior attempt // never pushed) returns found=false WITHOUT an error and leaves the