fix(generate): judge a retries ladder on its effective result

CHANGELOG.md

@@ -16,6 +16,19 @@ A `Migration` section is added to any release that bumps `schema_version`.
 
 ### Fixed
 
+- **generate:** A callback declaring `retries` is now judged on its ladder's
+  effective result, so a deploy that fails and is then rescued by a retry no
+  longer fails the run or gets denied in recorded state. A GitHub Actions job
+  result is immutable: once the base job ended in `failure`, `needs.<job>.result`
+  reported `failure` for the rest of the run even after a retry shim redeployed
+  the environment successfully. The finalize failure gate, the manifest update,
+  the run summary, and the native Deployment status all read that frozen result,
+  so a successful retry still went red, and the manifest recorded the
+  environment's sha and version while refusing to record the deploy that had in
+  fact happened. Those four now ask whether any attempt in the base-plus-retry
+  ladder succeeded. A manifest that does not use `retries` emits byte-identical
+  output.
+
 - **release:** The stale-draft reaper now sees every release instead of only the
   30 most recent. The release list is paginated by the GitHub API, and the
   listing read a single response without requesting a page size or following the
@@ -204,6 +217,16 @@ A `Migration` section is added to any release that bumps `schema_version`.
 - **git:** Pass `--` before the tag pattern in prefix-scoped `git tag -l`
   lookups as defense in depth beneath the new prefix validation.
 
