From ad4c60b0220d603d3d5b4b70bc64cd54ca1b6dbe Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Fri, 17 Jul 2026 05:34:22 -0400 Subject: [PATCH 1/2] fix(config): require trunk_branch so a manifest cannot silently generate a dead pipeline Omitting trunk_branch passed lint with no diagnostics and generated an orchestrate workflow whose push trigger read "branches: []", an allow-list matching no branch. GitHub accepts that workflow and reports it green while it never fires on a trunk push. The schema has always listed trunk_branch as required, so lint and the schema disagreed and the docs claimed it was both required and defaulted to main. 27 of the 31 generatable manifest examples in the documentation emitted a dead workflow. Enforce it in lint to match the schema, guard the generator against emitting any empty trigger allow-list, fix every documented example, and validate the docs examples against the schema with the test that already guards the README. No default is inferred: guessing main for a repository whose trunk is named otherwise would rebuild the same dead workflow silently. Signed-off-by: Joshua Temple --- CHANGELOG.md | 28 +++++++++ docs/src/content/docs/guides/action-pins.md | 3 + docs/src/content/docs/guides/companions.md | 4 ++ docs/src/content/docs/guides/components.md | 1 + docs/src/content/docs/reference/callbacks.md | 3 + docs/src/content/docs/reference/cli.md | 1 + docs/src/content/docs/reference/manifest.md | 24 ++++++- docs/src/content/docs/security.md | 1 + internal/config/artifact_upload_test.go | 1 + internal/config/parse.go | 14 +++++ internal/config/parse_test.go | 21 +++++-- internal/config/silent_dead_siblings_test.go | 60 ++++++++++++++++++ internal/config/trunk_branch_required_test.go | 55 ++++++++++++++++ internal/config/validate_artifacts_test.go | 1 + internal/config/validate_environment_test.go | 1 + internal/config/validate_shapes.go | 12 +++- .../config/validate_versionoverrides_test.go | 1 + internal/generate/generator.go | 62 ++++++++++++++++++- internal/generate/trunk_branch_guard_test.go | 48 ++++++++++++++ internal/hotfix/finalize.go | 9 ++- internal/hotfix/finalize_test.go | 3 +- internal/schema/schema_test.go | 53 ++++++++++++++++ 22 files changed, 395 insertions(+), 11 deletions(-) create mode 100644 internal/config/silent_dead_siblings_test.go create mode 100644 internal/config/trunk_branch_required_test.go create mode 100644 internal/generate/trunk_branch_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6683b115..fc77f1e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,34 @@ A `Migration` section is added to any release that bumps `schema_version`. per page, so the lookup scanned only the newest 30 of the repository's tags and resolved an empty version once the repository grew past 30 tags. +- **config:** `trunk_branch` is now enforced as required, matching the manifest + schema, which has always listed it as required. Omitting it passed `lint` with + no diagnostics and then generated an orchestrate workflow whose push trigger + read `branches: []`, an allow-list matching no branch. The workflow was + accepted by GitHub and reported green while never firing on a trunk push, so a + pipeline could be adopted, committed, and appear healthy without ever running. + Manifests that omit it now fail `lint` and `generate-workflow` with a message + naming the field. No manifest that works today is affected: every manifest that + sets `trunk_branch` generates byte-identical output, and one that omits it + could not run at all. There is no default: inferring `main` for a repository + whose trunk is named otherwise would rebuild the same dead workflow silently. + +- **config:** `publish.workflow` and `external[].deploys` are now enforced as + required, matching the schema. Both were accepted when absent and both then + vanished from generated output, so a manifest could declare a publish callback + that never published, or an external repository that coordinated nothing. + +- **generate:** Generation now fails rather than emitting any empty trigger + allow-list (`branches: []`, `paths: []`, `tags: []`). An empty allow-list + matches nothing and silently disables the workflow it guards, so it is now a + build error instead of a workflow that reports green forever and never runs. + +- **docs:** Every manifest example across the documentation now sets + `trunk_branch`, and the reference table no longer lists it as both required and + defaulted. Copying a documented example produced a pipeline that never ran. The + documentation examples are now validated against the published schema by the + same test that has always guarded the README examples. + - **generate:** A manifest-level `concurrency.group` is now namespaced per workflow instead of being emitted bare onto every cascade workflow. A GitHub concurrency group is matched repository-wide rather than per workflow, and a diff --git a/docs/src/content/docs/guides/action-pins.md b/docs/src/content/docs/guides/action-pins.md index 7bd422ac..0c790504 100644 --- a/docs/src/content/docs/guides/action-pins.md +++ b/docs/src/content/docs/guides/action-pins.md @@ -17,6 +17,7 @@ Generated workflows are build output, so their action pins are build output too. # .github/manifest.yaml ci: config: + trunk_branch: main pin_mode: sha ``` @@ -34,6 +35,7 @@ Under `sha`, the pin resolves to a 40-character commit SHA and the human-readabl ```yaml ci: config: + trunk_branch: main pin_mode: sha action_pins: actions/checkout: "0123456789abcdef0123456789abcdef01234567" @@ -63,6 +65,7 @@ Rather than run `reconcile` by hand, enable the reconcile companion so an extern ```yaml ci: config: + trunk_branch: main reconcile: enabled: true source: dependabot diff --git a/docs/src/content/docs/guides/companions.md b/docs/src/content/docs/guides/companions.md index 02698797..0a7ca719 100644 --- a/docs/src/content/docs/guides/companions.md +++ b/docs/src/content/docs/guides/companions.md @@ -13,6 +13,7 @@ The generated workflows are build output. If someone hand-edits `.github/workflo # .github/manifest.yaml ci: config: + trunk_branch: main drift_check: enabled: true comment: true @@ -27,6 +28,7 @@ The preview companion renders what the pipeline would do for a pull request with ```yaml ci: config: + trunk_branch: main pr_preview: enabled: true comment: true @@ -41,6 +43,7 @@ The validate check runs manifest validation as its own pull-request check, so a ```yaml ci: config: + trunk_branch: main validate_check: enabled: true ``` @@ -54,6 +57,7 @@ When the repository uses GitHub's merge queue, the merge-queue companion adds a ```yaml ci: config: + trunk_branch: main merge_queue: enabled: true ``` diff --git a/docs/src/content/docs/guides/components.md b/docs/src/content/docs/guides/components.md index 6d41678a..f0d0df72 100644 --- a/docs/src/content/docs/guides/components.md +++ b/docs/src/content/docs/guides/components.md @@ -127,6 +127,7 @@ the shared code it depends on: ```yaml ci: config: + trunk_branch: main environments: [dev, prod] shared_paths: - libs/common/** # every component depends on this diff --git a/docs/src/content/docs/reference/callbacks.md b/docs/src/content/docs/reference/callbacks.md index c16d425b..fafc2b81 100644 --- a/docs/src/content/docs/reference/callbacks.md +++ b/docs/src/content/docs/reference/callbacks.md @@ -409,6 +409,8 @@ Pass custom inputs via `inputs` and `env_inputs` in the manifest: ```yaml ci: config: + trunk_branch: main + environments: [prod] builds: - name: app workflow: .github/workflows/build-app.yaml @@ -441,6 +443,7 @@ Outputs from one callback are passed to dependents: ```yaml ci: config: + trunk_branch: main builds: - name: app workflow: .github/workflows/build-app.yaml diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 670e979c..49255432 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -939,6 +939,7 @@ block the output keeps the historical `rc.N` shape shown above. A manifest with ```yaml ci: config: + trunk_branch: main tag_grammar: prerelease_token: pre prerelease_separator: "" diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 50bca6b3..46933bff 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -101,7 +101,7 @@ The two fields that define the pipeline shape. | Field | Status | Type | Required | Default | Description | |-------|--------|------|----------|---------|-------------| -| `trunk_branch` | emitted | string | Yes | `main` | The trunk branch releases flow from. | +| `trunk_branch` | emitted | string | Yes | - | The trunk branch the orchestrate workflow runs on. | | `environments` | emitted | list of strings or objects | No | - | The promotion ladder. Each entry is a bare name or an object carrying that environment's name, optional role, and inline settings. Omit for a no-environment library or CLI project. | ```yaml @@ -153,6 +153,7 @@ produces cascade's historical grammar exactly: `vX.Y.Z` releases, `-rc.N` pre-re ```yaml ci: config: + trunk_branch: main tag_grammar: prefix: v prerelease_token: rc @@ -242,6 +243,7 @@ The `sha` values come from a single committed pin table (`internal/generate/acti ```yaml ci: config: + trunk_branch: main pin_mode: sha action_pins: actions/checkout: 0123456789abcdef0123456789abcdef01234567 @@ -265,6 +267,7 @@ Reference secrets by bare name; cascade wraps a bare name in a `${{ secrets.* }} ```yaml ci: config: + trunk_branch: main release_token: RELEASE_PAT state_token: STATE_PAT ``` @@ -278,6 +281,7 @@ A GitHub App avoids storing a long-lived PAT. Point `release_token_app` and `sta ```yaml ci: config: + trunk_branch: main release_token_app: app_id: CASCADE_APP_ID private_key: CASCADE_APP_PRIVATE_KEY @@ -295,6 +299,7 @@ Optional git identity and signing configuration for state commits. ```yaml ci: config: + trunk_branch: main git: mode: custom user_name: deploy-bot @@ -322,6 +327,7 @@ Optional pre-build validation callback. ```yaml ci: config: + trunk_branch: main validate: workflow: .github/workflows/validate.yaml supports_dry_run: false @@ -354,6 +360,8 @@ Builds produce artifacts (container images, binaries, and the like). `builds` is ```yaml ci: config: + trunk_branch: main + environments: [prod] builds: - name: app workflow: .github/workflows/build-app.yaml @@ -457,6 +465,7 @@ Deploys target environments. `deploys` is a list and shares most fields with `bu ```yaml ci: config: + trunk_branch: main deploys: - name: infra workflow: .github/workflows/deploy-infra.yaml @@ -533,6 +542,7 @@ The publish callback runs once per build when a release is published, at the poi ```yaml ci: config: + trunk_branch: main publish: workflow: .github/workflows/publish.yaml ``` @@ -554,6 +564,7 @@ ci: ```yaml ci: config: + trunk_branch: main external: - repo: org/cdk-infra ref: main @@ -585,6 +596,7 @@ For satellite repos that report deployments back to a primary. ```yaml ci: config: + trunk_branch: main notify: repo: org/my-backend workflow: external-update.yaml @@ -608,6 +620,7 @@ The primary validates the dispatched `deploy_name` and `environment` against its ```yaml ci: config: + trunk_branch: main release_build: disabled: false workflow: .github/workflows/release-assets.yaml @@ -655,6 +668,7 @@ leave the gate on. ```yaml ci: config: + trunk_branch: main allow_breaking_changes: true ``` @@ -670,6 +684,7 @@ native GitHub Environment support and deployment URLs. ```yaml ci: config: + trunk_branch: main environments: - name: production role: release @@ -704,6 +719,7 @@ Top-level concurrency block emitted onto the orchestrate, promote, hotfix, rollb ```yaml ci: config: + trunk_branch: main concurrency: group: cascade-${{ github.ref }} cancel_in_progress: false @@ -771,6 +787,7 @@ change orchestrate's cancellation behavior. ```yaml ci: config: + trunk_branch: main job_timeout_minutes: 30 ``` @@ -785,6 +802,7 @@ Non-push trigger types wired onto the generated workflows. ```yaml ci: config: + trunk_branch: main extra_triggers: schedule: - cron: "0 7 * * *" @@ -808,6 +826,7 @@ Opts the rollback workflow into a `repository_dispatch` trigger, driving the rol ```yaml ci: config: + trunk_branch: main rollback: repository_dispatch: types: [rollback-request] @@ -828,6 +847,7 @@ Each of these emits an additional workflow only when its block is present. Omit ```yaml ci: config: + trunk_branch: main pr_preview: enabled: true comment: true @@ -899,6 +919,7 @@ components](/cascade/guides/components/) for the operator walkthrough. ```yaml ci: config: + trunk_branch: main environments: [dev, staging, prod] components: api: @@ -1005,6 +1026,7 @@ major of exactly the components that declare it, and leaves the rest untouched. ```yaml ci: config: + trunk_branch: main environments: [dev, prod] shared_paths: - libs/common/** # every component depends on this diff --git a/docs/src/content/docs/security.md b/docs/src/content/docs/security.md index cb5fd172..3ab4ef39 100644 --- a/docs/src/content/docs/security.md +++ b/docs/src/content/docs/security.md @@ -70,6 +70,7 @@ Third-party action pinning is shipped today, not a roadmap item. Two mechanisms ```yaml ci: config: + trunk_branch: main pin_mode: sha action_pins: actions/checkout: a1b2c3d4e5f6... diff --git a/internal/config/artifact_upload_test.go b/internal/config/artifact_upload_test.go index 841cfdcf..65388db5 100644 --- a/internal/config/artifact_upload_test.go +++ b/internal/config/artifact_upload_test.go @@ -15,6 +15,7 @@ func TestArtifactUpload_ParsesOnBuildAndDeploy(t *testing.T) { y := `ci: config: schema_version: 1 + trunk_branch: main environments: [dev] builds: - name: app diff --git a/internal/config/parse.go b/internal/config/parse.go index 63d3ce43..f57ce59d 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -410,12 +410,26 @@ func Validate(cfg *TrunkConfig) []string { } } + // The publish callback is a reusable-workflow reference, and the promote + // generator emits the publish step only when the path is non-empty. An + // empty one is accepted by YAML and then silently drops the step, so a + // manifest would declare publish and never publish. Required, per schema. + if cfg.Publish != nil && cfg.Publish.Workflow == "" { + errors = append(errors, "publish.workflow is required when a publish block is present") + } + // Validate external repos (for primary repos) externalDeployNames := make(map[string]bool) for i, ext := range cfg.External { if ext.Repo == "" { errors = append(errors, fmt.Sprintf("external[%d].repo is required", i)) } + // An external entry exists to coordinate deploys in another repo. With + // none it emits no jobs and coordinates nothing, so the entry is inert + // rather than wrong, and nothing surfaces that. Required, per schema. + if len(ext.Deploys) == 0 { + errors = append(errors, fmt.Sprintf("external[%d].deploys is required and must not be empty", i)) + } for j, d := range ext.Deploys { if d.Name == "" { diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index e9c0d4bd..1019a1a3 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -232,6 +232,7 @@ func TestValidate(t *testing.T) { { name: "missing build name", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Workflow: ".github/workflows/build.yaml"}, @@ -242,6 +243,7 @@ func TestValidate(t *testing.T) { { name: "missing build workflow", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "app"}, @@ -252,6 +254,7 @@ func TestValidate(t *testing.T) { { name: "duplicate build names", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "app", Workflow: ".github/workflows/build1.yaml"}, @@ -263,6 +266,7 @@ func TestValidate(t *testing.T) { { name: "missing deploy name", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Deploys: []DeployConfig{ {Workflow: ".github/workflows/deploy.yaml"}, @@ -273,6 +277,7 @@ func TestValidate(t *testing.T) { { name: "missing deploy workflow", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Deploys: []DeployConfig{ {Name: "cdk"}, @@ -283,6 +288,7 @@ func TestValidate(t *testing.T) { { name: "duplicate deploy names", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Deploys: []DeployConfig{ {Name: "cdk", Workflow: ".github/workflows/deploy1.yaml"}, @@ -294,6 +300,7 @@ func TestValidate(t *testing.T) { { name: "valid env_inputs matching environments", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev", "test", "prod"), Deploys: []DeployConfig{ { @@ -311,6 +318,7 @@ func TestValidate(t *testing.T) { { name: "invalid env_inputs key not in environments", config: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev", "prod"), Deploys: []DeployConfig{ { @@ -566,6 +574,7 @@ func TestValidate_NewFields(t *testing.T) { { name: "valid run_policy values", cfg: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "a", Workflow: "w.yaml", RunPolicy: "default"}, @@ -718,6 +727,7 @@ func TestValidate_ValidateBlockPolicyParity(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cfg := TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "a", Workflow: "w.yaml"}, @@ -842,8 +852,9 @@ func TestValidate_ReleaseTag(t *testing.T) { { name: "valid release_build.tag reference", cfg: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), - ReleaseBuild: &ReleaseBuildConfig{Tag: "goreleaser.tag"}, + ReleaseBuild: &ReleaseBuildConfig{Tag: "goreleaser.tag"}, Builds: []BuildConfig{ {Name: "goreleaser", Workflow: "w.yaml"}, }, @@ -854,7 +865,7 @@ func TestValidate_ReleaseTag(t *testing.T) { name: "invalid release_build.tag - unknown callback", cfg: TrunkConfig{ Environments: EnvNames("dev"), - ReleaseBuild: &ReleaseBuildConfig{Tag: "nonexistent.tag"}, + ReleaseBuild: &ReleaseBuildConfig{Tag: "nonexistent.tag"}, Builds: []BuildConfig{ {Name: "app", Workflow: "w.yaml"}, }, @@ -865,7 +876,7 @@ func TestValidate_ReleaseTag(t *testing.T) { name: "invalid release_build.tag - bad format", cfg: TrunkConfig{ Environments: EnvNames("dev"), - ReleaseBuild: &ReleaseBuildConfig{Tag: "invalid"}, + ReleaseBuild: &ReleaseBuildConfig{Tag: "invalid"}, Builds: []BuildConfig{ {Name: "app", Workflow: "w.yaml"}, }, @@ -875,8 +886,9 @@ func TestValidate_ReleaseTag(t *testing.T) { { name: "release_build.tag with deploy callback", cfg: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), - ReleaseBuild: &ReleaseBuildConfig{Tag: "release.version"}, + ReleaseBuild: &ReleaseBuildConfig{Tag: "release.version"}, Deploys: []DeployConfig{ {Name: "release", Workflow: "w.yaml"}, }, @@ -886,6 +898,7 @@ func TestValidate_ReleaseTag(t *testing.T) { { name: "deploy depends_on references valid build", cfg: TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "app", Workflow: "w.yaml", Triggers: []string{"src/**"}}, diff --git a/internal/config/silent_dead_siblings_test.go b/internal/config/silent_dead_siblings_test.go new file mode 100644 index 00000000..5f7f693c --- /dev/null +++ b/internal/config/silent_dead_siblings_test.go @@ -0,0 +1,60 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidate_PublishWorkflowMissing_IsRejected covers a sibling of the +// trunk_branch gap. The published schema requires publish.workflow, but lint +// accepted its absence and the promote generator skips the publish step +// entirely when it is empty. A manifest could declare publish, validate, and +// silently never publish anything. +func TestValidate_PublishWorkflowMissing_IsRejected(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: EnvNames("prod"), + Publish: &PublishConfig{}, + } + + errs := Validate(cfg) + + require.NotEmpty(t, errs, "a publish block without a workflow must not validate") + assert.True(t, hasErrContaining(errs, "publish.workflow"), + "validation must name publish.workflow, got: %v", errs) +} + +// TestValidate_PublishWorkflowPresent_IsAccepted keeps the check off manifests +// that already work. +func TestValidate_PublishWorkflowPresent_IsAccepted(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: EnvNames("prod"), + Publish: &PublishConfig{Workflow: ".github/workflows/publish.yaml"}, + } + + assert.False(t, hasErrContaining(Validate(cfg), "publish.workflow"), + "a publish block that sets workflow must not trip the required check") +} + +// TestValidate_ExternalWithoutDeploys_IsRejected covers the other sibling: the +// schema requires external[].deploys, but lint accepted an external entry with +// none, which generates no jobs and coordinates nothing. +func TestValidate_ExternalWithoutDeploys_IsRejected(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: EnvNames("prod"), + External: []ExternalRepoConfig{{Repo: "acme/other"}}, + } + + errs := Validate(cfg) + + require.NotEmpty(t, errs, "an external repo with no deploys must not validate") + assert.True(t, hasErrContaining(errs, "external[0].deploys"), + "validation must name external[0].deploys, got: %v", errs) +} diff --git a/internal/config/trunk_branch_required_test.go b/internal/config/trunk_branch_required_test.go new file mode 100644 index 00000000..bbdc8ba6 --- /dev/null +++ b/internal/config/trunk_branch_required_test.go @@ -0,0 +1,55 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidate_TrunkBranchMissing_IsRejected pins lint to the published schema, +// which has always listed trunk_branch as required. Lint accepted its absence +// and generation then emitted an empty push allow-list, so a manifest could be +// valid, generate cleanly, and never run. +func TestValidate_TrunkBranchMissing_IsRejected(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + Environments: EnvNames("staging"), + } + + errs := Validate(cfg) + + require.NotEmpty(t, errs, "a manifest without trunk_branch must not validate") + assert.True(t, hasErrContaining(errs, "trunk_branch"), + "validation must name trunk_branch, got: %v", errs) +} + +// TestValidate_TrunkBranchPresent_IsAccepted keeps the required check from +// firing on the manifests that already work. +func TestValidate_TrunkBranchPresent_IsAccepted(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: EnvNames("staging"), + } + + assert.False(t, hasErrContaining(Validate(cfg), "trunk_branch"), + "a manifest that sets trunk_branch must not trip the required check") +} + +// TestValidate_TrunkBranchInherited_ByComponents guards the component path: +// components inherit trunk_branch from the shared defaults, so the required +// check must not fire once per component for a value set at the top level. +func TestValidate_TrunkBranchInherited_ByComponents(t *testing.T) { + cfg := &TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: EnvNames("staging"), + Components: map[string]ComponentConfig{ + "api": {Path: "api/", TagGrammar: &TagGrammarConfig{Prefix: strptr("api-v")}}, + }, + } + + assert.False(t, hasErrContaining(Validate(cfg), "trunk_branch"), + "an inherited trunk_branch must satisfy the check for every component") +} diff --git a/internal/config/validate_artifacts_test.go b/internal/config/validate_artifacts_test.go index 1a676901..33ee19da 100644 --- a/internal/config/validate_artifacts_test.go +++ b/internal/config/validate_artifacts_test.go @@ -14,6 +14,7 @@ import ( func TestValidate_ReleaseArtifacts(t *testing.T) { base := func(artifacts []ArtifactConfig) TrunkConfig { return TrunkConfig{ + TrunkBranch: "main", Environments: EnvNames("dev"), Builds: []BuildConfig{ {Name: "app", Workflow: "w.yaml", Artifacts: artifacts}, diff --git a/internal/config/validate_environment_test.go b/internal/config/validate_environment_test.go index bb5e9f53..3d61600c 100644 --- a/internal/config/validate_environment_test.go +++ b/internal/config/validate_environment_test.go @@ -161,6 +161,7 @@ func TestParseEnvironmentConfigReservedFields(t *testing.T) { t.Parallel() cfg := parseInline(t, ` +trunk_branch: main environments: - dev - name: prod diff --git a/internal/config/validate_shapes.go b/internal/config/validate_shapes.go index c725e96b..ba4004fc 100644 --- a/internal/config/validate_shapes.go +++ b/internal/config/validate_shapes.go @@ -364,7 +364,17 @@ func validateNotifyShapes(n *NotifyConfig) []string { func validateEmittedScalars(cfg *TrunkConfig) []string { var errs []string - errs = append(errs, validateGitRefName("trunk_branch", cfg.TrunkBranch)...) + // trunk_branch is required, matching the published schema. It is the sole + // source of the orchestrate push allow-list, so an unset value used to emit + // "branches: []" and produce a workflow that could never fire on a trunk + // push. There is no safe default to infer: guessing "main" for a repo whose + // trunk is "master" rebuilds the same dead workflow silently. Ask instead. + if cfg.TrunkBranch == "" { + errs = append(errs, "trunk_branch is required (for example \"main\"): "+ + "it sets the branch the orchestrate workflow runs on") + } else { + errs = append(errs, validateGitRefName("trunk_branch", cfg.TrunkBranch)...) + } if cfg.ManifestFile != "" { errs = append(errs, validateShellDoubleQuoted("manifest_file", cfg.ManifestFile)...) diff --git a/internal/config/validate_versionoverrides_test.go b/internal/config/validate_versionoverrides_test.go index 94174fb3..a9949fb0 100644 --- a/internal/config/validate_versionoverrides_test.go +++ b/internal/config/validate_versionoverrides_test.go @@ -116,6 +116,7 @@ func TestParseVersionOverridesAbsentStaysValid(t *testing.T) { t.Parallel() cfg := parseInline(t, ` +trunk_branch: main environments: [dev, prod] deploys: - name: app diff --git a/internal/generate/generator.go b/internal/generate/generator.go index bd01e627..4930aac2 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -346,6 +346,17 @@ func (g *Generator) Generate() (string, error) { return "", err } + // Guard the push allow-list before emission. trunk_branch is the sole + // source of the branch list, and an unset value renders "branches: []": an + // allow-list matching nothing, so orchestrate never fires on a trunk push. + // CLI-side config validation rejects this too, but Generate is exported and + // must not emit a workflow that is dead on arrival when called without it. + if !g.config.OrchestrateDispatchOnly() && g.config.TrunkBranch == "" { + return "", fmt.Errorf( + "trunk_branch is required to generate the orchestrate push trigger: " + + "an unset value emits an empty branch allow-list and the workflow would never run") + } + // Guard the external-release tag reference before emission. CLI-side // config validation checks the same format, but Generate is exported and // must not panic when called without it. @@ -369,7 +380,56 @@ func (g *Generator) Generate() (string, error) { return "", err } - return sb.String(), nil + out := sb.String() + if err := assertNoDeadAllowList(out); err != nil { + return "", err + } + + return out, nil +} + +// deadAllowLists are emitted fragments that each render a trigger filter +// matching nothing. A workflow carrying one is accepted by GitHub and reports +// green forever while never running, which makes the failure invisible. +var deadAllowLists = []string{ + "branches: []", + "branches:\n", + "paths: []", + "tags: []", +} + +// assertNoDeadAllowList is the last gate before emitted YAML leaves the +// generator. Individual fields are validated upstream, but this catches the +// class rather than the instance: any present-but-empty trigger allow-list, +// from any field or future code path, silently disables the workflow it +// guards. Failing here turns that silence into a build error. +func assertNoDeadAllowList(out string) error { + for _, frag := range deadAllowLists { + if !strings.Contains(out, frag) { + continue + } + // "branches:\n" is only dead when no list item follows it. + if frag == "branches:\n" && !hasEmptyBlockList(out, frag) { + continue + } + return fmt.Errorf( + "generated workflow contains an empty trigger allow-list (%q): "+ + "it would match nothing and the workflow would never run", + strings.TrimSuffix(frag, "\n")) + } + return nil +} + +// hasEmptyBlockList reports whether a block-style key is followed by something +// other than a "-" list item, meaning the key was emitted with no entries. +func hasEmptyBlockList(out, key string) bool { + idx := strings.Index(out, key) + if idx == -1 { + return false + } + rest := out[idx+len(key):] + next := strings.TrimLeft(rest, " ") + return !strings.HasPrefix(next, "- ") } // Validate checks for potential issues and returns warnings diff --git a/internal/generate/trunk_branch_guard_test.go b/internal/generate/trunk_branch_guard_test.go new file mode 100644 index 00000000..ca757359 --- /dev/null +++ b/internal/generate/trunk_branch_guard_test.go @@ -0,0 +1,48 @@ +package generate + +import ( + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestGenerate_MissingTrunkBranch_IsLoudNotEmptyAllowList pins the generator +// against the failure this guard exists to prevent: an unset trunk_branch used +// to render "branches: []", an allow-list matching no branch at all. The +// workflow was emitted, committed, and reported green while orchestrate could +// never fire on a trunk push. Generation must fail loudly instead. +func TestGenerate_MissingTrunkBranch_IsLoudNotEmptyAllowList(t *testing.T) { + dir := t.TempDir() + cfg := &config.TrunkConfig{ + SchemaVersion: 1, + Environments: config.EnvNames("staging"), + } + + _, err := NewGenerator(cfg, dir).Generate() + + require.Error(t, err, "generation must not succeed with an unset trunk_branch") + assert.Contains(t, err.Error(), "trunk_branch", + "the error must name the field the operator has to set") +} + +// TestGenerate_EmptyPushBranchList_NeverEmitted is the class guard. Any code +// path that renders the orchestrate push trigger with an empty branch list +// produces a workflow that silently never runs, so the emitted text must never +// contain the empty allow-list regardless of which field was left unset. +func TestGenerate_EmptyPushBranchList_NeverEmitted(t *testing.T) { + dir := t.TempDir() + cfg := &config.TrunkConfig{ + SchemaVersion: 1, + TrunkBranch: "main", + Environments: config.EnvNames("staging"), + } + + result, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + + assert.NotContains(t, result, "branches: []", + "an empty push branch allow-list matches no branch and makes the workflow dead") + assert.Contains(t, result, "branches: [main]") +} diff --git a/internal/hotfix/finalize.go b/internal/hotfix/finalize.go index 701430b9..133a9849 100644 --- a/internal/hotfix/finalize.go +++ b/internal/hotfix/finalize.go @@ -429,11 +429,14 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS return fmt.Errorf("%q is not a configured environment", targetEnv) } - // Resolve trunk the same way the state write does: the configured trunk - // branch, defaulting to "main". + // Resolve trunk from the manifest. trunk_branch is required, so a validated + // manifest always carries it, but this path parses without validating and + // the value decides which branch state is written to. Guessing "main" here + // would silently push state to the wrong branch on a repo whose trunk is + // named anything else, so an absent value is an error rather than a default. trunk := cfg.TrunkBranch if trunk == "" { - trunk = "main" + return fmt.Errorf("manifest has no trunk_branch: cannot resolve the branch to read hotfix state from") } // Read the manifest as it exists on trunk, not from the checked-out env diff --git a/internal/hotfix/finalize_test.go b/internal/hotfix/finalize_test.go index 7f1ad787..d894c04c 100644 --- a/internal/hotfix/finalize_test.go +++ b/internal/hotfix/finalize_test.go @@ -86,6 +86,7 @@ func writeFinalizeManifest(t *testing.T, envs []string, states map[string]envFix var b strings.Builder b.WriteString("ci:\n") b.WriteString(" config:\n") + b.WriteString(" trunk_branch: main\n") b.WriteString(" environments:\n") for _, e := range envs { b.WriteString(" - " + e + "\n") @@ -297,7 +298,7 @@ func TestFinalize_PreviousRingBounded(t *testing.T) { // genuine transition: the new snapshot prepends and the oldest is dropped, so // the ring stays bounded at MaxPreviousSnapshots. var b strings.Builder - b.WriteString("ci:\n config:\n environments:\n") + b.WriteString("ci:\n config:\n trunk_branch: main\n environments:\n") for _, e := range []string{"dev", "test", "prod"} { b.WriteString(" - " + e + "\n") } diff --git a/internal/schema/schema_test.go b/internal/schema/schema_test.go index 8e8e5fe4..52744014 100644 --- a/internal/schema/schema_test.go +++ b/internal/schema/schema_test.go @@ -13,6 +13,7 @@ package schema_test import ( "encoding/json" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -163,6 +164,58 @@ func TestSchema_ValidatesREADMEExamples(t *testing.T) { } } +// TestSchema_ValidatesDocsExamples holds the documentation site to the same bar +// the README has always been held to. Every ci-rooted example a reader can copy +// must validate against the published schema. +// +// The docs tree had no such check, and it drifted: examples omitted the required +// trunk_branch, which the schema rejects but lint used to accept, and generation +// then emitted an empty push allow-list. Following the docs produced a pipeline +// that reported green and never ran. This test is what makes the schema, lint, +// and docs agree by construction rather than by review. +func TestSchema_ValidatesDocsExamples(t *testing.T) { + sch := compileSchema(t) + root := repoRoot(t) + docsDir := filepath.Join(root, "docs", "src", "content", "docs") + + ciBlocks := 0 + err := filepath.WalkDir(docsDir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || (!strings.HasSuffix(path, ".md") && !strings.HasSuffix(path, ".mdx")) { + return nil + } + data, readErr := os.ReadFile(path) // #nosec G304 -- test-only walk of the in-repo docs tree + if readErr != nil { + return readErr + } + rel, relErr := filepath.Rel(root, path) + if relErr != nil { + rel = path + } + for i, block := range extractYAMLFences(string(data)) { + if !firstMeaningfulLineIsCI(block) { + continue + } + ciBlocks++ + t.Run(fmt.Sprintf("%s-block-%d", rel, i), func(t *testing.T) { + doc := loadYAMLDoc(t, []byte(block)) + if vErr := sch.Validate(toJSONValue(t, doc)); vErr != nil { + t.Fatalf("docs example in %s must validate against the published schema: %v", rel, vErr) + } + }) + } + return nil + }) + if err != nil { + t.Fatalf("walk docs tree: %v", err) + } + if ciBlocks < 2 { + t.Fatalf("expected at least 2 ci-rooted yaml blocks under %s, found %d", docsDir, ciBlocks) + } +} + // firstMeaningfulLineIsCI reports whether the first non-blank, non-comment line // of a YAML block is the top-level "ci:" key. func firstMeaningfulLineIsCI(block string) bool { From d749c1a942d641d5e49c213d2d0e71ec858f21d8 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Fri, 17 Jul 2026 05:53:58 -0400 Subject: [PATCH 2/2] fix(generate): make the empty-trigger-filter guard cover the class it claims The guard's comment claimed it caught any present-but-empty trigger allow-list from any field or code path, but it scanned four literal fragments with a first-occurrence-only match. Block-style paths:, tags:, and paths-ignore: were never caught, and "paths: []" and "tags: []" matched nothing that is emitted, since those sites emit block style. A block-style key whose first occurrence is populated also hid a later empty one, because the scan stopped at the match. No reachable instance exists today. The guard has one call site, on the orchestrate workflow, and every block-style emission site it can see is length-guarded, so nothing shipped dead in either implementation. The widening is justified by the class, not by an instance: a future emission site, or a future call site, that forgets a length guard should fail the build rather than ship a workflow that never runs. The defect being fixed is the claim itself. A guard that overstates its reach is worse than a narrow one, because the next reader assumes the class is covered and stops checking. Replace the fragment list with a line scan over every occurrence, every filter key, and both YAML list styles, so the comment is true. Reword the changelog to what the check does, and scope it to the orchestrate workflow. Signed-off-by: Joshua Temple --- CHANGELOG.md | 11 ++- internal/generate/dead_allow_list_test.go | 90 +++++++++++++++++++++++ internal/generate/generator.go | 84 +++++++++++++-------- 3 files changed, 151 insertions(+), 34 deletions(-) create mode 100644 internal/generate/dead_allow_list_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index fc77f1e0..13bc60a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,10 +52,13 @@ A `Migration` section is added to any release that bumps `schema_version`. vanished from generated output, so a manifest could declare a publish callback that never published, or an external repository that coordinated nothing. -- **generate:** Generation now fails rather than emitting any empty trigger - allow-list (`branches: []`, `paths: []`, `tags: []`). An empty allow-list - matches nothing and silently disables the workflow it guards, so it is now a - build error instead of a workflow that reports green forever and never runs. +- **generate:** Generation of the orchestrate workflow now fails rather than + emitting an empty trigger filter. A filter that is present but empty matches + nothing and silently disables the trigger it guards, and GitHub accepts the + workflow and reports it green forever. The check covers every `branches`, + `paths`, and `tags` filter (and their `-ignore` forms) in the emitted + workflow, in both YAML list styles, so an emission site that forgets a length + guard fails the build instead of shipping a workflow that never runs. - **docs:** Every manifest example across the documentation now sets `trunk_branch`, and the reference table no longer lists it as both required and diff --git a/internal/generate/dead_allow_list_test.go b/internal/generate/dead_allow_list_test.go new file mode 100644 index 00000000..92d5b83a --- /dev/null +++ b/internal/generate/dead_allow_list_test.go @@ -0,0 +1,90 @@ +package generate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAssertNoDeadAllowList exercises the guard directly. The cases below are +// unreachable through the CLI today, because every emission site that renders a +// block-style allow-list is length-guarded, so a config-driven test could not +// reach them. The guard's whole purpose is to hold when a future emission site +// is not guarded, which makes the function itself the unit under test. +func TestAssertNoDeadAllowList(t *testing.T) { + tests := []struct { + name string + out string + wantErr string + }{ + { + name: "flow-style empty branches", + out: "on:\n push:\n branches: []\n", + wantErr: "branches", + }, + { + name: "block-style empty branches", + out: "on:\n push:\n branches:\n workflow_dispatch:\n", + wantErr: "branches", + }, + { + // Both keys are block style and the first one has items. A scan that + // seeks only the first "branches:" line match lands on the populated + // one, judges it live, and never examines the dead one below it. + name: "empty branches after a populated one", + out: "on:\n push:\n branches:\n - main\n" + + " pull_request:\n branches:\n", + wantErr: "branches", + }, + { + name: "block-style empty paths", + out: "on:\n push:\n branches: [main]\n paths:\n", + wantErr: "paths", + }, + { + name: "block-style empty tags", + out: "on:\n push:\n tags:\n", + wantErr: "tags", + }, + { + name: "block-style empty paths-ignore", + out: "on:\n push:\n branches: [main]\n paths-ignore:\n", + wantErr: "paths-ignore", + }, + { + name: "populated block-style branches is fine", + out: "on:\n push:\n branches:\n - main\n - release/*\n", + }, + { + name: "populated block-style paths is fine", + out: "on:\n push:\n branches: [main]\n paths:\n - src/**\n", + }, + { + name: "populated flow-style branches is fine", + out: "on:\n push:\n branches: [main]\n workflow_dispatch:\n", + }, + { + name: "no push trigger at all is fine", + out: "on:\n workflow_dispatch:\n inputs:\n dry_run:\n type: boolean\n", + }, + { + // A mapping key that merely ends in a guarded name must not trip the + // scan, and neither may a value that happens to mention one. + name: "unrelated keys and values are fine", + out: "jobs:\n x:\n steps:\n - run: echo \"branches:\"\n name: list-branches\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := assertNoDeadAllowList(tt.out) + if tt.wantErr == "" { + assert.NoError(t, err, "must not flag a workflow that runs") + return + } + require.Error(t, err, "an empty allow-list must be rejected") + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 4930aac2..8576a088 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sort" "strings" @@ -388,48 +389,71 @@ func (g *Generator) Generate() (string, error) { return out, nil } -// deadAllowLists are emitted fragments that each render a trigger filter -// matching nothing. A workflow carrying one is accepted by GitHub and reports -// green forever while never running, which makes the failure invisible. -var deadAllowLists = []string{ - "branches: []", - "branches:\n", - "paths: []", - "tags: []", -} +// allowListKeyRe matches a trigger filter key emitted as a YAML mapping key at +// any indent, capturing its indent and its inline value. Anchoring at the start +// of the line keeps the scan off values and off keys that merely end in one of +// these names, so a "run: echo branches:" step is not mistaken for a filter. +var allowListKeyRe = regexp.MustCompile( + `^(\s*)(branches|branches-ignore|paths|paths-ignore|tags|tags-ignore):[ \t]*(.*)$`) // assertNoDeadAllowList is the last gate before emitted YAML leaves the -// generator. Individual fields are validated upstream, but this catches the -// class rather than the instance: any present-but-empty trigger allow-list, -// from any field or future code path, silently disables the workflow it -// guards. Failing here turns that silence into a build error. +// generator. Individual fields are validated upstream; this catches the class +// rather than the instance. A trigger filter that is present but empty matches +// nothing and silently disables the trigger it guards, and GitHub accepts the +// workflow and reports it green forever, so the failure is invisible. +// +// The scan covers every occurrence in the text it is given, every filter key, +// and both YAML list styles (flow "key: []" and a block "key:" with no items). +// It is deliberately not limited to the one field that prompted it: the point +// is that a future emission site that forgets a length guard fails the build +// rather than shipping a workflow that never runs. func assertNoDeadAllowList(out string) error { - for _, frag := range deadAllowLists { - if !strings.Contains(out, frag) { + lines := strings.Split(out, "\n") + for i, line := range lines { + m := allowListKeyRe.FindStringSubmatch(line) + if m == nil { continue } - // "branches:\n" is only dead when no list item follows it. - if frag == "branches:\n" && !hasEmptyBlockList(out, frag) { + indent, key, value := m[1], m[2], strings.TrimSpace(m[3]) + + if value != "" { + // Flow style. Only an empty sequence is dead; anything else has + // entries. A trailing comment is not a value we need to judge. + if value == "[]" { + return deadAllowListErr(key) + } continue } - return fmt.Errorf( - "generated workflow contains an empty trigger allow-list (%q): "+ - "it would match nothing and the workflow would never run", - strings.TrimSuffix(frag, "\n")) + // Block style: dead unless a deeper "-" item follows. + if !hasBlockItems(lines[i+1:], len(indent)) { + return deadAllowListErr(key) + } } return nil } -// hasEmptyBlockList reports whether a block-style key is followed by something -// other than a "-" list item, meaning the key was emitted with no entries. -func hasEmptyBlockList(out, key string) bool { - idx := strings.Index(out, key) - if idx == -1 { - return false +// deadAllowListErr reports an empty trigger filter under the key that carries it. +func deadAllowListErr(key string) error { + return fmt.Errorf( + "generated workflow contains an empty %q trigger filter: "+ + "it would match nothing and the workflow would never run", key) +} + +// hasBlockItems reports whether the lines following a block-style key hold at +// least one sequence item indented deeper than the key. Blank and comment lines +// are skipped; the first line at or above the key's indent ends the block. +func hasBlockItems(rest []string, keyIndent int) bool { + for _, line := range rest { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + if len(line)-len(strings.TrimLeft(line, " ")) <= keyIndent { + return false + } + return strings.HasPrefix(trimmed, "- ") } - rest := out[idx+len(key):] - next := strings.TrimLeft(rest, " ") - return !strings.HasPrefix(next, "- ") + return false } // Validate checks for potential issues and returns warnings