diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 2a6aa1e1..fa7e4510 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -11,7 +11,7 @@ Each field carries an emission status: |--------|---------| | **emitted** | Changes generated workflow output. | | **validated-only** | Parsed and schema-checked, but never appears in generated YAML. | -| **reserved** | Parsed, but has no generator consumption today. | +| **reserved** | Parses, but has no generator consumption today, so `cascade lint` rejects any manifest that sets it. | ## File shape and schema support @@ -944,16 +944,16 @@ These fields parse and pass schema validation but **never appear in generated YA |-------|-------|-----------------------| | `runs_on` | top-level and per-callback | GitHub Actions forbids `runs-on` on a reusable-workflow `uses:` caller job, so cascade-owned jobs are hardcoded `ubuntu-latest`. Per-environment runner overrides are blocked by the same structural limit. | | `concurrency` (per-callback) | `builds[]`, `deploys[]`, `publish` | GitHub Actions forbids a `concurrency:` block on a reusable-workflow caller job. Top-level `concurrency` is emitted; the per-callback form is not. | -| `timeout_minutes` (per-callback) | `builds[]`, `deploys[]`, `publish` | The timeout belongs inside the called workflow, not the caller job. Use top-level `job_timeout_minutes` to bound the cascade-owned jobs. | +| `timeout_minutes` (per-callback) | `builds[]`, `deploys[]`, `publish` | The timeout belongs inside the called workflow, not the caller job; it is never emitted, so `cascade lint` rejects it. Use top-level `job_timeout_minutes` to bound the cascade-owned jobs. | ## Reserved fields -Reserved fields parse but have zero generator consumption today. They reserve a stable shape so a future capability can land without a schema break. +Reserved fields parse but have zero generator consumption today. They reserve a stable shape so a future capability can land without a schema break. Because a manifest that sets one is silently inert, `cascade lint` treats reserved-field use as an error: remove the field (or, in the demo, keep it commented) until the capability is wired. | Field | Where | Note | |-------|-------|------| | `telemetry` | top-level | `enabled`, `adapter`, plus reserved `webhook` and `job_summary`. No generator consumption. | -| `rollout.type` / `rollout.canary` / `rollout.blue_green` | `deploys[]` | Reserved rollout sub-blocks (`rollout.fail_fast` and `rollout.max_parallel` are emitted; the rest are inert). | +| `rollout.type` / `rollout.canary` / `rollout.blue_green` | `deploys[]` | Reserved rollout sub-blocks (`rollout.fail_fast` and `rollout.max_parallel` are emitted and stay valid; the rest are inert and rejected). | | `release.version_overrides` | `release` | Reserved pointer to version-intent override files. | | `deploy_target` | `deploys[]` | Reserved shape for the GitOps mirror pattern. | diff --git a/docs/src/content/docs/reference/versioning.md b/docs/src/content/docs/reference/versioning.md index ab209689..b0d89931 100644 --- a/docs/src/content/docs/reference/versioning.md +++ b/docs/src/content/docs/reference/versioning.md @@ -55,7 +55,7 @@ A CLI supports the current schema version and the immediately preceding one (N-1 ## Reserved shapes -These fields parse and pass structural validation today but carry no generator, state, or runtime behavior. A manifest declaring them produces byte-identical generated workflows, so adopting the shape now is safe. Attaching behavior to any of them later is additive and does not bump `schema_version`. +These fields parse and pass structural validation today but carry no generator, state, or runtime behavior. Because a manifest that declares one is silently inert, `cascade lint` rejects reserved-field use as an error rather than accepting a no-op manifest: the shape is frozen so a future capability can land without a schema break, not so it can be set today. Attaching behavior to any of them later is additive and does not bump `schema_version`. ### Progressive rollout: canary and blue/green @@ -106,7 +106,7 @@ Only the addressing pointer is frozen in v1. The override-file format and the fo Two fields on `rollout:` are live today, not reserved. `rollout.fail_fast` and `rollout.max_parallel` (deploys and publish only) are emitted directly into the deploy job's `strategy:` block: `fail_fast` sets `strategy.fail-fast` (defaulting to `false` when unset), and `max_parallel`, when greater than zero, sets `strategy.max-parallel`. A manifest that sets either field changes the generated workflow. -Only the `type`, `canary`, and `blue_green` sub-blocks remain reserved and inert, as described above. Setting `type: canary` or populating a `canary:`/`blue_green:` sub-block parses and validates but has no effect on generated output today. +Only the `type`, `canary`, and `blue_green` sub-blocks remain reserved and inert, as described above. Setting `type: canary` or populating a `canary:`/`blue_green:` sub-block parses and structurally validates, but because it has no effect on generated output today, `cascade lint` rejects it; keep `rollout:` to `fail_fast` and `max_parallel` until the type-specific behavior is wired. ## Per-component versioning diff --git a/e2e/scenarios/23-canary-bluegreen-reserved.yaml b/e2e/scenarios/23-canary-bluegreen-reserved.yaml deleted file mode 100644 index 0116e6b7..00000000 --- a/e2e/scenarios/23-canary-bluegreen-reserved.yaml +++ /dev/null @@ -1,50 +0,0 @@ -name: "Canary and Blue/Green Reserved Shape" -description: | - Exercises the canary and blue_green rollout sub-blocks on deploy callbacks. - These blocks are reserved and shape-only today: they parse and pass structural - validation, but carry no generator behavior. The scenario declares both shapes - across two deploys, generates the workflows, then regenerates and proves the - output is byte-identical with no drift. - -config: - trunk_branch: main - environments: [dev, prod] - builds: - - name: app - workflow: build.yaml - triggers: ["src/**"] - deploys: - - name: app-canary - workflow: deploy-canary.yaml - triggers: ["src/**"] - rollout: - type: canary - canary: - percent: 10 - bake_time: 5m - promote_callback: .github/workflows/promote.yaml - rollback_callback: .github/workflows/rollback.yaml - - name: app-bg - workflow: deploy-bg.yaml - triggers: ["src/**"] - rollout: - type: blue_green - blue_green: - switch: .github/workflows/switch.yaml - -steps: - - name: "Seed a minimal source tree" - action: commit - commit: - message: "seed source" - files: - src/main.go: | - package main - - func main() {} - - - name: "Regenerate and confirm no drift" - action: verify - verify: - regenerate: true - expect_exit: 0 diff --git a/e2e/scenarios/24-gitops-deploy-target-reserved.yaml b/e2e/scenarios/24-gitops-deploy-target-reserved.yaml deleted file mode 100644 index 7f3e887d..00000000 --- a/e2e/scenarios/24-gitops-deploy-target-reserved.yaml +++ /dev/null @@ -1,45 +0,0 @@ -name: "GitOps Deploy Target Reserved Shape" -description: | - Exercises the reserved GitOps deploy_target variant (mode gitops) with the - enriched reserved fields branch and track_sha. This block is reserved and - shape-only today: it parses and passes structural validation, but carries no - generator behavior. The scenario declares the gitops shape on a deploy, - generates the workflows, then regenerates and proves the output is - byte-identical with no drift. - -config: - trunk_branch: main - environments: [dev, prod] - builds: - - name: app - workflow: build.yaml - triggers: ["src/**"] - deploys: - - name: app - workflow: deploy.yaml - triggers: ["src/**"] - deploy_target: - mode: gitops - repo: org/gitops-config - path: envs/prod/values.yaml - field: image.tag - value: "v1.2.3" - branch: release/prod - track_sha: true - -steps: - - name: "Seed a minimal source tree" - action: commit - commit: - message: "seed source" - files: - src/main.go: | - package main - - func main() {} - - - name: "Regenerate and confirm no drift" - action: verify - verify: - regenerate: true - expect_exit: 0 diff --git a/e2e/scenarios/25-telemetry-sink-reserved.yaml b/e2e/scenarios/25-telemetry-sink-reserved.yaml deleted file mode 100644 index 3b258ef0..00000000 --- a/e2e/scenarios/25-telemetry-sink-reserved.yaml +++ /dev/null @@ -1,45 +0,0 @@ -name: "Telemetry Sink Reserved Shape" -description: | - Exercises the reserved vendor-neutral telemetry seam (config.telemetry) with - the enriched reserved fields webhook (url, secret_name) and job_summary. This - block is reserved and shape-only today: it parses and passes structural - validation, but carries no generator or emit behavior. secret_name is a - reference to a GitHub Actions secret, never an inline token. The scenario - declares the telemetry shape, generates the workflows, then regenerates and - proves the output is byte-identical with no drift. - -config: - trunk_branch: main - environments: [dev, prod] - builds: - - name: app - workflow: build.yaml - triggers: ["src/**"] - deploys: - - name: app - workflow: deploy.yaml - triggers: ["src/**"] - telemetry: - enabled: true - adapter: none - webhook: - url: https://metrics.example.com/ingest - secret_name: TELEMETRY_TOKEN - job_summary: true - -steps: - - name: "Seed a minimal source tree" - action: commit - commit: - message: "seed source" - files: - src/main.go: | - package main - - func main() {} - - - name: "Regenerate and confirm no drift" - action: verify - verify: - regenerate: true - expect_exit: 0 diff --git a/e2e/scenarios/26-version-overrides-reserved.yaml b/e2e/scenarios/26-version-overrides-reserved.yaml deleted file mode 100644 index 0f231545..00000000 --- a/e2e/scenarios/26-version-overrides-reserved.yaml +++ /dev/null @@ -1,42 +0,0 @@ -name: "Version Overrides Reserved Shape" -description: | - Exercises the reserved version-intent override pointer - (config.release.version_overrides) with its reserved dir field. This block is - reserved and shape-only today: it parses and passes structural validation, but - carries no generator, state, or runtime behavior. dir is a relative pointer to - maintainer-committed override files; no override files are present in this - scenario. The scenario declares the version_overrides shape, generates the - workflows, then regenerates and proves the output is byte-identical with no - drift. - -config: - trunk_branch: main - environments: [dev, prod] - builds: - - name: app - workflow: build.yaml - triggers: ["src/**"] - deploys: - - name: app - workflow: deploy.yaml - triggers: ["src/**"] - release: - version_overrides: - dir: .cascade/version-overrides - -steps: - - name: "Seed a minimal source tree" - action: commit - commit: - message: "seed source" - files: - src/main.go: | - package main - - func main() {} - - - name: "Regenerate and confirm no drift" - action: verify - verify: - regenerate: true - expect_exit: 0 diff --git a/e2e/scenarios/59-lint-reserved-rejection.yaml b/e2e/scenarios/59-lint-reserved-rejection.yaml new file mode 100644 index 00000000..52227a5e --- /dev/null +++ b/e2e/scenarios/59-lint-reserved-rejection.yaml @@ -0,0 +1,75 @@ +name: "Lint Reserved Rejection" +description: | + Verifies that cascade lint rejects a manifest that populates a reserved + field (one that parses but is not wired to generation), and that the + merge_queue lane (which runs cascade lint --json as its validity gate) reds + when a candidate carries such a field. + + The positive step runs the merge_group lane against a valid manifest and + asserts the merge-queue-validate job succeeds. The negative step overwrites + the manifest with one declaring the reserved telemetry block and asserts the + same lane reds, proving reserved-field usage blocks a merge that a lenient + parse would have silently accepted as an inert manifest. + +config: + trunk_branch: main + environments: [dev] + builds: + - name: app + workflow: build.yaml + triggers: ["src/**"] + deploys: [] + merge_queue: + enabled: true + +steps: + - name: "Initial commit; assert the merge-queue lane runs cascade lint --json" + action: commit + commit: + message: "feat: add app" + files: + src/app.go: | + package main + func main() {} + expect: + workflow_files: + - path: ".github/workflows/cascade-merge-queue.yaml" + contains: + - "merge-queue-validate:" + - "cascade lint --json" + + - name: "POSITIVE: valid candidate passes the merge_group lane" + action: run_workflow + run_workflow: + workflow_path: ".github/workflows/cascade-merge-queue.yaml" + event: merge_group + expect: + jobs: + merge-queue-validate: success + + - name: "Overwrite the manifest with a reserved telemetry block" + action: commit + commit: + message: "test: stage a manifest that uses a reserved field" + files: + .github/manifest.yaml: | + ci: + config: + trunk_branch: main + environments: [dev] + builds: + - name: app + workflow: build.yaml + triggers: ["src/**"] + merge_queue: + enabled: true + telemetry: + enabled: true + adapter: none + + - name: "NEGATIVE: reserved-field candidate is blocked by the merge_group lane" + action: run_workflow + expect_failure: true + run_workflow: + workflow_path: ".github/workflows/cascade-merge-queue.yaml" + event: merge_group diff --git a/internal/config/parse.go b/internal/config/parse.go index 37b9da4f..5d4bf6fd 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -206,7 +206,6 @@ 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)...) @@ -267,7 +266,6 @@ 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)...) @@ -318,7 +316,6 @@ 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)...) } @@ -327,6 +324,10 @@ func Validate(cfg *TrunkConfig) []string { // mirroring the per-component unknown-field rejection one level up. errors = append(errors, validateUnknownTopLevel(cfg)...) + // Reserved-schema usage is a hard error (front-2 strictness): a field that + // parses but is not wired to generation must not be silently accepted. + errors = append(errors, validateReservedFields(cfg)...) + // Config-level structural validation for v1 reserved fields. errors = append(errors, validateConfigLevel(cfg)...) errors = append(errors, validateComponents(cfg)...) diff --git a/internal/config/reserved.go b/internal/config/reserved.go new file mode 100644 index 00000000..1154bf5b --- /dev/null +++ b/internal/config/reserved.go @@ -0,0 +1,165 @@ +package config + +import ( + "fmt" + "sort" +) + +// reservedCategory classifies why a manifest field that parses today is still +// rejected by lint. The category selects the operator-facing message, so the +// wording lives in exactly one place. +type reservedCategory int + +const ( + // reservedNotWired marks a field that is modeled and parses but is not wired + // to generation in this cascade version (category (a)). + reservedNotWired reservedCategory = iota + // reservedWrongPlace marks a field that is parsed and validated but is + // deliberately never emitted because it belongs somewhere else (category + // (b)): callback timeout_minutes belongs inside the called workflow. + reservedWrongPlace +) + +// errorFor renders the category message for a concrete field path. +func (c reservedCategory) errorFor(path string) string { + switch c { + case reservedWrongPlace: + return fmt.Sprintf("%s: timeout belongs in the called workflow, not the caller; GitHub forbids timeout-minutes on a job that calls a reusable workflow", path) + default: + return fmt.Sprintf("%s is reserved and not implemented in this cascade version; remove it from the manifest", path) + } +} + +// reservedFieldDoc records one reserved field path and its category. +type reservedFieldDoc struct { + path string + category reservedCategory +} + +// reservedFieldRegistry is the single, reviewable source of truth for the +// reserved-field taxonomy: every manifest field that parses today but is not +// wired to generation, hand-verified field-by-field against internal/generate +// (the doc comments in the schema are unreliable - some "reserved-shape" fields +// are in fact consumed). It mirrors the globalOnlyComponentFields pattern one +// level up. Presence detection is per-field (nil/zero checks) in +// validateReservedFields below; this table keeps the path -> category mapping in +// one place so a reviewer can audit the surface without reading the walk. +// +// Deliberately NOT reserved (consumed by generation, verified): rollout +// max_parallel and fail_fast (promote.go, generator.go) and config-level +// job_timeout_minutes (generator.go). target_sha is a component-state field on +// EnvState, not a manifest field, so it never reaches manifest lint. +var reservedFieldRegistry = []reservedFieldDoc{ + {"telemetry", reservedNotWired}, + {"deploys[].rollout.type", reservedNotWired}, + {"deploys[].rollout.canary", reservedNotWired}, + {"deploys[].rollout.blue_green", reservedNotWired}, + {"deploys[].deploy_target", reservedNotWired}, + {"release.version_overrides", reservedNotWired}, + {".timeout_minutes", reservedWrongPlace}, +} + +// validateReservedFields rejects any manifest that populates a reserved field. +// It walks the top-level surface and every component (components share the same +// build/deploy/release/validate/external types), so a reserved field is caught +// wherever it appears. +func validateReservedFields(cfg *TrunkConfig) []string { + if cfg == nil { + return nil + } + var errs []string + + // telemetry is a top-level-only block (global-only per component), so it is + // checked once here. + if cfg.Telemetry != nil { + errs = append(errs, reservedNotWired.errorFor("telemetry")) + } + + errs = append(errs, reservedInScope("", cfg.Builds, cfg.Deploys, cfg.Validate, cfg.Release, cfg.External)...) + + names := make([]string, 0, len(cfg.Components)) + for name := range cfg.Components { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + c := cfg.Components[name] + scope := fmt.Sprintf("components.%s.", name) + errs = append(errs, reservedInScope(scope, c.Builds, c.Deploys, c.Validate, c.Release, c.External)...) + } + + return errs +} + +// reservedInScope checks the callbacks and release block reachable from one +// scope (the top level, scope "", or a single component, scope +// "components.."). Factoring it keeps the top-level and per-component walks +// identical. +func reservedInScope(scope string, builds []BuildConfig, deploys []DeployConfig, validate *ValidateConfig, release *ReleaseConfig, external []ExternalRepoConfig) []string { + var errs []string + + for i := range builds { + errs = append(errs, reservedCallbackTimeout(fmt.Sprintf("%sbuilds[%d]", scope, i), builds[i].TimeoutMinutes)...) + } + for i := range deploys { + p := fmt.Sprintf("%sdeploys[%d]", scope, i) + errs = append(errs, reservedCallbackTimeout(p, deploys[i].TimeoutMinutes)...) + errs = append(errs, reservedRollout(p, deploys[i].Rollout)...) + errs = append(errs, reservedDeployTarget(p, deploys[i].DeployTarget)...) + } + if validate != nil { + errs = append(errs, reservedCallbackTimeout(scope+"validate", validate.TimeoutMinutes)...) + } + if release != nil && release.VersionOverrides != nil { + errs = append(errs, reservedNotWired.errorFor(scope+"release.version_overrides")) + } + for i := range external { + for j := range external[i].Deploys { + p := fmt.Sprintf("%sexternal[%d].deploys[%d]", scope, i, j) + errs = append(errs, reservedRollout(p, external[i].Deploys[j].Rollout)...) + errs = append(errs, reservedDeployTarget(p, external[i].Deploys[j].DeployTarget)...) + } + } + + return errs +} + +// reservedCallbackTimeout rejects a callback's timeout_minutes (category (b)). +// The per-callback timeout is parsed and validated but never emitted: every +// cascade callback is a reusable-workflow uses: job, and GitHub forbids +// timeout-minutes there, so the timeout belongs inside the called workflow. +func reservedCallbackTimeout(prefix string, timeoutMinutes int) []string { + if timeoutMinutes <= 0 { + return nil + } + return []string{reservedWrongPlace.errorFor(prefix + ".timeout_minutes")} +} + +// reservedRollout rejects the inert members of a rollout block (type, canary, +// blue_green). The consumed knobs max_parallel and fail_fast are intentionally +// not checked here - they are wired to the generated strategy block. +func reservedRollout(prefix string, r *RolloutConfig) []string { + if r == nil { + return nil + } + var errs []string + if r.Type != "" { + errs = append(errs, reservedNotWired.errorFor(prefix+".rollout.type")) + } + if r.Canary != nil { + errs = append(errs, reservedNotWired.errorFor(prefix+".rollout.canary")) + } + if r.BlueGreen != nil { + errs = append(errs, reservedNotWired.errorFor(prefix+".rollout.blue_green")) + } + return errs +} + +// reservedDeployTarget rejects a deploy_target block (the reserved GitOps-mirror +// deploy variant, not wired to generation). +func reservedDeployTarget(prefix string, dt *DeployTarget) []string { + if dt == nil { + return nil + } + return []string{reservedNotWired.errorFor(prefix + ".deploy_target")} +} diff --git a/internal/config/reserved_test.go b/internal/config/reserved_test.go new file mode 100644 index 00000000..1af5f52a --- /dev/null +++ b/internal/config/reserved_test.go @@ -0,0 +1,225 @@ +package config + +import "testing" + +// TestValidate_ReservedNotWired_Rejected asserts that populating a field which +// parses but is not wired to generation is a hard error with the category-(a) +// message. These fields are shape-only today; lint must reject their use rather +// than silently accept an inert manifest. +func TestValidate_ReservedNotWired_Rejected(t *testing.T) { + const notWired = "not implemented in this cascade version" + tests := []struct { + name string + manifest string + wantPath string + }{ + { + name: "telemetry", + manifest: ` +telemetry: + enabled: true + adapter: none +builds: + - name: app + workflow: b.yaml +`, + wantPath: "telemetry", + }, + { + name: "rollout.type", + manifest: ` +deploys: + - name: app + workflow: d.yaml + rollout: + type: canary +`, + wantPath: "deploys[0].rollout.type", + }, + { + name: "rollout.canary", + manifest: ` +deploys: + - name: app + workflow: d.yaml + rollout: + canary: + percent: 10 +`, + wantPath: "deploys[0].rollout.canary", + }, + { + name: "rollout.blue_green", + manifest: ` +deploys: + - name: app + workflow: d.yaml + rollout: + blue_green: + switch: .github/workflows/switch.yaml +`, + wantPath: "deploys[0].rollout.blue_green", + }, + { + name: "deploy_target", + manifest: ` +deploys: + - name: app + workflow: d.yaml + deploy_target: + mode: gitops + repo: org/gitops +`, + wantPath: "deploys[0].deploy_target", + }, + { + name: "release.version_overrides", + manifest: ` +builds: + - name: app + workflow: b.yaml +release: + version_overrides: + dir: .cascade/version-overrides +`, + wantPath: "release.version_overrides", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := Validate(parseInline(t, tt.manifest)) + if !hasErrContaining(errs, notWired) { + t.Fatalf("expected category-(a) %q message, got %v", notWired, errs) + } + if !hasErrContaining(errs, tt.wantPath) { + t.Fatalf("expected error naming %q, got %v", tt.wantPath, errs) + } + }) + } +} + +// TestValidate_ReservedNotWired_Component asserts the reserved-field walk reaches +// into components, so a reserved field on a component callback is rejected the +// same as at the top level. +func TestValidate_ReservedNotWired_Component(t *testing.T) { + cfg := parseInline(t, ` +schema_version: 1 +components: + api: + path: services/api + deploys: + - name: api + workflow: d.yaml + deploy_target: + mode: gitops + repo: org/gitops +`) + errs := Validate(cfg) + if !hasErrContaining(errs, "components.api.deploys[0].deploy_target") { + t.Fatalf("expected reserved field on component callback to be rejected, got %v", errs) + } + if !hasErrContaining(errs, "not implemented in this cascade version") { + t.Fatalf("expected category-(a) message for component reserved field, got %v", errs) + } +} + +// TestValidate_ReservedWrongPlace_Rejected asserts callback timeout_minutes is a +// hard error with the category-(b) message: the timeout belongs in the called +// workflow, not on the caller job. +func TestValidate_ReservedWrongPlace_Rejected(t *testing.T) { + const wrongPlace = "timeout belongs in the called workflow, not the caller" + 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, wrongPlace) { + t.Fatalf("expected category-(b) %q message, got %v", wrongPlace, errs) + } + }) + } +} + +// TestValidate_ConsumedRolloutFields_Clean is the guard: rollout knobs that ARE +// wired to generation (max_parallel, fail_fast) must never be swept up by the +// reserved-field rejection. Only the inert rollout members (type, canary, +// blue_green) are reserved. +func TestValidate_ConsumedRolloutFields_Clean(t *testing.T) { + cfg := parseInline(t, ` +deploys: + - name: app + workflow: d.yaml + inputs: + region: us-east-1 + rollout: + max_parallel: 2 + fail_fast: true +`) + for _, e := range Validate(cfg) { + if containsAny(e, "reserved", "not implemented in this cascade version") { + t.Fatalf("consumed rollout knobs must validate clean, got %v", e) + } + } +} + +// TestReservedFieldRegistry_MessagesMatchCategory guards the documented registry +// against drift: every entry must render its category's operator-facing message, +// so the field-path -> category -> message mapping stays internally consistent. +func TestReservedFieldRegistry_MessagesMatchCategory(t *testing.T) { + if len(reservedFieldRegistry) == 0 { + t.Fatal("reserved field registry must not be empty") + } + for _, f := range reservedFieldRegistry { + msg := f.category.errorFor(f.path) + switch f.category { + case reservedWrongPlace: + if !hasErrContaining([]string{msg}, "timeout belongs in the called workflow, not the caller") { + t.Errorf("%s: category (b) message wrong: %q", f.path, msg) + } + case reservedNotWired: + if !hasErrContaining([]string{msg}, "not implemented in this cascade version") { + t.Errorf("%s: category (a) message wrong: %q", f.path, msg) + } + default: + t.Errorf("%s: unknown reserved category %d", f.path, f.category) + } + } +} + +func containsAny(s string, subs ...string) bool { + for _, sub := range subs { + if hasErrContaining([]string{s}, sub) { + return true + } + } + return false +} diff --git a/internal/config/schema_v1_e2e_test.go b/internal/config/schema_v1_e2e_test.go index 57e57f57..4fbf0b05 100644 --- a/internal/config/schema_v1_e2e_test.go +++ b/internal/config/schema_v1_e2e_test.go @@ -7,8 +7,8 @@ import ( ) // fullSurfaceManifest exercises the full v1 schema surface (every reserved-shape -// field) through the on-disk load path the lint command uses, asserting -// it parses cleanly and validates with no errors. +// field) through the on-disk load path the lint command uses, asserting the +// reserved fields round-trip from disk and that lint rejects their use. const fullSurfaceManifest = `ci: config: schema_version: 1 @@ -153,8 +153,21 @@ func TestFullSurfaceManifestE2E(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("expected a clean full-surface manifest, got errors: %v", errs) + // The full-surface manifest deliberately populates reserved fields so the + // parse round-trip below is exercised end to end. Because those fields are + // reserved (not wired to generation), lint rejects the manifest: assert each + // reserved-usage error is reported rather than silently accepted. + errs := Validate(cfg) + for _, want := range []string{ + "telemetry is reserved and not implemented in this cascade version", + "deploys[0].rollout.type is reserved and not implemented in this cascade version", + "deploys[0].rollout.canary is reserved and not implemented in this cascade version", + "deploys[0].rollout.blue_green is reserved and not implemented in this cascade version", + "deploys[0].deploy_target is reserved and not implemented in this cascade version", + } { + if !hasErrContaining(errs, want) { + t.Fatalf("expected reserved-field rejection %q, got %v", want, errs) + } } // Spot-check that the reserved fields actually round-tripped from disk. diff --git a/internal/config/schema_v1_test.go b/internal/config/schema_v1_test.go index db5e420b..8fe2523a 100644 --- a/internal/config/schema_v1_test.go +++ b/internal/config/schema_v1_test.go @@ -474,10 +474,10 @@ validate: 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") { + if !hasErrContaining(errs, "timeout belongs in the called workflow, not the caller") { t.Fatalf("expected timeout_minutes rejection, got %v", errs) } - if !hasErrContaining(errs, "set timeout-minutes inside your callback workflow") { + if !hasErrContaining(errs, "GitHub forbids timeout-minutes on a job that calls a reusable workflow") { t.Fatalf("expected actionable timeout-minutes guidance, got %v", errs) } }) diff --git a/internal/config/validate_canary_test.go b/internal/config/validate_canary_test.go index cb51284d..995de523 100644 --- a/internal/config/validate_canary_test.go +++ b/internal/config/validate_canary_test.go @@ -10,7 +10,7 @@ import ( func TestValidateCanaryFields(t *testing.T) { t.Parallel() - t.Run("new canary fields validate at CurrentSchemaVersion", func(t *testing.T) { + t.Run("canary and rollout.type are reserved and rejected by lint", func(t *testing.T) { t.Parallel() cfg := parseInline(t, ` environments: [dev, prod] @@ -25,8 +25,12 @@ deploys: promote_callback: .github/workflows/promote.yaml rollback_callback: .github/workflows/rollback.yaml `) - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("expected no errors, got %v", errs) + errs := Validate(cfg) + if !hasErrContaining(errs, "deploys[0].rollout.type is reserved and not implemented in this cascade version") { + t.Fatalf("expected reserved rollout.type rejection, got %v", errs) + } + if !hasErrContaining(errs, "deploys[0].rollout.canary is reserved and not implemented in this cascade version") { + t.Fatalf("expected reserved rollout.canary rejection, got %v", errs) } }) diff --git a/internal/config/validate_deploytarget_test.go b/internal/config/validate_deploytarget_test.go index b496ffdd..79c5713c 100644 --- a/internal/config/validate_deploytarget_test.go +++ b/internal/config/validate_deploytarget_test.go @@ -111,8 +111,10 @@ deploys: branch: release/prod track_sha: true `) - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("expected no errors, got %v", errs) + // deploy_target parses and structurally validates, but using it is a hard + // lint error: the block is reserved and not wired to generation. + if errs := Validate(cfg); !hasErrContaining(errs, "deploys[0].deploy_target is reserved and not implemented in this cascade version") { + t.Fatalf("expected reserved deploy_target rejection, got %v", errs) } if got := cfg.GetSchemaVersion(); got != CurrentSchemaVersion { t.Fatalf("schema_version = %d, want %d (reserved shape must not bump)", got, CurrentSchemaVersion) diff --git a/internal/config/validate_telemetry_test.go b/internal/config/validate_telemetry_test.go index 3ec619fa..f7f340ae 100644 --- a/internal/config/validate_telemetry_test.go +++ b/internal/config/validate_telemetry_test.go @@ -148,8 +148,10 @@ telemetry: if cfg.Telemetry.JobSummary == nil || !*cfg.Telemetry.JobSummary { t.Fatalf("telemetry.job_summary: %#v", cfg.Telemetry.JobSummary) } - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("expected no errors, got %v", errs) + // telemetry parses and structurally validates, but using it is a hard lint + // error: the block is reserved and not wired to generation. + if errs := Validate(cfg); !hasErrContaining(errs, "telemetry is reserved and not implemented in this cascade version") { + t.Fatalf("expected reserved telemetry rejection, got %v", errs) } if got := cfg.GetSchemaVersion(); got != CurrentSchemaVersion { t.Fatalf("schema_version = %d, want %d (reserved shape must not bump)", got, CurrentSchemaVersion) diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index f4fb0b58..8b7bed2e 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -181,20 +181,6 @@ 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)} -} - // localCallbackPathRe bounds a bare-filename or .github/workflows/... local // callback value to characters safe to splice raw into a generated // workflow's uses: line. It rejects newlines, other control characters, diff --git a/internal/config/validate_versionoverrides_test.go b/internal/config/validate_versionoverrides_test.go index 6640b0a9..3604db43 100644 --- a/internal/config/validate_versionoverrides_test.go +++ b/internal/config/validate_versionoverrides_test.go @@ -96,8 +96,10 @@ release: if got := cfg.Release.VersionOverrides.Dir; got != ".cascade/version-overrides" { t.Fatalf("release.version_overrides.dir = %q", got) } - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("expected no errors, got %v", errs) + // version_overrides parses and structurally validates, but using it is a hard + // lint error: the pointer is reserved and not wired to generation. + if errs := Validate(cfg); !hasErrContaining(errs, "release.version_overrides is reserved and not implemented in this cascade version") { + t.Fatalf("expected reserved version_overrides rejection, got %v", errs) } if got := cfg.GetSchemaVersion(); got != CurrentSchemaVersion { t.Fatalf("schema_version = %d, want %d (reserved shape must not bump)", got, CurrentSchemaVersion) diff --git a/internal/generate/telemetry_reserved_test.go b/internal/generate/telemetry_reserved_test.go index b3cde684..5c257315 100644 --- a/internal/generate/telemetry_reserved_test.go +++ b/internal/generate/telemetry_reserved_test.go @@ -32,51 +32,14 @@ func writeTelemetryPlanManifest(t *testing.T, telemetry *config.TelemetryConfig) return dir } -// telemetryPlan runs Plan rooted at dir and returns the emitted set keyed by the -// path relative to dir. Plan mixes relative (workflow) and absolute (composite -// action under baseDir) paths, so paths are normalized against the resolved dir -// to make two runs in different temp dirs comparable. -func telemetryPlan(t *testing.T, dir string) map[string]string { - t.Helper() - chdir(t, dir) - - resolved, err := filepath.EvalSymlinks(dir) - require.NoError(t, err) - - planned, err := Plan(PlanOptions{ - ConfigPath: ".github/manifest.yaml", - ManifestKey: config.DefaultManifestKey, - ActionFolder: "manage-release", - OutputPath: ".github/workflows/orchestrate.yaml", - PromoteOutputPath: ".github/workflows/promote.yaml", - }) - require.NoError(t, err) - require.NotEmpty(t, planned) - - out := make(map[string]string, len(planned)) - for _, p := range planned { - abs := p.Path - if !filepath.IsAbs(abs) { - abs = filepath.Join(resolved, abs) - } - rel, rerr := filepath.Rel(resolved, abs) - require.NoError(t, rerr) - out[rel] = p.Content - } - return out -} - -// TestTelemetryReservedFieldsAreByteIdentical asserts that populating the reserved -// telemetry fields (webhook.url, webhook.secret_name, job_summary) produces a -// byte-identical generated plan set to a manifest that omits the telemetry block. -// These fields are reserved shape but not yet wired to generation/emit. The test -// drives the full Plan path (orchestrate, promote, composite action, and the rest) -// because telemetry is a config-level field that could affect any emitted file. -func TestTelemetryReservedFieldsAreByteIdentical(t *testing.T) { +// TestTelemetryReserved_PlanRejected asserts the generate path refuses a manifest +// that populates the reserved telemetry block. telemetry parses and structurally +// validates but is not wired to generation, so lint (which the Plan path runs) +// rejects it rather than emitting an inert plan. This exercises the rejection at +// the generator entrypoint, complementing the config-layer coverage. +func TestTelemetryReserved_PlanRejected(t *testing.T) { jobSummary := true - - // Case A: reserved telemetry fields populated. - withTelemetry := writeTelemetryPlanManifest(t, &config.TelemetryConfig{ + dir := writeTelemetryPlanManifest(t, &config.TelemetryConfig{ Enabled: true, Adapter: config.TelemetryAdapterNone, Webhook: &config.TelemetryWebhook{ @@ -85,28 +48,15 @@ func TestTelemetryReservedFieldsAreByteIdentical(t *testing.T) { }, JobSummary: &jobSummary, }) - plannedA := telemetryPlan(t, withTelemetry) - - // Case B: no telemetry block at all. - withoutTelemetry := writeTelemetryPlanManifest(t, nil) - plannedB := telemetryPlan(t, withoutTelemetry) - - // Guard against a vacuous comparison: the plan must be substantial for the - // equality to be meaningful. Sum every emitted file's length and require the - // total to clear a real-pipeline floor. - var totalA int - for _, content := range plannedA { - totalA += len(content) - } - require.NotEmpty(t, plannedA, "plan must emit at least one file") - require.Greater(t, totalA, 1024, "generated plan should be substantial") + chdir(t, dir) - require.Equal(t, len(plannedB), len(plannedA), - "reserved telemetry fields changed the set of emitted files") - for path, want := range plannedB { - got, ok := plannedA[path] - require.Truef(t, ok, "telemetry plan omitted %s that the empty plan emitted", path) - require.Equalf(t, want, got, - "reserved telemetry fields must not affect generated output for %s", path) - } + _, err := Plan(PlanOptions{ + ConfigPath: ".github/manifest.yaml", + ManifestKey: config.DefaultManifestKey, + ActionFolder: "manage-release", + OutputPath: ".github/workflows/orchestrate.yaml", + PromoteOutputPath: ".github/workflows/promote.yaml", + }) + require.Error(t, err, "Plan must reject a manifest that uses the reserved telemetry block") + require.Contains(t, err.Error(), "telemetry is reserved and not implemented in this cascade version") }