fix(foreman): fail loudly when a revision's prior attempt cannot be restored - #1365
fix(foreman): fail loudly when a revision's prior attempt cannot be restored#1365joryirving wants to merge 1 commit into
Conversation
…estored setupTaskBranch treated a reviseFromBranch restore miss as a soft condition: log, fall through to a fresh base-cut, proceed. Combined with allowOverwrite — which revisions and pr-fixes always carry — the base-cut then force-pushed over the surviving remote ref, silently destroying the prior attempt. Observed in the wild (defilantech#1364): a reviewed 554-line migration replaced by a +3/−1 CI tweak, three force-pushes in one day, two sibling PRs merged as empty lockfile husks that falsely closed their issues. A caller that names a prior attempt is asserting it exists; a miss means the ref was pruned or a concurrent fix attempt is racing this one — both conditions to surface, not paper over. Return an explicit error (the task fails with FailureCloneFailed carrying the message) instead of falling back. The base-branch path remains for the reset strategy and non-revision tasks, where no prior attempt is claimed. TestSetupTaskBranch_MissingRefFallsBackToBase (which pinned the old fallback) is inverted into TestSetupTaskBranch_MissingRefFailsLoud. Fixes defilantech#1364 AI assistance: authored with Claude Code. Signed-off-by: Jory Irving <jory.irving@stackadapt.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Defilan
left a comment
Defilan
left a comment
There was a problem hiding this comment.
Jory, clean safety fix, thank you. I traced it: the found == false path now returns instead of silently cutting from base, it's correctly gated to rebase revisions (first attempts carry no reviseFromBranch, so they're untouched), and it strengthens the honest-verdict guarantee rather than threatening it, a restore miss now surfaces as a failure instead of a GO on a wrong base. The regression test bites, and CI is green. One tiny thing: the AI-assistance line is in the commit message, but our policy keeps commits attribution-free and puts the disclosure in the PR body (it's already there), so please drop that line from the commit on the squash. Approving.
2 participants
What
A
reviseFromBranchrestore miss now fails the task with an explicit error instead of silently falling back to a fresh base-cut.Why
Fixes #1364
The fallback was a data-loss primitive: revisions and pr-fixes always carry
allowOverwrite, so the base-cut force-pushed over the surviving remote ref — reviewed work destroyed with no error anywhere. Observed blast radius in one day: a reviewed 554-line migration replaced by a +3/−1 CI tweak (pr-reviewer-action#444, three force-pushes, reviewer approval auto-dismissed each time), plus two sibling PRs merged as lockfile-only husks that falsely closed their issues (miso-chat#726/#727 → #718/#712 reopened).A caller that names a prior attempt is asserting it exists. A miss means the ref was pruned or — the prime suspect given three same-day occurrences — a concurrent fix attempt raced this one; both are conditions to surface. The failed task lands in the existing retry/escalation ladder instead of eating the branch.
How
setupTaskBranch: thefound == falsebranch returns an error (task fails asFailureCloneFailedwith the full message: ref, suspected causes, what was refused). The base-branch path is unchanged for theresetstrategy and non-revision tasks, where no prior attempt is claimed. Happy to add a dedicatedFailureRestoreFailedreason instead if you'd prefer the enum churn.TestSetupTaskBranch_MissingRefFallsBackToBase(which pinned the old fallback) inverted intoTestSetupTaskBranch_MissingRefFailsLoud; the restore/reset/default-strategy tests are untouched and green.Checklist
make testpasses locally (agent 88.0%, controller 82.8%)make lintpasses locally (0 issues)