From 934ef96cb98b59228f598a5cad2ecc73c7a5381c Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 18 Jul 2026 01:32:39 -0400 Subject: [PATCH] fix(state): preserve state.components on flat single-component write The flat state map models environments only, so parsing a component-scoped manifest lifted state.components into it as an empty entry keyed "components". Rebuilding the whole state node from that map collapsed state.components to {} and silently dropped every recorded component row while the write returned success. A finalize invoked without --component on such a manifest could land that loss on trunk. Treat components as a reserved, unowned subtree on the single-component write path: never rebuild it from the flat map, and preserve any existing subtree verbatim. A single-component manifest with no components subtree emits byte-identical output. Signed-off-by: Joshua Temple --- CHANGELOG.md | 12 ++ internal/config/statemerge.go | 40 ++++- .../statemerge_components_preserve_test.go | 140 ++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 internal/config/statemerge_components_preserve_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 45fa69e..71d1c53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,18 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **state:** A single-component (flat) state write on a manifest that carries + per-component state no longer destroys that state. The flat state map models + environments only, so parsing a component-scoped manifest lifted + `state.components` into it as an empty entry keyed `components`; rebuilding the + whole `state` node from that map then collapsed `state.components` to `{}` and + silently dropped every recorded component row while the write returned success. + A finalize invoked without `--component` on such a manifest (a stale + pre-migration workflow or a manual invocation) could land that loss on trunk. + The flat write path now treats `components` as a reserved, unowned subtree: it + is never rebuilt from the flat map and any existing subtree is preserved + verbatim. A single-component manifest with no `components` subtree emits + byte-identical output. - **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 diff --git a/internal/config/statemerge.go b/internal/config/statemerge.go index 50fcfeb..d1da7b9 100644 --- a/internal/config/statemerge.go +++ b/internal/config/statemerge.go @@ -108,9 +108,24 @@ func WriteScopedState(current []byte, manifestKey string, writes ...StateWrite) return data, nil } +// componentsStateKey is the reserved child of a `state` node that holds the +// per-component state subtree (state.components..). It is never an +// environment leaf. The typed flat state map (CICDFile.State) is +// map[string]*EnvState, so parsing a component-scoped manifest lifts +// state.components into that map as a bogus, empty EnvState keyed +// "components". A flat writer that rebuilt the whole state node from that map +// would emit `state.components: {}` and silently destroy every recorded +// component row. The single-component write path treats this key as a reserved, +// unowned sibling: it is never rebuilt from the flat map, and any existing +// subtree is preserved verbatim across the rebuild. +const componentsStateKey = "components" + // applySingleComponentWrites reconciles the whole single-component `state` node // from the union of set writes (byte-identical to the historical whole-node -// replacement) and applies the top-level latest_release directive. +// replacement) and applies the top-level latest_release directive. The reserved +// `components` subtree (state.components) is never treated as an env leaf: it is +// excluded from the rebuilt map and any existing subtree is carried over +// verbatim, so a flat writer on a component-scoped manifest can never wipe it. func applySingleComponentWrites(section *yaml.Node, writes []StateWrite) error { state := make(map[string]*EnvState) haveStateDirective := false @@ -118,6 +133,12 @@ func applySingleComponentWrites(section *yaml.Node, writes []StateWrite) error { if w.Env == "" { continue // latest_release directive, handled below } + if w.Env == componentsStateKey { + // The per-component state subtree, not an env leaf. A flat writer does + // not own it; skip it so the rebuild neither emits a bogus components + // row nor drops the real subtree (preserved verbatim below). + continue + } haveStateDirective = true if w.State != nil { state[w.Env] = w.State @@ -126,13 +147,23 @@ func applySingleComponentWrites(section *yaml.Node, writes []StateWrite) error { } if haveStateDirective { - if len(state) == 0 { + // Preserve an existing state.components subtree verbatim across the rebuild: + // the flat writer replaces the whole state node, but the components subtree + // is an unmodeled sibling it does not own. + var componentsNode *yaml.Node + if existing := mappingValue(section, "state"); existing != nil && existing.Kind == yaml.MappingNode { + componentsNode = mappingValue(existing, componentsStateKey) + } + if len(state) == 0 && componentsNode == nil { deleteMappingKey(section, "state") } else { node, err := valueNode(state) if err != nil { return fmt.Errorf("encoding state for state write: %w", err) } + if componentsNode != nil { + setMappingValue(node, componentsStateKey, componentsNode) + } setMappingValue(section, "state", node) } } @@ -334,6 +365,11 @@ func fetchedStateEnvKeys(current []byte, manifestKey string) []string { } keys := make([]string, 0, len(state.Content)/2) for i := 0; i+1 < len(state.Content); i += 2 { + if state.Content[i].Value == componentsStateKey { + // Reserved per-component subtree, not an env row: excluded so the + // wrapper never derives a delete directive that would drop it. + continue + } keys = append(keys, state.Content[i].Value) } return keys diff --git a/internal/config/statemerge_components_preserve_test.go b/internal/config/statemerge_components_preserve_test.go new file mode 100644 index 0000000..500c67c --- /dev/null +++ b/internal/config/statemerge_components_preserve_test.go @@ -0,0 +1,140 @@ +package config + +import ( + "strings" + "testing" +) + +// componentStateManifest is a component-scoped manifest carrying recorded state +// under state.components... Parsing it into the flat CICDFile.State +// map (map[string]*EnvState) lifts state.components into that map as a bogus, +// empty EnvState keyed "components". +const componentStateManifest = `ci: + config: + schema_version: 1 + trunk_branch: main + components: + api: + environments: + - name: staging + - name: prod + state: + components: + api: + staging: + sha: aaa111 + version: v1.0.0 + prod: + sha: bbb222 + version: v0.9.0 +` + +// TestWriteManifestState_FlatWriteOnComponentManifest_PreservesComponents drives +// the real production flat-write path (ParseManifestBytes then WriteManifestState, +// exactly what a finalize invoked without --component runs) against a manifest +// that carries per-component state. Before the fix, the flat rebuild replaced the +// whole state node from the typed map, whose bogus "components" EnvState collapsed +// state.components to {} and silently destroyed every recorded component row while +// the write returned success. The write must now preserve the subtree verbatim. +func TestWriteManifestState_FlatWriteOnComponentManifest_PreservesComponents(t *testing.T) { + file, err := ParseManifestBytes([]byte(componentStateManifest), "ci") + if err != nil { + t.Fatalf("ParseManifestBytes: %v", err) + } + // The flat parse lifts state.components into State as a bogus "components" key. + if _, lifted := file.State["components"]; !lifted { + t.Fatalf("precondition: expected flat parse to lift a bogus 'components' key, got keys %v", keysOfState(file.State)) + } + + got, err := WriteManifestState([]byte(componentStateManifest), "ci", file.State, file.LatestRelease) + if err != nil { + t.Fatalf("WriteManifestState: %v", err) + } + out := string(got) + + for _, want := range []string{"aaa111", "bbb222", "v1.0.0", "v0.9.0"} { + if !strings.Contains(out, want) { + t.Errorf("flat write destroyed component state (missing %q):\n%s", want, out) + } + } + if strings.Contains(out, "components: {}") { + t.Errorf("flat write collapsed state.components to an empty mapping:\n%s", out) + } +} + +// TestWriteManifestState_SingleComponentManifest_ByteIdentical pins that a genuine +// single-component manifest (no components: subtree) round-trips byte-identically +// through the fixed flat path, so the reserved-key handling adds nothing when no +// components subtree exists. +func TestWriteManifestState_SingleComponentManifest_ByteIdentical(t *testing.T) { + const flat = `ci: + config: + schema_version: 1 + trunk_branch: main + environments: + - name: staging + - name: prod + state: + staging: + sha: aaa111 + version: v1.0.0 + prod: + sha: bbb222 + version: v0.9.0 +` + final := map[string]*EnvState{ + "staging": {SHA: "aaa111", Version: "v1.0.0"}, + "prod": {SHA: "bbb222", Version: "v0.9.0"}, + } + got, err := WriteManifestState([]byte(flat), "ci", final, nil) + if err != nil { + t.Fatalf("WriteManifestState: %v", err) + } + oracle := referenceWholeNodeReplace(t, []byte(flat), "ci", final, nil) + if string(got) != string(oracle) { + t.Fatalf("single-component flat write drifted from whole-node-replace oracle\n--- got ---\n%s\n--- want ---\n%s", got, oracle) + } + if strings.Contains(string(got), "components") { + t.Fatalf("single-component write leaked a components key:\n%s", got) + } +} + +// TestWriteScopedState_ComponentWrite_PreservesSiblingComponents pins the +// #614-class invariant on the component-scoped path: writing one component's env +// leaf must not disturb a sibling component's recorded rows. +func TestWriteScopedState_ComponentWrite_PreservesSiblingComponents(t *testing.T) { + const twoComponents = `ci: + config: + schema_version: 1 + trunk_branch: main + state: + components: + api: + staging: + sha: aaa111 + web: + staging: + sha: ccc333 +` + got, err := WriteScopedState([]byte(twoComponents), "ci", + StateWrite{Component: "api", Env: "staging", State: &EnvState{SHA: "updated"}}, + ) + if err != nil { + t.Fatalf("WriteScopedState: %v", err) + } + out := string(got) + if !strings.Contains(out, "updated") { + t.Errorf("component write did not apply its own leaf:\n%s", out) + } + if !strings.Contains(out, "ccc333") { + t.Errorf("component write destroyed sibling component 'web':\n%s", out) + } +} + +func keysOfState(m map[string]*EnvState) []string { + ks := make([]string, 0, len(m)) + for k := range m { + ks = append(ks, k) + } + return ks +}