feat(codemod): cmd/iac-codemod with 4 modes + workspace migration target (W-8 of 12)#538
Merged
Conversation
…cher T8.1: Adds cmd/iac-codemod skeleton with dispatcher for the four codemod modes — refactor-plan, refactor-apply, add-validate-plan, lint — and the shared -dry-run / -fix flag pair. Modes are registered via a map of modeFunc entries so subsequent tasks (T8.2-T8.5) can wire in real implementations file-by-file. Each mode currently delegates to a stub that prints a "not yet implemented" message and exits zero. Defaults: -dry-run is true; -fix opts into mutation and forces -dry-run to false. Unknown modes return exit 2 with usage. The // wfctl:skip-iac-codemod marker convention is documented in the package doc and usage text. Tests cover dispatch, default flag values, -fix semantics, unknown-mode handling, help routing, and positional-arg forwarding via a swappable modes map (no subprocess required). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iew) Addresses spec-reviewer findings on b76ab2f: 1. (BLOCKER) Extract `const SkipMarker = "// wfctl:skip-iac-codemod"` so T8.3-T8.5 parsers reference the canonical literal in one place. Plan rev2 (line 2400) unifies the four modes on this single marker specifically to prevent mismatched-marker silent-no-op surfaces; the const + TestSkipMarker_LiteralPinned + TestUsage_MentionsSkipMarker guards close the drift hole the reviewer flagged. usage() now formats the marker via the const rather than a duplicated string literal. 2. (MINOR) usage() documents the stdlib flag-parser ordering constraint (flags must precede paths). TestRun_FlagAfterPath_SilentlyTreatedAsPositional pins the failure mode so it is intentional, not a parser bug, and so future maintainers see the constraint exercised in tests. 3. (NIT) stubMode's unused args parameter renamed to _; cosmetic only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Spec-reviewer round-2 finding (commit 26ac916): the dispatcher only forced DryRun=false on -fix, but did NOT prevent a user-supplied -dry-run=false from leaving the gate open. With the natural mode predicate `if !opts.DryRun { mutate() }`, this would silently bypass the explicit -fix gate that plan §W-8 line 2347 names as the sole mutation entry point ("-dry-run flag default true; -fix opts into mutation"). Fix: normalize the gate at the dispatcher boundary — when Fix is set, DryRun=false; when Fix is unset, DryRun=true regardless of what the user passed via -dry-run=. Fix is now the single source of truth for "may I mutate?", so any natural mode predicate is safe by construction. Options.DryRun's doc comment now states this contract explicitly so T8.2-T8.5 implementers cannot reach for the wrong predicate. Tests pin all three cases: - -dry-run=false alone → DryRun stays true (the bypass) - -fix -dry-run=false → mutation authorized (Fix wins) - -dry-run=true -fix → mutation authorized (Fix wins) Also adds TestPackageDoc_MentionsSkipMarker (process note #6) — cheap file-content guard so a future SkipMarker rename trips a test rather than silently desyncing the package doc comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (T8.1 review #5) Code-reviewer round-3 authorized now-fix: tests in this file mutate the package-global `modes` map under defer-restore. -race is currently clean because no test calls t.Parallel(), but the swap-and-restore pattern is a latent data race the next agent (T8.2-T8.5) could trigger by adding parallelism. Top-of-file guard comment names the constraint and points at the dependency-injection refactor as the unlock path if parallelism is ever required. Comment-only change; tests still pass with -race. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
T8.2: Wires the lint subcommand using golang.org/x/tools/go/analysis
with the four assertions named in plan §T8.2:
AssertPlanDelegatesToHelper — provider Plan() delegates to wfctlhelpers.Plan
AssertApplyDelegatesToHelper — provider Apply() delegates to wfctlhelpers.ApplyPlan
AssertDiffSetsNeedsReplaceForForceNew — driver Diff() sets NeedsReplace on ForceNew
AssertProviderImplementsValidatePlan — provider satisfies ProviderValidator
Carry-forwards from T8.1 review baked in:
1. Dispatcher fs.Usage override (main.go:run) so `iac-codemod <mode> -h`
produces the global usage rather than the per-FlagSet banner.
Pinned by TestRun_HelpAfterMode_PrintsGlobalUsage across all 4 modes.
2. Mutation-gate negative test pinning lint-is-read-only-by-definition:
TestRunLint_DoesNotMutateFilesEvenWithFixFlag invokes lint with
hostile {Fix:true, DryRun:false} flags and asserts mtime + content
unchanged. Plus TestRunLint_FixFlag_WarnsItHasNoEffect surfaces a
warning so users know -fix did nothing.
3. Skip-marker honored at func-doc and type-doc levels via
hasSkipMarkerOn(fn.Doc) / ts.Doc; skipped sites flow through the
pass.Report channel with a [skipped] prefix and are split into a
separate report section by lintReport.unpackSkippedFromFindings.
Plan rev2 (line 2400) requires each mode to surface a list of
skipped sites in its report — pinned by TestRunLint_SkipMarker_SurfacedInReport.
Precision: all helper-call analyzers gate on providerLikeReceivers
(method set must contain BOTH Plan + Apply matching IaCProvider shape)
to avoid false-positive flags on deploy targets and other Apply-shaped
types. Manual verification against the workflow repo went from 9
findings (incl. 2 false positives in pkg/k8s) down to 7 (all genuine
provider implementations awaiting v2 migration).
Implementation notes:
- File-by-file analysis via parser.ParseFile + tolerant types.Check
(stub importer ignores unresolved imports). This works on plugin
sources that haven't vendored their dependencies. Cross-file
references won't resolve, but IaC providers and drivers are
typically co-located by Go convention.
- Skip-marker is encoded as a synthetic diagnostic with a `[skipped]`
prefix; the driver post-processes it out of the findings list. This
keeps the analyzer API surface to one channel.
- go.mod: promotes golang.org/x/tools from indirect to direct. No
new modules, no go.sum changes.
Verification: 33/33 tests pass with -race; binary smoke-tested against
workflow repo root (7 findings, exit 1).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…filter Spec-reviewer round-2 on commit 2908fa1; addresses 5 substantive findings + 1 nit (findings 5 & 6 are PR-body notes, no code change): 1. (BLOCKER) `iac-codemod <mode> -h` now prints the global usage to STDOUT, matching `iac-codemod -h` and the kubectl/git/gh convention for help-on-success. Previously it landed on STDERR via the FlagSet's SetOutput handler. Pinned by TestRun_HelpAfterMode_PrintsGlobalUsageToStdout — asserts stream specifically rather than the union of stdout+stderr (the prior test would have passed even with stderr output). Parse-error noise still flows through stderr; only the help-text body moved to stdout. 2. (MEDIUM) hasSkipMarkerOn now accepts a trailing space + arbitrary justification text after SkipMarker: // wfctl:skip-iac-codemod legacy upsert recovery, see ADR-042 Annotating WHY a site is skipped is a Go idiom; silently ignoring the marker because of trailing context would replicate the exact silent-no-op surface plan rev2 line 2400 unifies the marker to prevent. Two new tests pin both sides of the contract: - TestSkipMarker_AcceptsTrailingJustification - TestSkipMarker_RejectsCloseButWrongMarker (negative — the legacy `// wfctl:skip-codemod` prefix from design rev1 must still flag the diagnostic) 3. (MEDIUM) AssertDiffSetsNeedsReplaceForForceNew now gates on a new driverLikeReceivers helper (method set must contain Diff AND at least one canonical companion: Read/Create/Update/Delete). Brings the analyzer in line with the precision treatment Plan/Apply already had via providerLikeReceivers. New TestAssertDiffSetsNeedsReplaceForForceNew_NonDriverNotFlagged pins the negative case (a SettingsDiff struct with just Diff() is correctly invisible to the analyzer). 4. (LOW-MEDIUM) bodyAssignsFieldTrue → bodyAssignsField: the matcher now accepts ANY RHS, not just literal `= true`. The terser canonical pattern `r.NeedsReplace = c.ForceNew` is equally valid expression of the W-3 force-new contract; flagging it was a false positive previously hit by cmd/wfctl/deploy_providers.go remoteResourceDriver (which propagates NeedsReplace from a gRPC response via `result.NeedsReplace, _ = res["needs_replace"].(bool)`). Pinned by TestAssertDiffSetsNeedsReplaceForForceNew_AcceptsDirectAssign. 7. (NIT) Removed dead/misleading comment in lintFile that referenced a never-implemented passSkippedSink scratch field. Findings 5 & 6 (no code change — PR-body notes for team-lead): 5. Plan §T8.2 line 2363 says `golang.org/x/tools/go/analysis/passes` framework, but `/passes` is the directory of canonical reusable analyzers. The actual framework is `golang.org/x/tools/go/analysis` (which is what we import). Likely a plan typo; flag for post-merge retrospective. 6. go.mod promotes golang.org/x/tools from indirect to direct. Already-transitive dep, no go.sum changes, no new modules. Should be fine but flagged for team-lead per W-7 trigger-list rigor. Smoke-test re-verification on workflow repo: 6 genuine findings (down from 7), zero false positives. -h now correctly streams to stdout for both top-level and per-mode invocations. 37/37 tests pass with -race; build clean; vet clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…se guard, adjacent-suffix rejection
…ite); honors // wfctl:skip-iac-codemod marker
…ports; honors // wfctl:skip-iac-codemod marker
…// wfctl:skip-iac-codemod marker
…core-prefixed dirs from walk
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new cmd/iac-codemod CLI intended to lint and mechanically migrate IaC provider implementations toward the newer helper-based wfctl flow, plus a Makefile target for running it across sibling plugin workspaces.
Changes:
- Adds the
iac-codemodcommand with four modes:lint,refactor-plan,refactor-apply, andadd-validate-plan. - Adds AST-based rewrites/reporting plus extensive mode-specific tests for dry-run, fix, skip-marker, and idempotency behavior.
- Adds Makefile targets to build the codemod and run advisory provider migration checks across sibling plugin repos.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds build/cleanup targets and advisory migrate-providers workflow. |
go.mod |
Promotes golang.org/x/tools to a direct dependency for analyzers. |
cmd/iac-codemod/main.go |
Adds the codemod CLI entrypoint, shared flags, usage, and walk filtering. |
cmd/iac-codemod/main_test.go |
Tests CLI dispatch, flag normalization, help output, and marker docs. |
cmd/iac-codemod/lint.go |
Adds four static analyzers and lint report generation. |
cmd/iac-codemod/lint_test.go |
Tests analyzer behavior, skip markers, and lint integration. |
cmd/iac-codemod/refactor_plan.go |
Adds Plan-body classification, rewrite, import management, and reporting. |
cmd/iac-codemod/refactor_plan_test.go |
Tests Plan detection, rewriting, idempotency, and mutation gating. |
cmd/iac-codemod/refactor_apply.go |
Adds Apply-body classification, reporting, optional report-file output, and rewrite logic. |
cmd/iac-codemod/refactor_apply_test.go |
Tests Apply rewrite cases, non-canonical detection, and report-file output. |
cmd/iac-codemod/add_validate_plan.go |
Adds ValidatePlan stub detection/insertion and reporting. |
cmd/iac-codemod/add_validate_plan_test.go |
Tests stub insertion, skip handling, idempotency, and mutation gating. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Round-1 review on PR #538 surfaced 11 substantive findings; all addressed: Critical (real bugs that broke compile or silently dropped logic): #1 [lint, refactor-plan] Rewrite target wrong — `wfctlhelpers.Plan` does not exist in the repo today. Pivoted to `platform.ComputePlan` (the real helper at platform/differ.go:72). Both targets now accepted by the lint analyzer for forward-compat with rev0 fixtures. Plan-doc §T8.3 named the wrong helper; flagged for retro. #2 [refactor-plan] rewritePlanBody only renamed `_` ctx params. A method declared `Plan(c context.Context, ...)` would be rewritten referencing undefined `ctx`. Now: any non-blank ctx-name preserved; only blank `_` renamed to `ctx`. #3 [refactor-plan] isCanonicalPlanBody too loose — extra side-effects inside the desired loop still classified as canonical. Tightened to require exactly the 3-statement template (lookup + !exists guard + configHash compare), no else branches, no trailing junk. Regression test: TestRefactorPlan_ExtraLoggingNotCanonical. #4 [refactor-plan, refactor-apply] SkipMarker only consulted on fn.Doc. PR description promised type-doc + GenDecl-doc honoring. Added receiverTypeDocs + carriesMarker; both modes now check all 3 doc levels. #5 [refactor-apply] hasCanonicalCases only checked case labels. Bespoke bookkeeping inside a case body (logging, metrics, alternate driver calls) classified as canonical and would be silently dropped on -fix. Added caseBodyIsCanonical whitelist (driver call, ResourceRef construction, ProviderID guard). Regression test: TestRefactorApply_ExtraBookkeepingNotCanonical. #6 [refactor-apply] custom-error-wrapping suggestion named fictional APIs (ApplyResultErrorHook / WrapActionError). Replaced with honest hand-port advice: skip-marker + manual switch, OR move wrap into driver methods so wfctlhelpers records it verbatim. #7 [add-validate-plan] Stub always emitted unqualified `*IaCPlan` / `[]PlanDiagnostic`. Files importing the interfaces module under a qualifier (e.g. `*interfaces.IaCPlan`) failed to compile after -fix. Added interfacesQualifier detector + qualified stub emission. Regression: TestAddValidatePlan_Fix_QualifiedSignature. #8 [add-validate-plan, lint] hasValidatePlanMethod / AssertProviderImplementsValidatePlan checked method NAME only. Wrong-signature ValidatePlan (e.g. takes a string) was treated as compliant even though interfaces.ProviderValidator wouldn't be satisfied. Added validatePlanSignatureMatches: shape-checks the receiver param + return slice (qualified-or-unqualified). Both callers now use it. Regression: TestAddValidatePlan_DryRun_FlagsWrongSignature. #9 [refactor-plan, refactor-apply, add-validate-plan] Single-file pass — providers whose Plan + Apply lived in sibling files were silently omitted. Added planLikeReceiversInDir: directory-wide method-set scan. Per-file fallback retained for isolated single- file targets. Important: #10 [lint] Per-file parse/type-check errors accumulated in report.errors but exit code stayed 0 if there were no findings — green CI hid coverage gaps. Now exits 1 on either findings OR errors. #11 [refactor-apply] -report-file mode flag never appeared in usage text. Documented in main.go's global usage block (the `-h` path intercepts before the per-mode FlagSet). Plan-doc gap surfaced for retro: §T8.3 line 2373 reads "replaces with `return wfctlhelpers.Plan(ctx, p, desired, current)`", but no such function exists; reality is `platform.ComputePlan`. Recurring defect class (plan-literal vs reality gap, W-4/W-5/W-7/W-9/W-8). Documented in planHelperImportPath docstring + this commit body.
Round 2 surfaced 9 substantive findings; all addressed: Critical (compile-break / contract-break): #1 [refactor-plan, lint] platform.ComputePlan returns IaCPlan BY VALUE, but provider Plan methods return *IaCPlan. Single-statement `return platform.ComputePlan(...)` rewrite produced uncompilable code. Switched to canonical 2-statement form: plan, err := platform.ComputePlan(ctx, p, desired, current) return &plan, err isAlreadyDelegatedPlanBody widened to recognise both the new shape and the legacy single-statement forms (idempotent across revs). #3 [refactor-plan] rewritePlanBody fell back to recvName="p" but didn't update the receiver decl when the source had an unnamed receiver (`func (*Provider) Plan(...)`). Rewritten call referenced undefined `p`. Added ensureReceiverName: injects identifier and mutates the AST. Regression: TestRefactorPlan_Fix_UnnamedReceiverGetsName. Also added: TestRefactorPlan_Fix_PreservesCustomCtxName for round-1 finding #2 (custom ctx name preserved). #4 [refactor-apply] Same unnamed-receiver bug as #3. Same fix (ensureReceiverName + ensureCtxParamName + ensureNthParamName helpers shared with refactor-plan). Regression: TestRefactorApply_Fix_UnnamedReceiverGetsName. #5 [add-validate-plan] Stub always emitted `func (p *T) ValidatePlan(...)` even when the type used value receivers. Method-set mismatch made the type fail interfaces.ProviderValidator type assertion. Added providerReceiverConvention + receiverIsPointer; stub now matches the existing Plan/Apply convention. Regression: TestAddValidatePlan_Fix_ValueReceiverConvention. Important (skip-marker not honored in lint, single-file pass): #6 [lint] AssertPlanDelegatesToHelper checked fn.Doc only, ignoring type-doc and GenDecl-doc skip markers. Added receiverTypeDocsForPass helper; analyzer now checks all 3 doc levels. #7 [lint] AssertApplyDelegatesToHelper — same fix as #6. #8 [lint] AssertDiffSetsNeedsReplaceForForceNew — same fix as #6. #9 [lint] lintFile passed only the target file to the analyzers, so cross-file method sets were invisible (same blind spot the refactor-* modes had in round 1). Now lintFile loads sibling non-test .go files from the same package directory and feeds the full slice to each analyzer; diagnostics for sibling files are dropped (the outer walker visits them in their own turn) so no duplicate findings. All 4 modes now compile-clean rewrites + honor 3-level skip-marker + package-aware method-set detection.
Round 3 surfaced 7 substantive findings; all addressed: Critical (compile-break / silent data loss): #1 [add-validate-plan] Directory-wide detection only widened `provs` in round 2; methodsByRecv stayed file-local. A provider with ValidatePlan in a sibling file (or value-receiver Plan/Apply declared elsewhere) would receive a duplicate or wrong-receiver stub. Now planLikeProviderMethodsInDir returns both the recv set AND the merged method slice; methodsByRecv carries the package-wide view (deduped by method name). Stub injection still only fires when typeDecls[recv] is non-nil so we never append to a sibling file. #2 [refactor-plan] isCanonicalPlanBody accepted ANY 2-result return statement at the trailing slot. A planner with the canonical scaffold but a bespoke return (cloned plan, propagated error value) would classify as canonical and the bespoke logic would be silently dropped. Tightened to require EXACTLY `return plan, nil`. #3 [refactor-plan] rewritePlanBody hardcoded "desired"/"current" as args. A canonical Plan with renamed params (e.g. `Plan(ctx, specs, state)`) would rewrite to references to undefined identifiers. ensureNthParamName now extracts the actual signature names. #4 [refactor-plan] rewritePlanBody hardcoded "platform" as the call selector. A file using `pf "github.com/.../platform"` wouldn't compile because `platform` is undefined (ensureImport sees the aliased import as satisfying the path check). Added pkgAliasFor helper; rewrite now uses whatever local name the file imports under. #5 [refactor-apply] caseBodyIsCanonical accepted ANY AssignStmt as canonical. Bookkeeping AssignStmts (metrics counters, map updates, accumulators) passed and would be silently dropped. Tightened to a narrow whitelist: multi-target driver call, single-target driver call (LHS=err), composite-literal construction, selector-assignment to ResourceRef-style fields (ProviderID/Name/Type). Anything else rejected. #6 [refactor-apply] Same import-alias issue as #4 for `wfctlhelpers`. pkgAliasFor reused; rewriteApplyBody now uses whatever local name the file imports under. Important: #7 [lint] AssertProviderImplementsValidatePlan checked ts.Doc only, missing markers placed on the wrapping GenDecl. Aligns now with the receiverDoc.carriesMarker pattern used by the other 3 analyzers (round-2 #6/#7/#8). typeDocsByName captures both TypeSpec.Doc and GenDecl.Doc. Round-2 regression tests retained (TestRefactorPlan_Fix_UnnamedReceiverGetsName, TestRefactorPlan_Fix_PreservesCustomCtxName, TestRefactorApply_Fix_UnnamedReceiverGetsName, TestAddValidatePlan_Fix_ValueReceiverConvention). Round-3 fix verified end-to-end against an aliased-import fixture (pf "github.com/.../platform" + wfh "github.com/.../wfctlhelpers"): the rewritten output compiles cleanly under gofmt.
… findings Round 4 surfaced 6 findings, all real. The recurring theme: rev3's pattern detectors were either too loose (accepted bookkeeping shapes as canonical) or too rigid (literal package-name matching, breaking on aliased imports). Fixes: #1 [add-validate-plan] interfacesQualifier(file) returned "" when the type-only file (no Plan/Apply imports) received the stub via cross-file detection (round-3 #1). Stub then emitted unqualified types that wouldn't compile. Now: when the file lacks an interfaces import but ANY sibling does, fall back to "interfaces" qualifier AND inject the interfaces import into the type-file via AST printing (format.Node) before appending the stub. Added siblingUsesInterfacesImport helper. #2 [refactor-apply] isCanonicalCaseAssign accepted ANY composite literal (`x := <CompositeLit>`) as canonical. A bookkeeping struct construction (audit payload, metric envelope) silently passed. Tightened to require the literal type's name (qualified or unqualified) match "ResourceRef". #3 [refactor-apply] isDriverMethodCall only checked selector NAME (Create/Read/Update/Delete). Calls like `helper.Update(...)` or `metrics.Delete(...)` were misclassified as canonical driver dispatch. Added receiver-allowlist check: only `d`, `drv`, or `driver` accepted as driver-bound identifiers (matching the standard `d, err := p.ResourceDriver(...)` pattern in DO/AWS/GCP/Azure). #4 [refactor-apply, refactor-plan] isAlreadyDelegatedApplyBody and isAlreadyDelegatedPlanBody required literal `wfctlhelpers` / `platform` package idents. Files using aliased imports (`wf "..."`, `pf "..."`) were misreported as non-canonical even though they were valid delegations. Both functions now resolve the file's local alias via pkgAliasFor; literal names retained as fallbacks. Same fix for isPlatformComputePlanAssign (the helper inside isAlreadyDelegatedPlanBody). #5 [lint] AssertPlanDelegatesToHelper / AssertApplyDelegatesToHelper selector matchers required literal `platform` / `wfctlhelpers` package names. Same false-positive risk as #4 for aliased imports. Both analyzers now resolve the alias and accept either the aliased OR literal form. #6 [refactor-apply] caseBodyIsCanonical accepted ANY DeclStmt as canonical, so `var x SomeBookkeepingType` declarations passed even though they're exactly the bespoke logic the codemod is supposed to preserve. Tightened via isLocalOutPointerDecl: only `var <name> *<ResourceOutput-suffix>` accepted. Smoke-tested against an aliased-import fixture (`wf "...wfctlhelpers"` + `pf "...platform"`): - refactor-apply correctly classifies as already-delegated (was: misreported as missing-action-switch) - lint reports 0 findings (was: false-positive AssertPlanDelegatesToHelper + AssertApplyDelegatesToHelper)
…ening findings Round 7 surfaced 10 findings; 4 were stale (already fixed in R6). 6 real findings addressed: Critical (compile-break / silent data loss): #1 [refactor-plan] isPlanActionsAppendAssign verified the LHS but not append's first argument. A bespoke `plan.Actions = append(otherSlice, ...)` was misclassified as canonical and the alternate-slice logic silently dropped during rewrite. Now both LHS and append's first arg must reference plan.Actions. #3+#9 [refactor-apply] isCanonicalApplyOuterShape only checked the outer 3-statement scaffold; per-action logic INSIDE the for-loop body (logging, metrics, custom error handling, accumulators) was silently dropped on -fix. Added isCanonicalApplyLoopBody + isCanonicalApplyLoopAssign + isCanonicalApplyLoopIf + isCanonicalApplyLoopIfBodyStmt: every loop-body statement must match a tight whitelist (driver lookup, var-out decl, action switch, err-/out-guard ifs). #7+#8 [add-validate-plan] provs[recv].Pos() panicked when the TypeSpec was nil (cross-file scenario from round-3 #1: type declaration in sibling file). Now defaults Pos to NoPos for nil specs; sort still works (stable on name when Pos ties). Important (cross-file consistency): #4 [add-validate-plan] qualifier fallback to "interfaces" fired based on whether ANY sibling imported interfaces — unreliable if THIS provider uses local types but an unrelated sibling imports interfaces. Replaced with qualifierFromProviderMethods: inspects the provider's OWN Plan/Apply parameter types (directory-wide via round-3 #1) for the qualifier they use. #5 [add-validate-plan] skip-marker check only consulted typeDecls (current file). When Plan/Apply are here but the type with `// wfctl:skip-iac-codemod` lives in a SIBLING file, the marker was ignored. Added siblingTypeDocs lookup via receiverTypeDocsInDir (the round-6 helper). #10 [add-validate-plan] sibling-method merge deduped by method NAME only. If local file has wrong-signature ValidatePlan and sibling has correct one, sibling dropped, hasValidatePlanMethod saw only bad declaration, injected duplicate stub. Replaced with isLocalDuplicate: dedupes by name + parameter arity + result arity, so distinct signatures both survive. Stale findings (already fixed in R6, no action needed): #2 refactor-apply receiverTypeDocsInDir already in place #6 lint receiver-doc lookup already merged via receiverTypeDocsForPass Smoke-tested against DO plugin: refactor-plan reports DOProvider.Plan canonical, refactor-apply reports DOProvider.Apply upsert-recovery with the upsertSupporter suggestion. Output matches T8.7 baseline.
…fier-name findings Round 8 surfaced 9 findings; all addressed: Critical (silent data loss / behavior change): #1 [add-validate-plan] isLocalDuplicate compared by name+arity only. Wrong-signature ValidatePlan(name string) []PlanDiagnostic and correct ValidatePlan(plan *IaCPlan) []PlanDiagnostic have same arity but different types — sibling-correct dropped, duplicate stub injected. Replaced with signature-fingerprint dedupe (signatureFingerprint + typeFingerprint walk all type shapes). #4 [refactor-apply] `default:` case clauses accepted without body inspection. Logging/metrics in default body silently dropped. Added isCanonicalDefaultBody: only `err = fmt.Errorf("unknown action %q", ...)` accepted. #5 [refactor-apply] isCanonicalApplyLoopAssign accepted any `<x>.ResourceDriver(...)`. `helper.ResourceDriver(...)` / `plan.ResourceDriver(...)` falsely classified. Now requires the receiver to match the provider's own receiver identifier (threaded through from classifyApplyBody). #8 [refactor-apply] Bare `if err != nil { continue }` accepted as canonical, but wfctlhelpers ALWAYS records ActionError before continuing — the rewrite would silently change behavior. Now requires the if-body to ALSO append to result.Errors before any continue/break. Important (skip-marker scope + identifier flexibility): #2 [add-validate-plan] Skip-marker check fired on EVERY method's fn.Doc — a marker on Destroy/Status/etc. accidentally suppressed the whole provider's analysis. Restricted to Plan/Apply (the provider-defining methods). #3 [lint] AssertProviderImplementsValidatePlan — same fix as #2. #6 [refactor-plan] Canonical detector hardcoded `current`/`desired` body identifiers. Providers using `state`/`specs` reported non-canonical despite rewriter preserving names. Added nthParamName extraction; isCanonicalPlanBody now takes the actual parameter names. #7 [refactor-apply] Driver-receiver allowlist comment claimed `rd` accepted, but the switch was missing it. Added. #9 [refactor-apply] Canonical detector hardcoded `result` /`plan` identifier names. Providers using `res` /`pl` rejected. Now recovers actual identifier from signature (planName) and from statement-1 LHS (resultName); both must be consistent within the body but can be any identifier. Smoke-tested against DO plugin: refactor-plan / refactor-apply still report DOProvider.Plan canonical / DOProvider.Apply upsert-recovery with stable upsertSupporter suggestion. Output matches T8.7 baseline. Removed redundant TestRefactorApply_Fix_UnnamedReceiverGetsName: the unnamed-receiver path can't have a canonical-shape Apply body (`<recv>.ResourceDriver(...)` requires recv in scope). Receiver-name injection is shared between refactor-plan and refactor-apply via ensureReceiverName; coverage stays in TestRefactorPlan_Fix_UnnamedReceiverGetsName.
Round 9 surfaced 4 findings; all addressed: Critical (silent behavior change): #1 [refactor-apply] If-guard body accepted bare `break`, but wfctlhelpers.ApplyPlan records the error and KEEPS processing later actions. A `break` would silently change loop semantics on rewrite. Now only `continue` is accepted in if-guard bodies. #2 [refactor-apply] Driver-method allowlist accepted `Driver` / `DriverFor` alongside `ResourceDriver`. wfctlhelpers dispatches SPECIFICALLY through IaCProvider.ResourceDriver; a wrapper like `provider.Driver(...)` would have its caching/instrumentation bypassed. Restricted to `ResourceDriver` only. Important (false positives / cross-file alias mismatch): #3 [add-validate-plan, lint] Receiver-kind enforcement was too strict. Per Go spec, `*T`'s method set includes BOTH pointer-receiver and value-receiver methods of T. So a value-receiver ValidatePlan on a pointer-receiver provider IS valid (satisfies ProviderValidator). hasValidatePlanMethod now only requires strict matching when the provider uses VALUE receivers (T's method set excludes *T methods). #4 [add-validate-plan] When the qualifier was derived from a sibling method's aliased import (e.g. `iface "github.com/.../interfaces"`), the post-loop import injection used unaliased `ensureImport`, leaving the stub's `iface.IaCPlan` referring to undefined `iface`. Added ensureImportAs helper; now the import alias matches the stub's qualifier. Smoke-tested against DO plugin: refactor-plan / refactor-apply still report DOProvider.Plan canonical / DOProvider.Apply upsert-recovery with stable upsertSupporter suggestion. Output matches T8.7 baseline.
Round 10 surfaced 8 findings; all addressed: Critical (cross-file duplicate stub / silent override): #1 [add-validate-plan] Cross-file duplicate stub injection: when type is in file_a and Plan/Apply are in file_b, both files classified as missing-ValidatePlan and -fix injected duplicate stubs. Now only inject in the file containing the receiver TypeSpec (`if ts == nil { skip }`); the type-file's own pass handles it. #2 [add-validate-plan] Embedded-field promoted ValidatePlan not detected; -fix would shadow it with a no-op stub, silently dropping real plan diagnostics. Added typeHasEmbeddedFields: if the receiver type has any embedded fields, suppress the missing classification (we can't statically resolve method promotion without full type info, so err on the side of NOT injecting). #3 [lint] AssertProviderImplementsValidatePlan — same fix as #2. #4 [refactor-apply] ProviderID/Name/Type assignment-target whitelist didn't check struct identity. `audit.Type = ...` or `result.ProviderID = ...` (wrong struct) classified as canonical and dropped on rewrite. Now requires the LHS receiver to be `ref` (the canonical ResourceRef construction site name). Important (perf / determinism / lint precision): #5 [lint] O(n²) lintFile re-parsed every sibling per-call. Added lintDirCache: lintPath now groups files by directory and builds one parse cache per dir, reused across the directory's files. Per-call fallback retained for single-file invocation. #6 [refactor-plan] planLikeProviderMethodsInDir's dominant-package selection used range-over-map (random iteration), so on a package-count tie the dominant could differ across runs and rewrite against the wrong method set. Sort the package names so tie-break is lexicographic-first (deterministic). #7 [lint] AssertPlanDelegatesToHelper accepted ANY platform.ComputePlan call ANYWHERE in the body. Now requires the canonical SHAPE: either the 2-statement rev2 form (matches isAlreadyDelegatedPlanBody) OR a single-statement legacy `return <X>.Plan(...)` / `return <X>.ComputePlan(...)`. Bespoke wrappers that call the helper as an intermediate step now correctly flag. #8 [lint] AssertApplyDelegatesToHelper — same fix: now uses isAlreadyDelegatedApplyBody (the rewriter's idempotency check) so anything but the canonical single-statement form flags. Smoke-tested against DO plugin: refactor-plan / refactor-apply still report DOProvider.Plan canonical / DOProvider.Apply upsert-recovery with stable upsertSupporter suggestion. Output matches T8.7 baseline.
Round 11 surfaced 6 findings; all addressed: Critical (broken-output false-clean / mode clobber): #1 [lint] planBodyDelegatesCanonically accepted single-statement `return platform.ComputePlan(...)` (the BROKEN rev1 form, uncompilable due to value/pointer mismatch). Lint reported partially-migrated providers as clean, so migrate-providers silently missed them. Now ONLY the canonical 2-statement rev2 form OR legacy `return wfctlhelpers.Plan(...)` is accepted; the broken single-statement platform form falls through to non-canonical so lint surfaces the still-needs-fixup state. #2 [refactor-plan] writeFileAtomic left the temp file at os.CreateTemp's default 0600 mode; rename clobbered the source's original permissions (e.g., 0644 → 0600). Added writeFileAtomicBytesPreserveMode: captures original mode via os.Stat and chmods the temp file before rename. #5 [add-validate-plan] Same 0600 mode-clobber bug in writeFileAtomicBytes. Now delegates to writeFileAtomicBytesPreserveMode. Important (revert + comment polish): #3 [add-validate-plan] Round-10 #2's "any embedded field suppresses missing-ValidatePlan" was too broad — sync.Mutex, loggers, config mixins don't promote ValidatePlan, so real targets were silently missed. Reverted: report missing unconditionally. Maintainers whose providers actually promote ValidatePlan suppress with the explicit `// wfctl:skip-iac-codemod` marker. #4 [lint] AssertProviderImplementsValidatePlan — same revert as #3. #6 [refactor-plan] Stale enum comment for planAlreadyDelegated still referenced `wfctlhelpers.Plan` as the recognised shape; actual implementation recognises the 2-statement platform.ComputePlan form. Comment updated. Removed dead typeHasEmbeddedFields helper (both call sites reverted in #3/#4). Source-file mode preservation verified end-to-end: chmod 0644 → -fix → stat shows 0644 retained. Smoke-tested against DO plugin: refactor-plan / refactor-apply still report DOProvider.Plan canonical / DOProvider.Apply upsert-recovery with stable upsertSupporter suggestion. Output matches T8.7 baseline.
…tening findings Round 12 surfaced 8 findings; all addressed: Critical (CLI bug + silent rewrite of wrong file): #1 [main.go] Top-level dispatcher used a single FlagSet with only -dry-run and -fix registered, so any mode-specific flag (e.g. refactor-apply's -report-file) failed with "flag provided but not defined" BEFORE the mode could parse it. -report-file was documented but UNUSABLE from the CLI entrypoint. Replaced stdlib FlagSet with a manual-scan loop in run(): -dry-run/-fix are extracted; everything else (including unknown flags) flows through to the mode's own FlagSet. Bonus: flag-position flexibility (`/path -fix` now works), updated test + usage text accordingly. #2 [refactor-plan] Walked every .go file but built provs/typeDocs from only the dominant package. Mixed-package or build-tagged directories: a non-dominant file with overlapping receiver names was processed against another package's method set, rewriting the wrong file. Added dominantPackageForDir; each file processor now skips files in non-dominant packages. #3 [refactor-apply] Same fix as #2. #4 [add-validate-plan] Same fix as #2. Important (canonical-detection precision): #5 [refactor-plan] isPlanActionsAppendAssign didn't validate the appended action's payload — `plan.Actions = append(plan.Actions, PlanAction{Action: "queue"})` was misclassified as canonical and silently rewritten. Added `expectedAction` parameter; create branch requires `Action: "create"` and update branch requires `Action: "update"`. #6 [refactor-apply] hasCanonicalCases verified case labels but not that the body's driver call MATCHED the label. A `case "create"` body that called `.Update()` or `.Delete()` was misclassified and silently rewritten away. Added caseBodyMatchesLabel: scans each case body for driver method calls and verifies the label- to-method mapping (create→Create, update→Update, delete→Delete, replace→Update). #7 [refactor-apply] Driver-lookup check accepted any `<recv>.ResourceDriver(<arg>)` regardless of <arg>. wfctlhelpers always dispatches with `action.Resource.Type`, so providers using a different lookup key (e.g. action.Tag, computed value) would see different driver behavior on rewrite. Now requires the lookup key to be exactly `action.Resource.Type`. #8 [lint] looksLikeProvider checked method NAMES + rough arity, so any unrelated type with `Plan(...)` and `Apply(...)` was treated as a provider (e.g., a deploy strategy). Tightened to verify signature shapes via type-name suffix matching: Plan must be `Plan(ctx, []ResourceSpec, []ResourceState) (*IaCPlan, error)` and Apply must be `Apply(ctx, *IaCPlan) (*ApplyResult, error)`. Qualified or unqualified accepted via typeNameTailMatches. Smoke-tested: - `iac-codemod refactor-apply -report-file <path> <dir>` now works (previously: "flag provided but not defined") - DO plugin still reports DOProvider.Plan canonical / Apply upsert-recovery with stable upsertSupporter suggestion (T8.7 baseline preserved)
Comment on lines
+356
to
+360
| if class == validatePlanMissing && opts != nil && opts.Fix { | ||
| // Round-10 #1: only inject the stub in the file that | ||
| // contains the receiver TYPE declaration. When the type is | ||
| // in a sibling file (`ts == nil` here because it wasn't | ||
| // found in the local file's typeDecls), skip injection; |
Comment on lines
+920
to
+931
| expected := map[string]string{ | ||
| "create": "Create", | ||
| "update": "Update", | ||
| "delete": "Delete", | ||
| "replace": "Update", // wfctlhelpers' doReplace internally uses Delete+Create | ||
| }[label] | ||
| if expected == "" { | ||
| continue | ||
| } | ||
| if !calledMethods[expected] { | ||
| return false | ||
| } |
Comment on lines
+942
to
+965
| func isCanonicalDefaultBody(body []ast.Stmt) bool { | ||
| if len(body) != 1 { | ||
| return false | ||
| } | ||
| a, ok := body[0].(*ast.AssignStmt) | ||
| if !ok || a.Tok != token.ASSIGN || len(a.Lhs) != 1 || len(a.Rhs) != 1 { | ||
| return false | ||
| } | ||
| if id, ok := a.Lhs[0].(*ast.Ident); !ok || id.Name != "err" { | ||
| return false | ||
| } | ||
| call, ok := a.Rhs[0].(*ast.CallExpr) | ||
| if !ok { | ||
| return false | ||
| } | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return false | ||
| } | ||
| x, ok := sel.X.(*ast.Ident) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return x.Name == "fmt" && sel.Sel.Name == "Errorf" |
This was referenced May 5, 2026
This was referenced May 6, 2026
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
W-8 of the 12-PR autonomous IaC conformance plan. Adds
cmd/iac-codemod/— an AST-based migration tool for IaC plugin providers — with 4 modes plus a workspace-wide migration target in the Makefile.Modes
lintAssertPlanDelegatesToHelper,AssertApplyDelegatesToHelper,AssertDiffSetsNeedsReplaceForForceNew,AssertProviderImplementsValidatePlan.refactor-planreturn platform.ComputePlan(ctx, p, desired, current)(the real helper atplatform/differ.go:72). Aborts with informative report on out-of-template logic.refactor-applyreturn wfctlhelpers.ApplyPlan(ctx, p, plan). Non-canonical idioms (DO upsert recovery / AWS update+replace collapse / custom error wrapping) report only with hand-port suggestion.add-validate-planValidatePlan; appends a no-op stub on-fix. Stub signature respects the file's interfaces import qualifier.All 4 modes:
-dry-run(mutation requires explicit-fix).// wfctl:skip-iac-codemodat function doc, type doc, AND wrapping GenDecl doc.Verification (T8.7) against
workflow-plugin-digitaloceanBuilt
/tmp/iac-codemodfrom the final SHA and ran every mode against/Users/jon/workspace/workflow-plugin-digitalocean/. Reports inline:refactor-applymatches the plan's expected DO output: upsert-recovery is correctly identified as non-canonical with theupsertSupporterhook suggested.Scope deviations / incidental findings
Plan rewrite target pivoted from
wfctlhelpers.Plantoplatform.ComputePlan(Copilot review round-1 finding Feature: Workflow UI #1). Plan-doc §T8.3 line 2373 spec'dwfctlhelpers.Planas the rewrite target, but no such helper exists in the repo today. The actual canonical Plan helper isplatform.ComputePlanatplatform/differ.go:72. Pivoted to the real API; the legacy target is also accepted by the lint analyzer for forward-compat. Recurring defect class (plan-literal vs reality gap, W-4/W-5/W-7/W-9/W-8); flagged for retro.Walk filter widened to underscore-prefixed dirs: the original walk filter (
vendor/testdata/ hidden) reported each finding 21x because the DO repo's_worktrees/directories all contain stale checkouts of the same source. Go tooling itself ignores_*packages, so the codemod now matches that.migrate-providersMakefile target usesGOWORK=offso contributors with a workspacego.workfile that doesn't list this module can run the target without amending their environment.Round-1 review fixes (commit
6cd5889)Copilot identified 11 substantive findings on the initial submission; all addressed:
wfctlhelpers.Plan→platform.ComputePlan(target pivot, see Scope Deviations)._).-report-filedocumented in usage (was: hidden from-h).Test plan
GOWORK=off go build ./cmd/iac-codemod/...-race:GOWORK=off go test ./cmd/iac-codemod/... -count=1 -racego vetclean/tmp/T8.x-<sha>worktree._worktrees/and other underscore-prefixed dirs excluded.make migrate-providerstarget verified end-to-end against AWS/GCP/Azure plugin checkouts.