+### Documentation
+
+- **manifest, callbacks:** `run_policy`, `on_failure`, and `retries` are now
+  documented as applying to trunk runs only. The promote generator never emitted
+  any of the three, so a promotion ran each deploy once regardless of what the
+  manifest declared. A promote-side retry needs a per-environment design, because
+  a promote deploy declaring `inputs` compiles to a matrix job whose single
+  aggregate result would make a retry redeploy every environment including the
+  ones that already succeeded ([#626](https://github.com/stablekernel/cascade/issues/626)).
+
 ### Added
 
 - **test:** A durable emitted-field guard: a reflection walk over the

docs/src/content/docs/reference/callbacks.md

@@ -523,7 +523,9 @@ Callback failures are handled by the `on_failure` policy:
 | `abort` | Fail the entire workflow |
 | `continue` | Other callbacks proceed |
 
-With `retries: N`, failed callbacks retry up to N times before final failure.
+With `retries: N`, a failed callback is retried up to N times before it counts as a final failure. Each retry is a separate job that re-invokes the same reusable workflow, and the chain stops at the first attempt that succeeds. A callback that fails and is then rescued by a retry counts as a success: the run stays green and the deploy is recorded in state.
+
+`run_policy`, `on_failure`, and `retries` apply to trunk runs (`orchestrate.yaml`). They are not emitted into `promote.yaml`; see [Policy fields](/reference/manifest/#policy-fields).
 
 ## Migrating from inline `run:`/`shell:` callbacks
 

docs/src/content/docs/reference/manifest.md

@@ -345,7 +345,7 @@ ci:
 | `env_inputs` | emitted | map | {} | Per-environment input overrides. |
 | `run_policy` | emitted | string | `default` | Execution policy. See [Policy fields](#policy-fields). |
 | `on_failure` | emitted | string | `abort` | Failure handling. See [Policy fields](#policy-fields). |
-| `retries` | emitted | int | 0 | Retry attempts (0-3). |
+| `retries` | emitted | int | 0 | Retry attempts (0-3). Trunk runs only. See [Policy fields](#policy-fields). |
 
 ## builds
 
@@ -1047,6 +1047,14 @@ versioning](/cascade/reference/versioning/#per-component-versioning).
 
 `run_policy`, `on_failure`, and `retries` apply to `validate`, each `builds` entry, and each `deploys` entry.
 
+:::caution[Trunk runs only]
+These three fields shape `orchestrate.yaml`, the workflow that runs on a trunk merge. They are **not** emitted into `promote.yaml`, so a promotion runs each deploy once, with no retry and no `on_failure` handling.
+
+`retries` is deliberately not emitted into `promote.yaml`. A promote deploy that declares `inputs` compiles to a matrix job fanned across environments, and a matrix job reports one aggregate result: `failure` if any environment failed. A retry gated on that aggregate would re-invoke the callback for **every** environment, redeploying the ones that already succeeded, and GitHub Actions offers no way for a calling workflow to re-run only the failed legs of a dependency's matrix. Retrying a healthy production environment because an unrelated environment failed is worse than not retrying, so a promote-side retry needs a design that retries per environment. Track it in [issue #626](https://github.com/stablekernel/cascade/issues/626).
+
+To make a promotion resilient to a transient failure, handle the retry inside the reusable workflow the deploy points at, where it can be scoped to the one environment that failed.
+:::
+
 | `run_policy` | Behavior |
 |--------------|----------|
 | `default` | Skip if any dependency was skipped. |

e2e/scenarios/73-deploy-retries.yaml

@@ -29,6 +29,30 @@ description: |
   occurrences. Asserting a per-shim conclusion here would silently read the wrong
   job, so it is left out rather than faked.
 
+  WHY base-fail-then-retry-SUCCEED IS NOT EXERCISED HERE, for the next author:
+  the case that matters most (the base attempt fails, a shim re-runs and
+  SUCCEEDS, so the environment really is deployed) cannot be expressed by this
+  harness, and the gap is structural rather than an omission:
+
+    1. Each shim is a separate GitHub Actions job, so it gets its own container
+       and its own workspace. A marker file written by the first attempt is gone
+       by the second, so the callback cannot count its own invocations locally.
+    2. Artifacts and the cache are the canonical cross-job state channels, and
+       both are unavailable: act is launched without an artifact server path
+       (harness/act.go), so upload-artifact/download-artifact cannot round-trip.
+    3. The callback body cannot self-discriminate which attempt it is either. The
+       generator emits identical with: inputs for the base job and every shim, and
+       inside a reusable workflow github.job resolves to the inner job id, which
+       is the same string for all three invocations.
+
+  A callback that fails once and then succeeds therefore has no way to keep state
+  across attempts here. Rather than fabricate a scenario that cannot prove the
+  claim, the runtime half is left unexercised and the effective-result gates are
+  pinned as emitted text above, with the four ladder shapes (no shims, base
+  succeeds, a middle shim succeeds, all attempts fail) covered as unit tests in
+  internal/generate/effective_result_test.go. Restoring the executing proof needs
+  an artifact server in the harness, not a change to this scenario.
+
   INNER-JOB-ID NAMESPACE TRAP, for the next author: expect.jobs entries are
   resolved by findJob (assert.go), which strips a "build-"/"deploy-" prefix and
   looks the remainder up among the INNER job ids of the called workflows. The
@@ -95,9 +119,21 @@ steps:
             # 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'"
+            # 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
+            # manifest-update gate must therefore judge the ladder's effective
+            # result ("did any attempt succeed?") rather than the base job's
+            # frozen one. The runtime half of this cannot be exercised here (see
+            # the note below), so the expressions are pinned as emitted text.
+            - "WEB_RESULT: ${{ (needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success') && 'success' || 'failure' }}"
+            - "!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')"
           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 }}"
 
   - name: "Orchestrate: the deploy callback genuinely fails at runtime"
     action: orchestrate

internal/generate/effective_result_test.go

@@ -0,0 +1,213 @@
+package generate
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stablekernel/cascade/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// A GitHub Actions job result is immutable: once deploy-web ends in 'failure',
+// needs.deploy-web.result reports 'failure' for the rest of the run even when a
+// retry shim re-invokes the same workflow and succeeds. Any gate that reads the
+// base job's result alone therefore contradicts what actually happened on the
+// environment. These tests pin the effective result, meaning "did any attempt in
+// the base-plus-shim ladder succeed?", across the four shapes a ladder can take.
+
+// retriesFixture writes a deploy callback workflow and returns a config whose
+// single deploy declares the requested retry count.
+func retriesFixture(t *testing.T, retries int) (*config.TrunkConfig, string) {
+	t.Helper()
+
+	tmpDir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".github/workflows"), 0o755))
+
+	deployWorkflow := `
+name: Deploy Web
+on:
+  workflow_call:
+    inputs:
+      environment:
+        type: string
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(tmpDir, ".github/workflows/deploy.yaml"),
+		[]byte(deployWorkflow), 0o644))
+
+	cfg := &config.TrunkConfig{
+		TrunkBranch:  "main",
+		Environments: config.EnvNames("dev"),
+		Deploys: []config.DeployConfig{
+			{
+				Name:     "web",
+				Workflow: ".github/workflows/deploy.yaml",
+				Triggers: []string{"src/**"},
+				Retries:  retries,
+			},
+		},
+	}
+	return cfg, tmpDir
+}
+
+// TestEffectiveResult_FailureGate_ToleratesSucceedingRetry is the core F1
+// regression. With retries: 2, a run where deploy-web fails and
+// deploy-web-retry-1 succeeds has genuinely deployed the environment, so the
+// finalize failure gate must not fire. Before the fix the gate read only
+// needs.deploy-web.result, which is frozen at 'failure', and the run went red
+// even though the retry succeeded.
+func TestEffectiveResult_FailureGate_ToleratesSucceedingRetry(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 2)
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	// The failure gate must consult the retry shims, not the base job alone.
+	assert.Contains(t, result,
+		"!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')",
+		"failure gate must exonerate a ladder in which some attempt succeeded")
+}
+
+// TestEffectiveResult_FailureGate_LadderClauseIsParenthesized guards the gate's
+// grouping. Callbacks are joined with " || ", so an unparenthesized ladder
+// clause would read "A || B && !C" and rely on GitHub Actions binding && more
+// tightly than ||. That happens to be true, but a gate deciding whether a run
+// goes red must not rest on implicit precedence: the clause is grouped as a unit.
+func TestEffectiveResult_FailureGate_LadderClauseIsParenthesized(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 2)
+	// A second, retry-free callback forces the " || " join that makes grouping matter.
+	cfg.Builds = []config.BuildConfig{{
+		Name:     "app",
+		Workflow: ".github/workflows/deploy.yaml",
+		Triggers: []string{"src/**"},
+	}}
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	assert.Contains(t, result,
+		"(contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result) && "+
+			"!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success'))",
+		"the ladder clause must be parenthesized as a unit, not left to operator precedence")
+}
+
+// TestEffectiveResult_ManifestGate_RecordsSucceedingRetry is the second half of
+// F1. The manifest update gates the deploys.web.* yq edits on WEB_RESULT, which
+// was bound to the immutable base result. A retry that deployed the environment
+// was therefore denied in recorded state.
+func TestEffectiveResult_ManifestGate_RecordsSucceedingRetry(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 2)
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	assert.NotContains(t, result, "WEB_RESULT: ${{ needs.deploy-web.result }}",
+		"WEB_RESULT must not bind to the immutable base result when retries are declared")
+	assert.Contains(t, result,
+		"WEB_RESULT: ${{ (needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success') && 'success' || 'failure' }}",
+		"WEB_RESULT must reflect whether any attempt in the ladder succeeded")
+}
+
+// TestEffectiveResult_ZeroRetries_IsUnchanged pins the frozen-schema promise:
+// a manifest that does not use retries must emit exactly what it emits today.
+// The effective-result expression must collapse to the bare base result when
+// there are no shims to consult.
+func TestEffectiveResult_ZeroRetries_IsUnchanged(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 0)
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	assert.Contains(t, result, "WEB_RESULT: ${{ needs.deploy-web.result }}",
+		"a retry-free deploy must keep the bare base result")
+	assert.Contains(t, result,
+		"contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result)",
+		"a retry-free deploy must keep today's failure gate verbatim")
+	// Scoped to the ladder: the state-push shell legitimately says "retrying".
+	assert.NotContains(t, result, "deploy-web-retry-", "a retry-free deploy must emit no ladder")
+}
+
+// TestEffectiveResult_SkippedBaseIsNotAFailure guards the crux of the design.
+// A shim whose predecessor did not fail is 'skipped', NOT 'failure', and a base
+// job whose triggers did not match is also 'skipped'. Expressing effective
+// failure as "not success" would turn both of those into spurious run failures.
+// The gate must stay anchored on the base job's failure/cancelled states and
+// only then ask whether a retry rescued it.
+func TestEffectiveResult_SkippedBaseIsNotAFailure(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 2)
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	// The gate keeps the failure/cancelled anchor, so a skipped base (triggers
+	// did not match) never trips it, and it never degrades to a bare !success.
+	assert.Contains(t, result,
+		"contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result) && "+
+			"!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')",
+		"effective failure must anchor on the base failing, then ask if a retry rescued it")
+	assert.NotContains(t, result, "!(needs.deploy-web.result == 'success')",
+		"a bare !success gate would turn a skipped base into a spurious failure")
+}
+
+// TestEffectiveResult_AllAttemptsFail_StillFails is the negative control: the
+// fix must not make a genuinely failed ladder look green. When the base and
+// every shim fail, the gate must still fire and the manifest must still refuse
+// to record the deploy.
+func TestEffectiveResult_AllAttemptsFail_StillFails(t *testing.T) {
+	cfg, tmpDir := retriesFixture(t, 2)
+
+	result, err := NewGenerator(cfg, tmpDir).Generate()
+	require.NoError(t, err)
+
+	// The gate is still present and still exits non-zero.
+	assert.Contains(t, result, "- name: Check for Failures")
+	assert.Contains(t, result, "exit 1")
+	// The manifest write is still conditional rather than unconditional.
+	assert.Contains(t, result, `if [[ "$WEB_RESULT" == "success" ]]; then`)
+}
+
+// TestEffectiveResult_Promote_DoesNotEmitRetryLadder pins the scope decision for
+// F2, so that a later author who adds a promote ladder must confront the reason
+// one is not there today rather than discovering it in production.
+//
+// retries is an orchestrate-only capability. A promote deploy that declares
+// inputs compiles to a matrix job fanned across environments with
+// fail-fast: false (see writeDeployJobs), and a matrix job exposes a single
+// aggregate result: 'failure' if ANY leg failed. A caller-side shim gated on
+// that aggregate re-invokes the reusable workflow for EVERY leg, redeploying the
+// environments that already succeeded. GitHub Actions offers no caller-side way
+// to re-run only the failed legs of a dependency's matrix, so a ladder here
+// would redeploy healthy production environments on account of an unrelated
+// environment's transient failure. That is worse than no retry.
+//
+// Emitting a ladder only on the non-matrix path was rejected too: it would make
+// retries silently work or not work depending on whether the deploy happened to
+// declare inputs. The documented claim is scoped to orchestrate instead.
+func TestEffectiveResult_Promote_DoesNotEmitRetryLadder(t *testing.T) {
+	for _, retries := range []int{0, 2} {
+		cfg, tmpDir := retriesFixture(t, retries)
+
+		result, err := NewPromoteGenerator(cfg, tmpDir).Generate()
+		require.NoError(t, err)
+
+		assert.NotContains(t, result, "deploy-web-retry-",
+			"promote emits no retry ladder at retries=%d; see this test's rationale", retries)
+	}
+}
+
+// TestEffectiveResult_Promote_RetriesDoNotPerturbOutput proves retries is inert
+// on the promote path rather than partially wired: a manifest that declares
+// retries must emit byte-identical promote output to one that does not.
+func TestEffectiveResult_Promote_RetriesDoNotPerturbOutput(t *testing.T) {
+	withRetries, dirA := retriesFixture(t, 2)
+	without, dirB := retriesFixture(t, 0)
+
+	a, err := NewPromoteGenerator(withRetries, dirA).Generate()
+	require.NoError(t, err)
+	b, err := NewPromoteGenerator(without, dirB).Generate()
+	require.NoError(t, err)
+
+	assert.Equal(t, b, a, "retries must not perturb promote output while it is orchestrate-only")
+}