From 0acc5ca87bd5dd3a4ea50a1a9e0bf015a1c5b69d Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 25 Jun 2026 22:57:47 -0400 Subject: [PATCH] fix(hotfix): fail loudly when a remote env tip diverges from recorded state The hotfix cherry-pick base is the recorded state SHA, but the existing divergence guard only inspected a local branch, which never exists in CI (only remote-tracking refs are fetched). A diverged env/ tip went undetected, the cherry-pick landed on a stale base, and the run died with a confusing merge-poll timeout. Add a plan-time check that compares the fetched remote env tip to the recorded state SHA and fails with an actionable message (env name, both SHAs, replay guidance) when they diverge. No-op when the ref is absent or in sync, so the normal path is unchanged. The generated workflow already fetches the refs, so no regeneration is needed. Signed-off-by: Joshua Temple --- internal/hotfix/chain.go | 30 ++++++++++++ internal/hotfix/chain_test.go | 89 +++++++++++++++++++++++++++++++++++ internal/hotfix/plan.go | 35 ++++++++++++-- 3 files changed, 151 insertions(+), 3 deletions(-) diff --git a/internal/hotfix/chain.go b/internal/hotfix/chain.go index 95d997da..825ad320 100644 --- a/internal/hotfix/chain.go +++ b/internal/hotfix/chain.go @@ -130,6 +130,27 @@ func commitPresentInEnv(fixSHA, baseSHA string, patches []string) (bool, error) return already, nil } +// verifyRemoteEnvTip fails the plan when the fetched remote env branch tip has +// diverged from the recorded base SHA. When the remote ref is absent (the env +// has never been hotfixed, so the apply job will create env/ at baseSHA) +// there is nothing to diverge from and the check passes, leaving the normal +// path untouched. This mirrors reconcileBranch's local-branch guard but runs +// against the remote-tracking ref the generated hotfix workflow fetches, which +// is the only env ref present in CI. +func (p *Planner) verifyRemoteEnvTip(branch, baseSHA string) error { + tip, exists, err := p.gitRunner.RemoteBranchSHA(p.remote, branch) + if err != nil { + return fmt.Errorf("reading remote tip of %s: %w", branch, err) + } + if !exists { + return nil + } + if tip != baseSHA { + return envTipDivergenceError(branch, tip, baseSHA) + } + return nil +} + // PlanChain validates and computes the per-environment plan for elevating a set // of trunk commits across the bottom-up environment chain up to and including // targetEnv. Commits are kept in the caller's ref order; environments run @@ -169,6 +190,15 @@ func (p *Planner) PlanChain(refs []string, targetEnv string) (*PlanChainResult, } baseSHA := state.SHA + // Guard against a remote env branch that has drifted from recorded state + // before deriving a cherry-pick base from it. The base is state.SHA, but + // the apply job opens the resolution PR against the live env/ branch; + // if the fetched remote tip has diverged the cherry-pick lands on a stale + // base and the PR is unmergeable, surfacing only as a merge-poll timeout. + if err := p.verifyRemoteEnvTip(envBranch(env), baseSHA); err != nil { + return nil, err + } + ep := EnvPlan{ Env: env, Branch: envBranch(env), diff --git a/internal/hotfix/chain_test.go b/internal/hotfix/chain_test.go index 3ec0ac35..28c059f2 100644 --- a/internal/hotfix/chain_test.go +++ b/internal/hotfix/chain_test.go @@ -353,3 +353,92 @@ func TestPlanChain_RejectsNonTrunkCommit(t *testing.T) { t.Errorf("error %q should mention trunk", err.Error()) } } + +// setRemoteEnvTip points the remote-tracking ref the generated workflow fetches +// (refs/remotes/origin/env/) at sha, simulating a fetched remote env branch +// whose tip the planner can inspect. Using update-ref avoids needing a real +// second repository to act as origin. +func setRemoteEnvTip(t *testing.T, env, sha string) { + t.Helper() + runGit(t, "update-ref", "refs/remotes/origin/env/"+env, sha) +} + +// short7 mirrors the planner's short() truncation so the divergence assertions +// can check that both SHAs surface in the operator-facing error. +func short7(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} + +// TestPlanChain_RemoteEnvTipDiverged_FailsWithGuidance asserts the planner +// aborts when the fetched remote env/ tip no longer matches the recorded +// state SHA the cherry-pick base is derived from. Without this guard the apply +// job cherry-picks onto the stale base and opens a PR GitHub finds unmergeable, +// surfacing only as a merge-poll timeout. +func TestPlanChain_RemoteEnvTipDiverged_FailsWithGuidance(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + diverged := commitFile(t, "b.txt", "two", "out-of-band env edit") + fix := commitFile(t, "c.txt", "three", "fix on trunk") + + // test records base as its state SHA, but the remote env/test tip has moved + // on to diverged: recorded state and the branch no longer agree. + manifest := writeManifestFull(t, []string{"dev", "test", "prod"}, map[string]envSpec{ + "dev": {sha: fix}, + "test": {sha: base}, + "prod": {sha: base}, + }) + setRemoteEnvTip(t, "test", diverged) + + p := newPlanner(t, manifest) + _, err := p.PlanChain([]string{fix}, "test") + if err == nil { + t.Fatal("expected divergence error when remote env tip differs from recorded state SHA") + } + msg := err.Error() + if !strings.Contains(msg, "env/test") { + t.Errorf("error %q should name the diverged env branch", msg) + } + if !strings.Contains(msg, short7(diverged)) || !strings.Contains(msg, short7(base)) { + t.Errorf("error %q should report both the branch tip %s and the recorded SHA %s", + msg, short7(diverged), short7(base)) + } + if !strings.Contains(strings.ToLower(msg), "replay") { + t.Errorf("error %q should give reconcile/replay guidance", msg) + } +} + +// TestPlanChain_RemoteEnvTipInSync_PlansCleanly is the negative control: when +// the remote env/ tip equals the recorded state SHA the plan proceeds +// exactly as before, proving the guard fires only on genuine divergence. +func TestPlanChain_RemoteEnvTipInSync_PlansCleanly(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + fix := commitFile(t, "c.txt", "three", "fix on trunk") + + manifest := writeManifestFull(t, []string{"dev", "test", "prod"}, map[string]envSpec{ + "dev": {sha: fix}, + "test": {sha: base}, + "prod": {sha: base}, + }) + // Remote env tip agrees with recorded state: no divergence. + setRemoteEnvTip(t, "test", base) + + p := newPlanner(t, manifest) + res, err := p.PlanChain([]string{fix}, "test") + if err != nil { + t.Fatalf("PlanChain with in-sync remote tip should succeed: %v", err) + } + if len(res.Envs) != 1 || res.Envs[0].Env != "test" { + t.Fatalf("expected a single test env plan, got %+v", res.Envs) + } + ep := res.Envs[0] + if ep.NoOp || len(ep.Commits) != 1 || ep.Commits[0] != fix { + t.Errorf("test env should plan the fix unchanged, got %+v", ep) + } + if ep.BaseSHA != base { + t.Errorf("base SHA = %q, want %q", ep.BaseSHA, base) + } +} diff --git a/internal/hotfix/plan.go b/internal/hotfix/plan.go index 50162a7d..b297aeab 100644 --- a/internal/hotfix/plan.go +++ b/internal/hotfix/plan.go @@ -64,6 +64,10 @@ type gitRunner interface { LocalBranchExists(name string) (bool, error) // LocalBranchSHA returns the tip SHA of a local branch. LocalBranchSHA(name string) (string, error) + // RemoteBranchSHA returns the tip SHA of a remote-tracking branch + // (refs/remotes//) and whether that ref exists. A missing ref + // returns ("", false, nil); only an unexpected git failure returns an error. + RemoteBranchSHA(remote, name string) (string, bool, error) // CreateBranch creates a branch pointing at sha. CreateBranch(name, sha string) error } @@ -97,6 +101,20 @@ func (execGitRunner) LocalBranchSHA(name string) (string, error) { return strings.TrimSpace(string(out)), nil } +func (execGitRunner) RemoteBranchSHA(remote, name string) (string, bool, error) { + ref := "refs/remotes/" + remote + "/" + name + out, err := exec.Command("git", "rev-parse", "--verify", "--quiet", ref).Output() + if err != nil { + // rev-parse --quiet exits non-zero with no output when the ref is + // absent; treat that as "remote branch not fetched", not a hard failure. + if _, ok := err.(*exec.ExitError); ok { + return "", false, nil + } + return "", false, fmt.Errorf("git rev-parse %s: %w", ref, err) + } + return strings.TrimSpace(string(out)), true, nil +} + func (execGitRunner) CreateBranch(name, sha string) error { if out, err := exec.Command("git", "branch", name, sha).CombinedOutput(); err != nil { return fmt.Errorf("git branch %s %s: %w\n%s", name, sha, err, out) @@ -318,9 +336,7 @@ func (p *Planner) reconcileBranch(branch, baseSHA string) (bool, error) { return false, fmt.Errorf("reading tip of %s: %w", branch, err) } if tip != baseSHA { - return false, fmt.Errorf( - "branch %s tip %s does not match recorded state SHA %s; this indicates an interrupted hotfix or manual edits: replay the hotfix workflow for the open PR, or reset %s to %s, before re-running", - branch, short(tip), short(baseSHA), branch, short(baseSHA)) + return false, envTipDivergenceError(branch, tip, baseSHA) } return false, nil } @@ -335,6 +351,19 @@ func (p *Planner) reconcileBranch(branch, baseSHA string) (bool, error) { return true, nil } +// envTipDivergenceError reports that an env branch tip no longer matches the +// recorded state SHA the cherry-pick base is derived from. It names the env +// branch and both SHAs and gives the operator the recovery path: replay the +// hotfix so recorded state matches the branch, or reset the branch back to the +// recorded SHA. Both the local-branch guard (reconcileBranch) and the chain +// path's remote-tip guard (verifyRemoteEnvTip) raise this single message so the +// two divergence checks stay in lockstep. +func envTipDivergenceError(branch, tip, baseSHA string) error { + return fmt.Errorf( + "branch %s tip %s does not match recorded state SHA %s; this indicates an interrupted hotfix or manual edits: replay the hotfix workflow for the open PR, or reset %s to %s, before re-running", + branch, short(tip), short(baseSHA), branch, short(baseSHA)) +} + // hotfixVersionCandidate returns the next free hotfix version over the base of // envVersion. An rc version yields its first nested hotfix segment. func hotfixVersionCandidate(envVersion string) (string, error) {