From 547394645daa0476c008969249d662b7644a433a Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 8 Jul 2026 07:23:52 -0400 Subject: [PATCH] feat(hotfix): thread the component through the hotfix plan and generated apply lane The generated per-component hotfix workflow calls hotfix plan --component, but the plan command did not register the flag and the apply lane materialized a flat env/ branch, so finalize looked for env// that never existed. hotfix plan now accepts --component and constructs env//; the generated apply lane materializes and protects env//, and the context job derives TARGET_ENV by stripping the component-aware prefix. Plan, apply, and finalize now agree on the same env branch. A single component keeps env/ and the flat TARGET_ENV, byte-identical. Refs #293. Signed-off-by: Joshua Temple --- internal/generate/hotfix.go | 46 +++++++-- .../generate/hotfix_component_lane_test.go | 82 ++++++++++++++++ internal/hotfix/chain.go | 6 +- internal/hotfix/command.go | 3 + internal/hotfix/command_test.go | 2 +- internal/hotfix/plan.go | 33 +++++-- internal/hotfix/plan_component_test.go | 97 +++++++++++++++++++ 7 files changed, 248 insertions(+), 21 deletions(-) create mode 100644 internal/generate/hotfix_component_lane_test.go create mode 100644 internal/hotfix/plan_component_test.go diff --git a/internal/generate/hotfix.go b/internal/generate/hotfix.go index 7556868e..ca7bd729 100644 --- a/internal/generate/hotfix.go +++ b/internal/generate/hotfix.go @@ -86,6 +86,32 @@ func (g *HotfixGenerator) writeComponentFlag(sb *strings.Builder, indent string) } } +// envBranchPrefix returns the env-integration-branch name prefix this hotfix +// workflow operates under, mirroring hotfix.EnvBranchName(component, ""): +// single-component yields "env/" (byte-identical to the historical flat form), +// a component yields "env//" so each component's integration branches +// occupy a disjoint namespace that agrees with the component-aware plan and +// finalize CLI paths. The apply lane appends the ${env} loop variable to form +// the branch, and the context job recovers TARGET_ENV by stripping this prefix +// from the merged resolution PR's base ref. +// +// The literal must stay in sync with hotfix.EnvBranchName in +// internal/hotfix/lifecycle.go, the same cross-package convention the hotfix +// label constants above follow. +func (g *HotfixGenerator) envBranchPrefix() string { + if g.componentName != "" { + return "env/" + g.componentName + "/" + } + return "env/" +} + +// envBranchRef returns the shell expression naming the env integration branch +// for the apply lane's ${env} loop variable: env/${env} single-component +// (byte-identical), env//${env} for a component. +func (g *HotfixGenerator) envBranchRef() string { + return g.envBranchPrefix() + "${env}" +} + // getStateTokenRef returns the token expression used to merge the clean-path // resolution PR. It mirrors the release and promote generators: users configure // the full expression via the state_token config option, and it defaults to the @@ -402,11 +428,11 @@ func (g *HotfixGenerator) writeApplyJob(sb *strings.Builder) { sb.WriteString(" continue-on-error: true\n") sb.WriteString(" run: |\n") sb.WriteString(" for env in $(echo \"$ENV_SEQUENCE\" | tr ',' '\\n'); do\n") - sb.WriteString(" PROT_PATH=\"repos/${{ github.repository }}/branches/env%2F${env}/protection\"\n") + fmt.Fprintf(sb, " PROT_PATH=\"repos/${{ github.repository }}/branches/%s${env}/protection\"\n", strings.ReplaceAll(g.envBranchPrefix(), "/", "%2F")) sb.WriteString(" PROT=$(gh api \"$PROT_PATH\" 2>/dev/null || echo '')\n") sb.WriteString(" CHECKS=$(echo \"$PROT\" | jq -r '.required_status_checks.contexts[]? // empty' 2>/dev/null || echo '')\n") sb.WriteString(" if [ -z \"$PROT\" ] || [ -z \"$CHECKS\" ]; then\n") - sb.WriteString(" echo \"::warning::Branch env/${env} has no required status checks; hotfix auto-merge will NOT be gated by required checks.\"\n") + fmt.Fprintf(sb, " echo \"::warning::Branch %s has no required status checks; hotfix auto-merge will NOT be gated by required checks.\"\n", g.envBranchRef()) sb.WriteString(" echo \"::warning::Configure protection: gh api \\\"$PROT_PATH\\\" -X PUT -f required_status_checks.strict=true -F required_status_checks.contexts[]=hotfix-check\"\n") sb.WriteString(" fi\n") sb.WriteString(" done\n") @@ -449,7 +475,7 @@ func (g *HotfixGenerator) writeApplyJob(sb *strings.Builder) { // A no-op env (all requested commits already present) has an empty commit // list; skip it and continue the chain to the next env. sb.WriteString(" if [ -z \"$COMMITS\" ]; then\n") - sb.WriteString(" echo \"::notice::env/${env}: all commits already present, skipping\"\n") + fmt.Fprintf(sb, " echo \"::notice::%s: all commits already present, skipping\"\n", g.envBranchRef()) sb.WriteString(" continue\n") sb.WriteString(" fi\n") sb.WriteString(" FIRST_COMMIT=$(echo \"$COMMITS\" | cut -d',' -f1)\n") @@ -458,9 +484,9 @@ func (g *HotfixGenerator) writeApplyJob(sb *strings.Builder) { // Materialize env/ at the planner's validated base if origin lacks it, // so the resolution PR has a base branch; the plan enforces tip == BASE when // the branch already exists, so this is a no-op create in that case. - sb.WriteString(" if ! git rev-parse --verify --quiet \"refs/remotes/origin/env/${env}\" >/dev/null; then\n") - sb.WriteString(" git push origin \"${BASE}:refs/heads/env/${env}\"\n") - sb.WriteString(" git fetch origin \"+refs/heads/env/${env}:refs/remotes/origin/env/${env}\"\n") + fmt.Fprintf(sb, " if ! git rev-parse --verify --quiet \"refs/remotes/origin/%s\" >/dev/null; then\n", g.envBranchRef()) + fmt.Fprintf(sb, " git push origin \"${BASE}:refs/heads/%s\"\n", g.envBranchRef()) + fmt.Fprintf(sb, " git fetch origin \"+refs/heads/%s:refs/remotes/origin/%s\"\n", g.envBranchRef(), g.envBranchRef()) sb.WriteString(" fi\n") sb.WriteString(" git switch -c \"$BRANCH\" \"$BASE\"\n") // The PR-body trailers carry the full comma-joined set of applied trunk @@ -484,7 +510,7 @@ func (g *HotfixGenerator) writeApplyJob(sb *strings.Builder) { sb.WriteString(" if $CLEAN; then\n") sb.WriteString(" git push origin \"$BRANCH\"\n") sb.WriteString(" gh pr create \\\n") - sb.WriteString(" --base \"env/${env}\" \\\n") + fmt.Fprintf(sb, " --base \"%s\" \\\n", g.envBranchRef()) sb.WriteString(" --head \"$BRANCH\" \\\n") fmt.Fprintf(sb, " --label %s \\\n", hotfixLabel) sb.WriteString(" --title \"hotfix(${env}): cherry-pick ${SHORT_SHA}\" \\\n") @@ -518,11 +544,11 @@ func (g *HotfixGenerator) writeApplyJob(sb *strings.Builder) { // just-merged tip. sb.WriteString(" git fetch origin '+refs/heads/env/*:refs/remotes/origin/env/*' --tags\n") sb.WriteString(" else\n") - sb.WriteString(" echo \"::warning::Cherry-pick conflicted on env/${env}; opening resolution PR and halting chain\"\n") + fmt.Fprintf(sb, " echo \"::warning::Cherry-pick conflicted on %s; opening resolution PR and halting chain\"\n", g.envBranchRef()) sb.WriteString(" git push origin \"$BRANCH\"\n") sb.WriteString(" CONFLICT_BODY=$(printf '%s\\n\\nConflicting files:\\n%s\\n\\nThis resolves %s.\\n\\nEnvironments still pending: %s.\\n\\nAfter merge, re-engage the hotfix workflow targeting %s.\\n\\nResolve locally:\\n git fetch && git switch %s\\n # resolve conflicts, then\\n git push --force-with-lease\\n' \"$BODY\" \"$CONFLICTS\" \"$env\" \"$REMAINING\" \"$HOTFIX_TARGET_ENV\" \"$BRANCH\")\n") sb.WriteString(" gh pr create \\\n") - sb.WriteString(" --base \"env/${env}\" \\\n") + fmt.Fprintf(sb, " --base \"%s\" \\\n", g.envBranchRef()) sb.WriteString(" --head \"$BRANCH\" \\\n") fmt.Fprintf(sb, " --label %s \\\n", hotfixConflictLabel) sb.WriteString(" --title \"hotfix(${env}): cherry-pick $(echo \"$CONFLICT_COMMIT\" | cut -c1-8) (conflicts)\" \\\n") @@ -592,7 +618,7 @@ func (g *HotfixGenerator) writeContextJob(sb *strings.Builder) { sb.WriteString(" BASE_REF: ${{ github.event.pull_request.base.ref }}\n") sb.WriteString(" PR_BODY: ${{ github.event.pull_request.body }}\n") sb.WriteString(" run: |\n") - sb.WriteString(" TARGET_ENV=\"${BASE_REF#env/}\"\n") + fmt.Fprintf(sb, " TARGET_ENV=\"${BASE_REF#%s}\"\n", g.envBranchPrefix()) // Recover the full comma-joined set of trunk fix commits and the trunk base // anchor from the trailers the apply job stamped into the resolution PR body. // The Source trailer carries every applied commit, so keeping the whole value diff --git a/internal/generate/hotfix_component_lane_test.go b/internal/generate/hotfix_component_lane_test.go new file mode 100644 index 00000000..6bff5833 --- /dev/null +++ b/internal/generate/hotfix_component_lane_test.go @@ -0,0 +1,82 @@ +package generate + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHotfixGenerator_Component_ApplyLaneUsesComponentEnvBranch proves the +// per-component apply lane creates and operates on env//: the +// branch is materialized, the resolution PRs base against it, and the +// branch-protection probe URL-encodes the nested prefix. The flat env/${env} +// form must not appear as a base ref in a component workflow. +func TestHotfixGenerator_Component_ApplyLaneUsesComponentEnvBranch(t *testing.T) { + gen := NewHotfixGenerator(threeEnvHotfixConfig(), "", WithHotfixComponentName("web")) + content, err := gen.Generate() + require.NoError(t, err) + + assert.Contains(t, content, `refs/heads/env/web/${env}`, + "apply lane must materialize the component-scoped env branch") + assert.Contains(t, content, `--base "env/web/${env}"`, + "resolution PRs must base against the component-scoped env branch") + assert.Contains(t, content, `branches/env%2Fweb%2F${env}/protection`, + "branch-protection probe must URL-encode the nested component prefix") + assert.NotContains(t, content, `--base "env/${env}"`, + "a component workflow must not base a PR on the flat env/${env} branch") +} + +// TestHotfixGenerator_Component_ContextStripsComponentPrefix proves the merged +// context job recovers TARGET_ENV by stripping the component-aware prefix +// env// from the PR base ref, so finalize resolves the same env the +// apply lane targeted. +func TestHotfixGenerator_Component_ContextStripsComponentPrefix(t *testing.T) { + gen := NewHotfixGenerator(threeEnvHotfixConfig(), "", WithHotfixComponentName("web")) + content, err := gen.Generate() + require.NoError(t, err) + + assert.Contains(t, content, `TARGET_ENV="${BASE_REF#env/web/}"`, + "context job must strip the component-aware prefix to derive TARGET_ENV") + assert.NotContains(t, content, `TARGET_ENV="${BASE_REF#env/}"`, + "a component workflow must not strip only the flat env/ prefix") +} + +// TestHotfixGenerator_Component_LaneAgreesPlanApplyFinalize is the composition +// gate: it proves the plan, apply, and finalize references in one generated +// per-component workflow all agree on env//. The plan and +// finalize CLI steps carry --component ; the apply lane bases PRs on +// env//${env}; and the context job strips env// to recover +// the target env. If any leg disagreed the merged-hotfix chain would break, so +// asserting all four in one workflow is the end-to-end agreement proof. +func TestHotfixGenerator_Component_LaneAgreesPlanApplyFinalize(t *testing.T) { + gen := NewHotfixGenerator(threeEnvHotfixConfig(), "", WithHotfixComponentName("web")) + content, err := gen.Generate() + require.NoError(t, err) + + // plan + finalize CLI steps are component-scoped. + assert.Equal(t, 2, strings.Count(content, "--component web \\"), + "both the plan and finalize CLI steps must thread --component web") + // apply lane bases on the component-scoped env branch. + assert.Contains(t, content, `--base "env/web/${env}"`) + // context/finalize recovers the same env by stripping the same prefix. + assert.Contains(t, content, `TARGET_ENV="${BASE_REF#env/web/}"`) +} + +// TestHotfixGenerator_SingleComponent_ApplyLaneFlatByteIdentical pins the +// no-component apply lane and context job to the historical flat forms, so the +// single-component output stays byte-identical to the pre-component behavior. +func TestHotfixGenerator_SingleComponent_ApplyLaneFlatByteIdentical(t *testing.T) { + gen := NewHotfixGenerator(threeEnvHotfixConfig(), "") + content, err := gen.Generate() + require.NoError(t, err) + + assert.Contains(t, content, `--base "env/${env}"`) + assert.Contains(t, content, `refs/heads/env/${env}`) + assert.Contains(t, content, `branches/env%2F${env}/protection`) + assert.Contains(t, content, `TARGET_ENV="${BASE_REF#env/}"`) + // No component-scoped branch name leaks into the single-component workflow. + assert.NotContains(t, content, "env/web/") + assert.NotContains(t, content, "--component") +} diff --git a/internal/hotfix/chain.go b/internal/hotfix/chain.go index 905da54d..49d599f2 100644 --- a/internal/hotfix/chain.go +++ b/internal/hotfix/chain.go @@ -216,7 +216,7 @@ func (p *Planner) PlanChain(refs []string, targetEnv string) (*PlanChainResult, // 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)) + singleFlightChecked, err := p.checkSingleFlight(p.envBranch(env)) if err != nil { return nil, err } @@ -227,14 +227,14 @@ func (p *Planner) PlanChain(refs []string, targetEnv string) (*PlanChainResult, // 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) + reset, err := p.verifyRemoteEnvTip(p.envBranch(env), baseSHA, diverged, singleFlightChecked) if err != nil { return nil, err } ep := EnvPlan{ Env: env, - Branch: envBranch(env), + Branch: p.envBranch(env), BaseSHA: baseSHA, BranchReset: reset, Commits: make([]string, 0, len(shas)), diff --git a/internal/hotfix/command.go b/internal/hotfix/command.go index 7516a645..2323a88f 100644 --- a/internal/hotfix/command.go +++ b/internal/hotfix/command.go @@ -143,6 +143,7 @@ func newPlanCommand() *cobra.Command { actor string remote string repo string + component string dryRun bool jsonOutput bool ghaOutput bool @@ -169,6 +170,7 @@ With --dry-run nothing is mutated (the env branch is planned but not created).`, opts := []Option{ WithDryRun(dryRun), WithRemote(remote), + WithPlanComponent(component), } if repo != "" { opts = append(opts, WithPRChecker(newRestPRChecker(repo))) @@ -230,6 +232,7 @@ With --dry-run nothing is mutated (the env branch is planned but not created).`, 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 the REST API (default: skip the check)") + cmd.Flags().StringVar(&component, "component", "", "Declared component to scope the hotfix to (default: single-component manifest)") 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") diff --git a/internal/hotfix/command_test.go b/internal/hotfix/command_test.go index 159a9c92..eed4b4fd 100644 --- a/internal/hotfix/command_test.go +++ b/internal/hotfix/command_test.go @@ -48,7 +48,7 @@ func TestNewCommand(t *testing.T) { func TestNewPlanCommand_Flags(t *testing.T) { cmd := newPlanCommand() assert.Equal(t, "plan", cmd.Name()) - for _, name := range []string{"config", "key", "commit", "commits", "target-env", "actor", "remote", "repo", "dry-run", "json", "gha-output"} { + for _, name := range []string{"config", "key", "commit", "commits", "target-env", "actor", "remote", "repo", "component", "dry-run", "json", "gha-output"} { assert.NotNil(t, cmd.Flags().Lookup(name), "plan flag %q should exist", name) } } diff --git a/internal/hotfix/plan.go b/internal/hotfix/plan.go index ffdfa5df..ad8dffb5 100644 --- a/internal/hotfix/plan.go +++ b/internal/hotfix/plan.go @@ -157,6 +157,13 @@ type Planner struct { // only when a real checker actually proved no resolution PR is open. realPRChecker bool gitRunner gitRunner + // component, when non-empty, names the declared component this hotfix is + // scoped to. It is set only via WithPlanComponent by a per-component generated + // hotfix workflow. An empty value selects the single-component path, whose + // integration branch is env/; a named component names the integration + // branch env// so each component's branches occupy a disjoint + // namespace and agree with the component-aware finalize path. + component string } // PlannerOptions carries the required inputs for NewPlanner. @@ -195,6 +202,17 @@ func WithRemote(remote string) Option { } } +// WithPlanComponent scopes the plan to a declared component. It names the +// integration branch env// so the plan agrees with the +// component-aware finalize path and each component's branches occupy a disjoint +// namespace. An empty name (the default) keeps the single-component behavior +// byte-identical, naming the branch env/. This mirrors the finalize +// package option WithComponent; the two names differ only because the plan and +// finalize option types are distinct. +func WithPlanComponent(name string) Option { + return func(p *Planner) { p.component = name } +} + // NewPlanner constructs a Planner from the manifest at opts.ConfigPath. func NewPlanner(opts PlannerOptions, options ...Option) (*Planner, error) { key := opts.ManifestKey @@ -300,7 +318,7 @@ func (p *Planner) Plan(fixRef, targetEnv string) (*PlanResult, error) { } baseSHA := state.SHA - branch := envBranch(targetEnv) + branch := p.envBranch(targetEnv) result := &PlanResult{ TargetEnv: targetEnv, FixSHA: fixSHA, @@ -462,12 +480,13 @@ 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. 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 EnvBranchName("", env) +// envBranch returns the integration branch name for env under this planner's +// component. The default (empty) component yields env/, byte-identical to +// the historical single-component name; a named component yields +// env// so the plan agrees with the component-aware finalize +// path and each component's integration branches occupy a disjoint namespace. +func (p *Planner) envBranch(env string) string { + return EnvBranchName(p.component, env) } // protectionSuggestions returns ready-to-run gh CLI commands an operator can diff --git a/internal/hotfix/plan_component_test.go b/internal/hotfix/plan_component_test.go new file mode 100644 index 00000000..b23f011d --- /dev/null +++ b/internal/hotfix/plan_component_test.go @@ -0,0 +1,97 @@ +package hotfix + +import ( + "os/exec" + "testing" +) + +// TestPlan_Component_UsesComponentScopedEnvBranch proves a planner scoped to a +// component names the integration branch env// and creates it at +// the recorded state SHA, so a per-component hotfix operates in its own branch +// namespace and agrees with the component-aware finalize path. +func TestPlan_Component_UsesComponentScopedEnvBranch(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + fix := commitFile(t, "b.txt", "two", "fix") + + manifest := writeManifest(t, []string{"dev", "test", "prod"}, map[string]string{ + "dev": fix, + "test": base, + "prod": base, + }) + + p := newPlanner(t, manifest, WithPlanComponent("web")) + res, err := p.Plan(fix, "test") + if err != nil { + t.Fatalf("Plan: %v", err) + } + if res.NoOp { + t.Fatal("expected non-noop plan") + } + if res.Branch != "env/web/test" { + t.Errorf("branch = %q, want env/web/test", res.Branch) + } + if !res.BranchCreated { + t.Error("expected BranchCreated=true when branch absent") + } + // The local branch must actually be created at the recorded base SHA under + // the component-scoped name. + got := gitOut(t, "rev-parse", "env/web/test") + if got != base { + t.Errorf("env/web/test tip = %q, want %q", got, base) + } +} + +// TestPlan_SingleComponent_UsesFlatEnvBranch pins the default (no component) +// planner to the historical flat env/ name, so the single-component path +// stays byte-identical to the pre-component behavior. +func TestPlan_SingleComponent_UsesFlatEnvBranch(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + fix := commitFile(t, "b.txt", "two", "fix") + + manifest := writeManifest(t, []string{"dev", "test", "prod"}, map[string]string{ + "dev": fix, + "test": base, + "prod": base, + }) + + p := newPlanner(t, manifest) + res, err := p.Plan(fix, "test") + if err != nil { + t.Fatalf("Plan: %v", err) + } + if res.Branch != "env/test" { + t.Errorf("branch = %q, want env/test", res.Branch) + } + // No component-scoped branch is created for the single-component path. + if err := exec.Command("git", "rev-parse", "--verify", "env/web/test").Run(); err == nil { + t.Error("single-component plan created a component-scoped env/web/test branch") + } +} + +// TestPlan_Component_SingleFlightQueriesComponentBranch proves the whole plan +// path, not just the reported branch, operates on the component-scoped branch: +// the single-flight open-PR gate is queried with env//, so a +// per-component hotfix can never be blocked or unblocked by another component's +// resolution PR. +func TestPlan_Component_SingleFlightQueriesComponentBranch(t *testing.T) { + newScratchRepo(t) + base := commitFile(t, "a.txt", "one", "first") + fix := commitFile(t, "b.txt", "two", "fix") + + manifest := writeManifest(t, []string{"dev", "test", "prod"}, map[string]string{ + "dev": fix, + "test": base, + "prod": base, + }) + + stub := &stubPRChecker{} + p := newPlanner(t, manifest, WithPlanComponent("web"), WithPRChecker(stub)) + if _, err := p.Plan(fix, "test"); err != nil { + t.Fatalf("Plan: %v", err) + } + if stub.calledWith != "env/web/test" { + t.Errorf("single-flight queried %q, want env/web/test", stub.calledWith) + } +}