diff --git a/internal/config/parse.go b/internal/config/parse.go index acd1b8bc..df202a5b 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -190,6 +190,7 @@ func Validate(cfg *TrunkConfig) []string { // rejects on a jobs..uses call. matrix: is builds-only. isReusable := b.Workflow != "" errors = append(errors, validateJobControlFields(fmt.Sprintf("builds[%d]", i), isReusable, b.RunsOn, b.Concurrency)...) + errors = append(errors, validateCallbackTimeout(fmt.Sprintf("builds[%d]", i), isReusable, b.TimeoutMinutes)...) errors = append(errors, validatePermissions(fmt.Sprintf("builds[%d]", i), b.Permissions)...) errors = append(errors, validateSecrets(fmt.Sprintf("builds[%d]", i), b.Secrets)...) @@ -250,6 +251,7 @@ func Validate(cfg *TrunkConfig) []string { // rejects on a jobs..uses call. rollout: is deploys-only. isReusable := d.Workflow != "" errors = append(errors, validateJobControlFields(fmt.Sprintf("deploys[%d]", i), isReusable, d.RunsOn, d.Concurrency)...) + errors = append(errors, validateCallbackTimeout(fmt.Sprintf("deploys[%d]", i), isReusable, d.TimeoutMinutes)...) errors = append(errors, validatePermissions(fmt.Sprintf("deploys[%d]", i), d.Permissions)...) errors = append(errors, validateSecrets(fmt.Sprintf("deploys[%d]", i), d.Secrets)...) errors = append(errors, validateRollout(fmt.Sprintf("deploys[%d]", i), d.Rollout, cfg.Environments)...) @@ -300,6 +302,7 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, validateLocalCallbackWorkflowPath("validate", v.Workflow)...) isReusable := v.Workflow != "" errors = append(errors, validateJobControlFields("validate", isReusable, v.RunsOn, v.Concurrency)...) + errors = append(errors, validateCallbackTimeout("validate", isReusable, v.TimeoutMinutes)...) errors = append(errors, validatePermissions("validate", v.Permissions)...) errors = append(errors, validateSecrets("validate", v.Secrets)...) } diff --git a/internal/config/schema_v1_test.go b/internal/config/schema_v1_test.go index 9edf8f2a..7c54c2f2 100644 --- a/internal/config/schema_v1_test.go +++ b/internal/config/schema_v1_test.go @@ -434,6 +434,93 @@ builds: }) } +// TestValidate_TimeoutMinutesOnCallback_Rejected asserts that a per-callback +// timeout_minutes on a reusable-workflow callback (builds, deploys, validate) is +// rejected at validation. GitHub forbids timeout-minutes on a job that calls a +// reusable workflow, and every cascade callback is a reusable-workflow uses: job, +// so the timeout must live inside the called workflow instead. +func TestValidate_TimeoutMinutesOnCallback_Rejected(t *testing.T) { + tests := []struct { + name string + manifest string + }{ + { + name: "build callback", + manifest: ` +builds: + - name: app + workflow: b.yaml + timeout_minutes: 15 +`, + }, + { + name: "deploy callback", + manifest: ` +deploys: + - name: app + workflow: d.yaml + timeout_minutes: 15 +`, + }, + { + name: "validate callback", + manifest: ` +validate: + workflow: v.yaml + timeout_minutes: 15 +`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := Validate(parseInline(t, tt.manifest)) + if !hasErrContaining(errs, "timeout_minutes is not valid on a reusable-workflow callback") { + t.Fatalf("expected timeout_minutes rejection, got %v", errs) + } + if !hasErrContaining(errs, "set timeout-minutes inside your callback workflow") { + t.Fatalf("expected actionable timeout-minutes guidance, got %v", errs) + } + }) + } +} + +// TestValidate_CallbackWithoutTimeout_Clean asserts the control cases that must +// keep validating clean: a callback without timeout_minutes, and a manifest-level +// config.job_timeout_minutes (the cascade-owned job timeout, a different field). +func TestValidate_CallbackWithoutTimeout_Clean(t *testing.T) { + t.Run("callback without timeout_minutes clean", func(t *testing.T) { + cfg := parseInline(t, ` +builds: + - name: app + workflow: b.yaml +deploys: + - name: svc + workflow: d.yaml +`) + for _, e := range Validate(cfg) { + if strings.Contains(e, "timeout_minutes is not valid") { + t.Fatalf("callback without timeout_minutes must validate clean, got %v", e) + } + } + }) + t.Run("config.job_timeout_minutes not rejected", func(t *testing.T) { + cfg := parseInline(t, ` +job_timeout_minutes: 20 +builds: + - name: app + workflow: b.yaml +`) + if cfg.JobTimeoutMinutes != 20 { + t.Fatalf("job_timeout_minutes should parse to 20, got %d", cfg.JobTimeoutMinutes) + } + for _, e := range Validate(cfg) { + if strings.Contains(e, "timeout_minutes is not valid") { + t.Fatalf("manifest-level job_timeout_minutes must not be rejected, got %v", e) + } + } + }) +} + func TestValidateConcurrencyRejectedOnReusableWorkflow(t *testing.T) { cfg := parseInline(t, ` deploys: diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index 8ffc73ef..0bf2d622 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -125,6 +125,20 @@ func validateJobControlFields(prefix string, isReusableWorkflow bool, runsOn *Ru return errs } +// validateCallbackTimeout rejects a per-callback timeout_minutes on a +// reusable-workflow callback. GitHub forbids timeout-minutes on a job that calls +// a reusable workflow (a jobs..uses job may only set uses, with, secrets, +// needs, if, permissions, strategy, name, and concurrency). Every cascade callback +// is a reusable-workflow uses: job, so the timeout must be declared inside the +// called workflow instead. +func validateCallbackTimeout(prefix string, isReusableWorkflow bool, timeoutMinutes int) []string { + if !isReusableWorkflow || timeoutMinutes <= 0 { + return nil + } + return []string{fmt.Sprintf( + "%s: timeout_minutes is not valid on a reusable-workflow callback; GitHub forbids timeout-minutes on a job that calls a reusable workflow - set timeout-minutes inside your callback workflow instead", prefix)} +} + // validateLocalCallbackWorkflowPath checks that a local callback workflow path // is either a bare filename, a .github/workflows/... path, or a cross-repo // external ref (containing "@"). Any other form is rejected because GitHub diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 5000a38b..08f92828 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -1239,12 +1239,11 @@ func (g *Generator) writeRetryJob(sb *strings.Builder, info CallbackInfo, workfl fmt.Fprintf(sb, " name: %s - Retry %d\n", info.DisplayName, retryNum) fmt.Fprintf(sb, " needs: [setup, %s]\n", prevJobName) fmt.Fprintf(sb, " if: needs.%s.result == 'failure'\n", prevJobName) - // An explicit per-callback timeout-minutes is valid only on a steps job, not - // on a reusable-workflow caller job. A retry shim re-invokes the reusable - // workflow via uses:, so the timeout must live inside the called workflow. - if info.TimeoutMinutes > 0 { - fmt.Fprintf(sb, " timeout-minutes: %d\n", info.TimeoutMinutes) - } + // timeout-minutes is forbidden on a reusable-workflow caller job + // (jobs..uses): GitHub rejects the workflow at parse time. A retry shim + // re-invokes the reusable workflow via uses:, so no timeout is emitted here. + // Per-callback timeout_minutes is rejected at config validation + // (validateCallbackTimeout); the timeout must live inside the called workflow. // Propagate the matrix strategy to the retry job so that ${{ matrix.* }} // references in the reusable workflow's inputs remain bound. Without this, diff --git a/internal/generate/graph.go b/internal/generate/graph.go index 75317354..683f7622 100644 --- a/internal/generate/graph.go +++ b/internal/generate/graph.go @@ -29,15 +29,15 @@ type CallbackInfo struct { RunPolicy string OnFailure string Retries int - TimeoutMinutes int // Job-level timeout-minutes (omitted when 0) Matrix *config.MatrixConfig // Build fan-out; nil for deploys and validate SupportsDryRun bool // When true, dry-run promotes invoke the callback with dry_run: true instead of skipping it // Per-callback job attributes carried from config. GHA forbids - // runs-on/permissions/concurrency on a reusable-workflow uses: callback, and - // schema validation rejects runs_on/permissions/concurrency on reusable - // callbacks. Callbacks are reusable-workflow only, so these fields are - // populated from config but never emitted as job-level keys. + // runs-on/permissions/concurrency/timeout-minutes on a reusable-workflow uses: + // callback, and schema validation rejects runs_on/permissions/concurrency/ + // timeout_minutes on reusable callbacks. Callbacks are reusable-workflow only, + // so these fields are populated from config but never emitted as job-level keys. + TimeoutMinutes int // Per-callback timeout-minutes; validated-against, never emitted (belongs inside the called workflow) RunsOn *config.RunsOn // Per-callback runner selection (#12) Permissions map[string]string // Per-callback job permissions, incl. id-token: write OIDC (#35, #15) Concurrency *config.ConcurrencyConfig // Per-callback concurrency override (#17)