Skip to content

feat(foreman): executor-owned revise-from-branch restore for revision tasks#967

Merged
Defilan merged 1 commit into
defilantech:mainfrom
joryirving:feat/revise-from-branch
Jul 3, 2026
Merged

feat(foreman): executor-owned revise-from-branch restore for revision tasks#967
Defilan merged 1 commit into
defilantech:mainfrom
joryirving:feat/revise-from-branch

Conversation

@joryirving

Copy link
Copy Markdown
Collaborator

What

Executor-owned branch restore for revision tasks: new AgenticTaskPayload.reviseFromBranch names a ref on the push remote, and the executor cuts the working branch from that ref (fetch + checkout -B from FETCH_HEAD) instead of from baseBranch, so a revision coder starts with its prior attempt's files present. On revision tasks, fetchIssueBodyIfNeeded now appends the GitHub issue under an ## Original issue (#N) heading behind the review-feedback prompt instead of suppressing the fetch.

Why

Without this, a review-feedback retry runs in a fresh workspace cut from the upstream base: it rebuilds from main, sees only the feedback (losing the original ask), and force-overwrites the prior attempt with a feedback-only diff. #951's live validation showed the prompt-driven alternative (model runs git fetch/checkout itself) dies under stuck-loop forcing windows — the executor owns git everywhere else and should own the restore too.

Part of #951 (the executor-restore half; revision profile pairing rides with the iteration loop in #959, which stacks on this branch).

