fix(fleet): preserve cli_version_sha across state writes and restore the sha-pin repin

.github/workflows/fleet-e2e.yaml

@@ -324,6 +324,23 @@ jobs:
           RC_BARE="${RC_VERSION#v}"
           echo "RC_BARE=$RC_BARE" >> "$GITHUB_ENV"
 
+          # Peel the rc tag to its commit SHA so the repin can SHA-pin each
+          # example repo's setup-cli self-action (cli_version_sha). cascade's
+          # release tags are annotated, so refs/tags/<rc> is a tag-object SHA,
+          # not a commit; ^{} dereferences to the underlying commit. A
+          # lightweight tag has no peeled ref, so fall back to the bare ref,
+          # which is already the commit.
+          RC_SHA=$(git ls-remote "https://github.com/${REPO}" "refs/tags/${RC_VERSION}^{}" | awk '{print $1}')
+          if [ -z "$RC_SHA" ]; then
+            RC_SHA=$(git ls-remote "https://github.com/${REPO}" "refs/tags/${RC_VERSION}" | awk '{print $1}')
+          fi
+          if ! printf '%s' "$RC_SHA" | grep -qE '^[0-9a-f]{40}$'; then
+            echo "::error::Could not resolve a commit SHA for tag ${RC_VERSION} (got '${RC_SHA}')"
+            exit 1
+          fi
+          echo "Resolved ${RC_VERSION} to commit ${RC_SHA}"
+          echo "RC_SHA=$RC_SHA" >> "$GITHUB_ENV"
+
           TMPDIR=$(mktemp -d)
           echo "Downloading $RC_VERSION linux/amd64 archive from $REPO"
           gh release download "$RC_VERSION" \
@@ -370,14 +387,15 @@ jobs:
             # 1. Point the manifest cli_version at the rc.
             sed -i -E "s|^([[:space:]]*cli_version:[[:space:]]*).*$|\1${RC_VERSION}|" "$manifest"
 
-            # 1b. Keep the example repos tag-pinned for the fleet: drop any
-            #     cli_version_sha so a pin_mode: sha repo regenerates with the
-            #     setup-cli@<cli_version> tag fallback, leaving manifest and
-            #     workflows consistent (no drift). SHA-pinning stays a generator
-            #     capability exercised by unit tests and cascade's own repo. The
-            #     routine state-write dropping cli_version_sha for pin_mode: sha
-            #     repos is tracked separately.
-            sed -i -E '/^[[:space:]]*cli_version_sha:[[:space:]]*/d' "$manifest"
+            # 1b. Pair cli_version_sha with the rc tag's peeled commit so the
+            #     regenerated setup-cli self-action ref is SHA-pinned (under
+            #     pin_mode: sha). Update it in place when present, else insert a
+            #     sibling line right after cli_version preserving its indent.
+            if grep -qE "^[[:space:]]*cli_version_sha:" "$manifest"; then
+              sed -i -E "s|^([[:space:]]*cli_version_sha:[[:space:]]*).*$|\1${RC_SHA}|" "$manifest"
+            else
+              sed -i -E "s|^([[:space:]]*)cli_version:([[:space:]]*).*$|&\n\1cli_version_sha:\2${RC_SHA}|" "$manifest"
+            fi
 
             # 2. Replace any other in-repo prerelease refs (rc OR dryrun, e.g. an
             #    explicit [email protected].. or a stale @v..-dryrun.. pin a suite

e2e/harness/multistep.go

