From 4ea9bcd287113368fee0973566b787d16085eaa0 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 18 Jul 2026 04:27:20 -0400 Subject: [PATCH 1/3] fix(generate): close four silent generated-output defects in promote, rollback, and deploy gating Each emitted valid YAML that did the wrong thing, so actionlint alone did not catch it. Regression tests assert the emitted output and red before the fix. - promote: matrix deploys now thread the per-promotion environment and sha to their callback (as orchestrate does) instead of deploying to an empty environment while the job name references a matrix key that was never set. - rollback: the repository_dispatch dry-run guard matches both a JSON boolean true (client_payload) and the string 'true' (workflow_dispatch); a bare "!= 'true'" read a boolean true as not-a-dry-run and ran real deploys. - generate: a dependent deploy is gated on the base deploy's effective result (base or any retry shim), reusing the retry-aware helper, and no longer emits the dependency clause twice. - promote: a dry-run promote no longer creates a real GitHub Deployment, and the terminal status counts a skipped deploy as success and includes the prod job. Signed-off-by: Joshua Temple --- CHANGELOG.md | 24 ++ e2e/scenarios/31-native-deployments.yaml | 6 +- .../32-rollback-repository-dispatch.yaml | 6 + e2e/scenarios/43-deploy-rollout-strategy.yaml | 5 + internal/generate/generator.go | 46 +++- internal/generate/native_deployments.go | 29 ++- internal/generate/native_deployments_test.go | 8 +- .../generate/pass10_silent_output_test.go | 240 ++++++++++++++++++ internal/generate/promote.go | 37 ++- internal/generate/rollback.go | 27 +- 10 files changed, 406 insertions(+), 22 deletions(-) create mode 100644 internal/generate/pass10_silent_output_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 054aae98..2a26bcec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,30 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **promote:** A matrix-based promote deploy (a deploy declaring inputs) now + threads the per-promotion environment and sha to its reusable-workflow + callback, matching orchestrate. The matrix path previously emitted only the + declared manifest inputs, so the callback ran with an empty environment while + the job name referenced `${{ matrix.environment }}`, a key the matrix builder + never set. environment and sha are added to every matrix entry and passed in + the `with:` block (sha only when the callback declares it, and neither when the + manifest already wires it as an explicit input). +- **rollback:** The repository_dispatch dry-run guard now treats a JSON boolean + `true` from `client_payload` and the string `'true'` from a workflow_dispatch + input alike. A bare `!= 'true'` compared a boolean against a string, which + GitHub Actions coerces numerically, so a natural `{"dry_run": true}` payload + read as not-a-dry-run and a dry-run rollback ran real deploys and wrote + rolled-back state. Without the trigger the output is unchanged. +- **generate:** A dependent deploy is now gated on the base deploy's effective + result (the base job or any retry shim succeeded), so a base deploy rescued by + a retry no longer skips the deploys that depend on it. The condition was + reading the base job's immutable `result`, frozen at `failure` after a rescued + attempt, and a deploy-on-deploy dependency also emitted the clause twice. +- **promote:** A dry-run promote no longer creates a real GitHub Deployment, and + the Deployment's terminal status no longer counts a legitimately skipped deploy + as a failure or omits the prod deploy result. The native-deployment lifecycle + steps are gated on a non-dry-run run, and the status expression judges each + deploy (including its prod job) on success-or-skipped. - **state:** A single-component (flat) state write on a manifest that carries per-component state no longer destroys that state. The flat state map models environments only, so parsing a component-scoped manifest lifted diff --git a/e2e/scenarios/31-native-deployments.yaml b/e2e/scenarios/31-native-deployments.yaml index 3e155026..313d5608 100644 --- a/e2e/scenarios/31-native-deployments.yaml +++ b/e2e/scenarios/31-native-deployments.yaml @@ -49,13 +49,15 @@ steps: contains: - " deployments: write" - " - name: Create deployment" - - " if: ${{ github.server_url == 'https://github.com' }}" + # A dry-run run skips its deploys, so it must not create a real + # Deployment: every lifecycle step is also gated on a non-dry-run run. + - " if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' }}" - " deployment_id=$(gh api repos/${{ github.repository }}/deployments \\" - " --field auto_inactive=false \\" - " - name: Set deployment in_progress" - " --field state=in_progress" - " - name: Set deployment status" - - " if: ${{ github.server_url == 'https://github.com' && always() }}" + - " if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' && always() }}" - " production) environment_url='https://app.example.com' ;;" - name: "Regenerate and confirm no drift" diff --git a/e2e/scenarios/32-rollback-repository-dispatch.yaml b/e2e/scenarios/32-rollback-repository-dispatch.yaml index 62d95526..dd0d8294 100644 --- a/e2e/scenarios/32-rollback-repository-dispatch.yaml +++ b/e2e/scenarios/32-rollback-repository-dispatch.yaml @@ -59,6 +59,12 @@ steps: # deploy guard and finalize gate coalesce dry_run and deployable. - "github.event.inputs.dry_run || github.event.client_payload.dry_run" - "github.event.inputs.deployable || github.event.client_payload.deployable" + # The dry_run guard matches both a JSON boolean true (client_payload) + # and the string 'true' (workflow_dispatch input): a natural + # {"dry_run": true} payload sends a boolean, and a bare "!= 'true'" + # would read it as not-a-dry-run and run real deploys. + - "(github.event.inputs.dry_run || github.event.client_payload.dry_run) != true" + - "(github.event.inputs.dry_run || github.event.client_payload.dry_run) != 'true'" not_contains: # the bare, un-coalesced reads must be gone once the toggle is on. - "ENVIRONMENT: ${{ github.event.inputs.environment }}" diff --git a/e2e/scenarios/43-deploy-rollout-strategy.yaml b/e2e/scenarios/43-deploy-rollout-strategy.yaml index a49e0468..77759669 100644 --- a/e2e/scenarios/43-deploy-rollout-strategy.yaml +++ b/e2e/scenarios/43-deploy-rollout-strategy.yaml @@ -72,3 +72,8 @@ steps: contains: - "fail-fast: true" - "max-parallel: 2" + # The matrix deploy must thread the per-promotion environment and sha + # to its callback (the way orchestrate does), or the deploy targets an + # empty environment while its job name references matrix.environment. + - "environment: ${{ matrix.environment }}" + - "sha: ${{ matrix.sha }}" diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 2422b471..7cba61fc 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -1264,6 +1264,9 @@ func (g *Generator) writeStrategyBlock(sb *strings.Builder, m *config.MatrixConf func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, needs []string) { var conditions []string buildLinkedDeploy := false + // Dependencies already emitted by the build-linked path below, so the + // general dependency loop does not emit a second, duplicate clause for them. + linkedDeps := make(map[string]struct{}) // For deploys with depends_on build, check if build ran successfully // instead of using setup detection @@ -1278,14 +1281,20 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee if err != nil { continue } - // Apply run_policy to build dependency check + // Record so the general dependency loop below does not emit + // a second, duplicate clause for the same dependency. + linkedDeps[depJobID] = struct{}{} + depRetries := g.graph.Nodes[depJobID].Retries + // Apply run_policy to the dependency check, judged on the + // dependency's effective result (base OR any retry shim + // succeeded) so a rescued deploy does not skip its dependents. switch info.RunPolicy { case config.RunPolicyAlways: - conditions = append(conditions, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", depJobID, depJobID)) + conditions = append(conditions, fmt.Sprintf("(%s || needs.%s.result == 'skipped')", effectiveSuccessCond(depJobID, depRetries), depJobID)) case config.RunPolicyForce: // No condition needed for force default: - conditions = append(conditions, fmt.Sprintf("needs.%s.result == 'success'", depJobID)) + conditions = append(conditions, effectiveDepSuccessGate(depJobID, depRetries)) } } break @@ -1309,12 +1318,17 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee if buildLinkedDeploy && depInfo.Type == config.CallbackTypeBuild { continue } + // Skip any dependency the build-linked path already emitted, so a + // deploy-on-deploy dependency is not gated by a duplicated clause. + if _, done := linkedDeps[depJobID]; done { + continue + } switch info.RunPolicy { case config.RunPolicyDefault, "": - conditions = append(conditions, fmt.Sprintf("needs.%s.result == 'success'", depJobID)) + conditions = append(conditions, effectiveDepSuccessGate(depJobID, depInfo.Retries)) case config.RunPolicyAlways: - conditions = append(conditions, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", depJobID, depJobID)) + conditions = append(conditions, fmt.Sprintf("(%s || needs.%s.result == 'skipped')", effectiveSuccessCond(depJobID, depInfo.Retries), depJobID)) case config.RunPolicyForce: // No dependency condition } @@ -1733,7 +1747,11 @@ func (g *Generator) writeNativeDeploymentSteps(sb *strings.Builder, sorted []str resultExpr = fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", strings.Join(conds, " && ")) } - writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, " ") + // A dry-run orchestrate skips its deploy callbacks, so it must not create a + // real GitHub Deployment either. github.event.inputs.dry_run is null-safe on + // the non-dispatch triggers (push/schedule/workflow_run), where it renders + // empty and reads as not-a-dry-run. + writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, "github.event.inputs.dry_run != 'true'", " ") } func (g *Generator) writeSummaryStep(sb *strings.Builder, sorted []string) { @@ -2118,6 +2136,22 @@ func effectiveSuccessCond(jobName string, retries int) string { return cond } +// effectiveDepSuccessGate renders the "dependency satisfied" clause for a +// downstream job's if: condition, judged on the dependency's effective result so +// a base deploy that failed but was rescued by a retry shim still lets its +// dependents run. It reuses effectiveSuccessCond (the shared retry-aware helper) +// and parenthesizes the disjunction only when a retry ladder is present, since +// callers join these clauses with " && "; with no retries it collapses to the +// bare needs..result == 'success', so a manifest without retries emits +// byte-identical output. +func effectiveDepSuccessGate(jobName string, retries int) string { + cond := effectiveSuccessCond(jobName, retries) + if retries > 0 { + return "(" + cond + ")" + } + return cond +} + // effectiveResultExpr renders a callback's effective result as a ${{ }} // expression evaluating to the string 'success' or 'failure', suitable for an // env: value that shell then compares against "success". diff --git a/internal/generate/native_deployments.go b/internal/generate/native_deployments.go index acd6b70a..87f39ac9 100644 --- a/internal/generate/native_deployments.go +++ b/internal/generate/native_deployments.go @@ -31,20 +31,33 @@ func deploymentAutoInactive(cfg *config.TrunkConfig) bool { // envExpr is the shell-safe expression that resolves to the target environment // name at run time (it differs between the orchestrate and promote seams). // resultExpr is the shell expression that evaluates to "success" or "failure" -// for the deploy outcome. indent is the per-step indent (matching the -// surrounding generated YAML). -func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, envExpr, resultExpr, indent string) { +// for the deploy outcome. dryRunGuard is the expression body (no ${{ }} wrapper) +// that is true when the run is NOT a dry run; a dry run must never create a real +// GitHub Deployment, so it is ANDed into every step's if:. An empty dryRunGuard +// leaves the steps ungated (no dry-run concept for that seam). indent is the +// per-step indent (matching the surrounding generated YAML). +func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, envExpr, resultExpr, dryRunGuard, indent string) { if !nativeDeploymentsEnabled(cfg) { return } body := indent + " " + // Compose the server guard with the non-dry-run guard so a dry run neither + // creates the Deployment nor posts status against a Deployment that was never + // created. serverGuard is the bare comparison (no ${{ }} wrapper) so it can + // be joined with the other clauses inside a single expression. + serverGuard := stripTokenExprWrapper(appTokenServerGuard) + createGuard := "${{ " + serverGuard + " }}" + if dryRunGuard != "" { + createGuard = "${{ " + serverGuard + " && " + dryRunGuard + " }}" + } + // Create the Deployment. The environment name is resolved at run time, so the // URL lookup is a shell case over the configured environment_config entries. sb.WriteString(indent + "- name: Create deployment\n") sb.WriteString(body + "id: cascade-deployment\n") - sb.WriteString(body + "if: " + appTokenServerGuard + "\n") + sb.WriteString(body + "if: " + createGuard + "\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") @@ -61,7 +74,7 @@ func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, en // Mark the deployment in_progress. sb.WriteString(indent + "- name: Set deployment in_progress\n") - sb.WriteString(body + "if: " + appTokenServerGuard + "\n") + sb.WriteString(body + "if: " + createGuard + "\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") @@ -72,8 +85,12 @@ func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, en // Report the terminal status. always() lets it run even when a deploy failed, // so the Deployment never sticks at in_progress. The URL is selected by the // runtime environment name from the configured environment_config entries. + statusGuard := serverGuard + if dryRunGuard != "" { + statusGuard += " && " + dryRunGuard + } sb.WriteString(indent + "- name: Set deployment status\n") - sb.WriteString(body + "if: ${{ " + stripTokenExprWrapper(appTokenServerGuard) + " && always() }}\n") + sb.WriteString(body + "if: ${{ " + statusGuard + " && always() }}\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") diff --git a/internal/generate/native_deployments_test.go b/internal/generate/native_deployments_test.go index 52f75cd2..52e4cb0e 100644 --- a/internal/generate/native_deployments_test.go +++ b/internal/generate/native_deployments_test.go @@ -32,7 +32,7 @@ on: require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/deploy.yaml"), []byte(deployWorkflow), 0o644)) cfg := &config.TrunkConfig{ - TrunkBranch: "main", + TrunkBranch: "main", Environments: []config.EnvironmentEntry{ {Name: "production", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://app.example.com"}}, }, @@ -63,8 +63,12 @@ func TestNativeDeployments_Enabled(t *testing.T) { "must POST to the deployment statuses collection") assert.Contains(t, out, "https://app.example.com", "the configured environment_url must be wired into the status update") - assert.Contains(t, out, appTokenServerGuard, + assert.Contains(t, out, "github.server_url == 'https://github.com'", "deployment steps must be guarded to real GitHub via the server_url guard") + // A dry-run orchestrate skips its deploys, so it must not create a real + // Deployment: every lifecycle step is also gated on a non-dry-run run. + assert.Contains(t, out, "github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true'", + "deployment steps must not run on a dry-run orchestrate") // The terminal status reflects the deploy job result, not a hardcoded value. assert.Contains(t, out, "needs.deploy-app.result", "terminal status must derive from the deploy job result") diff --git a/internal/generate/pass10_silent_output_test.go b/internal/generate/pass10_silent_output_test.go new file mode 100644 index 00000000..ede3acaa --- /dev/null +++ b/internal/generate/pass10_silent_output_test.go @@ -0,0 +1,240 @@ +package generate + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pass10DeployWorkflow is a deploy callback that declares the standard +// environment and sha inputs plus a custom region input, so a manifest deploy +// with inputs takes the matrix path and the framework may thread environment/sha. +const pass10DeployWorkflow = `name: Deploy +on: + workflow_call: + inputs: + environment: + type: string + sha: + type: string + region: + type: string +` + +func pass10Fixture(t *testing.T, workflow string) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github/workflows"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, ".github/workflows/deploy.yaml"), []byte(workflow), 0o644)) + return dir +} + +// pass10JobBlock returns the lines of the named job, from its " :" header up to +// the next top-level job header (a line starting with two spaces then a word +// then a colon at the same indent), so an assertion can scope to one job. +func pass10JobBlock(t *testing.T, content, jobID string) string { + t.Helper() + lines := strings.Split(content, "\n") + start := -1 + header := " " + jobID + ":" + for i, l := range lines { + if l == header { + start = i + break + } + } + require.GreaterOrEqual(t, start, 0, "job %q not found", jobID) + end := len(lines) + for i := start + 1; i < len(lines); i++ { + l := lines[i] + if len(l) > 2 && l[0] == ' ' && l[1] == ' ' && l[2] != ' ' && strings.HasSuffix(strings.TrimSpace(l), ":") && !strings.HasPrefix(l, " ") { + end = i + break + } + } + return strings.Join(lines[start:end], "\n") +} + +// TestGB2_PromoteMatrixDeploy_ThreadsEnvironmentAndSha proves the promote matrix +// deploy path passes the per-promotion environment and sha to its callback the +// way orchestrate does. Before the fix the matrix with: block carried only +// declared manifest inputs, so a promote deployed to an empty environment while +// the job name referenced ${{ matrix.environment }}, a key the matrix builder +// never set. +func TestGB2_PromoteMatrixDeploy_ThreadsEnvironmentAndSha(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + + block := pass10JobBlock(t, out, "deploy-web") + assert.Contains(t, block, "environment: ${{ matrix.environment }}", + "matrix deploy must thread the per-promotion environment to the callback") + assert.Contains(t, block, "sha: ${{ matrix.sha }}", + "matrix deploy must thread sha when the callback declares it") + + // The matrix builder must inject environment/sha onto each entry so the job + // name and the with: inputs resolve to real values. + assert.Contains(t, out, "'. + {environment: $env, sha: $sha}'", + "matrix builder must carry environment and sha on every matrix entry") +} + +// TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared keeps sha gated on +// declaration, matching orchestrate: a callback that does not declare sha must +// not receive it (an undeclared reusable-workflow input is a hard error). +func TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared(t *testing.T) { + wf := "name: Deploy\non:\n workflow_call:\n inputs:\n environment:\n type: string\n region:\n type: string\n" + dir := pass10Fixture(t, wf) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-web") + assert.Contains(t, block, "environment: ${{ matrix.environment }}") + assert.NotContains(t, block, "sha: ${{ matrix.sha }}", + "sha must not be threaded to a callback that does not declare it") +} + +// TestGM4_RollbackDispatch_DryRunTreatsBooleanAndString proves the rollback +// deploy guard and finalize gate treat a JSON boolean true (client_payload) and +// the string 'true' (workflow_dispatch) alike. Before the fix a bare "!= 'true'" +// read a boolean true as not-a-dry-run, so a dry-run rollback ran real deploys. +func TestGM4_RollbackDispatch_DryRunTreatsBooleanAndString(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + Rollback: &config.RollbackConfig{RepositoryDispatch: &config.RepositoryDispatchTrigger{Types: []string{"rollback-requested"}}}, + } + out, err := NewRollbackGenerator(cfg, dir).Generate() + require.NoError(t, err) + + coalesced := "github.event.inputs.dry_run || github.event.client_payload.dry_run" + // Both the boolean and string forms of dry_run must be excluded. + assert.Contains(t, out, "("+coalesced+") != true", + "guard must treat a JSON boolean true as a dry run") + assert.Contains(t, out, "("+coalesced+") != 'true'", + "guard must treat the string 'true' as a dry run") + // The old, single-form guard must be gone. + assert.NotContains(t, out, "("+coalesced+") != 'true' && (github.event.inputs.deployable", + "the boolean-blind single-comparison guard must be replaced") +} + +// TestGM4_Rollback_NoDispatch_ByteIdenticalDryRun keeps the non-dispatch output +// unchanged: without repository_dispatch the dry_run signal is always the +// workflow_dispatch string, so the guard stays the bare string comparison. +func TestGM4_Rollback_NoDispatch_ByteIdenticalDryRun(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + } + out, err := NewRollbackGenerator(cfg, dir).Generate() + require.NoError(t, err) + assert.Contains(t, out, "github.event.inputs.dry_run != 'true'") + assert.NotContains(t, out, "!= true &&", "non-dispatch guard must not gain the boolean comparison") +} + +// TestGM5_DependentDeploy_JudgesEffectiveResult proves a dependent deploy runs +// when the base deploy's ladder succeeded via a retry shim, not only when the +// immutable base result is success. It also proves the previously duplicated +// clause is gone. +func TestGM5_DependentDeploy_JudgesEffectiveResult(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, Retries: 2}, + {Name: "api", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + + block := pass10JobBlock(t, out, "deploy-api") + assert.Contains(t, block, "needs.deploy-web-retry-1.result == 'success'", + "dependent deploy must consult the base deploy's retry shims") + assert.Contains(t, block, "needs.deploy-web-retry-2.result == 'success'") + // The duplicated immutable-result clause must be gone. + assert.NotContains(t, block, "needs.deploy-web.result == 'success' &&\n needs.deploy-web.result == 'success'", + "the duplicated dependency clause must be removed") +} + +// TestGM5_DependentDeploy_NoRetries_SingleClause proves the N=0 path: no retries +// means the effective helper collapses to the bare result read, and the dedup +// leaves exactly one clause (not the previously duplicated pair). +func TestGM5_DependentDeploy_NoRetries_SingleClause(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}}, + {Name: "api", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-api") + assert.Equal(t, 1, strings.Count(block, "needs.deploy-web.result == 'success'"), + "a no-retry dependency must gate on exactly one, non-duplicated clause") + assert.NotContains(t, block, "retry", "no retry shims exist for a zero-retry dependency") +} + +// TestGM6_PromoteNativeDeployment_GuardsDryRunAndCountsSkips proves a dry-run +// promote does not create a real GitHub Deployment, and that the terminal status +// counts a legitimately skipped deploy as success and includes the prod deploy. +func TestGM6_PromoteNativeDeployment_GuardsDryRunAndCountsSkips(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + tru := true + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []config.EnvironmentEntry{ + {Name: "staging", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://staging.example.com"}}, + {Name: "production", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://app.example.com"}}, + }, + Deployments: &config.DeploymentsConfig{Enabled: &tru}, + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + + // A dry run must not create a real Deployment. + assert.Contains(t, out, "if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' }}", + "the Create deployment step must be gated on a non-dry-run promote") + assert.Contains(t, out, "github.event.inputs.dry_run != 'true' && always()", + "the status step must also be gated on a non-dry-run promote") + + // A skipped deploy is not a failure, and the prod deploy is not omitted. + assert.Contains(t, out, "needs.deploy-app.result == 'success' || needs.deploy-app.result == 'skipped'", + "a legitimately skipped deploy must not be counted as a Deployment failure") + assert.Contains(t, out, "needs.deploy-app-prod.result == 'success' || needs.deploy-app-prod.result == 'skipped'", + "the prod deploy result must be included in the Deployment status") +} diff --git a/internal/generate/promote.go b/internal/generate/promote.go index fe67b958..14b9dceb 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -448,6 +448,13 @@ func (g *PromoteGenerator) writeMatrixBuildingLogic(sb *strings.Builder, outputN sb.WriteString(" gsub(\"\\\\$\\\\{\\\\{ matrix.version \\\\}\\\\}\"; $version)\n") sb.WriteString(" else . end)')\n") sb.WriteString(" \n") + sb.WriteString(" # Carry the per-promotion environment and sha on the matrix entry so\n") + sb.WriteString(" # the deploy job name and its environment/sha with: inputs resolve.\n") + sb.WriteString(" RESOLVED=$(echo \"$RESOLVED\" | jq -c \\\n") + sb.WriteString(" --arg env \"$ENV\" \\\n") + sb.WriteString(" --arg sha \"$SHA\" \\\n") + sb.WriteString(" '. + {environment: $env, sha: $sha}')\n") + sb.WriteString(" \n") sb.WriteString(" # Add to matrix (with comma separator)\n") sb.WriteString(" if [ \"$FIRST\" = \"true\" ]; then\n") fmt.Fprintf(sb, " MATRIX_%s=\"${MATRIX_%s}${RESOLVED}\"\n", strings.ToUpper(outputName), strings.ToUpper(outputName)) @@ -774,6 +781,22 @@ func (g *PromoteGenerator) writeDeployJobs(sb *strings.Builder) { fmt.Fprintf(sb, " uses: %s\n", normalizeWorkflowPath(d.Workflow)) sb.WriteString(" with:\n") + // Thread the per-promotion environment and (when the callback + // declares it) sha from the matrix entry. Orchestrate auto-passes + // both to the same callback; without them here a promote deploys to + // an empty environment while the job name (Deploy X (${{ + // matrix.environment }})) references a key the matrix would not carry. + // environment mirrors orchestrate's contract that every deploy + // callback accepts it; sha is gated on declaration. Skip either when + // the manifest already wires it as an explicit input, so the with: + // block never emits a duplicate mapping key. + if _, ok := d.Inputs["environment"]; !ok { + sb.WriteString(" environment: ${{ matrix.environment }}\n") + } + if _, ok := d.Inputs["sha"]; !ok && g.deployHasInput(d.Name, "sha") { + sb.WriteString(" sha: ${{ matrix.sha }}\n") + } + // When the callback opts in to dry-run passthrough, forward the // dispatch input so it can emulate internally. if d.SupportsDryRun { @@ -1350,12 +1373,22 @@ func (g *PromoteGenerator) writeNativeDeploymentSteps(sb *strings.Builder) { if len(g.config.Environments) > 0 && len(g.config.Deploys) > 0 { var conds []string for _, d := range g.config.Deploys { - conds = append(conds, fmt.Sprintf("needs.deploy-%s.result == 'success'", d.Name)) + // A promotion runs a subset of deploys (only those whose scope + // includes the target env), and the prod deploy runs only in cascade + // mode, so a deploy job legitimately SKIPS. A skip is not a deploy + // failure, so success-or-skipped is the per-job success signal. The + // prod job is included so a failed prod deploy is not omitted from + // the Deployment's terminal status. + for _, job := range []string{fmt.Sprintf("deploy-%s", d.Name), fmt.Sprintf("deploy-%s-prod", d.Name)} { + conds = append(conds, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", job, job)) + } } resultExpr = fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", strings.Join(conds, " && ")) } - writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, " ") + // A dry-run promote skips every deploy and must not create a real GitHub + // Deployment; guard the Deployment lifecycle on the dispatch dry_run input. + writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, "github.event.inputs.dry_run != 'true'", " ") } // writeConcurrency emits a top-level concurrency: block on the promote workflow. diff --git a/internal/generate/rollback.go b/internal/generate/rollback.go index d5f53d7c..cfabb316 100644 --- a/internal/generate/rollback.go +++ b/internal/generate/rollback.go @@ -357,7 +357,7 @@ func (g *RollbackGenerator) writePreflightJob(sb *strings.Builder) { // Actions expression (a comparison or boolean operand). In the baseline it is // the bare github.event.inputs.; when the repository_dispatch trigger is // enabled the coalescing "inputs || client_payload" is wrapped in parentheses so -// the surrounding operator (e.g. != 'true', == '') binds to the whole coalesced +// the surrounding operator (e.g. != 'true', == ”) binds to the whole coalesced // value rather than only the client_payload half. func (g *RollbackGenerator) paramReadExpr(name string) string { if g.dispatchTrigger() != nil { @@ -372,9 +372,28 @@ func (g *RollbackGenerator) paramReadExpr(name string) string { // repository_dispatch trigger is enabled, so an external signal honors the same // dry-run and deployable scoping the manual path does. func (g *RollbackGenerator) rollbackDeployGuard(deployName string) string { - dryRun := g.paramReadExpr("dry_run") deployable := g.paramReadExpr("deployable") - return fmt.Sprintf("${{ %s != 'true' && (%s == '' || %s == '%s') }}", dryRun, deployable, deployable, deployName) + return fmt.Sprintf("${{ %s && (%s == '' || %s == '%s') }}", g.notDryRunExpr(), deployable, deployable, deployName) +} + +// notDryRunExpr returns the expression body (no ${{ }} wrapper) that is true +// when the rollback is NOT a dry run, so a deploy or finalize job may proceed. +// +// Without the repository_dispatch trigger the dry_run signal is only ever the +// workflow_dispatch input, which is always the string 'true' or 'false', so the +// output collapses to the bare string comparison and stays byte-identical to the +// baseline. With the trigger enabled the coalesced read can also carry a JSON +// boolean true from client_payload (a natural {"dry_run": true} payload), and +// GitHub Actions compares a boolean against the string 'true' by numeric +// coercion (the string casts to NaN), so a bare "!= 'true'" reads a boolean true +// as NOT a dry run and lets a dry-run rollback perform real deploys. Matching +// both the boolean literal and the string closes that gap for either source. +func (g *RollbackGenerator) notDryRunExpr() string { + if g.dispatchTrigger() == nil { + return "github.event.inputs.dry_run != 'true'" + } + v := g.paramRead("dry_run") // github.event.inputs.dry_run || github.event.client_payload.dry_run + return fmt.Sprintf("(%s) != true && (%s) != 'true'", v, v) } // writeDeployJobs emits one deploy job per configured deploy, re-running the same @@ -432,7 +451,7 @@ func (g *RollbackGenerator) writeFinalizeJob(sb *strings.Builder) { sb.WriteString(" finalize:\n") sb.WriteString(" name: Finalize\n") fmt.Fprintf(sb, " needs: %s\n", needsStr) - fmt.Fprintf(sb, " if: always() && needs.preflight.result == 'success' && %s != 'true'\n", g.paramReadExpr("dry_run")) + fmt.Fprintf(sb, " if: always() && needs.preflight.result == 'success' && %s\n", g.notDryRunExpr()) sb.WriteString(" runs-on: ubuntu-latest\n") // The finalize job commits the rolled-back state, so it needs contents: write. writeJobPermissions(sb, " ", [][2]string{{"contents", "write"}}) From c3c43f1e100b15628909b3916b5f34c7b7fee1e1 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 18 Jul 2026 04:35:31 -0400 Subject: [PATCH 2/3] style(generate): restore ASCII empty-string idiom in rollback comment Signed-off-by: Joshua Temple --- internal/generate/rollback.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/generate/rollback.go b/internal/generate/rollback.go index cfabb316..62739f4c 100644 --- a/internal/generate/rollback.go +++ b/internal/generate/rollback.go @@ -357,7 +357,7 @@ func (g *RollbackGenerator) writePreflightJob(sb *strings.Builder) { // Actions expression (a comparison or boolean operand). In the baseline it is // the bare github.event.inputs.; when the repository_dispatch trigger is // enabled the coalescing "inputs || client_payload" is wrapped in parentheses so -// the surrounding operator (e.g. != 'true', == ”) binds to the whole coalesced +// the surrounding operator (e.g. != 'true', == '') binds to the whole coalesced // value rather than only the client_payload half. func (g *RollbackGenerator) paramReadExpr(name string) string { if g.dispatchTrigger() != nil { From c8344603eacf6755e59c634866b296d2934e5110 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 18 Jul 2026 05:04:04 -0400 Subject: [PATCH 3/3] fix(promote): resolve deploy callback path so a matrix deploy threads sha The GB2 fix threaded sha only when the callback declares it, but declared-input detection read the raw workflow path (baseDir/deploy.yaml) while the emitted uses: line and the orchestrate generator resolve a bare filename to .github/workflows/deploy.yaml. A manifest referencing the callback by bare name therefore failed detection silently, so deployHasInput(sha) was false and the matrix with: block dropped sha while still threading environment. discoverDeployInputs now resolves through normalizeWorkflowPath (and skips cross-repo refs), matching the uses: emission, so a canonically-located callback is found and its declared sha is threaded. The regression tests reference the callback by bare filename, the way the e2e scenarios do, so they red when detection drops sha instead of passing against a fully-qualified shortcut. Signed-off-by: Joshua Temple --- .../generate/pass10_silent_output_test.go | 29 +++++++++++++++++-- internal/generate/promote.go | 14 ++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/internal/generate/pass10_silent_output_test.go b/internal/generate/pass10_silent_output_test.go index ede3acaa..1ca56175 100644 --- a/internal/generate/pass10_silent_output_test.go +++ b/internal/generate/pass10_silent_output_test.go @@ -67,12 +67,16 @@ func pass10JobBlock(t *testing.T, content, jobID string) string { // the job name referenced ${{ matrix.environment }}, a key the matrix builder // never set. func TestGB2_PromoteMatrixDeploy_ThreadsEnvironmentAndSha(t *testing.T) { + // The manifest references the callback by a BARE filename (deploy.yaml), the + // same way the e2e scenarios do. Declared-input detection must resolve it to + // the canonical .github/workflows/deploy.yaml where the file is staged; a + // fully-qualified path here would mask the resolution bug that dropped sha. dir := pass10Fixture(t, pass10DeployWorkflow) cfg := &config.TrunkConfig{ TrunkBranch: "main", Environments: config.EnvNames("dev", "prod"), Deploys: []config.DeployConfig{{ - Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, Inputs: map[string]interface{}{"region": "us-east-1"}, }}, } @@ -101,7 +105,7 @@ func TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared(t *testing.T) { TrunkBranch: "main", Environments: config.EnvNames("dev", "prod"), Deploys: []config.DeployConfig{{ - Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, Inputs: map[string]interface{}{"region": "us-east-1"}, }}, } @@ -113,6 +117,27 @@ func TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared(t *testing.T) { "sha must not be threaded to a callback that does not declare it") } +// TestGB2_PromoteMatrixDeploy_NoDuplicateShaWhenManifestInput proves that when +// the manifest already wires sha as an explicit deploy input, the with: block +// emits it exactly once (from the matrix-input loop) and the framework does not +// add a second sha key. +func TestGB2_PromoteMatrixDeploy_NoDuplicateShaWhenManifestInput(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1", "sha": "${{ matrix.sha }}"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-web") + assert.Equal(t, 1, strings.Count(block, "sha: ${{ matrix.sha }}"), + "sha must be emitted exactly once, not duplicated, when it is a manifest input") +} + // TestGM4_RollbackDispatch_DryRunTreatsBooleanAndString proves the rollback // deploy guard and finalize gate treat a JSON boolean true (client_payload) and // the string 'true' (workflow_dispatch) alike. Before the fix a bare "!= 'true'" diff --git a/internal/generate/promote.go b/internal/generate/promote.go index 14b9dceb..2db1913b 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -152,7 +152,19 @@ func (g *PromoteGenerator) Generate() (string, error) { // discoverDeployInputs parses deploy workflow files to discover their inputs func (g *PromoteGenerator) discoverDeployInputs() error { for _, d := range g.config.Deploys { - workflowPath := filepath.Join(g.baseDir, d.Workflow) + // Cross-repo callbacks reference a reusable workflow in another + // repository (org/repo/.github/workflows/file.yaml@ref); there is no + // local file to parse, and external deploys thread environment/sha + // directly rather than through declared-input detection. + if config.IsExternalWorkflow(d.Workflow) { + continue + } + // Resolve to the normalized on-disk location so a bare filename + // (deploy.yaml) resolves to .github/workflows/deploy.yaml, matching the + // emitted uses: reference and the orchestrate generator. Reading the raw + // d.Workflow silently missed a canonically-located file, which dropped + // declared-input detection (for example the sha a matrix deploy threads). + workflowPath := filepath.Join(g.baseDir, normalizeWorkflowPath(d.Workflow)) data, err := os.ReadFile(workflowPath) if err != nil { // Skip if workflow doesn't exist yet