How

  • repo.CreateBranchFromRemoteRef: ls-remote probe, then fetch + checkout -B <branch> FETCH_HEAD. A missing ref returns found=false without error; transport/auth failures error loudly. Same argv-safety guards as CreateBranchFromUpstream.
  • Executor setupTaskBranch (extracted from Execute step 4): issue-fix tasks with reviseFromBranch restore from origin; a pruned/never-pushed ref logs and falls back to the existing branch-from-upstream-base path ([FEATURE] Foreman coder: branch from upstream/main, not the fork's default branch (stale-base fix) #813) rather than failing. Base-branch and clone-HEAD paths unchanged.
  • Issue-body append is gated on reviseFromBranch being set, so composed prompts on non-revision tasks (bridge/hand-authored pipelines) keep the pre-existing "non-empty prompt suppresses the fetch" contract.
  • CRD manifests + foreman chart CRDs regenerated.

Standalone useful: hand-authored Pipeline Workloads and external retry bridges can set reviseFromBranch directly today; the controller starts stamping it in #959.

Verification: go test ./pkg/foreman/... (new unit tests: restore from existing ref, missing-ref fallback, argv validation, issue-append on revision tasks, composed-prompt no-op); KUBEBUILDER_ASSETS=... go test ./internal/foreman/controller/...; golangci-lint run ./internal/... ./pkg/... ./api/... 0 issues; gofmt -l clean.

AI assistance: authored by Claude (Fable 5) operating under the maintainer's direction; reviewed before submitting.

Checklist

  • Tests added/updated
  • make test passes locally
  • make lint passes locally
  • Commit messages follow conventional commits
  • All commits are signed off (git commit -s) per DCO
  • AI assistance (if any) is disclosed above, per CONTRIBUTING.md
  • Documentation updated (if user-facing change)

… tasks

A revision coder task had no way to start from its prior attempt: every
task runs in a fresh workspace and the working branch is cut from a
fresh fetch of the upstream base, so a review-feedback retry rebuilt
from main and force-overwrote the prior attempt with a feedback-only
diff. defilantech#951's live validation also showed the prompt-driven alternative
(model runs git fetch/checkout itself) dies under stuck-loop forcing
windows; the executor owns git everywhere else and now owns this too.

- AgenticTaskPayload.reviseFromBranch (+ CRD/chart sync): names a ref
  on the push remote to cut the working branch from instead of
  baseBranch. Issue-fix only.
- repo.CreateBranchFromRemoteRef: ls-remote probe, then fetch +
  checkout -B from FETCH_HEAD. A missing ref reports found=false (no
  error) so the caller can fall back; transport/auth failures error
  loudly. Same argv-safety guards as CreateBranchFromUpstream.
- executor setupTaskBranch: issue-fix tasks with reviseFromBranch
  restore the prior attempt from origin; a pruned or never-pushed ref
  logs and falls back to the branch-from-upstream-base path (defilantech#813)
  instead of failing the task.
- fetchIssueBodyIfNeeded: revision tasks append the issue body under an
  "## Original issue (#N)" heading behind their review-feedback prompt
  instead of suppressing the fetch, so a retry keeps the original ask
  and acceptance criteria. Composed prompts on non-revision tasks stay
  untouched (pre-defilantech#951 contract).

Part of defilantech#951 (the executor-restore half; revision profile pairing rides
with the iteration loop in defilantech#959).

Signed-off-by: Jory Irving <jory.irving@stackadapt.com>
@joryirving joryirving requested a review from Defilan as a code owner July 3, 2026 14:25
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.66667% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/foreman/agent/repo/branch.go 63.63% 4 Missing and 4 partials ⚠️
pkg/foreman/agent/executor_native.go 84.21% 4 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

joryirving added a commit to joryirving/LLMKube that referenced this pull request Jul 3, 2026
…e and a revision coder profile

defilantech#959's review held the iteration loop for defilantech#951: the retry coder task
rebuilt from main (nothing restored the prior attempt) and ran under
the issue-fix forcing profile that collapsed revision tasks in defilantech#951's
live validation. With the executor restore landed (defilantech#967), wire the
loop to it:

- reviewIterationSteps stamps payload.reviseFromBranch = the task's
  own branch name on iteration coder steps, alongside allowOverwrite.
  The prior attempt lives at that ref on the push remote; the executor
  now checks it out instead of resetting to the upstream base, so the
  retry amends r0 rather than force-overwriting it with a
  feedback-only diff.
- new Workload.spec.revisionCoderAgentRef (+ CRD/chart sync):
  iteration coder steps reference it when set, pairing revisions with
  a revision-tuned profile instead of the issue-fix forcing profile.
  When unset they fall back to coderAgentRef and the reconciler emits
  a Warning event (RevisionUnderIssueFixProfile) on the Workload —
  WorkloadReconciler grows the operator-standard events.EventRecorder
  (nil-safe for tests), wired in foreman-operator main; operator RBAC
  gains events.k8s.io create/patch.
- reviewFeedbackPrompt now matches the real semantics: the workspace
  starts from the restored prior attempt, so it directs the model to
  amend that work as a delta, not rebuild from scratch.

spec.maxReviewIterations keeps its default of 1: with the restore in
place an on-by-default iteration can no longer destroy a passing base
attempt.

Closes defilantech#951

Signed-off-by: Jory Irving <jory.irving@stackadapt.com>

@Defilan Defilan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Approve. This cleanly implements the executor-restore half of #951 and directly targets the force-overwrite flaw I flagged on #959.

Verified:

  • New AgenticTaskPayload.ReviseFromBranch field, CRDs regenerated and in sync across all four files (config + chart, byte-identical pairs), no deepcopy needed for a string.
  • The load-bearing part is correct: CreateBranchFromRemoteRef does ls-remote probe -> fetch origin <ref> -> checkout -B <branch> FETCH_HEAD, so the working branch ends up carrying the prior attempt's commits/files, not a fresh main. TestSetupTaskBranch_RestoresPriorAttempt asserts HEAD == priorSHA and that the restored file exists.
  • Correctly gated: only runs when ReviseFromBranch != "" && Kind == issue-fix; unset falls through to the unchanged upstream-base path, so normal issue-fix tasks are unaffected.
  • Failure handling is right: missing ref falls back to base (logged), but a real transport/auth failure returns an error rather than silently rebuilding from main and force-overwriting. Both paths tested. Ref argv-safety reused via gitRefSafe.
  • Stands alone (pure executor-layer, field defaults empty), DCO signed.

This gives #959 the exact lever it needs: once the iteration coder task stamps payload.ReviseFromBranch = <its push-remote branch>, the coder edits on top of prior work and the subsequent force-with-lease is safe and correct (prior + new edits, not a feedback-only diff rebuilt from main). Nice work.

@Defilan Defilan merged commit b76051c into defilantech:main Jul 3, 2026
24 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 3, 2026
joryirving added a commit to joryirving/LLMKube that referenced this pull request Jul 3, 2026
…e and a revision coder profile

defilantech#959's review held the iteration loop for defilantech#951: the retry coder task
rebuilt from main (nothing restored the prior attempt) and ran under
the issue-fix forcing profile that collapsed revision tasks in defilantech#951's
live validation. With the executor restore landed (defilantech#967), wire the
loop to it:

- reviewIterationSteps stamps payload.reviseFromBranch = the task's
  own branch name on iteration coder steps, alongside allowOverwrite.
  The prior attempt lives at that ref on the push remote; the executor
  now checks it out instead of resetting to the upstream base, so the
  retry amends r0 rather than force-overwriting it with a
  feedback-only diff.
- new Workload.spec.revisionCoderAgentRef (+ CRD/chart sync):
  iteration coder steps reference it when set, pairing revisions with
  a revision-tuned profile instead of the issue-fix forcing profile.
  When unset they fall back to coderAgentRef and the reconciler emits
  a Warning event (RevisionUnderIssueFixProfile) on the Workload —
  WorkloadReconciler grows the operator-standard events.EventRecorder
  (nil-safe for tests), wired in foreman-operator main; operator RBAC
  gains events.k8s.io create/patch.
- reviewFeedbackPrompt now matches the real semantics: the workspace
  starts from the restored prior attempt, so it directs the model to
  amend that work as a delta, not rebuild from scratch.

spec.maxReviewIterations keeps its default of 1: with the restore in
place an on-by-default iteration can no longer destroy a passing base
attempt.

Closes defilantech#951

Signed-off-by: Jory Irving <jory.irving@stackadapt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants