feat(lint): harden R-A9 from warning to error (PR1: Tasks 3+4)#583
Merged
Conversation
Flips the table-driven assertion in TestCheckRA9_SuspiciousProviderCredentialKey from `Severity=WARN` to `Severity=ERROR` (rev3 of the spaces-key plan), and adds TestRA9_CanonicalSingleEntry_Passes as the positive happy-path fixture verifying that a canonical single-entry SPACES key passes `--strict`. This is a deliberately failing test for Task 3 of the spaces-key-iac-resource plan; Task 4 will flip the rule severity in infra_align_rules.go to make it pass. Task 4 will also need to update TestInfraAlign_RA9_SuspiciousKey_Fires (line 976) which still asserts WARN at the integration level. Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7), Task 3 of PR1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates wfctl infra align test coverage around rule R-A9 (suspicious provider_credential secret key shape) as part of the spaces-key-iac-resource plan, by asserting the planned severity change and adding a strict-mode “canonical key” fixture.
Changes:
- Change the unit test
TestCheckRA9_SuspiciousProviderCredentialKeyto expectSeverity=ERROR(currently fails until the rule implementation is updated). - Add a new end-to-end strict-mode test ensuring the canonical single-entry
SPACESkey produces no R-A9 findings and exits 0.
Comment on lines
+942
to
+943
| if tc.wantFinding && findings[0].Severity != "ERROR" { | ||
| t.Errorf("expected Severity=ERROR, got %q", findings[0].Severity) |
Comment on lines
+1007
to
+1014
| // TestRA9_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 TestRA9_CanonicalSingleEntry_Passes(t *testing.T) { |
Implements Task 4 of the spaces-key-iac-resource plan
(docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7).
Changes:
- cmd/wfctl/infra_align_rules.go: flip checkRA9 severity from "WARN" to
"ERROR" and rewrite the diagnostic message with a fix-suggestion
("...use canonical %q (auto-derives sub-keys via
providerCredentialSubKeys[%q])"). With this change `wfctl infra align
--strict` exits non-zero whenever a secrets.generate entry has a
`provider_credential` key ending in a known sub-key suffix
(`_access_key`, `_secret_key`, ...). This blocks the doubled-create
anti-pattern at lint time, before plan/apply touches the cloud.
- cmd/wfctl/infra_align_test.go:976: update
TestInfraAlign_RA9_SuspiciousKey_Fires to assert "ERROR" instead of
"WARN" (the unit-level table test was flipped to ERROR by Task 3;
the integration-level helper still asserted WARN, which would have
regressed under the new rule severity).
- docs/WFCTL.md R-A9 rule table row: severity column WARN → ERROR with a
clarifying parenthetical ("doubled-create anti-pattern"). The plan
referenced docs/dsl-reference-embedded.md but that file does not exist
in the repo; the rule table that documents R-A9 lives in WFCTL.md.
Verification:
- GOWORK=off go test ./cmd/wfctl -run "TestRA9|TestCheckRA9|TestInfraAlign_RA9" -v
→ all 4 tests PASS
- GOWORK=off go test ./cmd/wfctl -count=1 → entire wfctl suite PASS
- GOWORK=off go build ./... → clean
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… prefix Renames TestRA9_CanonicalSingleEntry_Passes to TestInfraAlign_RA9_CanonicalSingleEntry_Passes per Copilot review (PR #583 inline comment, line 1014). Aligns with the existing TestInfraAlign_RA9_* naming convention used by sibling end-to-end tests in the same file (TestInfraAlign_RA9_SuspiciousKey_Fires, TestInfraAlign_RA9_CleanKey_DoesNotFire). No behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
intel352
changed the title
Contributor
Author
|
Addressed Copilot inline review (commit 22cc1a7): Line 1014 (test naming convention) — fixed. Renamed Line 943 (CI red until rule changes) — by design, will not change. This is the failing-test commit in a TDD pair (Task 3 of PR1). Task 4's implementation commit (288f68d) now stacks on top of Task 3's commit on this same branch, so the PR as a whole goes green when both commits are in place. Verified locally: Per the spaces-key-iac-resource plan (PR1 = Task 3 failing test + Task 4 implementation, same branch, single PR), CI gates only on the merged-PR state, not on each individual commit. |
Comment on lines
717
to
721
| findings = append(findings, AlignFinding{ | ||
| Rule: "R-A9", | ||
| Severity: "WARN", | ||
| Severity: "ERROR", | ||
| Resource: gen.Key, | ||
| Message: fmt.Sprintf( |
| 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 %q (auto-derives sub-keys via providerCredentialSubKeys[%q])", |
| | 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 | |
Comment on lines
972
to
+976
| findings, err := runInfraAlignChecks(opts) | ||
| 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") { |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Closes the scope gap flagged by code-reviewer + Copilot on Task 4 (implementer-1's commit 288f68d) of the spaces-key-iac-resource plan: the R-A9 rule now emits Severity="ERROR", but alignExitCode, runInfraAlign's exit-code error formatting, and renderAlignMarkdown all only recognized "FAIL" and "WARN". Result: R-A9's flip from WARN → ERROR silently downgraded to non-blocking — a hidden regression on the rev3 requirement that the doubled-create anti-pattern fail CI without --strict. Changes: - alignExitCode: treat ERROR identically to FAIL (always blocks; --strict irrelevant). Documented severity contract in the function comment. - runInfraAlign: include ERROR in the failCount tallied for the error message after a non-zero exit. - renderAlignMarkdown: count ERROR alongside FAIL/WARN; surface "N ERROR" in the summary line so CI consumers see the deploy-blocking signal. - AlignFinding doc comment: enumerate the three severities + their blocking semantics so future rule authors don't repeat this mistake. New tests: - TestAlignExitCode_ErrorSeverity_Returns1: ERROR returns exit 1 with strict=false AND strict=true (regression sentinel). - TestAlignRender_ErrorSeverity_CountedInSummary: summary line includes "1 ERROR" alongside "1 WARN". Smoke-tested locally: $ /tmp/wfctl infra align -c /tmp/test-ra9.yaml … | R-A9 | ERROR | SPACES_access_key | … | 1 ERROR error: align: 1 finding(s) require attention exit=1 Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7), extending Task 4 of PR1 per code-reviewer scope finding. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Scope extension — closes the gap flagged by code-reviewer (and Copilot independently) on Task 4's implementation: Commit 33df18c:
Smoke-tested: Full alignment suite stays green: |
Addresses three Copilot inline review comments on PR #583: 1. cmd/wfctl/infra_align_rules.go:727 — R-A9 diagnostic message no longer leaks the internal `providerCredentialSubKeys[%q]` Go symbol. Reworded to user-facing language: "use canonical key %q — bootstrap auto-derives the sub-keys for source %q". 2. docs/WFCTL.md (`infra align` section) — replaces the FAIL-or-WARN-only prose with an explicit three-tier severity table that documents ERROR's semantics (always non-zero exit, equivalent to FAIL for the gate, used for fix-suggestion-bearing rules like R-A9). Updates the --strict flag row to clarify that FAIL/ERROR always block regardless of --strict. 3. cmd/wfctl/infra_align_test.go:1014 — extends TestInfraAlign_RA9_SuspiciousKey_Fires to assert the user-visible CI gate behavior (alignExitCode returns 1 for both strict=false and strict=true), not just the severity label. Pairs the rule severity assertion with the exit-code assertion at the same boundary. All RA9/RA10 + alignment fixture suites stay green: go test ./cmd/wfctl -run "TestInfraAlign|TestCheckRA|TestAlign" → ok Smoke-tested via built wfctl: message + exit code both render correctly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Copilot round 2 addressed (commit 3ee2594):
Smoke-tested: Full alignment suite green: |
Comment on lines
+707
to
+708
| // ERROR in rev3 so `wfctl infra align --strict` blocks deploy when the | ||
| // anti-pattern is present). |
Comment on lines
726
to
729
| 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, | ||
| ), |
Comment on lines
+886
to
+891
| // 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) { |
Comment on lines
+903
to
+909
| // 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"}, |
4 tasks
intel352
added a commit
that referenced
this pull request
May 9, 2026
#586) Records the closeout for Tasks 5+6 of the spaces-key-iac-resource plan (docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7). The plan's PR2 was specified as a migration of core-dump/infra.yaml from a two-entry SPACES_access_key/SPACES_secret_key provider_credential schema to canonical single-entry. At impl time, verification against origin/main HEAD 3bb46833 showed the file already had the canonical shape — the migration had landed in PR #190 (TC1 cutover) or PR #194 (TC2 cutover) before the plan was authored. Smoke-confirmed: `wfctl infra align --strict -c infra.yaml --env staging` returns exit 0 with "No alignment issues found." — no R-A9 firing because there's nothing to fire on. Tasks 5+6 are marked completed as a no-op confirmation. PR1's R-A9 severity flip (workflow #583, merged) provides the ongoing regression protection: any future reintroduction of the two-entry shape will hit ERROR R-A9 at align-strict time, exit 1. ADR also captures the planner-blindspot lesson (operator memory about file shapes is unreliable; re-fetch origin/main before locking a plan that mutates external repo files) for the post-merge retro. Per team-lead's user-direction routing of the (a/b/c) options I had surfaced. 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
PR1 of the spaces-key-iac-resource plan
(
docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7) —two tasks land here as a TDD pair:
TestCheckRA9_SuspiciousProviderCredentialKeyfromWARNtoERROR,adds new positive fixture
TestRA9_CanonicalSingleEntry_Passes.WARNtoERRORincmd/wfctl/infra_align_rules.goand rewrites thediagnostic with a fix-suggestion. Also updates
TestInfraAlign_RA9_SuspiciousKey_Fires(line 976) which still assertedWARN at the integration level, plus the R-A9 row in
docs/WFCTL.md.After this lands,
wfctl infra align --strictexits non-zero whenever asecrets.generateentry has aprovider_credentialkey ending in a knownsub-key suffix (
_access_key,_secret_key, ...) for a source registeredin
providerCredentialSubKeys— blocking the doubled-create anti-patternat lint time, before plan/apply touches the cloud.
Test plan
GOWORK=off go test ./cmd/wfctl -run "TestRA9|TestCheckRA9|TestInfraAlign_RA9" -v→ all 4 PASSGOWORK=off go test ./cmd/wfctl -count=1→ entire wfctl suite PASSGOWORK=off go build ./...→ cleancore-dump/infra.yamlto canonical single-entry shape so it passes--strictunder the new error severity🤖 Generated with Claude Code