From 9ce9c56cce11df4f31c56c7b330ac11c25f7d5ad Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Mon, 20 Jul 2026 09:29:44 -0400 Subject: [PATCH] fix(generate): gate retry shims and their dependents with a status function A generated retry shim gated its re-invocation with a bare boolean expression (if: needs..result == 'failure'). GitHub applies an implicit needs-success requirement to any job whose if has no status-check function, so the shim was skipped exactly when the previous attempt failed, which is the only time it should run. The rescue never fired on real GitHub; the live fleet caught it (base=failure, retry=skipped) while act, which evaluates if leniently, ran the shim and kept the e2e harness green. The same flaw hit a dependent of a retried deploy: its effective-result OR gate references the base and its shims, but with no status function the dependent was skipped the moment the base failed, before the OR could rescue it via a shim (#639 fixed the needs list, not the if gate). Emit the retry shim gate as ${{ !cancelled() && needs..result == 'failure' }} and prefix a default-policy dependent of a retried dependency with !cancelled(). !cancelled() (not always()) is used so a cancelled run does not force a re-deploy; the == 'failure' clause already excludes a cancelled predecessor, so the status function only governs whole-run cancellation. The wrap is required because a bare YAML scalar may not begin with the "!" tag indicator. The no-retries and non-retried-dependent paths are byte-identical. Add a generation-correctness assertion pinning the shim and dependent if gates (and locking the bare form out), extend the censused depends_on assertions (GM5/GM7) with the status-function check, update the retries e2e scenario, and codify the general rule in CONTRIBUTING. Signed-off-by: Joshua Temple --- CONTRIBUTING.md | 1 + e2e/scenarios/73-deploy-retries.yaml | 20 ++++- .../generate/correctness_assertions_test.go | 75 +++++++++++++++++++ internal/generate/generator.go | 39 +++++++++- .../generate/pass10_silent_output_test.go | 26 +++++-- 5 files changed, 150 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9977ef7..46dd9be 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -66,6 +66,7 @@ cascade holds to a few conventions in its own codebase and in the workflows it g - **A component-scoped runtime reads resolved component values, never root ones**: the generator emits each per-component workflow from `config.TrunkConfig.ResolveComponent` (its deploy job names and gates, tag grammar, environments, publish shape), so every component-scoped decision a CLI path makes must read the value resolved for that component, or the runtime and the workflow it is driving disagree. Swapping the resolved config in whole is how a path reaches that when it can hold one, and it is the default: `promote`, `rollback`, and `orchestrate` preflights and finalizes all do. A path that cannot hold a swapped config resolves per value instead, through named resolvers every sink reads (see the `hotfix` carve-out below); that satisfies the rule by the same standard, since the test is that no sink is left reading a root value, not which mechanism got it there. What is forbidden either way is resolving a component and then reading root values anyway. A per-field copy silently leaves every uncopied override reading the root value, and the workflow and the runtime then disagree about job names, gates, or grammar: the component promote path once carried over only `environments`, so the generated deploy gates checked component deploy names the runtime never emitted and every deploy skipped while the promotion recorded success. On a path that reads the tag grammar, the component grammar's `StrictPrefix` invariant rides the swap (a component parses its tags literally), matching `ResolvedComponent.TagGrammarSpec`; `ResolveComponent` does not set it on the resolved config, so the swapping path forces it. A path that never parses a tag, such as `rollback`, has no grammar to carry and must not fake one. `hotfix` is the path that cannot hold a swapped config: both its verbs resolve the component out of the working config a second time to reach the component's grammar, and `ResolveComponent` clears `Components` on the config it returns, so a swapped-in config could not be resolved from again and would fail with "component is not declared". It therefore resolves its ladder and its grammar through named resolvers (`resolveEnvLadder`, `resolveFinalizeSpec`) that every sink reads. Reaching for a resolver rather than a swap needs that kind of structural reason, stated at the resolver, never a preference; and a resolver only holds the line while every sink reads it, so a new component-scoped read is a new sink to route through it. - **Every value sent to a length-capped API field is bounded before it is sent**: a GitHub API field with a documented maximum (a release body at 125,000 characters, a pull request or issue comment body at 65,536) rejects an oversized value with a 422 rather than truncating it server-side, so a value composed from unbounded input must be capped at the call site, not assumed to fit. Unbounded means the input grows with the repository rather than with the manifest: a changelog, a commit range, a diff, a log. Size the input by its worst realistic case, not its typical one: a changelog looks small until `previous_tag` is empty, which makes it span the repository's entire history, and that is the normal state both after a state reset and for the first cascade release in a repository with existing history. The release body reached 125,000 characters on a real repository this way and failed finalize, which stranded the state write and cascaded into every downstream job. A cap truncates on a boundary that keeps the retained content meaningful (a whole line or entry, never mid-item), counts characters the way the API does (runes, never bytes, so a multi-byte character is never split into invalid UTF-8), reserves its own marker's length from the budget so the result lands under the cap rather than at it, and leaves a marker saying what was dropped and linking to where the full content lives. Truncation is never silent. A field bounded by construction (a semver tag, a fixed-format release name) needs no cap; say so rather than adding one. - **Large or unbounded content passes by file reference, never through an action input or environment variable**: a generated Actions step must not place content that grows with the repository (a changelog, a diff, a commit range, a log) on a composite-action input or an environment variable. GitHub exposes an action input as an environment variable, and `execve` caps a single environment variable near 128KB, so a large value on that path fails the step with `E2BIG` before the program runs. The built-in changelog hit this on a real repository: an accumulated changelog around 135KB killed the Manage Release step and cascaded into every downstream job. Pass such content by file reference instead: write it to a file (the runner temp dir is job-constant and shared across steps in the same job) and pass the path, which stays small regardless of content size. The `manage-release` composite action takes a `changelog_file` path for exactly this reason, and the generated workflows write the changelog to `$RUNNER_TEMP/cascade-changelog.md` and pass the path rather than the content. A value bounded by construction (a tag, a SHA, a version) is fine on an input; the rule is for content whose size the manifest does not bound. +- **A job that must run after a needed job may have failed needs a status function in its `if`**: GitHub Actions applies an implicit needs-success requirement to any job whose `if` condition contains no status-check function (`always()`, `!cancelled()`, `success()`, `failure()`, or `cancelled()`). A bare boolean gate like `needs..result == 'failure'` therefore does NOT lift that implicit gate: the moment a needed job fails, the dependent job is skipped before its own `if` is even evaluated. Any generated job that is meant to run precisely because a needed job did not succeed (a retry shim gated on the previous attempt's failure, a dependent whose gate judges a retried dependency's effective result across its shims) must carry a status function in its `if`, or it silently never runs on real GitHub. Prefer `!cancelled()` over `always()` for a job that re-invokes a deployment, so a cancelled run does not force a re-deploy; the `== 'failure'` clause already excludes a cancelled predecessor, so `!cancelled()` only governs whole-run cancellation. An `if` expression that begins with `!` must be wrapped in `${{ }}` (a bare YAML scalar may not start with a tag indicator). This class of defect emits valid YAML that actionlint accepts and that act evaluates leniently enough to run, so it passes unit tests and the e2e harness; only the live fleet exercises the real-GitHub skip. When you emit such a gate, pin its status function in a generation-correctness assertion and lock the bare form out with a `not_contains`. - **Callback isolation**: generated workflows call your workflows via `workflow_call`, and cascade never reaches into your callback logic. - **Metadata courier**: cascade passes artifact identifiers and versions between stages. It never touches your container registry, package registry, or the systems you deploy to directly. diff --git a/e2e/scenarios/73-deploy-retries.yaml b/e2e/scenarios/73-deploy-retries.yaml index ad5ee80..3fcbf32 100644 --- a/e2e/scenarios/73-deploy-retries.yaml +++ b/e2e/scenarios/73-deploy-retries.yaml @@ -156,8 +156,14 @@ steps: - "deploy-web-retry-2:" # Each shim is gated on the attempt immediately before it, so the # chain is sequential rather than every shim firing off the original. - - "if: needs.deploy-web.result == 'failure'" - - "if: needs.deploy-web-retry-1.result == 'failure'" + # The gate is wrapped in ${{ }} and carries the !cancelled() status + # function: GitHub applies an implicit needs-success requirement to a + # job whose if: has no status function, which would SKIP the shim + # exactly when the predecessor failed (the only time it should run). + # This is a text assertion; act evaluates if: leniently and cannot + # prove the real-GitHub skip, which is what the live fleet caught. + - "if: ${{ !cancelled() && needs.deploy-web.result == 'failure' }}" + - "if: ${{ !cancelled() && needs.deploy-web-retry-1.result == 'failure' }}" # A job result is immutable, so needs.deploy-web.result stays # 'failure' for the whole run even after a shim redeploys the # environment successfully. Both the finalize failure gate and the @@ -172,12 +178,22 @@ steps: # its if: gate reads below are actually resolvable references. - "needs: [setup, deploy-web, deploy-web-retry-1, deploy-web-retry-2]" - "(needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')" + # notify's own if: must ALSO carry a status function, or GitHub's + # implicit needs-success gate skips it the moment deploy-web fails, + # before the effective-result OR above can rescue it via a shim. + - "!cancelled()" not_contains: # retries: 2 emits exactly two shims. - "deploy-web-retry-3:" # The manifest gate must never bind to the immutable base result: that # is what denied a retry-rescued deploy in recorded state. - "WEB_RESULT: ${{ needs.deploy-web.result }}" + # Lock the fix: the shim gate must never regress to the bare boolean + # form (no status function), which real GitHub skips when the base + # failed. The wrapped ${{ !cancelled() && ... }} form is asserted + # above; these bare forms must be absent. + - "if: needs.deploy-web.result == 'failure'" + - "if: needs.deploy-web-retry-1.result == 'failure'" - name: "Orchestrate: the deploy callback genuinely fails at runtime" action: orchestrate diff --git a/internal/generate/correctness_assertions_test.go b/internal/generate/correctness_assertions_test.go index 854a550..255d6e1 100644 --- a/internal/generate/correctness_assertions_test.go +++ b/internal/generate/correctness_assertions_test.go @@ -490,3 +490,78 @@ func TestGenCorrectness_ReconcileCommit_AppendVsFollowup(t *testing.T) { "followup mode must not push in place onto the PR head branch") }) } + +// TestGenCorrectness_RetryShim_IfGateCarriesStatusFunction pins the fix for a +// real-GitHub defect the fleet caught: a retry shim (and a dependent of a +// retried callback) whose if: gate is a BARE boolean expression is skipped by +// GitHub's implicit needs-success gate exactly when it is needed. GitHub only +// lifts that implicit gate when the if: contains a status-check function +// (always(), !cancelled(), success(), failure(), cancelled()); a bare +// "needs.X.result == 'failure'" does not qualify, so when X fails the shim is +// skipped and the rescue never runs. +// +// A retry re-invokes a deployment, so a cancelled run must NOT trigger it: +// !cancelled() (not always()) is the correct function. The "== 'failure'" +// clause already excludes a cancelled predecessor (its result is 'cancelled', +// not 'failure'), so !cancelled() only adds the honor-the-cancel behavior for a +// run cancelled mid-flight, which is exactly what a re-deploy shim wants. +// +// The `retries` field is an int, so it is outside the string-field correctness +// census surface (emitted_fields_guard_test.go walks string-carrying fields +// only); that is precisely why this shim-if defect escaped the census. This +// assertion, plus the dependent coverage folded into GM5/GM7 (the censused +// deploys[].depends_on[] assertions), is the regression guard. The runtime +// rescue itself is the fleet's proof; this pins the emitted shape. +func TestGenCorrectness_RetryShim_IfGateCarriesStatusFunction(t *testing.T) { + dir := correctnessDir(t) + + t.Run("retry_shims_and_dependent_lift_the_needs_gate", func(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, Retries: 2}, + {Name: "api", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + + retry1 := pass10JobBlock(t, out, "deploy-web-retry-1") + assert.Contains(t, retry1, "if: ${{ !cancelled() && needs.deploy-web.result == 'failure' }}", + "retry-1's if: must carry a status function so GitHub does not skip it via the implicit needs-success gate when the base fails") + assert.NotContains(t, retry1, "if: needs.deploy-web.result == 'failure'", + "a bare boolean gate is skipped on real GitHub exactly when the base failed") + + retry2 := pass10JobBlock(t, out, "deploy-web-retry-2") + assert.Contains(t, retry2, "if: ${{ !cancelled() && needs.deploy-web-retry-1.result == 'failure' }}", + "every rung of the ladder, including retry-N chained off retry-(N-1), must lift the implicit gate") + assert.NotContains(t, retry2, "if: needs.deploy-web-retry-1.result == 'failure'", + "retry-2's bare boolean gate would be skipped when retry-1 failed") + + dependent := pass10JobBlock(t, out, "deploy-api") + assert.Contains(t, dependent, "!cancelled()", + "a dependent of a retried deploy must lift the implicit needs-success gate so its effective-result OR is evaluated after the base failed but a shim rescued it") + assert.Contains(t, dependent, "(needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')", + "the dependent must still judge the dependency's effective result across the whole ladder") + }) + + t.Run("dependent_of_non_retried_deploy_is_unchanged", func(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}}, + {Name: "api", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + + dependent := pass10JobBlock(t, out, "deploy-api") + assert.NotContains(t, dependent, "!cancelled()", + "a dependent of a NON-retried deploy has no immutable-result hazard, so it must not gain a status function (no churn)") + assert.Contains(t, dependent, "needs.deploy-web.result == 'success'", + "the no-retry dependent still gates on the bare dependency success") + }) +} diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 2885267..20c1555 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -1275,6 +1275,13 @@ func (g *Generator) writeStrategyBlock(sb *strings.Builder, m *config.MatrixConf func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, needs []string) { var conditions []string + // dependsOnRetriedJob records that a gated dependency declares retries, so its + // effective-result clause references retry shims. When it does, the if: must + // carry a status-check function: GitHub otherwise applies its implicit + // needs-success gate and skips this job the moment the base dependency fails, + // before the effective-result OR can rescue it via a shim. Only the default + // run_policy needs this here; the always/force branches already emit always(). + dependsOnRetriedJob := false 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. @@ -1307,6 +1314,9 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee // No condition needed for force default: conditions = append(conditions, effectiveDepSuccessGate(depJobID, depRetries)) + if depRetries > 0 { + dependsOnRetriedJob = true + } } } break @@ -1339,6 +1349,9 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee switch info.RunPolicy { case config.RunPolicyDefault, "": conditions = append(conditions, effectiveDepSuccessGate(depJobID, depInfo.Retries)) + if depInfo.Retries > 0 { + dependsOnRetriedJob = true + } case config.RunPolicyAlways: conditions = append(conditions, fmt.Sprintf("(%s || needs.%s.result == 'skipped')", effectiveSuccessCond(depJobID, depInfo.Retries), depJobID)) case config.RunPolicyForce: @@ -1360,6 +1373,14 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee return } sb.WriteString(" if: |\n always() &&\n") + } else if dependsOnRetriedJob { + // A default-policy job gating on a retried dependency must lift GitHub's + // implicit needs-success gate, or it is skipped the moment the base + // dependency fails, before its effective-result OR can rescue it. Use + // !cancelled() (consistent with the retry shim's own gate) so a cancelled + // run does not force the dependent to run. dependsOnRetriedJob is only set + // after a condition was appended, so len(conditions) > 0 holds here. + sb.WriteString(" if: |\n !cancelled() &&\n") } else if len(conditions) > 0 { sb.WriteString(" if: |\n") } @@ -1584,7 +1605,23 @@ func (g *Generator) writeRetryJob(sb *strings.Builder, info CallbackInfo, workfl fmt.Fprintf(sb, " %s:\n", retryJobName) fmt.Fprintf(sb, " name: %s - Retry %d\n", info.DisplayName, retryNum) fmt.Fprintf(sb, " needs: [setup, %s]\n", prevJobName) - fmt.Fprintf(sb, " if: needs.%s.result == 'failure'\n", prevJobName) + // The gate must carry a status-check function. GitHub applies an implicit + // needs-success requirement to any job whose if: has no status function, so a + // BARE "needs..result == 'failure'" is skipped exactly when the + // predecessor failed, which is the only time a retry shim should run. Lift + // that implicit gate with !cancelled() rather than always(): a retry + // re-invokes the callback, and a run cancelled mid-flight (superseded by a + // newer run, or cancelled by an operator) must not re-deploy. The + // "== 'failure'" clause already excludes a cancelled predecessor (its result + // is 'cancelled', not 'failure'), so !cancelled() only governs the whole-run + // cancellation case, where honoring the cancel is correct. + // + // The expression is wrapped in ${{ }} (the idiom used across promote.go / + // external.go) because a bare YAML scalar may not begin with "!": leading + // "!" is a YAML tag indicator and the "&&" trips the anchor parser, both of + // which actionlint and GitHub reject. The wrapper keeps the "!" inside the + // expression context where it is the boolean-negation operator. + fmt.Fprintf(sb, " if: ${{ !cancelled() && needs.%s.result == 'failure' }}\n", prevJobName) // timeout-minutes is forbidden on a reusable-workflow caller job // (jobs..uses): GitHub rejects the workflow at parse time. A retry shim // re-invokes the reusable workflow via uses:, so no timeout is emitted here. diff --git a/internal/generate/pass10_silent_output_test.go b/internal/generate/pass10_silent_output_test.go index 5ffd02f..948d5e2 100644 --- a/internal/generate/pass10_silent_output_test.go +++ b/internal/generate/pass10_silent_output_test.go @@ -205,6 +205,11 @@ func TestGM5_DependentDeploy_JudgesEffectiveResult(t *testing.T) { 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 if: must carry a status function (!cancelled()). Without it GitHub's + // implicit needs-success gate skips the dependent the moment the base deploy + // fails, before the effective-result OR can rescue it via a shim. + assert.Contains(t, block, "!cancelled()", + "a dependent of a retried deploy must lift the implicit needs-success gate so its effective-result OR is actually evaluated") // 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") @@ -235,14 +240,16 @@ func TestGM5_DependentDeploy_NoRetries_SingleClause(t *testing.T) { // needs: list carries every retry shim job ID its dependency declares, not just // the base dependency's job ID. // -// The if: gate (effectiveDepSuccessGate, proven by TestGM5 above) already -// references needs.deploy-web-retry-1 / -2 so a retry-rescued dependency does -// not skip its dependents. But GitHub Actions can only resolve a needs. -// reference for a job actually listed in that job's needs:; a reference to a -// job outside needs: is rejected by actionlint at parse and, if it somehow ran, -// would resolve to an ever-empty value at runtime. Before this fix needs: was -// built from GetDirectDependencies alone (the base job ID only), so the if: -// gate and the needs: list silently disagreed. +// The if: gate (effectiveDepSuccessGate, proven by TestGM5 above) references +// needs.deploy-web-retry-1 / -2 so a retry-rescued dependency does not skip its +// dependents. Two conditions must hold for that to work on real GitHub. First, +// GitHub Actions can only resolve a needs. reference for a job actually +// listed in that job's needs:; a reference to a job outside needs: is rejected +// by actionlint at parse and, if it somehow ran, would resolve to an ever-empty +// value at runtime (this test pins the needs: list). Second, the if: must carry +// a status-check function so GitHub does not apply its implicit needs-success +// gate; without it the dependent skips the moment the base deploy fails, before +// the effective-result OR can rescue it (pinned here via !cancelled()). func TestGM7_DependentDeploy_NeedsIncludesRetryShims(t *testing.T) { dir := pass10Fixture(t, pass10DeployWorkflow) cfg := &config.TrunkConfig{ @@ -260,6 +267,9 @@ func TestGM7_DependentDeploy_NeedsIncludesRetryShims(t *testing.T) { assert.Contains(t, block, "needs: [setup, deploy-web, deploy-web-retry-1, deploy-web-retry-2]", "needs: must list the base dependency plus every retry shim it declares, "+ "in ladder order, so the if: gate referencing those shims is well-formed") + assert.Contains(t, block, "!cancelled()", + "the if: gate must lift the implicit needs-success gate; a bare boolean gate skips the "+ + "dependent when the base deploy fails, defeating the retry-rescue the needs: list enables") } // TestGM7_DependentDeploy_NoRetries_NeedsByteIdentical proves the no-retries