From 617a9ba23411aa9ddf6cea98f0d4715d27f09a3b Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 9 Jul 2026 03:20:33 -0400 Subject: [PATCH] fix(config): reject merge_group on extra_triggers, pointing at merge_queue.enabled extra_triggers has exactly one sink: it attaches to the orchestrate workflow, which cuts release tags, publishes releases, runs deploys, and writes state. So extra_triggers.merge_group would run that side-effecting lane on a merge-queue speculative branch, and finalize could publish a real release from a candidate commit that never lands, writing state to a gh-readonly-queue ref. Validation now rejects merge_group under extra_triggers (top-level and per-component) with an error pointing at merge_queue.enabled, the read-only lane that exists to gate pull requests inside a merge queue. Behavior change: a manifest that set extra_triggers.merge_group was accepted before and is now rejected at validate. Migrate by setting merge_queue.enabled. The other extra_triggers (schedule, repository_dispatch, workflow_run) are unchanged. The three schema copies and the docs document the change and the migration. Signed-off-by: Joshua Temple --- docs/public/manifest.schema.json | 2 +- docs/src/content/docs/guides/companions.md | 2 +- docs/src/content/docs/reference/manifest.md | 7 +- docs/src/content/docs/start/how-it-works.md | 2 +- .../34-extra-orchestrate-triggers.yaml | 11 +-- internal/config/schema_v1_e2e_test.go | 3 +- .../config/validate_extra_triggers_test.go | 79 +++++++++++++++++++ internal/config/validate_v1.go | 25 ++++++ internal/generate/command_test.go | 50 +++++++++++- internal/schema/manifest.schema.json | 2 +- schema/manifest.schema.json | 2 +- 11 files changed, 168 insertions(+), 17 deletions(-) create mode 100644 internal/config/validate_extra_triggers_test.go diff --git a/docs/public/manifest.schema.json b/docs/public/manifest.schema.json index c95de554..0eb2a1a4 100644 --- a/docs/public/manifest.schema.json +++ b/docs/public/manifest.schema.json @@ -680,7 +680,7 @@ "merge_group": { "type": "object", "additionalProperties": false, - "description": "When present (even empty), wires the merge-queue trigger." + "description": "Not allowed: extra_triggers attaches to the side-effecting orchestrate workflow, which cuts release tags, publishes releases, and runs deploys while writing state, so a speculative merge-queue build could publish a real release from a candidate commit. cascade rejects this at validate. To gate pull requests inside a merge queue, set merge_queue.enabled, which emits a read-only validation lane." } } }, diff --git a/docs/src/content/docs/guides/companions.md b/docs/src/content/docs/guides/companions.md index f3ba9f45..044e2cc1 100644 --- a/docs/src/content/docs/guides/companions.md +++ b/docs/src/content/docs/guides/companions.md @@ -58,7 +58,7 @@ ci: enabled: true ``` -Enable it when you have turned on GitHub merge queues for the repository and want the same manifest and orchestration checks applied to the combined merge candidate, not just to each pull request in isolation. Wiring the merge-queue trigger itself is handled by the `merge_group` entry under [`extra_triggers`](/cascade/reference/manifest/#extra_triggers); this companion adds the validation that runs on it. +Enable it when you have turned on GitHub merge queues for the repository and want the same manifest and orchestration checks applied to the combined merge candidate, not just to each pull request in isolation. `merge_queue.enabled` wires the `merge_group` trigger onto this read-only lane for you. Attaching a raw `merge_group` trigger to the side-effecting orchestrate workflow through [`extra_triggers`](/cascade/reference/manifest/#extra_triggers) is rejected at validate, because a speculative merge-queue build could otherwise publish a real release from a candidate commit; `merge_queue.enabled` is the supported path. ## Wayfinding diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 04ddb720..498eeb9a 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -668,7 +668,6 @@ ci: - cron: "0 7 * * *" repository_dispatch: types: [deploy-request] - merge_group: {} ``` | Sub-field | Status | Description | @@ -676,7 +675,9 @@ ci: | `schedule` | emitted | List of cron schedule entries. Each entry has one required key, `cron`. | | `repository_dispatch` | emitted | Wires the `repository_dispatch` trigger; `types` lists the event types. | | `workflow_run` | emitted | Wires the `workflow_run` trigger. | -| `merge_group` | emitted | Present (even empty) wires the merge-queue trigger. The validation lane behavior lives in the separate `merge_queue` block. | +| `merge_group` | rejected | Not allowed. `extra_triggers` attaches to the side-effecting orchestrate workflow, which cuts release tags, publishes releases, and runs deploys while writing state, so a speculative merge-queue build could publish a real release from a candidate commit. cascade rejects `extra_triggers.merge_group` at validate. To gate pull requests inside a merge queue, set [`merge_queue.enabled`](#merge_queue), which emits a read-only validation lane. | + +Migrating from a manifest that set `extra_triggers.merge_group`: remove that entry and set `merge_queue.enabled: true` instead. The read-only merge-queue lane runs `cascade parse-config` and a dry-run `cascade orchestrate setup` against the queued candidate without cutting tags, publishing releases, or writing state. ### rollback @@ -761,7 +762,7 @@ The check validates cascade's own configuration only, requests `contents: read` |-------|--------|------|---------|-------------| | `enabled` | emitted | bool | false | Emit `.github/workflows/cascade-merge-queue.yaml`, a `merge_group`-triggered lane that runs `cascade parse-config` and a dry-run `cascade orchestrate setup` against the merge-group candidate. | -The lane is read-only. This block owns the lane behavior; the raw `merge_group` trigger is expressible separately under `extra_triggers.merge_group`, and the two are intentionally distinct. +The lane is read-only, which is exactly what a merge queue needs: it validates the queued candidate without cutting tags, publishing releases, or writing state. This is the supported way to participate in a merge queue. Attaching the raw `merge_group` event to the side-effecting orchestrate workflow through `extra_triggers.merge_group` is rejected at validate, because a speculative merge-queue build could otherwise publish a real release from a candidate commit. ## components diff --git a/docs/src/content/docs/start/how-it-works.md b/docs/src/content/docs/start/how-it-works.md index 8668076a..c00b8299 100644 --- a/docs/src/content/docs/start/how-it-works.md +++ b/docs/src/content/docs/start/how-it-works.md @@ -36,7 +36,7 @@ Every push to your trunk branch runs the orchestrate workflow, which builds and - **Manifest field:** `trunk_branch`, with the paths filter from `triggers` (or the per-callback triggers). - **Generated file:** `orchestrate.yaml`. -- **Fires on:** `push` to `trunk_branch` (filtered by `triggers`) and `workflow_dispatch`. Optional `repository_dispatch`, `workflow_run`, and `merge_group` triggers come from `extra_triggers`. +- **Fires on:** `push` to `trunk_branch` (filtered by `triggers`) and `workflow_dispatch`. Optional `repository_dispatch` and `workflow_run` triggers come from `extra_triggers`. To participate in a merge queue, set `merge_queue.enabled` for the read-only validation lane; attaching a raw `merge_group` trigger to this side-effecting workflow is rejected at validate. The `environments` list is the spine of the graph. Its order is the promotion order: a change moves from each environment to the next, one step at a time, carrying the same built artifact forward. diff --git a/e2e/scenarios/34-extra-orchestrate-triggers.yaml b/e2e/scenarios/34-extra-orchestrate-triggers.yaml index f149ccdd..716963c8 100644 --- a/e2e/scenarios/34-extra-orchestrate-triggers.yaml +++ b/e2e/scenarios/34-extra-orchestrate-triggers.yaml @@ -4,11 +4,15 @@ description: | on: entry into the generated orchestrate workflow alongside the baseline push and workflow_dispatch triggers (#324). - Covers all four extra trigger kinds in one manifest: + Covers the legitimate extra trigger kinds in one manifest: - schedule with cron expressions - repository_dispatch with event types - workflow_run with workflows and types - - merge_group (bare presence) + + merge_group is intentionally absent: it attaches the raw merge_group event to + the side-effecting orchestrate workflow, so cascade rejects it at validate and + points the operator at merge_queue.enabled instead. The rejection is covered by + the config validation unit tests and by generate-workflow's validate path. Generator-output verification only. @@ -32,7 +36,6 @@ config: - Upstream CI types: - completed - merge_group: {} steps: - name: "Initial commit; assert every extra trigger is emitted in orchestrate.yaml" @@ -61,5 +64,3 @@ steps: - " workflow_run:\n" - " - 'Upstream CI'\n" - " - completed\n" - # merge_group extra trigger (bare presence). - - " merge_group:\n" diff --git a/internal/config/schema_v1_e2e_test.go b/internal/config/schema_v1_e2e_test.go index a69ad15f..d531b4d8 100644 --- a/internal/config/schema_v1_e2e_test.go +++ b/internal/config/schema_v1_e2e_test.go @@ -36,7 +36,6 @@ const fullSurfaceManifest = `ci: workflow_run: workflows: [Upstream CI] types: [completed] - merge_group: {} pr_preview: enabled: true comment: true @@ -171,7 +170,7 @@ func TestFullSurfaceManifestE2E(t *testing.T) { if cfg.Deploys[0].Rollout.GetType() != "canary" || cfg.Deploys[0].DeployTarget.GetMode() != "gitops" { t.Fatalf("deploy reserved fields not parsed: %#v", cfg.Deploys[0]) } - if cfg.ExtraTriggers == nil || cfg.ExtraTriggers.MergeGroup == nil { + if cfg.ExtraTriggers == nil || cfg.ExtraTriggers.WorkflowRun == nil { t.Fatalf("extra_triggers not parsed: %#v", cfg.ExtraTriggers) } if cfg.Components == nil || cfg.Components["api"].Path != "services/api" { diff --git a/internal/config/validate_extra_triggers_test.go b/internal/config/validate_extra_triggers_test.go new file mode 100644 index 00000000..7ceb38c8 --- /dev/null +++ b/internal/config/validate_extra_triggers_test.go @@ -0,0 +1,79 @@ +package config + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// extraTriggersBaseConfig is a minimal valid manifest used to isolate the +// extra_triggers.merge_group validation rule. +func extraTriggersBaseConfig() *TrunkConfig { + return &TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev", "prod"}, + } +} + +func TestValidate_ExtraTriggersMergeGroup_TopLevelRejected(t *testing.T) { + cfg := extraTriggersBaseConfig() + cfg.ExtraTriggers = &ExtraTriggers{MergeGroup: &MergeGroupTrigger{}} + + errs := Validate(cfg) + joined := strings.Join(errs, "\n") + + assert.NotEmpty(t, errs, "extra_triggers.merge_group on the orchestrate workflow must be rejected") + assert.Contains(t, joined, "extra_triggers.merge_group") + assert.Contains(t, joined, "merge_queue.enabled", + "the rejection must point the user at the merge_queue.enabled read-only lane") +} + +func TestValidate_ExtraTriggersMergeGroup_PerComponentRejected(t *testing.T) { + cfg := extraTriggersBaseConfig() + cfg.Components = map[string]ComponentConfig{ + "api": { + Path: "services/api", + TagPrefix: "api-v", + ExtraTriggers: &ExtraTriggers{ + MergeGroup: &MergeGroupTrigger{}, + }, + }, + } + + errs := Validate(cfg) + joined := strings.Join(errs, "\n") + + assert.NotEmpty(t, errs, "a component's extra_triggers.merge_group must be rejected") + assert.Contains(t, joined, "components.api.extra_triggers.merge_group") + assert.Contains(t, joined, "merge_queue.enabled") +} + +func TestValidate_ExtraTriggersMergeGroup_MergeQueueEnabledAccepted(t *testing.T) { + cfg := extraTriggersBaseConfig() + cfg.MergeQueue = &MergeQueueConfig{Enabled: true} + + errs := Validate(cfg) + assert.Empty(t, errs, "merge_queue.enabled is the supported read-only lane and must pass validation") +} + +func TestValidate_ExtraTriggersMergeGroup_OtherExtraTriggersAccepted(t *testing.T) { + cfg := extraTriggersBaseConfig() + cfg.ExtraTriggers = &ExtraTriggers{ + Schedule: []ScheduleEntry{{Cron: "0 2 * * *"}}, + RepositoryDispatch: &RepositoryDispatchTrigger{Types: []string{"external-update"}}, + WorkflowRun: &WorkflowRunTrigger{Workflows: []string{"Upstream CI"}, Types: []string{"completed"}}, + } + + errs := Validate(cfg) + assert.Empty(t, errs, + "schedule, repository_dispatch, and workflow_run extra triggers stay legitimate and must pass validation") +} + +func TestValidate_ExtraTriggersMergeGroup_NilIsValid(t *testing.T) { + cfg := extraTriggersBaseConfig() + cfg.ExtraTriggers = nil + + errs := Validate(cfg) + assert.Empty(t, errs) +} diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index a1d43cf4..f4fb0b58 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -433,6 +433,7 @@ func validateConfigLevel(cfg *TrunkConfig) []string { errs = append(errs, validateActionFolder(cfg.ActionFolder)...) errs = append(errs, validateReconcile(cfg.Reconcile)...) errs = append(errs, validateTagGrammar(cfg)...) + errs = append(errs, validateExtraTriggers("extra_triggers", cfg.ExtraTriggers)...) return errs } @@ -583,6 +584,28 @@ func validateRepositoryDispatchTypes(prefix string, rd *RepositoryDispatchTrigge return errs } +// validateExtraTriggers checks an extra_triggers block. The only rule is on +// merge_group: extra_triggers attaches its events to the orchestrate workflow, +// which cuts release tags, publishes releases, and runs deploys while writing +// state. A raw merge_group trigger there lets a speculative merge-queue build on +// a candidate branch that may never land publish a real release and write real +// state, because orchestrate derives its target branch from the run ref with no +// gh-readonly-queue guard. The supported way to gate pull requests inside a +// merge queue is the read-only merge_queue.enabled lane, so merge_group under +// extra_triggers is rejected and the user is pointed at it. The prefix carries +// the component scope (or the top-level path) for an actionable message. +func validateExtraTriggers(prefix string, et *ExtraTriggers) []string { + if et == nil || et.MergeGroup == nil { + return nil + } + return []string{fmt.Sprintf( + "%s.merge_group is not allowed: extra_triggers attaches to the orchestrate workflow, "+ + "which cuts release tags, publishes releases, and runs deploys while writing state, so a "+ + "speculative merge-queue build could publish a real release from a candidate commit. To gate "+ + "pull requests inside a merge queue, set merge_queue.enabled, which emits a read-only validation lane.", + prefix)} +} + // validateRollback checks the opt-in rollback configuration. A nil block is the // default and passes. When repository_dispatch is set, its event types are // validated the same way the shared repository_dispatch trigger is. @@ -823,6 +846,8 @@ func validateComponents(cfg *TrunkConfig) []string { "components.%s.concurrency.group cannot be overridden; the orchestrate group is derived per component", name)) } + errs = append(errs, validateExtraTriggers(fmt.Sprintf("components.%s.extra_triggers", name), comp.ExtraTriggers)...) + for _, key := range sortedKeys(toAnyKeyed(comp.Extra)) { if _, global := globalOnlyComponentFields[key]; global { errs = append(errs, fmt.Sprintf( diff --git a/internal/generate/command_test.go b/internal/generate/command_test.go index a9d1cd4a..a295ac25 100644 --- a/internal/generate/command_test.go +++ b/internal/generate/command_test.go @@ -1,6 +1,7 @@ package generate import ( + "io" "os" "os/exec" "path/filepath" @@ -434,7 +435,6 @@ func TestRunGenerateWorkflow_ExtraTriggers(t *testing.T) { workflow_run: workflows: ["Upstream CI"] types: [completed] - merge_group: {} ` configPath := filepath.Join(tmpDir, "manifest.yaml") require.NoError(t, os.WriteFile(configPath, []byte(manifestContent), 0644)) @@ -470,7 +470,6 @@ func TestRunGenerateWorkflow_ExtraTriggers(t *testing.T) { assert.Contains(t, content, " workflow_run:\n", "workflow_run block must be emitted") assert.Contains(t, content, " - 'Upstream CI'\n", "workflow_run workflow name must appear") assert.Contains(t, content, " - completed\n", "workflow_run type must appear") - assert.Contains(t, content, " merge_group:\n", "merge_group trigger must be emitted") // Lane behavior must not be conflated with raw trigger emission. assert.NotContains(t, content, "merge_queue:", "merge_queue lane config must not appear here") @@ -478,3 +477,50 @@ func TestRunGenerateWorkflow_ExtraTriggers(t *testing.T) { // Must still be valid YAML structure with a jobs: section. assert.Contains(t, content, "jobs:\n", "jobs section must be present") } + +// TestRunGenerateWorkflow_MergeGroupRejected asserts that extra_triggers.merge_group +// is refused by generate-workflow: it would attach the raw merge_group event to the +// side-effecting orchestrate workflow, so a speculative merge-queue build could +// publish a real release. The error must point the operator at merge_queue.enabled, +// the supported read-only lane. +func TestRunGenerateWorkflow_MergeGroupRejected(t *testing.T) { + tmpDir := t.TempDir() + + manifestContent := `ci: + config: + trunk_branch: main + environments: [dev] + builds: + - name: app + workflow: .github/workflows/build.yaml + triggers: ["src/**"] + extra_triggers: + merge_group: {} +` + configPath := filepath.Join(tmpDir, "manifest.yaml") + require.NoError(t, os.WriteFile(configPath, []byte(manifestContent), 0644)) + + // generate-workflow prints each validation error to stderr and returns a + // generic "config validation failed"; capture stderr to assert the operator + // sees the actionable merge_queue.enabled pointer. + origStderr := os.Stderr + rPipe, wPipe, pipeErr := os.Pipe() + require.NoError(t, pipeErr) + os.Stderr = wPipe + + opts := defaultOpts(configPath, "") + opts.validateOnly = true + err := runGenerateWorkflow(opts) + + require.NoError(t, wPipe.Close()) + os.Stderr = origStderr + captured, readErr := io.ReadAll(rPipe) + require.NoError(t, readErr) + stderr := string(captured) + + require.Error(t, err, "extra_triggers.merge_group on the orchestrate workflow must be rejected") + assert.Contains(t, err.Error(), "validation failed") + assert.Contains(t, stderr, "extra_triggers.merge_group") + assert.Contains(t, stderr, "merge_queue.enabled", + "the rejection must point the operator at the merge_queue.enabled read-only lane") +} diff --git a/internal/schema/manifest.schema.json b/internal/schema/manifest.schema.json index c95de554..0eb2a1a4 100644 --- a/internal/schema/manifest.schema.json +++ b/internal/schema/manifest.schema.json @@ -680,7 +680,7 @@ "merge_group": { "type": "object", "additionalProperties": false, - "description": "When present (even empty), wires the merge-queue trigger." + "description": "Not allowed: extra_triggers attaches to the side-effecting orchestrate workflow, which cuts release tags, publishes releases, and runs deploys while writing state, so a speculative merge-queue build could publish a real release from a candidate commit. cascade rejects this at validate. To gate pull requests inside a merge queue, set merge_queue.enabled, which emits a read-only validation lane." } } }, diff --git a/schema/manifest.schema.json b/schema/manifest.schema.json index c95de554..0eb2a1a4 100644 --- a/schema/manifest.schema.json +++ b/schema/manifest.schema.json @@ -680,7 +680,7 @@ "merge_group": { "type": "object", "additionalProperties": false, - "description": "When present (even empty), wires the merge-queue trigger." + "description": "Not allowed: extra_triggers attaches to the side-effecting orchestrate workflow, which cuts release tags, publishes releases, and runs deploys while writing state, so a speculative merge-queue build could publish a real release from a candidate commit. cascade rejects this at validate. To gate pull requests inside a merge queue, set merge_queue.enabled, which emits a read-only validation lane." } } },