From c4011f54ab18c7c5da2853d9edc103458df3cb33 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 27 Jun 2026 00:41:32 -0400 Subject: [PATCH] fix(statewrite): route orchestrate and external state writes through WriteManifestState orchestrate writeConfig and external writeManifest re-marshaled the typed CICDFile, dropping any manifest config the running binary does not model (and writeConfig dropped the ci: wrapper). This is the last lossy state-write class after #350/#371: it strips cli_version_sha (added in #384) on the next state commit for a binary that predates it, leaving a pin_mode:sha repo with SHA-pinned workflows but a manifest that no longer declares the pin, which reads as permanent drift and would re-open the setup-cli code-scanning alerts. Route both writers through config.WriteManifestState so only state and latest_release change and all other config, modeled or not, survives verbatim. Closes #372. Signed-off-by: Joshua Temple --- internal/external/command.go | 23 ++--- internal/external/command_test.go | 47 +++++++++++ internal/orchestrate/orchestrator.go | 21 ++++- internal/orchestrate/state_preserve_test.go | 93 +++++++++++++++++++++ 4 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 internal/orchestrate/state_preserve_test.go diff --git a/internal/external/command.go b/internal/external/command.go index 9ca1ae3..f83bef4 100644 --- a/internal/external/command.go +++ b/internal/external/command.go @@ -10,7 +10,6 @@ import ( "time" "github.com/spf13/cobra" - "gopkg.in/yaml.v3" "github.com/stablekernel/cascade/internal/config" "github.com/stablekernel/cascade/internal/git" @@ -280,24 +279,20 @@ func commitWithApplicationRetry(filePath, commitMsg string, maxAttempts int, app return fmt.Errorf("git push failed after %d attempts: %s: %w", maxAttempts, strings.TrimSpace(string(lastPushOut)), lastPushErr) } -// writeManifest writes the CICD file back to the manifest under the specified key. +// writeManifest writes the updated external deploy state back to the manifest +// under the specified key. It rewrites only the mutable state subtree, so every +// other manifest key, including config fields this binary does not model (for +// example cli_version_sha on a SHA-pinned repo running an older cascade) and the +// internal runtime fields, is preserved verbatim rather than dropped on the +// round-trip. The external update mutates only state, so a full re-marshal of +// the typed config is never needed here. func writeManifest(path, key string, file *config.CICDFile) error { - // Read existing manifest to preserve other keys - data, err := os.ReadFile(path) + current, err := os.ReadFile(path) if err != nil { return fmt.Errorf("reading manifest: %w", err) } - var manifest map[string]any - if err := yaml.Unmarshal(data, &manifest); err != nil { - return fmt.Errorf("parsing manifest: %w", err) - } - - // Update the CI key - manifest[key] = file - - // Write back - out, err := yaml.Marshal(manifest) + out, err := config.WriteManifestState(current, key, file.State, file.LatestRelease) if err != nil { return fmt.Errorf("marshaling manifest: %w", err) } diff --git a/internal/external/command_test.go b/internal/external/command_test.go index 504938a..d89ea65 100644 --- a/internal/external/command_test.go +++ b/internal/external/command_test.go @@ -102,6 +102,53 @@ other_key: preserved assert.Contains(t, content, "preserved") } +// TestWriteManifest_PreservesFullConfig guards the external state write against +// dropping manifest configuration on the round-trip (issue #372). The write must +// touch only the mutable state subtree: modeled config fields (cli_version_sha), +// config keys this binary does not model, and the internal runtime fields +// (manifest_file/manifest_key) must all be left untouched in the committed file. +func TestWriteManifest_PreservesFullConfig(t *testing.T) { + tmpDir := t.TempDir() + manifestPath := filepath.Join(tmpDir, "manifest.yaml") + + const sha = "5702d41a1234567890abcdef1234567890abcdef" + initial := `ci: + config: + trunk_branch: main + environments: [dev] + cli_version: v0.6.0 + pin_mode: sha + cli_version_sha: ` + sha + ` + future_field: keep-me + state: + dev: + sha: abc123 +` + require.NoError(t, os.WriteFile(manifestPath, []byte(initial), 0644)) + + // Parse the on-disk manifest, mutate only state, and write it back through + // the same path the external update command uses. + cicdFile, err := config.ParseManifestFile(manifestPath, "ci") + require.NoError(t, err) + cicdFile.State["dev"].External = map[string]*config.ExternalDeployState{ + "cdk": {Repo: "org/cdk-infra", SHA: "cdk123"}, + } + + require.NoError(t, writeManifest(manifestPath, "ci", cicdFile)) + + data, err := os.ReadFile(manifestPath) + require.NoError(t, err) + content := string(data) + + assert.Contains(t, content, "cdk123", "state not updated") + assert.Contains(t, content, "cli_version_sha: "+sha, "cli_version_sha dropped") + assert.Contains(t, content, "cli_version: v0.6.0", "cli_version dropped") + assert.Contains(t, content, "pin_mode: sha", "pin_mode dropped") + assert.Contains(t, content, "future_field: keep-me", "unmodeled config field dropped") + assert.NotContains(t, content, "manifest_file:", "internal runtime field leaked") + assert.NotContains(t, content, "manifest_key:", "internal runtime field leaked") +} + func TestUpdateCommand_RequiredFlags(t *testing.T) { cmd := NewCommand() diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index c77fcec..daaedb1 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -8,8 +8,6 @@ import ( "strings" "time" - "gopkg.in/yaml.v3" - "github.com/stablekernel/cascade/internal/changelog" "github.com/stablekernel/cascade/internal/config" "github.com/stablekernel/cascade/internal/git" @@ -462,9 +460,24 @@ func (o *Orchestrator) calculateChangelogRefs() (string, string) { return initialCommit, "" } -// writeConfig writes the updated cicd.yaml file. +// writeConfig writes the updated manifest back to disk. It rewrites only the +// mutable state subtree of the on-disk manifest, so the top-level manifest key +// wrapper and every config key, including ones this binary does not model (for +// example cli_version_sha on a SHA-pinned repo running an older cascade), are +// preserved verbatim rather than dropped on the round-trip. Finalize mutates +// only state, so a full re-marshal of the typed config is never needed here. func (o *Orchestrator) writeConfig() error { - data, err := yaml.Marshal(o.cicdFile) + key := config.DefaultManifestKey + if o.cicdFile.Config != nil && o.cicdFile.Config.ManifestKey != "" { + key = o.cicdFile.Config.ManifestKey + } + + current, err := os.ReadFile(o.configPath) + if err != nil { + return fmt.Errorf("failed to read config: %w", err) + } + + data, err := config.WriteManifestState(current, key, o.cicdFile.State, o.cicdFile.LatestRelease) if err != nil { return fmt.Errorf("failed to marshal config: %w", err) } diff --git a/internal/orchestrate/state_preserve_test.go b/internal/orchestrate/state_preserve_test.go new file mode 100644 index 0000000..6526460 --- /dev/null +++ b/internal/orchestrate/state_preserve_test.go @@ -0,0 +1,93 @@ +package orchestrate + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestWriteConfig_PreservesFullManifest guards against the state-write that +// silently dropped manifest configuration on the round-trip (issue #372). +// +// The finalize state write must touch only the mutable state subtree and leave +// every other manifest key byte-for-byte intact: the top-level manifest key +// wrapper, modeled config fields (cli_version_sha, which a SHA-pin repo relies +// on), config keys this binary does not model (a newer cascade release may add +// fields an older pinned binary cannot round-trip), and the internal runtime +// fields (manifest_file/manifest_key) must never leak into the committed file. +func TestWriteConfig_PreservesFullManifest(t *testing.T) { + dir := t.TempDir() + mfDir := filepath.Join(dir, ".github") + if err := os.MkdirAll(mfDir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(mfDir, "manifest.yaml") + + const sha = "5702d41a1234567890abcdef1234567890abcdef" + orig := `ci: + config: + trunk_branch: main + environments: + - dev + cli_version: v0.6.0 + pin_mode: sha + cli_version_sha: ` + sha + ` + future_field: keep-me + deploys: + - name: app + run: echo deploy + state: + dev: + sha: oldsha +` + if err := os.WriteFile(path, []byte(orig), 0o644); err != nil { + t.Fatal(err) + } + + o, err := NewOrchestrator(path, "ci", "dev") + if err != nil { + t.Fatalf("NewOrchestrator: %v", err) + } + + // Mutate state exactly as Finalize does, then persist. + o.cicdFile.State["dev"].SHA = "newsha" + o.cicdFile.State["dev"].Version = "v0.6.0-rc.1" + if err := o.writeConfig(); err != nil { + t.Fatalf("writeConfig: %v", err) + } + + out, err := os.ReadFile(path) + if err != nil { + t.Fatal(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 manifest key wrapper must survive. + if !strings.Contains(got, "ci:") { + t.Errorf("top-level 'ci:' wrapper dropped:\n%s", got) + } + // Modeled SHA-pin field must survive (the drift that started this). + if !strings.Contains(got, "cli_version_sha: "+sha) { + t.Errorf("cli_version_sha dropped:\n%s", got) + } + // Sibling config the round-trip should never touch. + if !strings.Contains(got, "cli_version: v0.6.0") { + t.Errorf("cli_version dropped:\n%s", got) + } + if !strings.Contains(got, "pin_mode: sha") { + t.Errorf("pin_mode dropped:\n%s", got) + } + // Config keys this binary does not model must survive (older-binary case). + if !strings.Contains(got, "future_field: keep-me") { + t.Errorf("unmodeled config field dropped:\n%s", got) + } + // Internal runtime fields must never leak into the committed manifest. + if strings.Contains(got, "manifest_file:") || strings.Contains(got, "manifest_key:") { + t.Errorf("internal runtime field leaked into manifest:\n%s", got) + } +}