From 7962f175d77b5d46689c6dc087384b97639de964 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 00:01:47 -0400 Subject: [PATCH 1/4] test(wfctl): failing test reproducing bootstrapSecrets empty-rotations bug Adds two TDD-style tests pinning the rotation-result contract that broke in core-dump rotate-spaces-key staging dispatch run 25616807427 (2026-05-09): Step 1: rotating credential "coredump-deploy-key" (type infra.spaces_key)... secret "SPACES_access_key": created WFCTL_NEW_KEY_CREATED_AT=2026-05-10T01:41:58Z rotate-and-prune: expected 1 rotation result, got 0 Root cause: `wfctl infra rotate-and-prune --name ` builds forceRotate keyed by the cloud-side resource name (per ADR 0023 + docs/runbooks/spaces-key-prune.md), but bootstrapSecrets checks forceRotate[gen.Key] keyed by the canonical generator key. Mismatch => force-rotate code path silently bypassed for that gen, "normal" first- time-create path runs instead, prints "created" instead of "rotated", and crucially never appends to the rotations slice. Side effects (DO API mint + GH Secrets store + WFCTL_NEW_KEY_CREATED_AT sidecar) commit under the disguise of a normal create; rotate-and-prune then errors out before pruning, leaving the operator with a freshly-rotated credential, the old credential still live, and no automated cleanup path. The first test exercises the full runInfraRotateAndPrune integration with key="SPACES" + name="coredump-deploy-key" (the exact shape of core-dump's infra.yaml on the failing run) and asserts code==0 with a populated rotations result. Existing rotate-and-prune tests stub bootstrapSecrets entirely, so they never hit the real keying contract. The second test pins the bootstrapSecrets unit contract: when forceRotate is properly keyed by gen.Key AND the underlying generator+Set commits, rotations MUST contain a RotationResult. This holds regardless of which architectural fix lands (CLI-side translation or bootstrapSecrets-side multi-key tolerance). Per workspace memories: feedback_proper_fixes_over_workarounds (no soft fallbacks), feedback_force_strict_contracts_no_compat. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../infra_bootstrap_rotation_count_test.go | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 cmd/wfctl/infra_bootstrap_rotation_count_test.go diff --git a/cmd/wfctl/infra_bootstrap_rotation_count_test.go b/cmd/wfctl/infra_bootstrap_rotation_count_test.go new file mode 100644 index 00000000..96d94d61 --- /dev/null +++ b/cmd/wfctl/infra_bootstrap_rotation_count_test.go @@ -0,0 +1,188 @@ +package main + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestBootstrapSecrets_ForceRotate_AppendsRotationResult_NameKeyMismatch +// pins the contract that surfaced as a real-world bug on 2026-05-09 in +// core-dump rotate-spaces-key staging dispatch run 25616807427: +// +// Step 1: rotating credential "coredump-deploy-key" (type infra.spaces_key)... +// secret "SPACES_access_key": created +// WFCTL_NEW_KEY_CREATED_AT=2026-05-10T01:41:58Z +// [external-plugins] shutting down plugin "digitalocean" +// rotate-and-prune: expected 1 rotation result, got 0 +// error: rotate-and-prune exited with code 1 +// +// Root cause: `wfctl infra rotate-and-prune --name ` builds +// `forceRotate := map[string]bool{name: true}` keyed by the cloud-side +// resource name (per ADR 0023 + docs/runbooks/spaces-key-prune.md), but +// `bootstrapSecrets` checks `forceRotate[gen.Key]` keyed by the canonical +// generator key. Mismatch → force-rotate code path silently bypassed → +// rotations slice never appended to → "expected 1 rotation result, got 0" +// AFTER the cloud and GH Secrets side effects already committed. +// +// This test exercises the full runInfraRotateAndPrune path with a config +// where `key != name` — the exact shape of core-dump's infra.yaml on the +// failing run. Without the fix, runInfraRotateAndPrune returns code=1 with +// "expected 1 rotation result, got 0" even though bootstrapSecrets minted +// the new credential. With the fix, the CLI translates --name→gen.Key +// (or bootstrapSecrets accepts both keying forms) and the rotation result +// surfaces correctly. +func TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult(t *testing.T) { + t.Setenv("WFCTL_CONFIRM_PRUNE", "1") + tmpDir := t.TempDir() + t.Setenv("WFCTL_STATE_DIR", tmpDir) + + // Write a config matching core-dump's shape: secrets.generate[].key=SPACES, + // secrets.generate[].name=coredump-deploy-key. The CLI takes + // --name coredump-deploy-key (cloud-side resource name). + cfgPath := filepath.Join(tmpDir, "infra.yaml") + body := `secrets: + provider: env + config: + prefix: WFCTL_TEST_ + generate: + - key: SPACES + type: provider_credential + source: digitalocean.spaces + name: coredump-deploy-key +` + if err := os.WriteFile(cfgPath, []byte(body), 0600); err != nil { + t.Fatalf("write fixture infra.yaml: %v", err) + } + + // Stub the underlying generator so we don't reach DO. Returns a JSON + // blob shaped like generateDOSpacesKey's real output. The stub also + // proves the rotation HAPPENED (it was called) so any "got 0" failure + // is purely the keying-mismatch bug, not a missing call. + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + return `{"access_key":"AK_NEW_FROM_STUB","secret_key":"SK_NEW_FROM_STUB","created_at":"2026-05-10T01:41:58Z"}`, nil + }) + + // Pre-populate the env-provider's backing values so the force-rotate + // path (which reads OLD access_key for revocation) finds a value to + // "revoke" without being a hard requirement. Env provider reads from + // process env via prefix WFCTL_TEST_. + t.Setenv("WFCTL_TEST_SPACES_access_key", "AK_OLD") + t.Setenv("WFCTL_TEST_SPACES_secret_key", "SK_OLD") + + // fakeProviderEnumerableDriver returns the OLD canonical key as an + // enumerable resource so the prune step has something to look at and + // runs without the EnumerateAll-empty escape. + fakeProv := &fakeProviderEnumerableDriver{ + outputs: []*interfaces.ResourceOutput{ + { + Name: "coredump-deploy-key", + Type: "infra.spaces_key", + ProviderID: "AK_OLD", + Outputs: map[string]any{ + "access_key": "AK_OLD", + "created_at": "2026-05-01T00:00:00Z", + "name": "coredump-deploy-key", + }, + }, + { + Name: "coredump-deploy-key", + Type: "infra.spaces_key", + ProviderID: "AK_NEW_FROM_STUB", + Outputs: map[string]any{ + "access_key": "AK_NEW_FROM_STUB", + "created_at": "2026-05-10T01:41:58Z", + "name": "coredump-deploy-key", + }, + }, + }, + } + + var out bytes.Buffer + code := runInfraRotateAndPrune([]string{ + "--type", "infra.spaces_key", + "--name", "coredump-deploy-key", + "--config", cfgPath, + "--confirm", "--non-interactive", + // --prune-first=false to keep the test focused on the rotate step's + // rotation-result contract; pre-prune is exercised by other tests. + "--prune-first=false", + }, fakeProv, &out) + + if code != 0 { + t.Fatalf("rotate-and-prune failed: code=%d, out=%q\n\nThis is the regression: the rotation succeeded (stub generator was called and minted AK_NEW_FROM_STUB) but bootstrapSecrets returned an empty rotations slice because forceRotate was keyed by --name (\"coredump-deploy-key\") while bootstrapSecrets checks forceRotate[gen.Key] (\"SPACES\"). The CLI must translate --name→gen.Key.", code, out.String()) + } + + // Sanity: output should reflect rotation success (not skipped/created path). + got := out.String() + if !strings.Contains(got, "AK_NEW_FROM_STUB") { + t.Errorf("output should report new access_key from rotation; got: %s", got) + } +} + +// TestBootstrapSecrets_ForceRotate_NameOnlyMatch_StillRotatesAndAppends +// is the unit-level lock for the keying contract: when force-rotate +// contains an entry that matches secrets.generate[].name (NOT .key), +// rotation must still run and rotations must be appended. Without this, +// the rotate-and-prune integration is fragile to internal CLI changes. +// +// This test is the architecturally-correct contract for the bug. It can +// be satisfied by either: +// - bootstrapSecrets accepting forceRotate keyed by name OR key, OR +// - runInfraRotateAndPrune translating name→key before the call. +// +// Whichever fix lands, this test pins the invariant: a successful +// force-rotate where Set committed must produce a RotationResult. +func TestBootstrapSecrets_ForceRotate_AppendsRotationResultEvenWhenStoreEmpty(t *testing.T) { + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + return `{"access_key":"AK_NEW","secret_key":"SK_NEW","created_at":"2026-05-10T01:41:58Z"}`, nil + }) + + // Empty store — same shape as the staging-dispatch case where the OLD + // canonical sub-keys had been wiped before the dispatch (or never + // existed under the canonical-key shape because the prior config used + // the misconfigured suffixed-key shape). The bug-side surface is: + // even with an empty store, forceRotate[gen.Key]=true MUST drive the + // rotation appendpath, NOT the "doesn't exist → mint fresh" path that + // bypasses the rotations slice append. + p := newTrackingProvider(map[string]string{}) + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces", Name: "coredump-deploy-key"}, + }, + } + forceRotate := map[string]bool{"SPACES": true} + + _, rotations, err := bootstrapSecrets(context.Background(), p, cfg, forceRotate) + if err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + + // The contract: force-rotate that successfully Set sub-keys MUST + // surface a RotationResult. Side effects committed without a + // RotationResult is the false-negative the staging run hit. + if len(rotations) != 1 { + t.Fatalf("expected len(rotations)==1 after successful force-rotate; got %d. Side effects committed (Set calls=%v) but rotations slice is empty — same false-negative as staging run 25616807427.", len(rotations), p.setCalls) + } + if rotations[0].Key != "SPACES" { + t.Errorf("rotations[0].Key = %q, want \"SPACES\"", rotations[0].Key) + } + if rotations[0].AccessKey != "AK_NEW" { + t.Errorf("rotations[0].AccessKey = %q, want \"AK_NEW\"", rotations[0].AccessKey) + } + if rotations[0].CreatedAt != "2026-05-10T01:41:58Z" { + t.Errorf("rotations[0].CreatedAt = %q, want \"2026-05-10T01:41:58Z\"", rotations[0].CreatedAt) + } + // And the side effects must have happened (proves the rotation ran). + if !containsSlice(p.setCalls, "SPACES_access_key") { + t.Errorf("Set(SPACES_access_key) must be called; setCalls=%v", p.setCalls) + } + if !containsSlice(p.setCalls, "SPACES_secret_key") { + t.Errorf("Set(SPACES_secret_key) must be called; setCalls=%v", p.setCalls) + } +} From 7d0cfda45304bbd3fcb1a7fac10721d8023ef9ff Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 00:02:18 -0400 Subject: [PATCH 2/4] fix(wfctl): bootstrapSecrets returns rotation results for force-rotated keys (v0.27.3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the false-negative "expected 1 rotation result, got 0" surfaced in core-dump rotate-spaces-key staging dispatch run 25616807427 (2026-05-09). Two-layer architectural fix: 1. CLI translation (primary fix). runInfraRotateAndPrune now uses a new buildRotateAndPruneForceRotateSet helper that translates the cloud-side --name (per ADR 0023 + docs/runbooks/spaces-key-prune.md; matches secrets.generate[].name) into the canonical gen.Key values that bootstrapSecrets keys forceRotate by. Match precedence: .name first, fallback to .key for configs that omit .name. Rejects unknown names with a fast-fail error message — same UX shape as buildForceRotateSet's typo guard for `wfctl infra bootstrap --force-rotate`. The CLI surface is the architecturally correct place for this translation: the --name flag is documented to take the cloud-side name, and bootstrapSecrets rightly keys by the canonical generator identity. 2. Strict-contract guard (defense in depth). bootstrapSecrets now pre-validates that every forceRotate entry matches a known secrets.generate[].Key, returning an error before touching the store if not. This prevents future callers from silently bypassing the force-rotate code path when they pass mismatched keys — the exact shape of the staging-dispatch bug. Per workspace memory feedback_proper_fixes_over_workarounds and the user mandate "no soft fallback paths": the guard errors LOUD rather than tolerating the mismatch, ensuring callers get a structured error instead of a committed-side-effect-with-empty-result false-negative. Test fixture writeMinimalRotationConfig is extended with a second generate entry whose key != name so existing rotate-and-prune tests that pass --name canonical-name continue to work; previously those tests relied on stub-bootstrapSecrets to absorb the keying mismatch, which masked the bug. Per workspace memories: feedback_proper_fixes_over_workarounds, feedback_force_strict_contracts_no_compat, feedback_no_invented_interfaces. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_bootstrap.go | 20 +++++++ cmd/wfctl/infra_rotate_and_prune.go | 70 +++++++++++++++++++++++- cmd/wfctl/infra_rotate_and_prune_test.go | 15 ++++- 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index cef7b3da..1d062463 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -509,6 +509,26 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg if len(revoker) > 0 { credRevoker = revoker[0] } + // Strict-contract guard: every forceRotate entry MUST match exactly one + // secrets.generate[].Key. A mismatch (e.g. caller passed a cloud-side + // resource Name without translating to gen.Key) means the force-rotate + // code path will be silently bypassed for that entry — the loop below + // keys forceRotate[gen.Key], so the rotation never runs and the rotations + // slice stays empty. This produced the staging-dispatch false-negative + // "expected 1 rotation result, got 0" surfaced 2026-05-09 (run 25616807427). + // Caller MUST translate name→key (see buildForceRotateSet in this file + // and buildRotateAndPruneForceRotateSet in infra_rotate_and_prune.go). + if len(forceRotate) > 0 && cfg != nil { + known := make(map[string]bool, len(cfg.Generate)) + for _, gen := range cfg.Generate { + known[gen.Key] = true + } + for k := range forceRotate { + if !known[k] { + return nil, nil, fmt.Errorf("forceRotate entry %q does not match any secrets.generate[].key — caller must translate cloud-side names to canonical keys before calling bootstrapSecrets", k) + } + } + } generated := map[string]string{} var rotations []RotationResult diff --git a/cmd/wfctl/infra_rotate_and_prune.go b/cmd/wfctl/infra_rotate_and_prune.go index 4d1461c4..c3ea03a0 100644 --- a/cmd/wfctl/infra_rotate_and_prune.go +++ b/cmd/wfctl/infra_rotate_and_prune.go @@ -156,6 +156,63 @@ func runInfraRotateAndPruneCmd(args []string) error { return fmt.Errorf("rotate-and-prune: no loaded provider implements EnumeratorAll") } +// buildRotateAndPruneForceRotateSet translates the cloud-side resource name +// passed via `wfctl infra rotate-and-prune --name ` into the canonical +// secrets.generate[].Key values that bootstrapSecrets keys its forceRotate +// map by. +// +// Per ADR 0023 + docs/runbooks/spaces-key-prune.md, `--name` is the +// cloud-side resource Name (e.g. "coredump-deploy-key"), matching the +// `secrets.generate[].name` field. bootstrapSecrets keys forceRotate by +// `gen.Key` (e.g. "SPACES"), so the CLI must translate or the force-rotate +// path is silently skipped — the false-negative that surfaced as staging +// run 25616807427 ("rotate-and-prune: expected 1 rotation result, got 0" +// AFTER side effects committed). +// +// Match precedence: +// 1. secrets.generate[].name == name → take that gen's Key +// 2. secrets.generate[].key == name → fallback for configs that omit Name +// +// Returns an error when no entry matches (typo guard, mirrors the +// buildForceRotateSet validation in infra_bootstrap.go) so the operator +// gets a fast-fail before bootstrap touches the store. +// +// Multiple gens sharing the same Name (legitimate when a single cloud +// resource produces multiple secret entries — the misconfigured form +// caught by R-A9 — or when an operator intentionally splits sub-keys +// across entries) all rotate together. The order is the YAML declaration +// order of secrets.generate[]. +func buildRotateAndPruneForceRotateSet(name string, cfg *SecretsConfig) (map[string]bool, error) { + if name == "" { + return nil, fmt.Errorf("--name is required") + } + if cfg == nil || len(cfg.Generate) == 0 { + return nil, fmt.Errorf("config has no secrets.generate entries; nothing to rotate for --name %q", name) + } + out := map[string]bool{} + // Pass 1: match by Name field (canonical form per ADR 0023). + for _, gen := range cfg.Generate { + if gen.Name == name { + out[gen.Key] = true + } + } + if len(out) > 0 { + return out, nil + } + // Pass 2: fallback to Key match for configs that don't set Name. + // Common in older configs and in unit-test fixtures where Name is + // elided when it would equal Key. + for _, gen := range cfg.Generate { + if gen.Key == name { + out[gen.Key] = true + } + } + if len(out) == 0 { + return nil, fmt.Errorf("no secrets.generate entry matches --name %q (matched neither .name nor .key)", name) + } + return out, nil +} + // recoveryRecord is persisted to ${WFCTL_STATE_DIR:-$HOME/.wfctl}/last-rotation.json // after a successful rotate step, BEFORE the prune step. Used by the // `wfctl infra prune --recovery-from-last-rotation` flow (Task 19) to recover @@ -316,7 +373,18 @@ func runInfraRotateAndPrune(args []string, provider pruneProvider, w io.Writer) fmt.Fprintf(w, "rotate-and-prune: resolve secrets provider: %v\n", err) return 1 } - forceRotate := map[string]bool{name: true} + // Translate the cloud-side resource name (--name; per ADR 0023 + + // docs/runbooks/spaces-key-prune.md) into the canonical secrets.generate[].Key + // values that bootstrapSecrets keys forceRotate by. Without this translation + // forceRotate[gen.Key] is false for every generator and bootstrapSecrets + // silently bypasses the force-rotate code path (rotations slice stays empty + // even when the underlying generator + Set side effects committed) — the + // staging-dispatch false-negative surfaced 2026-05-09 (run 25616807427). + forceRotate, err := buildRotateAndPruneForceRotateSet(name, cfg) + if err != nil { + fmt.Fprintf(w, "rotate-and-prune: %v\n", err) + return 1 + } revoker, closer := resolveCredentialRevoker(ctx, configFile, cfg, forceRotate) if closer != nil { defer closer.Close() diff --git a/cmd/wfctl/infra_rotate_and_prune_test.go b/cmd/wfctl/infra_rotate_and_prune_test.go index c47b5757..df79a857 100644 --- a/cmd/wfctl/infra_rotate_and_prune_test.go +++ b/cmd/wfctl/infra_rotate_and_prune_test.go @@ -57,8 +57,15 @@ func rotateAndPruneContains(haystack []string, needle string) bool { // for runInfraRotateAndPrune to load without erroring before reaching the // stubbed `bootstrapSecrets` hook. The config provides a `secrets:` block // (so parseSecretsConfig returns non-nil) with provider=env so -// resolveSecretsProvider builds without external dependencies. Returns the -// fixture's full path for use as `--config` arg. +// resolveSecretsProvider builds without external dependencies. +// +// Two generate entries are declared so callers can use either --name +// "test-key" (key + name both) or --name "canonical-name" (cloud-side +// name; key="canonical-key") — the latter exercises the name→key +// translation in buildRotateAndPruneForceRotateSet introduced to fix +// the staging-dispatch false-negative (run 25616807427, 2026-05-09). +// +// Returns the fixture's full path for use as `--config` arg. func writeMinimalRotationConfig(t *testing.T, tmpDir string) string { t.Helper() cfgPath := filepath.Join(tmpDir, "infra.yaml") @@ -71,6 +78,10 @@ func writeMinimalRotationConfig(t *testing.T, tmpDir string) string { type: provider_credential source: digitalocean.spaces name: test-key + - key: canonical-key + type: provider_credential + source: digitalocean.spaces + name: canonical-name ` if err := os.WriteFile(cfgPath, []byte(body), 0600); err != nil { t.Fatalf("write fixture infra.yaml: %v", err) From a82f75aa0f9e57bafcf976779640bf5466f0e5af Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 06:50:22 -0400 Subject: [PATCH 3/4] fix(wfctl): tighten rotate-and-prune --name validation per Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot findings on PR 594: 1. buildRotateAndPruneForceRotateSet (cmd/wfctl/infra_rotate_and_prune.go:185) was permissive: multi-Name match returned multiple Keys, and non-provider_credential matches were accepted. Both classes reintroduced the false-negative-after-side-effects failure mode this PR was opened to fix: - multi-Name → multi-rotate → fails len(rotations)==1 after side effects - non-provider_credential → rotates without appending RotationResult Helper now enforces a strict contract: - exactly one matching gen entry (zero or two-plus reject with named keys) - matched gen MUST be type=provider_credential 2. TestBootstrapSecrets_ForceRotate doc comment claimed bootstrapSecrets accepted forceRotate keyed by either .name OR .key. Actual implementation strictly requires .Key. Comment updated to document strict contract. Adds TestBuildRotateAndPruneForceRotateSet covering 8 cases including multi-match + non-provider_credential rejection. Local tests PASS. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../infra_bootstrap_rotation_count_test.go | 24 ++-- cmd/wfctl/infra_rotate_and_prune.go | 54 +++++--- ..._rotate_and_prune_force_rotate_set_test.go | 115 ++++++++++++++++++ 3 files changed, 165 insertions(+), 28 deletions(-) create mode 100644 cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go diff --git a/cmd/wfctl/infra_bootstrap_rotation_count_test.go b/cmd/wfctl/infra_bootstrap_rotation_count_test.go index 96d94d61..ca2c13a1 100644 --- a/cmd/wfctl/infra_bootstrap_rotation_count_test.go +++ b/cmd/wfctl/infra_bootstrap_rotation_count_test.go @@ -127,17 +127,23 @@ func TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult(t *testing.T) { // TestBootstrapSecrets_ForceRotate_NameOnlyMatch_StillRotatesAndAppends // is the unit-level lock for the keying contract: when force-rotate -// contains an entry that matches secrets.generate[].name (NOT .key), -// rotation must still run and rotations must be appended. Without this, -// the rotate-and-prune integration is fragile to internal CLI changes. +// is keyed by secrets.generate[].Key for a successful provider_credential +// force-rotate that hits an empty store, rotation must run and a +// RotationResult must be appended. Without this, the rotate-and-prune +// integration is fragile to internal CLI changes. // -// This test is the architecturally-correct contract for the bug. It can -// be satisfied by either: -// - bootstrapSecrets accepting forceRotate keyed by name OR key, OR -// - runInfraRotateAndPrune translating name→key before the call. +// Strict contract enforced by this PR: +// - forceRotate MUST be keyed by secrets.generate[].Key (canonical key). +// - bootstrapSecrets ignores entries that don't match a known generator +// Key (it does NOT accept generator .Name as a fallback key). +// - Name→Key translation is the CLI layer's responsibility — see +// buildRotateAndPruneForceRotateSet in infra_rotate_and_prune.go, +// which also enforces single-match + provider_credential constraints +// so this contract is unviolable from the operator side. // -// Whichever fix lands, this test pins the invariant: a successful -// force-rotate where Set committed must produce a RotationResult. +// Invariant pinned by this test: a successful force-rotate where Set +// committed MUST produce a RotationResult (not the false-negative the +// staging run hit on 2026-05-09). func TestBootstrapSecrets_ForceRotate_AppendsRotationResultEvenWhenStoreEmpty(t *testing.T) { withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { return `{"access_key":"AK_NEW","secret_key":"SK_NEW","created_at":"2026-05-10T01:41:58Z"}`, nil diff --git a/cmd/wfctl/infra_rotate_and_prune.go b/cmd/wfctl/infra_rotate_and_prune.go index c3ea03a0..ded5a3e2 100644 --- a/cmd/wfctl/infra_rotate_and_prune.go +++ b/cmd/wfctl/infra_rotate_and_prune.go @@ -177,11 +177,20 @@ func runInfraRotateAndPruneCmd(args []string) error { // buildForceRotateSet validation in infra_bootstrap.go) so the operator // gets a fast-fail before bootstrap touches the store. // -// Multiple gens sharing the same Name (legitimate when a single cloud -// resource produces multiple secret entries — the misconfigured form -// caught by R-A9 — or when an operator intentionally splits sub-keys -// across entries) all rotate together. The order is the YAML declaration -// order of secrets.generate[]. +// Strict-contract enforcement (per Copilot review on PR 594): +// - Exactly ONE matching gen entry. Zero or two-plus matches reject. +// - Match MUST be type=provider_credential. Other rotatable types +// (random_hex, random_base64, random_alphanumeric) are intentionally +// rejected here because rotate-and-prune's downstream invariants +// (len(rotations)==1, prune step expects an old credential to revoke) +// are coherent only for provider_credential. Operators rotating +// non-provider_credential generators must use `wfctl infra bootstrap +// --force-rotate` directly. +// +// Without these guards a multi-Name config or a non-provider_credential +// --name target would either rotate multiple keys then fail count check +// (side effects committed but errored) OR rotate without appending a +// RotationResult (the false-negative class this PR was opened to fix). func buildRotateAndPruneForceRotateSet(name string, cfg *SecretsConfig) (map[string]bool, error) { if name == "" { return nil, fmt.Errorf("--name is required") @@ -189,28 +198,35 @@ func buildRotateAndPruneForceRotateSet(name string, cfg *SecretsConfig) (map[str if cfg == nil || len(cfg.Generate) == 0 { return nil, fmt.Errorf("config has no secrets.generate entries; nothing to rotate for --name %q", name) } - out := map[string]bool{} - // Pass 1: match by Name field (canonical form per ADR 0023). + // Match precedence: secrets.generate[].name first (canonical per ADR 0023), + // then .key fallback for older configs / unit-test fixtures. + var matched []SecretGen for _, gen := range cfg.Generate { if gen.Name == name { - out[gen.Key] = true + matched = append(matched, gen) } } - if len(out) > 0 { - return out, nil - } - // Pass 2: fallback to Key match for configs that don't set Name. - // Common in older configs and in unit-test fixtures where Name is - // elided when it would equal Key. - for _, gen := range cfg.Generate { - if gen.Key == name { - out[gen.Key] = true + if len(matched) == 0 { + for _, gen := range cfg.Generate { + if gen.Key == name { + matched = append(matched, gen) + } } } - if len(out) == 0 { + if len(matched) == 0 { return nil, fmt.Errorf("no secrets.generate entry matches --name %q (matched neither .name nor .key)", name) } - return out, nil + if len(matched) > 1 { + keys := make([]string, len(matched)) + for i, g := range matched { + keys[i] = g.Key + } + return nil, fmt.Errorf("--name %q matches multiple secrets.generate entries (keys: %v); rotate-and-prune supports exactly one provider_credential per dispatch — use `wfctl infra bootstrap --force-rotate` for multi-entry rotation", name, keys) + } + if matched[0].Type != "provider_credential" { + return nil, fmt.Errorf("--name %q matches secrets.generate entry of type %q; rotate-and-prune only operates on provider_credential entries (use `wfctl infra bootstrap --force-rotate` for other rotatable types)", name, matched[0].Type) + } + return map[string]bool{matched[0].Key: true}, nil } // recoveryRecord is persisted to ${WFCTL_STATE_DIR:-$HOME/.wfctl}/last-rotation.json diff --git a/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go new file mode 100644 index 00000000..960a5165 --- /dev/null +++ b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go @@ -0,0 +1,115 @@ +package main + +import ( + "strings" + "testing" +) + +// TestBuildRotateAndPruneForceRotateSet exercises the strict-contract +// validation added per Copilot review on PR #594: +// - Exactly one matching gen entry (zero or two-plus reject) +// - Match must be type=provider_credential +// - Returns map[Key]bool with exactly one entry +// +// Without these guards, the staging-rotation false-negative class returns: +// multi-match → multi-rotate → fails len(rotations)==1 after side effects; +// non-provider_credential match → rotates without appending RotationResult. +func TestBuildRotateAndPruneForceRotateSet(t *testing.T) { + cases := []struct { + name string + argName string + gens []SecretGen + wantKeys []string // empty = expect error + wantErrFrag string // substring expected in error + }{ + { + name: "happy_path_match_by_name", + argName: "coredump-deploy-key", + gens: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Name: "coredump-deploy-key"}, + }, + wantKeys: []string{"SPACES"}, + }, + { + name: "happy_path_fallback_match_by_key", + argName: "SPACES", + gens: []SecretGen{ + {Key: "SPACES", Type: "provider_credential"}, + }, + wantKeys: []string{"SPACES"}, + }, + { + name: "empty_name_rejected", + argName: "", + gens: []SecretGen{{Key: "SPACES", Type: "provider_credential"}}, + wantErrFrag: "--name is required", + }, + { + name: "no_generate_entries", + argName: "anything", + gens: nil, + wantErrFrag: "no secrets.generate entries", + }, + { + name: "no_match", + argName: "ghost-name", + gens: []SecretGen{{Key: "SPACES", Type: "provider_credential", Name: "coredump-deploy-key"}}, + wantErrFrag: "no secrets.generate entry matches", + }, + { + name: "multi_match_by_name_rejected", + argName: "shared-name", + gens: []SecretGen{ + {Key: "SPACES_A", Type: "provider_credential", Name: "shared-name"}, + {Key: "SPACES_B", Type: "provider_credential", Name: "shared-name"}, + }, + wantErrFrag: "matches multiple secrets.generate entries", + }, + { + name: "non_provider_credential_rejected", + argName: "session-secret", + gens: []SecretGen{ + {Key: "SESSION_SECRET", Type: "random_hex", Name: "session-secret"}, + }, + wantErrFrag: "rotate-and-prune only operates on provider_credential", + }, + { + name: "name_match_takes_precedence_over_key_match", + argName: "SPACES", + gens: []SecretGen{ + {Key: "OTHER", Type: "provider_credential", Name: "SPACES"}, // matched (Pass 1 by Name) + {Key: "SPACES", Type: "provider_credential"}, // not matched (Pass 1 hit, Pass 2 skipped) + }, + wantKeys: []string{"OTHER"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := &SecretsConfig{Generate: tc.gens} + got, err := buildRotateAndPruneForceRotateSet(tc.argName, cfg) + + if tc.wantErrFrag != "" { + if err == nil { + t.Fatalf("expected error containing %q; got nil (got=%v)", tc.wantErrFrag, got) + } + if !strings.Contains(err.Error(), tc.wantErrFrag) { + t.Fatalf("expected error containing %q; got %q", tc.wantErrFrag, err.Error()) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != len(tc.wantKeys) { + t.Fatalf("expected %d keys, got %d (%v)", len(tc.wantKeys), len(got), got) + } + for _, k := range tc.wantKeys { + if !got[k] { + t.Fatalf("expected key %q in result, got %v", k, got) + } + } + }) + } +} From 8dc732fe69994cc076d5a8676c283fed255f47e8 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 07:09:25 -0400 Subject: [PATCH 4/4] fix(wfctl): close Key-collision gap + sync stale doc comments per Copilot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three Copilot findings on PR 594 round 2: 1. infra_rotate_and_prune.go: buildRotateAndPruneForceRotateSet caught multi-Name collisions but missed multi-Key-with-different-Names. Two gens with same .Key (different Names) → forceRotate[key]=true → bootstrapSecrets rotates BOTH → fails len(rotations)==1 after side effects committed (the same false-negative class via Key collision instead of Name collision). Added defense-in-depth Key-uniqueness check that rejects with named collision list. 2. infra_bootstrap_rotation_count_test.go: doc comment header listed a stale test name (TestBootstrapSecrets_ForceRotate_AppendsRotationResult_NameKeyMismatch) while the actual function is TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult. Synced. 3. infra_bootstrap_rotation_count_test.go: comment said the fix could be satisfied by "CLI translation OR bootstrapSecrets accepting both keying forms". Actual contract: bootstrapSecrets strictly requires forceRotate keyed by gen.Key (rejects unknown entries); CLI does the translation at buildRotateAndPruneForceRotateSet. Updated to match. Adds matched_key_shared_with_another_gen_rejected test case to TestBuildRotateAndPruneForceRotateSet covering the new defense-in-depth guard. All 9 cases PASS. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../infra_bootstrap_rotation_count_test.go | 7 ++++--- cmd/wfctl/infra_rotate_and_prune.go | 19 +++++++++++++++++++ ..._rotate_and_prune_force_rotate_set_test.go | 12 ++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/cmd/wfctl/infra_bootstrap_rotation_count_test.go b/cmd/wfctl/infra_bootstrap_rotation_count_test.go index ca2c13a1..c84f561d 100644 --- a/cmd/wfctl/infra_bootstrap_rotation_count_test.go +++ b/cmd/wfctl/infra_bootstrap_rotation_count_test.go @@ -11,7 +11,7 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" ) -// TestBootstrapSecrets_ForceRotate_AppendsRotationResult_NameKeyMismatch +// TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult // pins the contract that surfaced as a real-world bug on 2026-05-09 in // core-dump rotate-spaces-key staging dispatch run 25616807427: // @@ -35,8 +35,9 @@ import ( // failing run. Without the fix, runInfraRotateAndPrune returns code=1 with // "expected 1 rotation result, got 0" even though bootstrapSecrets minted // the new credential. With the fix, the CLI translates --name→gen.Key -// (or bootstrapSecrets accepts both keying forms) and the rotation result -// surfaces correctly. +// at the buildRotateAndPruneForceRotateSet boundary; bootstrapSecrets +// strictly requires forceRotate keyed by gen.Key and rejects unknown +// entries (no name-fallback on the bootstrap side). func TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult(t *testing.T) { t.Setenv("WFCTL_CONFIRM_PRUNE", "1") tmpDir := t.TempDir() diff --git a/cmd/wfctl/infra_rotate_and_prune.go b/cmd/wfctl/infra_rotate_and_prune.go index ded5a3e2..32868878 100644 --- a/cmd/wfctl/infra_rotate_and_prune.go +++ b/cmd/wfctl/infra_rotate_and_prune.go @@ -226,6 +226,25 @@ func buildRotateAndPruneForceRotateSet(name string, cfg *SecretsConfig) (map[str if matched[0].Type != "provider_credential" { return nil, fmt.Errorf("--name %q matches secrets.generate entry of type %q; rotate-and-prune only operates on provider_credential entries (use `wfctl infra bootstrap --force-rotate` for other rotatable types)", name, matched[0].Type) } + // Defense-in-depth: ensure the matched Key is unique across all + // secrets.generate[] entries. Without this, a config with two gens + // sharing the same Key (different Names) would set forceRotate[key]=true + // → bootstrapSecrets rotates BOTH → fails len(rotations)==1 after side + // effects committed (the same false-negative class this PR was opened + // to fix, just via Key collision instead of Name collision). + keyCount := 0 + var keyCollisionNames []string + for _, gen := range cfg.Generate { + if gen.Key == matched[0].Key { + keyCount++ + if gen.Name != "" { + keyCollisionNames = append(keyCollisionNames, gen.Name) + } + } + } + if keyCount > 1 { + return nil, fmt.Errorf("secrets.generate has %d entries with .key=%q (names: %v); rotate-and-prune requires Key uniqueness so the rotation count invariant holds", keyCount, matched[0].Key, keyCollisionNames) + } return map[string]bool{matched[0].Key: true}, nil } diff --git a/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go index 960a5165..b3a66a69 100644 --- a/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go +++ b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go @@ -82,6 +82,18 @@ func TestBuildRotateAndPruneForceRotateSet(t *testing.T) { }, wantKeys: []string{"OTHER"}, }, + { + // Defense-in-depth: even when --name picks a single gen, if the + // matched gen's Key is shared with another gen, forceRotate[key] + // would rotate both → fails len(rotations)==1. Reject up-front. + name: "matched_key_shared_with_another_gen_rejected", + argName: "primary-key", + gens: []SecretGen{ + {Key: "SHARED_KEY", Type: "provider_credential", Name: "primary-key"}, // matched + {Key: "SHARED_KEY", Type: "provider_credential", Name: "secondary-key"}, // collides on Key + }, + wantErrFrag: "rotate-and-prune requires Key uniqueness", + }, } for _, tc := range cases {