Feature Description
Make the payload.branchStrategy / payload.reviseFromBranch coupling foolproof, so that setting reviseFromBranch without also setting branchStrategy: rebase cannot silently no-op.
Problem Statement
As someone dispatching an in-place PR revision, I want setting reviseFromBranch to actually restore the prior attempt, not silently cut a fresh branch.
#1042 gated the reviseFromBranch restore in setupTaskBranch behind branchStrategy == rebase, and made the default reset:
strategy := task.Spec.Payload.BranchStrategy
if strategy == "" { strategy = BranchStrategyReset }
if strategy == BranchStrategyRebase &&
task.Spec.Payload.ReviseFromBranch != "" &&
task.Spec.Kind == AgenticTaskKindIssueFix { /* restore + rebase */ }So a task that sets reviseFromBranch but leaves branchStrategy unset defaults to reset and skips the restore entirely — it cuts a fresh branch and does the opposite of what the caller intended. There is no error and no warning; the payload just silently does nothing with reviseFromBranch.
This already bit #1043 (the --revise-from-branch CLI flag stamped reviseFromBranch but not branchStrategy, so the flag was a no-op post-#1042). Every future caller that sets reviseFromBranch is one forgotten field away from the same silent failure.
Proposed Solution
Pick one (or combine):
- Default to rebase when
reviseFromBranch is set (recommended). In setupTaskBranch, when branchStrategy is empty and reviseFromBranch != "", treat the effective strategy as rebase. Restore-and-rebase is the only sensible intent when a prior attempt is named; the explicit reset still wins when set. Callers can stop threading branchStrategy on the revision path.
- Validate the pairing. The AgenticTask validating webhook (and/or the CLI) rejects
reviseFromBranch set with branchStrategy: reset as a contradiction, so the mistake fails loud at admission instead of silently at execution.
- Warn. Log a clear warning when
reviseFromBranch is set but the effective strategy is reset, so the no-op is at least visible in the executor logs.
Alternatives Considered
Additional Context
Priority
Willingness to Contribute
Feature Description
Make the
payload.branchStrategy/payload.reviseFromBranchcoupling foolproof, so that settingreviseFromBranchwithout also settingbranchStrategy: rebasecannot silently no-op.Problem Statement
#1042 gated the
reviseFromBranchrestore insetupTaskBranchbehindbranchStrategy == rebase, and made the defaultreset:So a task that sets
reviseFromBranchbut leavesbranchStrategyunset defaults toresetand skips the restore entirely — it cuts a fresh branch and does the opposite of what the caller intended. There is no error and no warning; the payload just silently does nothing withreviseFromBranch.This already bit #1043 (the
--revise-from-branchCLI flag stampedreviseFromBranchbut notbranchStrategy, so the flag was a no-op post-#1042). Every future caller that setsreviseFromBranchis one forgotten field away from the same silent failure.Proposed Solution
Pick one (or combine):
reviseFromBranchis set (recommended). InsetupTaskBranch, whenbranchStrategyis empty andreviseFromBranch != "", treat the effective strategy asrebase. Restore-and-rebase is the only sensible intent when a prior attempt is named; the explicitresetstill wins when set. Callers can stop threadingbranchStrategyon the revision path.reviseFromBranchset withbranchStrategy: resetas a contradiction, so the mistake fails loud at admission instead of silently at execution.reviseFromBranchis set but the effective strategy isreset, so the no-op is at least visible in the executor logs.Alternatives Considered
Additional Context
resetdefault, so whatever lands here must keep an explicitresethonored; only the empty-plus-reviseFromBranchcase should change.Priority
Willingness to Contribute