diff --git a/docs/src/content/docs/guides/simulate-and-verify.md b/docs/src/content/docs/guides/simulate-and-verify.md index 7690191a..563d9648 100644 --- a/docs/src/content/docs/guides/simulate-and-verify.md +++ b/docs/src/content/docs/guides/simulate-and-verify.md @@ -94,6 +94,8 @@ Effects (in order): A `skipped` outcome is never a failure, but it does not count as a success either. When every configured deploy is skipped, nothing was deployed, so the finalize still gates. +The same gate applies to `simulate rollback`. A rollback re-deploys a prior SHA, and the real finalize refuses the state write when that deploy did not succeed, so injecting a failed (or all-skipped) outcome shows the `write state` effect gated and leaves the previewed state unchanged. With `--deployable`, only the scoped deploy's outcome is in scope, matching the live gate. + ## Multi-component (monorepo) manifests When a manifest declares `components:`, each component owns an independent state ladder recorded under `state.components..`, and there are no flat `state.` rows. Scope a simulation to one component with `--component`; the engine reads and replays that component's recorded state through the same path the component-scoped `promote`, `rollback`, and `hotfix` commands use, so the what-if matches the run cascade would actually perform for that component: diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index bcaecf07..86d8e262 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -114,7 +114,7 @@ before. :::note[Environment names are yours; roles default to position] The `environments` list is fully configurable. cascade attaches no meaning to specific labels: `dev`, `test`, `staging`, and `prod` are illustrative, not reserved. By default roles are decided by position, not by name: the last environment is the release stage, the second-to-last is the prerelease environment, and the publish boundary is the final crossing into the last environment. Set `role: release` or `role: prerelease` on an entry to declare that stage explicitly and override the positional default (see [`role`](#per-environment-settings)). The count is structural too: zero environments is release-only, one environment generates a single-environment Release workflow, and two or more enable the full promote cascade. -**Naming.** Environment, build, and deploy names become GitHub Actions job IDs and output keys, so keep them identifier-safe: letters, digits, and underscores (hyphens read as subtraction in GitHub Actions expressions). The generator-owned names `environment` and `dry_run` cannot be used as `dispatch_inputs`. +**Naming.** Environment, build, and deploy names become GitHub Actions job IDs and output keys, so keep them identifier-safe: letters, digits, and underscores (hyphens read as subtraction in GitHub Actions expressions). Hyphens in a name become underscores in the emitted output keys, so two names in the same section that differ only by `-` versus `_` would collide; validation rejects such pairs. The generator-owned names `environment` and `dry_run` cannot be used as `dispatch_inputs`. ::: ### Trigger configuration @@ -1098,7 +1098,7 @@ The implicit `release` slot tracks the most recently published (non-draft) GitHu `cascade lint` enforces the semantic rules the schema alone cannot: - `schema_version` should be `1`. Omitting it emits a warning. -- Environment, build, and deploy names must be identifier-safe (letters, digits, underscores). The generator-owned names `environment` and `dry_run` are reserved and cannot be used as `dispatch_inputs`. +- Environment, build, and deploy names must be identifier-safe (letters, digits, underscores). Within a section, two names that differ only by hyphen versus underscore are rejected: hyphens become underscores in job IDs and output keys, so those names would emit colliding outputs. The generator-owned names `environment` and `dry_run` are reserved and cannot be used as `dispatch_inputs`. - `pin_mode` must be `tag` or `sha`; `run_policy` must be `default`, `always`, or `force`; `on_failure` must be `abort` or `continue`; `retries` must be 0-3. - A repository cannot set both `external` (primary) and `notify` (satellite). - A per-callback `permissions` block is the complete permission set for that caller job and replaces the workflow default rather than merging. diff --git a/internal/config/parse.go b/internal/config/parse.go index 2c8e66ae..fe4ef291 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -187,9 +187,18 @@ func Validate(cfg *TrunkConfig) []string { // must be non-empty and unique, and any declared role must be a known value. errors = append(errors, validateEnvironments(cfg)...) - // Build name sets for each section (builds and deploys can share names) + // Raw-name uniqueness is not enough: output keys collapse hyphens to + // underscores (see OutputKey), so names distinguished only by "-" versus + // "_" would emit colliding outputs. Checked per section below. + errors = append(errors, validateOutputKeyCollisions("environments", envNames)...) + + // Build name sets for each section (builds and deploys can share names). + // Ordered slices are kept alongside for the output-key collision check so + // its error messages are deterministic. buildNames := make(map[string]bool) deployNames := make(map[string]bool) + buildNameList := make([]string, 0, len(cfg.Builds)) + deployNameList := make([]string, 0, len(cfg.Deploys)) // Known-field sets for callback strictness, derived once from the struct tags. buildFields := knownYAMLFields(reflect.TypeOf(BuildConfig{})) @@ -204,6 +213,7 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, fmt.Sprintf("duplicate build name: %s", b.Name)) } else { buildNames[b.Name] = true + buildNameList = append(buildNameList, b.Name) } // The name becomes part of the job ID (build-); enforce the job-ID // grammar so generation cannot emit invalid YAML. @@ -253,6 +263,7 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, fmt.Sprintf("duplicate deploy name: %s", d.Name)) } else { deployNames[d.Name] = true + deployNameList = append(deployNameList, d.Name) } // The name becomes part of the job ID (deploy-); enforce the job-ID // grammar so generation cannot emit invalid YAML. @@ -296,6 +307,13 @@ func Validate(cfg *TrunkConfig) []string { } } + // Reject names within a section whose output keys collide after the + // hyphen-to-underscore collapse (see OutputKey). Raw-name uniqueness above + // does not catch a "-"/"_" twin, which would silently shadow one entry's + // generated outputs. + errors = append(errors, validateOutputKeyCollisions("builds", buildNameList)...) + errors = append(errors, validateOutputKeyCollisions("deploys", deployNameList)...) + // Validate the validate callback structural rules. if cfg.Validate != nil { v := cfg.Validate diff --git a/internal/config/validate_outputkey_test.go b/internal/config/validate_outputkey_test.go new file mode 100644 index 00000000..b78bf313 --- /dev/null +++ b/internal/config/validate_outputkey_test.go @@ -0,0 +1,119 @@ +package config + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// outputKeyBaseConfig is a minimal valid multi-env manifest used to isolate the +// output-key collision validation rules. +func outputKeyBaseConfig() *TrunkConfig { + return &TrunkConfig{ + TrunkBranch: "main", + Environments: EnvNames("dev", "prod"), + } +} + +// TestValidate_OutputKeyCollisions covers the hyphen/underscore collapse rule: +// job IDs and expression references replace hyphens with underscores (see +// OutputKey), so two names in the same section that differ only by "-" versus +// "_" would emit the same output key and one would silently shadow the other. +// Such manifests must be rejected at validation time. +func TestValidate_OutputKeyCollisions(t *testing.T) { + tests := []struct { + name string + mutate func(cfg *TrunkConfig) + wantErrs bool + wantSubstr []string + }{ + { + name: "colliding build names rejected", + mutate: func(cfg *TrunkConfig) { + cfg.Builds = []BuildConfig{ + {Name: "api-x", Workflow: ".github/workflows/build1.yaml"}, + {Name: "api_x", Workflow: ".github/workflows/build2.yaml"}, + } + }, + wantErrs: true, + wantSubstr: []string{"api-x", "api_x", "output key"}, + }, + { + name: "colliding deploy names rejected", + mutate: func(cfg *TrunkConfig) { + cfg.Deploys = []DeployConfig{ + {Name: "svc-a", Workflow: ".github/workflows/deploy1.yaml"}, + {Name: "svc_a", Workflow: ".github/workflows/deploy2.yaml"}, + } + }, + wantErrs: true, + wantSubstr: []string{"svc-a", "svc_a", "output key"}, + }, + { + name: "colliding environment names rejected", + mutate: func(cfg *TrunkConfig) { + cfg.Environments = EnvNames("dev", "stage-1", "stage_1") + }, + wantErrs: true, + wantSubstr: []string{"stage-1", "stage_1", "output key"}, + }, + { + name: "distinct hyphenated names still pass", + mutate: func(cfg *TrunkConfig) { + cfg.Builds = []BuildConfig{ + {Name: "api-x", Workflow: ".github/workflows/build1.yaml"}, + {Name: "api-y", Workflow: ".github/workflows/build2.yaml"}, + } + cfg.Deploys = []DeployConfig{ + {Name: "svc_a", Workflow: ".github/workflows/deploy1.yaml"}, + {Name: "svc_b", Workflow: ".github/workflows/deploy2.yaml"}, + } + }, + wantErrs: false, + }, + { + name: "single entries unaffected", + mutate: func(cfg *TrunkConfig) { + cfg.Builds = []BuildConfig{ + {Name: "my-app", Workflow: ".github/workflows/build.yaml"}, + } + cfg.Deploys = []DeployConfig{ + {Name: "my_app", Workflow: ".github/workflows/deploy.yaml"}, + } + }, + wantErrs: false, + }, + { + name: "build and deploy may share a collapsed key across sections", + mutate: func(cfg *TrunkConfig) { + // Output keys are prefixed per section (run_build_* versus + // run_deploy_*), so a cross-section twin never collides. + cfg.Builds = []BuildConfig{ + {Name: "api-x", Workflow: ".github/workflows/build.yaml"}, + } + cfg.Deploys = []DeployConfig{ + {Name: "api_x", Workflow: ".github/workflows/deploy.yaml"}, + } + }, + wantErrs: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := outputKeyBaseConfig() + tt.mutate(cfg) + errs := Validate(cfg) + if tt.wantErrs { + assert.NotEmpty(t, errs, "colliding output keys must be rejected") + joined := strings.Join(errs, "\n") + for _, sub := range tt.wantSubstr { + assert.Contains(t, joined, sub) + } + } else { + assert.Empty(t, errs, "non-colliding names must pass validation") + } + }) + } +} diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index 1b03a337..df1b3899 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -111,6 +111,38 @@ func validateJobIDSafeName(prefix, name string) []string { "%s %q must contain only letters, digits, hyphens, and underscores", prefix, name)} } +// validateOutputKeyCollisions rejects two names in the same section whose +// derived output keys collide. Job IDs and the ${{ }} references built from +// them replace every hyphen with an underscore (see OutputKey), so two names +// that differ only by "-" versus "_" pass the raw-name uniqueness check yet +// emit the same output key, and one silently shadows the other in the +// generated workflow. Collisions are rejected rather than remapped for the +// same reason validateJobIDSafeName rejects rather than sanitizes: collapsing +// distinct names would silently merge two entries. Empty names and exact +// duplicates are skipped here; both are reported separately by the caller. +func validateOutputKeyCollisions(section string, names []string) []string { + var errs []string + seen := make(map[string]string, len(names)) + for _, name := range names { + if name == "" { + continue + } + key := OutputKey(name) + prev, ok := seen[key] + if !ok { + seen[key] = name + continue + } + if prev == name { + continue + } + errs = append(errs, fmt.Sprintf( + "%s %q and %q collide: both derive output key %q because hyphens become underscores in job IDs and outputs; rename one", + section, prev, name, key)) + } + return errs +} + // validateWorkflowRunXOR enforces that callbacks are reusable-workflow only. // Inline run: and shell: are no longer supported, so each is rejected with an // actionable error, and workflow: is required. diff --git a/internal/simulate/deploy_stub.go b/internal/simulate/deploy_stub.go index 934b7694..5dfe19b5 100644 --- a/internal/simulate/deploy_stub.go +++ b/internal/simulate/deploy_stub.go @@ -138,11 +138,22 @@ func (s *DeployStub) callbackEffect(kind, name string) Effect { // - Otherwise at least one deploy succeeded and none failed, so finalize // proceeds. func (s *DeployStub) gate() (blockedReason string) { + return s.gateScoped("") +} + +// gateScoped applies the same rules as gate but honors the deployable scope +// the real rollback finalize applies (gateOnDeployResults): when deployable is +// non-empty, only that deploy's outcome is in scope, and a deploy excluded by +// the scope neither gates nor counts as a success. +func (s *DeployStub) gateScoped(deployable string) (blockedReason string) { if s == nil || len(s.deploys) == 0 { return "" } anySucceeded := false for _, name := range s.deploys { + if deployable != "" && name != deployable { + continue // Out of scope: excluded by the deployable filter. + } switch s.outcomeFor(name) { case OutcomeFailure: return fmt.Sprintf("deploy %q simulated failure; trunk state left unchanged", name) diff --git a/internal/simulate/deploy_stub_test.go b/internal/simulate/deploy_stub_test.go index e73b0801..0e3216e0 100644 --- a/internal/simulate/deploy_stub_test.go +++ b/internal/simulate/deploy_stub_test.go @@ -85,6 +85,39 @@ func TestDeployStub_OneSucceedsOneSkipped_Proceeds(t *testing.T) { assert.Empty(t, stub.gate(), "one succeeding deploy is enough to advance finalize") } +// TestDeployStub_ScopedGate mirrors the real rollback gate's --deployable +// scoping (gateOnDeployResults): with a deployable set, only that deploy's +// outcome is in scope, so an out-of-scope failure neither gates nor counts. +func TestDeployStub_ScopedGate(t *testing.T) { + t.Parallel() + + stub := newDeployStub(nil, []string{"svc", "web"}, map[string]DeployOutcome{ + "web": OutcomeFailure, + }) + + assert.Empty(t, stub.gateScoped("svc"), + "an out-of-scope failure must not gate a scoped rollback") + + reason := stub.gateScoped("web") + require.NotEmpty(t, reason, "the scoped deploy failed, so the write must gate") + assert.Contains(t, reason, "web") +} + +// TestDeployStub_ScopedGate_SkippedInScope mirrors the all-skipped rule under +// scoping: the in-scope deploy was skipped, so nothing was deployed and the +// write gates even though an out-of-scope deploy succeeded. +func TestDeployStub_ScopedGate_SkippedInScope(t *testing.T) { + t.Parallel() + + stub := newDeployStub(nil, []string{"svc", "web"}, map[string]DeployOutcome{ + "svc": OutcomeSkipped, + }) + + reason := stub.gateScoped("svc") + require.NotEmpty(t, reason) + assert.Contains(t, reason, "no simulated deploy succeeded") +} + func TestDeployStub_DoesNotAliasInputs(t *testing.T) { t.Parallel() diff --git a/internal/simulate/rollback_action.go b/internal/simulate/rollback_action.go index 6c0d0317..3dd6c13d 100644 --- a/internal/simulate/rollback_action.go +++ b/internal/simulate/rollback_action.go @@ -59,6 +59,19 @@ func (a *RollbackAction) Apply(ctx ActionContext) (*ActionOutcome, error) { return nil, fmt.Errorf("plan rollback: %w", err) } + // Mirror the real rollback finalize, which runs the deploy-result gate + // between Plan and Apply (gateOnDeployResults in internal/rollback): a + // deploy that did not succeed refuses the state write. The simulated gate + // reads the injected outcomes instead of DEPLOY_RESULT_* env vars, honors + // the same deployable scoping, and holds back the revert, so the preview + // never asserts a state write the real operation would refuse. + if gated := ctx.Deploys.gateScoped(a.deployable); gated != "" { + return &ActionOutcome{ + Effects: gatedEffectsFromRollback(plan, gated), + AfterStatePath: ctx.ClonePath, + }, nil + } + if err := rb.Apply(plan); err != nil { return nil, fmt.Errorf("apply rollback: %w", err) } @@ -78,10 +91,7 @@ func effectsFromRollback(plan *rollback.Plan) []Effect { return nil } - target := plan.Environment - if plan.Deployable != "" { - target = fmt.Sprintf("%s/%s", plan.Environment, plan.Deployable) - } + target := rollbackTarget(plan) if plan.NoOp { return []Effect{{ @@ -109,6 +119,56 @@ func effectsFromRollback(plan *rollback.Plan) []Effect { } } +// gatedEffectsFromRollback renders the effect sequence for a rollback the +// deploy-result gate held back. Mirroring the promote preview, the resolved +// revert is still shown as run (the deploy callbacks did execute; one of them +// is what failed), but the write-state effect is gated with the blocking +// reason, because the real finalize aborts before Apply and leaves trunk state +// unchanged. A gated no-op plan collapses to a single gate effect: there is no +// revert to show and no write to hold back, but the real operation still +// refuses, so a clean skip would overstate it. +func gatedEffectsFromRollback(plan *rollback.Plan, reason string) []Effect { + if plan == nil { + return nil + } + + target := rollbackTarget(plan) + + if plan.NoOp { + return []Effect{{ + Disposition: DispositionGate, + Action: "rollback", + Target: target, + Detail: reason, + }} + } + + return []Effect{ + { + Disposition: DispositionRun, + Action: "revert", + Target: target, + Detail: fmt.Sprintf("to sha %s, version %s (from %s)", + shortOrNone(plan.Target.SHA), orNone(plan.Target.Version), plan.Target.Source), + }, + { + Disposition: DispositionGate, + Action: "write state", + Target: target, + Detail: reason, + }, + } +} + +// rollbackTarget renders the effect target for a plan: the environment alone, +// or env/deployable when the rollback is scoped to a single deploy. +func rollbackTarget(plan *rollback.Plan) string { + if plan.Deployable != "" { + return fmt.Sprintf("%s/%s", plan.Environment, plan.Deployable) + } + return plan.Environment +} + // emptyHistory is a record-only rollback.HistoryReader that reports no git // history. It pins target resolution to the in-state deploy-history ring so the // simulation is deterministic and never reads a git repository. diff --git a/internal/simulate/rollback_action_test.go b/internal/simulate/rollback_action_test.go index 55d118bf..a30334b6 100644 --- a/internal/simulate/rollback_action_test.go +++ b/internal/simulate/rollback_action_test.go @@ -137,6 +137,110 @@ func TestRollbackAction_LeavesOriginalUntouched(t *testing.T) { assert.Equal(t, before, after, "the original manifest bytes must be unchanged") } +// seedRollbackManifestWithDeploys writes the seeded prod manifest with the +// given deploy callbacks configured, so the deploy-result gate is in play. +func seedRollbackManifestWithDeploys(t *testing.T, deployNames ...string) string { + t.Helper() + + deploys := make([]config.DeployConfig, 0, len(deployNames)) + for _, name := range deployNames { + deploys = append(deploys, config.DeployConfig{ + Name: name, + Workflow: ".github/workflows/deploy.yaml", + }) + } + + return writeManifest(t, &config.CICDFile{ + Config: &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "uat", "prod"), + Deploys: deploys, + }, + State: map[string]*config.EnvState{ + "prod": { + SHA: "newsha0000000", + Version: "v2.0.0", + CommittedAt: "2026-02-01T10:00:00Z", + CommittedBy: "seed-user", + Previous: []config.EnvStateSnapshot{ + {SHA: "oldsha0000000", Version: "v1.0.0", CommittedAt: "2026-01-01T10:00:00Z", CommittedBy: "seed-user"}, + }, + }, + }, + }) +} + +// TestRollbackAction_GatesOnFailedDeployResult proves the simulated rollback +// applies the same deploy-result gate the real rollback finalize enforces +// (gateOnDeployResults): a deploy that did not succeed must hold back the +// state write, leaving the environment unchanged, rather than modeling a +// clean revert the real system would refuse. +func TestRollbackAction_GatesOnFailedDeployResult(t *testing.T) { + t.Parallel() + + path := seedRollbackManifestWithDeploys(t, "svc") + + engine, err := NewEngine(path, + WithActor("tester"), + WithDeployResults(map[string]DeployOutcome{"svc": OutcomeFailure})) + require.NoError(t, err) + + result, err := engine.Simulate(NewRollbackAction("prod", "", "")) + require.NoError(t, err) + + assert.False(t, result.Diff.Changed(), + "a gated rollback must not revert state; the real finalize aborts before Apply") + + require.NotEmpty(t, result.Effects) + last := result.Effects[len(result.Effects)-1] + assert.Equal(t, DispositionGate, last.Disposition, + "the write-state effect must be gated when a deploy failed") + assert.Equal(t, "write state", last.Action) + assert.Contains(t, last.Detail, "svc") +} + +// TestRollbackAction_GatesWhenNoDeploySucceeded mirrors the real gate's +// all-skipped rule: deploys are configured but none succeeded, so nothing was +// actually deployed and the state write is held back. +func TestRollbackAction_GatesWhenNoDeploySucceeded(t *testing.T) { + t.Parallel() + + path := seedRollbackManifestWithDeploys(t, "svc") + + engine, err := NewEngine(path, + WithActor("tester"), + WithDeployResults(map[string]DeployOutcome{"svc": OutcomeSkipped})) + require.NoError(t, err) + + result, err := engine.Simulate(NewRollbackAction("prod", "", "")) + require.NoError(t, err) + + assert.False(t, result.Diff.Changed()) + require.NotEmpty(t, result.Effects) + last := result.Effects[len(result.Effects)-1] + assert.Equal(t, DispositionGate, last.Disposition) +} + +// TestRollbackAction_ProceedsWhenDeploysSucceed pins the default: with all +// deploys succeeding (the injected default), the rollback reverts as before. +func TestRollbackAction_ProceedsWhenDeploysSucceed(t *testing.T) { + t.Parallel() + + path := seedRollbackManifestWithDeploys(t, "svc") + + engine, err := NewEngine(path, WithActor("tester")) + require.NoError(t, err) + + result, err := engine.Simulate(NewRollbackAction("prod", "", "")) + require.NoError(t, err) + + assert.True(t, result.Diff.Changed()) + require.NotEmpty(t, result.Effects) + last := result.Effects[len(result.Effects)-1] + assert.Equal(t, DispositionRun, last.Disposition) + assert.Equal(t, "write state", last.Action) +} + func TestRollbackAction_UnknownEnvErrors(t *testing.T) { t.Parallel()