From 90c0ed760dca817f5c82bd73bee26ee274dc0250 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 8 Jul 2026 04:29:11 -0400 Subject: [PATCH] feat(hotfix): namespace env branches per component and scope orphan detection Hotfix environment branches for a selected component are now named env//; with no component they stay env/, byte-identical. EnvBranchName and ParseEnvBranch centralize construction and parsing: the parser splits on segment count so env/web/staging reads as component web, env staging, never env web/staging, while env/ parses exactly as before. OrphanEnvBranches and HealOrphanEnvBranches take a component and only ever inspect that component's own env//* branches against its own state, so a component never flags or deletes a sibling's branch and the default component ignores every nested branch. This lays the naming and parsing primitives; the finalize state write, tag scoping, and rejoin cleanup that consume them follow. Refs #293. Signed-off-by: Joshua Temple --- internal/hotfix/lifecycle.go | 99 ++++++++++--- internal/hotfix/lifecycle_test.go | 229 +++++++++++++++++++++++++++--- internal/hotfix/plan.go | 7 +- internal/status/consistency.go | 7 +- 4 files changed, 293 insertions(+), 49 deletions(-) diff --git a/internal/hotfix/lifecycle.go b/internal/hotfix/lifecycle.go index 6f015808..e7e3ad11 100644 --- a/internal/hotfix/lifecycle.go +++ b/internal/hotfix/lifecycle.go @@ -25,21 +25,73 @@ func resolveTagGrammar(f *config.CICDFile) taggrammar.Spec { // is deleted. const EnvBranchPrefix = "env/" -// OrphanEnvBranches returns the env/* branches in branches that have no matching -// divergence in state. A branch env/ is healthy only while state[] -// reports IsDiverged(); a branch with no diverged env behind it is an orphan -// left over from an interrupted hotfix or manual meddling and should be flagged. +// EnvBranchName returns the integration branch name for env within component. +// The default (empty) component yields env/, byte-identical to the +// historical single-component form; a named component yields +// env// so each component's integration branches occupy a +// disjoint namespace and a hotfix on one component can never touch another's. +func EnvBranchName(component, env string) string { + if component == "" { + return EnvBranchPrefix + env + } + return EnvBranchPrefix + component + "/" + env +} + +// ParseEnvBranch splits an integration branch name into its component and env, +// the inverse of EnvBranchName. It reports ok=false for any branch that does not +// carry EnvBranchPrefix or whose remainder is not a well-formed env/ or +// env//. +// +// env/ parses to an empty component and , preserving the historical +// single-component reading. env// parses to and +// . The segment count after the prefix disambiguates the two forms, so a +// nested branch never has its component folded into the env: env/web/staging +// parses to component "web" and env "staging", not the naive +// strings.TrimPrefix("env/") result of env "web/staging". A bare prefix or a +// more deeply nested name is malformed and reports ok=false. +func ParseEnvBranch(branch string) (component, env string, ok bool) { + rest, found := strings.CutPrefix(branch, EnvBranchPrefix) + if !found { + return "", "", false + } + parts := strings.Split(rest, "/") + switch len(parts) { + case 1: + if parts[0] == "" { + return "", "", false + } + return "", parts[0], true + case 2: + if parts[0] == "" || parts[1] == "" { + return "", "", false + } + return parts[0], parts[1], true + default: + return "", "", false + } +} + +// OrphanEnvBranches returns the integration branches in branches that belong to +// component and have no matching divergence in state. state is component's own +// env subtree (state.components..), and for the default empty +// component it is the historical env-keyed state map. A branch is healthy only +// while state[] reports IsDiverged(); a branch with no diverged env behind +// it is an orphan left over from an interrupted hotfix or manual meddling and is +// flagged. // -// Non env/* branches are ignored. The returned slice preserves the input order -// and is nil when nothing is orphaned, so callers can treat a nil result as +// Only branches whose parsed component equals component are considered, so a +// component never inspects, and never flags, a sibling's env// +// branch, and the default component ignores every component-nested branch. Non +// env/* branches are ignored. The returned slice preserves the input order and +// is nil when nothing is orphaned, so callers can treat a nil result as // "consistent". -func OrphanEnvBranches(branches []string, state map[string]*config.EnvState) []string { +func OrphanEnvBranches(component string, branches []string, state map[string]*config.EnvState) []string { var orphans []string for _, branch := range branches { - if !strings.HasPrefix(branch, EnvBranchPrefix) { + comp, env, ok := ParseEnvBranch(branch) + if !ok || comp != component { continue } - env := strings.TrimPrefix(branch, EnvBranchPrefix) st := state[env] if st != nil && st.IsDiverged() { continue @@ -49,22 +101,23 @@ func OrphanEnvBranches(branches []string, state map[string]*config.EnvState) []s return orphans } -// HealOrphanEnvBranches deletes every env/* branch that OrphanEnvBranches flags -// as having no matching divergence, calling del to remove each branch on remote. -// In production del is git.DeleteRemoteBranch, whose delete of an absent branch -// is a no-op success, so HealOrphanEnvBranches is idempotent: re-running it, or -// running it against an orphan that is already gone, deletes nothing further and -// never errors. +// HealOrphanEnvBranches deletes every integration branch that OrphanEnvBranches +// flags for component as having no matching divergence, calling del to remove +// each branch on remote. In production del is git.DeleteRemoteBranch, whose +// delete of an absent branch is a no-op success, so HealOrphanEnvBranches is +// idempotent: re-running it, or running it against an orphan that is already +// gone, deletes nothing further and never errors. // -// Only orphans are deleted. A branch backing a diverged environment is never -// touched, because the deletion set comes from OrphanEnvBranches, which excludes -// it by the same IsDiverged() predicate the hotfix preflight and status -// consistency classify on. The returned slice lists the branches deleted, in -// input order, and is nil when nothing was orphaned. On the first delete error -// the heal stops and returns that error with a nil healed slice. -func HealOrphanEnvBranches(branches []string, state map[string]*config.EnvState, remote string, del func(remote, branch string) error) ([]string, error) { +// Only orphans in component's own namespace are deleted. A branch backing a +// diverged environment, and every sibling component's branch, is never touched, +// because the deletion set comes from OrphanEnvBranches, which excludes them by +// the component filter and the same IsDiverged() predicate the hotfix preflight +// and status consistency classify on. The returned slice lists the branches +// deleted, in input order, and is nil when nothing was orphaned. On the first +// delete error the heal stops and returns that error with a nil healed slice. +func HealOrphanEnvBranches(component string, branches []string, state map[string]*config.EnvState, remote string, del func(remote, branch string) error) ([]string, error) { var healed []string - for _, branch := range OrphanEnvBranches(branches, state) { + for _, branch := range OrphanEnvBranches(component, branches, state) { if err := del(remote, branch); err != nil { return nil, fmt.Errorf("deleting orphan branch %s on %s: %w", branch, remote, err) } diff --git a/internal/hotfix/lifecycle_test.go b/internal/hotfix/lifecycle_test.go index fff47d2e..9a9c63b0 100644 --- a/internal/hotfix/lifecycle_test.go +++ b/internal/hotfix/lifecycle_test.go @@ -9,67 +9,225 @@ import ( "github.com/stablekernel/cascade/internal/config" ) +func TestEnvBranchName(t *testing.T) { + tests := []struct { + name string + component string + env string + want string + }{ + { + name: "no component is byte-identical to the historical form", + component: "", + env: "staging", + want: "env/staging", + }, + { + name: "named component nests the env under the component", + component: "web", + env: "staging", + want: "env/web/staging", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := EnvBranchName(tt.component, tt.env); got != tt.want { + t.Fatalf("EnvBranchName(%q, %q) = %q, want %q", tt.component, tt.env, got, tt.want) + } + }) + } +} + +func TestParseEnvBranch(t *testing.T) { + tests := []struct { + name string + branch string + wantComponent string + wantEnv string + wantOK bool + }{ + { + name: "single-component env branch parses to an empty component", + branch: "env/staging", + wantEnv: "staging", + wantOK: true, + }, + { + name: "nested env branch parses component and env unambiguously", + branch: "env/web/staging", + wantComponent: "web", + wantEnv: "staging", + wantOK: true, + }, + { + name: "a non-env branch is not an env branch", + branch: "main", + wantOK: false, + }, + { + name: "a feature branch that merely contains a slash is not an env branch", + branch: "feature/x", + wantOK: false, + }, + { + name: "the bare prefix is malformed", + branch: "env/", + wantOK: false, + }, + { + name: "an over-nested branch is malformed", + branch: "env/web/staging/extra", + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + component, env, ok := ParseEnvBranch(tt.branch) + if ok != tt.wantOK || component != tt.wantComponent || env != tt.wantEnv { + t.Fatalf("ParseEnvBranch(%q) = (%q, %q, %v), want (%q, %q, %v)", + tt.branch, component, env, ok, tt.wantComponent, tt.wantEnv, tt.wantOK) + } + }) + } +} + +func TestParseEnvBranch_NestedEnvIsNotMisreadAsComponentPath(t *testing.T) { + // The naive strings.TrimPrefix("env/") footgun would read env/web/staging as + // env "web/staging"; the parse must keep the env exactly "staging". + component, env, ok := ParseEnvBranch("env/web/staging") + if !ok { + t.Fatalf("ParseEnvBranch(env/web/staging) ok = false, want true") + } + if env == "web/staging" { + t.Fatalf("env misparsed as %q; the component must be split off", env) + } + if component != "web" || env != "staging" { + t.Fatalf("ParseEnvBranch(env/web/staging) = (%q, %q), want (web, staging)", component, env) + } +} + +func TestEnvBranchNameParseEnvBranchRoundTrip(t *testing.T) { + cases := []struct { + component string + env string + }{ + {"", "staging"}, + {"web", "staging"}, + {"api", "prod"}, + } + for _, c := range cases { + branch := EnvBranchName(c.component, c.env) + component, env, ok := ParseEnvBranch(branch) + if !ok || component != c.component || env != c.env { + t.Fatalf("round-trip of (%q, %q) via %q = (%q, %q, %v)", + c.component, c.env, branch, component, env, ok) + } + } +} + func TestOrphanEnvBranches(t *testing.T) { tests := []struct { - name string - branches []string - state map[string]*config.EnvState - want []string + name string + component string + branches []string + state map[string]*config.EnvState + want []string }{ { - name: "no branches yields no orphans", - branches: nil, + name: "no branches yields no orphans", + component: "", + branches: nil, state: map[string]*config.EnvState{ "test": {SHA: "abc"}, }, want: nil, }, { - name: "branch with matching divergence is not an orphan", - branches: []string{"env/test"}, + name: "branch with matching divergence is not an orphan", + component: "", + branches: []string{"env/test"}, state: map[string]*config.EnvState{ "test": {Ref: "env/test", BaseSHA: "base", Patches: []string{"p1"}}, }, want: nil, }, { - name: "branch without matching divergence is an orphan", - branches: []string{"env/test"}, + name: "branch without matching divergence is an orphan", + component: "", + branches: []string{"env/test"}, state: map[string]*config.EnvState{ "test": {SHA: "abc"}, // not diverged }, want: []string{"env/test"}, }, { - name: "branch for an env absent from state is an orphan", - branches: []string{"env/staging"}, + name: "branch for an env absent from state is an orphan", + component: "", + branches: []string{"env/staging"}, state: map[string]*config.EnvState{ "test": {Ref: "env/test"}, }, want: []string{"env/staging"}, }, { - name: "non env-prefixed branches are ignored", - branches: []string{"main", "feature/x", "env/test"}, + name: "non env-prefixed branches are ignored", + component: "", + branches: []string{"main", "feature/x", "env/test"}, state: map[string]*config.EnvState{ "test": {SHA: "abc"}, }, want: []string{"env/test"}, }, { - name: "mixed orphan and healthy branches", - branches: []string{"env/test", "env/uat"}, + name: "mixed orphan and healthy branches", + component: "", + branches: []string{"env/test", "env/uat"}, state: map[string]*config.EnvState{ "test": {Ref: "env/test", Patches: []string{"p1"}}, "uat": {SHA: "abc"}, }, want: []string{"env/uat"}, }, + { + name: "default component ignores a component's nested branches", + component: "", + branches: []string{"env/test", "env/web/test"}, + state: map[string]*config.EnvState{ + "test": {SHA: "abc"}, // not diverged + }, + // env/web/test belongs to the web namespace, never the default one, + // so it is never flagged here even though it has no divergence. + want: []string{"env/test"}, + }, + { + name: "component scopes orphan detection to its own namespace", + component: "web", + branches: []string{"env/staging", "env/web/staging", "env/api/staging"}, + state: map[string]*config.EnvState{ + // web's own env subtree: staging is not diverged, so env/web/staging + // is web's orphan; env/staging and env/api/staging are siblings. + "staging": {SHA: "abc"}, + }, + want: []string{"env/web/staging"}, + }, + { + name: "a component never flags a sibling's diverged branch", + component: "web", + branches: []string{"env/api/staging", "env/web/staging"}, + state: map[string]*config.EnvState{ + // web's staging is diverged, so env/web/staging is healthy; + // env/api/staging is api's concern and must never appear here. + "staging": {Ref: "env/web/staging", BaseSHA: "base", Patches: []string{"p1"}}, + }, + want: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := OrphanEnvBranches(tt.branches, tt.state) + got := OrphanEnvBranches(tt.component, tt.branches, tt.state) if !reflect.DeepEqual(got, tt.want) { t.Fatalf("OrphanEnvBranches() = %v, want %v", got, tt.want) } @@ -94,7 +252,7 @@ func TestHealOrphanEnvBranches(t *testing.T) { } healed, err := HealOrphanEnvBranches( - []string{"main", "env/test", "env/uat", "env/staging"}, state, "origin", del) + "", []string{"main", "env/test", "env/uat", "env/staging"}, state, "origin", del) if err != nil { t.Fatalf("HealOrphanEnvBranches() error = %v", err) } @@ -122,11 +280,11 @@ func TestHealOrphanEnvBranches(t *testing.T) { return nil // already gone is success } - first, err := HealOrphanEnvBranches([]string{"env/uat"}, state, "origin", del) + first, err := HealOrphanEnvBranches("", []string{"env/uat"}, state, "origin", del) if err != nil { t.Fatalf("first heal error = %v", err) } - second, err := HealOrphanEnvBranches([]string{"env/uat"}, state, "origin", del) + second, err := HealOrphanEnvBranches("", []string{"env/uat"}, state, "origin", del) if err != nil { t.Fatalf("second heal error = %v", err) } @@ -143,7 +301,7 @@ func TestHealOrphanEnvBranches(t *testing.T) { t.Fatalf("delete must not be called when nothing is orphaned") return nil } - healed, err := HealOrphanEnvBranches([]string{"main", "env/test"}, state, "origin", del) + healed, err := HealOrphanEnvBranches("", []string{"main", "env/test"}, state, "origin", del) if err != nil { t.Fatalf("error = %v", err) } @@ -152,11 +310,38 @@ func TestHealOrphanEnvBranches(t *testing.T) { } }) + t.Run("a component heal never deletes a sibling's branch", func(t *testing.T) { + // web's staging is diverged, so env/web/staging is healthy. env/api/staging + // belongs to a sibling and must never be considered by web's heal, even + // though web's state has no record for it. + webState := map[string]*config.EnvState{ + "staging": {Ref: "env/web/staging", BaseSHA: "base", Patches: []string{"p1"}}, + } + var deleted []string + del := func(remote, branch string) error { + deleted = append(deleted, branch) + return nil + } + healed, err := HealOrphanEnvBranches( + "web", []string{"env/web/staging", "env/api/staging"}, webState, "origin", del) + if err != nil { + t.Fatalf("error = %v", err) + } + if healed != nil { + t.Fatalf("healed = %v, want nil (no web orphan, sibling out of scope)", healed) + } + for _, b := range deleted { + if b == "env/api/staging" { + t.Fatalf("web heal must never delete the sibling branch env/api/staging") + } + } + }) + t.Run("propagates a delete error with the healed-so-far set", func(t *testing.T) { del := func(remote, branch string) error { return errBoom } - healed, err := HealOrphanEnvBranches([]string{"env/uat"}, state, "origin", del) + healed, err := HealOrphanEnvBranches("", []string{"env/uat"}, state, "origin", del) if err == nil { t.Fatalf("expected error, got nil") } diff --git a/internal/hotfix/plan.go b/internal/hotfix/plan.go index a9479fcf..ffdfa5df 100644 --- a/internal/hotfix/plan.go +++ b/internal/hotfix/plan.go @@ -462,9 +462,12 @@ func hotfixVersionCandidate(spec taggrammar.Spec, envVersion string) (string, er return v.WithGrammar(spec).NextHotfix().String(), nil } -// envBranch returns the integration branch name for an environment. +// envBranch returns the integration branch name for an environment. It routes +// through EnvBranchName with the default (empty) component, so the single- +// component name env/ is preserved; component threading arrives with the +// component-aware finalize path. func envBranch(env string) string { - return "env/" + env + return EnvBranchName("", env) } // protectionSuggestions returns ready-to-run gh CLI commands an operator can diff --git a/internal/status/consistency.go b/internal/status/consistency.go index 267888cc..d7fad7d0 100644 --- a/internal/status/consistency.go +++ b/internal/status/consistency.go @@ -105,11 +105,14 @@ func runConsistency(opts consistencyOptions) error { return fmt.Errorf("listing branches: %w", err) } - orphans := hotfix.OrphanEnvBranches(branches, file.State) + // The default (empty) component covers the single-component manifest, whose + // env-keyed state is exactly the default component's env subtree. Per-component + // consistency scoping arrives with the component-aware finalize path. + orphans := hotfix.OrphanEnvBranches("", branches, file.State) var healed []string if opts.fix { - healed, err = hotfix.HealOrphanEnvBranches(branches, file.State, opts.remote, opts.deleter) + healed, err = hotfix.HealOrphanEnvBranches("", branches, file.State, opts.remote, opts.deleter) if err != nil { return fmt.Errorf("healing orphan branches: %w", err) }