feat(wfctl): infra audit-keys + prune + rotate-and-prune CLIs (PR5: Tasks 16-22)#584
Merged
Conversation
Adds TestInfraAuditKeys_ListsAll + fakeProviderEnumeratorAll fixture for
the new `wfctl infra audit-keys --type <T>` CLI. Verifies that the
command:
- delegates to interfaces.EnumeratorAll.EnumerateAll(resourceType)
- forwards the --type flag to the enumerator
- renders every returned key's identifying fields (Name, ProviderID/
access_key) into the writer
- exits 0 on success
This is the failing test for Task 16 of the spaces-key-iac-resource plan
(PR5). Until Task 17 implements runInfraAuditKeys + the registration of
`wfctl infra audit-keys`, this test fails to compile with `undefined:
runInfraAuditKeys`.
Pre-staged on feat/spaces-key-clis off feat/spaces-key-storage-filter
(PR4a). Will be rebased onto origin/main after PR4a merges + workflow
v0.26.0 tag is cut, then pushed as PR5.
Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7),
Task 16 of PR5.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the initial TDD “failing test” scaffold for the upcoming wfctl infra audit-keys subcommand, leveraging the new interfaces.EnumeratorAll provider capability.
Changes:
- Introduces
TestInfraAuditKeys_ListsAllto validate thataudit-keys --type <T>enumerates all resources of a given type and renders identifying fields. - Adds a minimal fake
EnumeratorAllimplementation to support the test scenario.
Comment on lines
+61
to
+63
| var out bytes.Buffer | ||
| exitCode := runInfraAuditKeys([]string{"--type", "infra.spaces_key"}, fakeProv, &out) | ||
| if exitCode != 0 { |
Implements `wfctl infra audit-keys --type <T>` for Task 17 of the spaces-key-iac-resource plan (PR5). The command loads iac.provider modules from infra.yaml (honoring --config / --env), finds the first provider that implements interfaces.EnumeratorAll, and prints every resource of `<T>` it returns as a fixed-width table (NAME / ACCESS_KEY / CREATED_AT). Read-only — drift correction surface for the destructive `wfctl infra prune` (Task 19) that comes next. Design notes: - runInfraAuditKeys takes interfaces.EnumeratorAll directly (not the broader IaCProvider) so unit tests can pass a minimal fake without implementing every IaCProvider method. The IaCProvider → EnumeratorAll type-assertion happens at the dispatcher boundary in runInfraAuditKeysCmd, where producing a structured error is appropriate (provider plugin doesn't support the optional interface). - auditKeysLoadProviders is a seam variable defaulting to defaultCleanupLoadProviders so audit-keys inherits the same env-resolution + plugin-discovery contract as `infra cleanup`. - auditKeysStdout / auditKeysStderr seam variables mirror the cleanup pattern so parallel tests don't race on global os.Stdout. Test coverage: TestInfraAuditKeys_ListsAll (Task 16, prior commit) now PASSes with this implementation; full cmd/wfctl test suite stays green. Smoke-tested: $ wfctl infra audit-keys --help # exit 0, prints flags $ wfctl infra audit-keys # exit 1, "no infra config found" Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7), Task 17 of PR5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds three failing tests for `wfctl infra prune` (Task 18 of PR5):
TestInfraPrune_RequiresTwoKeyOptIn — destructive prune must require
BOTH `--confirm` flag AND WFCTL_CONFIRM_PRUNE=1 env var. Either
alone (or neither) → non-zero exit before any cloud call.
TestInfraPrune_RequiresExcludeAccessKey — `--exclude-access-key` is
mandatory so the operator must explicitly name the active credential
they want preserved. Error message must mention the flag.
TestInfraPrune_FiltersByTimeAndExcludesAccessKey — happy path: with
both opt-ins + the exclude flag, prune deletes every key whose
created_at is older than --created-before EXCEPT the excluded
access_key. Tracks deletions by ProviderID on a fakeProviderWithDelete
that implements EnumerateAll + DeleteResource.
Local helper `pruneContains` (renamed to avoid collision with the
existing `containsString` in infra_templates.go).
Currently fails with `undefined: runInfraPrune` (test build broken) —
expected red state. Task 19 will introduce the implementation to make
all three pass. Stacked onto PR #584; final squash will land all
six PR5 commits together.
Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7),
Task 18 of PR5.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Comment on lines
+120
to
+124
| for _, p := range providers { | ||
| if enum, ok := p.(interfaces.EnumeratorAll); ok { | ||
| if rc := runInfraAuditKeys(args, enum, auditKeysStdout); rc != 0 { | ||
| return fmt.Errorf("audit-keys exited with code %d", rc) | ||
| } |
Comment on lines
+51
to
+55
| code := runInfraPrune([]string{ | ||
| "--type", "infra.spaces_key", | ||
| "--created-before", "2026-05-08T00:00:00Z", | ||
| "--exclude-access-key", "AK_NEW", | ||
| }, nil, &out) |
Comment on lines
+72
to
+73
| name, _ := o.Outputs["name"].(string) | ||
| ak, _ := o.Outputs["access_key"].(string) |
Comment on lines
+31
to
+34
| // writer. This is the failing test for Task 16 of the spaces-key-iac-resource | ||
| // plan (PR5). Until Task 17 implements runInfraAuditKeys + the registration | ||
| // of `wfctl infra audit-keys`, this test fails with `undefined: | ||
| // runInfraAuditKeys`. |
…minator
Implements `wfctl infra prune` for Task 19 of PR5. Destructive cloud-side
resource pruning with three-factor authorization plus mandatory exclusion
target so a typo can't accidentally nuke the active credential.
Authorization gauntlet (all three required):
- `--confirm` flag: explicit per-invocation consent
- WFCTL_CONFIRM_PRUNE=1 env var: two-key authorization (set by
operator; not by automation by default)
- interactive y/N prompt: skipped under `--non-interactive` for CI
Mandatory filter args (paranoia rail):
- `--created-before <RFC3339>`: only resources older than this eligible
- `--exclude-access-key <AK>`: this access_key preserved no matter what
Optional filters:
- `--allowlist <regex>`: skip names matching the regex
- `--recovery-from-last-rotation`: read filter args from the recovery
file written by `infra rotate-and-prune` (Task 21). On success the
recovery file is removed; on failure it's retained for re-invocation.
Design notes:
- runInfraPrune takes a narrow `pruneProvider` interface (EnumerateAll +
DeleteResource) so unit tests can use a minimal fake. Production code
wraps an interfaces.IaCProvider in pruneProviderAdapter which bridges
to the existing interfaces.EnumeratorAll + ResourceDriver.Delete
primitives at the boundary.
- Separates the recovery-file machinery (defaultStateDir,
recoveryFilePath, readRecoveryFile, recoveryFile struct) so Task 21
can reuse the writer side without code duplication.
- pruneStdout/pruneStderr seam vars + pruneLoadProviders seam mirror
the cleanup pattern so prune-related tests don't race on global
os.Stdout.
Test coverage: TestInfraPrune_RequiresTwoKeyOptIn,
TestInfraPrune_RequiresExcludeAccessKey, and
TestInfraPrune_FiltersByTimeAndExcludesAccessKey (Task 18, prior commit)
all PASS with this implementation. Full cmd/wfctl suite stays green.
Smoke-tested:
$ wfctl infra prune --confirm ... → exits 0/1 per the gauntlet
Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7),
Task 19 of PR5.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds two failing tests for `wfctl infra rotate-and-prune` per Task 20 of docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7): - TestInfraRotateAndPrune_HappyPath stubs the package-level bootstrapSecrets test hook (same pattern as the existing generateSecret hook in bootstrap_test.go) to return a single RotationResult, runs rotate-and-prune against a fake provider returning AK_OLD + AK_NEW from EnumerateAll, and asserts that AK_OLD is deleted, AK_NEW is preserved, and the recovery file at $WFCTL_STATE_DIR/last-rotation.json is removed on full success. - TestInfraRotateAndPrune_RecoveryFileWrittenWithCorrectPerms forces DeleteResource to return an error, then asserts that the recovery file is RETAINED (so `wfctl infra prune --recovery-from-last-rotation` can complete the prune without re-rotating) AND has permissions 0600 (owner-only — the file contains access_key + name metadata sufficient to identify the canonical credential). Adds the fakeProviderEnumerableDriver test double — same shape as the existing fakeProviderWithDelete in infra_prune_test.go but with an additional deleteErr hook so the failure-path test can simulate transient errors. Local rotateAndPruneContains helper avoids cross-file collisions with sibling test helpers (existing pruneContains is in a different file scope). Currently FAILS with `undefined: runInfraRotateAndPrune` — the failing-side signal Task 20 is supposed to produce. Task 21 lands the implementation in cmd/wfctl/infra_rotate_and_prune.go to make these PASS. Stacks on top of Task 19's implementation (commit 3185c5b) on the PR5 (feat/spaces-key-clis) branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
+111
to
+118
| code := runInfraRotateAndPrune([]string{ | ||
| "--type", "infra.spaces_key", | ||
| "--name", "test-key", | ||
| "--confirm", "--non-interactive", | ||
| }, fakeProv, &out) | ||
| if code != 0 { | ||
| t.Fatalf("rotate-and-prune failed: code=%d, out=%s", code, out.String()) | ||
| } |
Comment on lines
+93
to
+96
| case "audit-keys": | ||
| return runInfraAuditKeysCmd(args[1:]) | ||
| case "prune": | ||
| return runInfraPruneCmd(args[1:]) |
| fmt.Fprintf(w, " ✓ deleted %s\n", o.Name) | ||
| } | ||
| if failed > 0 { | ||
| fmt.Fprintf(w, "\n%d delete(s) failed; recovery file retained at %s\n", failed, recoveryFilePath()) |
| return 1 | ||
| } | ||
| if recoveryFromLastRotation { | ||
| _ = os.Remove(recoveryFilePath()) |
Comment on lines
+107
to
+108
| // move opt-in checks further from --confirm parsing. | ||
| // |
| fs.StringVar(&resourceType, "type", "", "Resource type (required, e.g. infra.spaces_key)") | ||
| fs.StringVar(&createdBefore, "created-before", "", "RFC3339 timestamp; only resources older than this are eligible") | ||
| fs.StringVar(&excludeAK, "exclude-access-key", "", "Access key to preserve (required: paranoia rail)") | ||
| fs.StringVar(&allowlist, "allowlist", "", "Regex matching names to skip (orthogonal to time filter)") |
Comment on lines
+181
to
+195
| for _, out := range outs { | ||
| ak, _ := out.Outputs["access_key"].(string) | ||
| ca, _ := out.Outputs["created_at"].(string) | ||
| name, _ := out.Outputs["name"].(string) | ||
| if ak == excludeAK { | ||
| continue | ||
| } | ||
| if allowlistRe != nil && allowlistRe.MatchString(name) { | ||
| continue | ||
| } | ||
| t, parseErr := time.Parse(time.RFC3339, ca) | ||
| if parseErr != nil || !t.Before(cutoff) { | ||
| continue | ||
| } | ||
| toDelete = append(toDelete, out) |
Comment on lines
+72
to
+75
| name, _ := o.Outputs["name"].(string) | ||
| ak, _ := o.Outputs["access_key"].(string) | ||
| ca, _ := o.Outputs["created_at"].(string) | ||
| fmt.Fprintf(w, "%-30s %-30s %s\n", name, ak, ca) |
Comment on lines
+83
to
+84
| // interfaces.EnumeratorAll for the requested --type, and dispatches to | ||
| // runInfraAuditKeys. |
…017)
Implements Task 21 of the spaces-key-iac-resource plan
(docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7).
Adds `cmd/wfctl/infra_rotate_and_prune.go` with three pieces:
1. recoveryRecord struct — JSON-superset of recoveryFile (defined in
infra_prune.go by Task 19) adding Source + RotatedAt for forensics.
The prune reader ignores the extra fields so this is a backwards-
compatible extension; both readers share the same canonical path.
2. writeRecoveryRecord(rec) — persists the JSON to
$WFCTL_STATE_DIR/last-rotation.json with 0600 file perms + 0700
parent dir. Sensitive credential metadata; only the owner reads it.
3. runInfraRotateAndPrune(args, provider, w) — the all-in-one CLI:
- Two-key opt-in: --confirm flag + WFCTL_CONFIRM_PRUNE=1 env (same as
plain prune, since rotation+prune is doubly destructive).
- Step 1: rotate via the existing parseSecretsConfig +
resolveSecretsProvider + resolveCredentialRevoker + bootstrapSecrets
chain, with forceRotate={name: true}. Returns []RotationResult per
ADR 0020 — no subprocess, no stderr parsing.
- Step 2: persist recovery record BEFORE the prune step, so a
mid-prune failure doesn't lose the data needed to finish cleanup
(an operator can recover via prune --recovery-from-last-rotation
without re-rotating, which would worsen any leak).
- Step 3: delegate to runInfraPrune passing rotated.CreatedAt as
--created-before and rotated.AccessKey as --exclude-access-key.
Older keys for the same Type are deleted; the just-rotated key is
preserved by the exclusion filter.
- On full success: rotate-and-prune (the file's writer) deletes the
recovery file. On prune failure: file is retained + recovery
instructions printed to stdout.
Provider is typed as `pruneProvider` (the narrow interface from
infra_prune.go) so the unit tests share a single fake surface with the
prune CLI itself — keeps blast radius small.
Verification:
- GOWORK=off go test ./cmd/wfctl -run TestInfraRotateAndPrune -v →
TestInfraRotateAndPrune_HappyPath PASS,
TestInfraRotateAndPrune_RecoveryFileWrittenWithCorrectPerms PASS.
- GOWORK=off go test ./cmd/wfctl -count=1 → entire wfctl suite PASS.
Test fixture adjustments (Task 20's test file):
- Added writeMinimalRotationConfig helper that writes a minimal
infra.yaml with secrets.provider=env (no external deps) so
parseSecretsConfig + resolveSecretsProvider succeed before the
bootstrapSecrets stub takes over. Plan's test code didn't pass a
--config but the actual implementation chain requires one;
surfacing the fixture in the test rather than special-casing the
impl keeps the hot path config-driven.
- Both tests now pass --config <fixture> alongside the existing flags.
Stacks on top of Task 20 (commit 739a0ef) on PR5 branch
(feat/spaces-key-clis).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Task 22 + Task 17 args bug fix) Closes Task 22 of PR5 — registers all three new infra subcommands under the `wfctl infra` dispatcher and adds operator-facing reference docs in docs/WFCTL.md. Bundled bug fix (per code-reviewer + Copilot 3212662894 retro): runInfraAuditKeysCmd and runInfraPruneCmd both forwarded the raw args slice (which can include --config / --env / -c) to their inner runInfraAuditKeys / runInfraPrune helpers whose narrow FlagSets only declared --type and prune-specific flags. Real CLI invocations like `wfctl infra audit-keys --type infra.spaces_key --config infra.yaml` exited 2 with `flag provided but not defined: -config` — documented flags broken. Unit tests of the inner functions never exercised the dispatcher's args-passing path so the bug shipped past review. Fix (team-lead's recommended option a): both dispatchers now CAPTURE every flag they pre-parse and SYNTHESIZE a clean inner-args slice with only flags the inner function understands. Added regression-sentinel smoke tests `TestInfraAuditKeysCmd_AcceptsConfigFlag` and `TestInfraPruneCmd_AcceptsConfigFlag` that prove `--config` + `--env` are accepted end-to-end through the dispatcher. rotate-and-prune dispatcher: Task 21's `runInfraRotateAndPrune` already declares --config / --env on its own FlagSet (it needs them for parseSecretsConfig in Step 1 of the rotate flow), so the dispatcher forwards args verbatim — no synthesize-clean-args dance required there. Files: - cmd/wfctl/infra.go: register `audit-keys`, `prune`, `rotate-and-prune` cases under the infra dispatcher; add usage-doc lines. - cmd/wfctl/infra_audit_keys.go: capture all dispatcher flags including --type; synthesize clean inner args for runInfraAuditKeys. - cmd/wfctl/infra_prune.go: same fix for runInfraPruneCmd; capture all flags + synthesize clean inner args. - cmd/wfctl/infra_rotate_and_prune.go: add runInfraRotateAndPruneCmd dispatcher + rotateAndPruneStdout / rotateAndPruneStderr seam vars + rotateAndPruneLoadProviders seam. - cmd/wfctl/infra_audit_keys_test.go: TestInfraAuditKeysCmd_AcceptsConfigFlag smoke test + minimal fakeIaCProviderForAuditKeys stub. - cmd/wfctl/infra_prune_test.go: TestInfraPruneCmd_AcceptsConfigFlag smoke test + fakeIaCProviderForPrune + fakeNoopDriver stubs. - docs/WFCTL.md: add command reference for all three subcommands (~120 lines: action table entries, full flag tables, exit codes, examples). Verification: $ go test ./cmd/wfctl → ok (full suite green) $ go build -o /tmp/wfctl ./cmd/wfctl $ /tmp/wfctl infra --help → lists audit-keys, prune, rotate-and-prune $ /tmp/wfctl infra audit-keys --help → exit 0, prints flags $ /tmp/wfctl infra prune --help → exit 0, prints flags $ /tmp/wfctl infra rotate-and-prune --help → exit 0, prints flags Note: `docs/dsl-reference-embedded.md` regen step from the plan skipped — the file does not exist in the current main; the `docs gen-dsl-reference` generator wasn't introduced in PR0/PR1/PR4a so there's nothing to regen. Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7), Task 22 of PR5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… step numbering, actionable recovery-cleanup warning Three Task 21 Minor follow-ups from code-reviewer (PR #584, on top of implementer-2's Task 22 wiring commit 5d8ae88): 1. **Unused --env**: replaced `var envName string; fs.StringVar(&envName, ...); _ = envName` with `_ = fs.String("env", "", ...)`. The flag is still declared so the dispatcher (`runInfraRotateAndPruneCmd` in 5d8ae88) can forward args verbatim including --env without the inner FlagSet erroring on unknown-flag, but no local var is allocated for a value that's never read inside this function. Doc comment now explicitly states the flag is consumed by the dispatcher; secrets-config resolution here happens via --config alone. 2. **Step-numbering**: previously the inline comments labelled three internal steps (1/rotate, 2/persist, 3/prune) but the user only saw two banners ("Step 1: rotating", "Step 2: pruning"). Aligned the comments to match the user-visible numbering: rotate is Step 1 (with recovery-write as a sub-step), prune is Step 2. 3. **Recovery-cleanup warning**: bare `failed to remove recovery file` replaced with explicit success-confirmation + manual-cleanup hint ("rotation+prune succeeded but failed to remove stale recovery file at PATH: ERR" + "this file is no longer needed; remove with `rm PATH` once safe."). Operator knows their data is fine and only the state file needs hand-clearing. Verification: - GOWORK=off go test ./cmd/wfctl -run TestInfraRotateAndPrune -v → both PASS - GOWORK=off go test ./cmd/wfctl -count=1 → entire wfctl suite PASS Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
+35
to
+40
| // runInfraRotateAndPrune already declares --config / --env (it needs them | ||
| // for parseSecretsConfig in Step 1 of the rotate flow), so the dispatcher | ||
| // forwards args verbatim — no synthesize-clean-inner-args dance required. | ||
| // We still pre-parse here to extract --config / --env for the provider | ||
| // loader, but we don't reformat the args slice. | ||
| func runInfraRotateAndPruneCmd(args []string) error { |
Comment on lines
+161
to
+180
| func runInfraRotateAndPrune(args []string, provider pruneProvider, w io.Writer) int { | ||
| fs := flag.NewFlagSet("infra rotate-and-prune", flag.ContinueOnError) | ||
| fs.SetOutput(w) | ||
| var resourceType, name, configFile, allowlist string | ||
| var confirm, nonInteractive bool | ||
| fs.StringVar(&resourceType, "type", "", "Resource type (required, e.g. infra.spaces_key)") | ||
| fs.StringVar(&name, "name", "", "Canonical credential name to rotate (required)") | ||
| fs.StringVar(&configFile, "config", "infra.yaml", "Config file") | ||
| fs.StringVar(&configFile, "c", "infra.yaml", "Config file (short)") | ||
| // --env is accepted-and-ignored here so the dispatcher (runInfraRotateAndPruneCmd) | ||
| // can forward args verbatim including --env without the inner FlagSet | ||
| // erroring on unknown-flag. The dispatcher already uses --env to scope | ||
| // provider loading; secrets-config resolution happens via --config alone. | ||
| _ = fs.String("env", "", "Environment name (consumed by dispatcher; ignored here)") | ||
| fs.StringVar(&allowlist, "allowlist", "", "Regex matching names to skip during prune") | ||
| fs.BoolVar(&confirm, "confirm", false, "Required: explicit confirmation flag") | ||
| fs.BoolVar(&nonInteractive, "non-interactive", false, "Skip the prune y/N prompt") | ||
| if err := fs.Parse(args); err != nil { | ||
| return 2 | ||
| } |
Comment on lines
+113
to
+128
| func writeRecoveryRecord(rec recoveryRecord) error { | ||
| dir, err := defaultStateDir() | ||
| if err != nil { | ||
| return fmt.Errorf("rotate-and-prune: resolve state dir: %w", err) | ||
| } | ||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return fmt.Errorf("rotate-and-prune: create state dir %s: %w", dir, err) | ||
| } | ||
| data, err := json.MarshalIndent(rec, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("rotate-and-prune: marshal recovery record: %w", err) | ||
| } | ||
| path := filepath.Join(dir, "last-rotation.json") | ||
| if err := os.WriteFile(path, data, 0600); err != nil { //nolint:gosec // intentional 0600 | ||
| return fmt.Errorf("rotate-and-prune: write recovery file %s: %w", path, err) | ||
| } |
Comment on lines
+181
to
+189
| for _, out := range outs { | ||
| ak, _ := out.Outputs["access_key"].(string) | ||
| ca, _ := out.Outputs["created_at"].(string) | ||
| name, _ := out.Outputs["name"].(string) | ||
| if ak == excludeAK { | ||
| continue | ||
| } | ||
| if allowlistRe != nil && allowlistRe.MatchString(name) { | ||
| continue |
Comment on lines
+72
to
+73
| name, _ := o.Outputs["name"].(string) | ||
| ak, _ := o.Outputs["access_key"].(string) |
Comment on lines
+107
to
+109
| // move opt-in checks further from --confirm parsing. | ||
| // | ||
| //nolint:cyclop // the validation gauntlet is intentional; splitting it would |
Comment on lines
+1545
to
+1557
| ``` | ||
| wfctl infra prune --type <T> --created-before <RFC3339> --exclude-access-key <AK> --confirm [--non-interactive] [--allowlist <regex>] [--recovery-from-last-rotation] | ||
| ``` | ||
|
|
||
| | Flag | Default | Description | | ||
| |------|---------|-------------| | ||
| | `--type` | _(required)_ | Resource type (e.g. `infra.spaces_key`) | | ||
| | `--created-before` | _(required)_ | RFC3339 timestamp; only resources older than this are eligible | | ||
| | `--exclude-access-key` | _(required)_ | Access key to preserve (paranoia rail) | | ||
| | `--allowlist` | `` | Regex matching resource names to skip (orthogonal to time filter) | | ||
| | `--confirm` | `false` | Required: explicit confirmation flag (paired with `WFCTL_CONFIRM_PRUNE=1` env var) | | ||
| | `--non-interactive` | `false` | Skip the y/N prompt (CI-friendly) | | ||
| | `--recovery-from-last-rotation` | `false` | Read filter args from `${WFCTL_STATE_DIR:-$HOME/.wfctl}/last-rotation.json` (written by `infra rotate-and-prune` for recovery from partial-failure rotations without re-rotating) | |
… + 3 Minor) Addresses code-reviewer's HOLD on Task 19 (commit 3185c5b) of PR #584. All five findings bundled into one commit per feedback_commit_label_by_content_not_round. Important #1 — flag rename: --allowlist → --preserve-names The --allowlist flag preserved matching names (skipped them during delete) but the name read as "list of resources allowed to be operated on", an ambiguity that on a destructive command is a real operator- error trap. Mental-model A would have an operator running `--allowlist '^manual-'` expecting to delete only manual-* keys, and instead deleting everything else (every production key). Renamed to --preserve-names (verb in the name; matches what the impl actually does: PRESERVE matching names, skip them during delete). No backward-compat alias since the flag shipped today and no operator has been instructed in any external doc to use --allowlist. Updates: - cmd/wfctl/infra_prune.go: runInfraPrune flag declaration + variable + error message - cmd/wfctl/infra_prune.go: runInfraPruneCmd dispatcher captures --preserve-names + forwards as --preserve-names - cmd/wfctl/infra_rotate_and_prune.go: runInfraRotateAndPrune flag declaration + variable; runInfraRotateAndPruneCmd dispatcher's pre-parse declaration; pruneArgs forwarding to runInfraPrune Important #2 — typed ProviderID/Name fields with Outputs[*] fallback The exclusion filter in runInfraPrune compared `out.Outputs["access_key"]` against --exclude-access-key. If a future EnumeratorAll-implementing provider populates ProviderID per the documented contract but doesn't redundantly write Outputs["access_key"], the exclude check would compare against an empty string, the active credential would be added to toDelete, and get silently deleted on a destructive run. Same risk in audit-keys' rendering loop (Outputs[*] only). Fix: prefer typed ProviderID/Name fields, fall back to Outputs[*] for backward compat with providers that populate both. Applied to: - cmd/wfctl/infra_prune.go: filter loop + dry-run rendering - cmd/wfctl/infra_audit_keys.go: rendering loop The DO provider (Task 15) populates both, so this is forward-looking defense; no behavior change for the only current consumer. Minor #1 — stray TODO fragment removed cmd/wfctl/infra_prune.go: the line `// move opt-in checks further from --confirm parsing.` was sandwiched between the exit-codes doc and the //nolint:cyclop directive. It was actually the second line of the nolint comment that gofmt split. Folded back into the nolint comment where it belongs. Minor #2 — conditional "recovery file retained" message cmd/wfctl/infra_prune.go: the failure message `%d delete(s) failed; recovery file retained at %s` fired unconditionally on delete failure, but the recovery file only exists if --recovery-from-last-rotation was set OR rotate-and-prune wrote it before this invocation. Plain prune invocations were getting pointed at a non-existent path. Conditional fix: only mention the recovery file when one actually exists. Minor #3 — surface os.Remove error as warning cmd/wfctl/infra_prune.go: `_ = os.Remove(recoveryFilePath())` after successful prune silently discarded errors. If perms changed or the file was locked, the next --recovery-from-last-rotation invocation would re-read stale data. Replaced with a non-fatal warning that's loud enough for operators to hand-clean. Verification: $ go test ./cmd/wfctl → ok $ wfctl infra prune --help → shows -preserve-names (no -allowlist) $ wfctl infra rotate-and-prune --help → shows -preserve-names Follow-up: PR #585 (docs) needs the runbook + ADR 0017 + WFCTL.md updated to the renamed flag. Will push as a stacked commit on docs/spaces-key-runbook. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to abe6b7b (the code rename). Updates the four docs/WFCTL.md references that still pointed at the old --allowlist flag name: - prune synopsis line - prune flag table row - rotate-and-prune synopsis line - rotate-and-prune flag table row These are the only remaining --allowlist references in the workflow repo. The runbook + ADR 0017 in docs/spaces-key-runbook (PR #585) get matching updates in a stacked commit on that branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
intel352
added a commit
that referenced
this pull request
May 9, 2026
…ionale Follow-up to abe6b7b (the code rename in PR #584). Updates the runbook + ADR 0017 to use --preserve-names everywhere the operator-facing flag appears, plus adds a rename-rationale block in each that explains why "allowlist" was retired (ambiguous verb on a destructive command — some operators read it as "list allowed to be operated on" / delete). Files: - docs/runbooks/spaces-key-prune.md: - "Allowlist for manual keys" section retitled "Preserving hand-created keys"; --allowlist → --preserve-names throughout. - Added a blockquote-style note explaining the rename, anchored to ADR 0017. - decisions/0017-prune-cli-two-key-opt-in.md: - Trade-study list updated (kept the prose generic — "Name regex as the primary filter" rather than naming a specific flag). - Decision section's --preserve-names paragraph extended with the naming rationale explicitly. Two remaining mentions of "allowlist" (runbook line 190, ADR 0017 line 65) are intentional — they're inside the rename-rationale text. No structural change — runbook still 7 H2 sections; ADR still follows workspace standard format. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Code-reviewer findings on Task 19 addressed — commits abe6b7b + 77882c2 on Important #1 — flag rename --allowlist → --preserve-namesRenamed in:
No backward-compat alias since the flag shipped today and no operator was instructed in any external doc to use --allowlist. Verb-in-the-name ( PR #585 (docs/spaces-key-runbook) gets matching runbook + ADR 0017 updates in commit c784ae6. Important #2 — typed ProviderID/Name fields with Outputs[*] fallbackBoth Same defensive fallback pattern in audit-keys' rendering loop (Copilot 3212662900 / 3212670097). Minors
VerificationReady for re-review. |
4 tasks
Comment on lines
+166
to
+174
| fs.StringVar(&resourceType, "type", "", "Resource type (required, e.g. infra.spaces_key)") | ||
| fs.StringVar(&name, "name", "", "Canonical credential name to rotate (required)") | ||
| fs.StringVar(&configFile, "config", "infra.yaml", "Config file") | ||
| fs.StringVar(&configFile, "c", "infra.yaml", "Config file (short)") | ||
| // --env is accepted-and-ignored here so the dispatcher (runInfraRotateAndPruneCmd) | ||
| // can forward args verbatim including --env without the inner FlagSet | ||
| // erroring on unknown-flag. The dispatcher already uses --env to scope | ||
| // provider loading; secrets-config resolution happens via --config alone. | ||
| _ = fs.String("env", "", "Environment name (consumed by dispatcher; ignored here)") |
Comment on lines
+246
to
+252
| ref := interfaces.ResourceRef{Type: o.Type, Name: o.Name, ProviderID: o.ProviderID} | ||
| if delErr := provider.DeleteResource(ctx, ref); delErr != nil { | ||
| fmt.Fprintf(w, "prune: delete %s: %v\n", o.Name, delErr) | ||
| failed++ | ||
| continue | ||
| } | ||
| fmt.Fprintf(w, " ✓ deleted %s\n", o.Name) |
Comment on lines
+91
to
+94
| // | ||
| // Until Task 21 lands runInfraRotateAndPrune in infra_rotate_and_prune.go, | ||
| // this test fails to compile with `undefined: runInfraRotateAndPrune` — the | ||
| // failing-side signal Task 20 is supposed to produce. |
Comment on lines
+126
to
+131
| // <T>` delegates to the provider's EnumeratorAll, then renders every | ||
| // returned key's identifying fields (Name, ProviderID/access_key) into the | ||
| // writer. This is the failing test for Task 16 of the spaces-key-iac-resource | ||
| // plan (PR5). Until Task 17 implements runInfraAuditKeys + the registration | ||
| // of `wfctl infra audit-keys`, this test fails with `undefined: | ||
| // runInfraAuditKeys`. |
Comment on lines
90
to
100
| return fmt.Errorf("audit-secrets exited with code %d", rc) | ||
| } | ||
| return nil | ||
| case "audit-keys": | ||
| return runInfraAuditKeysCmd(args[1:]) | ||
| case "prune": | ||
| return runInfraPruneCmd(args[1:]) | ||
| case "rotate-and-prune": | ||
| return runInfraRotateAndPruneCmd(args[1:]) | ||
| default: | ||
| return infraUsage() |
intel352
added a commit
that referenced
this pull request
May 9, 2026
…+24) (#585) * docs: spaces-key prune runbook + ADRs 0015-0018 + 0020 (PR6) Closes Tasks 23 + 24 of the spaces-key-iac-resource plan. PR6 ships operator + design documentation for the spaces-key infrastructure shipped in PR0-PR5. Files: - docs/runbooks/spaces-key-prune.md (Task 23) — operator runbook covering happy/multi-step/recovery/allowlist/GH-Secrets/FAQ. - decisions/0015-spaces-key-as-iac-resource.md — Approach B (two-phase fix). - decisions/0016-enumerator-all-interface.md — New optional interface for non-tag-supporting resource types. - decisions/0017-prune-cli-two-key-opt-in.md — `--exclude-access-key` + `--created-before` discriminator + two-key opt-in. - decisions/0018-bootstrap-key-exemption.md — Bootstrap key NOT an `infra.spaces_key` (chicken-and-egg + lifecycle separation). - decisions/0020-storage-filter-sidecar-metadata.md — Storage filter by `providerCredentialSubKeys[source]` allow-list. ADR 0019 (bootstrap-key rotation reaper) explicitly deferred per the design's decision log. ADR 0021 (rewriteTransport security fix) was authored separately by implementer-3 in PR4a; not modified here. Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(runbook): fix WFCTL_NEW_KEY_ACCESS_KEY false instruction + clarify opt-in terminology Two fixes from PR #585 code review: 1. **Important** (line 98) — the multi-step variant told operators to capture `WFCTL_NEW_KEY_ACCESS_KEY=<ak>` from `infra bootstrap --force-rotate` stderr. That line is never emitted: per the storage-filter contract (ADR 0020), `access_key` is a CANONICAL credential field (in `providerCredentialSubKeys["digitalocean.spaces"]`), so it gets STORED as a GH Secret named `SPACES_access_key`, NOT logged to stderr. Only SIDECAR fields (currently just `created_at`) emit `WFCTL_NEW_KEY_<UPPER>=` markers. Operators following the false instruction would have gotten stuck at Step 3 with no `--exclude-access-key` value, then either abandoned the multi-step path or — worse — passed an empty string and tripped the paranoia rail with a confusing error. Fix: rewrite Step 2's stderr capture comment to clarify what's actually emitted, then add an explicit access_key recovery block with two options: - Option A: `gh secret view SPACES_access_key --repo <owner>/<repo>` (read directly from the GH Secrets store). - Option B: `wfctl infra audit-keys` + match the row whose CREATED_AT equals the captured WFCTL_NEW_KEY_CREATED_AT=. 2. **Minor** (line 27 + FAQ) — the Overview said "all three opt-ins" while the FAQ said "two-key opt-in". Both are correct, but the relationship was implicit. Fix: rewrite the Overview list to spell out that opt-ins (1) + (2) are the "two-key" authorization (named after the two-person rule from physical security) and (3) is the runtime confirmation; cross-link from the FAQ. No structural change — runbook still has 7 H2 sections. Stacked on docs/spaces-key-runbook per feedback_commit_label_by_content_not_round. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: rename --allowlist references to --preserve-names + explain rationale Follow-up to abe6b7b (the code rename in PR #584). Updates the runbook + ADR 0017 to use --preserve-names everywhere the operator-facing flag appears, plus adds a rename-rationale block in each that explains why "allowlist" was retired (ambiguous verb on a destructive command — some operators read it as "list allowed to be operated on" / delete). Files: - docs/runbooks/spaces-key-prune.md: - "Allowlist for manual keys" section retitled "Preserving hand-created keys"; --allowlist → --preserve-names throughout. - Added a blockquote-style note explaining the rename, anchored to ADR 0017. - decisions/0017-prune-cli-two-key-opt-in.md: - Trade-study list updated (kept the prose generic — "Name regex as the primary filter" rather than naming a specific flag). - Decision section's --preserve-names paragraph extended with the naming rationale explicitly. Two remaining mentions of "allowlist" (runbook line 190, ADR 0017 line 65) are intentional — they're inside the rename-rationale text. No structural change — runbook still 7 H2 sections; ADR still follows workspace standard format. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR5 of the spaces-key-iac-resource plan. Adds three new
wfctl infrasubcommands that surroundiac.spaces_key(and any future non-tag-supporting resource type) with audit + prune tooling:wfctl infra audit-keys --type <T>— list every cloud-side resource of<T>(viaEnumeratorAll) with its identifying fields. Diff against state for the operator.wfctl infra prune --type <T>— delete cloud-side resources that have no corresponding state entry (with confirmation).wfctl infra rotate-and-prune --type <T>— rotate the active credential, then prune the orphaned old key once the new one is verified, with a recovery file written for safety.Depends on workflow v0.26.0 (released; bundles PR0 audit-secrets + PR1 R-A9 hardening + PR4a storage-filter + EnumeratorAll interface).
Stack
This PR opens with Task 16 (failing test for audit-keys) only. Subsequent tasks (17–22) will stack on this branch as commits and ship in the same squash-merge:
wfctl infra audit-keys(commit 5622d2f)wfctl infra audit-keyswfctl infra prunewfctl infra prunewfctl infra rotate-and-prunewfctl infra rotate-and-prune+ recovery filemain.go+ regen embedded docsVerification (this commit only)
This is the expected failure mode for the failing-test class. Task 17 will introduce
runInfraAuditKeysto make it pass.Test plan
undefined: runInfraAuditKeysEnumeratorAll🤖 Generated with Claude Code