fix(infra): --refresh-outputs reconciles stale routed secrets instead of skipping them

cmd/wfctl/infra.go

@@ -1511,7 +1511,7 @@ func runInfraApply(args []string) error {
 			}
 		}
 	}
-	return syncInfraOutputSecrets(ctx, secretsCfg, secretsProvider, states, wfCfg, envName, runHydrated)
+	return syncInfraOutputSecrets(ctx, secretsCfg, secretsProvider, states, wfCfg, envName, runHydrated, refreshOutputsFlag)
 }
 
 func runInfraStatus(args []string) error {

cmd/wfctl/infra_apply_refresh_flag_test.go

@@ -443,3 +443,155 @@ func TestApply_RefreshOutputsFlag_EnvVarAndFlagBothSet_RunsOnce(t *testing.T) {
 		t.Errorf("refresh should have populated 'id'; got outputs=%v", got.Outputs)
 	}
 }
+
+// ── End-to-end: --refresh-outputs updates existing infra_output secrets ────────
+
+// writeRefreshFlagTestConfigWithSecrets writes an infra YAML config with:
+//   - auto_bootstrap disabled
+//   - filesystem state backend
+//   - iac.provider + infra.vpc modules
+//   - secrets block (provider: env) with one infra_output generator whose
+//     source is "vpc-resource.runner_url"
+//
+// The generated secret key is RUNNER_URL (no prefix, env provider key ==
+// env var name). Returns the config path.
+func writeRefreshFlagTestConfigWithSecrets(t *testing.T, dir string) string {
+	t.Helper()
+	stateDir := filepath.Join(dir, "state")
+	if err := os.MkdirAll(stateDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	cfgPath := filepath.Join(dir, "infra.yaml")
+	cfg := `infra:
+  auto_bootstrap: false
+secrets:
+  provider: env
+  generate:
+    - key: RUNNER_URL
+      type: infra_output
+      source: vpc-resource.runner_url
+modules:
+  - name: cloud-provider
+    type: iac.provider
+    config:
+      provider: fake-provider
+  - name: state-store
+    type: iac.state
+    config:
+      backend: filesystem
+      directory: ` + stateDir + `
+  - name: vpc-resource
+    type: infra.vpc
+    config:
+      provider: cloud-provider
+`
+	if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	return cfgPath
+}
+
+// seedVPCStateWithRunnerURL seeds a vpc-resource state entry whose Outputs
+// include a runner_url field, simulating a provider whose URL changed.
+func seedVPCStateWithRunnerURL(t *testing.T, cfgPath, url string) {
+	t.Helper()
+	store, err := resolveStateStore(cfgPath, "")
+	if err != nil {
+		t.Fatalf("resolveStateStore: %v", err)
+	}
+	entry := interfaces.ResourceState{
+		ID:         "vpc-resource",
+		Name:       "vpc-resource",
+		Type:       "infra.vpc",
+		Provider:   "fake-provider",
+		ProviderID: "pid-runner",
+		Outputs:    map[string]any{"runner_url": url},
+	}
+	if err := store.SaveResource(context.Background(), entry); err != nil {
+		t.Fatalf("seed state: %v", err)
+	}
+}
+
+// TestApply_RefreshOutputsFlag_UpdatesExistingInfraOutputSecret is the
+// end-to-end regression test for the original bug:
+//
+// Given:
+//   - RUNNER_URL env-secret already exists with a stale value
+//   - State contains vpc-resource.runner_url with the new (live) value
+//
+// When:
+//   - wfctl infra apply --refresh-outputs --auto-approve is run
+//
+// Then:
+//   - RUNNER_URL is updated to the new value (not skipped as "already exists")
+//
+// This exercises the infra.go:1514 call site — if refreshOutputsFlag were not
+// passed through, the secret would be skipped and the assertion would fail.
+func TestApply_RefreshOutputsFlag_UpdatesExistingInfraOutputSecret(t *testing.T) {
+	t.Setenv("WFCTL_REFRESH_OUTPUTS", "") // ensure env-var pre-step does not fire
+	os.Unsetenv("WFCTL_REFRESH_OUTPUTS")
+
+	dir := t.TempDir()
+	cfgPath := writeRefreshFlagTestConfigWithSecrets(t, dir)
+
+	// Seed state with the new URL already in place (simulates provider URL change
+	// having been captured by a previous refresh or initial apply).
+	const newURL = "https://runner-new.example.com"
+	const staleURL = "https://runner-old.example.com"
+	seedVPCStateWithRunnerURL(t, cfgPath, newURL)
+
+	// Pre-populate the secret with the stale value — this is what would happen
+	// after the initial apply before the provider was re-created.
+	t.Setenv("RUNNER_URL", staleURL)
+
+	// The fake provider's plan/apply is a no-op; state remains as seeded above.
+	cleanup := installFakeRefreshProvider(t, map[string]map[string]any{
+		"pid-runner": {"runner_url": newURL},
+	})
+	defer cleanup()
+
+	if _, err := captureStdout(t, func() error {
+		return runInfraApply([]string{"--auto-approve", "--refresh-outputs", "-c", cfgPath})
+	}); err != nil {
+		t.Fatalf("runInfraApply --refresh-outputs: %v", err)
+	}
+
+	// RUNNER_URL must have been updated to the new value.
+	if got := os.Getenv("RUNNER_URL"); got != newURL {
+		t.Errorf("RUNNER_URL: got %q, want %q — secret was not updated by --refresh-outputs", got, newURL)
+	}
+}
+
+// TestApply_NoRefreshOutputsFlag_DoesNotOverwriteExistingSecret verifies that
+// without --refresh-outputs, a pre-existing infra_output secret is NOT
+// overwritten by a normal apply (the "already exists — skipped" invariant).
+func TestApply_NoRefreshOutputsFlag_DoesNotOverwriteExistingSecret(t *testing.T) {
+	t.Setenv("WFCTL_REFRESH_OUTPUTS", "")
+	os.Unsetenv("WFCTL_REFRESH_OUTPUTS")
+
+	dir := t.TempDir()
+	cfgPath := writeRefreshFlagTestConfigWithSecrets(t, dir)
+
+	const newURL = "https://runner-new.example.com"
+	const userManagedURL = "https://user-managed.example.com"
+	seedVPCStateWithRunnerURL(t, cfgPath, newURL)
+
+	// User-managed value that must survive the apply.
+	t.Setenv("RUNNER_URL", userManagedURL)
+
+	cleanup := installFakeRefreshProvider(t, map[string]map[string]any{
+		"pid-runner": {"runner_url": newURL},
+	})
+	defer cleanup()
+
+	if _, err := captureStdout(t, func() error {
+		return runInfraApply([]string{"--auto-approve", "-c", cfgPath})
+	}); err != nil {
+		t.Fatalf("runInfraApply: %v", err)
+	}
+
+	// Without --refresh-outputs, the existing secret must not be overwritten.
+	if got := os.Getenv("RUNNER_URL"); got != userManagedURL {
+		t.Errorf("RUNNER_URL: got %q, want %q — normal apply must not overwrite existing secret", got, userManagedURL)
+	}
+}

cmd/wfctl/infra_output_secrets.go

@@ -112,8 +112,13 @@ func stateKeys(m map[string]map[string]any) []string {
 }
 
 // syncInfraOutputSecrets writes infra_output-typed secrets after a successful
-// apply. It skips secrets that already exist in the provider so idempotent
-// re-runs never overwrite live values.
+// apply. In normal mode (refreshOutputs=false) it skips secrets that already
+// exist in the provider so idempotent re-runs never overwrite live values.
+// When refreshOutputs=true (operator-opt-in via --refresh-outputs), existing
+// secrets are reconciled: updated if the value changed, logged as "unchanged"
+// if the value is the same (readable providers only), or always overwritten for
+// write-only providers (e.g. GitHub Actions) where comparison is impossible.
+//
 // wfCfg and envName are used to resolve source module names through per-env
 // overrides so that "bmw-database.uri" finds "bmw-staging-db" in state when
 // --env staging renames the module.
@@ -125,7 +130,7 @@ func stateKeys(m map[string]map[string]any) []string {
 // without going through provider.Get (which is unsupported on write-only
 // providers like GitHub Actions). May be nil for callers that don't
 // have a same-process apply hand-off (e.g., wfctl infra outputs CLI).
-func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, provider secrets.Provider, states []interfaces.ResourceState, wfCfg *config.WorkflowConfig, envName string, hydrated map[string]string) error {
+func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, provider secrets.Provider, states []interfaces.ResourceState, wfCfg *config.WorkflowConfig, envName string, hydrated map[string]string, refreshOutputs bool) error {
 	if secretsCfg == nil {
 		return nil
 	}
@@ -162,40 +167,61 @@ func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, prov
 		_, ok := listSet[key]
 		return ok, nil
 	}
-	secretExists := func(key string) (bool, error) {
-		_, err := provider.Get(ctx, key)
-		switch {
-		case err == nil:
-			return true, nil
-		case errors.Is(err, secrets.ErrNotFound):
-			return false, nil
-		case errors.Is(err, secrets.ErrUnsupported):
-			return lookupViaList(key)
-		default:
-			return false, fmt.Errorf("check secret %q: %w", key, err)
-		}
-	}
 
 	stateOutputs := buildStateOutputsMap(states)
 
 	for _, gen := range gens {
-		exists, err := secretExists(gen.Key)
-		if err != nil {
-			return err
+		// Attempt to read the current value. This serves two purposes:
+		//   1. Existence check for readable providers.
+		//   2. Value comparison in refresh mode to avoid spurious updates.
+		currentVal, getErr := provider.Get(ctx, gen.Key)
+
+		var exists bool
+		var isReadable bool
+		switch {
+		case getErr == nil:
+			exists = true
+			isReadable = true
+		case errors.Is(getErr, secrets.ErrNotFound):
+			exists = false
+		case errors.Is(getErr, secrets.ErrUnsupported):
+			// Write-only provider (e.g. GitHub Actions): fall back to List.
+			var listLookupErr error
+			exists, listLookupErr = lookupViaList(gen.Key)
+			if listLookupErr != nil {
+				return listLookupErr
+			}
+		default:
+			return fmt.Errorf("check secret %q: %w", gen.Key, getErr)
 		}
-		if exists {
+
+		if exists && !refreshOutputs {
 			fmt.Printf("  secret %q: already exists — skipped\n", gen.Key)
 			continue
 		}
 
-		value, err := resolveInfraOutput(wfCfg, gen.Source, envName, stateOutputs, hydrated)
-		if err != nil {
-			return fmt.Errorf("generate infra_output secret %q: %w", gen.Key, err)
+		newValue, resolveErr := resolveInfraOutput(wfCfg, gen.Source, envName, stateOutputs, hydrated)
+		if resolveErr != nil {
+			return fmt.Errorf("generate infra_output secret %q: %w", gen.Key, resolveErr)
 		}
-		if err := provider.Set(ctx, gen.Key, value); err != nil {
-			return fmt.Errorf("store secret %q: %w", gen.Key, err)
+
+		if exists {
+			// refreshOutputs is true here (guarded by the continue above).
+			// For readable providers skip the Set when the value is unchanged.
+			if isReadable && currentVal == newValue {
+				fmt.Printf("  secret %q: unchanged\n", gen.Key)
+				continue
+			}
+			if err := provider.Set(ctx, gen.Key, newValue); err != nil {
+				return fmt.Errorf("store secret %q: %w", gen.Key, err)
+			}
+			fmt.Printf("  secret %q: updated from infra output\n", gen.Key)
+		} else {
+			if err := provider.Set(ctx, gen.Key, newValue); err != nil {
+				return fmt.Errorf("store secret %q: %w", gen.Key, err)
+			}
+			fmt.Printf("  secret %q: created from infra output\n", gen.Key)
 		}
-		fmt.Printf("  secret %q: created from infra output\n", gen.Key)
 	}
 	return nil
 }