@@ -266,6 +266,19 @@ type StepExpect struct {
 	Branches *BranchesExpect `yaml:"branches,omitempty"`
 	// PRs asserts open pull requests in Gitea (live check).
 	PRs *PRsExpect `yaml:"prs,omitempty"`
+	// Manifest asserts substrings present or absent in the live manifest after a
+	// step. It reads .github/manifest.yaml from Gitea, so it sees exactly what a
+	// state-writing step (orchestrate, promote) committed. A scenario uses it to
+	// assert a config field survives a routine state write rather than being
+	// dropped on finalize.
+	Manifest *ManifestExpect `yaml:"manifest,omitempty"`
+}
+
+// ManifestExpect asserts substrings against the live manifest read from Gitea.
+// Contains entries must each appear; NotContains entries must each be absent.
+type ManifestExpect struct {
+	Contains    []string `yaml:"contains,omitempty"`
+	NotContains []string `yaml:"not_contains,omitempty"`
 }
 
 // BranchesExpect asserts branch existence in Gitea. Exist entries must be

e2e/harness/runner.go

@@ -1108,9 +1108,42 @@ func (r *Runner) assertStep(ctx context.Context, step *Step, preState *Execution
 		allErrs = append(allErrs, errs...)
 	}
 
+	// Assert substrings in the live manifest (verifies a state write preserved
+	// config fields it does not itself touch).
+	if expect.Manifest != nil {
+		errs := r.assertManifest(ctx, expect.Manifest)
+		allErrs = append(allErrs, errs...)
+	}
+
 	return allErrs
 }
 
