diff --git a/CHANGELOG.md b/CHANGELOG.md index 6683b11..13bc60a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,37 @@ 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 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 + 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 7bd422a..0c79050 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 0269879..0a7ca71 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 6d41678..f0d0df7 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 c16d425..fafc2b8 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 670e979..4925543 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 50bca6b..46933bf 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 cb5fd17..3ab4ef3 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 841cfdc..65388db 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 63d3ce4..f57ce59 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 e9c0d4b..1019a1a 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 0000000..5f7f693 --- /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 0000000..bbdc8ba --- /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 1a67690..33ee19d 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 bb5e9f5..3d61600 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 c725e96..ba4004f 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 94174fb..a9949fb 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/dead_allow_list_test.go b/internal/generate/dead_allow_list_test.go new file mode 100644 index 0000000..92d5b83 --- /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 bd01e62..8576a08 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sort" "strings" @@ -346,6 +347,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 +381,79 @@ 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 +} + +// 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; 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 { + lines := strings.Split(out, "\n") + for i, line := range lines { + m := allowListKeyRe.FindStringSubmatch(line) + if m == nil { + continue + } + 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 + } + // Block style: dead unless a deeper "-" item follows. + if !hasBlockItems(lines[i+1:], len(indent)) { + return deadAllowListErr(key) + } + } + return nil +} + +// 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, "- ") + } + return false } // 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 0000000..ca75735 --- /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 701430b..133a984 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 7f1ad78..d894c04 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 8e8e5fe..5274401 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 {