From a76608508d216b8b2f41c41ad92d25bc61a77846 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sun, 28 Jun 2026 11:31:46 -0400 Subject: [PATCH] fix(hotfix): self-heal orphan env branches during plan When an env/ integration branch was left behind by an interrupted hotfix (apply ran and opened a PR, but finalize never ran so state[env].IsDiverged()==false), a subsequent hotfix plan would fail closed on the stale env tip even though the environment was not actually diverged. Introduce a safe self-heal path: - reconcileBranch and verifyRemoteEnvTip accept a singleFlightChecked bool that is only true when a real PRChecker ran and found no open cascade-hotfix PR, proving the branch is an abandoned orphan. - The self-heal fires exclusively when !IsDiverged() AND singleFlightChecked==true; every other mismatch continues to fail closed with the existing envTipDivergenceError. - ResetBranch is added to the gitRunner interface; it force-pushes the remote branch to the recorded base SHA then aligns the local ref. Replace ghPRChecker (gh CLI, absent from the pinned act runner image) with restPRChecker: a stdlib net/http client that reads GITHUB_API_URL, GH_TOKEN, and GITHUB_TOKEN from the environment and supports both bearer token and basic-auth credentials. It follows Link rel=next pagination and fails closed on non-200 responses. The generated plan job gains --repo so the single-flight gate is active in the workflow, job-level contents:write for the force-push, and GH_TOKEN so the REST checker can authenticate. Add unit tests (5 cases) covering the self-heal path, the fail-closed-when-diverged path, the fail-closed-when-not-checked path, the untouched-when-tip-matches path, and the chain mirror. Add REST checker tests (5 cases + 3 nextPageURL unit tests). Add e2e scenario hotfix-orphan-selfheal that creates the orphan condition, advances the recorded base past the stale tip, and proves the plan self-heals instead of aborting. Signed-off-by: Joshua Temple --- e2e/harness/hotfix_actions.go | 13 + e2e/harness/multistep.go | 12 +- .../hotfix/hotfix-orphan-selfheal.yaml | 117 +++++ internal/generate/hotfix.go | 18 + internal/generate/hotfix_test.go | 8 + internal/hotfix/chain.go | 78 +++- internal/hotfix/chain_test.go | 10 +- internal/hotfix/command.go | 192 +++++++-- internal/hotfix/plan.go | 141 +++++-- internal/hotfix/plan_test.go | 17 +- internal/hotfix/pr_checker_test.go | 242 +++++++++++ internal/hotfix/selfheal_test.go | 398 ++++++++++++++++++ 12 files changed, 1162 insertions(+), 84 deletions(-) create mode 100644 e2e/scenarios/hotfix/hotfix-orphan-selfheal.yaml create mode 100644 internal/hotfix/pr_checker_test.go create mode 100644 internal/hotfix/selfheal_test.go diff --git a/e2e/harness/hotfix_actions.go b/e2e/harness/hotfix_actions.go index 9d7a546d..785bd4e8 100644 --- a/e2e/harness/hotfix_actions.go +++ b/e2e/harness/hotfix_actions.go @@ -135,6 +135,19 @@ func (r *Runner) executeHotfixPlan(ctx context.Context, step *HotfixPlanStep) er for name, job := range result.Jobs { r.t.Logf(" - Job '%s': conclusion=%s", name, job.Conclusion) } + + // assert_branch_reset: true is a sharper signal than plan success alone; it + // proves the orphan self-heal diagnostic line was emitted, confirming + // ResetBranch fired rather than the plan succeeding for another reason + // (e.g. branch already at base, branch newly created). + if step.AssertBranchReset { + const marker = "cascade: orphan" + if !strings.Contains(result.Logs, marker) { + r.t.Logf(" HotfixPlan workflow logs:\n%s", result.Logs) + return fmt.Errorf("assert_branch_reset: expected orphan self-heal diagnostic in plan logs but it was not found") + } + r.t.Log(" HotfixPlan: orphan self-heal confirmed (branch_reset=true)") + } return nil } diff --git a/e2e/harness/multistep.go b/e2e/harness/multistep.go index 96d16e3f..2f6999e2 100644 --- a/e2e/harness/multistep.go +++ b/e2e/harness/multistep.go @@ -106,10 +106,14 @@ type Step struct { // workflow's plan job. CommitRef is the trunk commit to plan a hotfix for and is // resolved via the execution context (falling back to a literal SHA). type HotfixPlanStep struct { - CommitRef string `yaml:"commit_ref"` - TargetEnv string `yaml:"target_env"` - DryRun bool `yaml:"dry_run,omitempty"` - ExpectFailure bool `yaml:"expect_failure,omitempty"` + CommitRef string `yaml:"commit_ref"` + TargetEnv string `yaml:"target_env"` + DryRun bool `yaml:"dry_run,omitempty"` + ExpectFailure bool `yaml:"expect_failure,omitempty"` + // AssertBranchReset, when true, asserts that the plan workflow logged the + // orphan self-heal diagnostic line (branch_reset=true), confirming the heal + // fired rather than the plan merely succeeding for another reason. + AssertBranchReset bool `yaml:"assert_branch_reset,omitempty"` } // HotfixApplyStep defines a hotfix_apply action: a harness-driven cherry-pick of diff --git a/e2e/scenarios/hotfix/hotfix-orphan-selfheal.yaml b/e2e/scenarios/hotfix/hotfix-orphan-selfheal.yaml new file mode 100644 index 00000000..28eb4039 --- /dev/null +++ b/e2e/scenarios/hotfix/hotfix-orphan-selfheal.yaml @@ -0,0 +1,117 @@ +name: "Hotfix Orphan Self-Heal" +description: | + Verifies the orphan self-heal path: an env/ integration branch left + behind by an interrupted hotfix (the branch exists but the environment never + recorded divergence) is force-reset back to its recorded base by a later + hotfix plan, instead of failing closed on the stale tip. + + The branch is an orphan because the resolution PR was merged but the divergence + was never finalized, so state[test] reports no divergence while env/test leads + the recorded base. Trunk then advances past that tip. A new hotfix plan for + test, with the single-flight gate satisfied (no open resolution PR), proves the + branch is an abandoned orphan and resets it to the recorded base so the plan + succeeds rather than aborting. + +config: + trunk_branch: main + environments: [dev, test, prod] + builds: + - name: app + workflow: build.yaml + triggers: ["src/**"] + deploys: + - name: deploy-dev + workflow: deploy.yaml + triggers: ["src/**"] + - name: deploy-test + workflow: deploy.yaml + triggers: ["src/**"] + - name: deploy-prod + workflow: deploy.yaml + triggers: ["src/**"] + +steps: + - name: "Initial commit" + action: commit + commit: + message: "feat: add app" + files: + src/app.go: | + package main + func main() {} + + - name: "Orchestrate trunk into dev/test" + action: orchestrate + + - name: "Promote to establish a test/prod baseline" + action: promote + promote: + mode: default + + - name: "Commit a trunk fix to hotfix into test" + action: commit + commit: + message: "fix: patch for test env" + files: + src/fix.go: | + package main + func patch() {} + + - name: "Apply hotfix onto env/test, seeding the integration branch" + action: hotfix_apply + hotfix_apply: + target_env: test + commit_ref: commit2 + + # Merge the resolution PR so no open cascade-hotfix PR remains, but do NOT + # finalize: state[test] never records divergence. env/test now exists and + # leads the recorded base while the environment looks healthy. That is the + # orphan condition the self-heal targets. + - name: "Merge the resolution PR without finalizing divergence" + action: merge_pr + merge_pr: + label: cascade-hotfix + + # Advance trunk and promote so state[test].SHA moves forward, past the orphan + # env/test tip. The recorded base the next hotfix derives now disagrees with + # the stale env/test branch. + - name: "Advance trunk normally" + action: commit + commit: + message: "chore: trunk advance" + files: + src/advance.go: | + package main + func advance() {} + + - name: "Orchestrate the advance into dev/test" + action: orchestrate + + - name: "Promote so test advances past the orphan tip" + action: promote + promote: + mode: default + + - name: "Commit a new trunk fix to hotfix into test" + action: commit + commit: + message: "fix: second patch for test env" + files: + src/fix2.go: | + package main + func patch2() {} + + # The plan runs for real (not a dry run). The generated plan job passes --repo, + # so the single-flight gate runs with a real checker; finding no open + # resolution PR it proves env/test is an abandoned orphan and force-resets it + # to the recorded base. Before the self-heal this plan would fail closed on the + # stale env/test tip; success proves the heal. assert_branch_reset sharpens + # the signal: it checks the diagnostic line emitted when ResetBranch fires, + # distinguishing a genuine self-heal from a plan that succeeds for another + # reason (branch already at base, branch freshly created, etc.). + - name: "Plan hotfix for test self-heals the orphan env branch" + action: hotfix_plan + hotfix_plan: + target_env: test + commit_ref: commit4 + assert_branch_reset: true diff --git a/internal/generate/hotfix.go b/internal/generate/hotfix.go index 29c9d8e1..3c0ffbf1 100644 --- a/internal/generate/hotfix.go +++ b/internal/generate/hotfix.go @@ -218,6 +218,17 @@ func (g *HotfixGenerator) writePlanJob(sb *strings.Builder) { sb.WriteString(" name: Plan Hotfix\n") sb.WriteString(" if: github.event_name == 'workflow_dispatch'\n") sb.WriteString(" runs-on: ubuntu-latest\n") + // The plan job runs the single-flight gate via --repo and self-heals an + // abandoned env branch by force-pushing it back to its recorded base, so it + // needs contents: write. The single-flight lookup lists hotfix PRs, which + // requires pull-requests: read on a private repo. actions: read covers + // workflow introspection. These writes live on the plan job, not at the + // least-privilege top level. + writeJobPermissions(sb, " ", [][2]string{ + {"contents", "write"}, + {"pull-requests", "read"}, + {"actions", "read"}, + }) sb.WriteString(" outputs:\n") sb.WriteString(" branch: ${{ steps.plan.outputs.branch }}\n") sb.WriteString(" fix_sha: ${{ steps.plan.outputs.fix_sha }}\n") @@ -242,6 +253,9 @@ func (g *HotfixGenerator) writePlanJob(sb *strings.Builder) { sb.WriteString(" - name: Plan hotfix\n") sb.WriteString(" id: plan\n") sb.WriteString(" env:\n") + // GH_TOKEN authenticates the single-flight REST API call the planner makes when + // --repo is set. Without it the lookup may fail on private repos and the plan aborts. + sb.WriteString(" GH_TOKEN: ${{ github.token }}\n") sb.WriteString(" HOTFIX_COMMIT: ${{ github.event.inputs.commit }}\n") sb.WriteString(" HOTFIX_TARGET_ENV: ${{ github.event.inputs.target_env }}\n") sb.WriteString(" HOTFIX_DRY_RUN: ${{ github.event.inputs.dry_run }}\n") @@ -250,6 +264,10 @@ func (g *HotfixGenerator) writePlanJob(sb *strings.Builder) { fmt.Fprintf(sb, " --config %s \\\n", g.getManifestFilePath()) sb.WriteString(" --commits \"$HOTFIX_COMMIT\" \\\n") sb.WriteString(" --target-env \"$HOTFIX_TARGET_ENV\" \\\n") + // --repo wires the single-flight PR lookup to a real REST-backed checker. + // Without it the gate is inert (the no-op checker), which both skips the + // single-flight protection and, by design, disables orphan self-heal. + sb.WriteString(" --repo \"${{ github.repository }}\" \\\n") sb.WriteString(" --dry-run=\"$HOTFIX_DRY_RUN\" \\\n") sb.WriteString(" --gha-output\n") diff --git a/internal/generate/hotfix_test.go b/internal/generate/hotfix_test.go index 7abb9035..6914a7ea 100644 --- a/internal/generate/hotfix_test.go +++ b/internal/generate/hotfix_test.go @@ -117,6 +117,14 @@ func TestHotfixGenerator_PlanJobChainOutputs(t *testing.T) { assert.Contains(t, planJob, "env_sequence: ${{ steps.plan.outputs.env_sequence }}", "plan job must expose the env_sequence chain order") + + // The single-flight gate and orphan self-heal only run when --repo wires a + // real gh-backed checker; the plan job must pass the repository slug and carry + // contents: write so the self-heal force-push to origin can land. + assert.Contains(t, planJob, `--repo "${{ github.repository }}"`, + "plan job must pass --repo so the single-flight gate runs with a real checker") + assert.Contains(t, planJob, "contents: write", + "plan job must carry contents: write so the orphan self-heal can force-push env/") for _, key := range []string{ "commits_test: ${{ steps.plan.outputs.commits_test }}", "no_op_test: ${{ steps.plan.outputs.no_op_test }}", diff --git a/internal/hotfix/chain.go b/internal/hotfix/chain.go index 825ad320..905da54d 100644 --- a/internal/hotfix/chain.go +++ b/internal/hotfix/chain.go @@ -35,6 +35,12 @@ type EnvPlan struct { // NoOp is true when the whole requested set is already present in this env. NoOp bool `json:"no_op"` + // BranchReset is true when the remote env/ branch existed at a stale tip + // but was force-reset back to BaseSHA by the orphan self-heal. It is set only + // when the env is not diverged and the single-flight gate ran with a real + // checker that found no open resolution PR. + BranchReset bool `json:"branch_reset"` + // ConflictExpected hints whether a cherry-pick is likely to conflict. The // plan verb does not run the cherry-pick, so this is best-effort and false // by default; the workflow is authoritative. @@ -130,25 +136,39 @@ 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 { +// verifyRemoteEnvTip reconciles the fetched remote env branch tip against 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. When +// the tip has drifted it is either self-healed back to baseSHA or the plan +// aborts fail-closed, following the same orphan-safety rule as reconcileBranch: +// reset only when the env is not diverged AND the single-flight gate ran with a +// real checker that found no open resolution PR. 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. +// +// Returns whether the branch was (or, in dry-run, would be) reset. +func (p *Planner) verifyRemoteEnvTip(branch, baseSHA string, diverged, singleFlightChecked bool) (reset bool, err error) { tip, exists, err := p.gitRunner.RemoteBranchSHA(p.remote, branch) if err != nil { - return fmt.Errorf("reading remote tip of %s: %w", branch, err) + return false, fmt.Errorf("reading remote tip of %s: %w", branch, err) } if !exists { - return nil + return false, nil } if tip != baseSHA { - return envTipDivergenceError(branch, tip, baseSHA) + if !diverged && singleFlightChecked { + if p.dryRun { + return true, nil + } + if err := p.gitRunner.ResetBranch(p.remote, branch, baseSHA); err != nil { + return false, fmt.Errorf("self-healing orphan %s to %s: %w", branch, short(baseSHA), err) + } + return true, nil + } + return false, envTipDivergenceError(branch, tip, baseSHA, diverged) } - return nil + return false, nil } // PlanChain validates and computes the per-environment plan for elevating a set @@ -190,20 +210,34 @@ 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 { + // Single-flight gate per env, BEFORE the remote-tip reconciliation. The + // chain path historically skipped this gate; adding it here is what makes + // the per-env self-heal both enabled (a real checker can set + // singleFlightChecked) and safe (an open resolution PR aborts the plan + // exactly as the single-env path does, so a live hotfix is never reset). + diverged := state.IsDiverged() + singleFlightChecked, err := p.checkSingleFlight(envBranch(env)) + if err != nil { + return nil, err + } + + // Reconcile the remote env branch against 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. An abandoned orphan + // tip is self-healed; an in-progress hotfix stays fail-closed. + reset, err := p.verifyRemoteEnvTip(envBranch(env), baseSHA, diverged, singleFlightChecked) + if err != nil { return nil, err } ep := EnvPlan{ - Env: env, - Branch: envBranch(env), - BaseSHA: baseSHA, - Commits: make([]string, 0, len(shas)), + Env: env, + Branch: envBranch(env), + BaseSHA: baseSHA, + BranchReset: reset, + Commits: make([]string, 0, len(shas)), } for _, sha := range shas { present, err := commitPresentInEnv(sha, baseSHA, state.Patches) diff --git a/internal/hotfix/chain_test.go b/internal/hotfix/chain_test.go index 28c059f2..0dd8c113 100644 --- a/internal/hotfix/chain_test.go +++ b/internal/hotfix/chain_test.go @@ -405,8 +405,14 @@ func TestPlanChain_RemoteEnvTipDiverged_FailsWithGuidance(t *testing.T) { 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) + // With no real PRChecker injected the single-flight gate did not run, so the + // planner must stay fail-closed (no self-heal) and surface actionable + // recovery guidance. + if !strings.Contains(msg, "abandoned hotfix branch") { + t.Errorf("error %q should describe the abandoned branch", msg) + } + if !strings.Contains(msg, "--repo") { + t.Errorf("error %q should point at re-running with --repo", msg) } } diff --git a/internal/hotfix/command.go b/internal/hotfix/command.go index 0b207856..dedc7f1e 100644 --- a/internal/hotfix/command.go +++ b/internal/hotfix/command.go @@ -3,8 +3,11 @@ package hotfix import ( "encoding/json" "fmt" - "os/exec" + "io" + "net/http" + "os" "strings" + "time" "github.com/spf13/cobra" @@ -166,7 +169,7 @@ With --dry-run nothing is mutated (the env branch is planned but not created).`, WithRemote(remote), } if repo != "" { - opts = append(opts, WithPRChecker(newGHPRChecker(repo))) + opts = append(opts, WithPRChecker(newRestPRChecker(repo))) } planner, err := NewPlanner(PlannerOptions{ @@ -224,7 +227,7 @@ With --dry-run nothing is mutated (the env branch is planned but not created).`, cmd.Flags().StringVar(&targetEnv, "target-env", "", "Environment to hotfix (required)") cmd.Flags().StringVar(&actor, "actor", "", "Actor recorded on the plan (default: $GITHUB_ACTOR)") cmd.Flags().StringVar(&remote, "remote", defaultRemote, "Git remote env branches live on") - cmd.Flags().StringVar(&repo, "repo", "", "owner/repo for single-flight PR lookup via gh (default: skip the check)") + cmd.Flags().StringVar(&repo, "repo", "", "owner/repo for single-flight PR lookup via the REST API (default: skip the check)") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Compute the plan without mutating anything") cmd.Flags().BoolVar(&jsonOutput, "json", false, "Output the plan as JSON") cmd.Flags().BoolVar(&ghaOutput, "gha-output", false, "Write outputs to $GITHUB_OUTPUT for workflow consumption") @@ -236,33 +239,150 @@ With --dry-run nothing is mutated (the env branch is planned but not created).`, return cmd } -// ghPRChecker implements PRChecker by shelling out to the gh CLI. -type ghPRChecker struct { +// restPRChecker implements PRChecker via the GitHub/Gitea REST API. It calls +// GET {GITHUB_API_URL}/repos/{owner}/{repo}/pulls?state=open (paginated) and +// filters client-side by base branch and the cascade-hotfix label. This avoids +// depending on the gh CLI, which is not present in the pinned act runner image +// used by the e2e harness, and works identically against both github.com and a +// gitea server (both expose the same endpoint and response fields). +type restPRChecker struct { repo string + // apiBase and token override the corresponding environment variables when + // non-empty, so tests can inject an httptest server URL without setenv. + apiBase string + token string } -func newGHPRChecker(repo string) *ghPRChecker { - return &ghPRChecker{repo: repo} +// newRestPRChecker creates a PRChecker that queries the REST API for owner/repo. +// The API base URL is read from GITHUB_API_URL at call time; the authentication +// token is read from GH_TOKEN (then GITHUB_TOKEN) at call time. Both variables +// are populated automatically in GitHub Actions and by the act e2e harness. +func newRestPRChecker(repo string) *restPRChecker { + return &restPRChecker{repo: repo} } -// OpenHotfixPRs lists open PRs labeled cascade-hotfix whose base is baseBranch. -func (g *ghPRChecker) OpenHotfixPRs(baseBranch string) ([]OpenPR, error) { - out, err := exec.Command("gh", "pr", "list", - "--repo", g.repo, - "--state", "open", - "--base", baseBranch, - "--label", hotfixPRLabel, - "--json", "number,url", - ).Output() - if err != nil { - return nil, fmt.Errorf("gh pr list: %w", err) +// resolvedAPIBase returns the effective REST API base URL. +func (c *restPRChecker) resolvedAPIBase() string { + if c.apiBase != "" { + return c.apiBase + } + if u := os.Getenv("GITHUB_API_URL"); u != "" { + return u } + return "https://api.github.com" +} - var prs []OpenPR - if err := json.Unmarshal(out, &prs); err != nil { - return nil, fmt.Errorf("parsing gh pr list output: %w", err) +// resolvedToken returns the effective authentication token. +func (c *restPRChecker) resolvedToken() string { + if c.token != "" { + return c.token } - return prs, nil + if t := os.Getenv("GH_TOKEN"); t != "" { + return t + } + return os.Getenv("GITHUB_TOKEN") +} + +// prListEntry is the subset of a pull-request REST response the checker needs. +type prListEntry struct { + Number int `json:"number"` + HTMLURL string `json:"html_url"` + Base struct { + Ref string `json:"ref"` + } `json:"base"` + Labels []struct { + Name string `json:"name"` + } `json:"labels"` +} + +// OpenHotfixPRs lists open PRs labeled cascade-hotfix whose base is +// baseBranch. It queries the REST API with pagination (following Link rel=next) +// and filters client-side so the result is correct regardless of which +// server-side filter parameters a given gitea version supports. +func (c *restPRChecker) OpenHotfixPRs(baseBranch string) ([]OpenPR, error) { + client := &http.Client{Timeout: 30 * time.Second} + token := c.resolvedToken() + url := c.resolvedAPIBase() + "/repos/" + c.repo + "/pulls?state=open&per_page=100" + + var result []OpenPR + for url != "" { + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("building PR list request: %w", err) + } + req.Header.Set("Accept", "application/vnd.github+json") + if token != "" { + if i := strings.IndexByte(token, ':'); i >= 0 { + // "username:password" basic-auth credential (used by the act e2e + // harness, which passes the gitea admin credential as GITHUB_TOKEN). + req.SetBasicAuth(token[:i], token[i+1:]) + } else { + req.Header.Set("Authorization", "token "+token) + } + } + + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("PR list request to %s: %w", url, err) + } + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("reading PR list response: %w", readErr) + } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("PR list returned HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + } + + var page []prListEntry + if err := json.Unmarshal(body, &page); err != nil { + return nil, fmt.Errorf("parsing PR list response: %w", err) + } + + for _, pr := range page { + if pr.Base.Ref != baseBranch { + continue + } + for _, l := range pr.Labels { + // Both labels indicate an in-flight hotfix that must block the gate: + // cascade-hotfix is applied to a clean cherry-pick resolution PR; + // cascade-hotfix-conflict is applied when the cherry-pick conflicted + // and a human is actively resolving it. Either label means real work + // is in progress on env/; resetting would destroy that work. + if l.Name == hotfixPRLabel || l.Name == hotfixConflictPRLabel { + result = append(result, OpenPR{Number: pr.Number, URL: pr.HTMLURL}) + break + } + } + } + + url = nextPageURL(resp.Header.Get("Link")) + } + return result, nil +} + +// nextPageURL extracts the rel="next" URL from a GitHub/Gitea Link header. +// It returns the empty string when no next page exists. +func nextPageURL(link string) string { + if link == "" { + return "" + } + for _, part := range strings.Split(link, ",") { + part = strings.TrimSpace(part) + semi := strings.IndexByte(part, ';') + if semi < 0 { + continue + } + rawURL := strings.TrimSpace(part[:semi]) + rel := strings.TrimSpace(part[semi+1:]) + if !strings.HasPrefix(rawURL, "<") || !strings.HasSuffix(rawURL, ">") { + continue + } + if rel == `rel="next"` { + return rawURL[1 : len(rawURL)-1] + } + } + return "" } func writePlanGHAOutput(result *PlanResult) error { @@ -273,6 +393,7 @@ func writePlanGHAOutput(result *PlanResult) error { w.Set("base_sha", result.BaseSHA) w.SetBool("no_op", result.NoOp) w.SetBool("branch_created", result.BranchCreated) + w.SetBool("branch_reset", result.BranchReset) w.Set("hotfix_version_candidate", result.HotfixVersionCandidate) w.SetBool("conflict_expected", result.ConflictExpected) w.SetBool("dry_run", result.DryRun) @@ -280,7 +401,17 @@ func writePlanGHAOutput(result *PlanResult) error { return err } w.SetMultiline("protection_suggestions_text", strings.Join(result.ProtectionSuggestions, "\n")) - return w.Flush() + if err := w.Flush(); err != nil { + return err + } + // Emit a diagnostic line when the orphan self-heal fires so the event is + // observable in workflow logs without requiring a downstream step to echo + // the branch_reset output. The line also serves as the e2e assertion target. + if result.BranchReset { + fmt.Fprintf(os.Stderr, "cascade: orphan %s branch was reset to recorded base %s\n", + result.Branch, short(result.BaseSHA)) + } + return nil } // chainGHAOutputs renders a PlanChainResult into a deterministic, additive set @@ -293,6 +424,7 @@ func writePlanGHAOutput(result *PlanResult) error { // - commits_: comma-joined fix SHAs still to apply for that env // - no_op_: whether that env's whole requested set is already present // - conflict_expected_: best-effort cherry-pick conflict hint +// - reset_: whether the orphan self-heal reset that env branch to its base func chainGHAOutputs(result *PlanChainResult) (simple map[string]string, multiline map[string]string) { simple = make(map[string]string) multiline = make(map[string]string) @@ -303,6 +435,7 @@ func chainGHAOutputs(result *PlanChainResult) (simple map[string]string, multili simple["commits_"+ep.Env] = strings.Join(ep.Commits, ",") simple["no_op_"+ep.Env] = fmt.Sprintf("%v", ep.NoOp) simple["conflict_expected_"+ep.Env] = fmt.Sprintf("%v", ep.ConflictExpected) + simple["reset_"+ep.Env] = fmt.Sprintf("%v", ep.BranchReset) simple["base_"+ep.Env] = ep.BaseSHA } simple["env_sequence"] = strings.Join(envNames, ",") @@ -321,7 +454,18 @@ func writePlanChainGHAOutput(result *PlanChainResult) error { for k, v := range multiline { w.SetMultiline(k, v) } - return w.Flush() + if err := w.Flush(); err != nil { + return err + } + // Emit a diagnostic line for each env whose orphan branch was self-healed so + // the event is observable in workflow logs (mirrors writePlanGHAOutput). + for _, ep := range result.Envs { + if ep.BranchReset { + fmt.Fprintf(os.Stderr, "cascade: orphan %s branch was reset to recorded base %s\n", + ep.Branch, short(ep.BaseSHA)) + } + } + return nil } // printPlanChain renders the human-readable chain plan: the bottom-up env diff --git a/internal/hotfix/plan.go b/internal/hotfix/plan.go index b297aeab..ed75b1cf 100644 --- a/internal/hotfix/plan.go +++ b/internal/hotfix/plan.go @@ -70,6 +70,11 @@ type gitRunner interface { RemoteBranchSHA(remote, name string) (string, bool, error) // CreateBranch creates a branch pointing at sha. CreateBranch(name, sha string) error + // ResetBranch force-updates the remote branch on to point at + // sha, then best-effort aligns the local branch of the same name. It is the + // self-heal primitive: it rewrites history on the env branch, so callers must + // gate it behind the orphan-safety rule (see reconcileBranch). + ResetBranch(remote, name, sha string) error } type execGitRunner struct{} @@ -122,6 +127,22 @@ func (execGitRunner) CreateBranch(name, sha string) error { return nil } +func (execGitRunner) ResetBranch(remote, name, sha string) error { + // Force-update the remote ref first: the remote env branch is the one the + // apply job opens its resolution PR against, so it must carry the corrected + // base even if aligning the local branch later fails. + pushRef := sha + ":refs/heads/" + name + if out, err := exec.Command("git", "push", "--force", remote, pushRef).CombinedOutput(); err != nil { + return fmt.Errorf("git push --force %s %s: %w\n%s", remote, pushRef, err, out) + } + // Align the local branch best-effort. -f creates it when absent, which is + // harmless: the local ref is only a convenience for subsequent local steps. + if out, err := exec.Command("git", "branch", "-f", name, sha).CombinedOutput(); err != nil { + return fmt.Errorf("git branch -f %s %s: %w\n%s", name, sha, err, out) + } + return nil +} + // Planner validates and computes a hotfix plan for one environment. type Planner struct { cicd *config.CICDFile @@ -129,7 +150,12 @@ type Planner struct { dryRun bool remote string prChecker PRChecker - gitRunner gitRunner + // realPRChecker is true only when a non-nil PRChecker was injected via + // WithPRChecker. It distinguishes a genuine single-flight lookup from the + // no-op default, and gates orphan self-heal: the planner resets an env branch + // only when a real checker actually proved no resolution PR is open. + realPRChecker bool + gitRunner gitRunner } // PlannerOptions carries the required inputs for NewPlanner. @@ -154,6 +180,7 @@ func WithPRChecker(c PRChecker) Option { return func(p *Planner) { if c != nil { p.prChecker = c + p.realPRChecker = true } } } @@ -213,6 +240,12 @@ type PlanResult struct { // created at BaseSHA. False when it already existed at the expected tip. BranchCreated bool `json:"branch_created"` + // BranchReset is true when env/ existed at a stale tip but was (or, in + // dry-run, would be) force-reset back to BaseSHA by the orphan self-heal. It is + // set only when the env is not diverged and the single-flight gate ran with a + // real checker that found no open resolution PR. + BranchReset bool `json:"branch_reset"` + // HotfixVersionCandidate is the next free hotfix version over the target // env's current version base (e.g. v1.0.0-rc.1 -> v1.0.0-rc.1.hotfix.1). HotfixVersionCandidate string `json:"hotfix_version_candidate"` @@ -299,69 +332,119 @@ func (p *Planner) Plan(fixRef, targetEnv string) (*PlanResult, error) { // 4. Single-flight: refuse if a hotfix PR already targets env/. This // runs before any branch mutation so a blocked plan leaves no git state. - openPRs, err := p.prChecker.OpenHotfixPRs(branch) + // singleFlightChecked is true only when a real (non-no-op) checker confirmed + // no open resolution PR; it is the precondition for orphan self-heal. + singleFlightChecked, err := p.checkSingleFlight(branch) if err != nil { - return nil, fmt.Errorf("checking for open hotfix PRs: %w", err) - } - if len(openPRs) > 0 { - pr := openPRs[0] - return nil, fmt.Errorf("a hotfix PR (#%d %s) labeled %q already targets %s; resolve and finalize it, then re-dispatch this hotfix", - pr.Number, pr.URL, hotfixPRLabel, branch) + return nil, err } // 5. env/ branch reconciliation. Only after the single-flight gate - // passes do we create or validate the env branch. - created, err := p.reconcileBranch(branch, baseSHA) + // passes do we create, validate, or self-heal the env branch. + diverged := state.IsDiverged() + created, reset, err := p.reconcileBranch(branch, baseSHA, diverged, singleFlightChecked) if err != nil { return nil, err } result.BranchCreated = created + result.BranchReset = reset return result, nil } -// reconcileBranch ensures env/ exists at baseSHA. If absent it is +// checkSingleFlight runs the single-flight open-PR gate for branch. It returns +// the same abort error as before when a hotfix PR is already open against the +// branch. Otherwise it returns whether a real checker ran: true only when a +// non-no-op PRChecker was injected, which is the precondition the orphan +// self-heal requires before it may reset an env branch. +func (p *Planner) checkSingleFlight(branch string) (checked bool, err error) { + openPRs, err := p.prChecker.OpenHotfixPRs(branch) + if err != nil { + return false, fmt.Errorf("checking for open hotfix PRs: %w", err) + } + if len(openPRs) > 0 { + pr := openPRs[0] + return false, fmt.Errorf("a hotfix PR (#%d %s) labeled %q already targets %s; resolve and finalize it, then re-dispatch this hotfix", + pr.Number, pr.URL, hotfixPRLabel, branch) + } + return p.realPRChecker, nil +} + +// reconcileBranch ensures env/ is anchored at baseSHA. If absent it is // created at baseSHA (unless dry-run, where creation is only reported). If -// present its tip must equal baseSHA, otherwise the run is aborted with replay -// guidance. Returns whether the branch was (or would be) created. -func (p *Planner) reconcileBranch(branch, baseSHA string) (bool, error) { +// present and already at baseSHA it is left untouched. If present at a stale tip +// it is either self-healed back to baseSHA or the run aborts fail-closed. +// +// Self-heal safety rule: divergence is recorded ONLY at hotfix finalize, so a +// live in-flight hotfix (open resolution PR, real work on env/) reports +// !diverged while its branch legitimately leads baseSHA. Force-resetting that +// would destroy work. The reset is therefore safe ONLY when the env is not +// diverged AND the single-flight gate actually ran with a real checker that +// found no open hotfix PR. That intersection is exactly the OrphanEnvBranches +// population (an env/* branch with no diverged env behind it) proven not +// in-flight: an abandoned hotfix branch left by an interrupted run. In every +// other mismatch case the planner stays fail-closed with envTipDivergenceError. +// +// Returns whether the branch was (or would be) created and whether it was (or +// would be) reset; at most one of the two is true. +func (p *Planner) reconcileBranch(branch, baseSHA string, diverged, singleFlightChecked bool) (created, reset bool, err error) { exists, err := p.gitRunner.LocalBranchExists(branch) if err != nil { - return false, fmt.Errorf("checking branch %s: %w", branch, err) + return false, false, fmt.Errorf("checking branch %s: %w", branch, err) } if exists { tip, err := p.gitRunner.LocalBranchSHA(branch) if err != nil { - return false, fmt.Errorf("reading tip of %s: %w", branch, err) + return false, false, fmt.Errorf("reading tip of %s: %w", branch, err) } if tip != baseSHA { - return false, envTipDivergenceError(branch, tip, baseSHA) + if !diverged && singleFlightChecked { + if p.dryRun { + return false, true, nil + } + if err := p.gitRunner.ResetBranch(p.remote, branch, baseSHA); err != nil { + return false, false, fmt.Errorf("self-healing orphan %s to %s: %w", branch, short(baseSHA), err) + } + return false, true, nil + } + return false, false, envTipDivergenceError(branch, tip, baseSHA, diverged) } - return false, nil + return false, false, nil } // Branch absent: it will be created at baseSHA. if p.dryRun { - return true, nil + return true, false, nil } if err := p.gitRunner.CreateBranch(branch, baseSHA); err != nil { - return false, fmt.Errorf("creating %s at %s: %w", branch, short(baseSHA), err) + return false, false, fmt.Errorf("creating %s at %s: %w", branch, short(baseSHA), err) } - return true, nil + return true, false, 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 { +// branch and both SHAs and gives an actionable recovery path that depends on +// whether the environment has recorded divergence. 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. +// +// diverged true means the environment carries an in-progress hotfix (divergence +// was recorded at finalize), so the branch tip leads the recorded base on +// purpose and resetting it could discard real work. diverged false means no +// divergence is recorded, so the branch is an abandoned hotfix branch that +// cascade can self-heal once a real single-flight check confirms no resolution +// PR is open. +func envTipDivergenceError(branch, tip, baseSHA string, diverged bool) error { + if diverged { + return fmt.Errorf( + "branch %s tip %s does not match recorded state SHA %s. This environment carries an in-progress hotfix with recorded divergence, so the branch leads the recorded base on purpose. To recover, finalize or abandon the open resolution PR, or reset %s to %s only after confirming no work is lost, then re-run", + branch, short(tip), short(baseSHA), branch, short(baseSHA)) + } 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)) + "branch %s tip %s does not match recorded state SHA %s, and no divergence is recorded, so %s is an abandoned hotfix branch. Re-run the hotfix plan with --repo set so cascade can confirm no resolution PR is open and self-heal the branch, or remove it manually by resetting %s to %s. Run cascade status consistency to list orphan env branches", + branch, short(tip), short(baseSHA), branch, branch, short(baseSHA)) } // hotfixVersionCandidate returns the next free hotfix version over the base of diff --git a/internal/hotfix/plan_test.go b/internal/hotfix/plan_test.go index 252d5118..29e09f4b 100644 --- a/internal/hotfix/plan_test.go +++ b/internal/hotfix/plan_test.go @@ -255,7 +255,7 @@ func TestPlan_DryRunMutatesNothing(t *testing.T) { } } -func TestPlan_ExistingBranchTipMismatch_FailsWithReplayGuidance(t *testing.T) { +func TestPlan_ExistingBranchTipMismatch_FailsClosedWithoutRealChecker(t *testing.T) { newScratchRepo(t) base := commitFile(t, "a.txt", "one", "first") other := commitFile(t, "b.txt", "two", "other") @@ -270,13 +270,24 @@ func TestPlan_ExistingBranchTipMismatch_FailsWithReplayGuidance(t *testing.T) { "prod": base, }) + // No PRChecker is injected, so the single-flight gate did not run with a real + // checker. The planner must fail closed (never self-heal) even though the env + // is not diverged: it cannot prove no resolution PR is open. p := newPlanner(t, manifest) _, err := p.Plan(fix, "test") if err == nil { t.Fatal("expected tip-mismatch error") } - if !strings.Contains(strings.ToLower(err.Error()), "replay") { - t.Errorf("error %q should include replay guidance", err.Error()) + msg := err.Error() + if !strings.Contains(msg, "abandoned hotfix branch") { + t.Errorf("error %q should describe the abandoned branch", msg) + } + if !strings.Contains(msg, "--repo") { + t.Errorf("error %q should point at re-running with --repo", msg) + } + // The local env/test branch must not have been reset to base. + if got := gitOut(t, "rev-parse", "env/test"); got != other { + t.Errorf("env/test tip = %q, want it left at %q (no naive reset)", got, other) } } diff --git a/internal/hotfix/pr_checker_test.go b/internal/hotfix/pr_checker_test.go new file mode 100644 index 00000000..8e23ba42 --- /dev/null +++ b/internal/hotfix/pr_checker_test.go @@ -0,0 +1,242 @@ +package hotfix + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// prTestEntry is the JSON shape the httptest server returns for each PR entry, +// mirroring the subset of GitHub/Gitea pull-request response the checker reads. +type prTestEntry struct { + Number int `json:"number"` + HTMLURL string `json:"html_url"` + Base prTestBase `json:"base"` + Labels []prTestLabel `json:"labels"` +} + +type prTestBase struct { + Ref string `json:"ref"` +} + +type prTestLabel struct { + Name string `json:"name"` +} + +// newPRServer creates an httptest server that returns the given PRs on every +// GET /repos/{owner}/{repo}/pulls request. It does not implement pagination +// unless paginated is set; see newPaginatedPRServer for the two-page variant. +func newPRServer(t *testing.T, prs []prTestEntry) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(prs); err != nil { + t.Errorf("encoding PR list: %v", err) + } + })) +} + +// checker returns a restPRChecker pointed at the given httptest server. +func checker(t *testing.T, ts *httptest.Server, repo string) *restPRChecker { + t.Helper() + return &restPRChecker{repo: repo, apiBase: ts.URL, token: "test-token"} +} + +// (1) A PR that matches the base branch and carries the cascade-hotfix label is returned. +func TestRestPRChecker_MatchingPRReturned(t *testing.T) { + prs := []prTestEntry{ + { + Number: 42, + HTMLURL: "https://example.test/pulls/42", + Base: prTestBase{Ref: "env/test"}, + Labels: []prTestLabel{{Name: hotfixPRLabel}}, + }, + } + ts := newPRServer(t, prs) + defer ts.Close() + + c := checker(t, ts, "owner/repo") + got, err := c.OpenHotfixPRs("env/test") + if err != nil { + t.Fatalf("OpenHotfixPRs: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 PR, got %d", len(got)) + } + if got[0].Number != 42 || got[0].URL != "https://example.test/pulls/42" { + t.Errorf("got %+v, want {42 https://example.test/pulls/42}", got[0]) + } +} + +// (2) A PR whose base branch does not match the requested branch is filtered out. +func TestRestPRChecker_WrongBaseBranchFiltered(t *testing.T) { + prs := []prTestEntry{ + { + Number: 7, + HTMLURL: "https://example.test/pulls/7", + Base: prTestBase{Ref: "env/prod"}, + Labels: []prTestLabel{{Name: hotfixPRLabel}}, + }, + } + ts := newPRServer(t, prs) + defer ts.Close() + + c := checker(t, ts, "owner/repo") + got, err := c.OpenHotfixPRs("env/test") + if err != nil { + t.Fatalf("OpenHotfixPRs: %v", err) + } + if len(got) != 0 { + t.Errorf("expected 0 PRs for a wrong-base PR, got %d: %+v", len(got), got) + } +} + +// (3) A PR that targets the right base but lacks the cascade-hotfix label is filtered out. +func TestRestPRChecker_MissingLabelFiltered(t *testing.T) { + prs := []prTestEntry{ + { + Number: 11, + HTMLURL: "https://example.test/pulls/11", + Base: prTestBase{Ref: "env/test"}, + Labels: []prTestLabel{{Name: "some-other-label"}}, + }, + } + ts := newPRServer(t, prs) + defer ts.Close() + + c := checker(t, ts, "owner/repo") + got, err := c.OpenHotfixPRs("env/test") + if err != nil { + t.Fatalf("OpenHotfixPRs: %v", err) + } + if len(got) != 0 { + t.Errorf("expected 0 PRs for a non-hotfix PR, got %d: %+v", len(got), got) + } +} + +// (4) When the response spans two pages the checker follows the Link rel=next header +// and finds a match on the second page. +func TestRestPRChecker_PaginatedMatchOnPageTwo(t *testing.T) { + page1 := []prTestEntry{ + { + Number: 1, + HTMLURL: "https://example.test/pulls/1", + Base: prTestBase{Ref: "env/prod"}, + Labels: []prTestLabel{{Name: hotfixPRLabel}}, + }, + } + page2 := []prTestEntry{ + { + Number: 99, + HTMLURL: "https://example.test/pulls/99", + Base: prTestBase{Ref: "env/test"}, + Labels: []prTestLabel{{Name: hotfixPRLabel}}, + }, + } + + var page2URL string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.Contains(r.URL.RawQuery, "page=2") { + // Page 2: no next link. + if err := json.NewEncoder(w).Encode(page2); err != nil { + t.Errorf("encoding page 2: %v", err) + } + return + } + // Page 1: add Link header pointing at page 2. + w.Header().Set("Link", fmt.Sprintf(`<%s>; rel="next"`, page2URL)) + if err := json.NewEncoder(w).Encode(page1); err != nil { + t.Errorf("encoding page 1: %v", err) + } + })) + defer ts.Close() + + page2URL = ts.URL + "/repos/owner/repo/pulls?state=open&per_page=100&page=2" + c := &restPRChecker{repo: "owner/repo", apiBase: ts.URL, token: "test-token"} + got, err := c.OpenHotfixPRs("env/test") + if err != nil { + t.Fatalf("OpenHotfixPRs: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 PR from page 2, got %d", len(got)) + } + if got[0].Number != 99 { + t.Errorf("got PR %d, want 99", got[0].Number) + } +} + +// (5) A non-200 response causes an error so the gate fails closed. +func TestRestPRChecker_NonOKResponseReturnsError(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + })) + defer ts.Close() + + c := checker(t, ts, "owner/repo") + _, err := c.OpenHotfixPRs("env/test") + if err == nil { + t.Fatal("expected an error for a non-200 response, got nil") + } + if !strings.Contains(err.Error(), "404") { + t.Errorf("error %q should reference the HTTP status code", err.Error()) + } +} + +// (6) A PR carrying only the cascade-hotfix-conflict label is returned. Conflict +// resolution PRs represent active human work on env/; the single-flight +// gate must catch them to prevent ResetBranch from destroying that work. +func TestRestPRChecker_ConflictLabelPRReturned(t *testing.T) { + prs := []prTestEntry{ + { + Number: 13, + HTMLURL: "https://example.test/pulls/13", + Base: prTestBase{Ref: "env/test"}, + Labels: []prTestLabel{{Name: hotfixConflictPRLabel}}, + }, + } + ts := newPRServer(t, prs) + defer ts.Close() + + c := checker(t, ts, "owner/repo") + got, err := c.OpenHotfixPRs("env/test") + if err != nil { + t.Fatalf("OpenHotfixPRs: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected cascade-hotfix-conflict PR to be returned, got %d PRs", len(got)) + } + if got[0].Number != 13 { + t.Errorf("got PR %d, want 13", got[0].Number) + } +} + +// nextPageURL unit tests. + +func TestNextPageURL_ExtractsNext(t *testing.T) { + link := `; rel="next", ; rel="last"` + got := nextPageURL(link) + if got != "https://api.github.com/repos/o/r/pulls?page=2" { + t.Errorf("nextPageURL(%q) = %q, want the next URL", link, got) + } +} + +func TestNextPageURL_EmptyLinkReturnsEmpty(t *testing.T) { + if got := nextPageURL(""); got != "" { + t.Errorf("nextPageURL(%q) = %q, want empty", "", got) + } +} + +func TestNextPageURL_NoNextReturnsEmpty(t *testing.T) { + link := `; rel="first"` + if got := nextPageURL(link); got != "" { + t.Errorf("nextPageURL(%q) = %q, want empty (no next)", link, got) + } +} diff --git a/internal/hotfix/selfheal_test.go b/internal/hotfix/selfheal_test.go new file mode 100644 index 00000000..aa34415d --- /dev/null +++ b/internal/hotfix/selfheal_test.go @@ -0,0 +1,398 @@ +package hotfix + +import ( + "errors" + "strings" + "testing" +) + +// resetCall records one ResetBranch invocation so tests can assert the orphan +// self-heal fired (or did not) and targeted the recorded base SHA. +type resetCall struct { + remote string + name string + sha string +} + +// fakeGitRunner is a fully programmable gitRunner for the branch-reconciliation +// unit tests. It mutates no real repository: every method returns the configured +// value and ResetBranch/CreateBranch only record their arguments. This keeps the +// self-heal gating logic under test isolated from a live remote (a real reset +// force-pushes, which a unit test cannot satisfy). +type fakeGitRunner struct { + localExists bool + localSHA string + remoteSHA string + remoteExists bool + + creates []resetCall + resets []resetCall + + resetErr error +} + +func (f *fakeGitRunner) ResolveSHA(ref string) (string, error) { return ref, nil } +func (f *fakeGitRunner) LocalBranchExists(string) (bool, error) { + return f.localExists, nil +} +func (f *fakeGitRunner) LocalBranchSHA(string) (string, error) { return f.localSHA, nil } +func (f *fakeGitRunner) RemoteBranchSHA(string, string) (string, bool, error) { + return f.remoteSHA, f.remoteExists, nil +} +func (f *fakeGitRunner) CreateBranch(name, sha string) error { + f.creates = append(f.creates, resetCall{name: name, sha: sha}) + return nil +} +func (f *fakeGitRunner) ResetBranch(remote, name, sha string) error { + f.resets = append(f.resets, resetCall{remote: remote, name: name, sha: sha}) + return f.resetErr +} + +// recordingResetRunner wraps a real gitRunner (execGitRunner in the integration +// tests) and intercepts ResetBranch so a PlanChain run against a real scratch +// repo can assert the self-heal fired without force-pushing to a live remote. +type recordingResetRunner struct { + gitRunner + resets []resetCall + resetErr error +} + +func (r *recordingResetRunner) ResetBranch(remote, name, sha string) error { + r.resets = append(r.resets, resetCall{remote: remote, name: name, sha: sha}) + return r.resetErr +} + +// --- (a) orphan + not diverged + single-flight ran -> self-heal ------------- + +func TestReconcileBranch_OrphanNotDivergedCheckedResets(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + created, reset, err := p.reconcileBranch("env/test", "basesha", false /*diverged*/, true /*singleFlightChecked*/) + if err != nil { + t.Fatalf("reconcileBranch: %v", err) + } + if created { + t.Error("created should be false on a self-heal of an existing branch") + } + if !reset { + t.Error("reset should be true when the orphan branch was healed") + } + if len(fr.resets) != 1 { + t.Fatalf("expected exactly one ResetBranch call, got %d", len(fr.resets)) + } + got := fr.resets[0] + if got.remote != "origin" || got.name != "env/test" || got.sha != "basesha" { + t.Errorf("ResetBranch called with %+v, want {origin env/test basesha}", got) + } +} + +func TestReconcileBranch_OrphanDryRunDoesNotReset(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr, dryRun: true} + + created, reset, err := p.reconcileBranch("env/test", "basesha", false, true) + if err != nil { + t.Fatalf("reconcileBranch: %v", err) + } + if created { + t.Error("created should be false in a dry-run self-heal") + } + if !reset { + t.Error("dry-run should still report it WOULD reset the orphan branch") + } + if len(fr.resets) != 0 { + t.Errorf("dry-run must not call ResetBranch, got %d calls", len(fr.resets)) + } +} + +// --- (b) not diverged but single-flight NOT checked -> fail-closed ---------- + +// This is the critical safety case: without a real single-flight check the +// planner cannot prove the env is not carrying a live in-flight hotfix, so it +// must never reset even though the env is not diverged. +func TestReconcileBranch_NotCheckedFailsClosedNoReset(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + created, reset, err := p.reconcileBranch("env/test", "basesha", false /*diverged*/, false /*singleFlightChecked*/) + if err == nil { + t.Fatal("expected fail-closed divergence error, got nil") + } + if created || reset { + t.Errorf("created=%v reset=%v, both must be false when failing closed", created, reset) + } + if len(fr.resets) != 0 { + t.Fatalf("ResetBranch must NOT be called without a real single-flight check, got %d calls", len(fr.resets)) + } + msg := err.Error() + if !strings.Contains(msg, "abandoned hotfix branch") || !strings.Contains(msg, "--repo") { + t.Errorf("error %q should give the not-diverged actionable guidance", msg) + } +} + +// --- (c) diverged + tip mismatch -> fail-closed (diverged variant) ---------- + +func TestReconcileBranch_DivergedFailsClosedNoReset(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + // diverged=true models a live recorded divergence; even with a real checker + // the branch legitimately leads its base and must not be reset. + created, reset, err := p.reconcileBranch("env/test", "basesha", true /*diverged*/, true /*singleFlightChecked*/) + if err == nil { + t.Fatal("expected fail-closed divergence error, got nil") + } + if created || reset { + t.Errorf("created=%v reset=%v, both must be false when failing closed", created, reset) + } + if len(fr.resets) != 0 { + t.Fatalf("ResetBranch must NOT be called for a diverged env, got %d calls", len(fr.resets)) + } + if !strings.Contains(err.Error(), "in-progress hotfix") { + t.Errorf("error %q should be the diverged variant", err.Error()) + } +} + +// --- (d) stacked re-entry: tip == base -> untouched ------------------------- + +func TestReconcileBranch_TipMatchesBaseUntouched(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "basesha"} + p := &Planner{remote: "origin", gitRunner: fr} + + created, reset, err := p.reconcileBranch("env/test", "basesha", false, true) + if err != nil { + t.Fatalf("reconcileBranch: %v", err) + } + if created || reset { + t.Errorf("created=%v reset=%v, both must be false when the tip already matches", created, reset) + } + if len(fr.resets) != 0 || len(fr.creates) != 0 { + t.Errorf("an in-sync branch must not be created or reset; resets=%d creates=%d", len(fr.resets), len(fr.creates)) + } +} + +func TestReconcileBranch_SelfHealReportsResetError(t *testing.T) { + fr := &fakeGitRunner{localExists: true, localSHA: "staletip", resetErr: errors.New("push rejected")} + p := &Planner{remote: "origin", gitRunner: fr} + + _, _, err := p.reconcileBranch("env/test", "basesha", false, true) + if err == nil { + t.Fatal("expected the ResetBranch failure to surface") + } + if !strings.Contains(err.Error(), "self-healing orphan") { + t.Errorf("error %q should wrap the self-heal failure", err.Error()) + } +} + +// --- (e) chain.go mirror: verifyRemoteEnvTip -------------------------------- + +func TestVerifyRemoteEnvTip_OrphanNotDivergedCheckedResets(t *testing.T) { + fr := &fakeGitRunner{remoteExists: true, remoteSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + reset, err := p.verifyRemoteEnvTip("env/test", "basesha", false /*diverged*/, true /*singleFlightChecked*/) + if err != nil { + t.Fatalf("verifyRemoteEnvTip: %v", err) + } + if !reset { + t.Error("reset should be true when the remote orphan tip was healed") + } + if len(fr.resets) != 1 || fr.resets[0].sha != "basesha" { + t.Fatalf("expected one ResetBranch to basesha, got %+v", fr.resets) + } +} + +func TestVerifyRemoteEnvTip_NotCheckedFailsClosedNoReset(t *testing.T) { + fr := &fakeGitRunner{remoteExists: true, remoteSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + reset, err := p.verifyRemoteEnvTip("env/test", "basesha", false, false /*singleFlightChecked*/) + if err == nil { + t.Fatal("expected fail-closed divergence error, got nil") + } + if reset { + t.Error("reset must be false when failing closed") + } + if len(fr.resets) != 0 { + t.Fatalf("ResetBranch must NOT be called without a real single-flight check, got %d", len(fr.resets)) + } +} + +func TestVerifyRemoteEnvTip_DivergedFailsClosedNoReset(t *testing.T) { + fr := &fakeGitRunner{remoteExists: true, remoteSHA: "staletip"} + p := &Planner{remote: "origin", gitRunner: fr} + + reset, err := p.verifyRemoteEnvTip("env/test", "basesha", true /*diverged*/, true) + if err == nil { + t.Fatal("expected fail-closed divergence error, got nil") + } + if reset { + t.Error("reset must be false for a diverged env") + } + if len(fr.resets) != 0 { + t.Fatalf("ResetBranch must NOT be called for a diverged env, got %d", len(fr.resets)) + } + if !strings.Contains(err.Error(), "in-progress hotfix") { + t.Errorf("error %q should be the diverged variant", err.Error()) + } +} + +func TestVerifyRemoteEnvTip_AbsentRemoteIsClean(t *testing.T) { + fr := &fakeGitRunner{remoteExists: false} + p := &Planner{remote: "origin", gitRunner: fr} + + reset, err := p.verifyRemoteEnvTip("env/test", "basesha", false, true) + if err != nil { + t.Fatalf("verifyRemoteEnvTip: %v", err) + } + if reset || len(fr.resets) != 0 { + t.Errorf("an absent remote ref must not trigger a reset; reset=%v calls=%d", reset, len(fr.resets)) + } +} + +// --- (e) chain.go mirror: PlanChain integration ----------------------------- + +// TestPlanChain_OrphanRemoteTipSelfHealsWithRealChecker proves the production +// path: a real PRChecker reports no open hotfix PR, the remote env tip is an +// abandoned orphan (not diverged), and PlanChain self-heals it back to the +// recorded base instead of aborting. +func TestPlanChain_OrphanRemoteTipSelfHealsWithRealChecker(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + orphanTip := commitFile(t, "b.txt", "two", "abandoned hotfix tip") + 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/test points at the abandoned orphan tip, not the recorded base. + setRemoteEnvTip(t, "test", orphanTip) + + rr := &recordingResetRunner{gitRunner: execGitRunner{}} + checker := &stubPRChecker{prs: nil} + p := newPlanner(t, manifest, WithPRChecker(checker)) + p.gitRunner = rr + + res, err := p.PlanChain([]string{fix}, "test") + if err != nil { + t.Fatalf("PlanChain should self-heal the orphan, got: %v", err) + } + if len(res.Envs) != 1 { + t.Fatalf("expected one env plan, got %d", len(res.Envs)) + } + if !res.Envs[0].BranchReset { + t.Error("expected BranchReset=true on the healed env plan") + } + if len(rr.resets) != 1 || rr.resets[0].name != "env/test" || rr.resets[0].sha != base { + t.Fatalf("expected one ResetBranch(env/test -> base), got %+v", rr.resets) + } + if checker.calledWith != "env/test" { + t.Errorf("single-flight checker queried %q, want env/test", checker.calledWith) + } +} + +// TestPlanChain_OrphanRemoteTipNoopCheckerFailsClosed is the safety control for +// the chain path: without a real checker the same stale remote tip must abort +// fail-closed and never reset. +func TestPlanChain_OrphanRemoteTipNoopCheckerFailsClosed(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + orphanTip := commitFile(t, "b.txt", "two", "abandoned hotfix tip") + 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}, + }) + setRemoteEnvTip(t, "test", orphanTip) + + rr := &recordingResetRunner{gitRunner: execGitRunner{}} + p := newPlanner(t, manifest) // no PRChecker: realPRChecker stays false + p.gitRunner = rr + + _, err := p.PlanChain([]string{fix}, "test") + if err == nil { + t.Fatal("expected fail-closed divergence error without a real checker") + } + if len(rr.resets) != 0 { + t.Fatalf("ResetBranch must NOT be called without a real single-flight check, got %d", len(rr.resets)) + } + if !strings.Contains(err.Error(), "abandoned hotfix branch") { + t.Errorf("error %q should give the not-diverged guidance", err.Error()) + } +} + +// TestPlanChain_AbortsOnOpenConflictPR proves that an open cascade-hotfix-conflict +// PR - a conflict resolution PR with active human work on env/ - blocks +// the single-flight gate and prevents any branch reset, just as a cascade-hotfix +// PR does. The label distinction (cascade-hotfix vs cascade-hotfix-conflict) is +// resolved at the restPRChecker layer; above that layer both appear as an open +// OpenPR. This test drives the Plan path with a stubPRChecker that simulates the +// conflict-label case to assert the abort-no-reset behavior end-to-end. +func TestPlanChain_AbortsOnOpenConflictPR(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + orphanTip := commitFile(t, "b.txt", "two", "live conflict hotfix tip") + 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}, + }) + setRemoteEnvTip(t, "test", orphanTip) + + rr := &recordingResetRunner{gitRunner: execGitRunner{}} + // Stub returns a PR as if the restPRChecker found one labeled cascade-hotfix-conflict. + checker := &stubPRChecker{prs: []OpenPR{{Number: 55, URL: "https://example.test/pr/55"}}} + p := newPlanner(t, manifest, WithPRChecker(checker)) + p.gitRunner = rr + + _, err := p.PlanChain([]string{fix}, "test") + if err == nil { + t.Fatal("expected single-flight abort when a conflict-resolution hotfix PR is open") + } + if !strings.Contains(err.Error(), "55") { + t.Errorf("error %q should reference the open PR number", err.Error()) + } + if len(rr.resets) != 0 { + t.Fatalf("an open-conflict-PR abort must not reset any branch, got %d resets", len(rr.resets)) + } +} + +// TestPlanChain_AbortsOnOpenHotfixPR proves PlanChain now runs the single-flight +// gate the chain path previously lacked: an open resolution PR aborts the plan +// before any branch is touched. +func TestPlanChain_AbortsOnOpenHotfixPR(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + orphanTip := commitFile(t, "b.txt", "two", "live hotfix tip") + 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}, + }) + setRemoteEnvTip(t, "test", orphanTip) + + rr := &recordingResetRunner{gitRunner: execGitRunner{}} + checker := &stubPRChecker{prs: []OpenPR{{Number: 77, URL: "https://example.test/pr/77"}}} + p := newPlanner(t, manifest, WithPRChecker(checker)) + p.gitRunner = rr + + _, err := p.PlanChain([]string{fix}, "test") + if err == nil { + t.Fatal("expected single-flight abort when a hotfix PR is open") + } + if !strings.Contains(err.Error(), "77") { + t.Errorf("error %q should reference the open PR number", err.Error()) + } + if len(rr.resets) != 0 { + t.Fatalf("an open-PR abort must touch nothing, got %d resets", len(rr.resets)) + } +}