+// assertManifest reads the live manifest from Gitea and checks its content
+// against the expectation. Returns nil in unit-test mode (no harness). The read
+// sees exactly what the last state-writing step committed, so a Contains entry
+// that names a config field proves the field survived the write.
+func (r *Runner) assertManifest(ctx context.Context, expect *ManifestExpect) []error {
+	if r.harness == nil || r.harness.gitea == nil || r.harness.repo == nil {
+		return nil
+	}
+	content, err := r.harness.gitea.GetFileContent(ctx, r.harness.repo, ".github/manifest.yaml")
+	if err != nil {
+		return []error{fmt.Errorf("read manifest: %w", err)}
+	}
+	var errs []error
+	for _, want := range expect.Contains {
+		if !strings.Contains(content, want) {
+			errs = append(errs, fmt.Errorf("manifest expected to contain %q but did not:\n%s", want, content))
+		}
+	}
+	for _, unwant := range expect.NotContains {
+		if strings.Contains(content, unwant) {
+			errs = append(errs, fmt.Errorf("manifest expected NOT to contain %q but did:\n%s", unwant, content))
+		}
+	}
+	return errs
+}
+
 // assertBranches checks branch existence in Gitea against the expectation.
 // Returns nil in unit-test mode (no harness).
 func (r *Runner) assertBranches(ctx context.Context, expect *BranchesExpect) []error {

e2e/harness/scenario.go

@@ -127,6 +127,16 @@ type Config struct {
 	// manifest so a scenario can assert an overridden uses: ref is honored
 	// regardless of pin mode.
 	ActionPins map[string]string `yaml:"action_pins,omitempty"`
+	// CLIVersion carries the cli_version field through to the generated manifest
+	// so a scenario can fix the setup-cli self-action ref (and, under pin_mode:
+	// sha, the version comment that trails the pinned SHA).
+	CLIVersion string `yaml:"cli_version,omitempty"`
+	// CLIVersionSHA carries the 40-hex commit SHA that cli_version resolves to
+	// through to the generated manifest. Paired with pin_mode: sha it pins every
+	// generated setup-cli self-action ref to an immutable commit, so a scenario
+	// can assert the field survives a routine state write rather than being
+	// dropped on finalize.
+	CLIVersionSHA string `yaml:"cli_version_sha,omitempty"`
 }
 
 // PublishConfig defines a publish callback invoked after a release is published

e2e/scenarios/41-sha-pin-state-write-preserves-cli-version-sha.yaml

@@ -0,0 +1,59 @@
+name: "SHA pin: cli_version_sha survives a routine state write"
+description: |
+  Guards against a routine state write dropping cli_version_sha from a
+  pin_mode: sha manifest (#390). An earlier binary re-serialized the whole
+  manifest through its typed config struct on finalize, discarding any field
+  the running binary did not model, so the next state commit silently erased
+  the pin and the repo read as permanent drift. The current binary writes
+  state with YAML-node surgery that touches only the state subtree, leaving
+  every sibling config field byte-for-byte intact.
+
+  A pin_mode: sha manifest carries a populated cli_version_sha, so every
+  generated setup-cli self-action ref pins to that immutable commit. After a
+  routine orchestrate run finalizes fresh dev state, the live manifest must
+  still carry both cli_version_sha and pin_mode, and regenerating the
+  workflows from the written manifest must reproduce the same sha-pinned
+  output with no drift (idempotent regeneration).
+
+config:
+  trunk_branch: main
+  environments: [dev]
+  cli_version: v0.6.0
+  cli_version_sha: 9dc69a1f66753a3865c38c34eca5a931f677c803
+  pin_mode: sha
+  builds:
+    - name: app
+      workflow: build.yaml
+      triggers: ["src/**"]
+  deploys: []
+
+steps:
+  - name: "Initial commit; manifest carries the sha pin"
+    action: commit
+    commit:
+      message: "feat: add app"
+      files:
+        src/app.go: |
+          package main
+
+          func main() {}
+
+  - name: "Routine dev state write must preserve cli_version_sha"
+    action: orchestrate
+    expect:
+      state:
+        dev:
+          sha: commit1
+          version: "v0.1.0-rc.0"
+      jobs:
+        build-app: success
+      manifest:
+        contains:
+          - "cli_version_sha: 9dc69a1f66753a3865c38c34eca5a931f677c803"
+          - "pin_mode: sha"
+
+  - name: "Regenerate from the written manifest is drift-free and idempotent"
+    action: verify
+    verify:
+      regenerate: true
+      expect_exit: 0

internal/orchestrate/state_preserve_toplevel_test.go

@@ -0,0 +1,63 @@
+package orchestrate
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stablekernel/cascade/internal/config"
+)
+
+// TestWriteConfig_TopLevelCliVersionSHA_Preserved guards the node-level state
+// write against dropping cli_version_sha regardless of where the field sits in
+// the manifest tree.
+//
+// The existing TestWriteConfig_PreservesFullManifest covers the modeled
+// placement under ci.config. This case pins the other arrangement: a
+// cli_version_sha scalar at the top level of the document, a sibling of the
+// manifest-key section rather than a child of it. The state write replaces only
+// the state and latest_release keys inside the manifest-key section, so every
+// top-level sibling, including a top-level cli_version_sha, must survive the
+// round-trip byte-for-byte.
+func TestWriteConfig_TopLevelCliVersionSHA_Preserved(t *testing.T) {
+	const sha = "9dc69a1f66753a3865c38c34eca5a931f677c803"
+	orig := `schema_version: 1
+pin_mode: sha
+cli_version_sha: ` + sha + `
+manifest_file: .github/manifest.yaml
+ci:
+  config:
+    trunk_branch: main
+    environments:
+      - dev
+  state:
+    dev:
+      sha: oldsha
+`
+
+	out, err := config.WriteManifestState(
+		[]byte(orig),
+		"ci",
+		map[string]*config.EnvState{"dev": {SHA: "newsha"}},
+		nil,
+	)
+	if err != nil {
+		t.Fatalf("WriteManifestState: %v", err)
+	}
+	got := string(out)
+
+	// State must be updated.
+	if !strings.Contains(got, "newsha") {
+		t.Errorf("state not updated; missing newsha:\n%s", got)
+	}
+	// Top-level cli_version_sha sibling must survive untouched.
+	if !strings.Contains(got, "cli_version_sha: "+sha) {
+		t.Errorf("top-level cli_version_sha dropped:\n%s", got)
+	}
+	// Other top-level siblings must survive too.
+	if !strings.Contains(got, "pin_mode: sha") {
+		t.Errorf("top-level pin_mode dropped:\n%s", got)
+	}
+	if !strings.Contains(got, "schema_version: 1") {
+		t.Errorf("top-level schema_version dropped:\n%s", got)
+	}
+}

internal/orchestrate/typed_remarshal_drop_test.go

@@ -0,0 +1,85 @@
+package orchestrate
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stablekernel/cascade/internal/config"
+	"gopkg.in/yaml.v3"
+)
+
+// remarshalManifest is a pin_mode:sha manifest whose config carries both a
+// modeled SHA-pin field (cli_version_sha) and a key the running binary does not
+// model (future_field). future_field stands in for the older-binary case: any
+// cascade build cut before a config field was added sees that field as an
+// unmodeled key, exactly as builds predating cli_version_sha saw it.
+const remarshalManifest = `ci:
+  config:
+    trunk_branch: main
+    environments:
+      - dev
+    cli_version: v0.6.0
+    pin_mode: sha
+    cli_version_sha: abc123deadbeefabc123deadbeefabc123deadbe
+    future_field: keep-me
+  state:
+    dev:
+      version: v0.1.0-rc.0
+`
+
+// TestTypedRemarshal_UnmodeledConfig_IsDropped documents the lossy state-write
+// mechanism behind the SHA-pin drift (#372/#390). The original finalize write
+// re-serialized the whole manifest through the typed CICDFile struct. That
+// round-trip omits any key the running binary does not model and discards the
+// top-level manifest-key wrapper, so a pin_mode:sha repo loses its pin on the
+// next state commit and reads as permanent drift. This test pins the failing
+// behavior so the contrast with the WriteManifestState fix is explicit.
+func TestTypedRemarshal_UnmodeledConfig_IsDropped(t *testing.T) {
+	file, err := config.ParseManifestBytes([]byte(remarshalManifest), "ci")
+	if err != nil {
+		t.Fatalf("ParseManifestBytes: %v", err)
+	}
+
+	out, err := yaml.Marshal(file)
+	if err != nil {
+		t.Fatalf("Marshal: %v", err)
+	}
+	got := string(out)
+
+	if strings.Contains(got, "future_field") {
+		t.Errorf("typed remarshal was expected to drop the unmodeled future_field, but it survived:\n%s", got)
+	}
+	if strings.Contains(got, "ci:") {
+		t.Errorf("typed remarshal was expected to drop the 'ci:' wrapper, but it survived:\n%s", got)
+	}
+}
+
+// TestWriteManifestState_UnmodeledConfig_IsPreserved is the fix half: routing the
+// same state write through config.WriteManifestState performs YAML-node surgery,
+// touching only the state subtree. Unmodeled config (future_field), the modeled
+// SHA-pin field (cli_version_sha), and the manifest-key wrapper all survive
+// verbatim, while the state update is still applied.
+func TestWriteManifestState_UnmodeledConfig_IsPreserved(t *testing.T) {
+	state := map[string]*config.EnvState{
+		"dev": {SHA: "newsha", Version: "v0.2.0-rc.0"},
+	}
+
+	out, err := config.WriteManifestState([]byte(remarshalManifest), "ci", state, nil)
+	if err != nil {
+		t.Fatalf("WriteManifestState: %v", err)
+	}
+	got := string(out)
+
+	if !strings.Contains(got, "future_field: keep-me") {
+		t.Errorf("WriteManifestState dropped the unmodeled future_field:\n%s", got)
+	}
+	if !strings.Contains(got, "cli_version_sha: abc123deadbeefabc123deadbeefabc123deadbe") {
+		t.Errorf("WriteManifestState dropped cli_version_sha:\n%s", got)
+	}
+	if !strings.Contains(got, "ci:") {
+		t.Errorf("WriteManifestState dropped the 'ci:' wrapper:\n%s", got)
+	}
+	if !strings.Contains(got, "newsha") {
+		t.Errorf("WriteManifestState did not apply the state update:\n%s", got)
+	}
+}