Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.<job>.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.

Expand Down
20 changes: 18 additions & 2 deletions e2e/scenarios/73-deploy-retries.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
75 changes: 75 additions & 0 deletions internal/generate/correctness_assertions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
})
}
39 changes: 38 additions & 1 deletion internal/generate/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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")
}
Expand Down Expand Up @@ -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.<prev>.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.<id>.uses): GitHub rejects the workflow at parse time. A retry shim
// re-invokes the reusable workflow via uses:, so no timeout is emitted here.
Expand Down
Loading