diff --git a/cmd/wfctl/infra_align.go b/cmd/wfctl/infra_align.go index e6be2236a..74f6f1ebb 100644 --- a/cmd/wfctl/infra_align.go +++ b/cmd/wfctl/infra_align.go @@ -86,7 +86,7 @@ func runInfraAlign(args []string) error { if alignExitCode(findings, opts.strict) != 0 { var failCount int for _, f := range findings { - if f.Severity == "FAIL" || (opts.strict && f.Severity == "WARN") { + if f.Severity == "FAIL" || f.Severity == "ERROR" || (opts.strict && f.Severity == "WARN") { failCount++ } } @@ -235,9 +235,19 @@ func loadPlanJSON(path string) (*interfaces.IaCPlan, error) { } // alignExitCode returns 0 (success) or 1 (failure) based on findings and flags. +// +// Severities that always block (exit 1): +// - FAIL: deterministic failure (e.g. unresolved env var, R-A1 missing image) +// - ERROR: hard rule violation that must block deploy regardless of --strict +// (introduced in rev3 of the spaces-key plan for R-A9; ERROR is treated +// identically to FAIL by exit-code logic so a rule author can use ERROR to +// signal "this is fixable; here's the fix" without weakening the gate) +// +// Severities that block only under --strict: +// - WARN: advisory, surfaced as a heads-up but non-blocking by default func alignExitCode(findings []AlignFinding, strict bool) int { for _, f := range findings { - if f.Severity == "FAIL" { + if f.Severity == "FAIL" || f.Severity == "ERROR" { return 1 } if strict && f.Severity == "WARN" { @@ -268,11 +278,13 @@ func renderAlignMarkdown(findings []AlignFinding) string { } sb.WriteString("\n") - var failCount, warnCount int + var failCount, errorCount, warnCount int for _, f := range findings { switch f.Severity { case "FAIL": failCount++ + case "ERROR": + errorCount++ case "WARN": warnCount++ } @@ -282,6 +294,9 @@ func renderAlignMarkdown(findings []AlignFinding) string { if failCount > 0 { parts = append(parts, fmt.Sprintf("%d FAIL", failCount)) } + if errorCount > 0 { + parts = append(parts, fmt.Sprintf("%d ERROR", errorCount)) + } if warnCount > 0 { parts = append(parts, fmt.Sprintf("%d WARN", warnCount)) } diff --git a/cmd/wfctl/infra_align_rules.go b/cmd/wfctl/infra_align_rules.go index b0b599d01..4d7a5b3fb 100644 --- a/cmd/wfctl/infra_align_rules.go +++ b/cmd/wfctl/infra_align_rules.go @@ -16,8 +16,13 @@ import ( // AlignFinding is a single rule violation emitted by an alignment check. type AlignFinding struct { - Rule string // R-A1, R-A2, etc. - Severity string // FAIL or WARN + Rule string // R-A1, R-A2, etc. + // Severity is one of: + // - "FAIL" — always blocks (exit 1) + // - "ERROR" — always blocks (exit 1); used by rules that want to + // carry a fix-suggestion in the message (e.g. R-A9) + // - "WARN" — blocks only under --strict + Severity string Resource string // affected resource name Message string // human-readable description } @@ -686,15 +691,21 @@ func checkRA8(ctx *alignContext) []AlignFinding { // ── R-A9: suspicious provider_credential key suffix ──────────────────────── -// checkRA9 warns when a secrets.generate entry with type "provider_credential" -// uses a key that already ends with a known sub-key suffix (e.g. "_access_key", -// "_secret_key"). This pattern means the caller pre-appended the sub-key name -// that bootstrapSecrets will append again, producing double-suffixed storage -// keys such as SPACES_access_key_access_key. The canonical form is to use the -// root key (e.g. "SPACES") and let bootstrapSecrets derive the sub-key names. +// checkRA9 fires (as an ERROR) when a secrets.generate entry with type +// "provider_credential" uses a key that already ends with a known sub-key +// suffix (e.g. "_access_key", "_secret_key"). This is the doubled-create +// anti-pattern: each sub-keyed entry causes bootstrapSecrets to create a +// separate cloud credential, producing an orphaned pair of keys instead of +// a single canonical credential. The canonical form is to use the root key +// (e.g. "SPACES") and let bootstrapSecrets auto-derive the sub-key names +// from providerCredentialSubKeys[source]. // // The rule only fires for sources registered in providerCredentialSubKeys. // Unknown sources are skipped — we cannot predict their sub-key names. +// +// Severity: ERROR (was WARN through rev2 of the spaces-key plan; flipped to +// ERROR in rev3 so `wfctl infra align --strict` blocks deploy when the +// anti-pattern is present). func checkRA9(ctx *alignContext) []AlignFinding { var findings []AlignFinding for _, gen := range ctx.secretGens { @@ -710,11 +721,11 @@ func checkRA9(ctx *alignContext) []AlignFinding { if strings.HasSuffix(gen.Key, suffix) { findings = append(findings, AlignFinding{ Rule: "R-A9", - Severity: "WARN", + Severity: "ERROR", Resource: gen.Key, Message: fmt.Sprintf( - "provider_credential key %q ends with auto-generated suffix %q — use root key (e.g. %q) and let bootstrapSecrets derive sub-keys", - gen.Key, suffix, strings.TrimSuffix(gen.Key, suffix), + "provider_credential key %q ends in %q; use canonical key %q — bootstrap auto-derives the sub-keys for source %q", + gen.Key, suffix, strings.TrimSuffix(gen.Key, suffix), gen.Source, ), }) break // one finding per gen entry is enough diff --git a/cmd/wfctl/infra_align_test.go b/cmd/wfctl/infra_align_test.go index 0b969ceed..8b44527a2 100644 --- a/cmd/wfctl/infra_align_test.go +++ b/cmd/wfctl/infra_align_test.go @@ -883,6 +883,41 @@ func TestInfraAlign_ExitCode_WarnStrict(t *testing.T) { } } +// TestAlignExitCode_ErrorSeverity_Returns1 verifies that ERROR severity +// (introduced in rev3 of the spaces-key plan for R-A9) blocks deploy with +// exit 1 even without --strict. Without this, an ERROR finding silently +// downgrades to non-blocking — defeating the rev3 requirement that the +// doubled-create anti-pattern fail CI. +func TestAlignExitCode_ErrorSeverity_Returns1(t *testing.T) { + findings := []AlignFinding{ + {Rule: "R-A9", Severity: "ERROR", Resource: "SPACES_access_key", Message: "doubled-create"}, + } + if code := alignExitCode(findings, false); code != 1 { + t.Errorf("alignExitCode(ERROR, strict=false) = %d, want 1 — ERROR must always block", code) + } + if code := alignExitCode(findings, true); code != 1 { + t.Errorf("alignExitCode(ERROR, strict=true) = %d, want 1", code) + } +} + +// TestAlignRender_ErrorSeverity_CountedInSummary verifies that the markdown +// summary includes ERROR alongside FAIL/WARN counts. Without this, ERROR +// findings would render in the table but be invisible in the summary line, +// hiding the deploy-blocking signal from CI consumers. +func TestAlignRender_ErrorSeverity_CountedInSummary(t *testing.T) { + findings := []AlignFinding{ + {Rule: "R-A9", Severity: "ERROR", Resource: "SPACES_access_key", Message: "doubled-create"}, + {Rule: "R-A6", Severity: "WARN", Resource: "nats", Message: "advisory"}, + } + out := renderAlignMarkdown(findings) + if !strings.Contains(out, "1 ERROR") { + t.Errorf("summary should report '1 ERROR', got: %s", out) + } + if !strings.Contains(out, "1 WARN") { + t.Errorf("summary should still report '1 WARN', got: %s", out) + } +} + // ── R-A9: suspicious provider_credential key ────────────────────────────────── // TestCheckRA9_SuspiciousProviderCredentialKey is a table-driven unit test @@ -939,8 +974,8 @@ func TestCheckRA9_SuspiciousProviderCredentialKey(t *testing.T) { if tc.wantFinding && findings[0].Rule != "R-A9" { t.Errorf("expected Rule=R-A9, got %q", findings[0].Rule) } - if tc.wantFinding && findings[0].Severity != "WARN" { - t.Errorf("expected Severity=WARN, got %q", findings[0].Severity) + if tc.wantFinding && findings[0].Severity != "ERROR" { + t.Errorf("expected Severity=ERROR, got %q", findings[0].Severity) } }) } @@ -973,8 +1008,16 @@ modules: if err != nil { t.Fatalf("unexpected error: %v", err) } - if !findingsHaveRuleAndSeverity(findings, "R-A9", "WARN") { - t.Errorf("expected R-A9 WARN, got: %v", findings) + if !findingsHaveRuleAndSeverity(findings, "R-A9", "ERROR") { + t.Errorf("expected R-A9 ERROR, got: %v", findings) + } + // End-to-end: ERROR severity must produce a non-zero exit code regardless + // of --strict (the user-visible CI gate, not just the severity label). + if code := alignExitCode(findings, false); code != 1 { + t.Errorf("alignExitCode(strict=false) = %d, want 1 — R-A9 ERROR must block without --strict", code) + } + if code := alignExitCode(findings, true); code != 1 { + t.Errorf("alignExitCode(strict=true) = %d, want 1", code) } } @@ -1004,6 +1047,36 @@ secrets: } } +// TestInfraAlign_RA9_CanonicalSingleEntry_Passes is the positive happy-path fixture for +// the R-A9 severity flip (rev3): the canonical single-entry SPACES key with +// no doubled-create anti-pattern must pass `wfctl infra align --strict` with +// exit code 0 and produce zero R-A9 findings. +// +// This is the inverse of TestInfraAlign_RA9_SuspiciousKey_Fires: it locks in +// that the rule does not regress into false positives once it fires as ERROR. +func TestInfraAlign_RA9_CanonicalSingleEntry_Passes(t *testing.T) { + yaml := ` +secrets: + generate: + - key: SPACES + type: provider_credential + source: digitalocean.spaces + name: my-deploy-key +` + cfg := writeAlignYAML(t, yaml) + opts := alignOptions{configFile: cfg, strict: true} + findings, err := runInfraAlignChecks(opts) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if findingsHaveRule(findings, "R-A9") { + t.Fatalf("canonical shape should not trigger R-A9; got: %+v", findings) + } + if code := alignExitCode(findings, true); code != 0 { + t.Fatalf("canonical shape should pass --strict; got exit=%d, findings=%+v", code, findings) + } +} + func TestInfraAlign_RenderMarkdown(t *testing.T) { findings := []AlignFinding{ {Rule: "R-A6", Severity: "WARN", Resource: "nats-broker", Message: "internal service should use expose: internal"}, diff --git a/docs/WFCTL.md b/docs/WFCTL.md index 185024f4e..92f81c324 100644 --- a/docs/WFCTL.md +++ b/docs/WFCTL.md @@ -1437,8 +1437,14 @@ WFCTL_REFRESH_OUTPUTS=1 wfctl infra apply --auto-approve --skip-refresh -c infra #### `infra align` Run a battery of static alignment checks against a config (and optionally a -plan). Each rule (`R-A1` … `R-A10`) emits findings as a `FAIL` (always -non-zero exit) or `WARN` (non-zero only with `--strict`). +plan). Each rule (`R-A1` … `R-A10`) emits findings at one of three severity +tiers: + +- `FAIL` — deterministic failure; always non-zero exit (e.g. unresolved env var). +- `ERROR` — hard rule violation; always non-zero exit, equivalent to `FAIL` + for exit-code purposes. Used by rules that pair the violation with a + fix-suggestion in the message (e.g. `R-A9`). +- `WARN` — advisory; non-zero exit only with `--strict`. ``` wfctl infra align [--config ] [--env ] [--plan ] [--strict] [--strict-health] [--strict-cidr] [--max-changes N] @@ -1449,7 +1455,7 @@ wfctl infra align [--config ] [--env ] [--plan ] [--strict | `--config`, `-c` | _(auto-detected)_ | Config file (searches `infra.yaml`, `config/infra.yaml`) | | `--env` | `` | Environment name for per-env config resolution | | `--plan` | `` | Path to a plan JSON file. Enables `R-A7` (plan-output sanity) and `R-A10` (provider `ValidatePlan` dispatch). | -| `--strict` | `false` | Treat all `WARN` findings as `FAIL` (exit 1) | +| `--strict` | `false` | Treat all `WARN` findings as failing (exit 1). `FAIL` and `ERROR` always block regardless of this flag. | | `--strict-health` | `false` | Treat `R-A2` health-check `WARN`s as `FAIL` | | `--strict-cidr` | `false` | Enable strict CIDR overlap checks (reserved) | | `--max-changes` | `50` | Warn when the plan has more than N actions | @@ -1464,7 +1470,7 @@ wfctl infra align [--config ] [--env ] [--plan ] [--strict | R-A6 | Network/exposure alignment | FAIL or WARN | | R-A7 | Plan-output sanity (requires `--plan`) | FAIL or WARN | | R-A8 | WebAuthn RP_ID alignment | FAIL | -| R-A9 | Suspicious `provider_credential` key suffix | WARN | +| R-A9 | Suspicious `provider_credential` key suffix (doubled-create anti-pattern) | ERROR | | R-A10 | Provider `ValidatePlan` diagnostics (requires `--plan`) | FAIL or WARN | **R-A10 — provider-side cross-resource validation.** When `--plan` is given,