From 479fe62e7cc2c9f5ca1337744a7c1123a94fa528 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 15 Jul 2026 19:48:15 -0400 Subject: [PATCH] fix(config): validate run_policy, on_failure, and retries on the validate block Signed-off-by: Joshua Temple --- internal/config/parse.go | 36 ++++------------ internal/config/parse_test.go | 75 ++++++++++++++++++++++++++++++++++ internal/config/validate_v1.go | 17 ++++++++ 3 files changed, 100 insertions(+), 28 deletions(-) diff --git a/internal/config/parse.go b/internal/config/parse.go index e7184ee0..2c8e66ae 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -219,20 +219,8 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, validatePermissions(fmt.Sprintf("builds[%d]", i), b.Permissions)...) errors = append(errors, validateSecrets(fmt.Sprintf("builds[%d]", i), b.Secrets)...) - // Validate run_policy - if b.RunPolicy != "" && b.RunPolicy != RunPolicyDefault && b.RunPolicy != RunPolicyAlways && b.RunPolicy != RunPolicyForce { - errors = append(errors, fmt.Sprintf("builds[%d].run_policy must be one of: default, always, force", i)) - } - - // Validate on_failure - if b.OnFailure != "" && b.OnFailure != OnFailureAbort && b.OnFailure != OnFailureContinue { - errors = append(errors, fmt.Sprintf("builds[%d].on_failure must be one of: abort, continue", i)) - } - - // Validate retries - if b.Retries < 0 || b.Retries > 3 { - errors = append(errors, fmt.Sprintf("builds[%d].retries must be between 0 and 3", i)) - } + // Validate run_policy, on_failure, and retries + errors = append(errors, validateCallbackPolicy(fmt.Sprintf("builds[%d]", i), b.RunPolicy, b.OnFailure, b.Retries)...) // Validate depends_on references using ResolveDependency for _, dep := range b.DependsOn { @@ -282,20 +270,8 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, validateRollout(fmt.Sprintf("deploys[%d]", i), d.Rollout, envNames)...) errors = append(errors, validateDeployTarget(fmt.Sprintf("deploys[%d]", i), d.DeployTarget)...) - // Validate run_policy - if d.RunPolicy != "" && d.RunPolicy != RunPolicyDefault && d.RunPolicy != RunPolicyAlways && d.RunPolicy != RunPolicyForce { - errors = append(errors, fmt.Sprintf("deploys[%d].run_policy must be one of: default, always, force", i)) - } - - // Validate on_failure - if d.OnFailure != "" && d.OnFailure != OnFailureAbort && d.OnFailure != OnFailureContinue { - errors = append(errors, fmt.Sprintf("deploys[%d].on_failure must be one of: abort, continue", i)) - } - - // Validate retries - if d.Retries < 0 || d.Retries > 3 { - errors = append(errors, fmt.Sprintf("deploys[%d].retries must be between 0 and 3", i)) - } + // Validate run_policy, on_failure, and retries + errors = append(errors, validateCallbackPolicy(fmt.Sprintf("deploys[%d]", i), d.RunPolicy, d.OnFailure, d.Retries)...) // Validate depends_on references using ResolveDependency // Deploys can depend on builds (preferred) or other deploys @@ -329,6 +305,10 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, validateJobControlFields("validate", isReusable, v.RunsOn, v.Concurrency)...) errors = append(errors, validatePermissions("validate", v.Permissions)...) errors = append(errors, validateSecrets("validate", v.Secrets)...) + // run_policy/on_failure/retries obey the same rules as builds and + // deploys. Retries is a pointer here (inheritance deep-merge contract); + // RetryCount folds nil to 0, which is inside the bound. + errors = append(errors, validateCallbackPolicy("validate", v.RunPolicy, v.OnFailure, v.RetryCount())...) } // Unknown/misspelled top-level keys are hard errors (front-1 strictness), diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index 683ed8f6..e9c0d4bd 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -672,6 +672,81 @@ func TestValidate_NewFields(t *testing.T) { } } +func TestValidate_ValidateBlockPolicyParity(t *testing.T) { + tests := []struct { + name string + validate *ValidateConfig + wantErrs []string + }{ + { + name: "valid run_policy, on_failure, and retries", + validate: &ValidateConfig{ + Workflow: "w.yaml", + RunPolicy: "always", + OnFailure: "continue", + Retries: intPtr(3), + }, + wantErrs: nil, + }, + { + name: "unset retries is valid", + validate: &ValidateConfig{Workflow: "w.yaml"}, + wantErrs: nil, + }, + { + name: "invalid run_policy", + validate: &ValidateConfig{Workflow: "w.yaml", RunPolicy: "invalid"}, + wantErrs: []string{"validate.run_policy must be one of: default, always, force"}, + }, + { + name: "invalid on_failure", + validate: &ValidateConfig{Workflow: "w.yaml", OnFailure: "invalid"}, + wantErrs: []string{"validate.on_failure must be one of: abort, continue"}, + }, + { + name: "retries above bound", + validate: &ValidateConfig{Workflow: "w.yaml", Retries: intPtr(5)}, + wantErrs: []string{"validate.retries must be between 0 and 3"}, + }, + { + name: "retries below bound", + validate: &ValidateConfig{Workflow: "w.yaml", Retries: intPtr(-1)}, + wantErrs: []string{"validate.retries must be between 0 and 3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := TrunkConfig{ + Environments: EnvNames("dev"), + Builds: []BuildConfig{ + {Name: "a", Workflow: "w.yaml"}, + }, + Validate: tt.validate, + } + errs := Validate(&cfg) + if tt.wantErrs == nil { + if len(errs) != 0 { + t.Errorf("Validate() returned errors, want none: %v", errs) + } + return + } + for _, want := range tt.wantErrs { + found := false + for _, err := range errs { + if err == want { + found = true + break + } + } + if !found { + t.Errorf("Validate() missing expected error: %q, got: %v", want, errs) + } + } + }) + } +} + func TestHasExternalRelease(t *testing.T) { tests := []struct { name string diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index ff2fa74a..1b03a337 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -128,6 +128,23 @@ func validateWorkflowRunXOR(prefix, workflow, run, shell string) []string { return errs } +// validateCallbackPolicy enforces the run_policy/on_failure enums and the +// retries bound shared by every callback block (builds, deploys, validate). +// Empty strings mean "unset" and are accepted; retries must stay in 0..3. +func validateCallbackPolicy(prefix, runPolicy, onFailure string, retries int) []string { + var errs []string + if runPolicy != "" && runPolicy != RunPolicyDefault && runPolicy != RunPolicyAlways && runPolicy != RunPolicyForce { + errs = append(errs, fmt.Sprintf("%s.run_policy must be one of: default, always, force", prefix)) + } + if onFailure != "" && onFailure != OnFailureAbort && onFailure != OnFailureContinue { + errs = append(errs, fmt.Sprintf("%s.on_failure must be one of: abort, continue", prefix)) + } + if retries < 0 || retries > 3 { + errs = append(errs, fmt.Sprintf("%s.retries must be between 0 and 3", prefix)) + } + return errs +} + // validateExternalDeployWorkflowOnly enforces that external deploys are // reusable-workflow only. An external deploy resolves to a workflow in the // external repo, so it must declare workflow: and cannot use run:/shell:.